* [PATCH] rcu: Clean up rcu_init_nohz() by removing unnecessary statements @ 2018-02-28 9:04 Byungchul Park 2018-02-28 18:41 ` Paul E. McKenney 0 siblings, 1 reply; 4+ messages in thread From: Byungchul Park @ 2018-02-28 9:04 UTC (permalink / raw) To: jiangshanlai, paulmck, josh, rostedt, mathieu.desnoyers Cc: linux-kernel, kernel-team 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] rcu: Clean up rcu_init_nohz() by removing unnecessary statements 2018-02-28 9:04 [PATCH] rcu: Clean up rcu_init_nohz() by removing unnecessary statements Byungchul Park @ 2018-02-28 18:41 ` Paul E. McKenney 2018-03-02 0:01 ` Byungchul Park 0 siblings, 1 reply; 4+ messages in thread From: Paul E. McKenney @ 2018-02-28 18:41 UTC (permalink / raw) To: Byungchul Park Cc: jiangshanlai, josh, rostedt, mathieu.desnoyers, linux-kernel, kernel-team 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); } ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] rcu: Clean up rcu_init_nohz() by removing unnecessary statements 2018-02-28 18:41 ` Paul E. McKenney @ 2018-03-02 0:01 ` Byungchul Park 2018-03-02 1:00 ` Paul E. McKenney 0 siblings, 1 reply; 4+ messages in thread From: Byungchul Park @ 2018-03-02 0:01 UTC (permalink / raw) To: paulmck Cc: jiangshanlai, josh, rostedt, mathieu.desnoyers, linux-kernel, kernel-team 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] rcu: Clean up rcu_init_nohz() by removing unnecessary statements 2018-03-02 0:01 ` Byungchul Park @ 2018-03-02 1:00 ` Paul E. McKenney 0 siblings, 0 replies; 4+ messages in thread From: Paul E. McKenney @ 2018-03-02 1:00 UTC (permalink / raw) To: Byungchul Park Cc: jiangshanlai, josh, rostedt, mathieu.desnoyers, linux-kernel, kernel-team 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 > ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-03-02 1:00 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-02-28 9:04 [PATCH] rcu: Clean up rcu_init_nohz() by removing unnecessary statements Byungchul Park 2018-02-28 18:41 ` Paul E. McKenney 2018-03-02 0:01 ` Byungchul Park 2018-03-02 1:00 ` Paul E. McKenney
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.