Re: [PATCH v3 next 5/5] Avoid writing to node->next in the osq_lock() fast path

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

On Wed, 11 Mar 2026 15:27:08 -0400
Waiman Long <longman@redhat.com> wrote:

> On 3/6/26 5:51 PM, david.laight.linux@gmail.com wrote:
> > From: David Laight <david.laight.linux@gmail.com>
> >
> > When osq_lock() returns false or osq_unlock() returns static
> > analysis shows that node->next should always be NULL.
> > This means that it isn't necessary to explicitly set it to NULL
> > prior to atomic_xchg(&lock->tail, curr) on entry to osq_lock().
> >
> > Defer determining the address of the CPU's 'node' until after the
> > atomic_exchange() so that it isn't done in the uncontented path.
> >
> > Signed-off-by: David Laight <david.laight.linux@gmail.com>
> > ---
> >   kernel/locking/osq_lock.c | 6 ++----
> >   1 file changed, 2 insertions(+), 4 deletions(-)
> >
> > diff --git a/kernel/locking/osq_lock.c b/kernel/locking/osq_lock.c
> > index 0619691e2756..3f0cfdf1cd0f 100644
> > --- a/kernel/locking/osq_lock.c
> > +++ b/kernel/locking/osq_lock.c
> > @@ -92,13 +92,10 @@ 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 *node, *prev, *next;
> >   	unsigned int curr = encode_cpu(smp_processor_id());
> >   	unsigned int prev_cpu;
> >   
> > -	node->next = NULL;  
> 
> Although it does look like node->next should always be NULL when 
> entering osq_lock(), any future change may invalidate this assumption. I 
> know you want to not touch the osq_node cacheline on fast path, but we 
> will need a big comment here to explicitly spell out this assumption to 
> make sure that we won't break it in the future.

I've been looking at the code some more.
The first new patch adds a load of comments.
Unfortunately they are rather wrong (or in the wrong place) by the time
I finished. So I need to go around again at least once.
The main extra changes:
- Set node->prev to zero instead of node->locked = 1.
  This makes the loop after the smp_cond_load_relaxed() better.
  And the write to next->prev can move into osq_wait_next().
- Just call osq_wait_next() from osq_unlock() - it is the same code.
  Removes the unconditional cmpxchg on lock->tail (which needs to be
  _release in both places).
- Change node->next to be the cpu number - it is only used as a pointer
  once (I think Linus suggested that once).
- Stop 'node' being cache-line aligned - it only contains two cpu numbers.
  Just 8 byte align so it is all in one cache line.
  The data for the 'wrong' cpu is hardly ever accessed.

> 
> BTW, how much performance gain have you measured with this change? Can 
> we just leave it there to be safe.

You think I've been brave enough to run my changes :-)
Actually I've written a little test harness that lets me compile and run
the actual kernel source in usespace and 'prod' it with requests.
That let me add delays at particular points to check the unusual paths
(but not the effects of acquire/release).

I will run the next changes, hopefully there won't be anything nasty in
the rc kernel to catch me out.
But I've no real idea of how to exercise the code.

	David

> 
> > -
> >   	/*
> >   	 * We need both ACQUIRE (pairs with corresponding RELEASE in
> >   	 * unlock() uncontended, or fastpath) and RELEASE (to publish
> > @@ -109,6 +106,7 @@ bool osq_lock(struct optimistic_spin_queue *lock)
> >   	if (prev_cpu == OSQ_UNLOCKED_VAL)
> >   		return true;
> >   
> > +	node = this_cpu_ptr(&osq_node);
> >   	WRITE_ONCE(node->prev_cpu, prev_cpu);
> >   	prev = decode_cpu(prev_cpu);
> >   	node->locked = 0;  
> 
> I am fine with moving the initialization here. The other patches also 
> look good to me.
> 
> Cheers,
> Longman
>