Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Fri, 15 Apr 2022 10:35:15 -0700
From:   "Paul E. McKenney" <paulmck@kernel.org>
To:     "Joel Fernandes (Google)" <joel@joelfernandes.org>
Cc:     linux-kernel@vger.kernel.org,
        Josh Triplett <josh@joshtriplett.org>,
        Lai Jiangshan <jiangshanlai@gmail.com>,
        Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
        rcu@vger.kernel.org, Steven Rostedt <rostedt@goodmis.org>,
        rushikesh.s.kadam@intel.com, vineethrp@gmail.com, urezki@gmail.com
Subject: Re: [PATCH v3] rcu/nocb: Add an option to offload all CPUs on boot
Message-ID: <20220415173515.GL4285@paulmck-ThinkPad-P17-Gen-1>
Reply-To: paulmck@kernel.org
References: <20220415160224.1904505-1-joel@joelfernandes.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220415160224.1904505-1-joel@joelfernandes.org>
Precedence: bulk

On Fri, Apr 15, 2022 at 04:02:24PM +0000, Joel Fernandes (Google) wrote:
> From: Joel Fernandes <joel@joelfernandes.org>

Much better, thank you!

> On systems with CONFIG_RCU_NOCB_CPU=y, there is no default mask provided
> which ends up not offloading any CPU. This patch removes a dependency
> from the bootloader having to know about RCU, about how many CPUs the
> system has, and about how to provide the mask.

The "about how many CPUs the system has" does not apply to current
mainline, in which "rcu_nocbs=0-N" says to offload all CPUs.  It can be
added back in for a backport to v5.10.  ;-)

> With the new option enabled, all CPUs will be offloaded on boot.
> 
> Signed-off-by: Joel Fernandes <joel@joelfernandes.org>

I second Joel's call for testing and add to that a call for additional
review.

My thought is to queue this after some independent testing.

> ---
> v2 was forcing the option to override no_cbs=
> v3 is back to v1 but with a config option defaulting to 'n'.
> 
>  Documentation/admin-guide/kernel-parameters.txt |  3 +++
>  kernel/rcu/Kconfig                              | 13 +++++++++++++
>  kernel/rcu/tree_nocb.h                          | 16 ++++++++++++++--
>  3 files changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index f5a27f067db9..7648a7dd335e 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -4398,6 +4398,9 @@
>  			no-callback mode from boot but the mode may be
>  			toggled at runtime via cpusets.
>  
> +			Note that this argument takes precedence over
> +			the CONFIG_RCU_NOCB_CPU_DEFAULT_ALL option.

Very good, thank you!

Do we want to say the same about nohz_full?  Or am I misreading the
code below?

> +
>  	rcu_nocb_poll	[KNL]
>  			Rather than requiring that offloaded CPUs
>  			(specified by rcu_nocbs= above) explicitly
> diff --git a/kernel/rcu/Kconfig b/kernel/rcu/Kconfig
> index bf8e341e75b4..2f8bd694ed85 100644
> --- a/kernel/rcu/Kconfig
> +++ b/kernel/rcu/Kconfig
> @@ -223,6 +223,19 @@ config RCU_NOCB_CPU
>  	  Say Y here if you need reduced OS jitter, despite added overhead.
>  	  Say N here if you are unsure.
>  
> +config RCU_NOCB_CPU_DEFAULT_ALL
> +	bool "Offload RCU callback processing from all CPUs by default"
> +	depends on RCU_NOCB_CPU

The needed dependency on RCU_EXPERT is provided transitively via
RCU_NOCB_CPU, so this should be OK.  (To check, build a .config file,
queue this patch, and do "make oldconfig".  If any questions are asked,
a change is needed.)

> +	default n
> +	help
> +	  Use this option to offload callback processing from all CPUs
> +	  by default, in the absence of the rcu_nocbs boot parameter.

And also in the absence of the nohz_full boot parameter, correct?

> +	  This also avoids the need to use any boot parameters to achieve
> +	  the effect of offloading all CPUs on boot.
> +
> +	  Say Y here if you want offload all CPUs by default on boot.
> +	  Say N here if you are unsure.
> +
>  config TASKS_TRACE_RCU_READ_MB
>  	bool "Tasks Trace RCU readers use memory barriers in user and idle"
>  	depends on RCU_EXPERT
> diff --git a/kernel/rcu/tree_nocb.h b/kernel/rcu/tree_nocb.h
> index eeafb546a7a0..673fa0d1f801 100644
> --- a/kernel/rcu/tree_nocb.h
> +++ b/kernel/rcu/tree_nocb.h
> @@ -1165,12 +1165,21 @@ EXPORT_SYMBOL_GPL(rcu_nocb_cpu_offload);
>  void __init rcu_init_nohz(void)
>  {
>  	int cpu;
> -	bool need_rcu_nocb_mask = false;
> +	bool need_rcu_nocb_mask = false, offload_all = false;

Please use the extra line to perserve alphabetical order.  (I do know
about inverse tree, though I have no idea why it is considered to be a
good thing.)

>  	struct rcu_data *rdp;
>  
> +#if defined(CONFIG_RCU_NOCB_CPU_DEFAULT_ALL)
> +	if (!rcu_nocb_is_setup) {
> +		need_rcu_nocb_mask = true;
> +		offload_all = true;
> +	}
> +#endif

It would be nice to be able to use IS_ENABLED() here and below, but that
is a separate issue.  For one thing, need_rcu_nocb_mask would need to
be defined unconditionally.  So this is fine as it is for this patch.

> +
>  #if defined(CONFIG_NO_HZ_FULL)
> -	if (tick_nohz_full_running && cpumask_weight(tick_nohz_full_mask))
> +	if (tick_nohz_full_running && cpumask_weight(tick_nohz_full_mask)) {
>  		need_rcu_nocb_mask = true;
> +		offload_all = false; /* NO_HZ_FULL has its own mask. */

This is good, and is also why I am asking for the addition of nohz_full
above.

> +	}
>  #endif /* #if defined(CONFIG_NO_HZ_FULL) */
>  
>  	if (need_rcu_nocb_mask) {
> @@ -1191,6 +1200,9 @@ void __init rcu_init_nohz(void)
>  		cpumask_or(rcu_nocb_mask, rcu_nocb_mask, tick_nohz_full_mask);
>  #endif /* #if defined(CONFIG_NO_HZ_FULL) */
>  
> +	if (offload_all)
> +		cpumask_setall(rcu_nocb_mask);
> +
>  	if (!cpumask_subset(rcu_nocb_mask, cpu_possible_mask)) {
>  		pr_info("\tNote: kernel parameter 'rcu_nocbs=', 'nohz_full', or 'isolcpus=' contains nonexistent CPUs.\n");
>  		cpumask_and(rcu_nocb_mask, cpu_possible_mask,
> -- 
> 2.36.0.rc0.470.gd361397f0d-goog
>