Re: Deduplicating atomics written in terms of CAS

[Jens Gustedt <jens.gustedt@inria.fr>]

[-- Attachment #1: Type: text/plain, Size: 5056 bytes --]

Am Sonntag, den 17.05.2015, 12:22 -0400 schrieb Rich Felker:
> On Sun, May 17, 2015 at 08:49:04AM +0200, Jens Gustedt wrote:
> > Hello,
> > 
> > Am Sonntag, den 17.05.2015, 00:55 -0400 schrieb Rich Felker:
> > > Lots of archs define most or all of their atomics except a_cas in
> > > terms of a_cas. The attached atomic.h is a proposed replacement for
> > > arch-specific atomic.h that would live in src/internal. The arch
> > > atomic.h files would be replaced with atomic_arch.h, which could opt
> > > to define nothing but a_cas, or could define more primitives itself if
> > > it can do so more efficiently.
> > 
> > I like the approach
> > 
> > > The second attachment, atomic_generic.h, is an implementation of the
> > > atomics (and other non-atomic ops we've traditionally had in atomic.h)
> > > using GNU C builtins. This file can be used as-is for any new archs
> > > that satisfy the following conditions:
> > >
> > > - They're not supported by compilers too old to have the __sync_*
> > >   builtins.
> > > 
> > > - They don't need runtime switching/detection of atomic
> > >   implementations.
> > > 
> > > - GCC doesn't generate pathologically bad code for the builtins.
> > 
> > shouldn't this file then define or macros such as a_swap, too ?
> 
> Hm? I don't understand what you're asking.

Ah, I missed the list of defines that are actually all hidden at the
end of the file. I would have preference to put them in the beginning
of the file.

> > On quick inspection I found issues with the two 64 bit functions:
> > 
> > #ifndef a_and_64
> > static inline void a_and_64(volatile uint64_t *p, uint64_t v)
> > {
> >         union { uint64_t v; uint32_t r[2]; } u = { v };
> >         if (u.r[0]+1) a_and((int *)p, u.r[0]);
> >         if (u.r[1]+1) a_and((int *)p+1, u.r[1]);
> > }
> > #endif
> > 
> > #ifndef a_or_64
> > static inline void a_or_64(volatile uint64_t *p, uint64_t v)
> > {
> >         union { uint64_t v; uint32_t r[2]; } u = { v };
> >         if (u.r[0]) a_or((int *)p, u.r[0]);
> >         if (u.r[1]) a_or((int *)p+1, u.r[1]);
> > }
> > #endif
> > 
> > First I don't get it how we can expect these to be be atomic. It looks
> > to me that the two 32 bit words can be updated with quite a laps of
> > time between them if the thread is delayed. I didn't check this, do we
> > really need 64bit atomics?
> 
> These are misnomers. They're only used/needed as atomic bit-set and
> bit-clear. It would be nice to eliminate them completely, but malloc
> is using them right now. It would be easy to put the above logic
> directly in malloc and have the bitmasks be kept as a union of
> uint64_t and int[], but that's mildly ugly too I think.
> 
> > Then, the mix of uint32_t and int is unfortunate. This code is in
> > header files and thus visible to all compilation units, especially
> > user code that might use any optimization option that the compiler
> > offers. The cast to int* breaks aliasing rules, so compilers that are
> > used with aggressive optimization may produce wrong executables, in
> > pretending that *p didn't change.
> 
> The cast itself doesn't break aliasing rules. Only accessing the
> memory as int does that.

yes, sure, that's what I meant :)

> The intent was that a_or would only access
> the object via asm, so the C type rules would not apply -- that's how
> things originally worked when we only had i386 and x86_64. But now
> that a_or is a C wrapper for a_cas on many/most archs, we do have an
> aliasing problem, I think. That makes me more eagar to remove these.

yes, see my other answer about malloc

> > I only recently learned that even cast to volatile doesn't help in
> > cases where the original object to which p points is not declared
> > volatile. The C standard states that only volatile *declared* objects
> > are subject to the rules of volatile. Accessing through a volatile
> > pointer doesn't help.
> 
> I'm not so sure about that.

I am quite sure. We recently had a discussion on that in the
committee, and the outcome was basically what I was stating above.

> See this question on SO, which has two
> conflicting and both reasonable-sounding answers:
> 
> http://stackoverflow.com/questions/28654418/requirements-for-behavior-of-pointer-to-volatile-pointing-to-non-volatile-object

thanks for the pointer, I didn't knew about the text in the rationale.

This could be an indication that the text as it is in the standard is
a defect.

> In any case, all objects used with atomics in musl are declared
> volatile now, or that is the intent anyway. If I missed any please let
> me know.

I don't know of any, but I will look around a bit.

Jens


-- 
:: INRIA Nancy Grand Est ::: Camus ::::::: ICube/ICPS :::
:: ::::::::::::::: office Strasbourg : +33 368854536   ::
:: :::::::::::::::::::::: gsm France : +33 651400183   ::
:: ::::::::::::::: gsm international : +49 15737185122 ::
:: http://icube-icps.unistra.fr/index.php/Jens_Gustedt ::




[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]