[PATCH] sched: Prevent raising SCHED_SOFTIRQ when CPU is !active

[PATCH] sched: Prevent raising SCHED_SOFTIRQ when CPU is !active

[Anna-Maria Behnsen]

SCHED_SOFTIRQ is raised to trigger periodic load balancing. When CPU is not
active, CPU should not participate in load balancing.

The scheduler uses nohz.idle_cpus_mask to keep track of the CPUs which can
do idle load balancing. When bringing a CPU up the CPU is added to the mask
when it reaches the active state, but on teardown the CPU stays in the mask
until it goes offline and invokes sched_cpu_dying().

When SCHED_SOFTIRQ is raised on a !active CPU, there might be a pending
softirq when stopping the tick which triggers a warning in NOHZ code. The
SCHED_SOFTIRQ can also be raised by the scheduler tick which has the same
issue.

Therefore remove the CPU from nohz.idle_cpus_mask when it is marked
inactive and also prevent the scheduler_tick() from raising SCHED_SOFTIRQ
after this point.

Signed-off-by: Anna-Maria Behnsen <anna-maria@linutronix.de>
---
 kernel/sched/core.c | 7 ++++++-
 kernel/sched/fair.c | 7 +++++--
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 21b548b69455..69284dc121d3 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -7492,6 +7492,12 @@ int sched_cpu_deactivate(unsigned int cpu)
 	struct rq_flags rf;
 	int ret;
 
