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 > >>>>============================================================================== > >>>>--- 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) = v) > >>>> > >>>>+/* > >>>>+ * Utility macros. > >>>>+ */ > >>>>+#define DPCPU_SUM(n, var, sum) \ > >>>>+do { \ > >>>>+ (sum) = 0; \ > >>>>+ u_int i; \ > >>>>+ CPU_FOREACH(i) \ > >>>>+ (sum) += (DPCPU_ID_PTR(i, n))->var; \ > >>>>+} while (0) > >>> > >>>I'd suggest first swapping variable declaration and '(sum) = 0;'. > >>>Also using 'i' as a counter in macro can easly lead to name collision. > >>>If you need to do it, I'd suggest '_i' or something. > >> > >>Given that the DPCPU variable name space is flat and variable names have > >>to be unique, perhaps something like the following would address the > >>concerns raised? > >> > >>#define DPCPU_SUM(n, var, sum) \ > >>do { \ > >> u_int _##n##_i; \ > >> (sum) = 0; \ > >> CPU_FOREACH(_##n##_i) \ > >> (sum) += (DPCPU_ID_PTR(_##n##_i, n))->var; \ > >>} while (0) > > > >You do not have to jump through this. Mostly by convention, in our kernel > >sources, names with "_" prefix are reserved for the infrastructure (cannot > >say implementation). I think it is quite safe to use _i for the iteration > >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 ({}) extension > >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) = 0; \ > - u_int i; \ > - CPU_FOREACH(i) \ > - (sum) += (DPCPU_ID_PTR(i, n))->var; \ > + CPU_FOREACH(_i) \ > + (sum) += (DPCPU_ID_PTR(_i, n))->var; \ > } while (0)
You might want to introduce local accumulator to prevent several 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.
pgp7KqTFmqfH8.pgp
Description: PGP signature