On Wed, Oct 14, 2015 at 01:19:17PM -0700, Paul E. McKenney wrote:
> On Wed, Oct 14, 2015 at 11:55:56PM +0800, Boqun Feng wrote:
> > According to memory-barriers.txt, xchg, cmpxchg and their atomic{,64}_
> > versions all need to imply a full barrier, however they are now just
> > RELEASE+ACQUIRE, which is not a full barrier.
> >
> > So replace PPC_RELEASE_BARRIER and PPC_ACQUIRE_BARRIER with
> > PPC_ATOMIC_ENTRY_BARRIER and PPC_ATOMIC_EXIT_BARRIER in
> > __{cmp,}xchg_{u32,u64} respectively to guarantee a full barrier
> > semantics of atomic{,64}_{cmp,}xchg() and {cmp,}xchg().
> >
> > This patch is a complement of commit b97021f85517 ("powerpc: Fix
> > atomic_xxx_return barrier semantics").
> >
> > Acked-by: Michael Ellerman <mpe@xxxxxxxxxxxxxx>
> > Cc: <stable@xxxxxxxxxxxxxxx> # 3.4+
> > Signed-off-by: Boqun Feng <boqun.feng@xxxxxxxxx>
> > ---
> > arch/powerpc/include/asm/cmpxchg.h | 16 ++++++++--------
> > 1 file changed, 8 insertions(+), 8 deletions(-)
> >
> > diff --git a/arch/powerpc/include/asm/cmpxchg.h b/arch/powerpc/include/asm/cmpxchg.h
> > index ad6263c..d1a8d93 100644
> > --- a/arch/powerpc/include/asm/cmpxchg.h
> > +++ b/arch/powerpc/include/asm/cmpxchg.h
> > @@ -18,12 +18,12 @@ __xchg_u32(volatile void *p, unsigned long val)
> > unsigned long prev;
> >
> > __asm__ __volatile__(
> > - PPC_RELEASE_BARRIER
> > + PPC_ATOMIC_ENTRY_BARRIER
>
> This looks to be the lwsync instruction.
>
> > "1: lwarx %0,0,%2 \n"
> > PPC405_ERR77(0,%2)
> > " stwcx. %3,0,%2 \n\
> > bne- 1b"
> > - PPC_ACQUIRE_BARRIER
> > + PPC_ATOMIC_EXIT_BARRIER
>
> And this looks to be the sync instruction.
>
> > : "=&r" (prev), "+m" (*(volatile unsigned int *)p)
> > : "r" (p), "r" (val)
> > : "cc", "memory");
>
> Hmmm...
>
> Suppose we have something like the following, where "a" and "x" are both
> initially zero:
>
> CPU 0 CPU 1
> ----- -----
>
> WRITE_ONCE(x, 1); WRITE_ONCE(a, 2);
> r3 = xchg(&a, 1); smp_mb();
> r3 = READ_ONCE(x);
>
> If xchg() is fully ordered, we should never observe both CPUs'
> r3 values being zero, correct?
>
> And wouldn't this be represented by the following litmus test?
>
> PPC SB+lwsync-RMW2-lwsync+st-sync-leading
> ""
> {
> 0:r1=1; 0:r2=x; 0:r3=3; 0:r10=0 ; 0:r11=0; 0:r12=a;
> 1:r1=2; 1:r2=x; 1:r3=3; 1:r10=0 ; 1:r11=0; 1:r12=a;
> }
> P0 | P1 ;
> stw r1,0(r2) | stw r1,0(r12) ;
> lwsync | sync ;
> lwarx r11,r10,r12 | lwz r3,0(r2) ;
> stwcx. r1,r10,r12 | ;
> bne Fail0 | ;
> mr r3,r11 | ;
> Fail0: | ;
> exists
> (0:r3=0 /\ a=2 /\ 1:r3=0)
>
> I left off P0's trailing sync because there is nothing for it to order
> against in this particular litmus test. I tried adding it and verified
> that it has no effect.
>
> Am I missing something here? If not, it seems to me that you need
> the leading lwsync to instead be a sync.
>
If so, I will define PPC_ATOMIC_ENTRY_BARRIER as "sync" in the next
version of this patch, any concern?
Of course, I will wait to do that until we all understand this is
nececarry and agree to make the change.
> Of course, if I am not missing something, then this applies also to the
> value-returning RMW atomic operations that you pulled this pattern from.
For the value-returning RMW atomics, if the leading barrier is
necessarily to be "sync", I will just remove my __atomic_op_fence() in
patch 4, but I will remain patch 3 unchanged for the consistency of
__atomic_op_*() macros' definitions. Peter and Will, do that works for
you both?
Regards,
Boqun
> If so, it would seem that I didn't think through all the possibilities
> back when PPC_ATOMIC_EXIT_BARRIER moved to sync... In fact, I believe
> that I worried about the RMW atomic operation acting as a barrier,
> but not as the load/store itself. :-/
>
> Thanx, Paul
>
Attachment:
signature.asc
Description: PGP signature