Re: [RFC] [PATCH] powerpc: Fix missing barriers in atomic_exchange_and_add_{acq,rel}

[Torvald Riegel <triegel at redhat dot com>]

On Fri, 2014-10-24 at 08:15 -0500, Steven Munroe wrote:
> On Tue, 2014-10-21 at 21:54 +0200, Torvald Riegel wrote:
> > On powerpc, atomic_exchange_and_add is implemented without any barriers.
> > However, atomic_exchange_and_add_acq and atomic_exchange_and_add_rel
> > (which supposedly should have acquire / release barrier semantics) both
> > fall back to atomic_exchange_and_add if they are not defined (see
> > include/atomic.h).  I have not reviewed existing code to see whether
> > this would indeed cause a bug, but this lack of barriers likely is a
> > (future) fault, and prevents any use of
> > atomic_exchange_and_add_{acq,rel} that actually rely on the barrier
> > semantics.  Therefore, this patch defines
> > atomic_exchange_and_add_{acq,rel} using atomic_read_barrier /
> > atomic_write_barrier.
> > 
> > I have NOT tested this.  Can somebody who cares about powerpc please
> > have a look and test this?  Thanks!
> 
> This is incorrect. Do not apply these changes.

Have a look at the different variants of atomic_exchange_and_add*.  The
_acq and _rel suffixes are used on the current atomic ops to designate
those operations that have acquire and release barrier semantics.

> The requirement for memory barriers depend on how the result of the
> exchange and add is used.

Yes, see above.  Note that this doesn't add any barriers to
atomic_exchange_and_add, just to atomic_exchange_and_add_acq and
atomic_exchange_and_add_rel.

If you are concerned about uses of unnecessary memory barriers on
powerpc, the way forward is to help review concurrent code in glibc and
help moving it over to the C11 memory model (see below).

> For example if the value is a simple counter
> or the original value (result of the larx load) is used as an address
> there is NO need for a memory barrier. 

I do understand the concept of memory barriers, and their difference to
atomic HW operations.

Please also note that we won't be using address dependencies or such in
the near future, because the language support isn't there yet.  We don't
have the resources to do the complicated dance with the compiler that
the Linux kernel people do.  See
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4215.pdf for
more background.

> See PowerISA-2.07 BookII, Appendix B.2.1.2
> 
> So this is unnecessarily pessimistic and should not be the default.

It is not the default.  And there are no defaults really, just that some
implementations pick defaults.

BTW, we're very likely to transition to using the C11 memory model.  So
at that point, all barriers will be explicit in the atomic operations,
and we'll add actual memory_order_relaxed atomic operations (including
for CAS and other atomic RMW ops).  There won't be any default in the
C11-like atomics we use.