Re: spin_lock implicit/explicit memory barrier

[Manfred Spraul]

Hi,

[adding Peter, correcting Davidlohr's mail address]

On 08/10/2016 02:05 AM, Benjamin Herrenschmidt wrote:
> On Tue, 2016-08-09 at 20:52 +0200, Manfred Spraul wrote:
> > Hi Benjamin, Hi Michael,
> >
> > regarding commit 51d7d5205d33 ("powerpc: Add smp_mb() to
> > arch_spin_is_locked()"):
> >
> > For the ipc/sem code, I would like to replace the spin_is_locked() with
> > a smp_load_acquire(), see:
> >
> > http://git.cmpxchg.org/cgit.cgi/linux-mmots.git/tree/ipc/sem.c#n367
> >
> > http://www.ozlabs.org/~akpm/mmots/broken-out/ipc-semc-fix-complex_count-vs-simple-op-race.patch
> >
> > To my understanding, I must now add a smp_mb(), otherwise it would be
> > broken on PowerPC:
> >
> > The approach that the memory barrier is added into spin_is_locked()
> > doesn't work because the code doesn't use spin_is_locked().
> >
> > Correct?
> Right, otherwise you aren't properly ordered. The current powerpc locks provide
> good protection between what's inside vs. what's outside the lock but not vs.
> the lock *value* itself, so if, like you do in the sem code, use the lock
> value as something that is relevant in term of ordering, you probably need
> an explicit full barrier.
>
> Adding Paul McKenney.
Just to be safe, let's write down all barrier pairs:
entry and exit, simple and complex, and switching simple to complex and 
vice versa.

(@Davidlohr: Could you crosscheck, did I overlook a pair?)

1)
spin_lock/spin_unlock pair.

2)

||smp_load_acquire(&sma->complex_mode) and 
|||smp_store_release(sma->complex_mode, true) pair. 
||http://git.cmpxchg.org/cgit.cgi/linux-mmots.git/tree/ipc/sem.c#n374 
http://git.cmpxchg.org/cgit.cgi/linux-mmots.git/tree/ipc/sem.c#n321 The 
store_release guarantees that all data written by the complex_op syscall 
is - after the load_acquire - visible by the simple_op syscall. 3) 
smp_mb() [after spin_lock()] and |||smp_store_mb(sma->complex_mode, true) pair. 
|http://git.cmpxchg.org/cgit.cgi/linux-mmots.git/tree/ipc/sem.c#n287 
http://git.cmpxchg.org/cgit.cgi/linux-mmots.git/tree/ipc/sem.c#n372 This 
are actually two pairs: - Writing the lock variable must observed by the 
task that does spin_unlock_wait() - complex_mode must be observed by the 
task that does the smp_load_acquire() 4) spin_unlock_wait() and 
spin_unlock() pair 
http://git.cmpxchg.org/cgit.cgi/linux-mmots.git/tree/ipc/sem.c#n291 
http://git.cmpxchg.org/cgit.cgi/linux-mmots.git/tree/ipc/sem.c#n409 The 
data from the simple op must be observed by the following complex op. 
Right now, there is still an smp_rmb() in line 300: The control barrier 
from the loop inside spin_unlock_wait() is upgraded to an acquire 
barrier by an additional smp_rmb(). Is this smp_rmb() required? If I 
understand commit 2c6100227116 ("locking/qspinlock: Fix 
spin_unlock_wait() some more") right, with this commit qspinlock handle 
this case without the smp_rmb(). What I don't know if powerpc is using 
qspinlock already, or if powerpc works without the smp_rmb(). -- Manfred|