by Joel Fernandes

[permalink] [raw]

Subject: Re: [PATCH rcu 4/7] rcu/nocb: Add an option to offload all CPUs on boot

On 7/19/2022 6:53 PM, Paul E. McKenney wrote:
> On Tue, Jul 19, 2022 at 06:42:00PM -0400, Joel Fernandes wrote:
>>
>>
>> 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 <[email protected]>
>>>>>
>>>>> 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 <[email protected]>
>>>>> Reviewed-by: Uladzislau Rezki <[email protected]>
>>>>> (In v4.1, fixed issues with CONFIG maze reported by kernel test robot).
>>>>> Reported-by: kernel test robot <[email protected]>
>>>>> Signed-off-by: Joel Fernandes <[email protected]>
>>>>> Signed-off-by: Paul E. McKenney <[email protected]>
>>>>> ---
>>>>
>>>>
>>>> Reviewed-by: Neeraj Upadhyay <[email protected]>
>>>>
>>>> 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?
>
> That would be a good within-RCU workaround if we get an urgent complaint,
> but if this requires a change, shouldn't cpumask_setall() refrain from
> setting bits for non-existent CPUs? It does refrain from setting any
> bits beyond the largest-numbered CPU.
>
> But perhaps there is an early boot reason why cpumask_setall() cannot
> do this?

Agreed, it would be great if it did not set those bits. I checked other
places in the kernel like kernel/sched/core.c and cannot find that it is
masking the bits after the setall(), so maybe its Ok?

> Either way, we are just doing a pr_info(), not a WARN_ON() or similar,
> so the current state is probably fine.

Agreed, thanks.

- Joel

>
> Thanx, Paul
>
>> 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,