Date: Thu, 17 Jun 2010 10:13:00 +0300 From: Kostik Belousov <kostikbel@gmail.com> To: Lawrence Stewart <lstewart@freebsd.org> Cc: Matthew Fleming <mdf@freebsd.org>, src-committers@freebsd.org, Pawel Jakub Dawidek <pjd@freebsd.org>, John Baldwin <jhb@freebsd.org>, svn-src-all@freebsd.org, brde@optusnet.com.au, svn-src-head@freebsd.org Subject: Re: svn commit: r209119 - head/sys/sys Message-ID: <20100617071300.GX13238@deviant.kiev.zoral.com.ua> In-Reply-To: <4C198A90.3060905@freebsd.org> 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>
next in thread | previous in thread | raw e-mail | index | archive | help
--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--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20100617071300.GX13238>