[PATCH tip/core/rcu 0/4] Eliminate BUG_ON for v4.21/v5.0

[PATCH tip/core/rcu 0/4] Eliminate BUG_ON for v4.21/v5.0

[Paul E. McKenney]

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(-)

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

[Paul E. McKenney]

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 <torvalds@linux-foundation.org>
Signed-off-by: Paul E. McKenney <paulmck@linux.ibm.com>
Acked-by: Oleg Nesterov <oleg@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
---
 kernel/rcu/sync.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/kernel/rcu/sync.c b/kernel/rcu/sync.c
index 3f943efcf61c..a6ba446a9693 100644
--- a/kernel/rcu/sync.c
+++ b/kernel/rcu/sync.c
@@ -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);
 	}
 }
-- 
2.17.1

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

[Steven Rostedt]

On Sun, 11 Nov 2018 11:32:14 -0800
"Paul E. McKenney" <paulmck@linux.ibm.com> wrote:

> 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 <torvalds@linux-foundation.org>
> Signed-off-by: Paul E. McKenney <paulmck@linux.ibm.com>
> Acked-by: Oleg Nesterov <oleg@redhat.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> ---
>  kernel/rcu/sync.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/kernel/rcu/sync.c b/kernel/rcu/sync.c
> index 3f943efcf61c..a6ba446a9693 100644
> --- a/kernel/rcu/sync.c
> +++ b/kernel/rcu/sync.c
> @@ -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);
>  	}
>  }

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

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

[Paul E. McKenney]

On Sun, Nov 11, 2018 at 09:07:04PM -0500, Steven Rostedt wrote:
> On Sun, 11 Nov 2018 11:32:14 -0800
> "Paul E. McKenney" <paulmck@linux.ibm.com> wrote:
> 
> > 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 <torvalds@linux-foundation.org>
> > Signed-off-by: Paul E. McKenney <paulmck@linux.ibm.com>
> > Acked-by: Oleg Nesterov <oleg@redhat.com>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > ---
> >  kernel/rcu/sync.c | 13 ++++++-------
> >  1 file changed, 6 insertions(+), 7 deletions(-)
> > 
> > diff --git a/kernel/rcu/sync.c b/kernel/rcu/sync.c
> > index 3f943efcf61c..a6ba446a9693 100644
> > --- a/kernel/rcu/sync.c
> > +++ b/kernel/rcu/sync.c
> > @@ -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);
> >  	}
> >  }
> 
> 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

[PATCH tip/core/rcu 2/4] rcu: Eliminate BUG_ON() for kernel/rcu/tree.c

[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 <torvalds@linux-foundation.org>
Signed-off-by: Paul E. McKenney <paulmck@linux.ibm.com>
---
 kernel/rcu/tree.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 121f833acd04..bdb6659a8dbc 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -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;
-- 
2.17.1

[PATCH tip/core/rcu 3/4] rcu: Eliminate BUG_ON() for kernel/rcu/tree_plugin.h

[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 <torvalds@linux-foundation.org>
Signed-off-by: Paul E. McKenney <paulmck@linux.ibm.com>
---
 kernel/rcu/tree_plugin.h | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
index 05915e536336..105a19752ac2 100644
--- a/kernel/rcu/tree_plugin.h
+++ b/kernel/rcu/tree_plugin.h
@@ -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);
 }
 
-- 
2.17.1

[PATCH tip/core/rcu 4/4] rcu: Eliminate BUG_ON() for kernel/rcu/update.c

[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 <torvalds@linux-foundation.org>
Signed-off-by: Paul E. McKenney <paulmck@linux.ibm.com>
---
 kernel/rcu/update.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c
index f203b94f6b5b..cb26de25658a 100644
--- a/kernel/rcu/update.c
+++ b/kernel/rcu/update.c
@@ -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;
-- 
2.17.1