Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1490171 > unrolled thread

Re: [PATCH] sched/fair: Do not decay new task load on first enqueue

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.

Contents

  Re: [PATCH] sched/fair: Do not decay new task load on first enqueue Vincent Guittot <vincent.guittot@linaro.org> - 2016-09-23 16:40 +0200
    Re: [PATCH] sched/fair: Do not decay new task load on first enqueue Dietmar Eggemann <dietmar.eggemann@arm.com> - 2016-09-27 15:50 +0200
      Re: [PATCH] sched/fair: Do not decay new task load on first enqueue Matt Fleming <matt@codeblueprint.co.uk> - 2016-09-27 21:30 +0200
    Re: [PATCH] sched/fair: Do not decay new task load on first enqueue Matt Fleming <matt@codeblueprint.co.uk> - 2016-09-27 21:30 +0200

#1490171 — Re: [PATCH] sched/fair: Do not decay new task load on first enqueue

Hi Matt,

On 23 September 2016 at 13:58, Matt Fleming <matt@codeblueprint.co.uk> wrote:
> Since commit 7dc603c9028e ("sched/fair: Fix PELT integrity for new
> tasks") ::last_update_time will be set to a non-zero value in
> post_init_entity_util_avg(), which leads to p->se.avg.load_avg being
> decayed on enqueue before the task has even had a chance to run.
>
> For a NICE_0 task the sequence of events leading up to this with
> example load average changes might be,
>
>   sched_fork()
>     init_entity_runnable_average()
>       p->se.avg.load_avg = scale_load_down(se->load.weight);    // 1024
>
>   wake_up_new_task()
>     post_init_entity_util_avg()
>       attach_entity_load_avg()
>         p->se.last_update_time = cfs_rq->avg.last_update_time;
>
>     activate_task()
>       enqueue_task()
>         ...
>           enqueue_entity_load_avg()
>             migrated = !sa->last_update_time                    // false
>             if (!migrated)
>                     __update_load_avg()
>                       p->se.avg.load_avg = 1002

Does it mean that you can see the perf drop that you mention below
because load is decayed to 1002 instead of staying to 1024 ?

1002 mainly comes from period_contrib being set to 1023 during
init_entity_runnable_average so any delay longer than 1us between
attach_entity_load_avg and enqueue_entity_load_avg will trig the decay
of the load from 1024 to 1002

>
> This causes a performance regression for fork intensive workloads like
> hackbench. When balancing on fork we can end up picking the same CPU
> to enqueue on over and over. This leads to huge congestion when trying
> to simultaneously wake up tasks that are all on the same runqueue, and
> causes lots of migrations on wake up.
>
> The behaviour since commit 7dc603c9028e essentially defeats the
> scheduler's attempt to balance on fork(). Before, ::runnable_load_avg
> likely had a non-zero value when the hackbench tasks were dequeued
> (the fork()'d tasks immediately block reading on pipe/socket) but now
> the load balancer sees the CPU as having no runnable load.

But this patch doesn't change the behavior of runnable_load_avg, isn't
it ? it has only an impact on the initial value of p->se.avg.load_avg
when the task is enqueued.

>
> Arguably the real problem is that balancing on fork doesn't look at
> the blocked contribution of tasks, only the runnable load and it's
> possible for the two metrics to be wildly different on a relatively
> idle system.

fair enough

>
> But it still doesn't seem quite right to update a task's load_avg
> before it runs for the first time.
>
> Here are the results of running hackbench before 7dc603c9028e (old
> behaviour), with 7dc603c9028e applied (exiting behaviour), and after
> 7dc603c9028e with this patch on top (new behaviour),
>
> hackbench-process-sockets
>
>                          4.7.0-rc5             4.7.0-rc5             4.7.0-rc5
>                             before          7dc603c9028e                 after
> Amean    1        0.0611 (  0.00%)      0.0693 (-13.32%)      0.0600 (  1.87%)
> Amean    4        0.1777 (  0.00%)      0.1730 (  2.65%)      0.1790 ( -0.72%)
> Amean    7        0.2771 (  0.00%)      0.2816 ( -1.60%)      0.2741 (  1.08%)
> Amean    12       0.3851 (  0.00%)      0.4167 ( -8.20%)      0.3751 (  2.60%)
>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Mike Galbraith <umgwanakikbuti@gmail.com>
> Cc: Yuyang Du <yuyang.du@intel.com>
> Cc: Vincent Guittot <vincent.guittot@linaro.org>
> Cc: Dietmar Eggemann <dietmar.eggemann@arm.com>
> Signed-off-by: Matt Fleming <matt@codeblueprint.co.uk>
> ---
>  kernel/sched/fair.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 8fb4d1942c14..4a2d3ff772f8 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -3142,7 +3142,7 @@ enqueue_entity_load_avg(struct cfs_rq *cfs_rq, struct sched_entity *se)
>         int migrated, decayed;
>
>         migrated = !sa->last_update_time;
> -       if (!migrated) {
> +       if (!migrated && se->sum_exec_runtime) {
>                 __update_load_avg(now, cpu_of(rq_of(cfs_rq)), sa,
>                         se->on_rq * scale_load_down(se->load.weight),
>                         cfs_rq->curr == se, NULL);
> --
> 2.10.0
>

[toc] | [next] | [standalone]

#1491907

On 23/09/16 15:30, Vincent Guittot wrote:
> Hi Matt,
> 
> On 23 September 2016 at 13:58, Matt Fleming <matt@codeblueprint.co.uk> wrote:
>> Since commit 7dc603c9028e ("sched/fair: Fix PELT integrity for new
>> tasks") ::last_update_time will be set to a non-zero value in
>> post_init_entity_util_avg(), which leads to p->se.avg.load_avg being
>> decayed on enqueue before the task has even had a chance to run.
>>
>> For a NICE_0 task the sequence of events leading up to this with
>> example load average changes might be,
>>
>>   sched_fork()
>>     init_entity_runnable_average()
>>       p->se.avg.load_avg = scale_load_down(se->load.weight);    // 1024
>>
>>   wake_up_new_task()
>>     post_init_entity_util_avg()
>>       attach_entity_load_avg()
>>         p->se.last_update_time = cfs_rq->avg.last_update_time;
>>
>>     activate_task()
>>       enqueue_task()
>>         ...
>>           enqueue_entity_load_avg()
>>             migrated = !sa->last_update_time                    // false
>>             if (!migrated)
>>                     __update_load_avg()
>>                       p->se.avg.load_avg = 1002
> 
> Does it mean that you can see the perf drop that you mention below
> because load is decayed to 1002 instead of staying to 1024 ?

I think Matt is talking about the fact that the cfs->runnable_load_avg
value is 0 once the hackbench task is initially dequeued.

Without this patch the value of se->avg.load_avg (e.g. both times 1002)
is exactly the same when we add it to cfs_rq->runnable_load_avg in
enqueue_entity_load_avg() and when we subtract it in
dequeue_entity_load_avg(). That's because the initial runtime is short
(~250us on my hikey board).

With this patch we add 1024 and subtract ~1002 which lets
cfs_rq->runnable_load_avg still have a small positive value. This
favours that for the next hackbench task another cpu will be chosen in
(load-based) fork-balance.

> 
> 1002 mainly comes from period_contrib being set to 1023 during
> init_entity_runnable_average so any delay longer than 1us between
> attach_entity_load_avg and enqueue_entity_load_avg will trig the decay
> of the load from 1024 to 1002
> 

[...]

[toc] | [prev] | [next] | [standalone]

#1492114

On Tue, 27 Sep, at 02:48:31PM, Dietmar Eggemann wrote:
> 
> I think Matt is talking about the fact that the cfs->runnable_load_avg
> value is 0 once the hackbench task is initially dequeued.
 
Yes.

> Without this patch the value of se->avg.load_avg (e.g. both times 1002)
> is exactly the same when we add it to cfs_rq->runnable_load_avg in
> enqueue_entity_load_avg() and when we subtract it in
> dequeue_entity_load_avg(). That's because the initial runtime is short
> (~250us on my hikey board).
> 
> With this patch we add 1024 and subtract ~1002 which lets
> cfs_rq->runnable_load_avg still have a small positive value. This
> favours that for the next hackbench task another cpu will be chosen in
> (load-based) fork-balance.
 
Bingo, that's exactly it. Sorry if i was unclear.

[toc] | [prev] | [next] | [standalone]

#1492116

On Fri, 23 Sep, at 04:30:25PM, Vincent Guittot wrote:
> 
> Does it mean that you can see the perf drop that you mention below
> because load is decayed to 1002 instead of staying to 1024 ?
 
The performance drop comes from the fact that enqueueing/dequeueing a
task with load 1002 during fork() results in a zero runnable_load_avg,
which signals to the load balancer that the CPU is idle, so the next
time we fork() we'll pick the same CPU to enqueue on -- and the cycle
continues.

I mention the performance regression mainly because it's the thing
that led to me discovering this bug, and only a little as support for
applying the patch ;-)

> 1002 mainly comes from period_contrib being set to 1023 during
> init_entity_runnable_average so any delay longer than 1us between
> attach_entity_load_avg and enqueue_entity_load_avg will trig the decay
> of the load from 1024 to 1002
 
Right.

> But this patch doesn't change the behavior of runnable_load_avg, isn't
> it ? it has only an impact on the initial value of p->se.avg.load_avg
> when the task is enqueued.
 
Correct. It isn't guaranteed that runnable_load_avg will be non-zero
with this patch applied, that was just the case for the workload and
the machine I tested.

> > Arguably the real problem is that balancing on fork doesn't look at
> > the blocked contribution of tasks, only the runnable load and it's
> > possible for the two metrics to be wildly different on a relatively
> > idle system.
> 
> fair enough

I did have some patches somewhere to address this. I'll have to dig
them out.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web