On Wed, Jun 15, 2016 at 03:01:19PM -0400, Waiman Long wrote: > On 06/15/2016 04:04 AM, Boqun Feng wrote: > > Hi Waiman, > > > > On Tue, Jun 14, 2016 at 06:48:04PM -0400, Waiman Long wrote: > > > The osq_lock() and osq_unlock() function may not provide the necessary > > > acquire and release barrier in some cases. This patch makes sure > > > that the proper barriers are provided when osq_lock() is successful > > > or when osq_unlock() is called. > > > > > > Signed-off-by: Waiman Long<[email protected]> > > > --- > > > kernel/locking/osq_lock.c | 4 ++-- > > > 1 files changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/kernel/locking/osq_lock.c b/kernel/locking/osq_lock.c > > > index 05a3785..7dd4ee5 100644 > > > --- a/kernel/locking/osq_lock.c > > > +++ b/kernel/locking/osq_lock.c > > > @@ -115,7 +115,7 @@ bool osq_lock(struct optimistic_spin_queue *lock) > > > * cmpxchg in an attempt to undo our queueing. > > > */ > > > > > > - while (!READ_ONCE(node->locked)) { > > > + while (!smp_load_acquire(&node->locked)) { > > > /* > > > * If we need to reschedule bail... so we can block. > > > */ > > > @@ -198,7 +198,7 @@ void osq_unlock(struct optimistic_spin_queue *lock) > > > * Second most likely case. > > > */ > > > node = this_cpu_ptr(&osq_node); > > > - next = xchg(&node->next, NULL); > > > + next = xchg_release(&node->next, NULL); > > > if (next) { > > > WRITE_ONCE(next->locked, 1); > > So we still use WRITE_ONCE() rather than smp_store_release() here? > > > > Though, IIUC, This is fine for all the archs but ARM64, because there > > will always be a xchg_release()/xchg() before the WRITE_ONCE(), which > > carries a necessary barrier to upgrade WRITE_ONCE() to a RELEASE. > > > > Not sure whether it's a problem on ARM64, but I think we certainly need > > to add some comments here, if we count on this trick. > > > > Am I missing something or misunderstanding you here? > > > > Regards, > > Boqun > > The change on the unlock side is more for documentation purpose than is > actually needed. As you had said, the xchg() call has provided the necessary > memory barrier. Using the _release variant, however, may have some
But I'm afraid the barrier doesn't remain if we replace xchg() with
xchg_release() on ARM64v8, IIUC, xchg_release() is just a ldxr+stlxr
loop with no barrier on ARM64v8. This means the following code:
CPU 0 CPU 1 (next)
======================== ==================
WRITE_ONCE(x, 1); r1 =
smp_load_acquire(next->locked, 1);
xchg_release(&node->next, NULL); r2 = READ_ONCE(x);
WRITE_ONCE(next->locked, 1);
could result in (r1 == 1 && r2 == 0) on ARM64v8, IIUC.
I translated it into a litmus test:
AArch64 stlxr+str
""
{
0:X4=x; 0:X5=node; node=next;
1:X4=x; 1:X5=next;
}
P0 | P1 ;
MOV W0,#1 | LDAR W1,[X5];
STR W0,[X4] | LDR W2,[X4] ;
MOV X0,#0 | ;
LDXR X2,[X5] | ;
STLXR W1,X0,[X5]| ;
CBNZ W1, fail | ;
MOV W0, #1 | ;
STR W0,[X2] | ;
fail: | ;
exists
(0:X0 = 1 /\ 1:X1 = 1 /\ 1:X2 = 0)
and herd said "Sometimes".
But I may miss something here or make a mistake in the translation. So
add Will in Cc list ;-)
> performance benefit in some architectures.
>
> BTW, osq_lock/osq_unlock aren't general purpose locking primitives. So there
> is some leeways on how fancy we want on the lock and unlock sides.
>
Understood, I think it's fine if we rely on something subtle here, but
I just want to make we won't be bitten by some corner cases.
Regards,
Boqun
> Cheers,
> Longman
signature.asc
Description: PGP signature
