Re: [PATCH v4 next 0/9] locking/osq_lock: Optimisations to osq_lock code

[David Laight]

On Thu, 10 Sep 2026 11:31:19 +0000
Haakon Bugge <haakon.bugge@xxxxxxxxxx> wrote:

> > On Thu, 10 Sep 2026 09:45:47 +0000
> > Haakon Bugge <haakon.bugge@xxxxxxxxxx> wrote:
> >   
> > > > On 9 Sep 2026, at 22:33, Waiman Long <longman@xxxxxxxxxx> wrote:    
> > > 
> > > [snip]
> > >   
> > > > > Could you make that change to the existing code and rerun the test 
> > > > > again on arm64 to see if it can pass?     
> > > > 
> > > > osq_lock/unlock() is special in the sense that lock transfer can happen 
> > > > either in the lock cacheline or the node->locked cacheline. Try the 
> > > > patch below to see if it helps to pass the test.
> > > > 
> > > > Thanks,
> > > > Longman
> > > > 
> > > > diff --git a/kernel/locking/osq_lock.c b/kernel/locking/osq_lock.c
> > > > index b4233dc2c2b0..51cecf297692 100644
> > > > --- a/kernel/locking/osq_lock.c
> > > > +++ b/kernel/locking/osq_lock.c
> > > > @@ -143,7 +143,7 @@ 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() ||
> > > > +       if (smp_cond_load_acquire(&node->locked, VAL || need_resched() ||
> > > > vcpu_is_preempted(node_cpu(node->prev))))
> > > >                 return true;
> > > > 
> > > > @@ -224,11 +224,11 @@ void osq_unlock(struct optimistic_spin_queue *lock)
> > > >         node = this_cpu_ptr(&osq_node);
> > > >         next = xchg(&node->next, NULL);
> > > >         if (next) {
> > > > -               WRITE_ONCE(next->locked, 1);
> > > > +               smp_store_release(&next->locked, 1);
> > > >                 return;
> > > >         }
> > > > 
> > > >         next = osq_wait_next(lock, node, OSQ_UNLOCKED_VAL);
> > > >         if (next)
> > > > -               WRITE_ONCE(next->locked, 1);
> > > > +               smp_store_release(&next->locked, 1);
> > > >  }    
> > > 
> > > The test passes with the above patch:  
> 
> Confirming that a much more thorough test (permutating the test array
> size and padding) passed.
> 
> What concerns me is that I am unable to observe this bug testing
> mutexes or rwlocks.

The explicit test will be a lot more aggressive.
Especially if the lock hold time matters.

> 
> > Do you know which part matters?  
> 
> No, but now that I am able to test the OSQ locks as a module, I'll
> quickly find out.

That also means you can quickly check which _acquire/_release are definitely
required.
I'm pretty sure that (with my patches) the initial xchg() at the top of
osq_lock() only needs _acquire (_release was added to publish node->cpu).
But I think it doesn't even need _acquire.
osq_lock() itself relies on a data dependency.
Any concurrent osq_lock() relies on the smp_wmp() a bit further down
(I think that could be a store_release).

> 
> > > # dmesg|grep mx
> > > [    7.010502] mx_test: osq_lock                    padding:  8 result: SUCCESS sum: 0 elements: 1000 elapsed: 5.000 seconds
> > > [   12.014144] mx_test: osq_lock                    padding: 16 result: SUCCESS sum: 0 elements: 1000 elapsed: 5.004 seconds
> > > [   17.017572] mx_test: osq_lock                    padding: 24 result: SUCCESS sum: 0 elements: 1000 elapsed: 5.004 seconds
> > > [   22.019636] mx_test: osq_lock                    padding: 32 result: SUCCESS sum: 0 elements: 1000 elapsed: 5.004 seconds
> > > [   27.022192] mx_test: osq_lock                    padding: 40 result: SUCCESS sum: 0 elements: 1000 elapsed: 5.000 seconds
> > > [   32.024907] mx_test: osq_lock                    padding: 48 result: SUCCESS sum: 0 elements: 1000 elapsed: 5.004 seconds
> > > [   37.026035] mx_test: osq_lock                    padding: 56 result: SUCCESS sum: 0 elements: 1000 elapsed: 5.000 seconds
> > > 
> > > I'll use David's advise about including the osq_lock code in my test,
> > > so I can test it as a module, which will be more thorough.  
> > 
> > At least with a build/run option...  
> 
> Yes, I'll send a v2 of my mx_test including OSQ locks both as
> compiled-in and as a module. For the latter, I just did:
> 
> #include "osq_lock.c"

I tried to rename everything just to be certain the correct functions
are called.

> > > If you submit this patch, feel free to add:  
> > 
> > I'll roll it into my patches (as 1/n).  
> 
> Be aware that Waiman's patch did not apply on the top of your series,
> so the testing is solely v7.3-rc2 plus Waiman's patch.

The equivalent changes should be obvious.
Note that I merged the unlock and lock-fail paths.

David

> 
> 
> Thxs, Håkon
>