All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] sched/fair: fix imbalance due to CPU affinity
@ 2019-07-01 15:47 Vincent Guittot
  2019-07-01 19:03 ` Peter Zijlstra
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Vincent Guittot @ 2019-07-01 15:47 UTC (permalink / raw)
  To: linux-kernel, mingo, peterz; +Cc: Vincent Guittot

The load_balance() has a dedicated mecanism to detect when an imbalance
is due to CPU affinity and must be handled at parent level. In this case,
the imbalance field of the parent's sched_group is set.

The description of sg_imbalanced() gives a typical example of two groups
of 4 CPUs each and 4 tasks each with a cpumask covering 1 CPU of the first
group and 3 CPUs of the second group. Something like:

	{ 0 1 2 3 } { 4 5 6 7 }
	        *     * * *

But the load_balance fails to fix this UC on my octo cores system
made of 2 clusters of quad cores.

Whereas the load_balance is able to detect that the imbalanced is due to
CPU affinity, it fails to fix it because the imbalance field is cleared
before letting parent level a chance to run. In fact, when the imbalance is
detected, the load_balance reruns without the CPU with pinned tasks. But
there is no other running tasks in the situation described above and
everything looks balanced this time so the imbalance field is immediately
cleared.

The imbalance field should not be cleared if there is no other task to move
when the imbalance is detected.

Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
---

Sorry, I sent the patch before rebasing it on top of sched-tip and it might
conlfict when applying because it was on top of my ongoing rework of load_balance

This version is rebased on top of latest shced-tip/sched/core

 kernel/sched/fair.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index b798fe7..fff5632 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -8992,9 +8992,10 @@ static int load_balance(int this_cpu, struct rq *this_rq,
 out_balanced:
 	/*
 	 * We reach balance although we may have faced some affinity
-	 * constraints. Clear the imbalance flag if it was set.
+	 * constraints. Clear the imbalance flag only if other tasks got
+	 * a chance to move and fix the imbalance.
 	 */
-	if (sd_parent) {
+	if (sd_parent && !(env.flags & LBF_ALL_PINNED)) {
 		int *group_imbalance = &sd_parent->groups->sgc->imbalance;
 
 		if (*group_imbalance)
-- 
2.7.4


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] sched/fair: fix imbalance due to CPU affinity
  2019-07-01 15:47 [PATCH v2] sched/fair: fix imbalance due to CPU affinity Vincent Guittot
@ 2019-07-01 19:03 ` Peter Zijlstra
  2019-07-02  9:34 ` Valentin Schneider
  2019-07-25 16:13 ` [tip:sched/core] sched/fair: Fix " tip-bot for Vincent Guittot
  2 siblings, 0 replies; 7+ messages in thread
From: Peter Zijlstra @ 2019-07-01 19:03 UTC (permalink / raw)
  To: Vincent Guittot; +Cc: linux-kernel, mingo

On Mon, Jul 01, 2019 at 05:47:02PM +0200, Vincent Guittot wrote:
> The load_balance() has a dedicated mecanism to detect when an imbalance
> is due to CPU affinity and must be handled at parent level. In this case,
> the imbalance field of the parent's sched_group is set.
> 
> The description of sg_imbalanced() gives a typical example of two groups
> of 4 CPUs each and 4 tasks each with a cpumask covering 1 CPU of the first
> group and 3 CPUs of the second group. Something like:
> 
> 	{ 0 1 2 3 } { 4 5 6 7 }
> 	        *     * * *
> 
> But the load_balance fails to fix this UC on my octo cores system
> made of 2 clusters of quad cores.
> 
> Whereas the load_balance is able to detect that the imbalanced is due to
> CPU affinity, it fails to fix it because the imbalance field is cleared
> before letting parent level a chance to run. In fact, when the imbalance is
> detected, the load_balance reruns without the CPU with pinned tasks. But
> there is no other running tasks in the situation described above and
> everything looks balanced this time so the imbalance field is immediately
> cleared.
> 
> The imbalance field should not be cleared if there is no other task to move
> when the imbalance is detected.
> 
> Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>

Thanks!

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] sched/fair: fix imbalance due to CPU affinity
  2019-07-01 15:47 [PATCH v2] sched/fair: fix imbalance due to CPU affinity Vincent Guittot
  2019-07-01 19:03 ` Peter Zijlstra
@ 2019-07-02  9:34 ` Valentin Schneider
  2019-07-02 10:00   ` Vincent Guittot
  2019-07-25 16:13 ` [tip:sched/core] sched/fair: Fix " tip-bot for Vincent Guittot
  2 siblings, 1 reply; 7+ messages in thread
From: Valentin Schneider @ 2019-07-02  9:34 UTC (permalink / raw)
  To: Vincent Guittot, linux-kernel, mingo, peterz

