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

[Waiman Long <longman@redhat.com>]

On 9/15/26 4:26 AM, Peter Zijlstra wrote:
> On Mon, Sep 14, 2026 at 04:21:02PM -0400, Waiman Long 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 can be missing.
>>
>> 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 and reordering
>> wasn't a problem in the way osq_lock is being used by mutex and rwsem
>> for queuing purpose only. That atomic_xchg() barrier does not work
>> as a proper acquire barrier for osq_lock() if the lock hasn't been
>> acquired or isn't ready to be acquired when the barrier ends. So an
>> acquire barrier is still needed in order to have proper locking semantics.
>>
>> The performance impact stated in that patch is due to repeated issuance
>> of acquire barrier which can be expensive depending on the architectures
>> and the actual processor used. It was not clear what machine and what
>> benchmark was being used to produce the performance data. Anyway, with
>> the new smp_cond_load_acquire() helper, only one acquire barrier is
>> issued at the end of the loop. So even if there is a performance impact,
>> it should be less than a repeating one.
>>
>> As for the two percpu optimistic_spin_node.locked setting in osq_unlock(),
>> they are currently preceded by a full barrier xchg() call which can
>> provide the needed release barrier. Add comments saying that a release
>> barrier is needed for the proper functioning of the unlock operation
>> to alert people from accidentally remove the barrier when the code is
>> updated.
>>
>> 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 | 12 +++++++++---
>>   1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> [v2] Reword the commit log and keep the WRITE_ONCE() in osq_unlock()
>>       with comments.
>> [v3] Fix the comment above smp_cond_load_acquire().
> I still see no reason why this should be applied. Or even have this
> Fixes tag.

The main reason for this patch is for addressing the locking test 
failure reported by Håkon due to missing barrier. I do know that with 
the current osq_lock() use case, it is not a real problem. I just don't 
like inconsistency that an acquire barrier is just missing in just one 
place. I don't mind removing the Fixes tag though.

In your comment to David's "locking/osq_lock: Set prev_cpu=0 instead of 
locked=1" patch, you suggested adding smp_acquire__after_ctrl_dep() 
after finding that the lock had been granted which is exactly what the 
change from smp_cond_load_relaxed() to smp_cond_load_acquire() is doing. 
Right?

Cheers,
Longman