* [PATCH v3] sched/fair: Add ancestors of unthrottled undecayed cfs_rq
@ 2021-09-17 15:30 Michal Koutný
2021-09-17 16:05 ` Vincent Guittot
` (2 more replies)
0 siblings, 3 replies; 4+ messages in thread
From: Michal Koutný @ 2021-09-17 15:30 UTC (permalink / raw)
To: linux-kernel, Vincent Guittot
Cc: Phil Auld, Ingo Molnar, Peter Zijlstra, Juri Lelli,
Dietmar Eggemann, Steven Rostedt, Ben Segall, Mel Gorman,
Daniel Bristot de Oliveira, Odin Ugedal, Rik van Riel,
Giovanni Gherdovich
Since commit a7b359fc6a37 ("sched/fair: Correctly insert cfs_rq's to
list on unthrottle") we add cfs_rqs with no runnable tasks but not fully
decayed into the load (leaf) list. We may ignore adding some ancestors
and therefore breaking tmp_alone_branch invariant. This broke LTP test
cfs_bandwidth01 and it was partially fixed in commit fdaba61ef8a2
("sched/fair: Ensure that the CFS parent is added after unthrottling").
I noticed the named test still fails even with the fix (but with low
probability, 1 in ~1000 executions of the test). The reason is when
bailing out of unthrottle_cfs_rq early, we may miss adding ancestors of
the unthrottled cfs_rq, thus, not joining tmp_alone_branch properly.
Fix this by adding ancestors if we notice the unthrottled cfs_rq was
added to the load list.
Fixes: a7b359fc6a37 ("sched/fair: Correctly insert cfs_rq's to list on unthrottle")
Signed-off-by: Michal Koutný <mkoutny@suse.com>
---
Changes since v2 [1]:
- jump to completion for loop, don't duplicate it
- singled out of the series (to handle a fix separately from the rest)
[1] https://lore.kernel.org/r/20210819175034.4577-2-mkoutny@suse.com/
kernel/sched/fair.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index ff69f245b939..f6a05d9b5443 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -4936,8 +4936,12 @@ void unthrottle_cfs_rq(struct cfs_rq *cfs_rq)
/* update hierarchical throttle state */
walk_tg_tree_from(cfs_rq->tg, tg_nop, tg_unthrottle_up, (void *)rq);
- if (!cfs_rq->load.weight)
+ /* Nothing to run but something to decay (on_list)? Complete the branch */
+ if (!cfs_rq->load.weight) {
+ if (cfs_rq->on_list)
+ goto unthrottle_throttle;
return;
+ }
task_delta = cfs_rq->h_nr_running;
idle_task_delta = cfs_rq->idle_h_nr_running;
--
2.32.0
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH v3] sched/fair: Add ancestors of unthrottled undecayed cfs_rq
2021-09-17 15:30 [PATCH v3] sched/fair: Add ancestors of unthrottled undecayed cfs_rq Michal Koutný
@ 2021-09-17 16:05 ` Vincent Guittot
2021-09-20 20:06 ` Odin Ugedal
2021-10-01 12:10 ` [tip: sched/urgent] " tip-bot2 for Michal Koutný
2 siblings, 0 replies; 4+ messages in thread
From: Vincent Guittot @ 2021-09-17 16:05 UTC (permalink / raw)
To: Michal Koutný
Cc: linux-kernel, Phil Auld, Ingo Molnar, Peter Zijlstra, Juri Lelli,
Dietmar Eggemann, Steven Rostedt, Ben Segall, Mel Gorman,
Daniel Bristot de Oliveira, Odin Ugedal, Rik van Riel,
Giovanni Gherdovich
On Fri, 17 Sept 2021 at 17:32, Michal Koutný <mkoutny@suse.com> wrote:
>
> Since commit a7b359fc6a37 ("sched/fair: Correctly insert cfs_rq's to
> list on unthrottle") we add cfs_rqs with no runnable tasks but not fully
> decayed into the load (leaf) list. We may ignore adding some ancestors
> and therefore breaking tmp_alone_branch invariant. This broke LTP test
> cfs_bandwidth01 and it was partially fixed in commit fdaba61ef8a2
> ("sched/fair: Ensure that the CFS parent is added after unthrottling").
>
> I noticed the named test still fails even with the fix (but with low
> probability, 1 in ~1000 executions of the test). The reason is when
> bailing out of unthrottle_cfs_rq early, we may miss adding ancestors of
> the unthrottled cfs_rq, thus, not joining tmp_alone_branch properly.
>
> Fix this by adding ancestors if we notice the unthrottled cfs_rq was
> added to the load list.
>
> Fixes: a7b359fc6a37 ("sched/fair: Correctly insert cfs_rq's to list on unthrottle")
> Signed-off-by: Michal Koutný <mkoutny@suse.com>
Reviewed-by: Vincent Guittot <vincent.guittot@linaro.org>
> ---
>
> Changes since v2 [1]:
> - jump to completion for loop, don't duplicate it
> - singled out of the series (to handle a fix separately from the rest)
>
> [1] https://lore.kernel.org/r/20210819175034.4577-2-mkoutny@suse.com/
>
> kernel/sched/fair.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index ff69f245b939..f6a05d9b5443 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -4936,8 +4936,12 @@ void unthrottle_cfs_rq(struct cfs_rq *cfs_rq)
> /* update hierarchical throttle state */
> walk_tg_tree_from(cfs_rq->tg, tg_nop, tg_unthrottle_up, (void *)rq);
>
> - if (!cfs_rq->load.weight)
> + /* Nothing to run but something to decay (on_list)? Complete the branch */
> + if (!cfs_rq->load.weight) {
> + if (cfs_rq->on_list)
> + goto unthrottle_throttle;
> return;
> + }
>
> task_delta = cfs_rq->h_nr_running;
> idle_task_delta = cfs_rq->idle_h_nr_running;
> --
> 2.32.0
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH v3] sched/fair: Add ancestors of unthrottled undecayed cfs_rq
2021-09-17 15:30 [PATCH v3] sched/fair: Add ancestors of unthrottled undecayed cfs_rq Michal Koutný
2021-09-17 16:05 ` Vincent Guittot
@ 2021-09-20 20:06 ` Odin Ugedal
2021-10-01 12:10 ` [tip: sched/urgent] " tip-bot2 for Michal Koutný
2 siblings, 0 replies; 4+ messages in thread
From: Odin Ugedal @ 2021-09-20 20:06 UTC (permalink / raw)
To: Michal Koutný
Cc: open list, Vincent Guittot, Phil Auld, Ingo Molnar,
Peter Zijlstra, Juri Lelli, Dietmar Eggemann, Steven Rostedt,
Ben Segall, Mel Gorman, Daniel Bristot de Oliveira, Odin Ugedal,
Rik van Riel, Giovanni Gherdovich
fre. 17. sep. 2021 kl. 16:32 skrev Michal Koutný <mkoutny@suse.com>:
>
> Since commit a7b359fc6a37 ("sched/fair: Correctly insert cfs_rq's to
> list on unthrottle") we add cfs_rqs with no runnable tasks but not fully
> decayed into the load (leaf) list. We may ignore adding some ancestors
> and therefore breaking tmp_alone_branch invariant. This broke LTP test
> cfs_bandwidth01 and it was partially fixed in commit fdaba61ef8a2
> ("sched/fair: Ensure that the CFS parent is added after unthrottling").
>
> I noticed the named test still fails even with the fix (but with low
> probability, 1 in ~1000 executions of the test). The reason is when
> bailing out of unthrottle_cfs_rq early, we may miss adding ancestors of
> the unthrottled cfs_rq, thus, not joining tmp_alone_branch properly.
>
> Fix this by adding ancestors if we notice the unthrottled cfs_rq was
> added to the load list.
>
> Fixes: a7b359fc6a37 ("sched/fair: Correctly insert cfs_rq's to list on unthrottle")
> Signed-off-by: Michal Koutný <mkoutny@suse.com>
Thanks for catching this!
Reviewed-by: Odin Ugedal <odin@uged.al>
> Changes since v2 [1]:
> - jump to completion for loop, don't duplicate it
> - singled out of the series (to handle a fix separately from the rest)
>
> [1] https://lore.kernel.org/r/20210819175034.4577-2-mkoutny@suse.com/
>
> kernel/sched/fair.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index ff69f245b939..f6a05d9b5443 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -4936,8 +4936,12 @@ void unthrottle_cfs_rq(struct cfs_rq *cfs_rq)
> /* update hierarchical throttle state */
> walk_tg_tree_from(cfs_rq->tg, tg_nop, tg_unthrottle_up, (void *)rq);
>
> - if (!cfs_rq->load.weight)
> + /* Nothing to run but something to decay (on_list)? Complete the branch */
> + if (!cfs_rq->load.weight) {
> + if (cfs_rq->on_list)
> + goto unthrottle_throttle;
> return;
> + }
>
> task_delta = cfs_rq->h_nr_running;
> idle_task_delta = cfs_rq->idle_h_nr_running;
> --
> 2.32.0
>
^ permalink raw reply [flat|nested] 4+ messages in thread* [tip: sched/urgent] sched/fair: Add ancestors of unthrottled undecayed cfs_rq
2021-09-17 15:30 [PATCH v3] sched/fair: Add ancestors of unthrottled undecayed cfs_rq Michal Koutný
2021-09-17 16:05 ` Vincent Guittot
2021-09-20 20:06 ` Odin Ugedal
@ 2021-10-01 12:10 ` tip-bot2 for Michal Koutný
2 siblings, 0 replies; 4+ messages in thread
From: tip-bot2 for Michal Koutný @ 2021-10-01 12:10 UTC (permalink / raw)
To: linux-tip-commits
Cc: mkoutny, Peter Zijlstra (Intel),
Vincent Guittot, Odin Ugedal, x86, linux-kernel
The following commit has been merged into the sched/urgent branch of tip:
Commit-ID: 2630cde26711dab0d0b56a8be1616475be646d13
Gitweb: https://git.kernel.org/tip/2630cde26711dab0d0b56a8be1616475be646d13
Author: Michal Koutný <mkoutny@suse.com>
AuthorDate: Fri, 17 Sep 2021 17:30:37 +02:00
Committer: Peter Zijlstra <peterz@infradead.org>
CommitterDate: Fri, 01 Oct 2021 13:57:57 +02:00
sched/fair: Add ancestors of unthrottled undecayed cfs_rq
Since commit a7b359fc6a37 ("sched/fair: Correctly insert cfs_rq's to
list on unthrottle") we add cfs_rqs with no runnable tasks but not fully
decayed into the load (leaf) list. We may ignore adding some ancestors
and therefore breaking tmp_alone_branch invariant. This broke LTP test
cfs_bandwidth01 and it was partially fixed in commit fdaba61ef8a2
("sched/fair: Ensure that the CFS parent is added after unthrottling").
I noticed the named test still fails even with the fix (but with low
probability, 1 in ~1000 executions of the test). The reason is when
bailing out of unthrottle_cfs_rq early, we may miss adding ancestors of
the unthrottled cfs_rq, thus, not joining tmp_alone_branch properly.
Fix this by adding ancestors if we notice the unthrottled cfs_rq was
added to the load list.
Fixes: a7b359fc6a37 ("sched/fair: Correctly insert cfs_rq's to list on unthrottle")
Signed-off-by: Michal Koutný <mkoutny@suse.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Vincent Guittot <vincent.guittot@linaro.org>
Reviewed-by: Odin Ugedal <odin@uged.al>
Link: https://lore.kernel.org/r/20210917153037.11176-1-mkoutny@suse.com
---
kernel/sched/fair.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index ff69f24..f6a05d9 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -4936,8 +4936,12 @@ void unthrottle_cfs_rq(struct cfs_rq *cfs_rq)
/* update hierarchical throttle state */
walk_tg_tree_from(cfs_rq->tg, tg_nop, tg_unthrottle_up, (void *)rq);
- if (!cfs_rq->load.weight)
+ /* Nothing to run but something to decay (on_list)? Complete the branch */
+ if (!cfs_rq->load.weight) {
+ if (cfs_rq->on_list)
+ goto unthrottle_throttle;
return;
+ }
task_delta = cfs_rq->h_nr_running;
idle_task_delta = cfs_rq->idle_h_nr_running;
^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-10-01 12:10 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-17 15:30 [PATCH v3] sched/fair: Add ancestors of unthrottled undecayed cfs_rq Michal Koutný
2021-09-17 16:05 ` Vincent Guittot
2021-09-20 20:06 ` Odin Ugedal
2021-10-01 12:10 ` [tip: sched/urgent] " tip-bot2 for Michal Koutný
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).