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

[Peter Zijlstra <peterz@infradead.org>]

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*.

> 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.