On 01/07/2019 16:47, Vincent Guittot wrote:
> The load_balance() has a dedicated mecanism to detect when an imbalance
> is due to CPU affinity and must be handled at parent level. In this case,
> the imbalance field of the parent's sched_group is set.
> 
> The description of sg_imbalanced() gives a typical example of two groups
> of 4 CPUs each and 4 tasks each with a cpumask covering 1 CPU of the first
> group and 3 CPUs of the second group. Something like:
> 
> 	{ 0 1 2 3 } { 4 5 6 7 }
> 	        *     * * *
> 
> But the load_balance fails to fix this UC on my octo cores system
> made of 2 clusters of quad cores.
> 
> Whereas the load_balance is able to detect that the imbalanced is due to
> CPU affinity, it fails to fix it because the imbalance field is cleared
> before letting parent level a chance to run. In fact, when the imbalance is
> detected, the load_balance reruns without the CPU with pinned tasks. But
> there is no other running tasks in the situation described above and
> everything looks balanced this time so the imbalance field is immediately
> cleared.
> 
> The imbalance field should not be cleared if there is no other task to move
> when the imbalance is detected.
> 
> Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>

Does that want a

Cc: stable@vger.kernel.org
Fixes: afdeee0510db ("sched: Fix imbalance flag reset")

?


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] sched/fair: fix imbalance due to CPU affinity
  2019-07-02  9:34 ` Valentin Schneider
@ 2019-07-02 10:00   ` Vincent Guittot
  2019-07-02 14:29     ` Valentin Schneider
  0 siblings, 1 reply; 7+ messages in thread
From: Vincent Guittot @ 2019-07-02 10:00 UTC (permalink / raw)
  To: Valentin Schneider; +Cc: linux-kernel, Ingo Molnar, Peter Zijlstra

On Tue, 2 Jul 2019 at 11:34, Valentin Schneider
<valentin.schneider@arm.com> wrote:
>
> On 01/07/2019 16:47, Vincent Guittot wrote:
> > The load_balance() has a dedicated mecanism to detect when an imbalance
> > is due to CPU affinity and must be handled at parent level. In this case,
> > the imbalance field of the parent's sched_group is set.
> >
> > The description of sg_imbalanced() gives a typical example of two groups
> > of 4 CPUs each and 4 tasks each with a cpumask covering 1 CPU of the first
> > group and 3 CPUs of the second group. Something like:
> >
> >       { 0 1 2 3 } { 4 5 6 7 }
> >               *     * * *
> >
> > But the load_balance fails to fix this UC on my octo cores system
> > made of 2 clusters of quad cores.
> >
> > Whereas the load_balance is able to detect that the imbalanced is due to
> > CPU affinity, it fails to fix it because the imbalance field is cleared
> > before letting parent level a chance to run. In fact, when the imbalance is
> > detected, the load_balance reruns without the CPU with pinned tasks. But
> > there is no other running tasks in the situation described above and
> > everything looks balanced this time so the imbalance field is immediately
> > cleared.
> >
> > The imbalance field should not be cleared if there is no other task to move
> > when the imbalance is detected.
> >
> > Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
>
> Does that want a
>
> Cc: stable@vger.kernel.org
> Fixes: afdeee0510db ("sched: Fix imbalance flag reset")

I was not sure that this has been introduced by this patch or
following changes. I haven't been able to test it on such old kernel
with my platform

>
> ?
>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] sched/fair: fix imbalance due to CPU affinity
  2019-07-02 10:00   ` Vincent Guittot
@ 2019-07-02 14:29     ` Valentin Schneider
  2019-07-05 12:23       ` Vincent Guittot
  0 siblings, 1 reply; 7+ messages in thread
From: Valentin Schneider @ 2019-07-02 14:29 UTC (permalink / raw)
  To: Vincent Guittot; +Cc: linux-kernel, Ingo Molnar, Peter Zijlstra



On 02/07/2019 11:00, Vincent Guittot wrote:
>> Does that want a
>>
>> Cc: stable@vger.kernel.org
>> Fixes: afdeee0510db ("sched: Fix imbalance flag reset")
> 
> I was not sure that this has been introduced by this patch or
> following changes. I haven't been able to test it on such old kernel
> with my platform
> 

Right, seems like

  65a4433aebe3 ("sched/fair: Fix load_balance() affinity redo path")

also played in this area. From surface level it looks like it only reduced
the amount of CPUs the load_balance() redo can use (and interestingly it
mentions the exact same bug as you observed, through triggered slightly
differently).

I'd be inclined to say that the issue was introduced by afdeee0510db, since
from looking at the code from that time I can see the issue happening:

- try to pull from a CPU with only tasks pinned to itself
- set sgc->imbalance
- redo with a CPU that sees no big imbalance
- goto out_balanced
- env.LBF_ALL_PINNED is still set but we clear sgc->imbalance

>>
>> ?
>>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] sched/fair: fix imbalance due to CPU affinity
  2019-07-02 14:29     ` Valentin Schneider
@ 2019-07-05 12:23       ` Vincent Guittot
  0 siblings, 0 replies; 7+ messages in thread
