* [PATCH] sched/cfs: fix spurious active migration
@ 2019-11-29 14:04 Vincent Guittot
2019-12-02 13:22 ` Peter Zijlstra
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Vincent Guittot @ 2019-11-29 14:04 UTC (permalink / raw)
To: mingo, peterz, juri.lelli, dietmar.eggemann, rostedt, bsegall,
mgorman, linux-kernel
Cc: Vincent Guittot
The load balance can fail to find a suitable task during the periodic check
because the imbalance is smaller than half of the load of the waiting
tasks. This results in the increase of the number of failed load balance,
which can end up to start an active migration. This active migration is
useless because the current running task is not a better choice than the
waiting ones. In fact, the current task was probably not running but
waiting for the CPU during one of the previous attempts and it had already
not been selected.
When load balance fails too many times to migrate a task, we should relax
the contraint on the maximum load of the tasks that can be migrated
similarly to what is done with cache hotness.
Before the rework, load balance used to set the imbalance to the average
load_per_task in order to mitigate such situation. This increased the
likelihood of migrating a task but also of selecting a larger task than
needed while more appropriate ones were in the list.
Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
---
I haven't seen any noticable performance changes on the benchmarks that I
usually run but the problem can be easily highlight with a simple test
with 9 always running tasks on 8 cores.
kernel/sched/fair.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index e0d662a..d1b4fa7 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -7433,7 +7433,14 @@ static int detach_tasks(struct lb_env *env)
load < 16 && !env->sd->nr_balance_failed)
goto next;
- if (load/2 > env->imbalance)
+ /*
+ * Make sure that we don't migrate too much load.
+ * Nevertheless, let relax the constraint if
+ * scheduler fails to find a good waiting task to
+ * migrate.
+ */
+ if (load/2 > env->imbalance &&
+ env->sd->nr_balance_failed <= env->sd->cache_nice_tries)
goto next;
env->imbalance -= load;
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] sched/cfs: fix spurious active migration
2019-11-29 14:04 [PATCH] sched/cfs: fix spurious active migration Vincent Guittot
@ 2019-12-02 13:22 ` Peter Zijlstra
2019-12-02 13:51 ` Vincent Guittot
2019-12-02 15:12 ` Dietmar Eggemann
2019-12-17 12:39 ` [tip: sched/urgent] " tip-bot2 for Vincent Guittot
2 siblings, 1 reply; 6+ messages in thread
From: Peter Zijlstra @ 2019-12-02 13:22 UTC (permalink / raw)
To: Vincent Guittot
Cc: mingo, juri.lelli, dietmar.eggemann, rostedt, bsegall, mgorman,
linux-kernel
On Fri, Nov 29, 2019 at 03:04:47PM +0100, Vincent Guittot wrote:
> The load balance can fail to find a suitable task during the periodic check
> because the imbalance is smaller than half of the load of the waiting
> tasks. This results in the increase of the number of failed load balance,
> which can end up to start an active migration. This active migration is
> useless because the current running task is not a better choice than the
> waiting ones. In fact, the current task was probably not running but
> waiting for the CPU during one of the previous attempts and it had already
> not been selected.
>
> When load balance fails too many times to migrate a task, we should relax
> the contraint on the maximum load of the tasks that can be migrated
> similarly to what is done with cache hotness.
>
> Before the rework, load balance used to set the imbalance to the average
> load_per_task in order to mitigate such situation. This increased the
> likelihood of migrating a task but also of selecting a larger task than
> needed while more appropriate ones were in the list.
>
> Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
> ---
>
> I haven't seen any noticable performance changes on the benchmarks that I
> usually run but the problem can be easily highlight with a simple test
> with 9 always running tasks on 8 cores.
>
> kernel/sched/fair.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index e0d662a..d1b4fa7 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -7433,7 +7433,14 @@ static int detach_tasks(struct lb_env *env)
> load < 16 && !env->sd->nr_balance_failed)
> goto next;
>
> - if (load/2 > env->imbalance)
> + /*
> + * Make sure that we don't migrate too much load.
> + * Nevertheless, let relax the constraint if
> + * scheduler fails to find a good waiting task to
> + * migrate.
> + */
> + if (load/2 > env->imbalance &&
> + env->sd->nr_balance_failed <= env->sd->cache_nice_tries)
> goto next;
>
> env->imbalance -= load;
The alternative is carrying a flag that inhibits incrementing
nr_balance_failed.
Not migrating anything when doing so would make the imbalance worse is
not a failure after all.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] sched/cfs: fix spurious active migration
2019-12-02 13:22 ` Peter Zijlstra
@ 2019-12-02 13:51 ` Vincent Guittot
0 siblings, 0 replies; 6+ messages in thread
From: Vincent Guittot @ 2019-12-02 13:51 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Ingo Molnar, Juri Lelli, Dietmar Eggemann, Steven Rostedt,
Ben Segall, Mel Gorman, linux-kernel
On Mon, 2 Dec 2019 at 14:22, Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Fri, Nov 29, 2019 at 03:04:47PM +0100, Vincent Guittot wrote:
> > The load balance can fail to find a suitable task during the periodic check
> > because the imbalance is smaller than half of the load of the waiting
> > tasks. This results in the increase of the number of failed load balance,
> > which can end up to start an active migration. This active migration is
> > useless because the current running task is not a better choice than the
> > waiting ones. In fact, the current task was probably not running but
> > waiting for the CPU during one of the previous attempts and it had already
> > not been selected.
> >
> > When load balance fails too many times to migrate a task, we should relax
> > the contraint on the maximum load of the tasks that can be migrated
> > similarly to what is done with cache hotness.
> >
> > Before the rework, load balance used to set the imbalance to the average
> > load_per_task in order to mitigate such situation. This increased the
> > likelihood of migrating a task but also of selecting a larger task than
> > needed while more appropriate ones were in the list.
> >
> > Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
> > ---
> >
> > I haven't seen any noticable performance changes on the benchmarks that I
> > usually run but the problem can be easily highlight with a simple test
> > with 9 always running tasks on 8 cores.
> >
> > kernel/sched/fair.c | 9 ++++++++-
> > 1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > index e0d662a..d1b4fa7 100644
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -7433,7 +7433,14 @@ static int detach_tasks(struct lb_env *env)
> > load < 16 && !env->sd->nr_balance_failed)
> > goto next;
> >
> > - if (load/2 > env->imbalance)
> > + /*
> > + * Make sure that we don't migrate too much load.
> > + * Nevertheless, let relax the constraint if
> > + * scheduler fails to find a good waiting task to
> > + * migrate.
> > + */
> > + if (load/2 > env->imbalance &&
> > + env->sd->nr_balance_failed <= env->sd->cache_nice_tries)
> > goto next;
> >
> > env->imbalance -= load;
>
> The alternative is carrying a flag that inhibits incrementing
> nr_balance_failed.
>
> Not migrating anything when doing so would make the imbalance worse is
> not a failure after all.
Yeah I thought about this possibility but this behavior will make a
big difference compared to legacy load balance and i'm not sure about
the impact on performance because we can generate significant
unfairness with 2 tasks sharing a CPU while others have a full CPU in
the example that I mentioned above.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] sched/cfs: fix spurious active migration
2019-11-29 14:04 [PATCH] sched/cfs: fix spurious active migration Vincent Guittot
2019-12-02 13:22 ` Peter Zijlstra
@ 2019-12-02 15:12 ` Dietmar Eggemann
2019-12-02 15:36 ` Vincent Guittot
2019-12-17 12:39 ` [tip: sched/urgent] " tip-bot2 for Vincent Guittot
2 siblings, 1 reply; 6+ messages in thread
From: Dietmar Eggemann @ 2019-12-02 15:12 UTC (permalink / raw)
To: Vincent Guittot, mingo, peterz, juri.lelli, rostedt, bsegall,
mgorman, linux-kernel
On 29/11/2019 15:04, Vincent Guittot wrote:
> The load balance can fail to find a suitable task during the periodic check
> because the imbalance is smaller than half of the load of the waiting
> tasks. This results in the increase of the number of failed load balance,
> which can end up to start an active migration. This active migration is
> useless because the current running task is not a better choice than the
> waiting ones. In fact, the current task was probably not running but
> waiting for the CPU during one of the previous attempts and it had already
> not been selected.
>
> When load balance fails too many times to migrate a task, we should relax
> the contraint on the maximum load of the tasks that can be migrated
> similarly to what is done with cache hotness.
>
> Before the rework, load balance used to set the imbalance to the average
> load_per_task in order to mitigate such situation. This increased the
> likelihood of migrating a task but also of selecting a larger task than
> needed while more appropriate ones were in the list.
Why not use '&& !env->sd->nr_balance_failed' then? Too aggressive? But
the average load_per_task was calculated at each load balance attempt,
so it would have led to a migration at the first load balance.
This would be in sync with the LB_MIN check in the same switch case
(migrate_load). Although LB_MIN is false by default.
> Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
> ---
>
> I haven't seen any noticable performance changes on the benchmarks that I
> usually run but the problem can be easily highlight with a simple test
> with 9 always running tasks on 8 cores.
>
> kernel/sched/fair.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index e0d662a..d1b4fa7 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -7433,7 +7433,14 @@ static int detach_tasks(struct lb_env *env)
> load < 16 && !env->sd->nr_balance_failed)
> goto next;
>
> - if (load/2 > env->imbalance)
> + /*
> + * Make sure that we don't migrate too much load.
> + * Nevertheless, let relax the constraint if
> + * scheduler fails to find a good waiting task to
> + * migrate.
> + */
> + if (load/2 > env->imbalance &&
> + env->sd->nr_balance_failed <= env->sd->cache_nice_tries)
> goto next;
>
> env->imbalance -= load;
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] sched/cfs: fix spurious active migration
2019-12-02 15:12 ` Dietmar Eggemann
@ 2019-12-02 15:36 ` Vincent Guittot
0 siblings, 0 replies; 6+ messages in thread
From: Vincent Guittot @ 2019-12-02 15:36 UTC (permalink / raw)
To: Dietmar Eggemann
Cc: Ingo Molnar, Peter Zijlstra, Juri Lelli, Steven Rostedt,
Ben Segall, Mel Gorman, linux-kernel
On Mon, 2 Dec 2019 at 16:13, Dietmar Eggemann <dietmar.eggemann@arm.com> wrote:
>
> On 29/11/2019 15:04, Vincent Guittot wrote:
> > The load balance can fail to find a suitable task during the periodic check
> > because the imbalance is smaller than half of the load of the waiting
> > tasks. This results in the increase of the number of failed load balance,
> > which can end up to start an active migration. This active migration is
> > useless because the current running task is not a better choice than the
> > waiting ones. In fact, the current task was probably not running but
> > waiting for the CPU during one of the previous attempts and it had already
> > not been selected.
> >
> > When load balance fails too many times to migrate a task, we should relax
> > the contraint on the maximum load of the tasks that can be migrated
> > similarly to what is done with cache hotness.
> >
> > Before the rework, load balance used to set the imbalance to the average
> > load_per_task in order to mitigate such situation. This increased the
> > likelihood of migrating a task but also of selecting a larger task than
> > needed while more appropriate ones were in the list.
>
> Why not use '&& !env->sd->nr_balance_failed' then? Too aggressive? But
Yeah I have thought and tried !env->sd->nr_balance_failed. In fact, I
have tried both and I haven't seen any visible difference in my tests.
Nevertheless, using !env->sd->nr_balance_failed condition doesn't let
a chance to the current running task with a possibly correct load to
become a waiting task during the next load balance and to be selected
instead of a larger task, which will create another imbalance. Also
cache_nice_tries increase the number of trials to large domain span to
select different rqs before migrating a large task. Then,
env->sd->nr_balance_failed <= env->sd->cache_nice_tries is used in
can_migrate() for preventing migration because of cache_hotness so we
are aligned with this condition. At the opposite,
!env->sd->nr_balance_failed is used with LB_MIN which is disable by
default.
TBH, I don't have a strong opinion between !env->sd->nr_balance_failed
and env->sd->nr_balance_failed <= env->sd->cache_nice_tries.
> the average load_per_task was calculated at each load balance attempt,
> so it would have led to a migration at the first load balance.
>
> This would be in sync with the LB_MIN check in the same switch case
> (migrate_load). Although LB_MIN is false by default.
>
> > Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
> > ---
> >
> > I haven't seen any noticable performance changes on the benchmarks that I
> > usually run but the problem can be easily highlight with a simple test
> > with 9 always running tasks on 8 cores.
> >
> > kernel/sched/fair.c | 9 ++++++++-
> > 1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > index e0d662a..d1b4fa7 100644
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -7433,7 +7433,14 @@ static int detach_tasks(struct lb_env *env)
> > load < 16 && !env->sd->nr_balance_failed)
> > goto next;
> >
> > - if (load/2 > env->imbalance)
> > + /*
> > + * Make sure that we don't migrate too much load.
> > + * Nevertheless, let relax the constraint if
> > + * scheduler fails to find a good waiting task to
> > + * migrate.
> > + */
> > + if (load/2 > env->imbalance &&
> > + env->sd->nr_balance_failed <= env->sd->cache_nice_tries)
> > goto next;
> >
> > env->imbalance -= load;
> >
^ permalink raw reply [flat|nested] 6+ messages in thread
* [tip: sched/urgent] sched/cfs: fix spurious active migration
2019-11-29 14:04 [PATCH] sched/cfs: fix spurious active migration Vincent Guittot
2019-12-02 13:22 ` Peter Zijlstra
2019-12-02 15:12 ` Dietmar Eggemann
@ 2019-12-17 12:39 ` tip-bot2 for Vincent Guittot
2 siblings, 0 replies; 6+ messages in thread
From: tip-bot2 for Vincent Guittot @ 2019-12-17 12:39 UTC (permalink / raw)
To: linux-tip-commits; +Cc: Vincent Guittot, Peter Zijlstra (Intel), x86, LKML
The following commit has been merged into the sched/urgent branch of tip:
Commit-ID: 6cf82d559e1a1d89f06ff4d428aca479c1dd0be6
Gitweb: https://git.kernel.org/tip/6cf82d559e1a1d89f06ff4d428aca479c1dd0be6
Author: Vincent Guittot <vincent.guittot@linaro.org>
AuthorDate: Fri, 29 Nov 2019 15:04:47 +01:00
Committer: Peter Zijlstra <peterz@infradead.org>
CommitterDate: Tue, 17 Dec 2019 13:32:48 +01:00
sched/cfs: fix spurious active migration
The load balance can fail to find a suitable task during the periodic check
because the imbalance is smaller than half of the load of the waiting
tasks. This results in the increase of the number of failed load balance,
which can end up to start an active migration. This active migration is
useless because the current running task is not a better choice than the
waiting ones. In fact, the current task was probably not running but
waiting for the CPU during one of the previous attempts and it had already
not been selected.
When load balance fails too many times to migrate a task, we should relax
the contraint on the maximum load of the tasks that can be migrated
similarly to what is done with cache hotness.
Before the rework, load balance used to set the imbalance to the average
load_per_task in order to mitigate such situation. This increased the
likelihood of migrating a task but also of selecting a larger task than
needed while more appropriate ones were in the list.
Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lkml.kernel.org/r/1575036287-6052-1-git-send-email-vincent.guittot@linaro.org
---
kernel/sched/fair.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 146b6c8..ba749f5 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -7328,7 +7328,14 @@ static int detach_tasks(struct lb_env *env)
load < 16 && !env->sd->nr_balance_failed)
goto next;
- if (load/2 > env->imbalance)
+ /*
+ * Make sure that we don't migrate too much load.
+ * Nevertheless, let relax the constraint if
+ * scheduler fails to find a good waiting task to
+ * migrate.
+ */
+ if (load/2 > env->imbalance &&
+ env->sd->nr_balance_failed <= env->sd->cache_nice_tries)
goto next;
env->imbalance -= load;
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-12-17 12:40 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-29 14:04 [PATCH] sched/cfs: fix spurious active migration Vincent Guittot
2019-12-02 13:22 ` Peter Zijlstra
2019-12-02 13:51 ` Vincent Guittot
2019-12-02 15:12 ` Dietmar Eggemann
2019-12-02 15:36 ` Vincent Guittot
2019-12-17 12:39 ` [tip: sched/urgent] " tip-bot2 for Vincent Guittot
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.