Re: [PATCH] locking/osq_lock: Ensure proper locking semantics for osq_lock/osq_unlock()

[Waiman Long <longman@redhat.com>]

On 9/14/26 5:53 AM, David Laight wrote:
> On Thu, 10 Sep 2026 10:19:08 -0400
> Waiman Long <longman@redhat.com> wrote:
>
>> The osq_lock is special in the sense that lock transfer from one CPU to
>> the next can happen either over the common optimistic_spin_queue.tail
>> value with uncontended lock or over a lock waiter's own percpu
>> optimistic_spin_node.locked flag when the lock is contended.
>>
>> To ensure proper lock synchronization, we need to provide
>> the acquire/release semantics for the osq_lock/osq_unlock()
>> functions in both cases. This is currently the case for the
>> common optimistic_spin_queue.tail value, but not for the percpu
>> optimistic_spin_node.locked flag as the proper barriers are missing in
>> some places. Fix that by adding the needed barriers in those places.
>>
>> Note that the two percpu optimistic_spin_node.locked setting in
>> osq_unlock() are proceeded by a full barrier xchg() call, but the
>> contended cachelines are different. This should probably work in most
>> cases except in some exotic architectures where the barrier semantics
>> may be cacheline specific. Nevertheless a release barrier is still added
>> for safety reason as we may opt to relax the xchg() calls in the future.
>>
>> The "node->locked" read in osq_lock() was relaxed by commit 036cc30c6b6a
>> ("locking/osq: No need for load/acquire when acquire-polling") a while
>> ago as the smp_load_acquire() loop was causing a performance hit due to
>> the repeated acquire barriers in the loop and it argued that an earlier
>> atomic_xchg() call could provide the needed barrier. That may not be
>> enough especially if we have to loop for a while before the lock is
>> released. Now with the new smp_cond_load_acquire() helper, only one
>> acquire barrier is added at the end of the loop. So it shouldn't have
>> the performance hit noted in that commit.
>>
>> Currently osq_lock is used only by mutex and rw_semaphore code for queuing
>> purpose. As a result, the imperfect lock synchronization support does
>> not cause harmful consequence as the new osq_lock owner of a contended
>> osq_lock will still have to wait for the real mutex and rwsem lock to
>> be released by the pervious osq_lock owner before it can acquire it and
>> go into its critical section. For correctness, we still have to fix it
>> in case it is used elsewhere which doesn't have this inherent protection.
>>
>> Fixes: 036cc30c6b6a ("locking/osq: No need for load/acquire when acquire-polling")
>> Tested-by: Håkon Bugge <haakon.bugge@oracle.com>
>> Signed-off-by: Waiman Long <longman@redhat.com>
>> ---
>>   kernel/locking/osq_lock.c | 11 +++++++----
>>   1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/kernel/locking/osq_lock.c b/kernel/locking/osq_lock.c
>> index b4233dc2c2b0..ef1bbd914917 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))))
> The comment above needs changing to match.
>
> David

Thank for noticing that. I update the comment in v3 patch.

Cheers,
Longman