Re: [PATCH] Add atomic operations required by the new condition variable.

[Florian Weimer <fweimer at redhat dot com>]

On 05/27/2016 01:16 PM, Torvald Riegel wrote:
> On Fri, 2016-05-27 at 11:13 +0200, Florian Weimer wrote:
> > On 05/26/2016 01:04 AM, Torvald Riegel wrote:
> > > +# ifndef atomic_exchange_relaxed
> > > +/* XXX This unnecessarily has acquire MO.  */
> >
> > I don't understand the use of the XXX marker here.  If there is a
> > potential bug, this needs a longer explanation.  If this is just use of
> > an unnecessarily strong memory order, a generic remark somewhere in the
> > file that âarchitectures might override the following defines with
> > relaxed MO implementation for improved performanceâ or something like
> > that should address this, and that doesn't warrant an XXX marker.
>
> It's not a bug, but a less-than-ideal implementation.  What's the marker
> for this then?  (Having no marker isn't ideal because then we can't grep
> for this, for example.)
>
> Architectures should not do their arch-custom stuff in atomics if they
> can.  Note that the XXX is on code in #if !USE_ATOMIC_COMPILER_BUILTINS
> so it will go away eventually once arch maintainers confirm that the new
> atomic builtins work on their machines.

I file bugs in such cases (when there is a specific task to be done).

> > (Ideally, our atomics and how to implement them should be documented in
> > internals manual,
>
> I've said it before and it still stands: C11 is the documentation for
> the new-style atomics we use.  This is documented on the concurrency
> page on the wiki.  What else is there that would need to be documented?

We recently had the conversation on IRC.  I find it difficult to map the 
C11 atomics from the standard to the glibc implementation because C11 
has more atomics, and glibc has the old atomics.

Maybe an explicit list of all modern glibc atomics somewhere would help 
to avoid all doubts.

> > But that's an old and separate discussion.
> > Using the GCC syntax would also avoid the unfortunate naming pattern,
> > e.g. âatomic_fetch_or_releaseâ.)
>
> Remember that this isn't just an arbitrarily chosen naming pattern, but
> follows the C11 names with the difference that we always make the memory
> order an explicit part of the operation's name.  IOW, if people know C11
> atomics, it's *very* easy for them to use ours.  In the long run, more
> developers will know C11 atomics than GCC atomic builtins.

Sure, but it's unfortunate that âatomic_fetch_or_releaseâ doesn't 
naturally parse as âatomic_fetch_OP_MOâ because âorâ can be conjunction, 
not just as a bitwise operation.

Thanks,
Florian