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

[Haakon Bugge]

> On 10 Sep 2026, at 14:05, David Laight <david.laight.linux@xxxxxxxxx> wrote:
> 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.

The algorithm is the same for all lock types. osq_lock failed, whereas mutex
and rwlock, based on osq_lock, passes. Weird.

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

Only the first hunk is allegedly required:

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

I say allegedly because a passing test doesn't prove anything, it just
gives a good indication that it is working.

> That also means you can quickly check which _acquire/_release are definitely
> required.

I did it in another way. I just added an "atomic_xchg" test to my
mutual exclusion framework.

Lock acquire:
                while (atomic_xchg_acquire(&el->mx_atomic_xchg.lock, 1) != 0)
                        cpu_relax();

Lock release:
                atomic_set_release(&el->mx_atomic_xchg.lock, 0);

This passes. It also (obviously) passes with atomic_xchg() in the lock
acquire. But the _release _is_ required in the lock release.

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

atomic_xchg_acquire() relaxes ordering as compared to
atomic_xchg(). So, atomic_xchg_acquire() in the top of osq_lock() and
atomic_try_cmpxchg_release() in osq_unlock() makes sense to me.

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

I put in a WARN_ON_ONCE() and it fired :-)

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

I like to change one thing at a time. FYI, I am unable to work on this
until Monday. I'll re-apply your series and Waiman's first hunk and
re-test.


Thxs, Håkon