Message-ID: <20210422212448.GJ975577@paulmck-ThinkPad-P17-Gen-1>
Date:   Thu, 22 Apr 2021 14:24:48 -0700
From:   "Paul E. McKenney" <paulmck@...nel.org>
To:     Paul Gortmaker <paul.gortmaker@...driver.com>
Cc:     linux-kernel@...r.kernel.org, Ingo Molnar <mingo@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Frederic Weisbecker <frederic@...nel.org>
Subject: Re: [PATCH] sched/isolation: reconcile rcu_nocbs= and nohz_full=

On Mon, Apr 19, 2021 at 12:26:59AM -0400, Paul Gortmaker wrote:
> We have a mismatch between RCU and isolation -- in relation to what is
> considered the maximum valid CPU number.
> 
> This matters because nohz_full= and rcu_nocbs= are joined at the hip; in
> fact the former will enforce the latter.  So we don't want a CPU mask to
> be valid for one and denied for the other.
> 
> The difference 1st appeared as of v4.15; further details are below.

I pulled this into -rcu for testing and further review.

If it should instead go through some other tree:

Acked-by: Paul E. McKenney <paulmck@...nel.org>

> As it is confusing to anyone who isn't looking at the code regularly, a
> reminder is in order; three values exist here:
> 
> CONFIG_NR_CPUS	- compiled in maximum cap on number of CPUs supported.
> nr_cpu_ids 	- possible # of CPUs (typically reflects what ACPI says)
> cpus_present	- actual number of present/detected/installed CPUs.
> 
> For this example, I'll refer to NR_CPUS=64 from "make defconfig" and
> nr_cpu_ids=6 for ACPI reporting on a board that could run a six core,
> and present=4 for a quad that is physically in the socket.  From dmesg:
> 
>  smpboot: Allowing 6 CPUs, 2 hotplug CPUs
>  setup_percpu: NR_CPUS:64 nr_cpumask_bits:64 nr_cpu_ids:6 nr_node_ids:1
>  rcu: 	RCU restricting CPUs from NR_CPUS=64 to nr_cpu_ids=6.
>  smp: Brought up 1 node, 4 CPUs
> 
> And from userspace, see:
> 
>    paul@...sh:/sys/devices/system/cpu$ cat present
>    0-3
>    paul@...sh:/sys/devices/system/cpu$ cat possible
>    0-5
>    paul@...sh:/sys/devices/system/cpu$ cat kernel_max
>    63
> 
> Everything is fine if we boot 5x5 for rcu/nohz:
> 
>   Command line: BOOT_IMAGE=/boot/bzImage nohz_full=2-5 rcu_nocbs=2-5 root=/dev/sda1 ro
>   NO_HZ: Full dynticks CPUs: 2-5.
>   rcu: 	Offload RCU callbacks from CPUs: 2-5.
> 
> ..even though there is no CPU 4 or 5.  Both RCU and nohz_full are OK.
> Now we push that > 6 but less than NR_CPU and with 15x15 we get:
> 
>   Command line: BOOT_IMAGE=/boot/bzImage rcu_nocbs=2-15 nohz_full=2-15 root=/dev/sda1 ro
>   rcu: 	Note: kernel parameter 'rcu_nocbs=', 'nohz_full', or 'isolcpus=' contains nonexistent CPUs.
>   rcu: 	Offload RCU callbacks from CPUs: 2-5.
> 
> These are both functionally equivalent, as we are only changing flags on
> phantom CPUs that don't exist, but note the kernel interpretation changes.
> And worse, it only changes for one of the two - which is the problem.
> 
> RCU doesn't care if you want to restrict the flags on phantom CPUs but
> clearly nohz_full does after this change from v4.15 (edb9382175c3):
> 
> -       if (cpulist_parse(str, non_housekeeping_mask) < 0) {
> -               pr_warn("Housekeeping: Incorrect nohz_full cpumask\n");
> +       err = cpulist_parse(str, non_housekeeping_mask);
> +       if (err < 0 || cpumask_last(non_housekeeping_mask) >= nr_cpu_ids) {
> +               pr_warn("Housekeeping: nohz_full= or isolcpus= incorrect CPU range\n");
> 
> To be clear, the sanity check on "possible" (nr_cpu_ids) is new here.
> 
> The goal was reasonable ; not wanting housekeeping to land on a
> not-possible CPU, but note two things:
> 
> 1) this is an exclusion list, not an inclusion list; we are tracking
> non_housekeeping CPUs; not ones who are explicitly assigned housekeeping
> 
> 2) we went one further in 9219565aa890 - ensuring that housekeeping was
> sanity checking against present and not just possible CPUs.
> 
> To be clear, this means the check added in v4.15 is doubly redundant.
> And more importantly, overly strict/restrictive.
> 
> We care now, because the bitmap boot arg parsing now knows that a value
> of "N" is NR_CPUS; the size of the bitmap, but the bitmap code doesn't
> know anything about the subtleties of our max/possible/present CPU
> specifics as outlined above.
> 
> So drop the check added in v4.15 (edb9382175c3) and make RCU and
> nohz_full both in alignment again on NR_CPUS so "N" works for both,
> and then they can fall back to nr_cpu_ids internally just as before.
> 
>   Command line: BOOT_IMAGE=/boot/bzImage nohz_full=2-N rcu_nocbs=2-N root=/dev/sda1 ro
>   NO_HZ: Full dynticks CPUs: 2-5.
>   rcu: 	Offload RCU callbacks from CPUs: 2-5.
> 
> As shown above, with this change, RCU and nohz_full are in sync, even
> with the use of the "N" placeholder.  Same result is achieved with "15".
> 
> Cc: Ingo Molnar <mingo@...hat.com>
> Cc: Peter Zijlstra <peterz@...radead.org>
> Cc: Paul E. McKenney <paulmck@...nel.org>
> Cc: Frederic Weisbecker <frederic@...nel.org>
> Signed-off-by: Paul Gortmaker <paul.gortmaker@...driver.com>
> 
> diff --git a/kernel/sched/isolation.c b/kernel/sched/isolation.c
> index 5a6ea03f9882..7f06eaf12818 100644
> --- a/kernel/sched/isolation.c
> +++ b/kernel/sched/isolation.c
> @@ -81,11 +81,9 @@ static int __init housekeeping_setup(char *str, enum hk_flags flags)
>  {
>  	cpumask_var_t non_housekeeping_mask;
>  	cpumask_var_t tmp;
> -	int err;
>  
>  	alloc_bootmem_cpumask_var(&non_housekeeping_mask);
> -	err = cpulist_parse(str, non_housekeeping_mask);
> -	if (err < 0 || cpumask_last(non_housekeeping_mask) >= nr_cpu_ids) {
> +	if (cpulist_parse(str, non_housekeeping_mask) < 0) {
>  		pr_warn("Housekeeping: nohz_full= or isolcpus= incorrect CPU range\n");
>  		free_bootmem_cpumask_var(non_housekeeping_mask);
>  		return 0;
> -- 
> 2.25.1
> 

Powered by blists - more mailing lists