Re: [PATCH v4 next 2/9] locking/osq_lock: Save the cpu number for 'prev' not the node address

[Peter Zijlstra]

On Tue, Sep 15, 2026 at 10:33:40AM +0200, Peter Zijlstra wrote:
> On Mon, Sep 07, 2026 at 09:41:26AM +0100, David Laight wrote:
> > The cpu number of node->prev is needed for both the vcpu_is_preempted()
> > test and to update lock->tail.
> > This saves reading the cache line for the other cpu's per-cpu data.
> > 
> > The cpu member of optimistic_spin_node is no longer needed.
> > 
> > Merges patches 2 and 3 from v3.
> > 
> > Signed-off-by: David Laight <david.laight.linux@xxxxxxxxx>
> > ---
> >  kernel/locking/osq_lock.c | 33 ++++++++++++++-------------------
> >  1 file changed, 14 insertions(+), 19 deletions(-)
> > 
> > diff --git a/kernel/locking/osq_lock.c b/kernel/locking/osq_lock.c
> > index b17aa704c449..01988d00c480 100644
> > --- a/kernel/locking/osq_lock.c
> > +++ b/kernel/locking/osq_lock.c
> > @@ -34,9 +34,9 @@
> >   */
> >  
> >  struct optimistic_spin_node {
> > -	struct optimistic_spin_node *next, *prev;
> > +	struct optimistic_spin_node *next;
> >  	int locked; /* 1 if lock acquired */
> > -	int cpu; /* encoded CPU # + 1 value */
> > +	int prev; /* CPU number offset by 1 */
> >  };
> >  
> >  static DEFINE_PER_CPU_SHARED_ALIGNED(struct optimistic_spin_node, osq_node);
> 
> > @@ -114,13 +109,12 @@ osq_wait_next(struct optimistic_spin_queue *lock,
> >  bool osq_lock(struct optimistic_spin_queue *lock)
> >  {
> >  	struct optimistic_spin_node *node = this_cpu_ptr(&osq_node);
> > -	struct optimistic_spin_node *prev, *next;
> > +	struct optimistic_spin_node *prev_ptr, *next;
> >  	int curr = encode_cpu(smp_processor_id());
> > -	int old;
> > +	int prev;
> 
> I'm not a fan in the asymmetry of the naming, that is very confusing at
> best.

Maybe something like so?

---
--- a/kernel/locking/osq_lock.c
+++ b/kernel/locking/osq_lock.c
@@ -34,9 +34,9 @@
  */
 
 struct optimistic_spin_node {
-	struct optimistic_spin_node *next, *prev;
+	struct optimistic_spin_node *next;
 	int locked; /* 1 if lock acquired */
-	int cpu; /* encoded CPU # + 1 value */
+	int prev_cpu; /* CPU number offset by 1 */
 };
 
 static DEFINE_PER_CPU_SHARED_ALIGNED(struct optimistic_spin_node, osq_node);
@@ -50,11 +50,6 @@ static inline int encode_cpu(int cpu_nr)
 	return cpu_nr + 1;
 }
 
-static inline int node_cpu(struct optimistic_spin_node *node)
-{
-	return node->cpu - 1;
-}
-
 static inline struct optimistic_spin_node *decode_cpu(int encoded_cpu_val)
 {
 	int cpu_nr = encoded_cpu_val - 1;
@@ -116,11 +111,10 @@ bool osq_lock(struct optimistic_spin_que
 	struct optimistic_spin_node *node = this_cpu_ptr(&osq_node);
 	struct optimistic_spin_node *prev, *next;
 	int curr = encode_cpu(smp_processor_id());
-	int old;
+	int prev_cpu;
 
 	node->locked = 0;
 	node->next = NULL;
-	node->cpu = curr;
 
 	/*
 	 * We need both ACQUIRE (pairs with corresponding RELEASE in
@@ -128,12 +122,12 @@ bool osq_lock(struct optimistic_spin_que
 	 * the node fields we just initialised) semantics when updating
 	 * the lock tail.
 	 */
-	old = atomic_xchg(&lock->tail, curr);
-	if (old == OSQ_UNLOCKED_VAL)
+	prev_cpu = atomic_xchg(&lock->tail, curr);
+	if (prev_cpu == OSQ_UNLOCKED_VAL)
 		return true;
 
-	prev = decode_cpu(old);
-	node->prev = prev;
+	prev = decode_cpu(prev_cpu);
+	node->prev_cpu = prev_cpu;
 
 	/*
 	 * osq_lock()			unqueue
@@ -165,7 +159,7 @@ bool osq_lock(struct optimistic_spin_que
 	 * polling, be careful.
 	 */
 	if (smp_cond_load_relaxed(&node->locked, VAL || need_resched() ||
-				  vcpu_is_preempted(node_cpu(node->prev))))
+				  vcpu_is_preempted(node->prev_cpu - 1)))
 		return true;
 
 	/* unqueue */
@@ -200,7 +194,8 @@ bool osq_lock(struct optimistic_spin_que
 		 * Or we race against a concurrent unqueue()'s step-B, in which
 		 * case its step-C will write us a new @node->prev pointer.
 		 */
-		prev = READ_ONCE(node->prev);
+		prev_cpu = READ_ONCE(node->prev_cpu);
+		prev = decode_cpu(prev_cpu);
 	}
 
 	/*
@@ -210,7 +205,7 @@ bool osq_lock(struct optimistic_spin_que
 	 * back to @prev.
 	 */
 
-	next = osq_wait_next(lock, node, prev->cpu);
+	next = osq_wait_next(lock, node, prev_cpu);
 	if (!next)
 		return false;
 
@@ -222,7 +217,7 @@ bool osq_lock(struct optimistic_spin_que
 	 * it will wait in Step-A.
 	 */
 
-	WRITE_ONCE(next->prev, prev);
+	WRITE_ONCE(next->prev_cpu, prev_cpu);
 	WRITE_ONCE(prev->next, next);
 
 	return false;