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

[David Laight <david.laight.linux@gmail.com>]

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

>  		return true;
>  
> @@ -224,11 +224,14 @@ 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);
> +		/* Provide release barrier for unlock */
> +		smp_store_release(&next->locked, 1);
>  		return;
>  	}
>  
>  	next = osq_wait_next(lock, node, OSQ_UNLOCKED_VAL);
> -	if (next)
> -		WRITE_ONCE(next->locked, 1);
> +	if (next) {
> +		/* Provide release barrier for unlock */
> +		smp_store_release(&next->locked, 1);
> +	}
>  }