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

[Waiman Long]

On 9/10/26 11:30 AM, Haakon Bugge wrote:
> > 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.

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