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

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

[Waiman Long]

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

diff --git a/kernel/locking/osq_lock.c b/kernel/locking/osq_lock.c
index b4233dc2c2b0..37f1b6346c51 100644
--- a/kernel/locking/osq_lock.c
+++ b/kernel/locking/osq_lock.c
@@ -139,11 +139,11 @@ bool osq_lock(struct optimistic_spin_queue *lock)
 
 	/*
 	 * Wait to acquire the lock or cancellation. Note that need_resched()
-	 * will come with an IPI, which will wake smp_cond_load_relaxed() if it
+	 * will come with an IPI, which will wake smp_cond_load_acquire() if it
 	 * 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))))
 		return true;
 
@@ -224,11 +224,17 @@ void osq_unlock(struct optimistic_spin_queue *lock)
 	node = this_cpu_ptr(&osq_node);
 	next = xchg(&node->next, NULL);
 	if (next) {
+		/* The xchg() call above provides the release barrier */
 		WRITE_ONCE(next->locked, 1);
 		return;
 	}
 
 	next = osq_wait_next(lock, node, OSQ_UNLOCKED_VAL);
-	if (next)
+	if (next) {
+		/*
+		 * The xchg() call in osq_wait_next() before a non-NULL return
+		 * provides the release barrier.
+		 */
 		WRITE_ONCE(next->locked, 1);
+	}
 }
-- 
2.55.0

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

[Peter Zijlstra]

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.

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

[Waiman Long]

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

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

[David Laight]

On Tue, 15 Sep 2026 13:53:05 -0400
Waiman Long <longman@redhat.com> wrote:

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

I think it is a smaller barrier - since it is only in the 'lock acquired'
path.
Whether it is enough is another data point.
I don't remember anyone saying which memory reads are getting re-ordered.

I really do need to find out exactly what the barriers do (or rather which
feature of the cpu hardware makes them necessary).
They might be stopping out of order execution and speculative execution,
but the re-ordering of reads might be a feature of the cache.

David 

> 
> Cheers,
> Longman
> 

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

[Waiman Long]

On 9/15/26 2:08 PM, David Laight wrote:
> On Tue, 15 Sep 2026 13:53:05 -0400
> Waiman Long <longman@redhat.com> wrote:
>
>> 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?
> I think it is a smaller barrier - since it is only in the 'lock acquired'
> path.

If you look at how smp_cond_load_acquire() is implemented in 
include/asm-generic/barrier.h. It is just a smp_cond_load_relaxed() + 
smp_acquire__after_ctrl_dep() at the end. arm64 is the only exception 
with its own implementation where it uses smp_load_acquire() in the 
loop. However, arm64 also has its special __cmpwait_relaxed() 
instruction which ends when the processor detects a change in the 
cacheline and can wait for quite a while. So it is looping much less 
frequently before it breaks out, probably a few times at most. In that 
sense, it is almost the same as just having one acquire barrier issued 
at the end.

> Whether it is enough is another data point.
> I don't remember anyone saying which memory reads are getting re-ordered.
>
> I really do need to find out exactly what the barriers do (or rather which
> feature of the cpu hardware makes them necessary).
> They might be stopping out of order execution and speculative execution,
> but the re-ordering of reads might be a feature of the cache.

In essence, the acquire barrier at the beginning and the release barrier 
at the end of a critical section is to prevent all memory accesses 
within the critical section from flowing out of the critical section 
when viewing from another CPU's perspective while some memory access 
before and after the critical section can theoretically be viewed as 
happening inside the critical section. The rules are slightly different 
for read and write access which I can't recall right now.

Cheers,
Longman

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

[Peter Zijlstra]

On Tue, Sep 15, 2026 at 01:53:05PM -0400, Waiman Long wrote:
> The main reason for this patch is for addressing the locking test failure
> reported by Håkon due to missing barrier.

What crackpot test is that? osq isn't meant to be used as a stand alone
lock.

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

[Peter Zijlstra]

On Tue, Sep 15, 2026 at 01:53:05PM -0400, Waiman Long wrote:

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

It is similar, yes.

But all I did there was restore an ACQUIRE that David lost, silently.
That loop exit condition was smp_load_acquire() and he made it go away.

Now, you're right in that we can probably survive making it
smp_cond_load_acquire(). However:

 - you get to re-run the numbers from 036cc30c6b6a to show it doesn't
   regress

 - you don't put on Fixes, *unless* you put in a solid argument showing
   what is broken how. And so far, nothing actually needs this ordering
   because osq isn't actually a stand-alone lock -- it was never meant
   to be.