locking/pvqspinlock: Relax cmpxchg's to improve performance on some archs

All the locking related cmpxchg's in the following functions are replaced with the _acquire variants:
- pv_queued_spin_steal_lock()
- trylock_clear_pending()

This change should help performance on architectures that use LL/SC.

On a 2-core 16-thread Power8 system with pvqspinlock explicitly enabled, the performance of a locking microbenchmark with and without this patch on a 4.10-rc8 kernel with Xinhui's PPC qspinlock patch were as follows:

  # of thread     w/o patch    with patch      % Change
  -----------     ---------    ----------      --------
       4         4053.3 Mop/s  4223.7 Mop/s     +4.2%
       8         3310.4 Mop/s  3406.0 Mop/s     +2.9%
      12         2576.4 Mop/s  2674.6 Mop/s     +3.8%

Signed-off-by Waiman Long <[email protected]>

---
v4->v5:
- Correct some grammatical issues in comment.

v3->v4:
- Update the comment in pv_kick_node() to mention that the code may not work in some archs.

v2->v3:
- Reduce scope by relaxing cmpxchg's in fast path only.

v1->v2:
- Add comments in changelog and code for the rationale of the change.

 kernel/locking/qspinlock_paravirt.h | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

kernel/locking/qspinlock_paravirt.h +6 -6

index e6b2f7a..4614e39 100644
@@ -72,7 +72,7 @@ static inline bool pv_queued_spin_steal_lock(struct qspinlock *lock)
 	struct __qspinlock *l = (void *)lock;
 
 	if (!(atomic_read(&lock->val) & _Q_LOCKED_PENDING_MASK) &&
-	    (cmpxchg(&l->locked, 0, _Q_LOCKED_VAL) == 0)) {
+	    (cmpxchg_acquire(&l->locked, 0, _Q_LOCKED_VAL) == 0)) {
 		qstat_inc(qstat_pv_lock_stealing, true);
 		return true;
 	}

@@ -101,16 +101,16 @@ static __always_inline void clear_pending(struct qspinlock *lock)
 
 /*
  * The pending bit check in pv_queued_spin_steal_lock() isn't a memory
- * barrier. Therefore, an atomic cmpxchg() is used to acquire the lock
- * just to be sure that it will get it.
+ * barrier. Therefore, an atomic cmpxchg_acquire() is used to acquire the
+ * lock just to be sure that it will get it.
  */
 static __always_inline int trylock_clear_pending(struct qspinlock *lock)●●●●●●●●●●●
 {
 	struct __qspinlock *l = (void *)lock;
 
 	return !READ_ONCE(l->locked) &&
-	       (cmpxchg(&l->locked_pending, _Q_PENDING_VAL, _Q_LOCKED_VAL)
-			== _Q_PENDING_VAL);
+	       (cmpxchg_acquire(&l->locked_pending, _Q_PENDING_VAL,
+				_Q_LOCKED_VAL) == _Q_PENDING_VAL);
 }
 #else /* _Q_PENDING_BITS == 8 */
 static __always_inline void set_pending(struct qspinlock *lock)

@@ -138,7 +138,7 @@ static __always_inline int trylock_clear_pending(struct qspinlock *lock)
 		 */
 		old = val;
 		new = (val & ~_Q_PENDING_MASK) | _Q_LOCKED_VAL;
-		val = atomic_cmpxchg(&lock->val, old, new);
+		val = atomic_cmpxchg_acquire(&lock->val, old, new);
 
 		if (val == old)
 			return 1;

@@ -361,6 +361,13 @@ static void pv_kick_node(struct qspinlock *lock, struct mcs_spinlock *node)
 	 * observe its next->locked value and advance itself.
 	 *
 	 * Matches with smp_store_mb() and cmpxchg() in pv_wait_node()

kernel/locking/qspinlock_paravirt.h +0 -0

@@ -361,6 +361,13 @@ static void pv_kick_node(struct qspinlock *lock, struct mcs_spinlock *node)
 	 * observe its next->locked value and advance itself.
 	 *
 	 * Matches with smp_store_mb() and cmpxchg() in pv_wait_node()

Now, ARM64 for instance plays funny games, it does something along the lines of:

