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

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

[Byungchul Park]

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@lge.com>
---
 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;
 	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) */
-
-	if (!cpumask_available(rcu_nocb_mask) && need_rcu_nocb_mask) {
+	if (!cpumask_available(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;
 
 #if defined(CONFIG_NO_HZ_FULL)
 	if (tick_nohz_full_running)
-- 
1.9.1

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@lge.com>

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@linux.vnet.ibm.com>
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@lge.com>
    Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

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);
 	}

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

[Byungchul Park]

On 3/1/2018 3:41 AM, Paul E. McKenney wrote:
> 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@lge.com>
> 
> 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.

Why didn't I catch it in advance? :)

> 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.

No doubt. I agree with you.

Acked-by: Byungchul Park <byungchul.park@lge.com>

> 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.

I think so as you.

-- 
Thanks,
Byungchul

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

[Paul E. McKenney]

On Fri, Mar 02, 2018 at 09:01:19AM +0900, Byungchul Park wrote:
> On 3/1/2018 3:41 AM, Paul E. McKenney wrote:
> >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@lge.com>
> >
> >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.
> 
> Why didn't I catch it in advance? :)

I guess that sentiment goes well with my "why didn't I avoid that bug
in the first place?"  ;-)

> >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.
> 
> No doubt. I agree with you.
> 
> Acked-by: Byungchul Park <byungchul.park@lge.com>

Applied, thank you!

							Thanx, Paul

> >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.
> 
> I think so as you.
> 
> -- 
> Thanks,
> Byungchul
>