swait: add missing barrier to swake_up

PATCH In review Nicholas Piggin 4 participants 5 messages Sep 1, 2017 Scheduler

Share & tools

Nicholas Piggin

author

2017-09-01 06:15 UTC #0 lore raw

PATCH 1/1 swait: add missing barrier to swake_up

swake_up and swake_up_all test the swaitqueue outside the lock, but they are missing the barrier that would ensure visibility of a previous store that sets the wakeup condition with the load that tests the swaitqueue. This could lead to a lost wakeup if there is memory reordering. Fix this as prescribed by the waitqueue_active comments.

Signed-off-by Nicholas Piggin <[email protected]>

--
I noticed this when chasing down that rcu hang bug (which turned out to not be anything of the sort). I might be missing something here and it's safe somehow, but if so then it should have a comment where it diverges from normal waitqueues.

It looks like there's a few callers which are also testing swait_active before swake_up without a barrier which look wrong, so I must be missing something but I'm not sure what.

Thanks,
Nick

---
 kernel/sched/swait.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

kernel/sched/swait.c +10 -0

index 3d5610dcce11..9056278001d9 100644
@@ -33,6 +33,11 @@ void swake_up(struct swait_queue_head *q)
 {
 	unsigned long flags;
 
+	/*
+	 * See waitqueue_active() comments for checking waiters outside
+	 * the lock. Same principle applies here.
+	 */
+	smp_mb();
 	if (!swait_active(q))
 		return;

@@ -51,6 +56,11 @@ void swake_up_all(struct swait_queue_head *q)
 	struct swait_queue *curr;
 	LIST_HEAD(tmp);
 
+	/*
+	 * See waitqueue_active() comments for checking waiters outside
+	 * the lock. Same principle applies here.
+	 */
+	smp_mb();
 	if (!swait_active(q))
 		return;

Signature · 1 line

2.13.3

Andrea Parri

2017-09-01 09:23 UTC #1 lore raw

Quoted 14 lines · Nicholas Piggin

swake_up and swake_up_all test the swaitqueue outside the lock, but they are missing the barrier that would ensure visibility of a previous store that sets the wakeup condition with the load that tests the swaitqueue. This could lead to a lost wakeup if there is memory reordering. Fix this as prescribed by the waitqueue_active comments.
Signed-off-by Nicholas Piggin <[email protected]>
--
I noticed this when chasing down that rcu hang bug (which turned out to not be anything of the sort). I might be missing something here and it's safe somehow, but if so then it should have a comment where it diverges from normal waitqueues.

Quoted 3 lines

It looks like there's a few callers which are also testing swait_active before swake_up without a barrier which look wrong, so I must be missing something but I'm not sure what.

Hi Nicholas. I noticed

35a2897c2a306cca344ca5c0b43416707018f434
("sched/wait: Remove the lockless swait_active() check in swake_up*()")

in tip:locking/core.

Andrea

Quoted 38 lines

Thanks,
Nick

---
 kernel/sched/swait.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

kernel/sched/swait.c +10 -0

index 3d5610dcce11..9056278001d9 100644
@@ -33,6 +33,11 @@ void swake_up(struct swait_queue_head *q)
 {
 	unsigned long flags;
 
+	/*
+	 * See waitqueue_active() comments for checking waiters outside
+	 * the lock. Same principle applies here.
+	 */
+	smp_mb();
 	if (!swait_active(q))
 		return;

@@ -51,6 +56,11 @@ void swake_up_all(struct swait_queue_head *q)
 	struct swait_queue *curr;
 	LIST_HEAD(tmp);
 
+	/*
+	 * See waitqueue_active() comments for checking waiters outside
+	 * the lock. Same principle applies here.
+	 */
+	smp_mb();
 	if (!swait_active(q))
 		return;

Signature · 1 line

2.13.3

Nicholas Piggin

author

2017-09-01 09:58 UTC #2 lore raw

Quoted 19 lines · On Fri, 1 Sep 2017 11:23:22 +0200 Andrea Parri <[email protected]> wrote:

Quoted 14 lines · Nicholas Piggin
swake_up and swake_up_all test the swaitqueue outside the lock, but they are missing the barrier that would ensure visibility of a previous store that sets the wakeup condition with the load that tests the swaitqueue. This could lead to a lost wakeup if there is memory reordering. Fix this as prescribed by the waitqueue_active comments.
Signed-off-by Nicholas Piggin <[email protected]>
--
I noticed this when chasing down that rcu hang bug (which turned out to not be anything of the sort). I might be missing something here and it's safe somehow, but if so then it should have a comment where it diverges from normal waitqueues.
Quoted 3 lines
It looks like there's a few callers which are also testing swait_active before swake_up without a barrier which look wrong, so I must be missing something but I'm not sure what.

Quoted 6 lines

Hi Nicholas. I noticed
35a2897c2a306cca344ca5c0b43416707018f434
("sched/wait: Remove the lockless swait_active() check in swake_up*()")
in tip:locking/core.

Oh thanks, I missed that. Should be in 4.14/stable IMO.

As I said as well, several callers have picked up bad habits. RCU looks okay though.

Thanks,
Nick

Paul E. McKenney

2017-09-01 14:34 UTC #3 lore raw

Quoted 22 lines · Nicholas Piggin

Quoted 19 lines · On Fri, 1 Sep 2017 11:23:22 +0200 Andrea Parri <[email protected]> wrote:
Quoted 17 lines · Nicholas Piggin
swake_up and swake_up_all test the swaitqueue outside the lock, but they are missing the barrier that would ensure visibility of a previous store that sets the wakeup condition with the load that tests the swaitqueue. This could lead to a lost wakeup if there is memory reordering. Fix this as prescribed by the waitqueue_active comments.
Signed-off-by Nicholas Piggin <[email protected]>
--
I noticed this when chasing down that rcu hang bug (which turned out to not be anything of the sort). I might be missing something here and it's safe somehow, but if so then it should have a comment where it diverges from normal waitqueues.
It looks like there's a few callers which are also testing swait_active before swake_up without a barrier which look wrong, so I must be missing something but I'm not sure what.

Quoted 8 lines

Quoted 6 lines
Hi Nicholas. I noticed
35a2897c2a306cca344ca5c0b43416707018f434
("sched/wait: Remove the lockless swait_active() check in swake_up*()")
in tip:locking/core.
Oh thanks, I missed that. Should be in 4.14/stable IMO.

This might well have been helpful to me -- I had forgotten about that fix and am testing without it -- and suffering what look to be lost timeouts/wakeups. :-/

Thanx, Paul

Peter Zijlstra

maintainer

2017-09-01 11:16 UTC #4 lore raw

In reply to Nicholas Piggin

Quoted 6 lines · Nicholas Piggin

swake_up and swake_up_all test the swaitqueue outside the lock, but they are missing the barrier that would ensure visibility of a previous store that sets the wakeup condition with the load that tests the swaitqueue. This could lead to a lost wakeup if there is memory reordering. Fix this as prescribed by the waitqueue_active comments.

The below commit is in tip..

---

commit 35a2897c2a306cca344ca5c0b43416707018f434

Author: Boqun Feng <[email protected]>
Date:   Thu Jun 15 12:18:28 2017 +0800

sched/wait: Remove the lockless swait_active() check in swake_up*()

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 <[email protected]>

Signed-off-by Boqun Feng <[email protected]>

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Cc Krister Johansen <[email protected]>

Cc Linus Torvalds <[email protected]>

Cc Paul E. McKenney <[email protected]>

Cc Paul Gortmaker <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Thomas Gleixner <[email protected]>

Link http://lkml.kernel.org/r/20170615041828.zk3a3sfyudm5p6nl@tardis open here

Signed-off-by Ingo Molnar <[email protected]>

kernel/sched/swait.c +0 -6

index 3d5610dcce11..2227e183e202 100644
@@ -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)) {