cmpxchg(ptr, old, new)

{
	do {
		r = LL(ptr);
		if (r != old)
			return r; /* no barriers */
		r = new
	} while (SC_release(ptr, r));
	smp_mb();
	return r;
}

Thereby ordering things relative to the store on ptr, but the load can very much escape. The thinking is that if success, we must observe the latest value of ptr, but even in that case the load is not ordered and could happen before.

However, since we're guaranteed to observe the latest value of ptr (on success) it doesn't matter if we reordered the load, there is no newer value possible.

So heaps of tricky, but correct afaict. Will?

Quoted 20 lines · Peter Zijlstra

Now, ARM64 for instance plays funny games, it does something along the lines of:

cmpxchg(ptr, old, new)

{
	do {
		r = LL(ptr);
		if (r != old)
			return r; /* no barriers */
		r = new
	} while (SC_release(ptr, r));
	smp_mb();
	return r;
}

Thereby ordering things relative to the store on ptr, but the load can very much escape. The thinking is that if success, we must observe the latest value of ptr, but even in that case the load is not ordered and could happen before.

Quoted 5 lines

However, since we're guaranteed to observe the latest value of ptr (on success) it doesn't matter if we reordered the load, there is no newer value possible.

So heaps of tricky, but correct afaict. Will?

And could not PPC do something similar:

cmpxchg(ptr, old, new)

{
	lwsync();
	dp {
		r = LL(ptr);
		if (r != old)
			return;
		r = new;
	} while (SC(ptr, r));
	sync();
	return r;
}

?

the lwsync would make it store-release on SC with similar reasoning as above.

And lwsync allows 'stores reordered after loads', which allows the prior smp_store_release() to leak past.

Or is the reason this doesn't work on PPC that its RCpc?

Quoted 1 line

Or is the reason this doesn't work on PPC that its RCpc?

Here is an example why PPC needs a sync() before the cmpxchg():

	https://marc.info/?l=linux-kernel&m=144485396224519&w=2

and Paul Mckenney's detailed explanation about why this could happen:

	https://marc.info/?l=linux-kernel&m=144485909826241&w=2

(Somehow, I feel like he was answering to a similar question question as you ask here ;-))

And I think aarch64 doesn't have a problem here because it is "(other) multi-copy atomic". Will?

Quoted 1 line · Boqun Feng

Quoted 1 line

Or is the reason this doesn't work on PPC that its RCpc?

So that :-)

Quoted 10 lines

Here is an example why PPC needs a sync() before the cmpxchg():

	https://marc.info/?l=linux-kernel&m=144485396224519&w=2

and Paul Mckenney's detailed explanation about why this could happen:

	https://marc.info/?l=linux-kernel&m=144485909826241&w=2

(Somehow, I feel like he was answering to a similar question question as you ask here ;-))

Yes, and I had vague memories of having gone over this before, but couldn't quickly find things. Thanks!

Quoted 2 lines

And I think aarch64 doesn't have a problem here because it is "(other) multi-copy atomic". Will?

Right, its the RCpc vs RCsc thing. The ARM64 release is as you say multi-copy atomic, whereas the PPC lwsync is not.

This still leaves us with the situation that we need an smp_mb() between smp_store_release() and a possibly failing cmpxchg() if we want to guarantee the cmpxchg()'s load comes after the store-release.

  # of thread     w/o patch    with patch      % Change
  -----------     ---------    ----------      --------
       4         4053.3 Mop/s  4223.7 Mop/s     +4.2%
       8         3310.4 Mop/s  3406.0 Mop/s     +2.9%
      12         2576.4 Mop/s  2674.6 Mop/s     +3.8%

Quoted 5 lines · Waiman Long

  # of thread     w/o patch    with patch      % Change
  -----------     ---------    ----------      --------
       4         4053.3 Mop/s  4223.7 Mop/s     +4.2%
       8         3310.4 Mop/s  3406.0 Mop/s     +2.9%
      12         2576.4 Mop/s  2674.6 Mop/s     +3.8%

Waiman, could you run those numbers again but with the below 'fixed' ?

