Skip to contentSkip to conversation

Eliminate BUG_ON for v4.21/v5.0

PATCH In review Paul E. McKenney 2 participants 7 messages Nov 11, 2018 – Nov 12, 2018 RCU

lore
Share & tools
lore

Paul E. McKenney

author
#0 lore raw

cover Eliminate BUG_ON for v4.21/v5.0

Hello!

This series converts BUG_ON() and friends to WARN_ON() and friends, split out by file: (1) kernel/rcu/sync.c, (2) kernel/rcu/tree.c,
(3) kernel/rcu/tree_plugin.h, and (4) kernel/rcu/update.c.

Thanx, Paul

------------------------------------------------------------------------

 sync.c        |   13 ++++++-------
 tree.c        |    5 +++--
 tree_plugin.h |    9 ++++++---
 update.c      |    3 ++-
 4 files changed, 17 insertions(+), 13 deletions(-)
author
#1 lore raw

PATCH 1/4 rcu: Eliminate BUG_ON() for sync.c

The sync.c file has a number of calls to BUG_ON(), which panics the kernel, which is not a good strategy for devices (like embedded) that don't have a way to capture console output. This commit therefore changes these BUG_ON() calls to WARN_ON_ONCE(), but does so quite naively.

Reported-by Linus Torvalds <[email protected]>
Signed-off-by Paul E. McKenney <[email protected]>
Acked-by Oleg Nesterov <[email protected]>
Cc Peter Zijlstra <[email protected]>
---
 kernel/rcu/sync.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)
kernel/rcu/sync.c +6 -7
index 3f943efcf61c..a6ba446a9693 100644
@@ -125,8 +125,7 @@ void rcu_sync_enter(struct rcu_sync *rsp)
 		rsp->gp_state = GP_PENDING;
 	spin_unlock_irq(&rsp->rss_lock);
 
-	BUG_ON(need_wait && need_sync);
-
+	WARN_ON_ONCE(need_wait && need_sync);
 	if (need_sync) {
 		gp_ops[rsp->gp_type].sync();
 		rsp->gp_state = GP_PASSED;

@@ -139,7 +138,7 @@ void rcu_sync_enter(struct rcu_sync *rsp)
 		 * Nobody has yet been allowed the 'fast' path and thus we can
 		 * avoid doing any sync(). The callback will get 'dropped'.
 		 */
-		BUG_ON(rsp->gp_state != GP_PASSED);
+		WARN_ON_ONCE(rsp->gp_state != GP_PASSED);
 	}
 }
 

@@ -166,8 +165,8 @@ static void rcu_sync_func(struct rcu_head *rhp)
 	struct rcu_sync *rsp = container_of(rhp, struct rcu_sync, cb_head);
 	unsigned long flags;
 
-	BUG_ON(rsp->gp_state != GP_PASSED);
-	BUG_ON(rsp->cb_state == CB_IDLE);
+	WARN_ON_ONCE(rsp->gp_state != GP_PASSED);
+	WARN_ON_ONCE(rsp->cb_state == CB_IDLE);
 
 	spin_lock_irqsave(&rsp->rss_lock, flags);
 	if (rsp->gp_count) {

@@ -225,7 +224,7 @@ void rcu_sync_dtor(struct rcu_sync *rsp)
 {
 	int cb_state;
 
-	BUG_ON(rsp->gp_count);
+	WARN_ON_ONCE(rsp->gp_count);
 
 	spin_lock_irq(&rsp->rss_lock);
 	if (rsp->cb_state == CB_REPLAY)

@@ -235,6 +234,6 @@ void rcu_sync_dtor(struct rcu_sync *rsp)
 
 	if (cb_state != CB_IDLE) {
 		gp_ops[rsp->gp_type].wait();
-		BUG_ON(rsp->cb_state != CB_IDLE);
+		WARN_ON_ONCE(rsp->cb_state != CB_IDLE);
 	}
 }
Signature · 1 line
2.17.1
reviewer
#2 lore raw

Re: [PATCH tip/core/rcu 1/4] rcu: Eliminate BUG_ON() for sync.c

Quoted 57 lines · On Sun, 11 Nov 2018 11:32:14 -0800 "Paul E. McKenney" <[email protected]>…

The sync.c file has a number of calls to BUG_ON(), which panics the kernel, which is not a good strategy for devices (like embedded) that don't have a way to capture console output. This commit therefore changes these BUG_ON() calls to WARN_ON_ONCE(), but does so quite naively.

Reported-by Linus Torvalds <[email protected]>
Signed-off-by Paul E. McKenney <[email protected]>
Acked-by Oleg Nesterov <[email protected]>
Cc Peter Zijlstra <[email protected]>
---
 kernel/rcu/sync.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)
