svn commit: r209119 - head/sys/sys

Kostik Belousov kostikbel at gmail.com
Thu Jun 17 07:13:15 UTC 2010


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
> >>>>>>==============================================================================
> >>>>>>--- 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.
> 
> So, something like this?
> 
> #define DPCPU_SUM(n, var) __extension__                                \
> ({                                                                     \
>         u_int _i;                                                      \
>         __typeof((DPCPU_PTR(n))->var) sum;                             \
>                                                                        \
>         sum = 0;                                                       \
>         CPU_FOREACH(_i) {                                              \
>                 sum += (DPCPU_ID_PTR(_i, n))->var;                     \
>         }                                                              \
>         sum;                                                           \
> })
> 
> Which can be used like this:
> 
> totalss.n_in = DPCPU_SUM(ss, n_in);
Yes, exactly.

> 
> 
> I've tested the above and it works. I also prefer the idea of having 
> DPCPU_SUM return the sum so that you can do "var = DPCPU_SUM(...)". My 
> only concern with this method is that the caller no longer has the 
> choice to make the sum variable a larger type to avoid overflow. It 
> would be nice to be able to have the DPCPU vars be uint32_t but be able 
> to sum them into a uint64_t accumulator for example. Perhaps this isn't 
> 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 196 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/svn-src-all/attachments/20100617/77874b13/attachment.pgp


More information about the svn-src-all mailing list