lkml.org 
[lkml]   [2016]   [Aug]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: spin_lock implicit/explicit memory barrier
From
Date
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|


\
 
 \ /
  Last update: 2016-08-10 21:01    [W:0.098 / U:0.180 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site