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

[David Laight]

On Tue, 15 Sep 2026 12:26:02 +0200
Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:

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

I was trying to avoid changing lines that weren't affected by the patch.
So I left the original variable names alone.

Every time I come back to this code it makes my head hurt.
Perhaps I should do an early patch to rename everything to this/prev/next_cpu/ptr
(although maybe without the _ptr).

At one point I considered using OSQ_UNLOCKED_VAL (instead of 0) for
node->prev/next_cpu but decided against it because it make the code
even harder to read.
I think it was added when the list tail was changed from a pointer to
the cpu number to reduce its size - the value changed from NULL to 0.
Having a non-zero initialiser (eg -1) is just asking for trouble!

David
 
> 
> ---
> --- 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;