This is a continuation of some patches I wrote over two years ago.
They go a lot further and reduce the per-cpu data to a structure
that only contains two cpu numbers.

I've fixed some broken/missing memory barriers but left the initial xchg()
when acquiring the lock as a full barrier, I think it could be relaxed.

Tested with a userspace harness that can conditionally sleep at various points.

David Laight (9):
  locking/osq_lock: Add some comments about how it works
  locking/osq_lock: Save the cpu number for 'prev' not the node address
  locking/osq_lock: Set prev_cpu=0 instead of locked=1
  locking/osq_lock: Delete 'fast path' code from osq_unlock()
  locking/osq_lock: Avoid writing to node->next in the osq_lock() fast
    path
  locking/osq: Use cpu number for 'next' pointer
  locking/osq: Use 'unsigned int' for next/prev/tail
  locking/osq: inline encode_cpu() and rename decode_cpu()
  locking/osq_lock: Swap next<->prev and tail<->head

 include/linux/osq_lock.h  |  10 +-
 kernel/locking/osq_lock.c | 286 ++++++++++++++++++++------------------
 2 files changed, 154 insertions(+), 142 deletions(-)

-- 
2.39.5

On Mon, 7 Sept 2026 at 01:41, David Laight <david.laight.linux@gmail.com> wrote:
>
> I've fixed some broken/missing memory barriers but left the initial xchg()
> when acquiring the lock as a full barrier, I think it could be relaxed.

Well, it should almost certainly be at least an
atomic_cmpxchg_acquire(), since that's what osq_wait_next() uses for
the contention case.

It's a bit odd that the first initial xchg uses a different memory
ordering than the later one. Maybe there's some reason for it.

