* [PATCH tip/sched/core] swait: Remove the lockless swait_active() check in swake_up*()
@ 2017-07-30 13:47 Boqun Feng
2017-08-02 16:07 ` Paul E. McKenney
0 siblings, 1 reply; 4+ messages in thread
From: Boqun Feng @ 2017-07-30 13:47 UTC (permalink / raw)
To: linux-kernel
Cc: Steven Rostedt, Paul E . McKenney, Krister Johansen,
Paul Gortmaker, Thomas Gleixner, Boqun Feng, Ingo Molnar,
Peter Zijlstra
Steven Rostedt reported a potential race in RCU core because of
swake_up():
CPU0 CPU1
---- ----
__call_rcu_core() {
spin_lock(rnp_root)
need_wake = __rcu_start_gp() {
rcu_start_gp_advanced() {
gp_flags = FLAG_INIT
}
}
rcu_gp_kthread() {
swait_event_interruptible(wq,
gp_flags & FLAG_INIT) {
spin_lock(q->lock)
*fetch wq->task_list here! *
list_add(wq->task_list, q->task_list)
spin_unlock(q->lock);
*fetch old value of gp_flags here *
spin_unlock(rnp_root)
rcu_gp_kthread_wake() {
swake_up(wq) {
swait_active(wq) {
list_empty(wq->task_list)
} * return false *
if (condition) * false *
schedule();
In this case, a wakeup is missed, which could cause the rcu_gp_kthread
waits for a long time.
The reason of this is that we do a lockless swait_active() check in
swake_up(). To fix this, we can either 1) add a smp_mb() in swake_up()
before swait_active() to provide the proper order or 2) simply remove
the swait_active() in swake_up().
The solution 2 not only fixes this problem but also keeps the swait and
wait API as close as possible, as wake_up() doesn't provide a full
barrier and doesn't do a lockless check of the wait queue either.
Moreover, there are users already using swait_active() to do their quick
checks for the wait queues, so it make less sense that swake_up() and
swake_up_all() do this on their own.
This patch then removes the lockless swait_active() check in swake_up()
and swake_up_all().
Reported-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
Acked-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Tested-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---
kernel/sched/swait.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/kernel/sched/swait.c b/kernel/sched/swait.c
index 3d5610dcce11..2227e183e202 100644
--- a/kernel/sched/swait.c
+++ b/kernel/sched/swait.c
@@ -33,9 +33,6 @@ void swake_up(struct swait_queue_head *q)
{
unsigned long flags;
- if (!swait_active(q))
- return;
-
raw_spin_lock_irqsave(&q->lock, flags);
swake_up_locked(q);
raw_spin_unlock_irqrestore(&q->lock, flags);
@@ -51,9 +48,6 @@ void swake_up_all(struct swait_queue_head *q)
struct swait_queue *curr;
LIST_HEAD(tmp);
- if (!swait_active(q))
- return;
-
raw_spin_lock_irq(&q->lock);
list_splice_init(&q->task_list, &tmp);
while (!list_empty(&tmp)) {
--
2.13.3
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH tip/sched/core] swait: Remove the lockless swait_active() check in swake_up*()
2017-07-30 13:47 [PATCH tip/sched/core] swait: Remove the lockless swait_active() check in swake_up*() Boqun Feng
@ 2017-08-02 16:07 ` Paul E. McKenney
2017-08-02 17:12 ` Peter Zijlstra
0 siblings, 1 reply; 4+ messages in thread
From: Paul E. McKenney @ 2017-08-02 16:07 UTC (permalink / raw)
To: Boqun Feng
Cc: linux-kernel, Steven Rostedt, Krister Johansen, Paul Gortmaker,
Thomas Gleixner, Ingo Molnar, Peter Zijlstra
On Sun, Jul 30, 2017 at 09:47:35PM +0800, Boqun Feng wrote:
> Steven Rostedt reported a potential race in RCU core because of
> swake_up():
>
> CPU0 CPU1
> ---- ----
> __call_rcu_core() {
>
> spin_lock(rnp_root)
> need_wake = __rcu_start_gp() {
> rcu_start_gp_advanced() {
> gp_flags = FLAG_INIT
> }
> }
>
> rcu_gp_kthread() {
> swait_event_interruptible(wq,
> gp_flags & FLAG_INIT) {
> spin_lock(q->lock)
>
> *fetch wq->task_list here! *
>
> list_add(wq->task_list, q->task_list)
> spin_unlock(q->lock);
>
> *fetch old value of gp_flags here *
>
> spin_unlock(rnp_root)
>
> rcu_gp_kthread_wake() {
> swake_up(wq) {
> swait_active(wq) {
> list_empty(wq->task_list)
>
> } * return false *
>
> if (condition) * false *
> schedule();
>
> In this case, a wakeup is missed, which could cause the rcu_gp_kthread
> waits for a long time.
>
> The reason of this is that we do a lockless swait_active() check in
> swake_up(). To fix this, we can either 1) add a smp_mb() in swake_up()
> before swait_active() to provide the proper order or 2) simply remove
> the swait_active() in swake_up().
>
> The solution 2 not only fixes this problem but also keeps the swait and
> wait API as close as possible, as wake_up() doesn't provide a full
> barrier and doesn't do a lockless check of the wait queue either.
> Moreover, there are users already using swait_active() to do their quick
> checks for the wait queues, so it make less sense that swake_up() and
> swake_up_all() do this on their own.
>
> This patch then removes the lockless swait_active() check in swake_up()
> and swake_up_all().
>
> Reported-by: Steven Rostedt <rostedt@goodmis.org>
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> Acked-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> Tested-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Hearing no objections but not hearing anything else, either, I have
queued this for v4.14. If someone else would rather queue it, please
let me know.
Thanx, Paul
> ---
> kernel/sched/swait.c | 6 ------
> 1 file changed, 6 deletions(-)
>
> diff --git a/kernel/sched/swait.c b/kernel/sched/swait.c
> index 3d5610dcce11..2227e183e202 100644
> --- a/kernel/sched/swait.c
> +++ b/kernel/sched/swait.c
> @@ -33,9 +33,6 @@ void swake_up(struct swait_queue_head *q)
> {
> unsigned long flags;
>
> - if (!swait_active(q))
> - return;
> -
> raw_spin_lock_irqsave(&q->lock, flags);
> swake_up_locked(q);
> raw_spin_unlock_irqrestore(&q->lock, flags);
> @@ -51,9 +48,6 @@ void swake_up_all(struct swait_queue_head *q)
> struct swait_queue *curr;
> LIST_HEAD(tmp);
>
> - if (!swait_active(q))
> - return;
> -
> raw_spin_lock_irq(&q->lock);
> list_splice_init(&q->task_list, &tmp);
> while (!list_empty(&tmp)) {
> --
> 2.13.3
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH tip/sched/core] swait: Remove the lockless swait_active() check in swake_up*()
2017-08-02 16:07 ` Paul E. McKenney
@ 2017-08-02 17:12 ` Peter Zijlstra
2017-08-02 17:47 ` Paul E. McKenney
0 siblings, 1 reply; 4+ messages in thread
From: Peter Zijlstra @ 2017-08-02 17:12 UTC (permalink / raw)
To: Paul E. McKenney
Cc: Boqun Feng, linux-kernel, Steven Rostedt, Krister Johansen,
Paul Gortmaker, Thomas Gleixner, Ingo Molnar
On Wed, Aug 02, 2017 at 09:07:13AM -0700, Paul E. McKenney wrote:
> On Sun, Jul 30, 2017 at 09:47:35PM +0800, Boqun Feng wrote:
> > Steven Rostedt reported a potential race in RCU core because of
> > swake_up():
> >
> > CPU0 CPU1
> > ---- ----
> > __call_rcu_core() {
> >
> > spin_lock(rnp_root)
> > need_wake = __rcu_start_gp() {
> > rcu_start_gp_advanced() {
> > gp_flags = FLAG_INIT
> > }
> > }
> >
> > rcu_gp_kthread() {
> > swait_event_interruptible(wq,
> > gp_flags & FLAG_INIT) {
> > spin_lock(q->lock)
> >
> > *fetch wq->task_list here! *
> >
> > list_add(wq->task_list, q->task_list)
> > spin_unlock(q->lock);
> >
> > *fetch old value of gp_flags here *
> >
> > spin_unlock(rnp_root)
> >
> > rcu_gp_kthread_wake() {
> > swake_up(wq) {
> > swait_active(wq) {
> > list_empty(wq->task_list)
> >
> > } * return false *
> >
> > if (condition) * false *
> > schedule();
> >
> > In this case, a wakeup is missed, which could cause the rcu_gp_kthread
> > waits for a long time.
> >
> > The reason of this is that we do a lockless swait_active() check in
> > swake_up(). To fix this, we can either 1) add a smp_mb() in swake_up()
> > before swait_active() to provide the proper order or 2) simply remove
> > the swait_active() in swake_up().
> >
> > The solution 2 not only fixes this problem but also keeps the swait and
> > wait API as close as possible, as wake_up() doesn't provide a full
> > barrier and doesn't do a lockless check of the wait queue either.
> > Moreover, there are users already using swait_active() to do their quick
> > checks for the wait queues, so it make less sense that swake_up() and
> > swake_up_all() do this on their own.
> >
> > This patch then removes the lockless swait_active() check in swake_up()
> > and swake_up_all().
> >
> > Reported-by: Steven Rostedt <rostedt@goodmis.org>
> > Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> > Acked-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > Tested-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
>
> Hearing no objections but not hearing anything else, either, I have
> queued this for v4.14. If someone else would rather queue it, please
> let me know.
I have it too. Lets see who can get it into -tip first :-)
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH tip/sched/core] swait: Remove the lockless swait_active() check in swake_up*()
2017-08-02 17:12 ` Peter Zijlstra
@ 2017-08-02 17:47 ` Paul E. McKenney
0 siblings, 0 replies; 4+ messages in thread
From: Paul E. McKenney @ 2017-08-02 17:47 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Boqun Feng, linux-kernel, Steven Rostedt, Krister Johansen,
Paul Gortmaker, Thomas Gleixner, Ingo Molnar
On Wed, Aug 02, 2017 at 07:12:23PM +0200, Peter Zijlstra wrote:
> On Wed, Aug 02, 2017 at 09:07:13AM -0700, Paul E. McKenney wrote:
> > On Sun, Jul 30, 2017 at 09:47:35PM +0800, Boqun Feng wrote:
> > > Steven Rostedt reported a potential race in RCU core because of
> > > swake_up():
> > >
> > > CPU0 CPU1
> > > ---- ----
> > > __call_rcu_core() {
> > >
> > > spin_lock(rnp_root)
> > > need_wake = __rcu_start_gp() {
> > > rcu_start_gp_advanced() {
> > > gp_flags = FLAG_INIT
> > > }
> > > }
> > >
> > > rcu_gp_kthread() {
> > > swait_event_interruptible(wq,
> > > gp_flags & FLAG_INIT) {
> > > spin_lock(q->lock)
> > >
> > > *fetch wq->task_list here! *
> > >
> > > list_add(wq->task_list, q->task_list)
> > > spin_unlock(q->lock);
> > >
> > > *fetch old value of gp_flags here *
> > >
> > > spin_unlock(rnp_root)
> > >
> > > rcu_gp_kthread_wake() {
> > > swake_up(wq) {
> > > swait_active(wq) {
> > > list_empty(wq->task_list)
> > >
> > > } * return false *
> > >
> > > if (condition) * false *
> > > schedule();
> > >
> > > In this case, a wakeup is missed, which could cause the rcu_gp_kthread
> > > waits for a long time.
> > >
> > > The reason of this is that we do a lockless swait_active() check in
> > > swake_up(). To fix this, we can either 1) add a smp_mb() in swake_up()
> > > before swait_active() to provide the proper order or 2) simply remove
> > > the swait_active() in swake_up().
> > >
> > > The solution 2 not only fixes this problem but also keeps the swait and
> > > wait API as close as possible, as wake_up() doesn't provide a full
> > > barrier and doesn't do a lockless check of the wait queue either.
> > > Moreover, there are users already using swait_active() to do their quick
> > > checks for the wait queues, so it make less sense that swake_up() and
> > > swake_up_all() do this on their own.
> > >
> > > This patch then removes the lockless swait_active() check in swake_up()
> > > and swake_up_all().
> > >
> > > Reported-by: Steven Rostedt <rostedt@goodmis.org>
> > > Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> > > Acked-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > > Tested-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> >
> > Hearing no objections but not hearing anything else, either, I have
> > queued this for v4.14. If someone else would rather queue it, please
> > let me know.
>
> I have it too. Lets see who can get it into -tip first :-)
Heh! We could use it as a test of Ingo's handling of multiple identical
patches. ;-)
Thanx, Paul
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-08-02 17:47 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-07-30 13:47 [PATCH tip/sched/core] swait: Remove the lockless swait_active() check in swake_up*() Boqun Feng
2017-08-02 16:07 ` Paul E. McKenney
2017-08-02 17:12 ` Peter Zijlstra
2017-08-02 17:47 ` Paul E. McKenney
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.