We monitored our latency-sensitive RT tasks are randomly preempted by the
kthread migration/n, which means to migrate task from CPUn to the new
idle CPU which wakes up the migration/n. For example,
sensing_node-2511 [007] d... 945.351566: sched_switch: prev_comm=sensing_node prev_pid=2511 prev_prio=98 prev_state=S ==> next_comm=cat next_pid=2686 next_prio=120
cat-2686 [007] d... 945.351569: sched_switch: prev_comm=cat prev_pid=2686 prev_prio=120 prev_state=R+ ==> next_comm=sensing_node next_pid=2512 next_prio=98
sensing_node-2516 [004] dn.. 945.351571: sched_wakeup: comm=migration/7 pid=47 prio=0 target_cpu=007
sensing_node-2512 [007] d... 945.351572: sched_switch: prev_comm=sensing_node prev_pid=2512 prev_prio=98 prev_state=R ==> next_comm=migration/7 next_pid=47 next_prio=0
sensing_node-2516 [004] d... 945.351572: sched_switch: prev_comm=sensing_node prev_pid=2516 prev_prio=98 prev_state=S ==> next_comm=sensing_node next_pid=2502 next_prio=98
migration/7-47 [007] d... 945.351580: sched_switch: prev_comm=migration/7 prev_pid=47 prev_prio=0 prev_state=S ==> next_comm=sensing_node next_pid=2512 next_prio=98
sensing_node-2502 [004] d... 945.351605: sched_switch: prev_comm=sensing_node prev_pid=2502 prev_prio=98 prev_state=S ==> next_comm=cat next_pid=2686 next_prio=120
When CPU4 is waking migration/7, the CFS thread 'cat' is running on
CPU7, but then 'cat' is preempted by a RT task 'sensing_node', and
then the migration/7 preempts the RT task. The race happens between:
if (need_active_balance(&env)) {
and
raw_spin_rq_lock_irqsave(busiest, flags);
In order to reduce the race, we'd better do the last minute check before
waking up migration thread.
Signed-off-by: Yafang Shao <[email protected]>
Cc: Vincent Guittot <[email protected]>
Cc: Valentin Schneider <[email protected]>
---
- Prev version
https://lore.kernel.org/lkml/CAKfTPtBd349eyDhA5ThCAHFd83cGMQKb_LDxD4QvyP-cJOBjqA@mail.gmail.com/
- Similar discussion
https://lore.kernel.org/lkml/CAKfTPtBygNcVewbb0GQOP5xxO96am3YeTZNP5dK9BxKHJJAL-g@mail.gmail.com/
---
kernel/sched/fair.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 3248e24a90b0..597c7a940746 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -9797,6 +9797,20 @@ static int load_balance(int this_cpu, struct rq *this_rq,
/* Record that we found at least one task that could run on this_cpu */
env.flags &= ~LBF_ALL_PINNED;
+ /*
+ * There may be a race between load balance starting migration
+ * thread to pull the cfs running thread and the RT thread
+ * waking up and preempting cfs task before migration threads
+ * which then preempt the RT thread.
+ * We'd better do the last minute check before starting
+ * migration thread to avoid preempting latency-sensitive thread.
+ */
+ if (busiest->curr->sched_class != &fair_sched_class) {
+ raw_spin_unlock_irqrestore(&busiest->lock,
+ flags);
+ goto out;
+ }
+
/*
* ->active_balance synchronizes accesses to
* ->active_balance_work. Once set, it's cleared
--
2.17.1
Hi,
On 15/06/21 20:15, Yafang Shao wrote:
> - Prev version
> https://lore.kernel.org/lkml/CAKfTPtBd349eyDhA5ThCAHFd83cGMQKb_LDxD4QvyP-cJOBjqA@mail.gmail.com/
>
> - Similar discussion
> https://lore.kernel.org/lkml/CAKfTPtBygNcVewbb0GQOP5xxO96am3YeTZNP5dK9BxKHJJAL-g@mail.gmail.com/
I knew that sounded familiar :-)
> ---
> kernel/sched/fair.c | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 3248e24a90b0..597c7a940746 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -9797,6 +9797,20 @@ static int load_balance(int this_cpu, struct rq *this_rq,
> /* Record that we found at least one task that could run on this_cpu */
> env.flags &= ~LBF_ALL_PINNED;
>
> + /*
> + * There may be a race between load balance starting migration
> + * thread to pull the cfs running thread and the RT thread
> + * waking up and preempting cfs task before migration threads
> + * which then preempt the RT thread.
> + * We'd better do the last minute check before starting
> + * migration thread to avoid preempting latency-sensitive thread.
> + */
This can be summarized as in the below, no?
/*
* Don't cause a higher-than-CFS task to be preempted by
* the CPU stopper.
*/
> + if (busiest->curr->sched_class != &fair_sched_class) {
> + raw_spin_unlock_irqrestore(&busiest->lock,
> + flags);
> + goto out;
Since you goto out this could be moved before the
env.flags &= ~LBF_ALL_PINNED;
above (it only has an impact if you'd goto out_balanced).
> + }
> +
Other than the above, this looks OK to me.
Back then I had argued that having a >CFS task and holding the remote rq
lock could let us invoke detach_one_task() locally (rather than on the
stopper thread), but realistically if we got to this !ld_moved condition
then the chances of us actually pulling something here are very slim (we'd
depend on enqueues happening between ~detach_tasks() and here).
> /*
> * ->active_balance synchronizes accesses to
> * ->active_balance_work. Once set, it's cleared
> --
> 2.17.1
On Tue, 15 Jun 2021 at 16:55, Valentin Schneider
<[email protected]> wrote:
>
>
> Hi,
>
> On 15/06/21 20:15, Yafang Shao wrote:
>
> > - Prev version
> > https://lore.kernel.org/lkml/CAKfTPtBd349eyDhA5ThCAHFd83cGMQKb_LDxD4QvyP-cJOBjqA@mail.gmail.com/
> >
> > - Similar discussion
> > https://lore.kernel.org/lkml/CAKfTPtBygNcVewbb0GQOP5xxO96am3YeTZNP5dK9BxKHJJAL-g@mail.gmail.com/
>
> I knew that sounded familiar :-)
>
> > ---
> > kernel/sched/fair.c | 14 ++++++++++++++
> > 1 file changed, 14 insertions(+)
> >
> > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > index 3248e24a90b0..597c7a940746 100644
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -9797,6 +9797,20 @@ static int load_balance(int this_cpu, struct rq *this_rq,
> > /* Record that we found at least one task that could run on this_cpu */
> > env.flags &= ~LBF_ALL_PINNED;
> >
> > + /*
> > + * There may be a race between load balance starting migration
> > + * thread to pull the cfs running thread and the RT thread
> > + * waking up and preempting cfs task before migration threads
> > + * which then preempt the RT thread.
> > + * We'd better do the last minute check before starting
> > + * migration thread to avoid preempting latency-sensitive thread.
> > + */
>
> This can be summarized as in the below, no?
>
> /*
> * Don't cause a higher-than-CFS task to be preempted by
> * the CPU stopper.
> */
IMO, it's worth keeping the explanation that we are there because:
- a CFS task that was running during the 1st step : if
(busiest->nr_running > 1) { ...
so we didn't pull the task
- but it has been preempted while lb was deciding if it needs an
active load balance
so maybe something like:
/*
* Don't kick the active_load_balance_cpu_stop,
* if the CFS task has been preempted by higher
* priority task in the meantime.
*/
>
> > + if (busiest->curr->sched_class != &fair_sched_class) {
> > + raw_spin_unlock_irqrestore(&busiest->lock,
> > + flags);
> > + goto out;
>
> Since you goto out this could be moved before the
>
> env.flags &= ~LBF_ALL_PINNED;
>
> above (it only has an impact if you'd goto out_balanced).
Good point. My comment to move this test after env.flags &=
~LBF_ALL_PINNED; was valid only with goto out_one_pinned
>
> > + }
> > +
>
> Other than the above, this looks OK to me.
>
> Back then I had argued that having a >CFS task and holding the remote rq
> lock could let us invoke detach_one_task() locally (rather than on the
> stopper thread), but realistically if we got to this !ld_moved condition
> then the chances of us actually pulling something here are very slim (we'd
> depend on enqueues happening between ~detach_tasks() and here).
>
> > /*
> > * ->active_balance synchronizes accesses to
> > * ->active_balance_work. Once set, it's cleared
> > --
> > 2.17.1
On 15/06/21 17:45, Vincent Guittot wrote:
> On Tue, 15 Jun 2021 at 16:55, Valentin Schneider
> <[email protected]> wrote:
>> This can be summarized as in the below, no?
>>
>> /*
>> * Don't cause a higher-than-CFS task to be preempted by
>> * the CPU stopper.
>> */
>
> IMO, it's worth keeping the explanation that we are there because:
> - a CFS task that was running during the 1st step : if
> (busiest->nr_running > 1) { ...
> so we didn't pull the task
> - but it has been preempted while lb was deciding if it needs an
> active load balance
>
> so maybe something like:
> /*
> * Don't kick the active_load_balance_cpu_stop,
> * if the CFS task has been preempted by higher
> * priority task in the meantime.
> */
>
Ack
On Tue, Jun 15, 2021 at 08:15:51PM +0800, Yafang Shao wrote:
> ---
> kernel/sched/fair.c | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 3248e24a90b0..597c7a940746 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -9797,6 +9797,20 @@ static int load_balance(int this_cpu, struct rq *this_rq,
> /* Record that we found at least one task that could run on this_cpu */
> env.flags &= ~LBF_ALL_PINNED;
>
> + /*
> + * There may be a race between load balance starting migration
> + * thread to pull the cfs running thread and the RT thread
> + * waking up and preempting cfs task before migration threads
> + * which then preempt the RT thread.
> + * We'd better do the last minute check before starting
> + * migration thread to avoid preempting latency-sensitive thread.
> + */
> + if (busiest->curr->sched_class != &fair_sched_class) {
> + raw_spin_unlock_irqrestore(&busiest->lock,
> + flags);
This won't apply.
Also, there's still a race window, you've just shrunk it, not fixed it.
Busiest can reschedule between the mandatory rq unlock and doing the
stopper wakeup.
An actual fix might be to have the active migration done by a FIFO-1
task, instead of stopper. The obvious down-side is that that would mean
spawning yet another per-cpu kthread.
How much do we care?
On Wed, Jun 16, 2021 at 4:35 AM Peter Zijlstra <[email protected]> wrote:
>
> On Tue, Jun 15, 2021 at 08:15:51PM +0800, Yafang Shao wrote:
> > ---
> > kernel/sched/fair.c | 14 ++++++++++++++
> > 1 file changed, 14 insertions(+)
> >
> > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > index 3248e24a90b0..597c7a940746 100644
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -9797,6 +9797,20 @@ static int load_balance(int this_cpu, struct rq *this_rq,
> > /* Record that we found at least one task that could run on this_cpu */
> > env.flags &= ~LBF_ALL_PINNED;
> >
> > + /*
> > + * There may be a race between load balance starting migration
> > + * thread to pull the cfs running thread and the RT thread
> > + * waking up and preempting cfs task before migration threads
> > + * which then preempt the RT thread.
> > + * We'd better do the last minute check before starting
> > + * migration thread to avoid preempting latency-sensitive thread.
> > + */
> > + if (busiest->curr->sched_class != &fair_sched_class) {
> > + raw_spin_unlock_irqrestore(&busiest->lock,
> > + flags);
>
> This won't apply.
>
> Also, there's still a race window, you've just shrunk it, not fixed it.
> Busiest can reschedule between the mandatory rq unlock and doing the
> stopper wakeup.
>
> An actual fix might be to have the active migration done by a FIFO-1
> task, instead of stopper. The obvious down-side is that that would mean
> spawning yet another per-cpu kthread.
>
The stopper and the migration thread are different threads in the earlier days.
commit 969c79215a35 ("sched: replace migration_thread with cpu_stop")
merged them into one thread.
Regarding the priority of stopper (with highest priority) and
migration (higher than CFS, but lower than RT) , keeping them in one
single thread seems not a good way.
> How much do we care?
--
Thanks
Yafang
On Wed, Jun 16, 2021 at 09:44:46AM +0800, Yafang Shao wrote:
> On Wed, Jun 16, 2021 at 4:35 AM Peter Zijlstra <[email protected]> wrote:
> >
> > On Tue, Jun 15, 2021 at 08:15:51PM +0800, Yafang Shao wrote:
> > > ---
> > > kernel/sched/fair.c | 14 ++++++++++++++
> > > 1 file changed, 14 insertions(+)
> > >
> > > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > > index 3248e24a90b0..597c7a940746 100644
> > > --- a/kernel/sched/fair.c
> > > +++ b/kernel/sched/fair.c
> > > @@ -9797,6 +9797,20 @@ static int load_balance(int this_cpu, struct rq *this_rq,
> > > /* Record that we found at least one task that could run on this_cpu */
> > > env.flags &= ~LBF_ALL_PINNED;
> > >
> > > + /*
> > > + * There may be a race between load balance starting migration
> > > + * thread to pull the cfs running thread and the RT thread
> > > + * waking up and preempting cfs task before migration threads
> > > + * which then preempt the RT thread.
> > > + * We'd better do the last minute check before starting
> > > + * migration thread to avoid preempting latency-sensitive thread.
> > > + */
> > > + if (busiest->curr->sched_class != &fair_sched_class) {
> > > + raw_spin_unlock_irqrestore(&busiest->lock,
> > > + flags);
> >
> > This won't apply.
> >
> > Also, there's still a race window, you've just shrunk it, not fixed it.
> > Busiest can reschedule between the mandatory rq unlock and doing the
> > stopper wakeup.
> >
> > An actual fix might be to have the active migration done by a FIFO-1
> > task, instead of stopper. The obvious down-side is that that would mean
> > spawning yet another per-cpu kthread.
> >
>
> The stopper and the migration thread are different threads in the earlier days.
> commit 969c79215a35 ("sched: replace migration_thread with cpu_stop")
> merged them into one thread.
Yes, I know, I was there. But that's not what I'm saying, we need the
migration thread to be super high perio for other cases. That change
still makes sense.
> Regarding the priority of stopper (with highest priority) and
> migration (higher than CFS, but lower than RT) , keeping them in one
> single thread seems not a good way.
I never suggested as such.
Only the active migration of CFS can be done by a FIFO-1 task (the
lowest prio that is higher than CFS) and possible the numa balancing
thing.
Other migrations will still need to use stopper, and as such you'll keep
having interference from stopper.
The suggestion was adding a cfs_migration thread, specifically for
active balance (and maybe numa). Just not sure the cost of carrying yet
another per-cpu kernel thread is worth the benefit.
On Wed, 16 Jun 2021 at 09:15, Peter Zijlstra <[email protected]> wrote:
>
> On Wed, Jun 16, 2021 at 09:44:46AM +0800, Yafang Shao wrote:
> > On Wed, Jun 16, 2021 at 4:35 AM Peter Zijlstra <[email protected]> wrote:
> > >
> > > On Tue, Jun 15, 2021 at 08:15:51PM +0800, Yafang Shao wrote:
> > > > ---
> > > > kernel/sched/fair.c | 14 ++++++++++++++
> > > > 1 file changed, 14 insertions(+)
> > > >
> > > > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > > > index 3248e24a90b0..597c7a940746 100644
> > > > --- a/kernel/sched/fair.c
> > > > +++ b/kernel/sched/fair.c
> > > > @@ -9797,6 +9797,20 @@ static int load_balance(int this_cpu, struct rq *this_rq,
> > > > /* Record that we found at least one task that could run on this_cpu */
> > > > env.flags &= ~LBF_ALL_PINNED;
> > > >
> > > > + /*
> > > > + * There may be a race between load balance starting migration
> > > > + * thread to pull the cfs running thread and the RT thread
> > > > + * waking up and preempting cfs task before migration threads
> > > > + * which then preempt the RT thread.
> > > > + * We'd better do the last minute check before starting
> > > > + * migration thread to avoid preempting latency-sensitive thread.
> > > > + */
> > > > + if (busiest->curr->sched_class != &fair_sched_class) {
> > > > + raw_spin_unlock_irqrestore(&busiest->lock,
> > > > + flags);
> > >
> > > This won't apply.
> > >
> > > Also, there's still a race window, you've just shrunk it, not fixed it.
> > > Busiest can reschedule between the mandatory rq unlock and doing the
> > > stopper wakeup.
> > >
> > > An actual fix might be to have the active migration done by a FIFO-1
> > > task, instead of stopper. The obvious down-side is that that would mean
> > > spawning yet another per-cpu kthread.
> > >
> >
> > The stopper and the migration thread are different threads in the earlier days.
> > commit 969c79215a35 ("sched: replace migration_thread with cpu_stop")
> > merged them into one thread.
>
> Yes, I know, I was there. But that's not what I'm saying, we need the
> migration thread to be super high perio for other cases. That change
> still makes sense.
>
> > Regarding the priority of stopper (with highest priority) and
> > migration (higher than CFS, but lower than RT) , keeping them in one
> > single thread seems not a good way.
>
> I never suggested as such.
>
> Only the active migration of CFS can be done by a FIFO-1 task (the
> lowest prio that is higher than CFS) and possible the numa balancing
> thing.
>
> Other migrations will still need to use stopper, and as such you'll keep
> having interference from stopper.
>
> The suggestion was adding a cfs_migration thread, specifically for
> active balance (and maybe numa). Just not sure the cost of carrying yet
> another per-cpu kernel thread is worth the benefit.
Also, this will not completely remove the problem but only further
reduce the race window because the rq is locked and the irq disable in
active_load_balance_cpu_stop().
On Wed, Jun 16, 2021 at 09:29:55AM +0200, Vincent Guittot wrote:
> On Wed, 16 Jun 2021 at 09:15, Peter Zijlstra <[email protected]> wrote:
> > The suggestion was adding a cfs_migration thread, specifically for
> > active balance (and maybe numa). Just not sure the cost of carrying yet
> > another per-cpu kernel thread is worth the benefit.
>
> Also, this will not completely remove the problem but only further
> reduce the race window because the rq is locked and the irq disable in
> active_load_balance_cpu_stop().
It removes the problem of active migration interfering with this
worklaod, because the FIFO1 task will never run until that is done
(assuming he manages to not have his workload at FIFO1).
On Wed, Jun 16, 2021 at 3:15 PM Peter Zijlstra <[email protected]> wrote:
>
> On Wed, Jun 16, 2021 at 09:44:46AM +0800, Yafang Shao wrote:
> > On Wed, Jun 16, 2021 at 4:35 AM Peter Zijlstra <[email protected]> wrote:
> > >
> > > On Tue, Jun 15, 2021 at 08:15:51PM +0800, Yafang Shao wrote:
> > > > ---
> > > > kernel/sched/fair.c | 14 ++++++++++++++
> > > > 1 file changed, 14 insertions(+)
> > > >
> > > > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > > > index 3248e24a90b0..597c7a940746 100644
> > > > --- a/kernel/sched/fair.c
> > > > +++ b/kernel/sched/fair.c
> > > > @@ -9797,6 +9797,20 @@ static int load_balance(int this_cpu, struct rq *this_rq,
> > > > /* Record that we found at least one task that could run on this_cpu */
> > > > env.flags &= ~LBF_ALL_PINNED;
> > > >
> > > > + /*
> > > > + * There may be a race between load balance starting migration
> > > > + * thread to pull the cfs running thread and the RT thread
> > > > + * waking up and preempting cfs task before migration threads
> > > > + * which then preempt the RT thread.
> > > > + * We'd better do the last minute check before starting
> > > > + * migration thread to avoid preempting latency-sensitive thread.
> > > > + */
> > > > + if (busiest->curr->sched_class != &fair_sched_class) {
> > > > + raw_spin_unlock_irqrestore(&busiest->lock,
> > > > + flags);
> > >
> > > This won't apply.
> > >
> > > Also, there's still a race window, you've just shrunk it, not fixed it.
> > > Busiest can reschedule between the mandatory rq unlock and doing the
> > > stopper wakeup.
> > >
> > > An actual fix might be to have the active migration done by a FIFO-1
> > > task, instead of stopper. The obvious down-side is that that would mean
> > > spawning yet another per-cpu kthread.
> > >
> >
> > The stopper and the migration thread are different threads in the earlier days.
> > commit 969c79215a35 ("sched: replace migration_thread with cpu_stop")
> > merged them into one thread.
>
> Yes, I know, I was there. But that's not what I'm saying, we need the
> migration thread to be super high perio for other cases. That change
> still makes sense.
>
> > Regarding the priority of stopper (with highest priority) and
> > migration (higher than CFS, but lower than RT) , keeping them in one
> > single thread seems not a good way.
>
> I never suggested as such.
>
> Only the active migration of CFS can be done by a FIFO-1 task (the
> lowest prio that is higher than CFS) and possible the numa balancing
> thing.
>
> Other migrations will still need to use stopper, and as such you'll keep
> having interference from stopper.
>
> The suggestion was adding a cfs_migration thread, specifically for
> active balance (and maybe numa). Just not sure the cost of carrying yet
> another per-cpu kernel thread is worth the benefit.
Thanks for the clarification.
--
Thanks
Yafang
On Wed, Jun 16, 2021 at 4:29 PM Peter Zijlstra <[email protected]> wrote:
>
> On Wed, Jun 16, 2021 at 09:29:55AM +0200, Vincent Guittot wrote:
> > On Wed, 16 Jun 2021 at 09:15, Peter Zijlstra <[email protected]> wrote:
>
> > > The suggestion was adding a cfs_migration thread, specifically for
> > > active balance (and maybe numa). Just not sure the cost of carrying yet
> > > another per-cpu kernel thread is worth the benefit.
> >
> > Also, this will not completely remove the problem but only further
> > reduce the race window because the rq is locked and the irq disable in
> > active_load_balance_cpu_stop().
>
> It removes the problem of active migration interfering with this
> worklaod, because the FIFO1 task will never run until that is done
> (assuming he manages to not have his workload at FIFO1).
>
Right, the workload should have a higher priority than FIFO1 then.
I'm wondering why not just setting some flags to the running CFS and
then when the CFS task scheds out the CPU we migrate it to the new
idle CPU in active LB. Then we don't need to preempt any task.
--
Thanks
Yafang