kernel/rcu/sync.c +5 -6
index 3f943efcf61c..a6ba446a9693 100644
@@ -125,8 +125,7 @@ void rcu_sync_enter(struct rcu_sync *rsp)
 		rsp->gp_state = GP_PENDING;
 	spin_unlock_irq(&rsp->rss_lock);
 
-	BUG_ON(need_wait && need_sync);
-
+	WARN_ON_ONCE(need_wait && need_sync);
 	if (need_sync) {
 		gp_ops[rsp->gp_type].sync();
 		rsp->gp_state = GP_PASSED;

@@ -139,7 +138,7 @@ void rcu_sync_enter(struct rcu_sync *rsp)
 		 * Nobody has yet been allowed the 'fast' path and thus we can
 		 * avoid doing any sync(). The callback will get 'dropped'.
 		 */
-		BUG_ON(rsp->gp_state != GP_PASSED);
+		WARN_ON_ONCE(rsp->gp_state != GP_PASSED);
 	}
 }
 

@@ -166,8 +165,8 @@ static void rcu_sync_func(struct rcu_head *rhp)
 	struct rcu_sync *rsp = container_of(rhp, struct rcu_sync, cb_head);
 	unsigned long flags;
 
-	BUG_ON(rsp->gp_state != GP_PASSED);
-	BUG_ON(rsp->cb_state == CB_IDLE);
+	WARN_ON_ONCE(rsp->gp_state != GP_PASSED);
+	WARN_ON_ONCE(rsp->cb_state == CB_IDLE);
 
 	spin_lock_irqsave(&rsp->rss_lock, flags);
 	if (rsp->gp_count) {

@@ -225,7 +224,7 @@ void rcu_sync_dtor(struct rcu_sync *rsp)
 {
 	int cb_state;
 
-	BUG_ON(rsp->gp_count);
+	WARN_ON_ONCE(rsp->gp_count);
 
 	spin_lock_irq(&rsp->rss_lock);
 	if (rsp->cb_state == CB_REPLAY)

kernel/rcu/sync.c void rcu_sync_dtor(struct rcu_sync *rsp) · line 239

Quoted 7 lines
kernel/rcu/sync.c +1 -1
@@ -235,6 +234,6 @@ void rcu_sync_dtor(struct rcu_sync *rsp)
 
 	if (cb_state != CB_IDLE) {
 		gp_ops[rsp->gp_type].wait();
-		BUG_ON(rsp->cb_state != CB_IDLE);
+		WARN_ON_ONCE(rsp->cb_state != CB_IDLE);
 	}
 }

I take it that if any of these WARN_ON_ONCE() triggers, they wont cause immediate catastrophe, and/or there's no gentle way out like you have with the other patches exiting the function when one is hit.

-- Steve

author
#3 lore raw

Re: [PATCH tip/core/rcu 1/4] rcu: Eliminate BUG_ON() for sync.c

Quoted 61 lines · Steven Rostedt
Quoted 58 lines · On Sun, 11 Nov 2018 11:32:14 -0800 "Paul E. McKenney" <[email protected]>…

The sync.c file has a number of calls to BUG_ON(), which panics the kernel, which is not a good strategy for devices (like embedded) that don't have a way to capture console output. This commit therefore changes these BUG_ON() calls to WARN_ON_ONCE(), but does so quite naively.

Reported-by Linus Torvalds <[email protected]>
Signed-off-by Paul E. McKenney <[email protected]>
Acked-by Oleg Nesterov <[email protected]>
Cc Peter Zijlstra <[email protected]>
---
 kernel/rcu/sync.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)
kernel/rcu/sync.c +5 -6
index 3f943efcf61c..a6ba446a9693 100644
@@ -125,8 +125,7 @@ void rcu_sync_enter(struct rcu_sync *rsp)
 		rsp->gp_state = GP_PENDING;
 	spin_unlock_irq(&rsp->rss_lock);
 
