From owner-svn-src-all@FreeBSD.ORG Thu Jun 17 02:38:11 2010 Return-Path: Delivered-To: svn-src-all@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 1D7BA106566C; Thu, 17 Jun 2010 02:38:11 +0000 (UTC) (envelope-from lstewart@freebsd.org) Received: from lauren.room52.net (lauren.room52.net [210.50.193.198]) by mx1.freebsd.org (Postfix) with ESMTP id 703B78FC29; Thu, 17 Jun 2010 02:38:10 +0000 (UTC) Received: from lawrence1.loshell.room52.net (unknown [59.167.184.191]) by lauren.room52.net (Postfix) with ESMTPSA id ABAA77E853; Thu, 17 Jun 2010 12:38:08 +1000 (EST) Message-ID: <4C198A90.3060905@freebsd.org> Date: Thu, 17 Jun 2010 12:38:08 +1000 From: Lawrence Stewart User-Agent: Mozilla/5.0 (X11; U; FreeBSD amd64; en-AU; rv:1.9.1.9) Gecko/20100405 Thunderbird/3.0.4 MIME-Version: 1.0 To: Kostik Belousov 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> In-Reply-To: <20100614104349.GF13238@deviant.kiev.zoral.com.ua> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit 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-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 17 Jun 2010 02:38:11 -0000 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