Re: [PATCH] powerpc: Use lwsync on 64bit

[Torvald Riegel <triegel at redhat dot com>]

On Mon, 2017-02-13 at 12:23 -0200, Adhemerval Zanella wrote:
> 
> On 13/02/2017 11:42, Tulio Magno Quites Machado Filho wrote:
> > Anton Blanchard <anton@samba.org> writes:
> > 
> >> Either an isync or an lwsync can be used as an acquire barrier after
> >> a larx/stcx/bne sequence. All 64bit CPUs support lwsync and since the
> >> isync instruction has other side effects that we don't need, use lwsync.
> >>
> >> 2017-02-12  Anton Blanchard  <anton@samba.org>
> >>
> >> 	* sysdeps/powerpc/atomic-machine.h: Allow __ARCH_ACQ_INSTR to be
> >> 	overridden.
> >> 	* sysdeps/powerpc/powerpc64/atomic-machine.h: define __ARCH_ACQ_INSTR
> > 
> > LGTM.
> > 
> 
> I do not think this is correct for all the primitives that use
> __ARCH_ACQ_INSTR.  For instance __arch_atomic_exchange_32_acq is defined as
> 
> #define __arch_atomic_exchange_32_acq(mem, value)                             \
>   ({                                                                          \
>     __typeof (*mem) __val;                                                    \
>     __asm __volatile (                                                        \
>                       "1:       lwarx   %0,0,%2" MUTEX_HINT_ACQ "\n"          \
>                       "         stwcx.  %3,0,%2\n"                            \
>                       "         bne-    1b\n"                                 \
>                       "   " __ARCH_ACQ_INSTR                                  \
>                       : "=&r" (__val), "=m" (*mem)                            \
>                       : "b" (mem), "r" (value), "m" (*mem)                    \
>                       : "cr0", "memory");                                     \
>     __val;                                                                    \
>   })
> 
> 
> Which is analogous to C11:
> 
> #include <stdatomic.h>
> #include <stdint.h>
> 
> uint32_t foo (uint32_t *x)
> {
>   return atomic_exchange_explicit (x, 0, memory_order_acquire);
> }
> 
> And GCC 6.2.1 uses and 'isync' a memory barrier.  It is also on par with [1]
> which defines acquire semantics to require 'isync' instructions.
> 
> 
> [1] https://www.cl.cam.ac.uk/~pes20/cpp/cpp0xmappings.html

I don't know enough about the details of powerpc to comment in detail.
However, I'd like to make a few general comments:

* We should strive to be as close to C11 atomics as produced by the
compiler.  For example, if GCC issues an isync instead of lwsync, we
should be fine doing the same.  If you think GCC is generating
inefficient code, please take the change to GCC first and reach
consensus for a change there.  We'll just use GCC's atomics eventually
(though MUTEX_HINT_ACQ may be a reason for a special case), so applying
any change in GCC is necessary anyway to make such a change effective.

* Any changes in the atomics implementation should be reviewed carefully
by people deeply familiar with the HW memory models and how they map to
C11.  If in doubt, take your time.