-	BUG_ON(need_wait && need_sync);
-
+	WARN_ON_ONCE(need_wait && need_sync);
 	if (need_sync) {
 		gp_ops[rsp->gp_type].sync();
 		rsp->gp_state = GP_PASSED;

@@ -139,7 +138,7 @@ void rcu_sync_enter(struct rcu_sync *rsp)
 		 * Nobody has yet been allowed the 'fast' path and thus we can
 		 * avoid doing any sync(). The callback will get 'dropped'.
 		 */
-		BUG_ON(rsp->gp_state != GP_PASSED);
+		WARN_ON_ONCE(rsp->gp_state != GP_PASSED);
 	}
 }
 

@@ -166,8 +165,8 @@ static void rcu_sync_func(struct rcu_head *rhp)
 	struct rcu_sync *rsp = container_of(rhp, struct rcu_sync, cb_head);
 	unsigned long flags;
 
-	BUG_ON(rsp->gp_state != GP_PASSED);
-	BUG_ON(rsp->cb_state == CB_IDLE);
+	WARN_ON_ONCE(rsp->gp_state != GP_PASSED);
+	WARN_ON_ONCE(rsp->cb_state == CB_IDLE);
 
 	spin_lock_irqsave(&rsp->rss_lock, flags);
 	if (rsp->gp_count) {

@@ -225,7 +224,7 @@ void rcu_sync_dtor(struct rcu_sync *rsp)
 {
 	int cb_state;
 
-	BUG_ON(rsp->gp_count);
+	WARN_ON_ONCE(rsp->gp_count);
 
 	spin_lock_irq(&rsp->rss_lock);
 	if (rsp->cb_state == CB_REPLAY)

@@ -235,6 +234,6 @@ void rcu_sync_dtor(struct rcu_sync *rsp)
Quoted 10 lines

kernel/rcu/sync.c void rcu_sync_dtor(struct rcu_sync *rsp) · line 239

Quoted 6 lines
kernel/rcu/sync.c +1 -1
 	if (cb_state != CB_IDLE) {
 		gp_ops[rsp->gp_type].wait();
-		BUG_ON(rsp->cb_state != CB_IDLE);
+		WARN_ON_ONCE(rsp->cb_state != CB_IDLE);
 	}
 }

I take it that if any of these WARN_ON_ONCE() triggers, they wont cause immediate catastrophe, and/or there's no gentle way out like you have with the other patches exiting the function when one is hit.

Oleg was actually OK with removing them entirely:

"I added these BUG_ON's for documentation when I was prototyping this code, perhaps we can simply remove them."

And they are "cannot happen" types of things (famous last words). Oleg also has another approach that could rip-and-replace the current implementation, which would render these WARN*()s moot.

Thanx, Paul

author
#4 lore raw

PATCH 2/4 rcu: Eliminate BUG_ON() for kernel/rcu/tree.c

In reply to Paul E. McKenney

The tree.c file has a number of calls to BUG_ON(), which panics the kernel, which is not a good strategy for devices (like embedded) that don't have a way to capture console output. This commit therefore converts these BUG_ON() calls to WARN_ON_ONCE() and WARN_ONCE().

Reported-by Linus Torvalds <[email protected]>
Signed-off-by Paul E. McKenney <[email protected]>
---
 kernel/rcu/tree.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
kernel/rcu/tree.c +3 -2
index 121f833acd04..bdb6659a8dbc 100644
@@ -2826,7 +2826,7 @@ __call_rcu(struct rcu_head *head, rcu_callback_t func, int cpu, bool lazy)
 		 * Very early boot, before rcu_init().  Initialize if needed
 		 * and then drop through to queue the callback.
 		 */
-		BUG_ON(cpu != -1);
+		WARN_ON_ONCE(cpu != -1);
 		WARN_ON_ONCE(!rcu_is_watching());
 		if (rcu_segcblist_empty(&rdp->cblist))
 			rcu_segcblist_init(&rdp->cblist);

@@ -3485,7 +3485,8 @@ static int __init rcu_spawn_gp_kthread(void)
 
 	rcu_scheduler_fully_active = 1;
 	t = kthread_create(rcu_gp_kthread, NULL, "%s", rcu_state.name);
-	BUG_ON(IS_ERR(t));
+	if (WARN_ONCE(IS_ERR(t), "%s: Could not start grace-period kthread, OOM is now expected behavior\n", __func__))
+		return 0;
 	rnp = rcu_get_root();
 	raw_spin_lock_irqsave_rcu_node(rnp, flags);
 	rcu_state.gp_kthread = t;
Signature · 1 line
2.17.1
author
#5 lore raw

PATCH 3/4 rcu: Eliminate BUG_ON() for kernel/rcu/tree_plugin.h

In reply to Paul E. McKenney

The tree_plugin.h file has a number of calls to BUG_ON(), which panics the kernel, which is not a good strategy for devices (like embedded) that don't have a way to capture console output. This commit therefore converts these BUG_ON() calls to WARN_ON_ONCE() and WARN_ONCE().

Reported-by Linus Torvalds <[email protected]>
Signed-off-by Paul E. McKenney <[email protected]>
---
 kernel/rcu/tree_plugin.h | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)