kernel/locking/qspinlock_paravirt.h static void pv_kick_node(struct qspinlock *lock, struct mcs_spinlock *node) · line 371

Quoted 12 lines

kernel/locking/qspinlock_paravirt.h +7 -0

@@ -361,6 +361,13 @@ static void pv_kick_node(struct qspinlock *lock, struct mcs_spinlock *node)
 	 * observe its next->locked value and advance itself.
 	 *
 	 * Matches with smp_store_mb() and cmpxchg() in pv_wait_node()
+	 *
+	 * The write to next->locked in arch_mcs_spin_unlock_contended()
+	 * must be ordered before the read of pn->state in the cmpxchg()
+	 * below for the code to work correctly. However, this is not
+	 * guaranteed on all architectures when the cmpxchg() call fails.
+	 * Both x86 and PPC can provide that guarantee, but other
+	 * architectures not necessarily.
 	 */

	smp_mb();

kernel/locking/qspinlock_paravirt.h static void pv_kick_node(struct qspinlock *lock, struct mcs_spinlock *node) · line 373

Quoted 2 lines

kernel/locking/qspinlock_paravirt.h +0 -0

 	if (cmpxchg(&pn->state, vcpu_halted, vcpu_hashed) != vcpu_halted)
 		return;

Ideally this Power CPU can optimize back-to-back SYNC instructions, but who knows...

Quoted 22 lines · Peter Zijlstra

Quoted 5 lines · Waiman Long

  # of thread     w/o patch    with patch      % Change
  -----------     ---------    ----------      --------
       4         4053.3 Mop/s  4223.7 Mop/s     +4.2%
       8         3310.4 Mop/s  3406.0 Mop/s     +2.9%
      12         2576.4 Mop/s  2674.6 Mop/s     +3.8%

Waiman, could you run those numbers again but with the below 'fixed' ?

kernel/locking/qspinlock_paravirt.h static void pv_kick_node(struct qspinlock *lock, struct mcs_spinlock *node) · line 371

Quoted 12 lines

kernel/locking/qspinlock_paravirt.h +7 -0

@@ -361,6 +361,13 @@ static void pv_kick_node(struct qspinlock *lock, struct mcs_spinlock *node)
 	 * observe its next->locked value and advance itself.
 	 *
 	 * Matches with smp_store_mb() and cmpxchg() in pv_wait_node()
+	 *
+	 * The write to next->locked in arch_mcs_spin_unlock_contended()
+	 * must be ordered before the read of pn->state in the cmpxchg()
+	 * below for the code to work correctly. However, this is not
+	 * guaranteed on all architectures when the cmpxchg() call fails.
+	 * Both x86 and PPC can provide that guarantee, but other
+	 * architectures not necessarily.
 	 */

	smp_mb();

Quoted 4 lines

kernel/locking/qspinlock_paravirt.h static void pv_kick_node(struct qspinlock *lock, struct mcs_spinlock *node) · line 373

Quoted 2 lines

kernel/locking/qspinlock_paravirt.h +0 -0

 	if (cmpxchg(&pn->state, vcpu_halted, vcpu_hashed) != vcpu_halted)
 		return;

Ideally this Power CPU can optimize back-to-back SYNC instructions, but who knows...

Yes, I can run the numbers again. However, the changes here is in the slowpath. My current patch optimizes the fast path only and my original test doesn't stress the slowpath at all, I think. I will have to make some changes to stress the slowpath.

Quoted 23 lines · Waiman Long

Quoted 22 lines · Peter Zijlstra

Quoted 5 lines · Waiman Long

  # of thread     w/o patch    with patch      % Change
  -----------     ---------    ----------      --------
       4         4053.3 Mop/s  4223.7 Mop/s     +4.2%
       8         3310.4 Mop/s  3406.0 Mop/s     +2.9%
      12         2576.4 Mop/s  2674.6 Mop/s     +3.8%

Waiman, could you run those numbers again but with the below 'fixed' ?

kernel/locking/qspinlock_paravirt.h static void pv_kick_node(struct qspinlock *lock, struct mcs_spinlock *node) · line 371

Quoted 12 lines

kernel/locking/qspinlock_paravirt.h +7 -0

