John: Comments below..
On Mar 30, 2015, at 9:16 AM, John Baldwin <j...@freebsd.org> wrote: > On Saturday, March 28, 2015 12:50:24 PM Randall Stewart wrote: >> Author: rrs >> Date: Sat Mar 28 12:50:24 2015 >> New Revision: 280785 >> URL: https://svnweb.freebsd.org/changeset/base/280785 >> >> Log: >> Change the callout to supply -1 to indicate we are not changing >> CPU, also add protection against invalid CPU's as well as >> split c_flags and c_iflags so that if a user plays with the active >> flag (the one expected to be played with by callers in MPSAFE) without >> a lock, it won't adversely affect the callout system by causing a corrupt >> list. This also means that all callers need to use the macros and *not* >> play with the falgs directly (like netgraph used to). >> >> Differential Revision: htts://reviews.freebsd.org/D1894 >> Reviewed by: .. timed out but looked at by jhb, imp, adrian hselasky >> tested by hiren and netflix. >> Sponsored by: Netflix Inc. > > Please use NOCPU rather than -1 directly for the CPU field when not > moving a callout. ok, did not no a “NOCPU” was defined .. thanks.. > >> Modified: head/sys/sys/callout.h >> ============================================================================== >> --- head/sys/sys/callout.h Sat Mar 28 12:23:15 2015 (r280784) >> +++ head/sys/sys/callout.h Sat Mar 28 12:50:24 2015 (r280785) >> @@ -63,8 +63,23 @@ struct callout_handle { >> }; >> >> #ifdef _KERNEL >> +/* >> + * Note the flags field is actually *two* fields. The c_flags >> + * field is the one that caller operations that may, or may not have >> + * a lock touches i.e. callout_deactivate(). The other, the c_iflags, >> + * is the internal flags that *must* be kept correct on which the >> + * callout system depend on i.e. callout_migrating() & callout_pending(), >> + * these are used internally by the callout system to determine which >> + * list and other critical internal state. Callers *should not* use the >> + * c_flags field directly but should use the macros! >> + * >> + * If the caller wants to keep the c_flags field sane they >> + * should init with a mutex *or* if using the older >> + * mpsafe option, they *must* lock there own lock >> + * before calling callout_deactivate(). > > Some wording suggestions: > > "is actually" -> "is split across" > > The second sentence quite seem to be English ("have a lock touches" > which I think means "hold a lock while touching" or some such), but > you can perhaps use this for the rest of the comment: > > "The c_iflags field holds internal flags that are protected by internal > locks of the callout subsystem. The c_flags field holds external flags. > The caller must hold its own lock while manipulating or reading external > flags via callout_active(), callout_deactivate(), callout_reset*(), or > callout_stop() to avoid races." > > (Also, please use double spaces after periods) ok I will commit that with my split to short/short. > >> + */ >> #define callout_active(c) ((c)->c_flags & CALLOUT_ACTIVE) >> -#define callout_migrating(c) ((c)->c_flags & CALLOUT_DFRMIGRATION) >> +#define callout_migrating(c) ((c)->c_iflags & CALLOUT_DFRMIGRATION) > > I would just move this into the C file. It isn't useful outside of the > implementation as far as I know. This then avoids having to explain to > users that they shouldn't see it in the block comment since it would no > longer be there. :) > Ok that sounds fine, I too doubt it would be used outside the kern_timeout.c file ;-) R >> #define callout_deactivate(c) ((c)->c_flags &= ~CALLOUT_ACTIVE) >> #define callout_drain(c) _callout_stop_safe(c, 1) >> void callout_init(struct callout *, int); > > -- > John Baldwin -------- Randall Stewart r...@netflix.com 803-317-4952 _______________________________________________ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