From: Vincent Guittot @ 2019-07-05 12:23 UTC (permalink / raw)
  To: Valentin Schneider; +Cc: linux-kernel, Ingo Molnar, Peter Zijlstra

On Tue, 2 Jul 2019 at 16:29, Valentin Schneider
<valentin.schneider@arm.com> wrote:
>
>
>
> On 02/07/2019 11:00, Vincent Guittot wrote:
> >> Does that want a
> >>
> >> Cc: stable@vger.kernel.org
> >> Fixes: afdeee0510db ("sched: Fix imbalance flag reset")
> >
> > I was not sure that this has been introduced by this patch or
> > following changes. I haven't been able to test it on such old kernel
> > with my platform
> >
>
> Right, seems like
>
>   65a4433aebe3 ("sched/fair: Fix load_balance() affinity redo path")
>
> also played in this area. From surface level it looks like it only reduced
> the amount of CPUs the load_balance() redo can use (and interestingly it
> mentions the exact same bug as you observed, through triggered slightly
> differently).
>
> I'd be inclined to say that the issue was introduced by afdeee0510db, since
> from looking at the code from that time I can see the issue happening:

I agree that the patch seems to be the root cause when reading code.
But it also means that the bug is there for almost  5 years and has
never been seen before I did some functional tests on my rework of the
load balance
That's why a real test would have confirmed that nothing else happens
in the meantime

>
> - try to pull from a CPU with only tasks pinned to itself
> - set sgc->imbalance
> - redo with a CPU that sees no big imbalance
> - goto out_balanced
> - env.LBF_ALL_PINNED is still set but we clear sgc->imbalance
>
> >>
> >> ?
> >>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [tip:sched/core] sched/fair: Fix imbalance due to CPU affinity
  2019-07-01 15:47 [PATCH v2] sched/fair: fix imbalance due to CPU affinity Vincent Guittot
  2019-07-01 19:03 ` Peter Zijlstra
  2019-07-02  9:34 ` Valentin Schneider
@ 2019-07-25 16:13 ` tip-bot for Vincent Guittot
  2 siblings, 0 replies; 7+ messages in thread
From: tip-bot for Vincent Guittot @ 2019-07-25 16:13 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: peterz, linux-kernel, torvalds, hpa, vincent.guittot, tglx, mingo

Commit-ID:  f6cad8df6b30a5d2bbbd2e698f74b4cafb9fb82b
Gitweb:     https://git.kernel.org/tip/f6cad8df6b30a5d2bbbd2e698f74b4cafb9fb82b
Author:     Vincent Guittot <vincent.guittot@linaro.org>
AuthorDate: Mon, 1 Jul 2019 17:47:02 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Thu, 25 Jul 2019 15:51:52 +0200

sched/fair: Fix imbalance due to CPU affinity

The load_balance() has a dedicated mecanism to detect when an imbalance
is due to CPU affinity and must be handled at parent level. In this case,
the imbalance field of the parent's sched_group is set.

The description of sg_imbalanced() gives a typical example of two groups
of 4 CPUs each and 4 tasks each with a cpumask covering 1 CPU of the first
group and 3 CPUs of the second group. Something like:

	{ 0 1 2 3 } { 4 5 6 7 }
	        *     * * *

But the load_balance fails to fix this UC on my octo cores system
made of 2 clusters of quad cores.

Whereas the load_balance is able to detect that the imbalanced is due to
CPU affinity, it fails to fix it because the imbalance field is cleared
before letting parent level a chance to run. In fact, when the imbalance is
detected, the load_balance reruns without the CPU with pinned tasks. But
there is no other running tasks in the situation described above and
everything looks balanced this time so the imbalance field is immediately
cleared.

The imbalance field should not be cleared if there is no other task to move
when the imbalance is detected.

Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Link: https://lkml.kernel.org/r/1561996022-28829-1-git-send-email-vincent.guittot@linaro.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/sched/fair.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index b5546a15206c..9be36ffb5689 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -9048,9 +9048,10 @@ more_balance:
 out_balanced:
 	/*
 	 * We reach balance although we may have faced some affinity
-	 * constraints. Clear the imbalance flag if it was set.
+	 * constraints. Clear the imbalance flag only if other tasks got
+	 * a chance to move and fix the imbalance.
 	 */
-	if (sd_parent) {
+	if (sd_parent && !(env.flags & LBF_ALL_PINNED)) {
 		int *group_imbalance = &sd_parent->groups->sgc->imbalance;
 
 		if (*group_imbalance)

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2019-07-25 16:14 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-01 15:47 [PATCH v2] sched/fair: fix imbalance due to CPU affinity Vincent Guittot
2019-07-01 19:03 ` Peter Zijlstra
2019-07-02  9:34 ` Valentin Schneider
2019-07-02 10:00   ` Vincent Guittot
2019-07-02 14:29     ` Valentin Schneider
2019-07-05 12:23       ` Vincent Guittot
2019-07-25 16:13 ` [tip:sched/core] sched/fair: Fix " tip-bot 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.