@@ -361,6 +361,13 @@ static void pv_kick_node(struct qspinlock *lock, struct mcs_spinlock *node)
 	 * observe its next->locked value and advance itself.
 	 *
 	 * Matches with smp_store_mb() and cmpxchg() in pv_wait_node()
+	 *
+	 * The write to next->locked in arch_mcs_spin_unlock_contended()
+	 * must be ordered before the read of pn->state in the cmpxchg()
+	 * below for the code to work correctly. However, this is not
+	 * guaranteed on all architectures when the cmpxchg() call fails.
+	 * Both x86 and PPC can provide that guarantee, but other
+	 * architectures not necessarily.
 	 */

	smp_mb();

Quoted 8 lines

Quoted 4 lines

kernel/locking/qspinlock_paravirt.h static void pv_kick_node(struct qspinlock *lock, struct mcs_spinlock *node) · line 373

Quoted 2 lines

kernel/locking/qspinlock_paravirt.h +0 -0

 	if (cmpxchg(&pn->state, vcpu_halted, vcpu_hashed) != vcpu_halted)
 		return;

Ideally this Power CPU can optimize back-to-back SYNC instructions, but who knows...

Yes, I can run the numbers again. However, the changes here is in the slowpath. My current patch optimizes the fast path only and my original test doesn't stress the slowpath at all, I think. I will have to make some changes to stress the slowpath.

Looking at past emails, I remember why I put the comment there. Putting an smp_mb() here will definitely has an negative performance impact on x86. So I put in the comment here to remind me that the current code may not work for ARM64.

To fix that, my current thought is to have a cmpxchg variant that guarantees ordering for both success and failure, for example, cmpxchg_ordered(). In that way, we only need to insert the barrier for architectures that need it. That will be a separate patch instead of integrating into this one.

Quoted 37 lines · Waiman Long

Quoted 23 lines · Waiman Long

Quoted 22 lines · Peter Zijlstra

Quoted 5 lines · Waiman Long

  # of thread     w/o patch    with patch      % Change
  -----------     ---------    ----------      --------
       4         4053.3 Mop/s  4223.7 Mop/s     +4.2%
       8         3310.4 Mop/s  3406.0 Mop/s     +2.9%
      12         2576.4 Mop/s  2674.6 Mop/s     +3.8%

Waiman, could you run those numbers again but with the below 'fixed' ?

kernel/locking/qspinlock_paravirt.h static void pv_kick_node(struct qspinlock *lock, struct mcs_spinlock *node) · line 371

Quoted 12 lines

kernel/locking/qspinlock_paravirt.h +7 -0

@@ -361,6 +361,13 @@ static void pv_kick_node(struct qspinlock *lock, struct mcs_spinlock *node)
 	 * observe its next->locked value and advance itself.
 	 *
 	 * Matches with smp_store_mb() and cmpxchg() in pv_wait_node()
+	 *
+	 * The write to next->locked in arch_mcs_spin_unlock_contended()
+	 * must be ordered before the read of pn->state in the cmpxchg()
+	 * below for the code to work correctly. However, this is not
+	 * guaranteed on all architectures when the cmpxchg() call fails.
+	 * Both x86 and PPC can provide that guarantee, but other
+	 * architectures not necessarily.
 	 */

	smp_mb();

Quoted 8 lines

Quoted 4 lines

kernel/locking/qspinlock_paravirt.h static void pv_kick_node(struct qspinlock *lock, struct mcs_spinlock *node) · line 373

Quoted 2 lines

kernel/locking/qspinlock_paravirt.h +0 -0

 	if (cmpxchg(&pn->state, vcpu_halted, vcpu_hashed) != vcpu_halted)
 		return;

Ideally this Power CPU can optimize back-to-back SYNC instructions, but who knows...

Yes, I can run the numbers again. However, the changes here is in the slowpath. My current patch optimizes the fast path only and my original test doesn't stress the slowpath at all, I think. I will have to make some changes to stress the slowpath.

Looking at past emails, I remember why I put the comment there. Putting an smp_mb() here will definitely has an negative performance impact on x86. So I put in the comment here to remind me that the current code may not work for ARM64.

Quoted 5 lines

