[PATCH v2] kernel/isolation: Assert that a housekeeping CPU comes up at boot time

[PATCH v2] kernel/isolation: Assert that a housekeeping CPU comes up at boot time

[Nicholas Piggin]

With the change to allow the boot CPU0 to be isolated, it is possible
to specify command line options that result in no housekeeping CPU
online at boot.

An 8 CPU system booted with "nohz_full=0-6 maxcpus=4", for example.

It is not easily possible at housekeeping init time to know all the
various SMP options that will result in an invalid configuration, so
this patch adds a sanity check after SMP init, to ensure that a
housekeeping CPU has been onlined.

The panic is undesirable, but it's better than the alternative of an
obscure non deterministic failure. The panic will reliably happen
when advanced parameters are used incorrectly.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
v2: Fix a NULL pointer dereference when not overriding housekeeping,
    noticed by kernel test robot and Qais, who fixed it and verified
    the fix (thanks!)

 kernel/sched/isolation.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/kernel/sched/isolation.c b/kernel/sched/isolation.c
index 123ea07a3f3b..a9ca8628c1a2 100644
--- a/kernel/sched/isolation.c
+++ b/kernel/sched/isolation.c
@@ -63,6 +63,32 @@ void __init housekeeping_init(void)
 	WARN_ON_ONCE(cpumask_empty(housekeeping_mask));
 }
 
+static int __init housekeeping_verify_smp(void)
+{
+	int cpu;
+
+	if (!housekeeping_flags)
+		return 0;
+
+	/*
+	 * Early housekeeping setup is done before CPUs come up, and there are
+	 * a range of options scattered around that can restrict which CPUs
+	 * come up. It is possible to pass in a combination of housekeeping
+	 * and SMP arguments that result in housekeeping assigned to an
+	 * offline CPU.
+	 *
+	 * Check that condition here after SMP comes up, and give a useful
+	 * error message rather than an obscure non deterministic crash or
+	 * hang later.
+	 */
+	for_each_online_cpu(cpu) {
+		if (cpumask_test_cpu(cpu, housekeeping_mask))
+			return 0;
+	}
+	panic("Housekeeping: nohz_full= or isolcpus= resulted in no online CPUs for housekeeping.\n");
+}
+core_initcall(housekeeping_verify_smp);
+
 static int __init housekeeping_setup(char *str, enum hk_flags flags)
 {
 	cpumask_var_t non_housekeeping_mask;
-- 
2.20.1

Re: [PATCH v2] kernel/isolation: Assert that a housekeeping CPU comes up at boot time

[Frederic Weisbecker]

On Tue, Jun 25, 2019 at 10:17:20AM +1000, Nicholas Piggin wrote:
> +static int __init housekeeping_verify_smp(void)
> +{
> +	int cpu;
> +
> +	if (!housekeeping_flags)
> +		return 0;
> +
> +	/*
> +	 * Early housekeeping setup is done before CPUs come up, and there are
> +	 * a range of options scattered around that can restrict which CPUs
> +	 * come up. It is possible to pass in a combination of housekeeping
> +	 * and SMP arguments that result in housekeeping assigned to an
> +	 * offline CPU.
> +	 *
> +	 * Check that condition here after SMP comes up, and give a useful
> +	 * error message rather than an obscure non deterministic crash or
> +	 * hang later.
> +	 */
> +	for_each_online_cpu(cpu) {
> +		if (cpumask_test_cpu(cpu, housekeeping_mask))
> +			return 0;
> +	}
> +	panic("Housekeeping: nohz_full= or isolcpus= resulted in no online CPUs for housekeeping.\n");

Ok let's keep the panic. But let's simplify and spare long iterations off boot load:

    if (!cpumask_intersects(cpu_online_mask, housekeeping_mask))
        panic(...);

Thanks.

Re: [PATCH v2] kernel/isolation: Assert that a housekeeping CPU comes up at boot time

[Qais Yousef]

On 06/25/19 10:17, Nicholas Piggin wrote:
> With the change to allow the boot CPU0 to be isolated, it is possible
> to specify command line options that result in no housekeeping CPU
> online at boot.
> 
> An 8 CPU system booted with "nohz_full=0-6 maxcpus=4", for example.
> 
> It is not easily possible at housekeeping init time to know all the
> various SMP options that will result in an invalid configuration, so
> this patch adds a sanity check after SMP init, to ensure that a
> housekeeping CPU has been onlined.
> 
> The panic is undesirable, but it's better than the alternative of an
> obscure non deterministic failure. The panic will reliably happen
> when advanced parameters are used incorrectly.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> v2: Fix a NULL pointer dereference when not overriding housekeeping,
>     noticed by kernel test robot and Qais, who fixed it and verified
>     the fix (thanks!)

Glad I could help. But for the record my problem wasn't a NULL pointer
dereference and simply the loop didn't hit the condition to 'return 0' so I hit
the panic.

I tested that the fix does indeed skip the verification if no nohz_full nor
isolcpus is passed. But I didn't do the reverse check, although from the code
this flag is only set in housekeeping_setup() so it should continue to work as
intended.

Thanks!

--
Qais Yousef