* [PATCH] sched, fair: try to prevent migration thread from preempting non-cfs task @ 2021-06-15 12:15 Yafang Shao 2021-06-15 14:55 ` Valentin Schneider 2021-06-15 20:35 ` Peter Zijlstra 0 siblings, 2 replies; 11+ messages in thread From: Yafang Shao @ 2021-06-15 12:15 UTC (permalink / raw) To: vincent.guittot, mingo, peterz, juri.lelli, dietmar.eggemann, rostedt, bsegall, mgorman, bristot Cc: linux-kernel, Yafang Shao, Valentin Schneider 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 <laoar.shao@gmail.com> Cc: Vincent Guittot <vincent.guittot@linaro.org> Cc: Valentin Schneider <valentin.schneider@arm.com> --- - 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] sched, fair: try to prevent migration thread from preempting non-cfs task 2021-06-15 12:15 [PATCH] sched, fair: try to prevent migration thread from preempting non-cfs task Yafang Shao @ 2021-06-15 14:55 ` Valentin Schneider 2021-06-15 15:45 ` Vincent Guittot 2021-06-15 20:35 ` Peter Zijlstra 1 sibling, 1 reply; 11+ messages in thread From: Valentin Schneider @ 2021-06-15 14:55 UTC (permalink / raw) To: Yafang Shao, vincent.guittot, mingo, peterz, juri.lelli, dietmar.eggemann, rostedt, bsegall, mgorman, bristot Cc: linux-kernel, Yafang Shao 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] sched, fair: try to prevent migration thread from preempting non-cfs task 2021-06-15 14:55 ` Valentin Schneider @ 2021-06-15 15:45 ` Vincent Guittot 2021-06-15 16:39 ` Valentin Schneider 0 siblings, 1 reply; 11+ messages in thread From: Vincent Guittot @ 2021-06-15 15:45 UTC (permalink / raw) To: Valentin Schneider Cc: Yafang Shao, Ingo Molnar, Peter Zijlstra, Juri Lelli, Dietmar Eggemann, Steven Rostedt, Ben Segall, Mel Gorman, Daniel Bristot de Oliveira, linux-kernel On Tue, 15 Jun 2021 at 16:55, Valentin Schneider <valentin.schneider@arm.com> 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] sched, fair: try to prevent migration thread from preempting non-cfs task 2021-06-15 15:45 ` Vincent Guittot @ 2021-06-15 16:39 ` Valentin Schneider 0 siblings, 0 replies; 11+ messages in thread From: Valentin Schneider @ 2021-06-15 16:39 UTC (permalink / raw) To: Vincent Guittot Cc: Yafang Shao, Ingo Molnar, Peter Zijlstra, Juri Lelli, Dietmar Eggemann, Steven Rostedt, Ben Segall, Mel Gorman, Daniel Bristot de Oliveira, linux-kernel On 15/06/21 17:45, Vincent Guittot wrote: > On Tue, 15 Jun 2021 at 16:55, Valentin Schneider > <valentin.schneider@arm.com> 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] sched, fair: try to prevent migration thread from preempting non-cfs task 2021-06-15 12:15 [PATCH] sched, fair: try to prevent migration thread from preempting non-cfs task Yafang Shao 2021-06-15 14:55 ` Valentin Schneider @ 2021-06-15 20:35 ` Peter Zijlstra 2021-06-16 1:44 ` Yafang Shao 1 sibling, 1 reply; 11+ messages in thread From: Peter Zijlstra @ 2021-06-15 20:35 UTC (permalink / raw) To: Yafang Shao Cc: vincent.guittot, mingo, juri.lelli, dietmar.eggemann, rostedt, bsegall, mgorman, bristot, linux-kernel, Valentin Schneider, Thomas Gleixner 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? ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] sched, fair: try to prevent migration thread from preempting non-cfs task 2021-06-15 20:35 ` Peter Zijlstra @ 2021-06-16 1:44 ` Yafang Shao 2021-06-16 7:15 ` Peter Zijlstra 0 siblings, 1 reply; 11+ messages in thread From: Yafang Shao @ 2021-06-16 1:44 UTC (permalink / raw) To: Peter Zijlstra Cc: Vincent Guittot, Ingo Molnar, Juri Lelli, Dietmar Eggemann, Steven Rostedt, Benjamin Segall, Mel Gorman, Daniel Bristot de Oliveira, LKML, Valentin Schneider, Thomas Gleixner On Wed, Jun 16, 2021 at 4:35 AM Peter Zijlstra <peterz@infradead.org> 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] sched, fair: try to prevent migration thread from preempting non-cfs task 2021-06-16 1:44 ` Yafang Shao @ 2021-06-16 7:15 ` Peter Zijlstra 2021-06-16 7:29 ` Vincent Guittot 2021-06-16 9:45 ` Yafang Shao 0 siblings, 2 replies; 11+ messages in thread From: Peter Zijlstra @ 2021-06-16 7:15 UTC (permalink / raw) To: Yafang Shao Cc: Vincent Guittot, Ingo Molnar, Juri Lelli, Dietmar Eggemann, Steven Rostedt, Benjamin Segall, Mel Gorman, Daniel Bristot de Oliveira, LKML, Valentin Schneider, Thomas Gleixner On Wed, Jun 16, 2021 at 09:44:46AM +0800, Yafang Shao wrote: > On Wed, Jun 16, 2021 at 4:35 AM Peter Zijlstra <peterz@infradead.org> 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] sched, fair: try to prevent migration thread from preempting non-cfs task 2021-06-16 7:15 ` Peter Zijlstra @ 2021-06-16 7:29 ` Vincent Guittot 2021-06-16 8:29 ` Peter Zijlstra 2021-06-16 9:45 ` Yafang Shao 1 sibling, 1 reply; 11+ messages in thread From: Vincent Guittot @ 2021-06-16 7:29 UTC (permalink / raw) To: Peter Zijlstra Cc: Yafang Shao, Ingo Molnar, Juri Lelli, Dietmar Eggemann, Steven Rostedt, Benjamin Segall, Mel Gorman, Daniel Bristot de Oliveira, LKML, Valentin Schneider, Thomas Gleixner On Wed, 16 Jun 2021 at 09:15, Peter Zijlstra <peterz@infradead.org> 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 <peterz@infradead.org> 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(). ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] sched, fair: try to prevent migration thread from preempting non-cfs task 2021-06-16 7:29 ` Vincent Guittot @ 2021-06-16 8:29 ` Peter Zijlstra 2021-06-16 9:49 ` Yafang Shao 0 siblings, 1 reply; 11+ messages in thread From: Peter Zijlstra @ 2021-06-16 8:29 UTC (permalink / raw) To: Vincent Guittot Cc: Yafang Shao, Ingo Molnar, Juri Lelli, Dietmar Eggemann, Steven Rostedt, Benjamin Segall, Mel Gorman, Daniel Bristot de Oliveira, LKML, Valentin Schneider, Thomas Gleixner On Wed, Jun 16, 2021 at 09:29:55AM +0200, Vincent Guittot wrote: > On Wed, 16 Jun 2021 at 09:15, Peter Zijlstra <peterz@infradead.org> 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). ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] sched, fair: try to prevent migration thread from preempting non-cfs task 2021-06-16 8:29 ` Peter Zijlstra @ 2021-06-16 9:49 ` Yafang Shao 0 siblings, 0 replies; 11+ messages in thread From: Yafang Shao @ 2021-06-16 9:49 UTC (permalink / raw) To: Peter Zijlstra Cc: Vincent Guittot, Ingo Molnar, Juri Lelli, Dietmar Eggemann, Steven Rostedt, Benjamin Segall, Mel Gorman, Daniel Bristot de Oliveira, LKML, Valentin Schneider, Thomas Gleixner On Wed, Jun 16, 2021 at 4:29 PM Peter Zijlstra <peterz@infradead.org> wrote: > > On Wed, Jun 16, 2021 at 09:29:55AM +0200, Vincent Guittot wrote: > > On Wed, 16 Jun 2021 at 09:15, Peter Zijlstra <peterz@infradead.org> 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] sched, fair: try to prevent migration thread from preempting non-cfs task 2021-06-16 7:15 ` Peter Zijlstra 2021-06-16 7:29 ` Vincent Guittot @ 2021-06-16 9:45 ` Yafang Shao 1 sibling, 0 replies; 11+ messages in thread From: Yafang Shao @ 2021-06-16 9:45 UTC (permalink / raw) To: Peter Zijlstra Cc: Vincent Guittot, Ingo Molnar, Juri Lelli, Dietmar Eggemann, Steven Rostedt, Benjamin Segall, Mel Gorman, Daniel Bristot de Oliveira, LKML, Valentin Schneider, Thomas Gleixner On Wed, Jun 16, 2021 at 3:15 PM Peter Zijlstra <peterz@infradead.org> 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 <peterz@infradead.org> 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-06-16 9:49 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-06-15 12:15 [PATCH] sched, fair: try to prevent migration thread from preempting non-cfs task Yafang Shao 2021-06-15 14:55 ` Valentin Schneider 2021-06-15 15:45 ` Vincent Guittot 2021-06-15 16:39 ` Valentin Schneider 2021-06-15 20:35 ` Peter Zijlstra 2021-06-16 1:44 ` Yafang Shao 2021-06-16 7:15 ` Peter Zijlstra 2021-06-16 7:29 ` Vincent Guittot 2021-06-16 8:29 ` Peter Zijlstra 2021-06-16 9:49 ` Yafang Shao 2021-06-16 9:45 ` Yafang Shao
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.