+	/*
+	 * Remove CPU from nohz.idle_cpus_mask to prevent participating in
+	 * load balancing when not active
+	 */
+	nohz_balance_exit_idle(rq);
+
 	set_cpu_active(cpu, false);
 	/*
 	 * We've cleared cpu_active_mask, wait for all preempt-disabled and RCU
@@ -7598,7 +7604,6 @@ int sched_cpu_dying(unsigned int cpu)
 
 	calc_load_migrate(rq);
 	update_max_interval();
-	nohz_balance_exit_idle(rq);
 	hrtick_clear(rq);
 	return 0;
 }
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 04a3ce20da67..fd422b8eb859 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -10700,8 +10700,11 @@ static __latent_entropy void run_rebalance_domains(struct softirq_action *h)
  */
 void trigger_load_balance(struct rq *rq)
 {
-	/* Don't need to rebalance while attached to NULL domain */
-	if (unlikely(on_null_domain(rq)))
+	/*
+	 * Don't need to rebalance while attached to NULL domain or
+	 * runqueue CPU is not active
+	 */
+	if (unlikely(on_null_domain(rq) || !cpu_active(cpu_of(rq))))
 		return;
 
 	if (time_after_eq(jiffies, rq->next_balance))
-- 
2.20.1

Re: [PATCH] sched: Prevent raising SCHED_SOFTIRQ when CPU is !active

[Peter Zijlstra]

On Tue, Dec 15, 2020 at 11:44:00AM +0100, Anna-Maria Behnsen wrote:
> SCHED_SOFTIRQ is raised to trigger periodic load balancing. When CPU is not
> active, CPU should not participate in load balancing.
> 
> The scheduler uses nohz.idle_cpus_mask to keep track of the CPUs which can
> do idle load balancing. When bringing a CPU up the CPU is added to the mask
> when it reaches the active state, but on teardown the CPU stays in the mask
> until it goes offline and invokes sched_cpu_dying().
> 
> When SCHED_SOFTIRQ is raised on a !active CPU, there might be a pending
> softirq when stopping the tick which triggers a warning in NOHZ code. The
> SCHED_SOFTIRQ can also be raised by the scheduler tick which has the same
> issue.
> 
> Therefore remove the CPU from nohz.idle_cpus_mask when it is marked
> inactive and also prevent the scheduler_tick() from raising SCHED_SOFTIRQ
> after this point.
> 
> Signed-off-by: Anna-Maria Behnsen <anna-maria@linutronix.de>
> ---
>  kernel/sched/core.c | 7 ++++++-
>  kernel/sched/fair.c | 7 +++++--
>  2 files changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 21b548b69455..69284dc121d3 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -7492,6 +7492,12 @@ int sched_cpu_deactivate(unsigned int cpu)
>  	struct rq_flags rf;
>  	int ret;
>  
> +	/*
> +	 * Remove CPU from nohz.idle_cpus_mask to prevent participating in
> +	 * load balancing when not active
> +	 */
> +	nohz_balance_exit_idle(rq);
> +
>  	set_cpu_active(cpu, false);
>  	/*
>  	 * We've cleared cpu_active_mask, wait for all preempt-disabled and RCU

OK, so we must clear the state before !active, because getting an
interrupt/softirq after would trigger the badness. And we're guaranteed
nothing blocks between them to re-set it.

That works I suppose.

Thanks!

Re: [PATCH] sched: Prevent raising SCHED_SOFTIRQ when CPU is !active

[Anna-Maria Behnsen]

On Tue, 15 Dec 2020, Peter Zijlstra wrote:

> On Tue, Dec 15, 2020 at 11:44:00AM +0100, Anna-Maria Behnsen wrote:
> > SCHED_SOFTIRQ is raised to trigger periodic load balancing. When CPU is not
> > active, CPU should not participate in load balancing.
> > 
> > The scheduler uses nohz.idle_cpus_mask to keep track of the CPUs which can
> > do idle load balancing. When bringing a CPU up the CPU is added to the mask
> > when it reaches the active state, but on teardown the CPU stays in the mask
> > until it goes offline and invokes sched_cpu_dying().
> > 
> > When SCHED_SOFTIRQ is raised on a !active CPU, there might be a pending
> > softirq when stopping the tick which triggers a warning in NOHZ code. The
> > SCHED_SOFTIRQ can also be raised by the scheduler tick which has the same
> > issue.
> > 
> > Therefore remove the CPU from nohz.idle_cpus_mask when it is marked
> > inactive and also prevent the scheduler_tick() from raising SCHED_SOFTIRQ
> > after this point.
> > 
> > Signed-off-by: Anna-Maria Behnsen <anna-maria@linutronix.de>
> > ---
> >  kernel/sched/core.c | 7 ++++++-
> >  kernel/sched/fair.c | 7 +++++--
> >  2 files changed, 11 insertions(+), 3 deletions(-)
> > 
> > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> > index 21b548b69455..69284dc121d3 100644
> > --- a/kernel/sched/core.c
> > +++ b/kernel/sched/core.c
> > @@ -7492,6 +7492,12 @@ int sched_cpu_deactivate(unsigned int cpu)
> >  	struct rq_flags rf;
> >  	int ret;
> >  
> > +	/*
> > +	 * Remove CPU from nohz.idle_cpus_mask to prevent participating in
> > +	 * load balancing when not active
> > +	 */
> > +	nohz_balance_exit_idle(rq);
> > +
> >  	set_cpu_active(cpu, false);
> >  	/*
> >  	 * We've cleared cpu_active_mask, wait for all preempt-disabled and RCU
> 
> OK, so we must clear the state before !active, because getting an
> interrupt/softirq after would trigger the badness. And we're guaranteed
> nothing blocks between them to re-set it.

As far as I understood, it is not a problem whether the delete is before or
after !active. When it is deleted after, the remote CPU will return in
kick_ilb() because cpu is not idle, because it is running the hotplug
thread.

Thanks,

	Anna-Maria

Re: [PATCH] sched: Prevent raising SCHED_SOFTIRQ when CPU is !active

[Steven Rostedt]

On Tue, 15 Dec 2020 15:23:39 +0100 (CET)
Anna-Maria Behnsen <anna-maria@linutronix.de> wrote:

> > > +	/*
> > > +	 * Remove CPU from nohz.idle_cpus_mask to prevent participating in
> > > +	 * load balancing when not active
> > > +	 */
> > > +	nohz_balance_exit_idle(rq);
> > > +
> > >  	set_cpu_active(cpu, false);
> > >  	/*
> > >  	 * We've cleared cpu_active_mask, wait for all preempt-disabled and RCU  
> > 
> > OK, so we must clear the state before !active, because getting an
> > interrupt/softirq after would trigger the badness. And we're guaranteed
> > nothing blocks between them to re-set it.  
> 
> As far as I understood, it is not a problem whether the delete is before or
> after !active. When it is deleted after, the remote CPU will return in
> kick_ilb() because cpu is not idle, because it is running the hotplug
> thread.

I was thinking that disabling it after may also cause some badness. Even if
it does not, I think there's no harm in clearing it just before setting cpu
active to false. And I find that the safer option.

-- Steve

Re: [PATCH] sched: Prevent raising SCHED_SOFTIRQ when CPU is !active

[Peter Zijlstra]

On Tue, Dec 15, 2020 at 09:34:15AM -0500, Steven Rostedt wrote:
> On Tue, 15 Dec 2020 15:23:39 +0100 (CET)
> Anna-Maria Behnsen <anna-maria@linutronix.de> wrote:
> 
> > > > +	/*
> > > > +	 * Remove CPU from nohz.idle_cpus_mask to prevent participating in
> > > > +	 * load balancing when not active
> > > > +	 */
> > > > +	nohz_balance_exit_idle(rq);
> > > > +
> > > >  	set_cpu_active(cpu, false);
> > > >  	/*
> > > >  	 * We've cleared cpu_active_mask, wait for all preempt-disabled and RCU  
> > > 
> > > OK, so we must clear the state before !active, because getting an
> > > interrupt/softirq after would trigger the badness. And we're guaranteed
> > > nothing blocks between them to re-set it.  
> > 
> > As far as I understood, it is not a problem whether the delete is before or
> > after !active. When it is deleted after, the remote CPU will return in
> > kick_ilb() because cpu is not idle, because it is running the hotplug
> > thread.
> 
> I was thinking that disabling it after may also cause some badness. Even if
> it does not, I think there's no harm in clearing it just before setting cpu
> active to false. And I find that the safer option.

The paranoid in me wanted to write it like:

	preempt_disable();
	nohz_balance_exit_idle(rq);
	set_cpu_active(cpu, false);
	preempt_enable();

(or possibly even local_irq_disable), to guarantee we don't hit idle
between them (which could re-set the nohz idle state we just cleared).

But then I gave up :-)

Re: [PATCH] sched: Prevent raising SCHED_SOFTIRQ when CPU is !active

[Thomas Gleixner]

On Tue, Dec 15 2020 at 16:05, Peter Zijlstra wrote:
> On Tue, Dec 15, 2020 at 09:34:15AM -0500, Steven Rostedt wrote:
>> On Tue, 15 Dec 2020 15:23:39 +0100 (CET)
>> Anna-Maria Behnsen <anna-maria@linutronix.de> wrote:
>> 
>> > > > +	/*
>> > > > +	 * Remove CPU from nohz.idle_cpus_mask to prevent participating in
>> > > > +	 * load balancing when not active
>> > > > +	 */
>> > > > +	nohz_balance_exit_idle(rq);
>> > > > +
>> > > >  	set_cpu_active(cpu, false);
>> > > >  	/*
>> > > >  	 * We've cleared cpu_active_mask, wait for all preempt-disabled and RCU  
>> > > 
>> > > OK, so we must clear the state before !active, because getting an
>> > > interrupt/softirq after would trigger the badness. And we're guaranteed
>> > > nothing blocks between them to re-set it.  
>> > 
>> > As far as I understood, it is not a problem whether the delete is before or
>> > after !active. When it is deleted after, the remote CPU will return in
>> > kick_ilb() because cpu is not idle, because it is running the hotplug
>> > thread.
>> 
>> I was thinking that disabling it after may also cause some badness. Even if
>> it does not, I think there's no harm in clearing it just before setting cpu
>> active to false. And I find that the safer option.
>
> The paranoid in me wanted to write it like:
>
> 	preempt_disable();
> 	nohz_balance_exit_idle(rq);
> 	set_cpu_active(cpu, false);
> 	preempt_enable();
>
> (or possibly even local_irq_disable), to guarantee we don't hit idle
> between them (which could re-set the nohz idle state we just cleared).
>
> But then I gave up :-)

I might be missing something, but how is the CPU which runs the pinned
kernel thread, i.e. the hotplug thread, supposed to go idle between the
two calls?

Really the order is completely irrelevant.

  Remote kick_ilb() checks nohz_mask _AND_ idle_cpu()

  Local nohz_enter() checks cpu_active()

I still might be missing something magic though, mushrooms perhaps. :)

Thanks,

        tglx

Re: [PATCH] sched: Prevent raising SCHED_SOFTIRQ when CPU is !active

[Peter Zijlstra]

On Tue, Dec 15, 2020 at 06:52:49PM +0100, Thomas Gleixner wrote:
> I might be missing something, but how is the CPU which runs the pinned
> kernel thread, i.e. the hotplug thread, supposed to go idle between the
> two calls?

Take a mutex or something other daft. My disabling preemption around it
we basically assert the two functions are non-blocking and none of that
cruft matters.

Re: [PATCH] sched: Prevent raising SCHED_SOFTIRQ when CPU is !active

[Thomas Gleixner]

On Wed, Dec 16 2020 at 09:19, Peter Zijlstra wrote:
> On Tue, Dec 15, 2020 at 06:52:49PM +0100, Thomas Gleixner wrote:
>> I might be missing something, but how is the CPU which runs the pinned
>> kernel thread, i.e. the hotplug thread, supposed to go idle between the
>> two calls?
>
> Take a mutex or something other daft. My disabling preemption around it
> we basically assert the two functions are non-blocking and none of that
> cruft matters.

That'd be really daft, but yes we can do that for paranoia sake.

Thanks,

        tglx

Re: [PATCH] sched: Prevent raising SCHED_SOFTIRQ when CPU is !active

[Steven Rostedt]

On Tue, 15 Dec 2020 11:44:00 +0100
Anna-Maria Behnsen <anna-maria@linutronix.de> wrote:

> SCHED_SOFTIRQ is raised to trigger periodic load balancing. When CPU is not
> active, CPU should not participate in load balancing.
> 
> The scheduler uses nohz.idle_cpus_mask to keep track of the CPUs which can
> do idle load balancing. When bringing a CPU up the CPU is added to the mask
> when it reaches the active state, but on teardown the CPU stays in the mask
> until it goes offline and invokes sched_cpu_dying().
> 
> When SCHED_SOFTIRQ is raised on a !active CPU, there might be a pending
> softirq when stopping the tick which triggers a warning in NOHZ code. The
> SCHED_SOFTIRQ can also be raised by the scheduler tick which has the same
> issue.
> 
> Therefore remove the CPU from nohz.idle_cpus_mask when it is marked
> inactive and also prevent the scheduler_tick() from raising SCHED_SOFTIRQ
> after this point.
> 

Makes sense.

Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

-- Steve

> Signed-off-by: Anna-Maria Behnsen <anna-maria@linutronix.de>
> ---
>  kernel/sched/core.c | 7 ++++++-
>  kernel/sched/fair.c | 7 +++++--
>  2 files changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 21b548b69455..69284dc121d3 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -7492,6 +7492,12 @@ int sched_cpu_deactivate(unsigned int cpu)
>  	struct rq_flags rf;
>  	int ret;
>  
> +	/*
> +	 * Remove CPU from nohz.idle_cpus_mask to prevent participating in
> +	 * load balancing when not active
> +	 */
> +	nohz_balance_exit_idle(rq);
> +
>  	set_cpu_active(cpu, false);
>  	/*
>  	 * We've cleared cpu_active_mask, wait for all preempt-disabled and RCU
> @@ -7598,7 +7604,6 @@ int sched_cpu_dying(unsigned int cpu)
>  
>  	calc_load_migrate(rq);
>  	update_max_interval();
> -	nohz_balance_exit_idle(rq);
>  	hrtick_clear(rq);
>  	return 0;
>  }
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 04a3ce20da67..fd422b8eb859 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -10700,8 +10700,11 @@ static __latent_entropy void run_rebalance_domains(struct softirq_action *h)
>   */
>  void trigger_load_balance(struct rq *rq)
>  {
> -	/* Don't need to rebalance while attached to NULL domain */
> -	if (unlikely(on_null_domain(rq)))
> +	/*
> +	 * Don't need to rebalance while attached to NULL domain or
> +	 * runqueue CPU is not active
> +	 */
> +	if (unlikely(on_null_domain(rq) || !cpu_active(cpu_of(rq))))
>  		return;
>  
>  	if (time_after_eq(jiffies, rq->next_balance))

[tip: sched/core] sched: Prevent raising SCHED_SOFTIRQ when CPU is !active

[tip-bot2 for Anna-Maria Behnsen]

The following commit has been merged into the sched/core branch of tip:

Commit-ID:     e0b257c3b71bd98a4866c3daecf000998aaa4927
Gitweb:        https://git.kernel.org/tip/e0b257c3b71bd98a4866c3daecf000998aaa4927
Author:        Anna-Maria Behnsen <anna-maria@linutronix.de>
AuthorDate:    Tue, 15 Dec 2020 11:44:00 +01:00
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Thu, 14 Jan 2021 11:20:09 +01:00

sched: Prevent raising SCHED_SOFTIRQ when CPU is !active

SCHED_SOFTIRQ is raised to trigger periodic load balancing. When CPU is not
active, CPU should not participate in load balancing.

The scheduler uses nohz.idle_cpus_mask to keep track of the CPUs which can
do idle load balancing. When bringing a CPU up the CPU is added to the mask
when it reaches the active state, but on teardown the CPU stays in the mask
until it goes offline and invokes sched_cpu_dying().

When SCHED_SOFTIRQ is raised on a !active CPU, there might be a pending
softirq when stopping the tick which triggers a warning in NOHZ code. The
SCHED_SOFTIRQ can also be raised by the scheduler tick which has the same
issue.

Therefore remove the CPU from nohz.idle_cpus_mask when it is marked
inactive and also prevent the scheduler_tick() from raising SCHED_SOFTIRQ
after this point.

Signed-off-by: Anna-Maria Behnsen <anna-maria@linutronix.de>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Link: https://lkml.kernel.org/r/20201215104400.9435-1-anna-maria@linutronix.de
---
 kernel/sched/core.c | 7 ++++++-
 kernel/sched/fair.c | 7 +++++--
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 4fe4cbf..06b4499 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -7596,6 +7596,12 @@ int sched_cpu_deactivate(unsigned int cpu)
 	struct rq_flags rf;
 	int ret;
 
+	/*
+	 * Remove CPU from nohz.idle_cpus_mask to prevent participating in
+	 * load balancing when not active
+	 */
+	nohz_balance_exit_idle(rq);
+
 	set_cpu_active(cpu, false);
 	/*
 	 * We've cleared cpu_active_mask, wait for all preempt-disabled and RCU
@@ -7702,7 +7708,6 @@ int sched_cpu_dying(unsigned int cpu)
 
 	calc_load_migrate(rq);
 	update_max_interval();
-	nohz_balance_exit_idle(rq);
 	hrtick_clear(rq);
 	return 0;
 }
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 39c5bda..389cb58 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -10700,8 +10700,11 @@ static __latent_entropy void run_rebalance_domains(struct softirq_action *h)
  */
 void trigger_load_balance(struct rq *rq)
 {
-	/* Don't need to rebalance while attached to NULL domain */
-	if (unlikely(on_null_domain(rq)))
+	/*
+	 * Don't need to rebalance while attached to NULL domain or
+	 * runqueue CPU is not active
+	 */
+	if (unlikely(on_null_domain(rq) || !cpu_active(cpu_of(rq))))
 		return;
 
 	if (time_after_eq(jiffies, rq->next_balance))