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

[Waiman Long <longman@redhat.com>]

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