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

[Waiman Long]

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@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, 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);
 }