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<Waiman.Long@hpe.com>
> > > ---
> > >   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