[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <1431883157.4219.3.camel@inria.fr>
Date: Sun, 17 May 2015 19:19:17 +0200
From: Jens Gustedt <jens.gustedt@...ia.fr>
To: musl@...ts.openwall.com
Subject: Re: Deduplicating atomics written in terms of CAS
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 ::
Download attachment "signature.asc" of type "application/pgp-signature" (182 bytes)
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