But even more importantly, that code right now  explicitly *states*
that it needs a full barrier ("We need both ACQUIRE [..] and
RELEASE"), so that *comment* would also have to be fixed with a why
the ordering isn't as important as it states.

And finally: none of that will ever be noticeable on x86, since there
are no memory orderings on atomics there: lock is all-or-nothing.

End result: I'd love to see actual performance numbers if they exist.
And any memory ordering change would require explaining why it's ok
and some other architecture to test it.

Or am I missing something?

            Linus

On Mon, 7 Sep 2026 09:08:28 -0700
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Mon, 7 Sept 2026 at 01:41, David Laight <david.laight.linux@gmail.com> wrote:
> >
> > I've fixed some broken/missing memory barriers but left the initial xchg()
> > when acquiring the lock as a full barrier, I think it could be relaxed.  
> 
> Well, it should almost certainly be at least an
> atomic_cmpxchg_acquire(), since that's what osq_wait_next() uses for
> the contention case.

I'm not sure, but am no expert on acquire/release barriers.
The 'fast path' osq_lock() code only has one memory access so there
isn't anything to sequence it with.
The important one is the smp_wmb() a bit lower down that ensures the
list tail (or head) is written before the back link.
When that was missing things went badly wrong.
(I think the WRITE_ONCE() could be a store_release() instead.)

The ACQUIRE semantics were added to ensure the 'node->next = NULL'
assignment happened before the xchg().
That assignment goes away in patch 5.
But I'd want someone who really understands arm64 to comment.

> 
> It's a bit odd that the first initial xchg uses a different memory
> ordering than the later one. Maybe there's some reason for it.

I think the 'entry' ones want to be acquire and the 'exit' ones release.
osq_unlock() used release, but the equivalent code in osq_wait_next()
used acquire.
They can't both have been correct!

> 
> But even more importantly, that code right now  explicitly *states*
> that it needs a full barrier ("We need both ACQUIRE [..] and
> RELEASE"), so that *comment* would also have to be fixed with a why
> the ordering isn't as important as it states.

I left that comment alone - matching the xchg().
Even though there are now no fields to publish.

> And finally: none of that will ever be noticeable on x86, since there
> are no memory orderings on atomics there: lock is all-or-nothing.

Indeed.
I don't have a little arm test system, never mind a big one where this
would all show up.

> End result: I'd love to see actual performance numbers if they exist.
> And any memory ordering change would require explaining why it's ok
> and some other architecture to test it.

This could even be one of the strange places where making the code
slower actually speeds things up overall.
osq_lock() is only used for contended sleep locks, and then not even for
the first thread to be waiting.
If you get a lot of threads queued you really need to fix the locking!

> 
> Or am I missing something?

Probably the same thing as I am....

David

> 
>             Linus

> On 7 Sep 2026, at 19:27, David Laight <david.laight.linux@gmail.com> wrote:
> 
> On Mon, 7 Sep 2026 09:08:28 -0700
> Linus Torvalds <torvalds@linux-foundation.org> wrote:
> 
>> On Mon, 7 Sept 2026 at 01:41, David Laight <david.laight.linux@gmail.com> wrote:
>>> 
>>> I've fixed some broken/missing memory barriers but left the initial xchg()
>>> when acquiring the lock as a full barrier, I think it could be relaxed.  
>> 
>> Well, it should almost certainly be at least an
>> atomic_cmpxchg_acquire(), since that's what osq_wait_next() uses for
>> the contention case.
> 
> I'm not sure, but am no expert on acquire/release barriers.
> The 'fast path' osq_lock() code only has one memory access so there
> isn't anything to sequence it with.
> The important one is the smp_wmb() a bit lower down that ensures the
> list tail (or head) is written before the back link.
> When that was missing things went badly wrong.
> (I think the WRITE_ONCE() could be a store_release() instead.)
> 
> The ACQUIRE semantics were added to ensure the 'node->next = NULL'
> assignment happened before the xchg().
> That assignment goes away in patch 5.
> But I'd want someone who really understands arm64 to comment.

These are preliminary results. I added osq_lock's to my
mutual-exclusion selftest [1], which has not yet been reviewed. The
test is based on v7.3-rc2.

For lock acquisition, I used:

	preempt_disable();
	while (!osq_lock(&el->mx_osq_lock.lock)) {
		preempt_enable();
		cond_resched();
		preempt_disable();
	}

with the corresponding release:

	osq_unlock(&el->mx_osq_lock.lock);
	preempt_enable();

Assuming that this is a correct use of the OSQ API, the OSQ test fails
on a 160-CPU bare-metal Arm system. The same test passes on a 512-CPU
AMD x86_64 system as expected, showing at least that the test is
capable of passing.

I then applied this series, but the OSQ test still failed in the same
way on Arm. I observed no new mutex or rwsem test failures on Arm, and
the test continued to pass on the x86_64 system.


Thxs, Håkon

[1] https://lore.kernel.org/lkml/20260817130239.343594-1-haakon.bugge@oracle.com/


> 
>> 
>> It's a bit odd that the first initial xchg uses a different memory
>> ordering than the later one. Maybe there's some reason for it.
> 
> I think the 'entry' ones want to be acquire and the 'exit' ones release.
> osq_unlock() used release, but the equivalent code in osq_wait_next()
> used acquire.
> They can't both have been correct!
> 
>> 
>> But even more importantly, that code right now  explicitly *states*
>> that it needs a full barrier ("We need both ACQUIRE [..] and
>> RELEASE"), so that *comment* would also have to be fixed with a why
>> the ordering isn't as important as it states.
> 
> I left that comment alone - matching the xchg().
> Even though there are now no fields to publish.
> 
>> And finally: none of that will ever be noticeable on x86, since there
>> are no memory orderings on atomics there: lock is all-or-nothing.
> 
> Indeed.
> I don't have a little arm test system, never mind a big one where this
> would all show up.
> 
>> End result: I'd love to see actual performance numbers if they exist.
>> And any memory ordering change would require explaining why it's ok
>> and some other architecture to test it.
> 
> This could even be one of the strange places where making the code
> slower actually speeds things up overall.
> osq_lock() is only used for contended sleep locks, and then not even for
> the first thread to be waiting.
> If you get a lot of threads queued you really need to fix the locking!
> 
>> 
>> Or am I missing something?
> 
> Probably the same thing as I am....
> 
> David
> 
>> 
>>            Linus

On 9/9/26 10:15 AM, Haakon Bugge wrote:
>
>> On 7 Sep 2026, at 19:27, David Laight <david.laight.linux@gmail.com> wrote:
>>
>> On Mon, 7 Sep 2026 09:08:28 -0700
>> Linus Torvalds <torvalds@linux-foundation.org> wrote:
>>
>>> On Mon, 7 Sept 2026 at 01:41, David Laight <david.laight.linux@gmail.com> wrote:
>>>> I've fixed some broken/missing memory barriers but left the initial xchg()
>>>> when acquiring the lock as a full barrier, I think it could be relaxed.
>>> Well, it should almost certainly be at least an
>>> atomic_cmpxchg_acquire(), since that's what osq_wait_next() uses for
>>> the contention case.
>> I'm not sure, but am no expert on acquire/release barriers.
>> The 'fast path' osq_lock() code only has one memory access so there
>> isn't anything to sequence it with.
>> The important one is the smp_wmb() a bit lower down that ensures the
>> list tail (or head) is written before the back link.
>> When that was missing things went badly wrong.
>> (I think the WRITE_ONCE() could be a store_release() instead.)
>>
>> The ACQUIRE semantics were added to ensure the 'node->next = NULL'
>> assignment happened before the xchg().
>> That assignment goes away in patch 5.
>> But I'd want someone who really understands arm64 to comment.
> These are preliminary results. I added osq_lock's to my
> mutual-exclusion selftest [1], which has not yet been reviewed. The
> test is based on v7.3-rc2.
>
> For lock acquisition, I used:
>
> 	preempt_disable();
> 	while (!osq_lock(&el->mx_osq_lock.lock)) {
> 		preempt_enable();
> 		cond_resched();
> 		preempt_disable();
> 	}
>
> with the corresponding release:
>
> 	osq_unlock(&el->mx_osq_lock.lock);
> 	preempt_enable();
>
> Assuming that this is a correct use of the OSQ API, the OSQ test fails
> on a 160-CPU bare-metal Arm system. The same test passes on a 512-CPU
> AMD x86_64 system as expected, showing at least that the test is
> capable of passing.

osq_unlock() must provide the release barrier. I think the two 
"WRITE_ONCE(next->locked, 1)" should have been 
"smp_store_release(&next->locked, 1)".  There is an xchg() call before 
the WRITE_ONCE's, but it is on a different cacheline so it may not apply.

Could you make that change to the existing code and rerun the test again 
on arm64 to see if it can pass?

Thanks,
Longman

On 9/9/26 4:14 PM, Waiman Long wrote:
> On 9/9/26 10:15 AM, Haakon Bugge wrote:
>>
>>> On 7 Sep 2026, at 19:27, David Laight <david.laight.linux@gmail.com> 
>>> wrote:
>>>
>>> On Mon, 7 Sep 2026 09:08:28 -0700
>>> Linus Torvalds <torvalds@linux-foundation.org> wrote:
>>>
>>>> On Mon, 7 Sept 2026 at 01:41, David Laight 
>>>> <david.laight.linux@gmail.com> wrote:
>>>>> I've fixed some broken/missing memory barriers but left the 
>>>>> initial xchg()
>>>>> when acquiring the lock as a full barrier, I think it could be 
>>>>> relaxed.
>>>> Well, it should almost certainly be at least an
>>>> atomic_cmpxchg_acquire(), since that's what osq_wait_next() uses for
>>>> the contention case.
>>> I'm not sure, but am no expert on acquire/release barriers.
>>> The 'fast path' osq_lock() code only has one memory access so there
>>> isn't anything to sequence it with.
>>> The important one is the smp_wmb() a bit lower down that ensures the
>>> list tail (or head) is written before the back link.
>>> When that was missing things went badly wrong.
>>> (I think the WRITE_ONCE() could be a store_release() instead.)
>>>
>>> The ACQUIRE semantics were added to ensure the 'node->next = NULL'
>>> assignment happened before the xchg().
>>> That assignment goes away in patch 5.
>>> But I'd want someone who really understands arm64 to comment.
>> These are preliminary results. I added osq_lock's to my
>> mutual-exclusion selftest [1], which has not yet been reviewed. The
>> test is based on v7.3-rc2.
>>
>> For lock acquisition, I used:
>>
>>     preempt_disable();
>>     while (!osq_lock(&el->mx_osq_lock.lock)) {
>>         preempt_enable();
>>         cond_resched();
>>         preempt_disable();
>>     }
>>
>> with the corresponding release:
>>
>>     osq_unlock(&el->mx_osq_lock.lock);
>>     preempt_enable();
>>
>> Assuming that this is a correct use of the OSQ API, the OSQ test fails
>> on a 160-CPU bare-metal Arm system. The same test passes on a 512-CPU
>> AMD x86_64 system as expected, showing at least that the test is
>> capable of passing.
>
> osq_unlock() must provide the release barrier. I think the two 
> "WRITE_ONCE(next->locked, 1)" should have been 
> "smp_store_release(&next->locked, 1)".  There is an xchg() call before 
> the WRITE_ONCE's, but it is on a different cacheline so it may not apply.
>
> 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);
  }

> On 9 Sep 2026, at 22:33, Waiman Long <longman@redhat.com> 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:

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

If you submit this patch, feel free to add:

Tested-by: Håkon Bugge <haakon.bugge@oracle.com>


Thxs, Håkon

On Thu, 10 Sep 2026 09:45:47 +0000
Haakon Bugge <haakon.bugge@oracle.com> wrote:

> > On 9 Sep 2026, at 22:33, Waiman Long <longman@redhat.com> 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:

Do you know which part matters?

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

> If you submit this patch, feel free to add:

I'll roll it into my patches (as 1/n).

David

> 
> Tested-by: Håkon Bugge <haakon.bugge@oracle.com>
> 
> 
> Thxs, Håkon
> 
> 

> On Thu, 10 Sep 2026 09:45:47 +0000
> Haakon Bugge <haakon.bugge@oracle.com> wrote:
> 
> > > On 9 Sep 2026, at 22:33, Waiman Long <longman@redhat.com> 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.

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

> > # 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"

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


Thxs, Håkon

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

...
> What concerns me is that I am unable to observe this bug testing
> mutexes or rwlocks.

That might be because there is only one osq_lock so you always get
contention - whereas the other cases have separate locks.
You do still need to hit the same array element for it to matter.

I'm not sure is makes much sense to have a lot of locks/counters.

David

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

> > On Thu, 10 Sep 2026 09:45:47 +0000
> > Haakon Bugge <haakon.bugge@oracle.com> wrote:
> >   
> > > > On 9 Sep 2026, at 22:33, Waiman Long <longman@redhat.com> 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
> 

> On 10 Sep 2026, at 14:05, David Laight <david.laight.linux@gmail.com> wrote:
> On Thu, 10 Sep 2026 11:31:19 +0000
> Haakon Bugge <haakon.bugge@oracle.com> wrote:
> 
> > > On Thu, 10 Sep 2026 09:45:47 +0000
> > > Haakon Bugge <haakon.bugge@oracle.com> wrote:
> > > 
> > > > > On 9 Sep 2026, at 22:33, Waiman Long <longman@redhat.com> 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

On Thu, 10 Sep 2026 15:30:03 +0000
Haakon Bugge <haakon.bugge@oracle.com> wrote:

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

Indeed.

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

Isn't that accessing another branch of the 'big union'?
So will be overlaying some other field - possibly the count itself?
It seems unlikely that adding a barrier between the lock and unlock
(or unlock and lock) will have any effect.
In any case I think it is enough to use smp_rmb(), smp_wmb() or smp_mb().

David

> On 11 Sep 2026, at 19:15, David Laight <david.laight.linux@gmail.com> wrote:
> On Thu, 10 Sep 2026 15:30:03 +0000
> Haakon Bugge <haakon.bugge@oracle.com> wrote:

[snip]

> > 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.
> 
> Isn't that accessing another branch of the 'big union'?

Not sure I quite understand. During the run of one particular test, it
is _only_ the corresponding struct in the union that is
accessed. 

> So will be overlaying some other field - possibly the count itself?

pahole gives:

struct {
	atomic_t           lock __attribute__((__aligned__(4))); /*     0     4 */
	/* XXX 4 bytes hole, try to pack */
	long int           counter;            /*     8     8 */
} __attribute__((__aligned__(8))) mx_atomic_xchg __attribute__((__aligned__(8)));  /*     0    16 */

> It seems unlikely that adding a barrier between the lock and unlock
> (or unlock and lock) will have any effect.
> In any case I think it is enough to use smp_rmb(), smp_wmb() or smp_mb().

That might well be, but the intent here was only to mimic
the sequence atomic_xchg() in osq_lock() and corresponding
atomic_try_cmpxchg_release() in osq_unlock().


Thxs, Håkon

On Mon, 14 Sep 2026 11:46:02 +0000
Haakon Bugge <haakon.bugge@oracle.com> wrote:

> > On 11 Sep 2026, at 19:15, David Laight <david.laight.linux@gmail.com> wrote:
> > On Thu, 10 Sep 2026 15:30:03 +0000
> > Haakon Bugge <haakon.bugge@oracle.com> wrote:  
> 
> [snip]
> 
> > > 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.  
> > 
> > Isn't that accessing another branch of the 'big union'?  
> 
> Not sure I quite understand. During the run of one particular test, it
> is _only_ the corresponding struct in the union that is
> accessed. 

I misunderstood what you'd done.
I thought you were adding an extra barrier to the osq_lock test.
One of the possibilities is that it is speculating all the way through
into the osq_unlock() code and the first read there is being done
before the read of node->locked in smp_cond_load_relaxed().
If adding smp_rmb() between osq_lock() and osq_unlock() doesn't fix
it then that isn't the problem.

This code really wants the cpu to speculate around the loop in
smp_cond_load_xxx() so that it doesn't load any other cache lines.
(I've not looked at the object code (any architecture) to try to guess
what happens.)

David

> 
> > So will be overlaying some other field - possibly the count itself?  
> 
> pahole gives:
> 
> struct {
> 	atomic_t           lock __attribute__((__aligned__(4))); /*     0     4 */
> 	/* XXX 4 bytes hole, try to pack */
> 	long int           counter;            /*     8     8 */
> } __attribute__((__aligned__(8))) mx_atomic_xchg __attribute__((__aligned__(8)));  /*     0    16 */
> 
> > It seems unlikely that adding a barrier between the lock and unlock
> > (or unlock and lock) will have any effect.
> > In any case I think it is enough to use smp_rmb(), smp_wmb() or smp_mb().  
> 
> That might well be, but the intent here was only to mimic
> the sequence atomic_xchg() in osq_lock() and corresponding
> atomic_try_cmpxchg_release() in osq_unlock().
> 
> 
> Thxs, Håkon
> 

On 9/10/26 11:30 AM, Haakon Bugge wrote:
>
>> On 10 Sep 2026, at 14:05, David Laight <david.laight.linux@gmail.com> wrote:
>> On Thu, 10 Sep 2026 11:31:19 +0000
>> Haakon Bugge <haakon.bugge@oracle.com> wrote:
>>
>>>> On Thu, 10 Sep 2026 09:45:47 +0000
>>>> Haakon Bugge <haakon.bugge@oracle.com> wrote:
>>>>
>>>>>> On 9 Sep 2026, at 22:33, Waiman Long <longman@redhat.com> 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.

The purpose of osq_lock is for queuing the lock waiters with minimal 
contention on the lock cacheline. Even when the locking semantics isn't 
fully correct, it won't have an ill effect on the locking behavior of 
rwsem and mutex.  We may have 2 waiters spinning on the lock cacheline 
instead of one, for instance.

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

Yes, as said in my patch, the other two hunks are not really necessary 
for arm64 due to what how its barriers work.

Cheers,
Longman

On Wed, 9 Sep 2026 14:15:03 +0000
Haakon Bugge <haakon.bugge@oracle.com> wrote:

> > On 7 Sep 2026, at 19:27, David Laight <david.laight.linux@gmail.com> wrote:
> > 
> > On Mon, 7 Sep 2026 09:08:28 -0700
> > Linus Torvalds <torvalds@linux-foundation.org> wrote:
> >   
> >> On Mon, 7 Sept 2026 at 01:41, David Laight <david.laight.linux@gmail.com> wrote:  
> >>> 
> >>> I've fixed some broken/missing memory barriers but left the initial xchg()
> >>> when acquiring the lock as a full barrier, I think it could be relaxed.    
> >> 
> >> Well, it should almost certainly be at least an
> >> atomic_cmpxchg_acquire(), since that's what osq_wait_next() uses for
> >> the contention case.  
> > 
> > I'm not sure, but am no expert on acquire/release barriers.
> > The 'fast path' osq_lock() code only has one memory access so there
> > isn't anything to sequence it with.
> > The important one is the smp_wmb() a bit lower down that ensures the
> > list tail (or head) is written before the back link.
> > When that was missing things went badly wrong.
> > (I think the WRITE_ONCE() could be a store_release() instead.)
> > 
> > The ACQUIRE semantics were added to ensure the 'node->next = NULL'
> > assignment happened before the xchg().
> > That assignment goes away in patch 5.
> > But I'd want someone who really understands arm64 to comment.  
> 
> These are preliminary results. I added osq_lock's to my
> mutual-exclusion selftest [1], which has not yet been reviewed. The
> test is based on v7.3-rc2.
> 
> For lock acquisition, I used:
> 
> 	preempt_disable();
> 	while (!osq_lock(&el->mx_osq_lock.lock)) {
> 		preempt_enable();
> 		cond_resched();
> 		preempt_disable();
> 	}
> 
> with the corresponding release:
> 
> 	osq_unlock(&el->mx_osq_lock.lock);
> 	preempt_enable();
> 
> Assuming that this is a correct use of the OSQ API,

Looks reasonable.
I doubt the rwsem code ever stresses it that much.

> the OSQ test fails on a 160-CPU bare-metal Arm system.

The inter-cpu delays will definitely show up any memory ordering issues.
I don't have access to anything of that nature.

> The same test passes on a 512-CPU
> AMD x86_64 system as expected, showing at least that the test is
> capable of passing.
> 
> I then applied this series, but the OSQ test still failed in the same
> way on Arm. I observed no new mutex or rwsem test failures on Arm, and
> the test continued to pass on the x86_64 system.

At least I haven't made it worse :-)
Might be worth removing all the _release and _acquire (so all the xchg
become full barriers) to see if that makes a difference.

For testing you want the option of compiling a separate copy of the lock
code into the module itself.
Then you can test changes to the lock code as well as changes to the
test itself.
I did that for mul_u64_add_u64_div_u64() so I could test the 32bit code
on x86-64.
It required some pretty horrid #defines - and I missed redefining
EXPORT_SYMBOL() to be a no-op.

David

> 
> 
> Thxs, Håkon
> 
> [1] https://lore.kernel.org/lkml/20260817130239.343594-1-haakon.bugge@oracle.com/
> 
> 
> >   
> >> 
> >> It's a bit odd that the first initial xchg uses a different memory
> >> ordering than the later one. Maybe there's some reason for it.  
> > 
> > I think the 'entry' ones want to be acquire and the 'exit' ones release.
> > osq_unlock() used release, but the equivalent code in osq_wait_next()
> > used acquire.
> > They can't both have been correct!
> >   
> >> 
> >> But even more importantly, that code right now  explicitly *states*
> >> that it needs a full barrier ("We need both ACQUIRE [..] and
> >> RELEASE"), so that *comment* would also have to be fixed with a why
> >> the ordering isn't as important as it states.  
> > 
> > I left that comment alone - matching the xchg().
> > Even though there are now no fields to publish.
> >   
> >> And finally: none of that will ever be noticeable on x86, since there
> >> are no memory orderings on atomics there: lock is all-or-nothing.  
> > 
> > Indeed.
> > I don't have a little arm test system, never mind a big one where this
> > would all show up.
> >   
> >> End result: I'd love to see actual performance numbers if they exist.
> >> And any memory ordering change would require explaining why it's ok
> >> and some other architecture to test it.  
> > 
> > This could even be one of the strange places where making the code
> > slower actually speeds things up overall.
> > osq_lock() is only used for contended sleep locks, and then not even for
> > the first thread to be waiting.
> > If you get a lot of threads queued you really need to fix the locking!
> >   
> >> 
> >> Or am I missing something?  
> > 
> > Probably the same thing as I am....
> > 
> > David
> >   
> >> 
> >>            Linus  
> 
>