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

[Waiman Long <longman@redhat.com>]

On 9/14/26 7:22 AM, Peter Zijlstra wrote:
> On Thu, Sep 10, 2026 at 10:19:08AM -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 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.
> You need to substantiate this *should*.
I don't have a good benchmark that can show any noticeable performance 
difference by adding an acquire barrier. My suspicion is that a 
repeating acquire barrier can be expensive depending on the actual 
processor used. Anyway the other osq_lock/osq_unlock() entry and exit 
points all have suitable barrier. I doubt adding one more acquire 
barrier will have a noticeable impact given that it is one-off at the 
end instead of a repeating one like the original osq_lock() code.
>> 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")
> This doesn't make sense. You cannot argue that the code is correct as is
> and still add Fixes.

That is true. I added this paragraph to show that it is not a serious 
bug, but I still think it is better to fix it or we have to explicitly 
say that it is not a real lock in term of how it can be used. I have 
taken it out to avoid confusion.

Cheers,
Longman


>