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

[David Laight]

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

> > On 11 Sep 2026, at 19:15, David Laight <david.laight.linux@xxxxxxxxx> wrote:
> > On Thu, 10 Sep 2026 15:30:03 +0000
> > Haakon Bugge <haakon.bugge@xxxxxxxxxx> 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
>