From owner-svn-src-head@FreeBSD.ORG Thu Jun 17 07:13:15 2010 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 2DB491065673; Thu, 17 Jun 2010 07:13:15 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from mail.zoral.com.ua (mx0.zoral.com.ua [91.193.166.200]) by mx1.freebsd.org (Postfix) with ESMTP id 83E058FC32; Thu, 17 Jun 2010 07:13:14 +0000 (UTC) Received: from deviant.kiev.zoral.com.ua (root@deviant.kiev.zoral.com.ua [10.1.1.148]) by mail.zoral.com.ua (8.14.2/8.14.2) with ESMTP id o5H7D0ve064840 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Thu, 17 Jun 2010 10:13:00 +0300 (EEST) (envelope-from kostikbel@gmail.com) Received: from deviant.kiev.zoral.com.ua (kostik@localhost [127.0.0.1]) by deviant.kiev.zoral.com.ua (8.14.4/8.14.4) with ESMTP id o5H7D0pY011141; Thu, 17 Jun 2010 10:13:00 +0300 (EEST) (envelope-from kostikbel@gmail.com) Received: (from kostik@localhost) by deviant.kiev.zoral.com.ua (8.14.4/8.14.4/Submit) id o5H7D0nX011140; Thu, 17 Jun 2010 10:13:00 +0300 (EEST) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: deviant.kiev.zoral.com.ua: kostik set sender to kostikbel@gmail.com using -f Date: Thu, 17 Jun 2010 10:13:00 +0300 From: Kostik Belousov To: Lawrence Stewart Message-ID: <20100617071300.GX13238@deviant.kiev.zoral.com.ua> References: <201006130239.o5D2du3m086332@svn.freebsd.org> <20100613101025.GD1320@garage.freebsd.pl> <4C158B71.205@freebsd.org> <20100614085205.GD13238@deviant.kiev.zoral.com.ua> <4C1605A7.2000202@freebsd.org> <20100614104349.GF13238@deviant.kiev.zoral.com.ua> <4C198A90.3060905@freebsd.org> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="wrIrAujhz1F5gxq3" Content-Disposition: inline In-Reply-To: <4C198A90.3060905@freebsd.org> User-Agent: Mutt/1.4.2.3i X-Virus-Scanned: clamav-milter 0.95.2 at skuns.kiev.zoral.com.ua X-Virus-Status: Clean X-Spam-Status: No, score=-2.3 required=5.0 tests=ALL_TRUSTED,AWL,BAYES_50, DNS_FROM_OPENWHOIS autolearn=no version=3.2.5 X-Spam-Checker-Version: SpamAssassin 3.2.5 (2008-06-10) on skuns.kiev.zoral.com.ua Cc: Matthew Fleming , src-committers@freebsd.org, Pawel Jakub Dawidek , John Baldwin , svn-src-all@freebsd.org, brde@optusnet.com.au, svn-src-head@freebsd.org Subject: Re: svn commit: r209119 - head/sys/sys X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.5 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: Thu, 17 Jun 2010 07:13:15 -0000 --wrIrAujhz1F5gxq3 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Jun 17, 2010 at 12:38:08PM +1000, Lawrence Stewart wrote: > On 06/14/10 20:43, Kostik Belousov wrote: > >On Mon, Jun 14, 2010 at 08:34:15PM +1000, Lawrence Stewart wrote: > >>On 06/14/10 18:52, Kostik Belousov wrote: > >>>On Mon, Jun 14, 2010 at 11:52:49AM +1000, Lawrence Stewart wrote: > >>>>On 06/13/10 20:10, Pawel Jakub Dawidek wrote: > >>>>>On Sun, Jun 13, 2010 at 02:39:55AM +0000, Lawrence Stewart wrote: > >>>>[snip] > >>>>>> > >>>>>>Modified: head/sys/sys/pcpu.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/pcpu.h Sun Jun 13 01:27:29 2010 (r209118) > >>>>>>+++ head/sys/sys/pcpu.h Sun Jun 13 02:39:55 2010 (r209119) > >>>>>>@@ -106,6 +106,17 @@ extern uintptr_t dpcpu_off[]; > >>>>>> #define DPCPU_ID_GET(i, n) (*DPCPU_ID_PTR(i, n)) > >>>>>> #define DPCPU_ID_SET(i, n, v) (*DPCPU_ID_PTR(i, n) =3D v) > >>>>>> > >>>>>>+/* > >>>>>>+ * Utility macros. > >>>>>>+ */ > >>>>>>+#define DPCPU_SUM(n, var, sum) \ > >>>>>>+do { \ > >>>>>>+ (sum) =3D 0; \ > >>>>>>+ u_int i; \ > >>>>>>+ CPU_FOREACH(i) \ > >>>>>>+ (sum) +=3D (DPCPU_ID_PTR(i, n))->var; \ > >>>>>>+} while (0) > >>>>> > >>>>>I'd suggest first swapping variable declaration and '(sum) =3D 0;'. > >>>>>Also using 'i' as a counter in macro can easly lead to name collisio= n. > >>>>>If you need to do it, I'd suggest '_i' or something. > >>>> > >>>>Given that the DPCPU variable name space is flat and variable names h= ave > >>>>to be unique, perhaps something like the following would address the > >>>>concerns raised? > >>>> > >>>>#define DPCPU_SUM(n, var, sum) = \ > >>>>do { = \ > >>>> u_int _##n##_i; = =20 > >>>> \ > >>>> (sum) =3D 0; = =20 > >>>> \ > >>>> CPU_FOREACH(_##n##_i) = =20 > >>>> \ > >>>> (sum) +=3D (DPCPU_ID_PTR(_##n##_i, n))->var; = =20 > >>>> \ > >>>>} while (0) > >>> > >>>You do not have to jump through this. Mostly by convention, in our ker= nel > >>>sources, names with "_" prefix are reserved for the infrastructure=20 > >>>(cannot > >>>say implementation). I think it is quite safe to use _i for the iterat= ion > >>>variable. > >>> > >>>As an example of this, look at sys/sys/mount.h, implementation of > >>>VFS_NEEDGIANT, VFS_LOCK_GIANT etc macros. They do use gcc ({}) extensi= on > >>>to provide function-like macros, but this is irrelevant. Or, look at > >>>the VFS_ASSERT_GIANT that is exactly like what you need. > >> > >>Ok cool, thanks for the info and pointers (I didn't know about the ({}) > >>extension or that "_" prefix was definitely reserved). I'm happy to use > >>_i. Does the following diff against head look suitable to commit? > >> > >>--- a/sys/sys/pcpu.h Sun Jun 13 02:39:55 2010 +0000 > >>+++ b/sys/sys/pcpu.h Mon Jun 14 20:12:27 2010 +1000 > >>@@ -111,10 +111,10 @@ > >> */ > >> #define DPCPU_SUM(n, var, sum) = \ > >> do { = \ > >>+ u_int _i; \ > >> (sum) =3D 0; = \ > >>- u_int i; \ > >>- CPU_FOREACH(i) \ > >>- (sum) +=3D (DPCPU_ID_PTR(i, n))->var; = \ > >>+ CPU_FOREACH(_i) \ > >>+ (sum) +=3D (DPCPU_ID_PTR(_i, n))->var; = \ > >> } while (0) > > > >You might want to introduce local accumulator to prevent several=20 > >evaluations > >of sum, to avoid possible side-effects. Then, after, the loop, do single > >asignment to the the sum. > > > >Or, you could ditch the sum at all, indeed using ({}) and returning the > >result. __typeof is your friend to select proper type of accumulator. >=20 > So, something like this? >=20 > #define DPCPU_SUM(n, var) __extension__ \ > ({ \ > u_int _i; \ > __typeof((DPCPU_PTR(n))->var) sum; \ > \ > sum =3D 0; \ > CPU_FOREACH(_i) { \ > sum +=3D (DPCPU_ID_PTR(_i, n))->var; \ > } \ > sum; \ > }) >=20 > Which can be used like this: >=20 > totalss.n_in =3D DPCPU_SUM(ss, n_in); Yes, exactly. >=20 >=20 > I've tested the above and it works. I also prefer the idea of having=20 > DPCPU_SUM return the sum so that you can do "var =3D DPCPU_SUM(...)". My= =20 > only concern with this method is that the caller no longer has the=20 > choice to make the sum variable a larger type to avoid overflow. It=20 > would be nice to be able to have the DPCPU vars be uint32_t but be able= =20 > to sum them into a uint64_t accumulator for example. Perhaps this isn't= =20 > really an issue though... I'm not sure. You are worried about overflow in the sum of 32 or 64 variables, but if this is the case, then each member of the sum can overflow as well, IMO. Either ignore the issue, or use a uintmax_t. --wrIrAujhz1F5gxq3 Content-Type: application/pgp-signature Content-Disposition: inline -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.10 (FreeBSD) iEYEARECAAYFAkwZyvwACgkQC3+MBN1Mb4hCygCggVYn/S5PXUgEhI/9Kgsdy7gp 5WwAnjK91AIc6FSJLUPctT7CFkRb5V0t =nbCD -----END PGP SIGNATURE----- --wrIrAujhz1F5gxq3--