svn commit: r280785 - in head/sys: kern netgraph/atm/sscop netgraph/atm/uni sys

[Randall Stewart]

John:

Comments below..


On Mar 30, 2015, at 9:16 AM, John Baldwin <jhb at 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
rrs at netflix.com
803-317-4952