svn commit: r280785 - in head/sys: kern netgraph/atm/sscop netgraph/atm/uni sys
John Baldwin
jhb at freebsd.org
Mon Mar 30 16:40:24 UTC 2015
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.
> 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)
> + */
> #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. :)
> #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
More information about the svn-src-all
mailing list