scheduler: convert BUG_ON()s in try_to_wake_up_local() to WARN_ON_ONCE()s

PATCH In review Tejun Heo 4 participants 5 messages Mar 18, 2013 – Mar 21, 2013 Scheduler

Share & tools

Tejun Heo

author

2013-03-18 19:22 UTC #0 lore raw

PATCH 1/1 scheduler: convert BUG_ON()s in try_to_wake_up_local() to WARN_ON_ONCE()s

try_to_wake_up_local() should only be invoked to wake up another task in the same runqueue and BUG_ON()s are used to enforce the rule. Missing try_to_wake_up_local() can stall workqueue execution but such stalls are likely to be finite either by another work item being queued or the one blocked getting unblocked. There's no reason to trigger BUG while holding rq lock crashing the whole system.

Convert BUG_ON()s in try_to_wake_up_local() to WARN_ON_ONCE()s.

Signed-off-by Tejun Heo <[email protected]>

Cc Steven Rostedt <[email protected]>

---
 kernel/sched/core.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

kernel/sched/core.c +4 -2

index 7f12624..a2eda4d 100644
@@ -1498,8 +1498,10 @@ static void try_to_wake_up_local(struct task_struct *p)
 {
 	struct rq *rq = task_rq(p);
 
-	BUG_ON(rq != this_rq());
-	BUG_ON(p == current);
+	if (WARN_ON_ONCE(rq != this_rq()) ||
+	    WARN_ON_ONCE(p == current))
+		return;
+
 	lockdep_assert_held(&rq->lock);
 
 	if (!raw_spin_trylock(&p->pi_lock)) {

Steven Rostedt

reviewer Acked-by on v1 1/1

2013-03-18 19:32 UTC #1 lore raw

Quoted 11 lines · Tejun Heo

try_to_wake_up_local() should only be invoked to wake up another task in the same runqueue and BUG_ON()s are used to enforce the rule. Missing try_to_wake_up_local() can stall workqueue execution but such stalls are likely to be finite either by another work item being queued or the one blocked getting unblocked. There's no reason to trigger BUG while holding rq lock crashing the whole system.
Convert BUG_ON()s in try_to_wake_up_local() to WARN_ON_ONCE()s.
Signed-off-by Tejun Heo <[email protected]>
Cc Steven Rostedt <[email protected]>

Acked-by Steven Rostedt <[email protected]>

-- Steve

Quoted 21 lines

---
 kernel/sched/core.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

kernel/sched/core.c +4 -2

index 7f12624..a2eda4d 100644
@@ -1498,8 +1498,10 @@ static void try_to_wake_up_local(struct task_struct *p)
 {
 	struct rq *rq = task_rq(p);
 
-	BUG_ON(rq != this_rq());
-	BUG_ON(p == current);
+	if (WARN_ON_ONCE(rq != this_rq()) ||
+	    WARN_ON_ONCE(p == current))
+		return;
+
 	lockdep_assert_held(&rq->lock);
 
 	if (!raw_spin_trylock(&p->pi_lock)) {

Peter Zijlstra

maintainer

2013-03-19 10:14 UTC #2 lore raw

In reply to Tejun Heo

Quoted 8 lines · Tejun Heo

try_to_wake_up_local() should only be invoked to wake up another task in the same runqueue and BUG_ON()s are used to enforce the rule. Missing try_to_wake_up_local() can stall workqueue execution but such stalls are likely to be finite either by another work item being queued or the one blocked getting unblocked. There's no reason to trigger BUG while holding rq lock crashing the whole system.
Convert BUG_ON()s in try_to_wake_up_local() to WARN_ON_ONCE()s.

Doesn't really matter either way, the printk()s triggered by either will very likely kill the system anyhow ;-)

Steven Rostedt

reviewer

2013-03-19 11:29 UTC #3 lore raw

Quoted 8 lines · Peter Zijlstra

Quoted 7 lines · Tejun Heo
try_to_wake_up_local() should only be invoked to wake up another task in the same runqueue and BUG_ON()s are used to enforce the rule. Missing try_to_wake_up_local() can stall workqueue execution but such stalls are likely to be finite either by another work item being queued or the one blocked getting unblocked. There's no reason to trigger BUG while holding rq lock crashing the whole system.

Quoted 4 lines

Quoted 1 line
Convert BUG_ON()s in try_to_wake_up_local() to WARN_ON_ONCE()s.
Doesn't really matter either way, the printk()s triggered by either will very likely kill the system anyhow ;-)

Only if something blocks on the console lock, and it tries to wake it up. But even then, it has to be a reverse rq locking order, or wakeup on the same rq that its running on (which is very unlikely).

I admit that it doesn't "solve" the bug. But at least there's a chance to investigate what happened when the bug does occur. Right now we have only one conclusion when the bug triggers, and that's a hard lockup.

-- Steve

tip-bot for Tejun Heo

merge bot

2013-03-21 18:15 UTC #4 lore raw

[tip:sched/urgent] sched: Convert BUG_ON() s in try_to_wake_up_local() to WARN_ON_ONCE()s

In reply to Tejun Heo

sched: Convert BUG_ON()s in try_to_wake_up_local() to WARN_ON_ONCE()s

Commit-ID:  383efcd00053ec40023010ce5034bd702e7ab373
Gitweb:     http://git.kernel.org/tip/383efcd00053ec40023010ce5034bd702e7ab373
Author:     Tejun Heo <[email protected]>
AuthorDate: Mon, 18 Mar 2013 12:22:34 -0700
Committer:  Ingo Molnar <[email protected]>
CommitDate: Thu, 21 Mar 2013 11:48:20 +0100

sched: Convert BUG_ON()s in try_to_wake_up_local() to WARN_ON_ONCE()s

Convert BUG_ON()s in try_to_wake_up_local() to WARN_ON_ONCE()s.

Signed-off-by Tejun Heo <[email protected]>

Acked-by Steven Rostedt <[email protected]>

Cc Peter Zijlstra <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

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

---
 kernel/sched/core.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

kernel/sched/core.c +4 -2

index b7b03cd..306943f 100644
@@ -1488,8 +1488,10 @@ static void try_to_wake_up_local(struct task_struct *p)
 {
 	struct rq *rq = task_rq(p);
 
-	BUG_ON(rq != this_rq());
-	BUG_ON(p == current);
+	if (WARN_ON_ONCE(rq != this_rq()) ||
+	    WARN_ON_ONCE(p == current))
+		return;
+
 	lockdep_assert_held(&rq->lock);
 
 	if (!raw_spin_trylock(&p->pi_lock)) {