* [PATCH] locking/osq_lock: Ensure proper locking semantics for osq_lock/osq_unlock()
@ 2026-09-10 14:19 Waiman Long
2026-09-14 8:10 ` [PATCH] p " Haakon Bugge
` (3 more replies)
0 siblings, 4 replies; 10+ messages in thread
From: Waiman Long @ 2026-09-10 14:19 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Will Deacon, Boqun Feng
Cc: linux-kernel, Davidlohr Bueso, Haakon Bugge, David Laight,
Linus Torvalds, Yafang Shao, Steven Rostedt, 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 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))))
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);
+ }
}
--
2.55.0
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] p for osq_lock/osq_unlock()
2026-09-10 14:19 [PATCH] locking/osq_lock: Ensure proper locking semantics for osq_lock/osq_unlock() Waiman Long
@ 2026-09-14 8:10 ` Haakon Bugge
2026-09-14 9:53 ` [PATCH] locking/osq_lock: Ensure proper locking semantics " David Laight
` (2 subsequent siblings)
3 siblings, 0 replies; 10+ messages in thread
From: Haakon Bugge @ 2026-09-14 8:10 UTC (permalink / raw)
To: Waiman Long
Cc: Peter Zijlstra, Ingo Molnar, Will Deacon, Boqun Feng,
linux-kernel, Davidlohr Bueso, David Laight, Linus Torvalds,
Yafang Shao, Steven Rostedt
> On 10 Sep 2026, at 16:19, 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
s/proceeded/preceded/
> contended cachelines are different. This should probably work in most
> cases except in some exotic architectures where the barrier semantics
> may be cacheline specific.
As of today, doesn't atomic_xchg() provide full memory barrier?
From the doc: "RMW operations that have a return value are fully
ordered". I assume this applies to both the intra- and inter-
cacheline cases.
> 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
s/pervious/previous/
> 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>
With the nits fixed and the memory ordering of atomic_xchg() clarified,
Acked-by: Håkon Bugge <haakon.bugge@oracle.com>
Thxs, Håkon
> ---
> 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))))
> 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);
> + }
> }
> --
> 2.55.0
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] locking/osq_lock: Ensure proper locking semantics for osq_lock/osq_unlock()
2026-09-10 14:19 [PATCH] locking/osq_lock: Ensure proper locking semantics for osq_lock/osq_unlock() Waiman Long
2026-09-14 8:10 ` [PATCH] p " Haakon Bugge
@ 2026-09-14 9:53 ` David Laight
2026-09-14 20:22 ` Waiman Long
2026-09-14 11:22 ` Peter Zijlstra
[not found] ` <B7EAC6BE-D491-428E-8943-CE2F6DF390C6@oracle.com>
3 siblings, 1 reply; 10+ messages in thread
From: David Laight @ 2026-09-14 9:53 UTC (permalink / raw)
To: Waiman Long
Cc: Peter Zijlstra, Ingo Molnar, Will Deacon, Boqun Feng,
linux-kernel, Davidlohr Bueso, Haakon Bugge, Linus Torvalds,
Yafang Shao, Steven Rostedt
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);
> + }
> }
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] locking/osq_lock: Ensure proper locking semantics for osq_lock/osq_unlock()
2026-09-10 14:19 [PATCH] locking/osq_lock: Ensure proper locking semantics for osq_lock/osq_unlock() Waiman Long
2026-09-14 8:10 ` [PATCH] p " Haakon Bugge
2026-09-14 9:53 ` [PATCH] locking/osq_lock: Ensure proper locking semantics " David Laight
@ 2026-09-14 11:22 ` Peter Zijlstra
2026-09-15 2:30 ` Waiman Long
[not found] ` <B7EAC6BE-D491-428E-8943-CE2F6DF390C6@oracle.com>
3 siblings, 1 reply; 10+ messages in thread
From: Peter Zijlstra @ 2026-09-14 11:22 UTC (permalink / raw)
To: Waiman Long
Cc: Ingo Molnar, Will Deacon, Boqun Feng, linux-kernel,
Davidlohr Bueso, Haakon Bugge, David Laight, Linus Torvalds,
Yafang Shao, Steven Rostedt
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] locking/osq_lock: Ensure proper locking semantics for osq_lock/osq_unlock()
2026-09-14 9:53 ` [PATCH] locking/osq_lock: Ensure proper locking semantics " David Laight
@ 2026-09-14 20:22 ` Waiman Long
0 siblings, 0 replies; 10+ messages in thread
From: Waiman Long @ 2026-09-14 20:22 UTC (permalink / raw)
To: David Laight
Cc: Peter Zijlstra, Ingo Molnar, Will Deacon, Boqun Feng,
linux-kernel, Davidlohr Bueso, Haakon Bugge, Linus Torvalds,
Yafang Shao, Steven Rostedt
On 9/14/26 5:53 AM, David Laight wrote:
> 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
Thank for noticing that. I update the comment in v3 patch.
Cheers,
Longman
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] locking/osq_lock: Ensure proper locking semantics for osq_lock/osq_unlock()
[not found] ` <B7EAC6BE-D491-428E-8943-CE2F6DF390C6@oracle.com>
@ 2026-09-15 1:09 ` Waiman Long
0 siblings, 0 replies; 10+ messages in thread
From: Waiman Long @ 2026-09-15 1:09 UTC (permalink / raw)
To: Haakon Bugge
Cc: Peter Zijlstra, Ingo Molnar, Will Deacon, Boqun Feng,
linux-kernel, Davidlohr Bueso, David Laight, Linus Torvalds,
Yafang Shao, Steven Rostedt
On 9/14/26 6:30 AM, Haakon Bugge wrote:
> [$Subject fixed]
>
>> On 10 Sep 2026, at 16:19, 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
>
> s/proceeded/preceded/
>
>> contended cachelines are different. This should probably work in most
>> cases except in some exotic architectures where the barrier semantics
>> may be cacheline specific.
>
> As of today, doesn't atomic_xchg() provide full memory barrier?
> From the doc: "RMW operations that have a return value are fully
> ordered". I assume this applies to both the intra- and inter-
> cacheline cases.
Yes, you are right. That is why I decide to keep the WRITE_ONCE() in v2/v3.
Cheers,
Longman
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] locking/osq_lock: Ensure proper locking semantics for osq_lock/osq_unlock()
2026-09-14 11:22 ` Peter Zijlstra
@ 2026-09-15 2:30 ` Waiman Long
2026-09-15 5:19 ` Peter Zijlstra
2026-09-15 8:26 ` Peter Zijlstra
0 siblings, 2 replies; 10+ messages in thread
From: Waiman Long @ 2026-09-15 2:30 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Ingo Molnar, Will Deacon, Boqun Feng, linux-kernel,
Davidlohr Bueso, Haakon Bugge, David Laight, Linus Torvalds,
Yafang Shao, Steven Rostedt
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
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] locking/osq_lock: Ensure proper locking semantics for osq_lock/osq_unlock()
2026-09-15 2:30 ` Waiman Long
@ 2026-09-15 5:19 ` Peter Zijlstra
2026-09-15 8:36 ` David Laight
2026-09-15 8:26 ` Peter Zijlstra
1 sibling, 1 reply; 10+ messages in thread
From: Peter Zijlstra @ 2026-09-15 5:19 UTC (permalink / raw)
To: Waiman Long
Cc: Ingo Molnar, Will Deacon, Boqun Feng, linux-kernel,
Davidlohr Bueso, Haakon Bugge, David Laight, Linus Torvalds,
Yafang Shao, Steven Rostedt
On Mon, Sep 14, 2026 at 10:30:12PM -0400, Waiman Long wrote:
> On 9/14/26 7:22 AM, Peter Zijlstra wrote:
> > 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.
"It is not a real lock" will do. It has always been meant as a optimistic
spin queue -- very much not a lock.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] locking/osq_lock: Ensure proper locking semantics for osq_lock/osq_unlock()
2026-09-15 2:30 ` Waiman Long
2026-09-15 5:19 ` Peter Zijlstra
@ 2026-09-15 8:26 ` Peter Zijlstra
1 sibling, 0 replies; 10+ messages in thread
From: Peter Zijlstra @ 2026-09-15 8:26 UTC (permalink / raw)
To: Waiman Long
Cc: Ingo Molnar, Will Deacon, Boqun Feng, linux-kernel,
Davidlohr Bueso, Haakon Bugge, David Laight, Linus Torvalds,
Yafang Shao, Steven Rostedt
On Mon, Sep 14, 2026 at 10:30:12PM -0400, Waiman Long wrote:
> On 9/14/26 7:22 AM, Peter Zijlstra wrote:
> > > The "node->locked" read in osq_lock() was relaxed by commit 036cc30c6ba
> > > ("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.
The obvious benchmark is the one that got used for 036cc30c6ba, no?
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] locking/osq_lock: Ensure proper locking semantics for osq_lock/osq_unlock()
2026-09-15 5:19 ` Peter Zijlstra
@ 2026-09-15 8:36 ` David Laight
0 siblings, 0 replies; 10+ messages in thread
From: David Laight @ 2026-09-15 8:36 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Waiman Long, Ingo Molnar, Will Deacon, Boqun Feng, linux-kernel,
Davidlohr Bueso, Haakon Bugge, Linus Torvalds, Yafang Shao,
Steven Rostedt
On Tue, 15 Sep 2026 07:19:05 +0200
Peter Zijlstra <peterz@infradead.org> wrote:
> On Mon, Sep 14, 2026 at 10:30:12PM -0400, Waiman Long wrote:
> > On 9/14/26 7:22 AM, Peter Zijlstra wrote:
>
> > > 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.
>
> "It is not a real lock" will do. It has always been meant as a optimistic
> spin queue -- very much not a lock.
>
>
I'm not so sure, assuming that is probably short sighted and it will
cause grief sometime in the future.
I did start trying to locate the actual race.
If a read barrier fixes it there must be two reads that get swapped,
so it should be possible to find them.
Unfortunately I don't have a system that is likely to reproduce the error.
David
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2026-09-15 8:36 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-09-10 14:19 [PATCH] locking/osq_lock: Ensure proper locking semantics for osq_lock/osq_unlock() Waiman Long
2026-09-14 8:10 ` [PATCH] p " Haakon Bugge
2026-09-14 9:53 ` [PATCH] locking/osq_lock: Ensure proper locking semantics " David Laight
2026-09-14 20:22 ` Waiman Long
2026-09-14 11:22 ` Peter Zijlstra
2026-09-15 2:30 ` Waiman Long
2026-09-15 5:19 ` Peter Zijlstra
2026-09-15 8:36 ` David Laight
2026-09-15 8:26 ` Peter Zijlstra
[not found] ` <B7EAC6BE-D491-428E-8943-CE2F6DF390C6@oracle.com>
2026-09-15 1:09 ` Waiman Long
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®