On Wed, Jun 15, 2016 at 03:01:19PM -0400, Waiman Long wrote:

On 06/15/2016 04:04 AM, Boqun Feng wrote:

quoted

Hi Waiman,

On Tue, Jun 14, 2016 at 06:48:04PM -0400, Waiman Long wrote:

quoted

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<redacted>
---
  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