To fix that, my current thought is to have a cmpxchg variant that guarantees ordering for both success and failure, for example, cmpxchg_ordered(). In that way, we only need to insert the barrier for architectures that need it. That will be a separate patch instead of integrating into this one.

Might as well do an explicit:

	smp_mb__before_atomic()
	cmpxchg_relaxed()
	smp_mb__after_atomic()

I suppose and not introduce new primitives.

Quoted 43 lines · Peter Zijlstra

Quoted 42 lines · Waiman Long

Quoted 23 lines · Waiman Long

Quoted 22 lines · Peter Zijlstra

Quoted 5 lines · Waiman Long

  # of thread     w/o patch    with patch      % Change
  -----------     ---------    ----------      --------
       4         4053.3 Mop/s  4223.7 Mop/s     +4.2%
       8         3310.4 Mop/s  3406.0 Mop/s     +2.9%
      12         2576.4 Mop/s  2674.6 Mop/s     +3.8%

Waiman, could you run those numbers again but with the below 'fixed' ?

kernel/locking/qspinlock_paravirt.h static void pv_kick_node(struct qspinlock *lock, struct mcs_spinlock *node) · line 371

Quoted 12 lines

kernel/locking/qspinlock_paravirt.h +7 -0

@@ -361,6 +361,13 @@ static void pv_kick_node(struct qspinlock *lock, struct mcs_spinlock *node)
 	 * observe its next->locked value and advance itself.
 	 *
 	 * Matches with smp_store_mb() and cmpxchg() in pv_wait_node()
+	 *
+	 * The write to next->locked in arch_mcs_spin_unlock_contended()
+	 * must be ordered before the read of pn->state in the cmpxchg()
+	 * below for the code to work correctly. However, this is not
+	 * guaranteed on all architectures when the cmpxchg() call fails.
+	 * Both x86 and PPC can provide that guarantee, but other
+	 * architectures not necessarily.
 	 */

	smp_mb();

Quoted 8 lines

Quoted 4 lines

kernel/locking/qspinlock_paravirt.h static void pv_kick_node(struct qspinlock *lock, struct mcs_spinlock *node) · line 373

Quoted 2 lines

kernel/locking/qspinlock_paravirt.h +0 -0

 	if (cmpxchg(&pn->state, vcpu_halted, vcpu_hashed) != vcpu_halted)
 		return;

Ideally this Power CPU can optimize back-to-back SYNC instructions, but who knows...

Yes, I can run the numbers again. However, the changes here is in the slowpath. My current patch optimizes the fast path only and my original test doesn't stress the slowpath at all, I think. I will have to make some changes to stress the slowpath.

Looking at past emails, I remember why I put the comment there. Putting an smp_mb() here will definitely has an negative performance impact on x86. So I put in the comment here to remind me that the current code may not work for ARM64.

To fix that, my current thought is to have a cmpxchg variant that guarantees ordering for both success and failure, for example, cmpxchg_ordered(). In that way, we only need to insert the barrier for architectures that need it. That will be a separate patch instead of integrating into this one.

Quoted 7 lines

Might as well do an explicit:

	smp_mb__before_atomic()
	cmpxchg_relaxed()
	smp_mb__after_atomic()

I suppose and not introduce new primitives.

Quoted 8 lines · Waiman Long

Quoted 6 lines · Peter Zijlstra

Might as well do an explicit:

	smp_mb__before_atomic()
	cmpxchg_relaxed()
	smp_mb__after_atomic()

Quoted 1 line

Quoted 1 line

I suppose and not introduce new primitives.

I think we don't need smp_mb__after_atomic(). The read has to be fully ordered, but the write part may not need it as the control dependency of the old value should guard against incorrect action. Right?

Quoted 9 lines · Waiman Long

Quoted 8 lines · Waiman Long

Quoted 6 lines · Peter Zijlstra

Might as well do an explicit:

	smp_mb__before_atomic()
	cmpxchg_relaxed()
	smp_mb__after_atomic()

Quoted 5 lines

Quoted 1 line

Quoted 1 line

I suppose and not introduce new primitives.

I think we don't need smp_mb__after_atomic(). The read has to be fully ordered, but the write part may not need it as the control dependency of the old value should guard against incorrect action. Right?

