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

[PATCH] kernel/isolation: Asset 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>
---
 kernel/sched/isolation.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/kernel/sched/isolation.c b/kernel/sched/isolation.c
index 123ea07a3f3b..7b9e1e0d4ec3 100644
--- a/kernel/sched/isolation.c
+++ b/kernel/sched/isolation.c
@@ -63,6 +63,29 @@ void __init housekeeping_init(void)
 	WARN_ON_ONCE(cpumask_empty(housekeeping_mask));
 }
 
+static int __init housekeeping_verify_smp(void)
+{
+	int cpu;
+
+	/*
+	 * 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] kernel/isolation: Asset that a housekeeping CPU comes up at boot time

[Nicholas Piggin]

Nicholas Piggin's on June 1, 2019 9:39 pm:
> 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.

Ping on this one? This should resolve Frederic's remaining objection
to the series (at least until he solves it more generally).

As the series has already been merged, should we get this upstream
before release?

Thanks,
Nick

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

[Peter Zijlstra]

On Mon, Jun 10, 2019 at 05:24:32PM +1000, Nicholas Piggin wrote:
> Nicholas Piggin's on June 1, 2019 9:39 pm:
> > 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.
> 
> Ping on this one? This should resolve Frederic's remaining objection
> to the series (at least until he solves it more generally).
> 
> As the series has already been merged, should we get this upstream
> before release?

I was hoping for feedback from Frederic, lacking that, I've queued it
now.

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

[Frederic Weisbecker]

On Mon, Jun 17, 2019 at 05:59:31PM +0200, Peter Zijlstra wrote:
> On Mon, Jun 10, 2019 at 05:24:32PM +1000, Nicholas Piggin wrote:
> > Nicholas Piggin's on June 1, 2019 9:39 pm:
> > > 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.
> > 
> > Ping on this one? This should resolve Frederic's remaining objection
> > to the series (at least until he solves it more generally).
> > 
> > As the series has already been merged, should we get this upstream
> > before release?
> 
> I was hoping for feedback from Frederic, lacking that, I've queued it
> now.
> 

Sorry I just came back from vacation. Any chance we can use a WARN() instead?
I prefer to use panic() only when data is really threatened or such.

Thanks.

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

[Nicholas Piggin]

Frederic Weisbecker's on June 18, 2019 5:05 am:
> On Mon, Jun 17, 2019 at 05:59:31PM +0200, Peter Zijlstra wrote:
>> On Mon, Jun 10, 2019 at 05:24:32PM +1000, Nicholas Piggin wrote:
>> > Nicholas Piggin's on June 1, 2019 9:39 pm:
>> > > 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.
>> > 
>> > Ping on this one? This should resolve Frederic's remaining objection
>> > to the series (at least until he solves it more generally).
>> > 
>> > As the series has already been merged, should we get this upstream
>> > before release?
>> 
>> I was hoping for feedback from Frederic, lacking that, I've queued it
>> now.
>> 
> 
> Sorry I just came back from vacation. Any chance we can use a WARN() instead?
> I prefer to use panic() only when data is really threatened or such.

I thought it was decided to panic here, because we don't assign a house
keeping CPU so the system is unlikely to behave properly. A warn might
scroll off the screen by the time things grind to a halt.

This is a one-time boot parameter misconfiguration, many cases of which
can cause a panic and boot stop.

No question if we can make this more dynamic that would be better, but
for near term at least can we go with this?

Thanks,
Nick

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

[Qais Yousef]

On 06/01/19 21:39, 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>
> ---
>  kernel/sched/isolation.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/kernel/sched/isolation.c b/kernel/sched/isolation.c
> index 123ea07a3f3b..7b9e1e0d4ec3 100644
> --- a/kernel/sched/isolation.c
> +++ b/kernel/sched/isolation.c
> @@ -63,6 +63,29 @@ void __init housekeeping_init(void)
>  	WARN_ON_ONCE(cpumask_empty(housekeeping_mask));
>  }
>  
> +static int __init housekeeping_verify_smp(void)
> +{
> +	int cpu;
> +
> +	/*
> +	 * 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");

I am hitting this panic when I boot my juno board.


I have CONFIG_CPU_ISOLATION=y but I don't pass nohuz_full nor isolcpus in the
commandline. I think what's going on is that housekeeping_setup() doesn't get
called and hence housekeeping_mask isn't initialized in my case, causing this
check to fail and trigger the panic.

The below seems to 'fix' it though not sure if it's the right way forward.
A revert obviously fixes it too but I doubt we want that :-)


diff --git a/kernel/sched/isolation.c b/kernel/sched/isolation.c
index 7b9e1e0d4ec3..a9ca8628c1a2 100644
--- a/kernel/sched/isolation.c
+++ b/kernel/sched/isolation.c
@@ -67,6 +67,9 @@ 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


Cheers

--
Qais Yousef


> +}
> +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] kernel/isolation: Asset that a housekeeping CPU comes up at boot time

[Nicholas Piggin]

Qais Yousef's on June 24, 2019 8:57 pm:
> On 06/01/19 21:39, 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>
>> ---
>>  kernel/sched/isolation.c | 23 +++++++++++++++++++++++
>>  1 file changed, 23 insertions(+)
>> 
>> diff --git a/kernel/sched/isolation.c b/kernel/sched/isolation.c
>> index 123ea07a3f3b..7b9e1e0d4ec3 100644
>> --- a/kernel/sched/isolation.c
>> +++ b/kernel/sched/isolation.c
>> @@ -63,6 +63,29 @@ void __init housekeeping_init(void)
>>  	WARN_ON_ONCE(cpumask_empty(housekeeping_mask));
>>  }
>>  
>> +static int __init housekeeping_verify_smp(void)
>> +{
>> +	int cpu;
>> +
>> +	/*
>> +	 * 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");
> 
> I am hitting this panic when I boot my juno board.
> 
> 
> I have CONFIG_CPU_ISOLATION=y but I don't pass nohuz_full nor isolcpus in the
> commandline. I think what's going on is that housekeeping_setup() doesn't get
> called and hence housekeeping_mask isn't initialized in my case, causing this
> check to fail and trigger the panic.
> 
> The below seems to 'fix' it though not sure if it's the right way forward.
> A revert obviously fixes it too but I doubt we want that :-)

That'll do it. Thanks for the report and investigation.

> 
> 
> diff --git a/kernel/sched/isolation.c b/kernel/sched/isolation.c
> index 7b9e1e0d4ec3..a9ca8628c1a2 100644
> --- a/kernel/sched/isolation.c
> +++ b/kernel/sched/isolation.c
> @@ -67,6 +67,9 @@ 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
> 
> 
> Cheers
> 
> --
> Qais Yousef
> 
> 
>> +}
>> +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
>> 
>