Re: [PATCH v4 next 3/9] locking/osq_lock: Set prev_cpu=0 instead of locked=1

[David Laight <david.laight.linux@gmail.com>]

On Wed, 9 Sep 2026 14:01:59 -0400
Waiman Long <longman@redhat.com> wrote:

> On 9/7/26 4:41 AM, David Laight wrote:
> > There is no need for separate prev_cpu and locked members of
> > struct optimistic_spin_node.
> > Using a single field simplifies the code slightly.
> > It also removes any possibility of the two values being out of sync.
> >
> > When cancelling a lock request explicitly set prev_cpu to zero.
> > Nothing actually looks at the field, but it means that it will be zero
> > after a subsequent 'fast path' osq_lock() call making things consistent.
> > The cache line is likely to be dirty (or be dirtied) so there shouldn't
> > be a performance hit.
> >
> > Signed-off-by: David Laight <david.laight.linux@gmail.com>
> > ---
> >   kernel/locking/osq_lock.c | 57 +++++++++++++++++++--------------------
> >   1 file changed, 28 insertions(+), 29 deletions(-)
> >
> > diff --git a/kernel/locking/osq_lock.c b/kernel/locking/osq_lock.c
> > index 01988d00c480..23f00c670507 100644
> > --- a/kernel/locking/osq_lock.c
> > +++ b/kernel/locking/osq_lock.c
> > @@ -35,7 +35,6 @@
> >   
> >   struct optimistic_spin_node {
> >   	struct optimistic_spin_node *next;
> > -	int locked; /* 1 if lock acquired */
> >   	int prev; /* CPU number offset by 1 */
> >   };
> >     
> I think we should document the fact that prev=0 can be viewed as a 
> marker that the osq lock has been acquired.

I think that happens a bit later in the series.
Trying to keep the comments in step is quite hard work.
That is part the reason why patch 1 partially describes 'where we are
aiming at'.
I also got in a slight mess with pointers that are cpu numbers.
Mostly trying to keep the comments concise.

> > @@ -113,7 +112,6 @@ bool osq_lock(struct optimistic_spin_queue *lock)
> >   	int curr = encode_cpu(smp_processor_id());
> >   	int prev;
> >   
> > -	node->locked = 0;
> >   	node->next = NULL;
> >   
> >   	/*
> > @@ -158,46 +156,47 @@ bool osq_lock(struct optimistic_spin_queue *lock)
> >   	 * is implemented with a monitor-wait. vcpu_is_preempted() relies on
> >   	 * polling, be careful.
> >   	 */
> > -	if (smp_cond_load_relaxed(&node->locked, VAL || need_resched() ||
> > -				  vcpu_is_preempted(node->prev - 1)))
> > -		return true;
> > +	prev = smp_cond_load_relaxed(&node->prev, !VAL || need_resched() ||
> > +				     vcpu_is_preempted(VAL - 1));
> >     
> What is the purpose of assigning the return value to prev and 
> immediately read node->prev into prev again in the next statement?

It isn't, it is only re-read when the code goes around the loop.

> > -	/* unqueue */
> >   	/*
> > -	 * Step - A  -- stabilize @prev
> > +	 * Step - A
> >   	 *
> > -	 * Undo our @prev->next assignment; this will make @prev's
> > -	 * unlock()/unqueue() wait for a next pointer since @lock points to us
> > -	 * (or later).
> > +	 * Loop until either node->prev is zero (lock acquired) or we
> > +	 * atomically change prev->next from node to NULL (stopping prev
> > +	 * handing on the lock).
> > +	 * Note that 'prev' can unlink itself concurrently with this
> > +	 * test so that prev/prev_ptr can be stale, but since it
> > +	 * is per-cpu data the memory can always be read.  
> I don't quite understand what you mean by "it is per-cpu data the memory 
> can always be read".
> >   	 */
> >   
> > -	for (;;) {
> > -		/*
> > -		 * cpu_relax() below implies a compiler barrier which would
> > -		 * prevent this comparison being optimized away.
> > -		 */
> > +	for (;; prev = READ_ONCE(node->prev)) {
> > +		if (!prev)
> > +			/* Lock acquired */
> > +			return true;
> > +
> > +		prev_ptr = decode_cpu(prev);
> > +
> >   		if (data_race(prev_ptr->next) == node &&
> >   		    cmpxchg(&prev_ptr->next, node, NULL) == node)
> >   			break;
> >   
> >   		/*
> > -		 * We can only fail the cmpxchg() racing against an unlock(),
> > -		 * in which case we should observe @node->locked becoming
> > -		 * true.
> > +		 * 'prev' must have unlinked (or be in the process of unlinking)
> > +		 * itself from the list.  
> 
> Should we also moved the deleted comment about cpu_relax() to here?

I nearly gave that comment its own paragraph in the commit message.
It was added when the data_race() was added (because of a KASAN splat).
The real comment should have been that the initial compare is only
there to make it less likely that an expensive atomic operation fails.
The loop now has a READ_ONCE() at the top so nothing can be lifted out
of the loop.

Thinking about it, I'm not even sure the test is worth while.
If 'prev' isn't zero/NULL then we expect to be able to unlink ourselves
from it - so the cmpxchg() is expected to succeed.
The extra compare only makes sense when there is a reasonably likely hood
of the cmpxchg failing.

I could add a patch to remove it/them.

David

> 
> Cheers,
> Longman
>