You'd think that, but IIRC there was something funny about using the SC return flag for control dependencies. Will?

Quoted 13 lines · Peter Zijlstra

Quoted 12 lines · Waiman Long

Quoted 8 lines · Waiman Long

Quoted 7 lines · Peter Zijlstra

Might as well do an explicit:

	smp_mb__before_atomic()
	cmpxchg_relaxed()
	smp_mb__after_atomic()

I suppose and not introduce new primitives.

I think we don't need smp_mb__after_atomic(). The read has to be fully ordered, but the write part may not need it as the control dependency of the old value should guard against incorrect action. Right?

Quoted 2 lines

You'd think that, but IIRC there was something funny about using the SC return flag for control dependencies. Will?

Yeah, that's right, you can't use the STXR status flag to create control dependencies.

Will

Quoted 14 lines · Will Deacon

Quoted 13 lines · Peter Zijlstra

Quoted 12 lines · Waiman Long

Quoted 8 lines · Waiman Long

Quoted 7 lines · Peter Zijlstra

Might as well do an explicit:

	smp_mb__before_atomic()
	cmpxchg_relaxed()
	smp_mb__after_atomic()

I suppose and not introduce new primitives.

I think we don't need smp_mb__after_atomic(). The read has to be fully ordered, but the write part may not need it as the control dependency of the old value should guard against incorrect action. Right?

Quoted 6 lines

Quoted 2 lines

You'd think that, but IIRC there was something funny about using the SC return flag for control dependencies. Will?

Yeah, that's right, you can't use the STXR status flag to create control dependencies.

Will

Actually, the code sequence that I plan to use are:

        smp_mb__before_atomic();
        if (cmpxchg_relaxed(&pn->state, vcpu_halted, vcpu_hashed)
            != vcpu_halted)
                return;

        WRITE_ONCE(l->locked, _Q_SLOW_VAL);
        (void)pv_hash(lock, pn);

I am planning to use the comparison of the returned value (pn->state) again vcpu_halted as the control dependency. I don't see how the status flag of STXR is affecting this.

Yeah, that's right, you can't use the STXR status flag to create control dependencies.

Quoted 2 lines · Will Deacon

Yeah, that's right, you can't use the STXR status flag to create control dependencies.

Just for my elucidation; you can't use it to create a control dependency on the store, but you can use it to create a control dependency on the corresponding load, right?

Now, IIRC, we've defined control dependencies as being LOAD->STORE ordering, so in that respect nothing is lost. But maybe we should explicitly mention that if the LOAD is part of an (otherwise) atomic RmW the STORE is not constrained.

Quoted 4 lines · Peter Zijlstra

Quoted 2 lines · Will Deacon

Yeah, that's right, you can't use the STXR status flag to create control dependencies.

Quoted 3 lines

Just for my elucidation; you can't use it to create a control dependency on the store, but you can use it to create a control dependency on the corresponding load, right?

Hmm, sort of, but I'd say that the reads are really ordered due to read-after-read ordering in that case. Control dependencies to loads don't give you order.

Quoted 4 lines

Now, IIRC, we've defined control dependencies as being LOAD->STORE ordering, so in that respect nothing is lost. But maybe we should explicitly mention that if the LOAD is part of an (otherwise) atomic RmW the STORE is not constrained.

I could well be misreading your suggestion, but it feels like that's too weak. You can definitely still have control dependencies off the LL part of the LL/SC pair, just not off the SC part.

E.g. this version of LB is forbidden on arm64:

P0:
if (atomic_inc_return_relaxed(&x) == 2)

	atomic_set(&y, 1);

P1:
if (atomic_inc_return_relaxed(&y) == 2)

	atomic_set(&x, 1);

Perhaps when you say "the STORE", you mean the store in the atomic RmW, rather than the store in the LOAD->STORE control dependency?

Quoted 9 lines · Will Deacon

Quoted 7 lines · Peter Zijlstra

Quoted 2 lines · Will Deacon

Yeah, that's right, you can't use the STXR status flag to create control dependencies.

Just for my elucidation; you can't use it to create a control dependency on the store, but you can use it to create a control dependency on the corresponding load, right?