kernel/rcu/tree_plugin.h +6 -3
index 05915e536336..105a19752ac2 100644
@@ -1464,7 +1464,8 @@ static void __init rcu_spawn_boost_kthreads(void)
 
 	for_each_possible_cpu(cpu)
 		per_cpu(rcu_cpu_has_work, cpu) = 0;
-	BUG_ON(smpboot_register_percpu_thread(&rcu_cpu_thread_spec));
+	if (WARN_ONCE(smpboot_register_percpu_thread(&rcu_cpu_thread_spec), "%s: Could not start rcuo kthread, OOM is now expected behavior\n", __func__))
+		return;
 	rcu_for_each_leaf_node(rnp)
 		(void)rcu_spawn_one_boost_kthread(rnp);
 }

@@ -2322,7 +2323,8 @@ static int rcu_nocb_kthread(void *arg)
 		tail = rdp->nocb_follower_tail;
 		rdp->nocb_follower_tail = &rdp->nocb_follower_head;
 		raw_spin_unlock_irqrestore(&rdp->nocb_lock, flags);
-		BUG_ON(!list);
+		if (WARN_ON_ONCE(!list))
+			continue;
 		trace_rcu_nocb_wake(rcu_state.name, rdp->cpu, TPS("WokeNonEmpty"));
 
 		/* Each pass through the following loop invokes a callback. */

@@ -2495,7 +2497,8 @@ static void rcu_spawn_one_nocb_kthread(int cpu)
 	/* Spawn the kthread for this CPU. */
 	t = kthread_run(rcu_nocb_kthread, rdp_spawn,
 			"rcuo%c/%d", rcu_state.abbr, cpu);
-	BUG_ON(IS_ERR(t));
+	if (WARN_ONCE(IS_ERR(t), "%s: Could not start rcuo kthread, OOM is now expected behavior\n", __func__))
+		return;
 	WRITE_ONCE(rdp_spawn->nocb_kthread, t);
 }
Signature · 1 line
2.17.1
author
#6 lore raw

PATCH 4/4 rcu: Eliminate BUG_ON() for kernel/rcu/update.c

In reply to Paul E. McKenney

The update.c file has a number of calls to BUG_ON(), which panics the kernel, which is not a good strategy for devices (like embedded) that don't have a way to capture console output. This commit therefore converts these BUG_ON() calls to WARN_ON_ONCE() and WARN_ONCE().

Reported-by Linus Torvalds <[email protected]>
Signed-off-by Paul E. McKenney <[email protected]>
---
 kernel/rcu/update.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
kernel/rcu/update.c +2 -1
index f203b94f6b5b..cb26de25658a 100644
@@ -822,7 +822,8 @@ static int __init rcu_spawn_tasks_kthread(void)
 	struct task_struct *t;
 
 	t = kthread_run(rcu_tasks_kthread, NULL, "rcu_tasks_kthread");
-	BUG_ON(IS_ERR(t));
+	if (WARN_ONCE(IS_ERR(t), "%s: Could not start Tasks-RCU grace-period kthread, OOM is now expected behavior\n", __func__))
+		return 0;
 	smp_mb(); /* Ensure others see full kthread. */
 	WRITE_ONCE(rcu_tasks_kthread_ptr, t);
 	return 0;
Signature · 1 line
2.17.1