Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1629966 > unrolled thread
| Started by | Tejun Heo <tj@kernel.org> |
|---|---|
| First post | 2017-04-24 22:20 +0200 |
| Last post | 2017-05-04 21:10 +0200 |
| Articles | 20 on this page of 35 — 3 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
[PATCH 2/2] sched/fair: Always propagate runnable_load_avg Tejun Heo <tj@kernel.org> - 2017-04-24 22:20 +0200
Re: [PATCH 2/2] sched/fair: Always propagate runnable_load_avg Vincent Guittot <vincent.guittot@linaro.org> - 2017-04-25 10:50 +0200
Re: [PATCH 2/2] sched/fair: Always propagate runnable_load_avg Vincent Guittot <vincent.guittot@linaro.org> - 2017-04-25 11:10 +0200
Re: [PATCH 2/2] sched/fair: Always propagate runnable_load_avg Vincent Guittot <vincent.guittot@linaro.org> - 2017-04-25 15:00 +0200
Re: [PATCH 2/2] sched/fair: Always propagate runnable_load_avg Tejun Heo <tj@kernel.org> - 2017-04-25 21:00 +0200
Re: [PATCH 2/2] sched/fair: Always propagate runnable_load_avg Tejun Heo <tj@kernel.org> - 2017-04-25 22:50 +0200
Re: [PATCH 2/2] sched/fair: Always propagate runnable_load_avg Tejun Heo <tj@kernel.org> - 2017-04-25 23:10 +0200
Re: [PATCH 2/2] sched/fair: Always propagate runnable_load_avg Vincent Guittot <vincent.guittot@linaro.org> - 2017-04-26 12:30 +0200
Re: [PATCH 2/2] sched/fair: Always propagate runnable_load_avg Tejun Heo <tj@kernel.org> - 2017-04-27 02:40 +0200
Re: [PATCH 2/2] sched/fair: Always propagate runnable_load_avg Vincent Guittot <vincent.guittot@linaro.org> - 2017-04-27 10:30 +0200
Re: [PATCH 2/2] sched/fair: Always propagate runnable_load_avg Tejun Heo <tj@kernel.org> - 2017-04-28 18:20 +0200
Re: [PATCH 2/2] sched/fair: Always propagate runnable_load_avg Vincent Guittot <vincent.guittot@linaro.org> - 2017-05-02 09:00 +0200
Re: [PATCH 2/2] sched/fair: Always propagate runnable_load_avg Tejun Heo <tj@kernel.org> - 2017-05-02 23:00 +0200
Re: [PATCH 2/2] sched/fair: Always propagate runnable_load_avg Vincent Guittot <vincent.guittot@linaro.org> - 2017-05-03 09:30 +0200
Re: [PATCH 2/2] sched/fair: Always propagate runnable_load_avg Vincent Guittot <vincent.guittot@linaro.org> - 2017-05-03 10:00 +0200
Re: [PATCH 2/2] sched/fair: Always propagate runnable_load_avg Vincent Guittot <vincent.guittot@linaro.org> - 2017-04-26 20:20 +0200
Re: [PATCH 2/2] sched/fair: Always propagate runnable_load_avg Tejun Heo <tj@kernel.org> - 2017-04-27 01:00 +0200
Re: [PATCH 2/2] sched/fair: Always propagate runnable_load_avg Vincent Guittot <vincent.guittot@linaro.org> - 2017-04-27 10:30 +0200
Re: [PATCH 2/2] sched/fair: Always propagate runnable_load_avg Tejun Heo <tj@kernel.org> - 2017-04-28 22:40 +0200
Re: [PATCH 2/2] sched/fair: Always propagate runnable_load_avg Tejun Heo <tj@kernel.org> - 2017-04-28 22:40 +0200
Re: [PATCH 2/2] sched/fair: Always propagate runnable_load_avg Peter Zijlstra <peterz@infradead.org> - 2017-05-01 18:00 +0200
Re: [PATCH 2/2] sched/fair: Always propagate runnable_load_avg Tejun Heo <tj@kernel.org> - 2017-05-03 00:10 +0200
Re: [PATCH 2/2] sched/fair: Always propagate runnable_load_avg Vincent Guittot <vincent.guittot@linaro.org> - 2017-05-02 09:20 +0200
Re: [PATCH 2/2] sched/fair: Always propagate runnable_load_avg Vincent Guittot <vincent.guittot@linaro.org> - 2017-05-02 15:30 +0200
Re: [PATCH 2/2] sched/fair: Always propagate runnable_load_avg Tejun Heo <tj@kernel.org> - 2017-05-03 00:40 +0200
Re: [PATCH 2/2] sched/fair: Always propagate runnable_load_avg Tejun Heo <tj@kernel.org> - 2017-05-03 00:00 +0200
Re: [PATCH 2/2] sched/fair: Always propagate runnable_load_avg Vincent Guittot <vincent.guittot@linaro.org> - 2017-05-03 09:40 +0200
Re: [PATCH 2/2] sched/fair: Always propagate runnable_load_avg Peter Zijlstra <peterz@infradead.org> - 2017-05-03 11:40 +0200
Re: [PATCH 2/2] sched/fair: Always propagate runnable_load_avg Vincent Guittot <vincent.guittot@linaro.org> - 2017-05-03 12:40 +0200
Re: [PATCH 2/2] sched/fair: Always propagate runnable_load_avg Peter Zijlstra <peterz@infradead.org> - 2017-05-03 15:20 +0200
Re: [PATCH 2/2] sched/fair: Always propagate runnable_load_avg Tejun Heo <tj@kernel.org> - 2017-05-03 23:50 +0200
Re: [PATCH 2/2] sched/fair: Always propagate runnable_load_avg Vincent Guittot <vincent.guittot@linaro.org> - 2017-05-04 10:30 +0200
Re: [PATCH 2/2] sched/fair: Always propagate runnable_load_avg Tejun Heo <tj@kernel.org> - 2017-05-04 19:50 +0200
Re: [PATCH 2/2] sched/fair: Always propagate runnable_load_avg Vincent Guittot <vincent.guittot@linaro.org> - 2017-05-04 21:10 +0200
Re: [PATCH 2/2] sched/fair: Always propagate runnable_load_avg Tejun Heo <tj@kernel.org> - 2017-05-04 21:10 +0200
Page 1 of 2 [1] 2 Next page →
| From | Tejun Heo <tj@kernel.org> |
|---|---|
| Date | 2017-04-24 22:20 +0200 |
| Subject | [PATCH 2/2] sched/fair: Always propagate runnable_load_avg |
| Message-ID | <tzSCK-37e-15@gated-at.bofh.it> |
We noticed that with cgroup CPU controller in use, the scheduling
latency gets wonky regardless of nesting level or weight
configuration. This is easily reproducible with Chris Mason's
schbench[1].
All tests are run on a single socket, 16 cores, 32 threads machine.
While the machine is mostly idle, it isn't completey. There's always
some variable management load going on. The command used is
schbench -m 2 -t 16 -s 10000 -c 15000 -r 30
which measures scheduling latency with 32 threads each of which
repeatedly runs for 15ms and then sleeps for 10ms. Here's a
representative result when running from the root cgroup.
# ~/schbench -m 2 -t 16 -s 10000 -c 15000 -r 30
Latency percentiles (usec)
50.0000th: 26
75.0000th: 62
90.0000th: 74
95.0000th: 86
*99.0000th: 887
99.5000th: 3692
99.9000th: 10832
min=0, max=13374
The following is inside a first level CPU cgroup with the maximum
weight.
# ~/schbench -m 2 -t 16 -s 10000 -c 15000 -r 30
Latency percentiles (usec)
50.0000th: 31
75.0000th: 65
90.0000th: 71
95.0000th: 91
*99.0000th: 7288
99.5000th: 10352
99.9000th: 12496
min=0, max=13023
Note the drastic increase in p99 scheduling latency. After
investigation, it turned out that the update_sd_lb_stats(), which is
used by load_balance() to pick the most loaded group, was often
picking the wrong group. A CPU which has one schbench running and
another queued wouldn't report the correspondingly higher
weighted_cpuload() and get looked over as the target of load
balancing.
weighted_cpuload() is the root cfs_rq's runnable_load_avg which is the
sum of the load_avg of all queued sched_entities. Without cgroups or
at the root cgroup, each task's load_avg contributes directly to the
sum. When a task wakes up or goes to sleep, the change is immediately
reflected on runnable_load_avg which in turn affects load balancing.
However, when CPU cgroup is in use, a nesting cfs_rq blocks this
immediate reflection. When a task wakes up inside a cgroup, the
nested cfs_rq's runnable_load_avg is updated but doesn't get
propagated to the parent. It only affects the matching sched_entity's
load_avg over time which then gets propagated to the runnable_load_avg
at that level. This makes weighted_cpuload() often temporarily out of
sync leading to suboptimal behavior of load_balance() and increase in
scheduling latencies as shown above.
This patch fixes the issue by updating propagate_entity_load_avg() to
always propagate to the parent's runnable_load_avg. Combined with the
previous patch, this keeps a cfs_rq's runnable_load_avg always the sum
of the scaled loads of all tasks queued below removing the artifacts
from nesting cfs_rqs. The following is from inside three levels of
nesting with the patch applied.
# ~/schbench -m 2 -t 16 -s 10000 -c 15000 -r 30
Latency percentiles (usec)
50.0000th: 40
75.0000th: 71
90.0000th: 89
95.0000th: 108
*99.0000th: 679
99.5000th: 3500
99.9000th: 10960
min=0, max=13790
[1] git://git.kernel.org/pub/scm/linux/kernel/git/mason/schbench.git
Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Vincent Guittot <vincent.guittot@linaro.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Paul Turner <pjt@google.com>
Cc: Chris Mason <clm@fb.com>
---
kernel/sched/fair.c | 34 +++++++++++++++++++++-------------
1 file changed, 21 insertions(+), 13 deletions(-)
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -3075,7 +3075,8 @@ update_tg_cfs_util(struct cfs_rq *cfs_rq
/* Take into account change of load of a child task group */
static inline void
-update_tg_cfs_load(struct cfs_rq *cfs_rq, struct sched_entity *se)
+update_tg_cfs_load(struct cfs_rq *cfs_rq, struct sched_entity *se,
+ bool propagate_avg)
{
struct cfs_rq *gcfs_rq = group_cfs_rq(se);
long load = 0, delta;
@@ -3113,9 +3114,11 @@ update_tg_cfs_load(struct cfs_rq *cfs_rq
se->avg.load_avg = load;
se->avg.load_sum = se->avg.load_avg * LOAD_AVG_MAX;
- /* Update parent cfs_rq load */
- add_positive(&cfs_rq->avg.load_avg, delta);
- cfs_rq->avg.load_sum = cfs_rq->avg.load_avg * LOAD_AVG_MAX;
+ if (propagate_avg) {
+ /* Update parent cfs_rq load */
+ add_positive(&cfs_rq->avg.load_avg, delta);
+ cfs_rq->avg.load_sum = cfs_rq->avg.load_avg * LOAD_AVG_MAX;
+ }
/*
* If the sched_entity is already enqueued, we also have to update the
@@ -3147,22 +3150,27 @@ static inline int test_and_clear_tg_cfs_
/* Update task and its cfs_rq load average */
static inline int propagate_entity_load_avg(struct sched_entity *se)
{
- struct cfs_rq *cfs_rq;
+ struct cfs_rq *cfs_rq = cfs_rq_of(se);
+ bool propagate_avg;
if (entity_is_task(se))
return 0;
- if (!test_and_clear_tg_cfs_propagate(se))
- return 0;
-
- cfs_rq = cfs_rq_of(se);
+ propagate_avg = test_and_clear_tg_cfs_propagate(se);
- set_tg_cfs_propagate(cfs_rq);
+ /*
+ * We want to keep @cfs_rq->runnable_load_avg always in sync so
+ * that the load balancer can accurately determine the busiest CPU
+ * regardless of cfs_rq nesting.
+ */
+ update_tg_cfs_load(cfs_rq, se, propagate_avg);
- update_tg_cfs_util(cfs_rq, se);
- update_tg_cfs_load(cfs_rq, se);
+ if (propagate_avg) {
+ set_tg_cfs_propagate(cfs_rq);
+ update_tg_cfs_util(cfs_rq, se);
+ }
- return 1;
+ return propagate_avg;
}
#else /* CONFIG_FAIR_GROUP_SCHED */
[toc] | [next] | [standalone]
| From | Vincent Guittot <vincent.guittot@linaro.org> |
|---|---|
| Date | 2017-04-25 10:50 +0200 |
| Message-ID | <tA4ky-2l1-3@gated-at.bofh.it> |
| In reply to | #1629966 |
On 24 April 2017 at 22:14, Tejun Heo <tj@kernel.org> wrote:
> We noticed that with cgroup CPU controller in use, the scheduling
> latency gets wonky regardless of nesting level or weight
> configuration. This is easily reproducible with Chris Mason's
> schbench[1].
>
> All tests are run on a single socket, 16 cores, 32 threads machine.
> While the machine is mostly idle, it isn't completey. There's always
> some variable management load going on. The command used is
>
> schbench -m 2 -t 16 -s 10000 -c 15000 -r 30
>
> which measures scheduling latency with 32 threads each of which
> repeatedly runs for 15ms and then sleeps for 10ms. Here's a
> representative result when running from the root cgroup.
>
> # ~/schbench -m 2 -t 16 -s 10000 -c 15000 -r 30
> Latency percentiles (usec)
> 50.0000th: 26
> 75.0000th: 62
> 90.0000th: 74
> 95.0000th: 86
> *99.0000th: 887
> 99.5000th: 3692
> 99.9000th: 10832
> min=0, max=13374
>
> The following is inside a first level CPU cgroup with the maximum
> weight.
>
> # ~/schbench -m 2 -t 16 -s 10000 -c 15000 -r 30
> Latency percentiles (usec)
> 50.0000th: 31
> 75.0000th: 65
> 90.0000th: 71
> 95.0000th: 91
> *99.0000th: 7288
> 99.5000th: 10352
> 99.9000th: 12496
> min=0, max=13023
>
> Note the drastic increase in p99 scheduling latency. After
> investigation, it turned out that the update_sd_lb_stats(), which is
> used by load_balance() to pick the most loaded group, was often
> picking the wrong group. A CPU which has one schbench running and
> another queued wouldn't report the correspondingly higher
> weighted_cpuload() and get looked over as the target of load
> balancing.
>
> weighted_cpuload() is the root cfs_rq's runnable_load_avg which is the
> sum of the load_avg of all queued sched_entities. Without cgroups or
> at the root cgroup, each task's load_avg contributes directly to the
> sum. When a task wakes up or goes to sleep, the change is immediately
> reflected on runnable_load_avg which in turn affects load balancing.
>
> However, when CPU cgroup is in use, a nesting cfs_rq blocks this
> immediate reflection. When a task wakes up inside a cgroup, the
> nested cfs_rq's runnable_load_avg is updated but doesn't get
> propagated to the parent. It only affects the matching sched_entity's
> load_avg over time which then gets propagated to the runnable_load_avg
> at that level. This makes weighted_cpuload() often temporarily out of
> sync leading to suboptimal behavior of load_balance() and increase in
> scheduling latencies as shown above.
>
> This patch fixes the issue by updating propagate_entity_load_avg() to
> always propagate to the parent's runnable_load_avg. Combined with the
> previous patch, this keeps a cfs_rq's runnable_load_avg always the sum
> of the scaled loads of all tasks queued below removing the artifacts
> from nesting cfs_rqs. The following is from inside three levels of
> nesting with the patch applied.
So you are changing the purpose of propagate_entity_load_avg which
aims to propagate load_avg/util_avg changes only when a task migrate
and you also want to propagate the enqueue/dequeue in the parent
cfs_rq->runnable_load_avg
>
> # ~/schbench -m 2 -t 16 -s 10000 -c 15000 -r 30
> Latency percentiles (usec)
> 50.0000th: 40
> 75.0000th: 71
> 90.0000th: 89
> 95.0000th: 108
> *99.0000th: 679
> 99.5000th: 3500
> 99.9000th: 10960
> min=0, max=13790
>
> [1] git://git.kernel.org/pub/scm/linux/kernel/git/mason/schbench.git
>
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Cc: Vincent Guittot <vincent.guittot@linaro.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Mike Galbraith <efault@gmx.de>
> Cc: Paul Turner <pjt@google.com>
> Cc: Chris Mason <clm@fb.com>
> ---
> kernel/sched/fair.c | 34 +++++++++++++++++++++-------------
> 1 file changed, 21 insertions(+), 13 deletions(-)
>
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -3075,7 +3075,8 @@ update_tg_cfs_util(struct cfs_rq *cfs_rq
>
> /* Take into account change of load of a child task group */
> static inline void
> -update_tg_cfs_load(struct cfs_rq *cfs_rq, struct sched_entity *se)
> +update_tg_cfs_load(struct cfs_rq *cfs_rq, struct sched_entity *se,
> + bool propagate_avg)
> {
> struct cfs_rq *gcfs_rq = group_cfs_rq(se);
> long load = 0, delta;
> @@ -3113,9 +3114,11 @@ update_tg_cfs_load(struct cfs_rq *cfs_rq
> se->avg.load_avg = load;
> se->avg.load_sum = se->avg.load_avg * LOAD_AVG_MAX;
>
> - /* Update parent cfs_rq load */
> - add_positive(&cfs_rq->avg.load_avg, delta);
> - cfs_rq->avg.load_sum = cfs_rq->avg.load_avg * LOAD_AVG_MAX;
> + if (propagate_avg) {
> + /* Update parent cfs_rq load */
> + add_positive(&cfs_rq->avg.load_avg, delta);
> + cfs_rq->avg.load_sum = cfs_rq->avg.load_avg * LOAD_AVG_MAX;
> + }
>
> /*
> * If the sched_entity is already enqueued, we also have to update the
> @@ -3147,22 +3150,27 @@ static inline int test_and_clear_tg_cfs_
> /* Update task and its cfs_rq load average */
> static inline int propagate_entity_load_avg(struct sched_entity *se)
> {
> - struct cfs_rq *cfs_rq;
> + struct cfs_rq *cfs_rq = cfs_rq_of(se);
> + bool propagate_avg;
>
> if (entity_is_task(se))
> return 0;
>
> - if (!test_and_clear_tg_cfs_propagate(se))
> - return 0;
> -
> - cfs_rq = cfs_rq_of(se);
> + propagate_avg = test_and_clear_tg_cfs_propagate(se);
>
> - set_tg_cfs_propagate(cfs_rq);
> + /*
> + * We want to keep @cfs_rq->runnable_load_avg always in sync so
> + * that the load balancer can accurately determine the busiest CPU
> + * regardless of cfs_rq nesting.
> + */
> + update_tg_cfs_load(cfs_rq, se, propagate_avg);
>
> - update_tg_cfs_util(cfs_rq, se);
> - update_tg_cfs_load(cfs_rq, se);
> + if (propagate_avg) {
> + set_tg_cfs_propagate(cfs_rq);
> + update_tg_cfs_util(cfs_rq, se);
> + }
>
> - return 1;
> + return propagate_avg;
> }
>
> #else /* CONFIG_FAIR_GROUP_SCHED */
[toc] | [prev] | [next] | [standalone]
| From | Vincent Guittot <vincent.guittot@linaro.org> |
|---|---|
| Date | 2017-04-25 11:10 +0200 |
| Message-ID | <tA4DU-2H8-15@gated-at.bofh.it> |
| In reply to | #1630302 |
On 25 April 2017 at 10:46, Vincent Guittot <vincent.guittot@linaro.org> wrote:
> On 24 April 2017 at 22:14, Tejun Heo <tj@kernel.org> wrote:
>> We noticed that with cgroup CPU controller in use, the scheduling
>> latency gets wonky regardless of nesting level or weight
>> configuration. This is easily reproducible with Chris Mason's
>> schbench[1].
>>
>> All tests are run on a single socket, 16 cores, 32 threads machine.
>> While the machine is mostly idle, it isn't completey. There's always
>> some variable management load going on. The command used is
>>
>> schbench -m 2 -t 16 -s 10000 -c 15000 -r 30
>>
>> which measures scheduling latency with 32 threads each of which
>> repeatedly runs for 15ms and then sleeps for 10ms. Here's a
>> representative result when running from the root cgroup.
>>
>> # ~/schbench -m 2 -t 16 -s 10000 -c 15000 -r 30
>> Latency percentiles (usec)
>> 50.0000th: 26
>> 75.0000th: 62
>> 90.0000th: 74
>> 95.0000th: 86
>> *99.0000th: 887
>> 99.5000th: 3692
>> 99.9000th: 10832
>> min=0, max=13374
>>
>> The following is inside a first level CPU cgroup with the maximum
>> weight.
>>
>> # ~/schbench -m 2 -t 16 -s 10000 -c 15000 -r 30
>> Latency percentiles (usec)
>> 50.0000th: 31
>> 75.0000th: 65
>> 90.0000th: 71
>> 95.0000th: 91
>> *99.0000th: 7288
>> 99.5000th: 10352
>> 99.9000th: 12496
>> min=0, max=13023
>>
>> Note the drastic increase in p99 scheduling latency. After
>> investigation, it turned out that the update_sd_lb_stats(), which is
>> used by load_balance() to pick the most loaded group, was often
>> picking the wrong group. A CPU which has one schbench running and
>> another queued wouldn't report the correspondingly higher
>> weighted_cpuload() and get looked over as the target of load
>> balancing.
>>
>> weighted_cpuload() is the root cfs_rq's runnable_load_avg which is the
>> sum of the load_avg of all queued sched_entities. Without cgroups or
>> at the root cgroup, each task's load_avg contributes directly to the
>> sum. When a task wakes up or goes to sleep, the change is immediately
>> reflected on runnable_load_avg which in turn affects load balancing.
>>
>> However, when CPU cgroup is in use, a nesting cfs_rq blocks this
>> immediate reflection. When a task wakes up inside a cgroup, the
>> nested cfs_rq's runnable_load_avg is updated but doesn't get
>> propagated to the parent. It only affects the matching sched_entity's
>> load_avg over time which then gets propagated to the runnable_load_avg
>> at that level. This makes weighted_cpuload() often temporarily out of
>> sync leading to suboptimal behavior of load_balance() and increase in
>> scheduling latencies as shown above.
>>
>> This patch fixes the issue by updating propagate_entity_load_avg() to
>> always propagate to the parent's runnable_load_avg. Combined with the
>> previous patch, this keeps a cfs_rq's runnable_load_avg always the sum
>> of the scaled loads of all tasks queued below removing the artifacts
>> from nesting cfs_rqs. The following is from inside three levels of
>> nesting with the patch applied.
>
> So you are changing the purpose of propagate_entity_load_avg which
> aims to propagate load_avg/util_avg changes only when a task migrate
> and you also want to propagate the enqueue/dequeue in the parent
> cfs_rq->runnable_load_avg
In fact you want that sched_entity load_avg reflects
cfs_rq->runnable_load_avg and not cfs_rq->avg.load_avg
>
>>
>> # ~/schbench -m 2 -t 16 -s 10000 -c 15000 -r 30
>> Latency percentiles (usec)
>> 50.0000th: 40
>> 75.0000th: 71
>> 90.0000th: 89
>> 95.0000th: 108
>> *99.0000th: 679
>> 99.5000th: 3500
>> 99.9000th: 10960
>> min=0, max=13790
>>
>> [1] git://git.kernel.org/pub/scm/linux/kernel/git/mason/schbench.git
>>
>> Signed-off-by: Tejun Heo <tj@kernel.org>
>> Cc: Vincent Guittot <vincent.guittot@linaro.org>
>> Cc: Ingo Molnar <mingo@redhat.com>
>> Cc: Peter Zijlstra <peterz@infradead.org>
>> Cc: Mike Galbraith <efault@gmx.de>
>> Cc: Paul Turner <pjt@google.com>
>> Cc: Chris Mason <clm@fb.com>
>> ---
>> kernel/sched/fair.c | 34 +++++++++++++++++++++-------------
>> 1 file changed, 21 insertions(+), 13 deletions(-)
>>
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -3075,7 +3075,8 @@ update_tg_cfs_util(struct cfs_rq *cfs_rq
>>
>> /* Take into account change of load of a child task group */
>> static inline void
>> -update_tg_cfs_load(struct cfs_rq *cfs_rq, struct sched_entity *se)
>> +update_tg_cfs_load(struct cfs_rq *cfs_rq, struct sched_entity *se,
>> + bool propagate_avg)
>> {
>> struct cfs_rq *gcfs_rq = group_cfs_rq(se);
>> long load = 0, delta;
>> @@ -3113,9 +3114,11 @@ update_tg_cfs_load(struct cfs_rq *cfs_rq
>> se->avg.load_avg = load;
>> se->avg.load_sum = se->avg.load_avg * LOAD_AVG_MAX;
>>
>> - /* Update parent cfs_rq load */
>> - add_positive(&cfs_rq->avg.load_avg, delta);
>> - cfs_rq->avg.load_sum = cfs_rq->avg.load_avg * LOAD_AVG_MAX;
>> + if (propagate_avg) {
>> + /* Update parent cfs_rq load */
>> + add_positive(&cfs_rq->avg.load_avg, delta);
>> + cfs_rq->avg.load_sum = cfs_rq->avg.load_avg * LOAD_AVG_MAX;
>> + }
>>
>> /*
>> * If the sched_entity is already enqueued, we also have to update the
>> @@ -3147,22 +3150,27 @@ static inline int test_and_clear_tg_cfs_
>> /* Update task and its cfs_rq load average */
>> static inline int propagate_entity_load_avg(struct sched_entity *se)
>> {
>> - struct cfs_rq *cfs_rq;
>> + struct cfs_rq *cfs_rq = cfs_rq_of(se);
>> + bool propagate_avg;
>>
>> if (entity_is_task(se))
>> return 0;
>>
>> - if (!test_and_clear_tg_cfs_propagate(se))
>> - return 0;
>> -
>> - cfs_rq = cfs_rq_of(se);
>> + propagate_avg = test_and_clear_tg_cfs_propagate(se);
>>
>> - set_tg_cfs_propagate(cfs_rq);
>> + /*
>> + * We want to keep @cfs_rq->runnable_load_avg always in sync so
>> + * that the load balancer can accurately determine the busiest CPU
>> + * regardless of cfs_rq nesting.
>> + */
>> + update_tg_cfs_load(cfs_rq, se, propagate_avg);
>>
>> - update_tg_cfs_util(cfs_rq, se);
>> - update_tg_cfs_load(cfs_rq, se);
>> + if (propagate_avg) {
>> + set_tg_cfs_propagate(cfs_rq);
>> + update_tg_cfs_util(cfs_rq, se);
>> + }
>>
>> - return 1;
>> + return propagate_avg;
>> }
>>
>> #else /* CONFIG_FAIR_GROUP_SCHED */
[toc] | [prev] | [next] | [standalone]
| From | Vincent Guittot <vincent.guittot@linaro.org> |
|---|---|
| Date | 2017-04-25 15:00 +0200 |
| Message-ID | <tA8et-4Pu-3@gated-at.bofh.it> |
| In reply to | #1630322 |
On 25 April 2017 at 11:05, Vincent Guittot <vincent.guittot@linaro.org> wrote:
> On 25 April 2017 at 10:46, Vincent Guittot <vincent.guittot@linaro.org> wrote:
>> On 24 April 2017 at 22:14, Tejun Heo <tj@kernel.org> wrote:
>>> We noticed that with cgroup CPU controller in use, the scheduling
>>> latency gets wonky regardless of nesting level or weight
>>> configuration. This is easily reproducible with Chris Mason's
>>> schbench[1].
>>>
>>> All tests are run on a single socket, 16 cores, 32 threads machine.
>>> While the machine is mostly idle, it isn't completey. There's always
>>> some variable management load going on. The command used is
>>>
>>> schbench -m 2 -t 16 -s 10000 -c 15000 -r 30
>>>
>>> which measures scheduling latency with 32 threads each of which
>>> repeatedly runs for 15ms and then sleeps for 10ms. Here's a
>>> representative result when running from the root cgroup.
>>>
>>> # ~/schbench -m 2 -t 16 -s 10000 -c 15000 -r 30
>>> Latency percentiles (usec)
>>> 50.0000th: 26
>>> 75.0000th: 62
>>> 90.0000th: 74
>>> 95.0000th: 86
>>> *99.0000th: 887
>>> 99.5000th: 3692
>>> 99.9000th: 10832
>>> min=0, max=13374
>>>
>>> The following is inside a first level CPU cgroup with the maximum
>>> weight.
>>>
>>> # ~/schbench -m 2 -t 16 -s 10000 -c 15000 -r 30
>>> Latency percentiles (usec)
>>> 50.0000th: 31
>>> 75.0000th: 65
>>> 90.0000th: 71
>>> 95.0000th: 91
>>> *99.0000th: 7288
>>> 99.5000th: 10352
>>> 99.9000th: 12496
>>> min=0, max=13023
>>>
>>> Note the drastic increase in p99 scheduling latency. After
>>> investigation, it turned out that the update_sd_lb_stats(), which is
>>> used by load_balance() to pick the most loaded group, was often
>>> picking the wrong group. A CPU which has one schbench running and
>>> another queued wouldn't report the correspondingly higher
>>> weighted_cpuload() and get looked over as the target of load
>>> balancing.
>>>
>>> weighted_cpuload() is the root cfs_rq's runnable_load_avg which is the
>>> sum of the load_avg of all queued sched_entities. Without cgroups or
>>> at the root cgroup, each task's load_avg contributes directly to the
>>> sum. When a task wakes up or goes to sleep, the change is immediately
>>> reflected on runnable_load_avg which in turn affects load balancing.
>>>
>>> However, when CPU cgroup is in use, a nesting cfs_rq blocks this
>>> immediate reflection. When a task wakes up inside a cgroup, the
>>> nested cfs_rq's runnable_load_avg is updated but doesn't get
>>> propagated to the parent. It only affects the matching sched_entity's
>>> load_avg over time which then gets propagated to the runnable_load_avg
>>> at that level. This makes weighted_cpuload() often temporarily out of
>>> sync leading to suboptimal behavior of load_balance() and increase in
>>> scheduling latencies as shown above.
>>>
>>> This patch fixes the issue by updating propagate_entity_load_avg() to
>>> always propagate to the parent's runnable_load_avg. Combined with the
>>> previous patch, this keeps a cfs_rq's runnable_load_avg always the sum
>>> of the scaled loads of all tasks queued below removing the artifacts
>>> from nesting cfs_rqs. The following is from inside three levels of
>>> nesting with the patch applied.
>>
>> So you are changing the purpose of propagate_entity_load_avg which
>> aims to propagate load_avg/util_avg changes only when a task migrate
>> and you also want to propagate the enqueue/dequeue in the parent
>> cfs_rq->runnable_load_avg
>
> In fact you want that sched_entity load_avg reflects
> cfs_rq->runnable_load_avg and not cfs_rq->avg.load_avg
I have run a quick test with your patches and schbench on my platform.
I haven't been able to reproduce your regression but my platform is
quite different from yours (only 8 cores without SMT)
But most importantly, the parent cfs_rq->runnable_load_avg never
reaches 0 (or almost 0) when it is idle. Instead, it still has a
runnable_load_avg (this is not due to rounding computation) whereas
runnable_load_avg should be 0
Just to be curious, Is your regression still there if you disable
SMT/hyperthreading on your paltform?
Regards,
Vincent
>
>>
>>>
>>> # ~/schbench -m 2 -t 16 -s 10000 -c 15000 -r 30
>>> Latency percentiles (usec)
>>> 50.0000th: 40
>>> 75.0000th: 71
>>> 90.0000th: 89
>>> 95.0000th: 108
>>> *99.0000th: 679
>>> 99.5000th: 3500
>>> 99.9000th: 10960
>>> min=0, max=13790
>>>
>>> [1] git://git.kernel.org/pub/scm/linux/kernel/git/mason/schbench.git
>>>
>>> Signed-off-by: Tejun Heo <tj@kernel.org>
>>> Cc: Vincent Guittot <vincent.guittot@linaro.org>
>>> Cc: Ingo Molnar <mingo@redhat.com>
>>> Cc: Peter Zijlstra <peterz@infradead.org>
>>> Cc: Mike Galbraith <efault@gmx.de>
>>> Cc: Paul Turner <pjt@google.com>
>>> Cc: Chris Mason <clm@fb.com>
>>> ---
>>> kernel/sched/fair.c | 34 +++++++++++++++++++++-------------
>>> 1 file changed, 21 insertions(+), 13 deletions(-)
>>>
>>> --- a/kernel/sched/fair.c
>>> +++ b/kernel/sched/fair.c
>>> @@ -3075,7 +3075,8 @@ update_tg_cfs_util(struct cfs_rq *cfs_rq
>>>
>>> /* Take into account change of load of a child task group */
>>> static inline void
>>> -update_tg_cfs_load(struct cfs_rq *cfs_rq, struct sched_entity *se)
>>> +update_tg_cfs_load(struct cfs_rq *cfs_rq, struct sched_entity *se,
>>> + bool propagate_avg)
>>> {
>>> struct cfs_rq *gcfs_rq = group_cfs_rq(se);
>>> long load = 0, delta;
>>> @@ -3113,9 +3114,11 @@ update_tg_cfs_load(struct cfs_rq *cfs_rq
>>> se->avg.load_avg = load;
>>> se->avg.load_sum = se->avg.load_avg * LOAD_AVG_MAX;
>>>
>>> - /* Update parent cfs_rq load */
>>> - add_positive(&cfs_rq->avg.load_avg, delta);
>>> - cfs_rq->avg.load_sum = cfs_rq->avg.load_avg * LOAD_AVG_MAX;
>>> + if (propagate_avg) {
>>> + /* Update parent cfs_rq load */
>>> + add_positive(&cfs_rq->avg.load_avg, delta);
>>> + cfs_rq->avg.load_sum = cfs_rq->avg.load_avg * LOAD_AVG_MAX;
>>> + }
>>>
>>> /*
>>> * If the sched_entity is already enqueued, we also have to update the
>>> @@ -3147,22 +3150,27 @@ static inline int test_and_clear_tg_cfs_
>>> /* Update task and its cfs_rq load average */
>>> static inline int propagate_entity_load_avg(struct sched_entity *se)
>>> {
>>> - struct cfs_rq *cfs_rq;
>>> + struct cfs_rq *cfs_rq = cfs_rq_of(se);
>>> + bool propagate_avg;
>>>
>>> if (entity_is_task(se))
>>> return 0;
>>>
>>> - if (!test_and_clear_tg_cfs_propagate(se))
>>> - return 0;
>>> -
>>> - cfs_rq = cfs_rq_of(se);
>>> + propagate_avg = test_and_clear_tg_cfs_propagate(se);
>>>
>>> - set_tg_cfs_propagate(cfs_rq);
>>> + /*
>>> + * We want to keep @cfs_rq->runnable_load_avg always in sync so
>>> + * that the load balancer can accurately determine the busiest CPU
>>> + * regardless of cfs_rq nesting.
>>> + */
>>> + update_tg_cfs_load(cfs_rq, se, propagate_avg);
>>>
>>> - update_tg_cfs_util(cfs_rq, se);
>>> - update_tg_cfs_load(cfs_rq, se);
>>> + if (propagate_avg) {
>>> + set_tg_cfs_propagate(cfs_rq);
>>> + update_tg_cfs_util(cfs_rq, se);
>>> + }
>>>
>>> - return 1;
>>> + return propagate_avg;
>>> }
>>>
>>> #else /* CONFIG_FAIR_GROUP_SCHED */
[toc] | [prev] | [next] | [standalone]
| From | Tejun Heo <tj@kernel.org> |
|---|---|
| Date | 2017-04-25 21:00 +0200 |
| Message-ID | <tAdQR-8r1-3@gated-at.bofh.it> |
| In reply to | #1630474 |
Hello, On Tue, Apr 25, 2017 at 02:59:18PM +0200, Vincent Guittot wrote: > >> So you are changing the purpose of propagate_entity_load_avg which > >> aims to propagate load_avg/util_avg changes only when a task migrate > >> and you also want to propagate the enqueue/dequeue in the parent > >> cfs_rq->runnable_load_avg Yeah, it always propagates runnable_load_avg and load_avg/util_avg too on migrations. > > In fact you want that sched_entity load_avg reflects > > cfs_rq->runnable_load_avg and not cfs_rq->avg.load_avg Yes, that's how it gets changed. The load balancer assumes that the root's runnable_load_avg is the total sum of all currently active tasks. Nesting cfs_rq's shouldn't change that and how it should be mapped is clearly defined (scaled recursively till it reaches the root), which is what the code calculates. The change in cfs_rq->avg.load_avg's behavior is to reflect that immediate propagation as load_avg and runnable_load_avg are tightly coupled. While it does change a nested cfs_rq's load_avg behavior. It sheds of the extra layer of averaging and directly reflects the scaled load avgs of its members, which are already time averaged. I could have missed something but couldn't spot anything which can break from this. > I have run a quick test with your patches and schbench on my platform. > I haven't been able to reproduce your regression but my platform is > quite different from yours (only 8 cores without SMT) > But most importantly, the parent cfs_rq->runnable_load_avg never > reaches 0 (or almost 0) when it is idle. Instead, it still has a > runnable_load_avg (this is not due to rounding computation) whereas > runnable_load_avg should be 0 Heh, let me try that out. Probably a silly mistake somewhere. > Just to be curious, Is your regression still there if you disable > SMT/hyperthreading on your paltform? Will try that too. I can't see why HT would change it because I see single CPU queues misevaluated. Just in case, you need to tune the test params so that it doesn't load the machine too much and that there are some non-CPU intensive workloads going on to purturb things a bit. Anyways, I'm gonna try disabling HT. Thanks. -- tejun
[toc] | [prev] | [next] | [standalone]
| From | Tejun Heo <tj@kernel.org> |
|---|---|
| Date | 2017-04-25 22:50 +0200 |
| Message-ID | <tAfzk-14W-37@gated-at.bofh.it> |
| In reply to | #1630911 |
On Tue, Apr 25, 2017 at 11:49:41AM -0700, Tejun Heo wrote: > Will try that too. I can't see why HT would change it because I see > single CPU queues misevaluated. Just in case, you need to tune the > test params so that it doesn't load the machine too much and that > there are some non-CPU intensive workloads going on to purturb things > a bit. Anyways, I'm gonna try disabling HT. It's finickier but after changing the duty cycle a bit, it reproduces w/ HT off. I think the trick is setting the number of threads to the number of logical CPUs and tune -s/-c so that p99 starts climbing up. The following is from the root cgroup. ~/schbench -m 2 -t 8 -s 15000 -c 10000 -r 30 Latency percentiles (usec) 50.0000th: 51 75.0000th: 62 90.0000th: 67 95.0000th: 70 *99.0000th: 1482 99.5000th: 5048 99.9000th: 9008 min=0, max=10066 And the following is from a first-level cgroup with maximum CPU weight. # ~/schbench -m 2 -t 8 -s 15000 -c 10000 -r 30 Latency percentiles (usec) 50.0000th: 51 75.0000th: 62 90.0000th: 71 95.0000th: 84 *99.0000th: 10064 99.5000th: 10064 99.9000th: 10064 min=0, max=10089 It's interesting that p99 ends up aligned on the CPU burn duration. It looks like some threads end up wating for full durations. The following is with the patches applied in the same cgroup setup. # ~/schbench -m 2 -t 8 -s 15000 -c 10000 -r 30 Latency percentiles (usec) 50.0000th: 64 75.0000th: 73 90.0000th: 102 95.0000th: 111 *99.0000th: 1954 99.5000th: 5432 99.9000th: 9520 min=0, max=10012 The numbers fluctuate quite a bit between runs but the pattern is still very clear - e.g. 10ms p99 never shows up in the root cgroup or on the patched kernel. Thanks. -- tejun
[toc] | [prev] | [next] | [standalone]
| From | Tejun Heo <tj@kernel.org> |
|---|---|
| Date | 2017-04-25 23:10 +0200 |
| Message-ID | <tAfSG-1qD-41@gated-at.bofh.it> |
| In reply to | #1630911 |
On Tue, Apr 25, 2017 at 11:49:41AM -0700, Tejun Heo wrote: > > I have run a quick test with your patches and schbench on my platform. > > I haven't been able to reproduce your regression but my platform is > > quite different from yours (only 8 cores without SMT) > > But most importantly, the parent cfs_rq->runnable_load_avg never > > reaches 0 (or almost 0) when it is idle. Instead, it still has a > > runnable_load_avg (this is not due to rounding computation) whereas > > runnable_load_avg should be 0 > > Heh, let me try that out. Probably a silly mistake somewhere. This is from the follow-up patch. I was confused. Because we don't propagate decays, we still should decay the runnable_load_avg; otherwise, we end up accumulating errors in the counter. I'll drop the last patch. Thanks. -- tejun
[toc] | [prev] | [next] | [standalone]
| From | Vincent Guittot <vincent.guittot@linaro.org> |
|---|---|
| Date | 2017-04-26 12:30 +0200 |
| Message-ID | <tAsmS-Y5-23@gated-at.bofh.it> |
| In reply to | #1631000 |
On 25 April 2017 at 23:08, Tejun Heo <tj@kernel.org> wrote: > On Tue, Apr 25, 2017 at 11:49:41AM -0700, Tejun Heo wrote: >> > I have run a quick test with your patches and schbench on my platform. >> > I haven't been able to reproduce your regression but my platform is >> > quite different from yours (only 8 cores without SMT) >> > But most importantly, the parent cfs_rq->runnable_load_avg never >> > reaches 0 (or almost 0) when it is idle. Instead, it still has a >> > runnable_load_avg (this is not due to rounding computation) whereas >> > runnable_load_avg should be 0 >> >> Heh, let me try that out. Probably a silly mistake somewhere. > > This is from the follow-up patch. I was confused. Because we don't > propagate decays, we still should decay the runnable_load_avg; > otherwise, we end up accumulating errors in the counter. I'll drop > the last patch. Ok, the runnable_load_avg goes back to 0 when I drop patch 3. But i see runnable_load_avg sometimes significantly higher than load_avg which is normally not possible as load_avg = runnable_load_avg + sleeping task's load_avg Then, I just have the opposite behavior on my platform. I see a increase of latency at p99 with your patches. My platform is a hikey : 2x4 cores ARM and I have used schbench -m 2 -t 4 -s 10000 -c 15000 -r 30 so I have 1 worker thread per CPU which is similar to what you are doing on your platform With v4.11-rc8. I have run 10 times the test and get consistent results schbench -m 2 -t 4 -s 10000 -c 15000 -r 30 Latency percentiles (usec) 50.0000th: 255 75.0000th: 350 90.0000th: 454 95.0000th: 489 *99.0000th: 539 99.5000th: 585 99.9000th: 10224 min=0, max=13567 With your patches i see an increase of the latency for p99. I run 10 times the test too and half tests show latency increase like below schbench$ ./schbench -m 2 -t 4 -s 10000 -c 15000 -r 30 Latency percentiles (usec) 50.0000th: 216 75.0000th: 295 90.0000th: 395 95.0000th: 444 *99.0000th: 2034 99.5000th: 5960 99.9000th: 12240 min=0, max=14744 > > Thanks. > > -- > tejun
[toc] | [prev] | [next] | [standalone]
| From | Tejun Heo <tj@kernel.org> |
|---|---|
| Date | 2017-04-27 02:40 +0200 |
| Message-ID | <tAFDr-1lq-11@gated-at.bofh.it> |
| In reply to | #1631359 |
Hello, Vincent. On Wed, Apr 26, 2017 at 12:21:52PM +0200, Vincent Guittot wrote: > > This is from the follow-up patch. I was confused. Because we don't > > propagate decays, we still should decay the runnable_load_avg; > > otherwise, we end up accumulating errors in the counter. I'll drop > > the last patch. > > Ok, the runnable_load_avg goes back to 0 when I drop patch 3. But i > see runnable_load_avg sometimes significantly higher than load_avg > which is normally not possible as load_avg = runnable_load_avg + > sleeping task's load_avg So, while load_avg would eventually converge on runnable_load_avg + blocked load_avg given stable enough workload for long enough, runnable_load_avg jumping above load avg temporarily is expected, AFAICS. That's the whole point of it, a sum closely tracking what's currently on the cpu so that we can pick the cpu which has the most on it now. It doesn't make sense to try to pick threads off of a cpu which is generally loaded but doesn't have much going on right now, after all. > Then, I just have the opposite behavior on my platform. I see a > increase of latency at p99 with your patches. > My platform is a hikey : 2x4 cores ARM and I have used schbench -m 2 > -t 4 -s 10000 -c 15000 -r 30 so I have 1 worker thread per CPU which > is similar to what you are doing on your platform > > With v4.11-rc8. I have run 10 times the test and get consistent results ... > *99.0000th: 539 ... > With your patches i see an increase of the latency for p99. I run 10 > *99.0000th: 2034 I see. This is surprising given that at least the purpose of the patch is restoring cgroup behavior to match !cgroup one. I could have totally messed it up tho. Hmm... there are several ways forward I guess. * Can you please double check that the higher latencies w/ the patch is reliably reproducible? The test machines that I use have variable management load. They never dominate the machine but are enough to disturb the results so that to drawing out a reliable pattern takes a lot of repeated runs. I'd really appreciate if you could double check that the pattern is reliable with different run patterns (ie. instead of 10 consecutive runs after another, interleaved). * Is the board something easily obtainable? It'd be the eaisest for me to set up the same environment and reproduce the problem. I looked up hikey boards on amazon but couldn't easily find 2x4 core ones. If there's something I can easily buy, please point me to it. If there's something I can loan, that'd be great too. * If not, I'll try to clean up the debug patches I have and send them your way to get more visiblity but given these things tend to be very iterative, it might take quite a few back and forth. Thanks! -- tejun
[toc] | [prev] | [next] | [standalone]
| From | Vincent Guittot <vincent.guittot@linaro.org> |
|---|---|
| Date | 2017-04-27 10:30 +0200 |
| Message-ID | <tAMYi-6lN-27@gated-at.bofh.it> |
| In reply to | #1631806 |
On 27 April 2017 at 02:30, Tejun Heo <tj@kernel.org> wrote:
> Hello, Vincent.
>
> On Wed, Apr 26, 2017 at 12:21:52PM +0200, Vincent Guittot wrote:
>> > This is from the follow-up patch. I was confused. Because we don't
>> > propagate decays, we still should decay the runnable_load_avg;
>> > otherwise, we end up accumulating errors in the counter. I'll drop
>> > the last patch.
>>
>> Ok, the runnable_load_avg goes back to 0 when I drop patch 3. But i
>> see runnable_load_avg sometimes significantly higher than load_avg
>> which is normally not possible as load_avg = runnable_load_avg +
>> sleeping task's load_avg
>
> So, while load_avg would eventually converge on runnable_load_avg +
> blocked load_avg given stable enough workload for long enough,
> runnable_load_avg jumping above load avg temporarily is expected,
No, it's not. Look at load_avg/runnable_avg at root domain when only
task are involved, runnable_load_avg will never be higher than
load_avg because
load_avg = /sum load_avg of tasks attached to the cfs_rq
runnable_load_avg = /sum load_avg of tasks attached and enqueued
to the cfs_rq
load_avg = runnable_load_avg + blocked tasks and as a result
runnable_load_avg is always lower than load_avg.
And with the propagate load/util_avg patchset, we can even reflect
task migration directly at root domain whereas we had to wait for the
util/load_avg and runnable_load_avg to converge to the new value
before
Just to confirm one of my assumption, the latency regression was
already there in previous kernel versions and is not a result of
propagate load/util_avg patchset, isn't it ?
> AFAICS. That's the whole point of it, a sum closely tracking what's
> currently on the cpu so that we can pick the cpu which has the most on
> it now. It doesn't make sense to try to pick threads off of a cpu
> which is generally loaded but doesn't have much going on right now,
> after all.
The only interest of runnable_load_avg is to be null when a cfs_rq is
idle whereas load_avg is not but not to be higher than load_avg. The
root cause is that load_balance only looks at "load" but not number of
task currently running and that's probably the main problem:
runnable_load_avg has been added because load_balance fails to filter
idle group and idle rq. We should better add a new type in
group_classify to tag group that are idle and the same in
find_busiest_queue more.
>
>> Then, I just have the opposite behavior on my platform. I see a
>> increase of latency at p99 with your patches.
>> My platform is a hikey : 2x4 cores ARM and I have used schbench -m 2
>> -t 4 -s 10000 -c 15000 -r 30 so I have 1 worker thread per CPU which
>> is similar to what you are doing on your platform
>>
>> With v4.11-rc8. I have run 10 times the test and get consistent results
> ...
>> *99.0000th: 539
> ...
>> With your patches i see an increase of the latency for p99. I run 10
>> *99.0000th: 2034
>
> I see. This is surprising given that at least the purpose of the
> patch is restoring cgroup behavior to match !cgroup one. I could have
> totally messed it up tho. Hmm... there are several ways forward I
> guess.
>
> * Can you please double check that the higher latencies w/ the patch
> is reliably reproducible? The test machines that I use have
> variable management load. They never dominate the machine but are
> enough to disturb the results so that to drawing out a reliable
> pattern takes a lot of repeated runs. I'd really appreciate if you
> could double check that the pattern is reliable with different run
> patterns (ie. instead of 10 consecutive runs after another,
> interleaved).
I always let time between 2 consecutive run and the 10 consecutive
runs take around 2min to execute
Then I have run several time these 10 consecutive test and results stay the same
>
> * Is the board something easily obtainable? It'd be the eaisest for
> me to set up the same environment and reproduce the problem. I
> looked up hikey boards on amazon but couldn't easily find 2x4 core
It is often named hikey octo cores but I use 2x4 cores just to point
out that there are 2 clusters which is important for scheduler
topology and behavior
> ones. If there's something I can easily buy, please point me to it.
> If there's something I can loan, that'd be great too.
It looks like most of web site are currently out of stock
>
> * If not, I'll try to clean up the debug patches I have and send them
> your way to get more visiblity but given these things tend to be
> very iterative, it might take quite a few back and forth.
Yes, that could be usefull. Even a trace of regression could be useful
I can also push on my git tree the debug patch that i use for tracking
load metrics if you want. It's ugly but it does the job
Thanks
>
> Thanks!
>
> --
> tejun
[toc] | [prev] | [next] | [standalone]
| From | Tejun Heo <tj@kernel.org> |
|---|---|
| Date | 2017-04-28 18:20 +0200 |
| Message-ID | <tBgMF-1o8-9@gated-at.bofh.it> |
| In reply to | #1631958 |
Hello, Vincent.
On Thu, Apr 27, 2017 at 10:28:01AM +0200, Vincent Guittot wrote:
> On 27 April 2017 at 02:30, Tejun Heo <tj@kernel.org> wrote:
> > Hello, Vincent.
> >
> > On Wed, Apr 26, 2017 at 12:21:52PM +0200, Vincent Guittot wrote:
> >> > This is from the follow-up patch. I was confused. Because we don't
> >> > propagate decays, we still should decay the runnable_load_avg;
> >> > otherwise, we end up accumulating errors in the counter. I'll drop
> >> > the last patch.
> >>
> >> Ok, the runnable_load_avg goes back to 0 when I drop patch 3. But i
> >> see runnable_load_avg sometimes significantly higher than load_avg
> >> which is normally not possible as load_avg = runnable_load_avg +
> >> sleeping task's load_avg
> >
> > So, while load_avg would eventually converge on runnable_load_avg +
> > blocked load_avg given stable enough workload for long enough,
> > runnable_load_avg jumping above load avg temporarily is expected,
>
> No, it's not. Look at load_avg/runnable_avg at root domain when only
> task are involved, runnable_load_avg will never be higher than
> load_avg because
> load_avg = /sum load_avg of tasks attached to the cfs_rq
> runnable_load_avg = /sum load_avg of tasks attached and enqueued
> to the cfs_rq
> load_avg = runnable_load_avg + blocked tasks and as a result
> runnable_load_avg is always lower than load_avg.
We don't actually calculate that tho, but yeah, given the calculations
we do, runnable shouldn't be significantly higher than load_avg. I
see runable going above load_avg by a bit often but the margins are
small. I wonder whether you're seeing the errors accumulating from
the incorrect min() capping.
> And with the propagate load/util_avg patchset, we can even reflect
> task migration directly at root domain whereas we had to wait for the
> util/load_avg and runnable_load_avg to converge to the new value
> before
>
> Just to confirm one of my assumption, the latency regression was
> already there in previous kernel versions and is not a result of
> propagate load/util_avg patchset, isn't it ?
Yeah, the latency problem is independent of the propagation logic.
It's really the meaning of the top level running_avg_load being
different w/ and w/o cgroups.
> > AFAICS. That's the whole point of it, a sum closely tracking what's
> > currently on the cpu so that we can pick the cpu which has the most on
> > it now. It doesn't make sense to try to pick threads off of a cpu
> > which is generally loaded but doesn't have much going on right now,
> > after all.
>
> The only interest of runnable_load_avg is to be null when a cfs_rq is
> idle whereas load_avg is not but not to be higher than load_avg. The
> root cause is that load_balance only looks at "load" but not number of
> task currently running and that's probably the main problem:
> runnable_load_avg has been added because load_balance fails to filter
> idle group and idle rq. We should better add a new type in
> group_classify to tag group that are idle and the same in
> find_busiest_queue more.
I'll follow up in the other subthread but there really is fundamental
difference in how we calculate runnable_avg w/ and w/o cgroups.
Indepndent of whether we can improve the load balancer further, it is
an existing bug.
> > * Can you please double check that the higher latencies w/ the patch
> > is reliably reproducible? The test machines that I use have
> > variable management load. They never dominate the machine but are
> > enough to disturb the results so that to drawing out a reliable
> > pattern takes a lot of repeated runs. I'd really appreciate if you
> > could double check that the pattern is reliable with different run
> > patterns (ie. instead of 10 consecutive runs after another,
> > interleaved).
>
> I always let time between 2 consecutive run and the 10 consecutive
> runs take around 2min to execute
>
> Then I have run several time these 10 consecutive test and results stay the same
Can you please try the patch at the end of this message? I'm
wondering whether you're seeing the errors accumulating from the wrong
min().
> > ones. If there's something I can easily buy, please point me to it.
> > If there's something I can loan, that'd be great too.
>
> It looks like most of web site are currently out of stock
:(
> > * If not, I'll try to clean up the debug patches I have and send them
> > your way to get more visiblity but given these things tend to be
> > very iterative, it might take quite a few back and forth.
>
> Yes, that could be usefull. Even a trace of regression could be useful
Yeah, will clean it up a bit and post. Will also post some traces.
And here's the updated patch. I didn't add the suggested
scale_down()'s. I'll follow up on that in the other subthread.
Thanks.
kernel/sched/fair.c | 47 ++++++++++++++++++++---------------------------
1 file changed, 20 insertions(+), 27 deletions(-)
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -3078,37 +3078,30 @@ static inline void
update_tg_cfs_load(struct cfs_rq *cfs_rq, struct sched_entity *se)
{
struct cfs_rq *gcfs_rq = group_cfs_rq(se);
- long delta, load = gcfs_rq->avg.load_avg;
+ long load = 0, delta;
/*
- * If the load of group cfs_rq is null, the load of the
- * sched_entity will also be null so we can skip the formula
+ * A cfs_rq's load avg contribution to the parent should be scaled
+ * to the sched_entity's weight. Use freshly calculated shares
+ * instead of @se->load.weight as the latter may not reflect
+ * changes from the current scheduling operation.
+ *
+ * Note that the propagation source is runnable_load_avg instead of
+ * load_avg. This keeps every cfs_rq's runnable_load_avg true to
+ * the sum of the scaled loads of all tasks queued under it, which
+ * is important for the correct operation of the load balancer.
+ *
+ * This can make the sched_entity's load_avg jumpier but that
+ * correctly reflects what would happen without cgroups if each
+ * task's load is scaled across nesting - the load is being
+ * averaged at the task and each cfs_rq.
*/
- if (load) {
- long tg_load;
+ if (gcfs_rq->load.weight) {
+ long shares = calc_cfs_shares(gcfs_rq, gcfs_rq->tg);
- /* Get tg's load and ensure tg_load > 0 */
- tg_load = atomic_long_read(&gcfs_rq->tg->load_avg) + 1;
-
- /* Ensure tg_load >= load and updated with current load*/
- tg_load -= gcfs_rq->tg_load_avg_contrib;
- tg_load += load;
-
- /*
- * We need to compute a correction term in the case that the
- * task group is consuming more CPU than a task of equal
- * weight. A task with a weight equals to tg->shares will have
- * a load less or equal to scale_load_down(tg->shares).
- * Similarly, the sched_entities that represent the task group
- * at parent level, can't have a load higher than
- * scale_load_down(tg->shares). And the Sum of sched_entities'
- * load must be <= scale_load_down(tg->shares).
- */
- if (tg_load > scale_load_down(gcfs_rq->tg->shares)) {
- /* scale gcfs_rq's load into tg's shares*/
- load *= scale_load_down(gcfs_rq->tg->shares);
- load /= tg_load;
- }
+ load = min_t(long, scale_load_down(shares),
+ gcfs_rq->runnable_load_avg *
+ shares / gcfs_rq->load.weight);
}
delta = load - se->avg.load_avg;
[toc] | [prev] | [next] | [standalone]
| From | Vincent Guittot <vincent.guittot@linaro.org> |
|---|---|
| Date | 2017-05-02 09:00 +0200 |
| Message-ID | <tCzWW-2WF-5@gated-at.bofh.it> |
| In reply to | #1632998 |
On 28 April 2017 at 18:14, Tejun Heo <tj@kernel.org> wrote:
> Hello, Vincent.
>
>>
>> The only interest of runnable_load_avg is to be null when a cfs_rq is
>> idle whereas load_avg is not but not to be higher than load_avg. The
>> root cause is that load_balance only looks at "load" but not number of
>> task currently running and that's probably the main problem:
>> runnable_load_avg has been added because load_balance fails to filter
>> idle group and idle rq. We should better add a new type in
>> group_classify to tag group that are idle and the same in
>> find_busiest_queue more.
>
> I'll follow up in the other subthread but there really is fundamental
> difference in how we calculate runnable_avg w/ and w/o cgroups.
> Indepndent of whether we can improve the load balancer further, it is
> an existing bug.
I'd like to weight that a bit.
The runnable_load_avg works correctly as it is because it reflects
correctly the load of runnable entity at root domain
If you start to propagate the runnable_load_avg on the load_avg of the
group entity, the load will become unstable.
runnable_load_avg has been added to fix load_balance being unable to
select the right busiest rq. So the goal is to use more and more
load_avg not the opposite
>
>> > * Can you please double check that the higher latencies w/ the patch
>> > is reliably reproducible? The test machines that I use have
>> > variable management load. They never dominate the machine but are
>> > enough to disturb the results so that to drawing out a reliable
>> > pattern takes a lot of repeated runs. I'd really appreciate if you
>> > could double check that the pattern is reliable with different run
>> > patterns (ie. instead of 10 consecutive runs after another,
>> > interleaved).
>>
>> I always let time between 2 consecutive run and the 10 consecutive
>> runs take around 2min to execute
>>
>> Then I have run several time these 10 consecutive test and results stay the same
>
> Can you please try the patch at the end of this message? I'm
> wondering whether you're seeing the errors accumulating from the wrong
> min().
I still have the regression with the patch below.
The regression comes from the use runnable_load_avg to propagate. As
load_avg becomes null at some point, it break the computation of share
and the load_avg stay very low
>
>> > ones. If there's something I can easily buy, please point me to it.
>> > If there's something I can loan, that'd be great too.
>>
>> It looks like most of web site are currently out of stock
>
> :(
>
>> > * If not, I'll try to clean up the debug patches I have and send them
>> > your way to get more visiblity but given these things tend to be
>> > very iterative, it might take quite a few back and forth.
>>
>> Yes, that could be usefull. Even a trace of regression could be useful
>
> Yeah, will clean it up a bit and post. Will also post some traces.
>
> And here's the updated patch. I didn't add the suggested
> scale_down()'s. I'll follow up on that in the other subthread.
> Thanks.
>
> kernel/sched/fair.c | 47 ++++++++++++++++++++---------------------------
> 1 file changed, 20 insertions(+), 27 deletions(-)
>
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -3078,37 +3078,30 @@ static inline void
> update_tg_cfs_load(struct cfs_rq *cfs_rq, struct sched_entity *se)
> {
> struct cfs_rq *gcfs_rq = group_cfs_rq(se);
> - long delta, load = gcfs_rq->avg.load_avg;
> + long load = 0, delta;
>
> /*
> - * If the load of group cfs_rq is null, the load of the
> - * sched_entity will also be null so we can skip the formula
> + * A cfs_rq's load avg contribution to the parent should be scaled
> + * to the sched_entity's weight. Use freshly calculated shares
> + * instead of @se->load.weight as the latter may not reflect
> + * changes from the current scheduling operation.
> + *
> + * Note that the propagation source is runnable_load_avg instead of
> + * load_avg. This keeps every cfs_rq's runnable_load_avg true to
> + * the sum of the scaled loads of all tasks queued under it, which
> + * is important for the correct operation of the load balancer.
> + *
> + * This can make the sched_entity's load_avg jumpier but that
> + * correctly reflects what would happen without cgroups if each
> + * task's load is scaled across nesting - the load is being
> + * averaged at the task and each cfs_rq.
> */
> - if (load) {
> - long tg_load;
> + if (gcfs_rq->load.weight) {
> + long shares = calc_cfs_shares(gcfs_rq, gcfs_rq->tg);
>
> - /* Get tg's load and ensure tg_load > 0 */
> - tg_load = atomic_long_read(&gcfs_rq->tg->load_avg) + 1;
> -
> - /* Ensure tg_load >= load and updated with current load*/
> - tg_load -= gcfs_rq->tg_load_avg_contrib;
> - tg_load += load;
> -
> - /*
> - * We need to compute a correction term in the case that the
> - * task group is consuming more CPU than a task of equal
> - * weight. A task with a weight equals to tg->shares will have
> - * a load less or equal to scale_load_down(tg->shares).
> - * Similarly, the sched_entities that represent the task group
> - * at parent level, can't have a load higher than
> - * scale_load_down(tg->shares). And the Sum of sched_entities'
> - * load must be <= scale_load_down(tg->shares).
> - */
> - if (tg_load > scale_load_down(gcfs_rq->tg->shares)) {
> - /* scale gcfs_rq's load into tg's shares*/
> - load *= scale_load_down(gcfs_rq->tg->shares);
> - load /= tg_load;
> - }
> + load = min_t(long, scale_load_down(shares),
> + gcfs_rq->runnable_load_avg *
> + shares / gcfs_rq->load.weight);
> }
>
> delta = load - se->avg.load_avg;
[toc] | [prev] | [next] | [standalone]
| From | Tejun Heo <tj@kernel.org> |
|---|---|
| Date | 2017-05-02 23:00 +0200 |
| Message-ID | <tCN3P-3du-5@gated-at.bofh.it> |
| In reply to | #1634173 |
Hello, Vincent. On Tue, May 02, 2017 at 08:56:52AM +0200, Vincent Guittot wrote: > On 28 April 2017 at 18:14, Tejun Heo <tj@kernel.org> wrote: > > I'll follow up in the other subthread but there really is fundamental > > difference in how we calculate runnable_avg w/ and w/o cgroups. > > Indepndent of whether we can improve the load balancer further, it is > > an existing bug. > > I'd like to weight that a bit. > The runnable_load_avg works correctly as it is because it reflects > correctly the load of runnable entity at root domain > If you start to propagate the runnable_load_avg on the load_avg of the > group entity, the load will become unstable. > runnable_load_avg has been added to fix load_balance being unable to > select the right busiest rq. So the goal is to use more and more > load_avg not the opposite I have a hard time understanding what you're trying to say here. Without cgroup, the load balancer uses the sum of load_avgs of the running tasks on the queue. As shown by the debug trace, the load balancer repeatedly ends up picking the wrong CPU when cgroup is involved because it ends up including the load_avgs of nested blocked tasks into runnable_load_avg of root - we lose the distinction between running and blocked load_avgs when we pass through a nested cfs_rq. We can further improve the load balancer all we want, for example, right now, we would end up picking a CPU with one task which has a really high weight over another CPU with two normal weight tasks even, which isn't ideal; however, there is something obviously broken in the existing mechanism and we want to fix that first independent of further improvements, and it won't be a good idea to paper over an existing problem with a different mechanism either. > >> I always let time between 2 consecutive run and the 10 consecutive > >> runs take around 2min to execute > >> > >> Then I have run several time these 10 consecutive test and results stay the same > > > > Can you please try the patch at the end of this message? I'm > > wondering whether you're seeing the errors accumulating from the wrong > > min(). > > I still have the regression with the patch below. > The regression comes from the use runnable_load_avg to propagate. As > load_avg becomes null at some point, it break the computation of share > and the load_avg stay very low That's surprising given that what the patch does is bringing the cgroup behavior closer to !cgroup behavior. It'd be great to be able to reproduce the problem and trace it. It looks like the board is pretty standardized. Would the following be equivalent to the one you have? http://a.co/f3dD1lm If so, I can just buy it, get your test image and repro it here and trace why the regression is happening with the setup. We might be hitting something else. Thanks. -- tejun
[toc] | [prev] | [next] | [standalone]
| From | Vincent Guittot <vincent.guittot@linaro.org> |
|---|---|
| Date | 2017-05-03 09:30 +0200 |
| Message-ID | <tCWTv-1Q1-7@gated-at.bofh.it> |
| In reply to | #1634621 |
On 2 May 2017 at 22:56, Tejun Heo <tj@kernel.org> wrote: > Hello, Vincent. > > On Tue, May 02, 2017 at 08:56:52AM +0200, Vincent Guittot wrote: >> On 28 April 2017 at 18:14, Tejun Heo <tj@kernel.org> wrote: >> > I'll follow up in the other subthread but there really is fundamental >> > difference in how we calculate runnable_avg w/ and w/o cgroups. >> > Indepndent of whether we can improve the load balancer further, it is >> > an existing bug. >> >> I'd like to weight that a bit. >> The runnable_load_avg works correctly as it is because it reflects >> correctly the load of runnable entity at root domain >> If you start to propagate the runnable_load_avg on the load_avg of the >> group entity, the load will become unstable. >> runnable_load_avg has been added to fix load_balance being unable to >> select the right busiest rq. So the goal is to use more and more >> load_avg not the opposite > > I have a hard time understanding what you're trying to say here. > > Without cgroup, the load balancer uses the sum of load_avgs of the > running tasks on the queue. As shown by the debug trace, the load > balancer repeatedly ends up picking the wrong CPU when cgroup is > involved because it ends up including the load_avgs of nested blocked > tasks into runnable_load_avg of root - we lose the distinction between > running and blocked load_avgs when we pass through a nested cfs_rq. > > We can further improve the load balancer all we want, for example, > right now, we would end up picking a CPU with one task which has a > really high weight over another CPU with two normal weight tasks even, > which isn't ideal; however, there is something obviously broken in the > existing mechanism and we want to fix that first independent of > further improvements, and it won't be a good idea to paper over an > existing problem with a different mechanism either. That's probably where I disagree. IMO, runnable_load_avg is already a fix to compensate the fact when PELT has been rewritten, the load balance has not been update accordingly and was unable to make the right choice between 2 groups: one group having only 1 task but a higher load and another group with several tasks but a lower load (either because of the blocked load or because a runnable task with far higher load than others). runnable_load_avg has been put back to quickly fix the 1st case but it doesn't fix the other. There were also some issues of propagating load update with task migration but this has been fixed now. So we have 2 use case with the exact same behavior a group with a higher load (runnable_load_avg or load_avg) but only 1 task and another one with several tasks but a lower load. At now runnable_load_avg can only fix one with the impact of breaking load_avg behavior whereas one fix in the load_balance can fix both withour breaking load_avg behavior. We should better work on removing runnable_load_avg completely than adding more stuff on it > >> >> I always let time between 2 consecutive run and the 10 consecutive >> >> runs take around 2min to execute >> >> >> >> Then I have run several time these 10 consecutive test and results stay the same >> > >> > Can you please try the patch at the end of this message? I'm >> > wondering whether you're seeing the errors accumulating from the wrong >> > min(). >> >> I still have the regression with the patch below. >> The regression comes from the use runnable_load_avg to propagate. As >> load_avg becomes null at some point, it break the computation of share >> and the load_avg stay very low > > That's surprising given that what the patch does is bringing the > cgroup behavior closer to !cgroup behavior. It'd be great to be able But it's also break load_avg which is used to calculate per cfs_rq share and this change generates the regression on my board > to reproduce the problem and trace it. It looks like the board is > pretty standardized. Would the following be equivalent to the one you > have? > > http://a.co/f3dD1lm Yes it's this board i have the previous version but it should not change the behavior > > If so, I can just buy it, get your test image and repro it here and > trace why the regression is happening with the setup. We might be > hitting something else. I have pushed my branch which includes v4.11-rc8 + your patches + some debug trace here: https://git.linaro.org/people/vincent.guittot/kernel.git/log/?h=test-tejun-patches and uploaded a trace_cmd's trace that shows the regression http://people.linaro.org/~vincent.guittot/trace-tejun-patches As an example, we can easily see arounf time stamp 94.826469 sec that cpu5 is idle while CPU2 and CPU3 share their time between several task. This doesn't happens without your patch > > Thanks. > > -- > tejun
[toc] | [prev] | [next] | [standalone]
| From | Vincent Guittot <vincent.guittot@linaro.org> |
|---|---|
| Date | 2017-05-03 10:00 +0200 |
| Message-ID | <tCXmx-23P-3@gated-at.bofh.it> |
| In reply to | #1634783 |
On 3 May 2017 at 09:25, Vincent Guittot <vincent.guittot@linaro.org> wrote: > On 2 May 2017 at 22:56, Tejun Heo <tj@kernel.org> wrote: >> Hello, Vincent. >> >> On Tue, May 02, 2017 at 08:56:52AM +0200, Vincent Guittot wrote: >>> On 28 April 2017 at 18:14, Tejun Heo <tj@kernel.org> wrote: >>> > I'll follow up in the other subthread but there really is fundamental >>> > difference in how we calculate runnable_avg w/ and w/o cgroups. >>> > Indepndent of whether we can improve the load balancer further, it is >>> > an existing bug. >>> >>> I'd like to weight that a bit. >>> The runnable_load_avg works correctly as it is because it reflects >>> correctly the load of runnable entity at root domain >>> If you start to propagate the runnable_load_avg on the load_avg of the >>> group entity, the load will become unstable. >>> runnable_load_avg has been added to fix load_balance being unable to >>> select the right busiest rq. So the goal is to use more and more >>> load_avg not the opposite >> >> I have a hard time understanding what you're trying to say here. >> >> Without cgroup, the load balancer uses the sum of load_avgs of the >> running tasks on the queue. As shown by the debug trace, the load >> balancer repeatedly ends up picking the wrong CPU when cgroup is >> involved because it ends up including the load_avgs of nested blocked >> tasks into runnable_load_avg of root - we lose the distinction between >> running and blocked load_avgs when we pass through a nested cfs_rq. >> >> We can further improve the load balancer all we want, for example, >> right now, we would end up picking a CPU with one task which has a >> really high weight over another CPU with two normal weight tasks even, >> which isn't ideal; however, there is something obviously broken in the >> existing mechanism and we want to fix that first independent of >> further improvements, and it won't be a good idea to paper over an >> existing problem with a different mechanism either. > > That's probably where I disagree. > IMO, runnable_load_avg is already a fix to compensate the fact when > PELT has been rewritten, the load balance has not been update > accordingly and was unable to make the right choice > between 2 groups: one group having only 1 task but a higher load and > another group with several tasks but a lower load (either because of > the blocked load or because a runnable task with far higher load than > others). runnable_load_avg has been put back to quickly fix the 1st > case but it doesn't fix the other. There were also some issues of > propagating load update with task migration but this has been fixed > now. > So we have 2 use case with the exact same behavior a group with a > higher load (runnable_load_avg or load_avg) but only 1 task and > another one with several tasks but a lower load. At now > runnable_load_avg can only fix one with the impact of breaking > load_avg behavior whereas one fix in the load_balance can fix both > withour breaking load_avg behavior. > We should better work on removing runnable_load_avg completely than > adding more stuff on it > >> >>> >> I always let time between 2 consecutive run and the 10 consecutive >>> >> runs take around 2min to execute >>> >> >>> >> Then I have run several time these 10 consecutive test and results stay the same >>> > >>> > Can you please try the patch at the end of this message? I'm >>> > wondering whether you're seeing the errors accumulating from the wrong >>> > min(). >>> >>> I still have the regression with the patch below. >>> The regression comes from the use runnable_load_avg to propagate. As >>> load_avg becomes null at some point, it break the computation of share >>> and the load_avg stay very low >> >> That's surprising given that what the patch does is bringing the >> cgroup behavior closer to !cgroup behavior. It'd be great to be able > > But it's also break load_avg which is used to calculate per cfs_rq > share and this change generates the regression on my board > >> to reproduce the problem and trace it. It looks like the board is >> pretty standardized. Would the following be equivalent to the one you >> have? >> >> http://a.co/f3dD1lm > > Yes it's this board i have the previous version but it should not > change the behavior >> >> If so, I can just buy it, get your test image and repro it here and >> trace why the regression is happening with the setup. We might be >> hitting something else. > > I have pushed my branch which includes v4.11-rc8 + your patches + some > debug trace here: > https://git.linaro.org/people/vincent.guittot/kernel.git/log/?h=test-tejun-patches > > and uploaded a trace_cmd's trace that shows the regression > http://people.linaro.org/~vincent.guittot/trace-tejun-patches I have also uploaded the trace withour your patches so you can compare both: http://people.linaro.org/~vincent.guittot/trace-v4.8-rc11 > > As an example, we can easily see arounf time stamp 94.826469 sec that > cpu5 is idle while CPU2 and CPU3 share their time between several > task. > This doesn't happens without your patch > >> >> Thanks. >> >> -- >> tejun
[toc] | [prev] | [next] | [standalone]
| From | Vincent Guittot <vincent.guittot@linaro.org> |
|---|---|
| Date | 2017-04-26 20:20 +0200 |
| Message-ID | <tAzHH-5SI-7@gated-at.bofh.it> |
| In reply to | #1629966 |
On 24 April 2017 at 22:14, Tejun Heo <tj@kernel.org> wrote: > We noticed that with cgroup CPU controller in use, the scheduling > > Note the drastic increase in p99 scheduling latency. After > investigation, it turned out that the update_sd_lb_stats(), which is > used by load_balance() to pick the most loaded group, was often > picking the wrong group. A CPU which has one schbench running and Can the problem be on the load balance side instead ? and more precisely in the wakeup path ? After looking at the trace, it seems that task placement happens at wake up path and if it fails to select the right idle cpu at wake up, you will have to wait for a load balance which is alreayd too late > another queued wouldn't report the correspondingly higher It will as load_avg includes the runnable_load_avg so whatever load is in runnable_load_avg will be in load_avg too. But at the contrary, runnable_load_avg will not have the blocked that is going to wake up soon in the case of schbench One last thing, the load_avg of an idle CPU can stay blocked for a while (until a load balance happens that will update blocked load) and can be seen has "busy" whereas it is not. Could it be a reason of your problem ? I have an ongoing patch to solve the problem at least partly if this can be a reason > weighted_cpuload() and get looked over as the target of load > balancing. > > weighted_cpuload() is the root cfs_rq's runnable_load_avg which is the > sum of the load_avg of all queued sched_entities. Without cgroups or > at the root cgroup, each task's load_avg contributes directly to the > sum. When a task wakes up or goes to sleep, the change is immediately > reflected on runnable_load_avg which in turn affects load balancing. > > #else /* CONFIG_FAIR_GROUP_SCHED */
[toc] | [prev] | [next] | [standalone]
| From | Tejun Heo <tj@kernel.org> |
|---|---|
| Date | 2017-04-27 01:00 +0200 |
| Message-ID | <tAE4G-mI-15@gated-at.bofh.it> |
| In reply to | #1631639 |
Hello, On Wed, Apr 26, 2017 at 08:12:09PM +0200, Vincent Guittot wrote: > On 24 April 2017 at 22:14, Tejun Heo <tj@kernel.org> wrote: > Can the problem be on the load balance side instead ? and more > precisely in the wakeup path ? > After looking at the trace, it seems that task placement happens at > wake up path and if it fails to select the right idle cpu at wake up, > you will have to wait for a load balance which is alreayd too late Oh, I was tracing most of scheduler activities and the ratios of wakeups picking idle CPUs were about the same regardless of cgroup membership. I can confidently say that the latency issue that I'm seeing is from load balancer picking the wrong busiest CPU, which is not to say that there can be other problems. > > another queued wouldn't report the correspondingly higher > > It will as load_avg includes the runnable_load_avg so whatever load is > in runnable_load_avg will be in load_avg too. But at the contrary, > runnable_load_avg will not have the blocked that is going to wake up > soon in the case of schbench Decaying contribution of blocked tasks don't affect the busiest CPU selection. Without cgroup, runnable_load_avg is immediately increased and decreased as tasks enter and leave the queue and otherwise we end up with CPUs which are idle when there are threads queued on different CPUs accumulating scheduling latencies. The patch doesn't change how the busiest CPU is picked. It already uses runnable_load_avg. The change that cgroup causes is that it blocks updates to runnable_load_avg from newly scheduled or sleeping tasks. The issue isn't about whether runnable_load_avg or load_avg should be used but the unexpected differences in the metrics that the load balancer uses depending on whether cgroup is used or not. > One last thing, the load_avg of an idle CPU can stay blocked for a > while (until a load balance happens that will update blocked load) and > can be seen has "busy" whereas it is not. Could it be a reason of your > problem ? AFAICS, the load balancer doesn't use load_avg. Thanks. -- tejun
[toc] | [prev] | [next] | [standalone]
| From | Vincent Guittot <vincent.guittot@linaro.org> |
|---|---|
| Date | 2017-04-27 10:30 +0200 |
| Message-ID | <tAMYi-6lN-9@gated-at.bofh.it> |
| In reply to | #1631787 |
On 27 April 2017 at 00:52, Tejun Heo <tj@kernel.org> wrote: > Hello, > > On Wed, Apr 26, 2017 at 08:12:09PM +0200, Vincent Guittot wrote: >> On 24 April 2017 at 22:14, Tejun Heo <tj@kernel.org> wrote: >> Can the problem be on the load balance side instead ? and more >> precisely in the wakeup path ? >> After looking at the trace, it seems that task placement happens at >> wake up path and if it fails to select the right idle cpu at wake up, >> you will have to wait for a load balance which is alreayd too late > > Oh, I was tracing most of scheduler activities and the ratios of > wakeups picking idle CPUs were about the same regardless of cgroup > membership. I can confidently say that the latency issue that I'm > seeing is from load balancer picking the wrong busiest CPU, which is > not to say that there can be other problems. ok. Is there any trace that you can share ? your behavior seems different of mine > >> > another queued wouldn't report the correspondingly higher >> >> It will as load_avg includes the runnable_load_avg so whatever load is >> in runnable_load_avg will be in load_avg too. But at the contrary, >> runnable_load_avg will not have the blocked that is going to wake up >> soon in the case of schbench > > Decaying contribution of blocked tasks don't affect the busiest CPU > selection. Without cgroup, runnable_load_avg is immediately increased > and decreased as tasks enter and leave the queue and otherwise we end > up with CPUs which are idle when there are threads queued on different > CPUs accumulating scheduling latencies. > > The patch doesn't change how the busiest CPU is picked. It already > uses runnable_load_avg. The change that cgroup causes is that it > blocks updates to runnable_load_avg from newly scheduled or sleeping > tasks. > > The issue isn't about whether runnable_load_avg or load_avg should be > used but the unexpected differences in the metrics that the load I think that's the root of the problem. I explain a bit more my view on the other thread > balancer uses depending on whether cgroup is used or not. > >> One last thing, the load_avg of an idle CPU can stay blocked for a >> while (until a load balance happens that will update blocked load) and >> can be seen has "busy" whereas it is not. Could it be a reason of your >> problem ? > > AFAICS, the load balancer doesn't use load_avg. > > Thanks. > > -- > tejun
[toc] | [prev] | [next] | [standalone]
| From | Tejun Heo <tj@kernel.org> |
|---|---|
| Date | 2017-04-28 22:40 +0200 |
| Message-ID | <tBkQh-4e7-5@gated-at.bofh.it> |
| In reply to | #1631949 |
Hello, Vincent. On Thu, Apr 27, 2017 at 10:29:10AM +0200, Vincent Guittot wrote: > On 27 April 2017 at 00:52, Tejun Heo <tj@kernel.org> wrote: > > Hello, > > > > On Wed, Apr 26, 2017 at 08:12:09PM +0200, Vincent Guittot wrote: > >> On 24 April 2017 at 22:14, Tejun Heo <tj@kernel.org> wrote: > >> Can the problem be on the load balance side instead ? and more > >> precisely in the wakeup path ? > >> After looking at the trace, it seems that task placement happens at > >> wake up path and if it fails to select the right idle cpu at wake up, > >> you will have to wait for a load balance which is alreayd too late > > > > Oh, I was tracing most of scheduler activities and the ratios of > > wakeups picking idle CPUs were about the same regardless of cgroup > > membership. I can confidently say that the latency issue that I'm > > seeing is from load balancer picking the wrong busiest CPU, which is > > not to say that there can be other problems. > > ok. Is there any trace that you can share ? your behavior seems > different of mine I'm attaching the debug patch. With your change (avg instead of runnable_avg), the following trace shows why it's wrong. It's dumping a case where group A has a CPU w/ more than two schbench threads and B doesn't, but the load balancer is determining that B is loaded heavier. dbg_odd: odd: dst=28 idle=2 brk=32 lbtgt=0-31 type=2 dbg_odd_dump: A: grp=1,17 w=2 avg=7.247 grp=8.337 sum=8.337 pertask=2.779 dbg_odd_dump: A: gcap=1.150 gutil=1.095 run=3 idle=0 gwt=2 type=2 nocap=1 dbg_odd_dump: A: CPU001: run=1 schb=1 dbg_odd_dump: A: Q001-asdf: w=1.000,l=0.525,u=0.513,r=0.527 run=1 hrun=1 tgs=100.000 tgw=17.266 dbg_odd_dump: A: Q001-asdf: schbench(153757C):w=1.000,l=0.527,u=0.514 dbg_odd_dump: A: Q001-/: w=5.744,l=2.522,u=0.520,r=3.067 run=1 hrun=1 tgs=1.000 tgw=0.000 dbg_odd_dump: A: Q001-/: asdf(C):w=5.744,l=3.017,u=0.521 dbg_odd_dump: A: CPU017: run=2 schb=2 dbg_odd_dump: A: Q017-asdf: w=2.000,l=0.989,u=0.966,r=0.988 run=2 hrun=2 tgs=100.000 tgw=17.266 dbg_odd_dump: A: Q017-asdf: schbench(153737C):w=1.000,l=0.493,u=0.482 schbench(153739):w=1.000,l=0.494,u=0.483 dbg_odd_dump: A: Q017-/: w=10.653,l=7.888,u=0.973,r=5.270 run=1 hrun=2 tgs=1.000 tgw=0.000 dbg_odd_dump: A: Q017-/: asdf(C):w=10.653,l=5.269,u=0.966 dbg_odd_dump: B: grp=14,30 w=2 avg=7.666 grp=8.819 sum=8.819 pertask=4.409 dbg_odd_dump: B: gcap=1.150 gutil=1.116 run=2 idle=0 gwt=2 type=2 nocap=1 dbg_odd_dump: B: CPU014: run=1 schb=1 dbg_odd_dump: B: Q014-asdf: w=1.000,l=1.004,u=0.970,r=0.492 run=1 hrun=1 tgs=100.000 tgw=17.266 dbg_odd_dump: B: Q014-asdf: schbench(153760C):w=1.000,l=0.491,u=0.476 dbg_odd_dump: B: Q014-/: w=5.605,l=11.146,u=0.970,r=5.774 run=1 hrun=1 tgs=1.000 tgw=0.000 dbg_odd_dump: B: Q014-/: asdf(C):w=5.605,l=5.766,u=0.970 dbg_odd_dump: B: CPU030: run=1 schb=1 dbg_odd_dump: B: Q030-asdf: w=1.000,l=0.538,u=0.518,r=0.558 run=1 hrun=1 tgs=100.000 tgw=17.266 dbg_odd_dump: B: Q030-asdf: schbench(153747C):w=1.000,l=0.537,u=0.516 dbg_odd_dump: B: Q030-/: w=5.758,l=3.186,u=0.541,r=3.044 run=1 hrun=1 tgs=1.000 tgw=0.000 dbg_odd_dump: B: Q030-/: asdf(C):w=5.758,l=3.092,u=0.516 You can notice that B's pertask weight is 4.409 which is way higher than A's 2.779, and this is from Q014-asdf's contribution to Q014-/ is twice as high as it should be. The root queue's runnable avg should only contain what's currently active but because we're scaling load avg which includes both active and blocked, we're ending up picking group B over A. This shows up in the total number of times we pick the wrong queue and thus latency. I'm running the following script with the debug patch applied. #!/bin/bash date cat /proc/self/cgroup echo 1000 > /sys/module/fair/parameters/dbg_odd_nth echo 0 > /sys/module/fair/parameters/dbg_odd_cnt ~/schbench -m 2 -t 16 -s 10000 -c 15000 -r 30 cat /sys/module/fair/parameters/dbg_odd_cnt With your patch applied, in the root cgroup, Fri Apr 28 12:48:59 PDT 2017 0::/ Latency percentiles (usec) 50.0000th: 26 75.0000th: 63 90.0000th: 78 95.0000th: 88 *99.0000th: 707 99.5000th: 5096 99.9000th: 10352 min=0, max=13743 577 In the /asdf cgroup, Fri Apr 28 13:19:53 PDT 2017 0::/asdf Latency percentiles (usec) 50.0000th: 35 75.0000th: 67 90.0000th: 81 95.0000th: 98 *99.0000th: 2212 99.5000th: 4536 99.9000th: 11024 min=0, max=13026 1708 The last line is the number of times the load balancer picked a group w/o more than two schbench threads on a CPU over one w/. Some number of these are expected as there are other threads and there are some plays in all the calculations but propgating avg or not propgating at all significantly increases the count and latency. > > The issue isn't about whether runnable_load_avg or load_avg should be > > used but the unexpected differences in the metrics that the load > > I think that's the root of the problem. I explain a bit more my view > on the other thread So, when picking the busiest group, the only thing which matters is the queue's runnable_load_avg, which should approximate the sum of all on-queue loads on that CPU. If we don't propagate or propagate load_avg, we're factoring in blocked avg of descendent cgroups into the root's runnable_load_avg which is obviously wrong. We can argue whether overriding a cfs_rq se's load_avg to the scaled runnable_load_avg of the cfs_rq is the right way to go or we should introduce a separate channel to propagate runnable_load_avg; however, it's clear that we need to fix runnable_load_avg propagation one way or another. The thing with cfs_rq se's load_avg is that, it isn't really used anywhere else AFAICS, so overriding it to the cfs_rq's runnable_load_avg isn't prettiest but doesn't really change anything. Thanks. -- tejun
[toc] | [prev] | [next] | [standalone]
| From | Tejun Heo <tj@kernel.org> |
|---|---|
| Date | 2017-04-28 22:40 +0200 |
| Message-ID | <tBkQh-4e7-13@gated-at.bofh.it> |
| In reply to | #1633141 |
Here's the debug patch.
The debug condition triggers when the load balancer picks a group w/o
more than one schbench threads on a CPU over one w/.
/sys/module/fair/parameters/dbg_odd_cnt: resettable counter
/sys/module/fair/parameters/dbg_odd_nth: dump group states on Nth
occurrence via trace_printk()
The load / weights are printed out so that NICE_0_LOAD is 1.000.
Thanks.
---
kernel/sched/fair.c | 160 +++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 159 insertions(+), 1 deletion(-)
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -32,11 +32,18 @@
#include <linux/mempolicy.h>
#include <linux/migrate.h>
#include <linux/task_work.h>
+#include <linux/moduleparam.h>
#include <trace/events/sched.h>
#include "sched.h"
+static unsigned long dbg_odd_nth;
+static unsigned long dbg_odd_cnt;
+
+module_param(dbg_odd_nth, ulong, 0644);
+module_param(dbg_odd_cnt, ulong, 0644);
+
/*
* Targeted preemption latency for CPU-bound tasks:
*
@@ -7413,6 +7420,149 @@ static inline void update_sg_lb_stats(st
sgs->group_type = group_classify(group, sgs);
}
+static int count_schb(struct rq *rq)
+{
+ unsigned long flags;
+ struct task_struct *p;
+ int cnt = 0;
+
+ raw_spin_lock_irqsave(&rq->lock, flags);
+
+ list_for_each_entry(p, &rq->cfs_tasks, se.group_node)
+ if (!strncmp(p->comm, "schbench", 8))
+ cnt++;
+
+ raw_spin_unlock_irqrestore(&rq->lock, flags);
+
+ return cnt;
+}
+
+static bool sg_has_two_schb(struct sched_group *sg)
+{
+ int cpu;
+
+ for_each_cpu(cpu, sched_group_cpus(sg))
+ if (count_schb(cpu_rq(cpu)) >= 2)
+ return true;
+ return false;
+}
+
+static DEFINE_PER_CPU(char [PAGE_SIZE], odd_buf);
+
+#define lbw(x) (int)((x) / NICE_0_LOAD), (int)(((x) % NICE_0_LOAD) * 1000 / NICE_0_LOAD)
+#define lba(x) (int)((scale_load(x)) / NICE_0_LOAD), (int)(((scale_load(x)) % NICE_0_LOAD) * 1000 / NICE_0_LOAD)
+
+static int odd_append_se(struct sched_entity *se, const char *postfix,
+ int cnt, char *buf, size_t size)
+{
+#define odd_append(fmt, args...) do { \
+ cnt += scnprintf(buf + cnt, size - cnt, fmt, ##args); \
+ cnt = min_t(int, cnt, size); \
+} while (0)
+
+ if (entity_is_task(se)) {
+ struct task_struct *task = task_of(se);
+ odd_append(" %s(%d%s)", task->comm, task->pid, postfix);
+ } else {
+ char nbuf[64];
+ cgroup_name(se->my_q->tg->css.cgroup, nbuf, sizeof(nbuf));
+ odd_append(" %s(%s)", nbuf, postfix);
+ }
+ odd_append(":w=%d.%03d,l=%d.%03d,u=%d.%03d",
+ lbw(se->load.weight),
+ lba(se->avg.load_avg),
+ lba(se->avg.util_avg));
+
+ return cnt;
+}
+
+static void dbg_odd_dump(const char *pref,
+ struct sched_group *sg, struct sg_lb_stats *sgs)
+{
+ int cpu;
+
+ trace_printk("%sgrp=%*pbl w=%u avg=%d.%03d grp=%d.%03d sum=%d.%03d pertask=%d.%03d\n", pref,
+ cpumask_pr_args(sched_group_cpus(sg)), sg->group_weight,
+ lba(sgs->avg_load), lba(sgs->group_load),
+ lba(sgs->sum_weighted_load), lba(sgs->load_per_task));
+ trace_printk("%sgcap=%d.%03d gutil=%d.%03d run=%u idle=%u gwt=%u type=%d nocap=%d\n",
+ pref,
+ lba(sgs->group_capacity), lba(sgs->group_util),
+ sgs->sum_nr_running, sgs->idle_cpus, sgs->group_weight,
+ sgs->group_type, sgs->group_no_capacity);
+
+ for_each_cpu(cpu, sched_group_cpus(sg)) {
+ struct task_group *tg;
+ unsigned long flags;
+
+ trace_printk("%sCPU%03d: run=%u schb=%d\n", pref, cpu,
+ cpu_rq(cpu)->nr_running, count_schb(cpu_rq(cpu)));
+
+ raw_spin_lock_irqsave(&cpu_rq(cpu)->lock, flags);
+
+ list_for_each_entry_rcu(tg, &task_groups, list) {
+ struct cfs_rq *cfs_rq = tg->cfs_rq[cpu];
+ char qname[32] = "root";
+ int depth = 0;
+ long tg_weight = 0, tg_shares = 0;
+ struct sched_entity *se;
+ char *buf = per_cpu_ptr(odd_buf, cpu);
+ int cnt;
+
+ if (!cfs_rq->nr_running)
+ continue;
+
+ if (cfs_rq->tg) {
+ cgroup_name(cfs_rq->tg->css.cgroup, qname, sizeof(qname));
+ if (cfs_rq->tg->se[cpu])
+ depth = cfs_rq->tg->se[cpu]->depth;
+ tg_weight = atomic_long_read(&cfs_rq->tg->load_avg);
+ tg_shares = cfs_rq->tg->shares;
+ }
+
+ trace_printk("%sQ%03d-%s@%d: w=%d.%03d,l=%d.%03d,u=%d.%03d,r=%d.%03d run=%u hrun=%u tgs=%d.%03d tgw=%d.%03d\n",
+ pref, cpu, qname, depth,
+ lbw(cfs_rq->load.weight),
+ lba(cfs_rq->avg.load_avg),
+ lba(cfs_rq->avg.util_avg),
+ lba(cfs_rq->runnable_load_avg),
+ cfs_rq->nr_running, cfs_rq->h_nr_running,
+ lbw(tg_shares),
+ lba(tg_weight));
+
+ buf[0] = '\0';
+ cnt = 0;
+
+ if (cfs_rq->curr)
+ cnt = odd_append_se(cfs_rq->curr, "C", cnt, buf, PAGE_SIZE);
+
+ for (se = __pick_first_entity(cfs_rq); se;
+ se = __pick_next_entity(se))
+ cnt = odd_append_se(se, "", cnt, buf, PAGE_SIZE);
+
+ trace_printk("%sQ%03d-%s@%d: %s\n",
+ pref, cpu, qname, depth, buf);
+ }
+
+ raw_spin_unlock_irqrestore(&cpu_rq(cpu)->lock, flags);
+ }
+}
+
+/* a has >= 2 dts, b doesn't */
+static void dbg_odd(struct lb_env *env,
+ struct sched_group *sga, struct sg_lb_stats *sgsa,
+ struct sched_group *sgb, struct sg_lb_stats *sgsb)
+{
+ if (dbg_odd_nth && (dbg_odd_cnt++ % dbg_odd_nth))
+ return;
+
+ trace_printk("odd: dst=%d idle=%d brk=%u lbtgt=%*pbl type=%d\n",
+ env->dst_cpu, env->idle, env->loop_break,
+ cpumask_pr_args(env->cpus), env->fbq_type);
+ dbg_odd_dump("A: ", sga, sgsa);
+ dbg_odd_dump("B: ", sgb, sgsb);
+}
+
/**
* update_sd_pick_busiest - return 1 on busiest group
* @env: The load balancing environment.
@@ -7432,6 +7582,8 @@ static bool update_sd_pick_busiest(struc
struct sg_lb_stats *sgs)
{
struct sg_lb_stats *busiest = &sds->busiest_stat;
+ bool busiest_has_two = sds->busiest && sg_has_two_schb(sds->busiest);
+ bool sg_has_two = sg_has_two_schb(sg);
if (sgs->group_type > busiest->group_type)
return true;
@@ -7439,8 +7591,14 @@ static bool update_sd_pick_busiest(struc
if (sgs->group_type < busiest->group_type)
return false;
- if (sgs->avg_load <= busiest->avg_load)
+ if (sgs->avg_load <= busiest->avg_load) {
+ if (sg_has_two && !busiest_has_two)
+ dbg_odd(env, sg, sgs, sds->busiest, busiest);
return false;
+ }
+
+ if (!sg_has_two && busiest_has_two)
+ dbg_odd(env, sds->busiest, busiest, sg, sgs);
if (!(env->sd->flags & SD_ASYM_CPUCAPACITY))
goto asym_packing;
[toc] | [prev] | [next] | [standalone]
Page 1 of 2 [1] 2 Next page →
Back to top | Article view | linux.kernel
csiph-web