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

[David Laight]

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

> > On 7 Sep 2026, at 19:27, David Laight <david.laight.linux@xxxxxxxxx> wrote:
> > 
> > On Mon, 7 Sep 2026 09:08:28 -0700
> > Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
> >   
> >> On Mon, 7 Sept 2026 at 01:41, David Laight <david.laight.linux@xxxxxxxxx> 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@xxxxxxxxxx/
> 
> 
> >   
> >> 
> >> 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  
> 
>