Date: Mon, 9 May 2011 01:42:28 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Attilio Rao <attilio@freebsd.org> Cc: svn-src-projects@freebsd.org, src-committers@freebsd.org, Bruce Evans <brde@optusnet.com.au> Subject: Re: svn commit: r221614 - projects/largeSMP/sys/powerpc/include Message-ID: <20110509003256.F2061@besplex.bde.org> In-Reply-To: <BANLkTi=mnZc6fy8TGpLMsMLDV8UpK5J-Gw@mail.gmail.com> References: <201105080039.p480doiZ021493@svn.freebsd.org> <20110508202725.K1192@besplex.bde.org> <BANLkTi=mnZc6fy8TGpLMsMLDV8UpK5J-Gw@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
This message is in MIME format. The first part should be readable text, while the remaining parts are likely unreadable without MIME-aware tools. --0-2138985585-1304869348=:2061 Content-Type: TEXT/PLAIN; charset=X-UNKNOWN; format=flowed Content-Transfer-Encoding: QUOTED-PRINTABLE On Sun, 8 May 2011, Attilio Rao wrote: > 2011/5/8 Bruce Evans <brde@optusnet.com.au>: >> On Sun, 8 May 2011, Attilio Rao wrote: >> >>> Log: >>> =C2=A0All architectures define the size-bounded types (uint32_t, uint64= _t, >>> etc.) >>> =C2=A0starting from base C types (int, long, etc). >>> =C2=A0That is also reflected when building atomic operations, as the >>> =C2=A0size-bounded types are built from the base C types. >>> >>> =C2=A0However, powerpc does the inverse thing, leading to a serie of na= sty >> >> I think you mean that it does the inverse thing for atomic ops only. The quoted material is now almost unreadable due to hard \xc2\xa0 (these weren't there when I replied before). In fact, the problem is so large in this mail that it should have been filtered as spam. Source code that began with tabs is now completely unreadable. [... context lost to unreadability] > So the thing is that we define the uintXX_t type from int,long,etc and > then we should really expect to follow the same pattern for atomic > instructions. _stdint.h has to do this for the technical reason that there is nothing else except the basic types to start with (short of using attribute(()) and gcc SImode or simlar). But once we have the uintXX_t types declared, and we do in atomic.h, we can start from them almost as easily. There is a minor problem translating back to the basic types. MD code can easily know the translation, but has to be especially careful when variants of the arch support either 32 or 64-bit longs. _stdint has some idfefs related to this, which may make similar ifdefs in atomic.h uneccessary provided you translate in the right direction. > I don't see the point to have something like that, also because is > never used in the kernel. ["That" was definitions with intentional syntax errors for unsupported cases (except the undefs make it unclear whether these ifdes are used).] Yes, I think there as some confusion about what must be supported (I'm not sure about this myself for userland (ab)use of atomic(9)), and the syntax errors were there for debugging this. >> No type problems at this level. > > Yes, but I also want uniformity in the implementation, thus try to > give a common pattern. [This level in (the !#&&#\xc2\xa0 deleted) was the inline function level.] I meant that I liked this part. Inline functions tend to have less type hacks than macros. >>> - =C2=A0 =C2=A064-bit atomic_add not implemented >> >> Now correctly left out when it is not supported. > > See below, for non powerpc64 case it must not be used. Yes, it's best to leave out unsupported cases. I think this is similar to i386 until recently. i386 needed cmpxchg8b for atomic ops on 64-bit variables, but the code for that wasn't written. I think it should have remained unwritten, and 64-bit variables used less so that atomic ops on them are not needed. >>> +#define =C2=A0 =C2=A0 =C2=A0 =C2=A0__atomic_add_long(p, v, t) =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0\ >>> + =C2=A0 =C2=A0long atomic_add not implemented >>> #endif >> >> atomic_add_long() should never be supported, but it is (mis)used in >> standard kernel files so not having it will break more than just LINT. >> But the above is not atomic_add_long()...hacks are used below to make >> that sort of work. > > In which sense "should never be supported"? > If you are on a 64 bit arch and you want and atomic operation for long > you are supposed to use 64-bit version directly? I don't get the > point. 64-bit arches should have 128-bit longs (and maybe 64-bit ints, 32-bit shorts and 16-bit chars)). If they did, then support for atomic ops on longs would be even further off than for 64-bit longs on 32-bit arches, since amd64 doesn't seem to have cmpxchg16b. I guess amd64 could use SSE for 128-bit atomic ops and later AVX for 256-bit ones, but in the kernel those would be more inefficient than usual. "Should never be supported" is in the sense that if you support it then someone might use it and you have to keep supporting it, including on future arches where the support may be too expensive. It's better to discourage use of longs. I think I would prefer guaranteed support for atomic ops on uint64_t (even on 32-bit arches) and use of uint64_t in MI code. long is too flexible. >> This uses bogus casts to break warnings. =C2=A0amd64 is still missing th= is bug. >> Someone added this bug to i386 after reviewers objected to it. =C2=A0Eve= n i386 >> only has this bug for accesses to pointers. =C2=A0For char/short/long, i= t uses >> type-safe code with separate functions/#defines like the ones for int. > > I'll probabilly may have get from the i386 version, I'll verify, thanks. Check the amd64 and sparc64 versions first. >>> [... often I snip bits without noting this as here] >>> -#if 0 >>> -_ATOMIC_CLEAR(8, 8, uint8_t) >>> -_ATOMIC_CLEAR(8, char, u_char) >>> -_ATOMIC_CLEAR(16, 16, uint16_t) >>> -_ATOMIC_CLEAR(16, short, u_short) >>> -#endif >>> -_ATOMIC_CLEAR(32, 32, uint32_t) >>> -_ATOMIC_CLEAR(32, int, u_int) >>> -#ifdef __powerpc64__ >>> -_ATOMIC_CLEAR(64, 64, uint64_t) >>> -_ATOMIC_CLEAR(64, long, u_long) >>> -_ATOMIC_CLEAR(64, ptr, uintptr_t) >>> -#else >>> -_ATOMIC_CLEAR(32, long, u_long) >>> -_ATOMIC_CLEAR(32, ptr, uintptr_t) >>> -#endif >> >> Seems a better way, and still used on amd64 and i386, to generate >> separate type-safe code for each type supported. =C2=A0Here the only err= or >> relative to amd64 and i386 seems to have been to generate from long >> to a 32/64 suffix and #define from that back to a long suffix, instead >> of to generate from long to a long suffix and #define from that to a >> 32/64 suffix. > > I'm a bit confused on what you are stating here exactly... are in > favor of this change or not? I may have been confused by the patch, but the above seems to delete too much. The macros are a bit obfuscatory, but they let you generate a largish inline function from 1 line of code and thus handle N different types with almost full type safety in only N lines of code. This seems preferable to 1-line macros that expand to a function call with parameter casting. Both convert the parameters, but the functions have implicit conversions which should generate warnings about some type errors, while explicit casts should suppress warnings. >> i386 uses a function to type-pun from fetchadd_long to fetchadd_int(). >> This has minor technical advantages. =C2=A0Why be different? > > I'm not entirely sure what you mean with 'function to type-pun' but > will look at that as well, thanks. Just that the wrapper to make some type puns (pun for the pointer and null conversion for the value) is implemented as an inline function on i386 and as a macro on powerpc32. We do the same for cmpset. fetchadd is shorter on at least i386, partly because it is only used in the SMP case and it is assumed that all CPUs that support SMP have xaddl; so it would be easy and cleaner on at least i386 to duplicate it with s/int/long/ to avoid the type puns. This would also fit better with the structure of the file for powerpc64 (there has to be a separate function for longs in the 64-bit case). Bruce --0-2138985585-1304869348=:2061--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20110509003256.F2061>