From owner-svn-src-all@freebsd.org Thu May 5 12:21:51 2016 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id D56A2B2EE4A; Thu, 5 May 2016 12:21:51 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail110.syd.optusnet.com.au (mail110.syd.optusnet.com.au [211.29.132.97]) by mx1.freebsd.org (Postfix) with ESMTP id 7EE4A10FE; Thu, 5 May 2016 12:21:50 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from c122-106-149-109.carlnfd1.nsw.optusnet.com.au (c122-106-149-109.carlnfd1.nsw.optusnet.com.au [122.106.149.109]) by mail110.syd.optusnet.com.au (Postfix) with ESMTPS id DC2EB780A5A; Thu, 5 May 2016 22:21:47 +1000 (AEST) Date: Thu, 5 May 2016 22:21:46 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: John Baldwin cc: Bruce Evans , Pedro Giffuni , "Ngie Cooper (yaneurabeya)" , src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r298933 - in head: share/man/man9 sys/amd64/include sys/dev/acpica sys/dev/drm2 sys/dev/drm2/i915 sys/kern sys/sys sys/x86/acpica sys/x86/x86 In-Reply-To: <2067797.hs1zGgLCXN@ralph.baldwin.cx> Message-ID: <20160505211514.U2142@besplex.bde.org> References: <201605021800.u42I0cjK084243@repo.freebsd.org> <20160504031930.A3395@besplex.bde.org> <1928389.rOu33C1eaq@ralph.baldwin.cx> <2067797.hs1zGgLCXN@ralph.baldwin.cx> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.1 cv=EfU1O6SC c=1 sm=1 tr=0 a=R/f3m204ZbWUO/0rwPSMPw==:117 a=L9H7d07YOLsA:10 a=9cW_t1CCXrUA:10 a=s5jvgZ67dGcA:10 a=kj9zAlcOel0A:10 a=GMFLL_GuBbYjx0O4ks8A:9 a=CjuIK1q_8ugA:10 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.22 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, 05 May 2016 12:21:51 -0000 On Wed, 4 May 2016, John Baldwin wrote: > On Tuesday, May 03, 2016 11:19:44 AM John Baldwin wrote: >> On Wednesday, May 04, 2016 03:58:40 AM Bruce Evans wrote: >> >>> BTW, I don't like select's and bitset's use of longs. Using unsigned >>> for select is a historical mistake. Bitset apparently copied select >>> except it unimproved to signed long. Bitstring uses unsigned char with >>> no optimizations. Sigset uses uint32_t with no obvious optimizations, >>> but compilers do a good job with with it due to its fixed size. I doubt >>> that the manual optimization of using a wider size is important. >> >> I agree, but cpuset_t is already part of the ABI in existing releases. :( >> Changing it to uint32_t would break the ABI for big-endian platforms. > > How about this: It seems reasonable. I didn't check many details. > diff --git a/sys/sys/_bitset.h b/sys/sys/_bitset.h > index 26a8848..89dd7b6 100644 > --- a/sys/sys/_bitset.h > +++ b/sys/sys/_bitset.h > @@ -36,26 +36,15 @@ > * Macros addressing word and bit within it, tuned to make compiler > * optimize cases when SETSIZE fits into single machine word. > */ > -#define _BITSET_BITS (sizeof(long) * NBBY) > +#define _BITSET_BITS (sizeof(long) * 8) > > -#define __bitset_words(_s) (howmany(_s, _BITSET_BITS)) > +#define _howmany(x, y) (((x) + ((y) - 1)) / (y)) > > -#define __bitset_mask(_s, n) \ > - (1L << ((__bitset_words((_s)) == 1) ? \ > - (__size_t)(n) : ((n) % _BITSET_BITS))) > - > -#define __bitset_word(_s, n) \ > - ((__bitset_words((_s)) == 1) ? 0 : ((n) / _BITSET_BITS)) > +#define __bitset_words(_s) (_howmany(_s, _BITSET_BITS)) This still has a bogus underscore on _s. Unfortunately, the problem pointed out by kib does apply here, and in select.h too, not like I said before. Our select implemenation breaks user code like: #include // for its undocumented nested pollution void foo(void) { static void *_howmany(void); } Change it to use upper case instead of adding an underscore. Lower case should indicate a safe (function-like) macro. > > #define BITSET_DEFINE(t, _s) \ > struct t { \ > long __bits[__bitset_words((_s))]; \ > } > > -#define BITSET_T_INITIALIZER(x) \ > - { .__bits = { x } } > - > -#define BITSET_FSET(n) \ > - [ 0 ... ((n) - 1) ] = (-1L) > - I didn't expect you to clean the layering too, but this reminds me that I don't like the layering. Little _foo.h heads may be the cleanest way to reduce namespace pollution, but they are bad for readabilty and efficiency. New header should be designed to be clean from the start and not need multiple layers. For bitsets, we now have _bitset.h, bitset.h, _cpuset.h, cpuset.h and bitstring.h. Since yesterday we have had gibbs' improved bitstring.h. It is too over-engineered for me, but it s now just as optimal as bitset.h (perhaps more). gibbs used bitstrings instead of bitsets since he thought bitsets were too kernel-ish. We still have much larger messes using little headers cleaning time types. This was used mainly to clean sys/stat.h, but that cleaning is mostly broken now, and sys/time.h and time.h remain a mess of pollution. sys/stat.h now uses timespecs unconditionally, but they only exist in POSIX >= 2001. Old versions used __timespec (which was declared in the little headers) in the !_BSD_VISIBLE case. We still have the little headers -- _timespec.h, timespec.h and _timeval.h. _timespec existed solely to declare struct __timespec and timespec.h existed solely to declare struct timespec and some pollution. Now _timespec.h exists solely to declare struct timespec, and timespec.h grew more pollution. _timeval.h always declared solely to declare struct timeval and associated types. The little time headers do serve the purpose providing a single place to declare 1 struct with minor collateral pollution, but since the struct is so simple it is better to repeat its definition (ifdefed). > diff --git a/sys/sys/bitset.h b/sys/sys/bitset.h > index d130522..f1c7bf8 100644 > --- a/sys/sys/bitset.h > +++ b/sys/sys/bitset.h > @@ -32,6 +32,13 @@ > #ifndef _SYS_BITSET_H_ > #define _SYS_BITSET_H_ > > +#define __bitset_mask(_s, n) \ > + (1L << ((__bitset_words((_s)) == 1) ? \ > + (__size_t)(n) : ((n) % _BITSET_BITS))) > + The longs should be u_longs. Compilers would warn about shifting this 1L the sign bit. Maybe the expression is too complicated for them to see that. Bruce