From owner-svn-src-head@FreeBSD.ORG Mon Mar 30 16:40:24 2015 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id AEDC1F8A; Mon, 30 Mar 2015 16:40:24 +0000 (UTC) Received: from bigwig.baldwin.cx (bigwig.baldwin.cx [IPv6:2001:470:1f11:75::1]) (using TLSv1 with cipher DHE-RSA-CAMELLIA256-SHA (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 8600FD13; Mon, 30 Mar 2015 16:40:24 +0000 (UTC) Received: from ralph.baldwin.cx (pool-173-54-116-245.nwrknj.fios.verizon.net [173.54.116.245]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id 92F15B9C6; Mon, 30 Mar 2015 12:40:23 -0400 (EDT) From: John Baldwin To: Randall Stewart Subject: Re: svn commit: r280785 - in head/sys: kern netgraph/atm/sscop netgraph/atm/uni sys Date: Mon, 30 Mar 2015 11:16:20 -0400 Message-ID: <32487399.PTq7ESkWJT@ralph.baldwin.cx> User-Agent: KMail/4.14.2 (FreeBSD/10.1-STABLE; KDE/4.14.2; amd64; ; ) In-Reply-To: <201503281250.t2SCoOkt020297@svn.freebsd.org> References: <201503281250.t2SCoOkt020297@svn.freebsd.org> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.7 (bigwig.baldwin.cx); Mon, 30 Mar 2015 12:40:23 -0400 (EDT) Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 30 Mar 2015 16:40:24 -0000 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