Re: [PATCH] rcu: Clean up rcu_init_nohz() by removing unnecessary statements

[Paul E. McKenney]

On Wed, Feb 28, 2018 at 06:04:55PM +0900, Byungchul Park wrote:
> Since the commit 44c65ff2e3b0(rcu: Eliminate NOCBs CPU-state Kconfig
> options) made nocb-cpus identified only through the rcu_nocbs= boot
> parameter, we don't have to care NOCBs CPU-state Kconfig options
> anymore, which means now we can just rely on rcu_nocb_mask to
> decide whether going ahead in rcu_init_nohz().
> 
> Remove the deprecated code.
> 
> Signed-off-by: Byungchul Park <byungchul.park@xxxxxxx>

Good catch!  However, you missed a (relatively harmless) bug in my commit
44c65ff2e3b0, namely that if neither the nohz_full= nor the rcu_nocbs=
kernel boot parameters specify any CPUs, we don't need to allocate
rcu_nocb_mask.  (That is, when both of those parameters are omitted.)

Now, if the rcu_nocbs= kernel boot parameter was specified, we know that
rcu_nocb_mask was already allocated in rcu_nocb_setup().  So in
rcu_init_nohz() we only need to do the allocation if NO_HZ_FULL needs
some NOCBs CPUs, that is, when tick_nohz_full_running and when there
is at least one CPU specified in tick_nohz_full_mask.

So the change that is actually needed is to reverse the initialization
of need_rcu_nocb_mask, that is, to initialize it to false rather than
to true.  I annotated your patch with my reasoning and have a patch
below with your Reported-by.  Please take a look and let me know what
you think.

If I am not too confused, the only effect of this bug was to needlessly
allocate rcu_nocb_mask and to initialize it to all zeros bits, but please
let me know if I missed something.

But you did find a bug in RCU, so you are ahead of the formal-verification
folks.  ;-)

							Thanx, Paul

> ---
>  kernel/rcu/tree_plugin.h | 10 +---------
>  1 file changed, 1 insertion(+), 9 deletions(-)
> 
> diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
> index b0d7f9b..510a6af 100644
> --- a/kernel/rcu/tree_plugin.h
> +++ b/kernel/rcu/tree_plugin.h
> @@ -2313,22 +2313,14 @@ static void do_nocb_deferred_wakeup(struct rcu_data *rdp)
>  void __init rcu_init_nohz(void)
>  {
>  	int cpu;
> -	bool need_rcu_nocb_mask = true;

I initialize this to "false" instead of "true" as discussed above.

>  	struct rcu_state *rsp;
> 
> -#if defined(CONFIG_NO_HZ_FULL)
> -	if (tick_nohz_full_running && cpumask_weight(tick_nohz_full_mask))
> -		need_rcu_nocb_mask = true;
> -#endif /* #if defined(CONFIG_NO_HZ_FULL) */

The above is needed to decide if NO_HZ_FULL needs NOCBs CPUs.

> -
> -	if (!cpumask_available(rcu_nocb_mask) && need_rcu_nocb_mask) {
> +	if (!cpumask_available(rcu_nocb_mask)) {

And we still need this check.  If the rcu_nocbs= kernel boot parameter
was not specified and if NO_HZ_FULL doesn't need NOCBs CPUs, we shouldn't
allocate rcu_nocb_mask.

>  		if (!zalloc_cpumask_var(&rcu_nocb_mask, GFP_KERNEL)) {
>  			pr_info("rcu_nocb_mask allocation failed, callback offloading disabled.\n");
>  			return;
>  		}
>  	}
> -	if (!cpumask_available(rcu_nocb_mask))
> -		return;

This check remains as an optimization.  We -could- execute the code
below even if rcu_nocb_mask had not been allocated, but the code below
would end up painstakingly rechecking rcu_nocb_mask for each CPU for
each flavor of RCU.  So why not just take an early exit in that case?

>  #if defined(CONFIG_NO_HZ_FULL)
>  	if (tick_nohz_full_running)

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

commit 628e565ede22b0155bf2e0daed4d70ea0b5e6d65
Author: Paul E. McKenney <paulmck@xxxxxxxxxxxxxxxxxx>
Date:   Wed Feb 28 10:34:54 2018 -0800

    rcu: Don't allocate rcu_nocb_mask if no one needs it
    
    Commit 44c65ff2e3b0 ("rcu: Eliminate NOCBs CPU-state Kconfig options")
    made allocation of rcu_nocb_mask depend only on the rcu_nocbs=,
    nohz_full=, or isolcpus= kernel boot parameters.  However, it failed
    to change the initial value of rcu_init_nohz()'s local variable
    need_rcu_nocb_mask to false, which can result in useless allocation
    of an all-zero rcu_nocb_mask.  This commit therefore fixes this bug by
    changing the initial value of need_rcu_nocb_mask to false.
    
    While we are in the area, also correct the error message that is printed
    when someone specifies that can-never-exist CPUs should be NOCBs CPUs.
    
    Reported-by: Byungchul Park <byungchul.park@xxxxxxx>
    Signed-off-by: Paul E. McKenney <paulmck@xxxxxxxxxxxxxxxxxx>

diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
index b0d7f9ba6bf2..fb3d20d868ed 100644
--- a/kernel/rcu/tree_plugin.h
+++ b/kernel/rcu/tree_plugin.h
@@ -2313,7 +2313,7 @@ static void do_nocb_deferred_wakeup(struct rcu_data *rdp)
 void __init rcu_init_nohz(void)
 {
 	int cpu;
-	bool need_rcu_nocb_mask = true;
+	bool need_rcu_nocb_mask = false;
 	struct rcu_state *rsp;
 
 #if defined(CONFIG_NO_HZ_FULL)
@@ -2336,7 +2336,7 @@ void __init rcu_init_nohz(void)
 #endif /* #if defined(CONFIG_NO_HZ_FULL) */
 
 	if (!cpumask_subset(rcu_nocb_mask, cpu_possible_mask)) {
-		pr_info("\tNote: kernel parameter 'rcu_nocbs=' contains nonexistent CPUs.\n");
+		pr_info("\tNote: kernel parameter 'rcu_nocbs=', 'nohz_full', or 'isolcpus=' contains nonexistent CPUs.\n");
 		cpumask_and(rcu_nocb_mask, cpu_possible_mask,
 			    rcu_nocb_mask);
 	}