Quoted 3 lines

Hmm, sort of, but I'd say that the reads are really ordered due to read-after-read ordering in that case. Control dependencies to loads don't give you order.

No, I meant _from_ the LL load, not _to_ a later load.

Quoted 19 lines

Quoted 4 lines

Now, IIRC, we've defined control dependencies as being LOAD->STORE ordering, so in that respect nothing is lost. But maybe we should explicitly mention that if the LOAD is part of an (otherwise) atomic RmW the STORE is not constrained.

I could well be misreading your suggestion, but it feels like that's too weak. You can definitely still have control dependencies off the LL part of the LL/SC pair, just not off the SC part.

E.g. this version of LB is forbidden on arm64:

P0:
if (atomic_inc_return_relaxed(&x) == 2)

	atomic_set(&y, 1);

P1:
if (atomic_inc_return_relaxed(&y) == 2)

	atomic_set(&x, 1);

Quoted 2 lines

Perhaps when you say "the STORE", you mean the store in the atomic RmW, rather than the store in the LOAD->STORE control dependency?

Yes. So I was looking to exclude (SC) STORE -> STORE order through control dependencies.

Quoted 1 line

No, I meant _from_ the LL load, not _to_ a later load.

Sorry, I'm still not following enough to give you a definitive answer on that. Could you give an example, please? These sequences usually run in a loop, so the conditional branch back (based on the status flag) is where the read-after-read comes in.

Any control dependencies from the loaded data exist regardless of the status flag.

Quoted 9 lines · Will Deacon

Quoted 1 line

No, I meant _from_ the LL load, not _to_ a later load.

Sorry, I'm still not following enough to give you a definitive answer on that. Could you give an example, please? These sequences usually run in a loop, so the conditional branch back (based on the status flag) is where the read-after-read comes in.

Any control dependencies from the loaded data exist regardless of the status flag.

Basically what Waiman ended up doing, something like:

        if (cmpxchg_relaxed(&pn->state, vcpu_halted, vcpu_hashed) != vcpu_halted)
                return;

        WRITE_ONCE(l->locked, _Q_SLOW_VAL);

Where the STORE depends on the LL value being 'complete'.

For any RmW we can only create a control dependency from the LOAD. The the same could be done for something like:

	if (atomic_inc_not_zero(&obj->refs))
		WRITE_ONCE(obj->foo, 1);

Where we only do the STORE if we acquire the reference. While the WRITE_ONCE() will not be ordered against the increment, it is ordered against the LL and we know it must not be 0.

Per the LL/SC loop we'll have observed a !0 value and committed the SC (which need not be visible or ordered against any later store) but both STORES (SC and the WRITE_ONCE) must be after the ->refs LOAD.

Quoted 11 lines · Peter Zijlstra

Quoted 8 lines · Will Deacon

Quoted 1 line

No, I meant _from_ the LL load, not _to_ a later load.

Sorry, I'm still not following enough to give you a definitive answer on that. Could you give an example, please? These sequences usually run in a loop, so the conditional branch back (based on the status flag) is where the read-after-read comes in.

Any control dependencies from the loaded data exist regardless of the status flag.

Basically what Waiman ended up doing, something like:

Quoted 7 lines

        if (cmpxchg_relaxed(&pn->state, vcpu_halted, vcpu_hashed) != vcpu_halted)
                return;

        WRITE_ONCE(l->locked, _Q_SLOW_VAL);

Where the STORE depends on the LL value being 'complete'.

Quoted 9 lines · Will Deacon

Quoted 1 line

No, I meant _from_ the LL load, not _to_ a later load.

Sorry, I'm still not following enough to give you a definitive answer on that. Could you give an example, please? These sequences usually run in a loop, so the conditional branch back (based on the status flag) is where the read-after-read comes in.

Any control dependencies from the loaded data exist regardless of the status flag.

Basically what Waiman ended up doing, something like:

        if (cmpxchg_relaxed(&pn->state, vcpu_halted, vcpu_hashed) != vcpu_halted)
                return;

        WRITE_ONCE(l->locked, _Q_SLOW_VAL);

Where the STORE depends on the LL value being 'complete'.