svn commit: r209119 - head/sys/sys

[Lawrence Stewart]

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);


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.

Thoughts?

Cheers,
Lawrence