* [PATCH v2 1/2] locking: Make owner_on_cpu() into <linux/sched.h>
2021-12-03 7:59 [PATCH v2 0/2] locking: Fix racy reads of owner->on_cpu Kefeng Wang
@ 2021-12-03 7:59 ` Kefeng Wang
2021-12-06 15:15 ` [tip: locking/core] " tip-bot2 for Kefeng Wang
2021-12-03 7:59 ` [PATCH v2 2/2] locking: Mark racy reads of owner->on_cpu Kefeng Wang
` (2 subsequent siblings)
3 siblings, 1 reply; 7+ messages in thread
From: Kefeng Wang @ 2021-12-03 7:59 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Ingo Molnar, Will Deacon, Waiman Long, Boqun Feng,
Thomas Gleixner, Mark Rutland, Paul E. McKenney, linux-kernel,
kasan-dev, Kefeng Wang
Move the owner_on_cpu() from kernel/locking/rwsem.c into
include/linux/sched.h with under CONFIG_SMP, then use it
in the mutex/rwsem/rtmutex to simplify the code.
Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
include/linux/sched.h | 9 +++++++++
kernel/locking/mutex.c | 11 ++---------
kernel/locking/rtmutex.c | 5 ++---
kernel/locking/rwsem.c | 9 ---------
4 files changed, 13 insertions(+), 21 deletions(-)
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 78c351e35fec..ff609d9c2f21 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -2171,6 +2171,15 @@ extern long sched_getaffinity(pid_t pid, struct cpumask *mask);
#endif
#ifdef CONFIG_SMP
+static inline bool owner_on_cpu(struct task_struct *owner)
+{
+ /*
+ * As lock holder preemption issue, we both skip spinning if
+ * task is not on cpu or its cpu is preempted
+ */
+ return owner->on_cpu && !vcpu_is_preempted(task_cpu(owner));
+}
+
/* Returns effective CPU energy utilization, as seen by the scheduler */
unsigned long sched_cpu_util(int cpu, unsigned long max);
#endif /* CONFIG_SMP */
diff --git a/kernel/locking/mutex.c b/kernel/locking/mutex.c
index db1913611192..5e3585950ec8 100644
--- a/kernel/locking/mutex.c
+++ b/kernel/locking/mutex.c
@@ -367,8 +367,7 @@ bool mutex_spin_on_owner(struct mutex *lock, struct task_struct *owner,
/*
* Use vcpu_is_preempted to detect lock holder preemption issue.
*/
- if (!owner->on_cpu || need_resched() ||
- vcpu_is_preempted(task_cpu(owner))) {
+ if (!owner_on_cpu(owner) || need_resched()) {
ret = false;
break;
}
@@ -403,14 +402,8 @@ static inline int mutex_can_spin_on_owner(struct mutex *lock)
* structure won't go away during the spinning period.
*/
owner = __mutex_owner(lock);
-
- /*
- * As lock holder preemption issue, we both skip spinning if task is not
- * on cpu or its cpu is preempted
- */
-
if (owner)
- retval = owner->on_cpu && !vcpu_is_preempted(task_cpu(owner));
+ retval = owner_on_cpu(owner);
/*
* If lock->owner is not set, the mutex has been released. Return true
diff --git a/kernel/locking/rtmutex.c b/kernel/locking/rtmutex.c
index 0c6a48dfcecb..41152e8e799a 100644
--- a/kernel/locking/rtmutex.c
+++ b/kernel/locking/rtmutex.c
@@ -1379,9 +1379,8 @@ static bool rtmutex_spin_on_owner(struct rt_mutex_base *lock,
* for CONFIG_PREEMPT_RCU=y)
* - the VCPU on which owner runs is preempted
*/
- if (!owner->on_cpu || need_resched() ||
- rt_mutex_waiter_is_top_waiter(lock, waiter) ||
- vcpu_is_preempted(task_cpu(owner))) {
+ if (!owner_on_cpu(owner) || need_resched() ||
+ rt_mutex_waiter_is_top_waiter(lock, waiter)) {
res = false;
break;
}
diff --git a/kernel/locking/rwsem.c b/kernel/locking/rwsem.c
index 04a74d040a6d..69aba4abe104 100644
--- a/kernel/locking/rwsem.c
+++ b/kernel/locking/rwsem.c
@@ -658,15 +658,6 @@ static inline bool rwsem_try_write_lock_unqueued(struct rw_semaphore *sem)
return false;
}
-static inline bool owner_on_cpu(struct task_struct *owner)
-{
- /*
- * As lock holder preemption issue, we both skip spinning if
- * task is not on cpu or its cpu is preempted
- */
- return owner->on_cpu && !vcpu_is_preempted(task_cpu(owner));
-}
-
static inline bool rwsem_can_spin_on_owner(struct rw_semaphore *sem)
{
struct task_struct *owner;
--
2.26.2
^ permalink raw reply related [flat|nested] 7+ messages in thread* [tip: locking/core] locking: Make owner_on_cpu() into <linux/sched.h>
2021-12-03 7:59 ` [PATCH v2 1/2] locking: Make owner_on_cpu() into <linux/sched.h> Kefeng Wang
@ 2021-12-06 15:15 ` tip-bot2 for Kefeng Wang
0 siblings, 0 replies; 7+ messages in thread
From: tip-bot2 for Kefeng Wang @ 2021-12-06 15:15 UTC (permalink / raw)
To: linux-tip-commits; +Cc: Kefeng Wang, Peter Zijlstra (Intel), x86, linux-kernel
The following commit has been merged into the locking/core branch of tip:
Commit-ID: c0bed69daf4b67809b58cc7cd81a8fa4f45bc161
Gitweb: https://git.kernel.org/tip/c0bed69daf4b67809b58cc7cd81a8fa4f45bc161
Author: Kefeng Wang <wangkefeng.wang@huawei.com>
AuthorDate: Fri, 03 Dec 2021 15:59:34 +08:00
Committer: Peter Zijlstra <peterz@infradead.org>
CommitterDate: Sat, 04 Dec 2021 10:56:25 +01:00
locking: Make owner_on_cpu() into <linux/sched.h>
Move the owner_on_cpu() from kernel/locking/rwsem.c into
include/linux/sched.h with under CONFIG_SMP, then use it
in the mutex/rwsem/rtmutex to simplify the code.
Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lore.kernel.org/r/20211203075935.136808-2-wangkefeng.wang@huawei.com
---
include/linux/sched.h | 9 +++++++++
kernel/locking/mutex.c | 11 ++---------
kernel/locking/rtmutex.c | 5 ++---
kernel/locking/rwsem.c | 9 ---------
4 files changed, 13 insertions(+), 21 deletions(-)
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 78c351e..ff609d9 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -2171,6 +2171,15 @@ extern long sched_getaffinity(pid_t pid, struct cpumask *mask);
#endif
#ifdef CONFIG_SMP
+static inline bool owner_on_cpu(struct task_struct *owner)
+{
+ /*
+ * As lock holder preemption issue, we both skip spinning if
+ * task is not on cpu or its cpu is preempted
+ */
+ return owner->on_cpu && !vcpu_is_preempted(task_cpu(owner));
+}
+
/* Returns effective CPU energy utilization, as seen by the scheduler */
unsigned long sched_cpu_util(int cpu, unsigned long max);
#endif /* CONFIG_SMP */
diff --git a/kernel/locking/mutex.c b/kernel/locking/mutex.c
index db19136..5e35859 100644
--- a/kernel/locking/mutex.c
+++ b/kernel/locking/mutex.c
@@ -367,8 +367,7 @@ bool mutex_spin_on_owner(struct mutex *lock, struct task_struct *owner,
/*
* Use vcpu_is_preempted to detect lock holder preemption issue.
*/
- if (!owner->on_cpu || need_resched() ||
- vcpu_is_preempted(task_cpu(owner))) {
+ if (!owner_on_cpu(owner) || need_resched()) {
ret = false;
break;
}
@@ -403,14 +402,8 @@ static inline int mutex_can_spin_on_owner(struct mutex *lock)
* structure won't go away during the spinning period.
*/
owner = __mutex_owner(lock);
-
- /*
- * As lock holder preemption issue, we both skip spinning if task is not
- * on cpu or its cpu is preempted
- */
-
if (owner)
- retval = owner->on_cpu && !vcpu_is_preempted(task_cpu(owner));
+ retval = owner_on_cpu(owner);
/*
* If lock->owner is not set, the mutex has been released. Return true
diff --git a/kernel/locking/rtmutex.c b/kernel/locking/rtmutex.c
index f896208..0c1f2e3 100644
--- a/kernel/locking/rtmutex.c
+++ b/kernel/locking/rtmutex.c
@@ -1382,9 +1382,8 @@ static bool rtmutex_spin_on_owner(struct rt_mutex_base *lock,
* for CONFIG_PREEMPT_RCU=y)
* - the VCPU on which owner runs is preempted
*/
- if (!owner->on_cpu || need_resched() ||
- rt_mutex_waiter_is_top_waiter(lock, waiter) ||
- vcpu_is_preempted(task_cpu(owner))) {
+ if (!owner_on_cpu(owner) || need_resched() ||
+ rt_mutex_waiter_is_top_waiter(lock, waiter)) {
res = false;
break;
}
diff --git a/kernel/locking/rwsem.c b/kernel/locking/rwsem.c
index c51387a..b92d0a8 100644
--- a/kernel/locking/rwsem.c
+++ b/kernel/locking/rwsem.c
@@ -613,15 +613,6 @@ static inline bool rwsem_try_write_lock_unqueued(struct rw_semaphore *sem)
return false;
}
-static inline bool owner_on_cpu(struct task_struct *owner)
-{
- /*
- * As lock holder preemption issue, we both skip spinning if
- * task is not on cpu or its cpu is preempted
- */
- return owner->on_cpu && !vcpu_is_preempted(task_cpu(owner));
-}
-
static inline bool rwsem_can_spin_on_owner(struct rw_semaphore *sem)
{
struct task_struct *owner;
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v2 2/2] locking: Mark racy reads of owner->on_cpu
2021-12-03 7:59 [PATCH v2 0/2] locking: Fix racy reads of owner->on_cpu Kefeng Wang
2021-12-03 7:59 ` [PATCH v2 1/2] locking: Make owner_on_cpu() into <linux/sched.h> Kefeng Wang
@ 2021-12-03 7:59 ` Kefeng Wang
2021-12-06 15:15 ` [tip: locking/core] " tip-bot2 for Marco Elver
2021-12-03 8:33 ` [PATCH v2 0/2] locking: Fix " Peter Zijlstra
2021-12-03 15:49 ` Waiman Long
3 siblings, 1 reply; 7+ messages in thread
From: Kefeng Wang @ 2021-12-03 7:59 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Ingo Molnar, Will Deacon, Waiman Long, Boqun Feng,
Thomas Gleixner, Mark Rutland, Paul E. McKenney, linux-kernel,
kasan-dev, Marco Elver, Kefeng Wang
From: Marco Elver <elver@google.com>
One of the more frequent data races reported by KCSAN is the racy read
in mutex_spin_on_owner(), which is usually reported as "race of unknown
origin" without showing the writer. This is due to the racing write
occurring in kernel/sched. Locally enabling KCSAN in kernel/sched shows:
| write (marked) to 0xffff97f205079934 of 4 bytes by task 316 on cpu 6:
| finish_task kernel/sched/core.c:4632 [inline]
| finish_task_switch kernel/sched/core.c:4848
| context_switch kernel/sched/core.c:4975 [inline]
| __schedule kernel/sched/core.c:6253
| schedule kernel/sched/core.c:6326
| schedule_preempt_disabled kernel/sched/core.c:6385
| __mutex_lock_common kernel/locking/mutex.c:680
| __mutex_lock kernel/locking/mutex.c:740 [inline]
| __mutex_lock_slowpath kernel/locking/mutex.c:1028
| mutex_lock kernel/locking/mutex.c:283
| tty_open_by_driver drivers/tty/tty_io.c:2062 [inline]
| ...
|
| read to 0xffff97f205079934 of 4 bytes by task 322 on cpu 3:
| mutex_spin_on_owner kernel/locking/mutex.c:370
| mutex_optimistic_spin kernel/locking/mutex.c:480
| __mutex_lock_common kernel/locking/mutex.c:610
| __mutex_lock kernel/locking/mutex.c:740 [inline]
| __mutex_lock_slowpath kernel/locking/mutex.c:1028
| mutex_lock kernel/locking/mutex.c:283
| tty_open_by_driver drivers/tty/tty_io.c:2062 [inline]
| ...
|
| value changed: 0x00000001 -> 0x00000000
This race is clearly intentional, and the potential for miscompilation
is slim due to surrounding barrier() and cpu_relax(), and the value
being used as a boolean.
Nevertheless, marking this reader would more clearly denote intent and
make it obvious that concurrency is expected. Use READ_ONCE() to avoid
having to reason about compiler optimizations now and in future.
With previous refactor, mark the read to owner->on_cpu in owner_on_cpu(),
which immediately precedes the loop executing mutex_spin_on_owner().
Signed-off-by: Marco Elver <elver@google.com>
Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
include/linux/sched.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/sched.h b/include/linux/sched.h
index ff609d9c2f21..0b9b0e3f4791 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -2177,7 +2177,7 @@ static inline bool owner_on_cpu(struct task_struct *owner)
* As lock holder preemption issue, we both skip spinning if
* task is not on cpu or its cpu is preempted
*/
- return owner->on_cpu && !vcpu_is_preempted(task_cpu(owner));
+ return READ_ONCE(owner->on_cpu) && !vcpu_is_preempted(task_cpu(owner));
}
/* Returns effective CPU energy utilization, as seen by the scheduler */
--
2.26.2
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [tip: locking/core] locking: Mark racy reads of owner->on_cpu
2021-12-03 7:59 ` [PATCH v2 2/2] locking: Mark racy reads of owner->on_cpu Kefeng Wang
@ 2021-12-06 15:15 ` tip-bot2 for Marco Elver
0 siblings, 0 replies; 7+ messages in thread
From: tip-bot2 for Marco Elver @ 2021-12-06 15:15 UTC (permalink / raw)
To: linux-tip-commits
Cc: Marco Elver, Kefeng Wang, Peter Zijlstra (Intel), x86,
linux-kernel
The following commit has been merged into the locking/core branch of tip:
Commit-ID: 4cf75fd4a2545ca4deea992f929602c9fdbe8058
Gitweb: https://git.kernel.org/tip/4cf75fd4a2545ca4deea992f929602c9fdbe8058
Author: Marco Elver <elver@google.com>
AuthorDate: Fri, 03 Dec 2021 15:59:35 +08:00
Committer: Peter Zijlstra <peterz@infradead.org>
CommitterDate: Sat, 04 Dec 2021 10:56:25 +01:00
locking: Mark racy reads of owner->on_cpu
One of the more frequent data races reported by KCSAN is the racy read
in mutex_spin_on_owner(), which is usually reported as "race of unknown
origin" without showing the writer. This is due to the racing write
occurring in kernel/sched. Locally enabling KCSAN in kernel/sched shows:
| write (marked) to 0xffff97f205079934 of 4 bytes by task 316 on cpu 6:
| finish_task kernel/sched/core.c:4632 [inline]
| finish_task_switch kernel/sched/core.c:4848
| context_switch kernel/sched/core.c:4975 [inline]
| __schedule kernel/sched/core.c:6253
| schedule kernel/sched/core.c:6326
| schedule_preempt_disabled kernel/sched/core.c:6385
| __mutex_lock_common kernel/locking/mutex.c:680
| __mutex_lock kernel/locking/mutex.c:740 [inline]
| __mutex_lock_slowpath kernel/locking/mutex.c:1028
| mutex_lock kernel/locking/mutex.c:283
| tty_open_by_driver drivers/tty/tty_io.c:2062 [inline]
| ...
|
| read to 0xffff97f205079934 of 4 bytes by task 322 on cpu 3:
| mutex_spin_on_owner kernel/locking/mutex.c:370
| mutex_optimistic_spin kernel/locking/mutex.c:480
| __mutex_lock_common kernel/locking/mutex.c:610
| __mutex_lock kernel/locking/mutex.c:740 [inline]
| __mutex_lock_slowpath kernel/locking/mutex.c:1028
| mutex_lock kernel/locking/mutex.c:283
| tty_open_by_driver drivers/tty/tty_io.c:2062 [inline]
| ...
|
| value changed: 0x00000001 -> 0x00000000
This race is clearly intentional, and the potential for miscompilation
is slim due to surrounding barrier() and cpu_relax(), and the value
being used as a boolean.
Nevertheless, marking this reader would more clearly denote intent and
make it obvious that concurrency is expected. Use READ_ONCE() to avoid
having to reason about compiler optimizations now and in future.
With previous refactor, mark the read to owner->on_cpu in owner_on_cpu(),
which immediately precedes the loop executing mutex_spin_on_owner().
Signed-off-by: Marco Elver <elver@google.com>
Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lore.kernel.org/r/20211203075935.136808-3-wangkefeng.wang@huawei.com
---
include/linux/sched.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/sched.h b/include/linux/sched.h
index ff609d9..0b9b0e3 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -2177,7 +2177,7 @@ static inline bool owner_on_cpu(struct task_struct *owner)
* As lock holder preemption issue, we both skip spinning if
* task is not on cpu or its cpu is preempted
*/
- return owner->on_cpu && !vcpu_is_preempted(task_cpu(owner));
+ return READ_ONCE(owner->on_cpu) && !vcpu_is_preempted(task_cpu(owner));
}
/* Returns effective CPU energy utilization, as seen by the scheduler */
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 0/2] locking: Fix racy reads of owner->on_cpu
2021-12-03 7:59 [PATCH v2 0/2] locking: Fix racy reads of owner->on_cpu Kefeng Wang
2021-12-03 7:59 ` [PATCH v2 1/2] locking: Make owner_on_cpu() into <linux/sched.h> Kefeng Wang
2021-12-03 7:59 ` [PATCH v2 2/2] locking: Mark racy reads of owner->on_cpu Kefeng Wang
@ 2021-12-03 8:33 ` Peter Zijlstra
2021-12-03 15:49 ` Waiman Long
3 siblings, 0 replies; 7+ messages in thread
From: Peter Zijlstra @ 2021-12-03 8:33 UTC (permalink / raw)
To: Kefeng Wang
Cc: Ingo Molnar, Will Deacon, Waiman Long, Boqun Feng,
Thomas Gleixner, Mark Rutland, Paul E. McKenney, linux-kernel,
kasan-dev
On Fri, Dec 03, 2021 at 03:59:33PM +0800, Kefeng Wang wrote:
> v2:
> - adding owner_on_cpu() refactor, shared by mutex/rtmutex/rwsem
>
> v1: https://lore.kernel.org/all/20211202101238.33546-1-elver@google.com/
>
> Kefeng Wang (1):
> locking: Make owner_on_cpu() into <linux/sched.h>
>
> Marco Elver (1):
> locking: Mark racy reads of owner->on_cpu
>
> include/linux/sched.h | 9 +++++++++
> kernel/locking/mutex.c | 11 ++---------
> kernel/locking/rtmutex.c | 5 ++---
> kernel/locking/rwsem.c | 9 ---------
> 4 files changed, 13 insertions(+), 21 deletions(-)
Thanks!
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 0/2] locking: Fix racy reads of owner->on_cpu
2021-12-03 7:59 [PATCH v2 0/2] locking: Fix racy reads of owner->on_cpu Kefeng Wang
` (2 preceding siblings ...)
2021-12-03 8:33 ` [PATCH v2 0/2] locking: Fix " Peter Zijlstra
@ 2021-12-03 15:49 ` Waiman Long
3 siblings, 0 replies; 7+ messages in thread
From: Waiman Long @ 2021-12-03 15:49 UTC (permalink / raw)
To: Kefeng Wang, Peter Zijlstra
Cc: Ingo Molnar, Will Deacon, Boqun Feng, Thomas Gleixner,
Mark Rutland, Paul E. McKenney, linux-kernel, kasan-dev
On 12/3/21 02:59, Kefeng Wang wrote:
> v2:
> - adding owner_on_cpu() refactor, shared by mutex/rtmutex/rwsem
>
> v1: https://lore.kernel.org/all/20211202101238.33546-1-elver@google.com/
>
> Kefeng Wang (1):
> locking: Make owner_on_cpu() into <linux/sched.h>
>
> Marco Elver (1):
> locking: Mark racy reads of owner->on_cpu
>
> include/linux/sched.h | 9 +++++++++
> kernel/locking/mutex.c | 11 ++---------
> kernel/locking/rtmutex.c | 5 ++---
> kernel/locking/rwsem.c | 9 ---------
> 4 files changed, 13 insertions(+), 21 deletions(-)
>
LGTM
Acked-by: Waiman Long <longman@redhat.com>
^ permalink raw reply [flat|nested] 7+ messages in thread