From owner-svn-src-head@FreeBSD.ORG Mon Mar 30 22:23:17 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 8831B6ED for ; Mon, 30 Mar 2015 22:23:17 +0000 (UTC) Received: from mail-pd0-x22a.google.com (mail-pd0-x22a.google.com [IPv6:2607:f8b0:400e:c02::22a]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 47310EA for ; Mon, 30 Mar 2015 22:23:17 +0000 (UTC) Received: by pdbni2 with SMTP id ni2so188999819pdb.1 for ; Mon, 30 Mar 2015 15:23:16 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=netflix.com; s=google; h=content-type:mime-version:subject:from:in-reply-to:date:cc :message-id:references:to; bh=QuY9P2zLBA/2ZIYVF1E46vP1N3YZa6JUyZs+8dtusWM=; b=C2BBdSoqWJ0pjyuSzhY5jv7QVqz21HkbEEdkFXmt1B/+q5lJmTEIYK/jbQoesnUCN+ pUhMTpzkAuQRAh86JBj03i5ApnkiaRZhQw0B0ZnrRhODxkS1WT2U0eb7eBZTioLEDewJ M9IFkEiQyOKfNPMihkv6qDGW6YUgFh/2I9vRk= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:content-type:mime-version:subject:from :in-reply-to:date:cc:message-id:references:to; bh=QuY9P2zLBA/2ZIYVF1E46vP1N3YZa6JUyZs+8dtusWM=; b=mnxXAC7dm/Ya8iXbsfuz3+IOGeYnlWHuvqYz0CMKXp7hLY3i8a+mzFzYcGInpOidsV vRfbwuN+UkGZnm9E3ZP8OI33mmY/Nwi3wu90PRWmi6tLAe5Gid5n04/gB6bylPEXpHJA QRy3iQXiLo8TuvTsME1Z1tToy8zqlNcHIg8dESFzk0jMt/dIii64k9SA/90X1zdI76oP zNeticZgtPya8/4wC37VMmeXfycpBfOtGJCWPIY9HfB07vozZjoU94rd+7eQs94dRr0S /r1DUu2BgkMelkzEGNZuJRwguIzOu7iApZTY05GJ7VqWejI5A4KPUH5IwEoxOrTgGFGX x9ow== X-Gm-Message-State: ALoCoQmpvfgkuhHqZvfeT5koWe3rgq9yCLJhOFXGR4zbiqp3nfgx8gnt6SQyIz3NsukxWZgAy3B5 X-Received: by 10.68.69.103 with SMTP id d7mr18461957pbu.145.1427754196826; Mon, 30 Mar 2015 15:23:16 -0700 (PDT) Received: from lgml-rrs.corp.netflix.com ([69.53.237.72]) by mx.google.com with ESMTPSA id x3sm11732117pdo.0.2015.03.30.15.23.15 (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Mon, 30 Mar 2015 15:23:15 -0700 (PDT) Mime-Version: 1.0 (Mac OS X Mail 7.3 \(1878.6\)) Subject: Re: svn commit: r280785 - in head/sys: kern netgraph/atm/sscop netgraph/atm/uni sys From: Randall Stewart In-Reply-To: <32487399.PTq7ESkWJT@ralph.baldwin.cx> Date: Mon, 30 Mar 2015 16:23:15 -0600 Message-Id: References: <201503281250.t2SCoOkt020297@svn.freebsd.org> <32487399.PTq7ESkWJT@ralph.baldwin.cx> To: John Baldwin X-Mailer: Apple Mail (2.1878.6) Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable X-Content-Filtered-By: Mailman/MimeDel 2.1.18-1 Cc: svn-src-head@freebsd.org, Randall Stewart , 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 22:23:17 -0000 John: Comments below.. On Mar 30, 2015, at 9:16 AM, John Baldwin 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 >>=20 >> 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). >>=20 >> 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. >=20 > Please use NOCPU rather than -1 directly for the CPU field when not > moving a callout. ok, did not no a =93NOCPU=94 was defined .. thanks.. >=20 >> Modified: head/sys/sys/callout.h >> = =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D >> --- 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 { >> }; >>=20 >> #ifdef _KERNEL >> +/*=20 >> + * 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=20 >> + * c_flags field directly but should use the macros! >> + * =20 >> + * If the caller wants to keep the c_flags field sane they=20 >> + * should init with a mutex *or* if using the older >> + * mpsafe option, they *must* lock there own lock >> + * before calling callout_deactivate(). >=20 > Some wording suggestions: >=20 > "is actually" -> "is split across" >=20 > 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: >=20 > "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." >=20 > (Also, please use double spaces after periods) ok I will commit that with my split to short/short. >=20 >> + */ >> #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) >=20 > 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. :) >=20 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 &=3D = ~CALLOUT_ACTIVE) >> #define callout_drain(c) _callout_stop_safe(c, 1) >> void callout_init(struct callout *, int); >=20 > --=20 > John Baldwin -------- Randall Stewart rrs@netflix.com 803-317-4952