Re: [PATCH rcu 4/7] rcu/nocb: Add an option to offload all CPUs on boot
From: Joel Fernandes
Date: Tue Jul 19 2022 - 18:42:09 EST
On 7/19/2022 2:12 PM, Paul E. McKenney wrote:
> On Tue, Jul 19, 2022 at 03:04:07PM +0530, Neeraj Upadhyay wrote:
>>
>>
>> On 6/21/2022 4:15 AM, Paul E. McKenney wrote:
>>> From: Joel Fernandes <joel@xxxxxxxxxxxxxxxxx>
>>>
>>> Systems built with CONFIG_RCU_NOCB_CPU=y but booted without either
>>> the rcu_nocbs= or rcu_nohz_full= kernel-boot parameters will not have
>>> callback offloading on any of the CPUs, nor can any of the CPUs be
>>> switched to enable callback offloading at runtime. Although this is
>>> intentional, it would be nice to have a way to offload all the CPUs
>>> without having to make random bootloaders specify either the rcu_nocbs=
>>> or the rcu_nohz_full= kernel-boot parameters.
>>>
>>> This commit therefore provides a new CONFIG_RCU_NOCB_CPU_DEFAULT_ALL
>>> Kconfig option that switches the default so as to offload callback
>>> processing on all of the CPUs. This default can still be overridden
>>> using the rcu_nocbs= and rcu_nohz_full= kernel-boot parameters.
>>>
>>> Reviewed-by: Kalesh Singh <kaleshsingh@xxxxxxxxxx>
>>> Reviewed-by: Uladzislau Rezki <urezki@xxxxxxxxx>
>>> (In v4.1, fixed issues with CONFIG maze reported by kernel test robot).
>>> Reported-by: kernel test robot <lkp@xxxxxxxxx>
>>> Signed-off-by: Joel Fernandes <joel@xxxxxxxxxxxxxxxxx>
>>> Signed-off-by: Paul E. McKenney <paulmck@xxxxxxxxxx>
>>> ---
>>
>>
>> Reviewed-by: Neeraj Upadhyay <quic_neeraju@xxxxxxxxxxx>
>>
>> One query on cpumask_setall() below
>>
>>> Documentation/admin-guide/kernel-parameters.txt | 6 ++++++
>>> kernel/rcu/Kconfig | 13 +++++++++++++
>>> kernel/rcu/tree_nocb.h | 15 ++++++++++++++-
>>> 3 files changed, 33 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
>>> index 2522b11e593f2..34605c275294c 100644
>>> --- a/Documentation/admin-guide/kernel-parameters.txt
>>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>>> @@ -3659,6 +3659,9 @@
>>> just as if they had also been called out in the
>>> rcu_nocbs= boot parameter.
>>> + Note that this argument takes precedence over
>>> + the CONFIG_RCU_NOCB_CPU_DEFAULT_ALL option.
>>> +
>>> noiotrap [SH] Disables trapped I/O port accesses.
>>> noirqdebug [X86-32] Disables the code which attempts to detect and
>>> @@ -4557,6 +4560,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.
>>> +
>>> 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 1c630e573548d..27aab870ae4cf 100644
>>> --- a/kernel/rcu/Kconfig
>>> +++ b/kernel/rcu/Kconfig
>>> @@ -262,6 +262,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
>>> + default n
>>> + help
>>> + Use this option to offload callback processing from all CPUs
>>> + by default, in the absence of the rcu_nocbs or nohz_full boot
>>> + parameter. 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 && TASKS_TRACE_RCU
>>> diff --git a/kernel/rcu/tree_nocb.h b/kernel/rcu/tree_nocb.h
>>> index 4cf9a29bba79d..60cc92cc66552 100644
>>> --- a/kernel/rcu/tree_nocb.h
>>> +++ b/kernel/rcu/tree_nocb.h
>>> @@ -1197,11 +1197,21 @@ void __init rcu_init_nohz(void)
>>> {
>>> int cpu;
>>> bool need_rcu_nocb_mask = false;
>>> + bool offload_all = false;
>>> struct rcu_data *rdp;
>>> +#if defined(CONFIG_RCU_NOCB_CPU_DEFAULT_ALL)
>>> + if (!rcu_state.nocb_is_setup) {
>>> + need_rcu_nocb_mask = true;
>>> + offload_all = true;
>>> + }
>>> +#endif /* #if defined(CONFIG_RCU_NOCB_CPU_DEFAULT_ALL) */
>>> +
>>> #if defined(CONFIG_NO_HZ_FULL)
>>> - if (tick_nohz_full_running && !cpumask_empty(tick_nohz_full_mask))
>>> + if (tick_nohz_full_running && !cpumask_empty(tick_nohz_full_mask)) {
>>> need_rcu_nocb_mask = true;
>>> + offload_all = false; /* NO_HZ_FULL has its own mask. */
>>> + }
>>> #endif /* #if defined(CONFIG_NO_HZ_FULL) */
>>> if (need_rcu_nocb_mask) {
>>> @@ -1222,6 +1232,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);
>>
>> Do we need to do a cpumask_and(rcu_nocb_mask, cpu_possible_mask,
>> rcu_nocb_mask) after setting all cpus in rcu_nocb_mask (cpumask_subset()
>> check below takes care of it though)?
>
> Without that cpumask_and(), systems with sparse CPU numbering schemes
> (for example, 0, 4, 8, 12, ...) will get a pr_info(), and as you noted,
> the needed cpumask_and().
>
> I am inclined to see a complaint before we change this. And perhaps if
> this is to change, the change should be in cpumask_setall() rather than
> in rcu_init_nohz(). But that is an argument for later, if at all. ;-)
>
>>> +
>>> if (!cpumask_subset(rcu_nocb_mask, cpu_possible_mask)) {
We could also suppress the pr_info() by making it conditional.
like:
if (!CONFIG_RCU_NOCB_CPU_DEFAULT_ALL) {
pr_info(...);
}
In other words, we could make the cpumask_and() as expected/normal on
systems with sparse CPU numbering schemes. Would that work?
Thanks,
- Joel
>>> pr_info("\tNote: kernel parameter 'rcu_nocbs=', 'nohz_full', or 'isolcpus=' contains nonexistent CPUs.\n");
>>> cpumask_and(rcu_nocb_mask, cpu_possible_mask,