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

Groups > linux.kernel > #1489970 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH] sched/fair: Do not decay new task load on first enqueue Matt Fleming <matt@codeblueprint.co.uk> - 2016-09-23 14:00 +0200
    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
    Re: [PATCH] sched/fair: Do not decay new task load on first enqueue Peter Zijlstra <peterz@infradead.org> - 2016-09-28 12:20 +0200
      Re: [PATCH] sched/fair: Do not decay new task load on first enqueue Dietmar Eggemann <dietmar.eggemann@arm.com> - 2016-09-28 13:10 +0200
        Re: [PATCH] sched/fair: Do not decay new task load on first enqueue Peter Zijlstra <peterz@infradead.org> - 2016-09-28 13:20 +0200
          Re: [PATCH] sched/fair: Do not decay new task load on first enqueue Dietmar Eggemann <dietmar.eggemann@arm.com> - 2016-09-28 13:40 +0200
            Re: [PATCH] sched/fair: Do not decay new task load on first enqueue Vincent Guittot <vincent.guittot@linaro.org> - 2016-09-28 13:50 +0200
              Re: [PATCH] sched/fair: Do not decay new task load on first enqueue Vincent Guittot <vincent.guittot@linaro.org> - 2016-09-28 14:10 +0200
                Re: [PATCH] sched/fair: Do not decay new task load on first enqueue Matt Fleming <matt@codeblueprint.co.uk> - 2016-10-04 23:30 +0200
              Re: [PATCH] sched/fair: Do not decay new task load on first enqueue Matt Fleming <matt@codeblueprint.co.uk> - 2016-10-04 22:20 +0200
            Re: [PATCH] sched/fair: Do not decay new task load on first enqueue Vincent Guittot <vincent.guittot@linaro.org> - 2016-09-28 14:30 +0200
              Re: [PATCH] sched/fair: Do not decay new task load on first enqueue Vincent Guittot <vincent.guittot@linaro.org> - 2016-09-28 15:20 +0200
                Re: [PATCH] sched/fair: Do not decay new task load on first enqueue Dietmar Eggemann <dietmar.eggemann@arm.com> - 2016-09-29 18:20 +0200
                  Re: [PATCH] sched/fair: Do not decay new task load on first enqueue Vincent Guittot <vincent.guittot@linaro.org> - 2016-10-03 15:10 +0200
          Re: [PATCH] sched/fair: Do not decay new task load on first enqueue Dietmar Eggemann <dietmar.eggemann@arm.com> - 2016-09-28 20:10 +0200
      Re: [PATCH] sched/fair: Do not decay new task load on first enqueue Matt Fleming <matt@codeblueprint.co.uk> - 2016-09-28 21:40 +0200
        Re: [PATCH] sched/fair: Do not decay new task load on first enqueue Matt Fleming <matt@codeblueprint.co.uk> - 2016-09-30 22:40 +0200
        Re: [PATCH] sched/fair: Do not decay new task load on first enqueue Wanpeng Li <kernellwp@gmail.com> - 2016-10-09 05:40 +0200
          Re: [PATCH] sched/fair: Do not decay new task load on first enqueue Matt Fleming <matt@codeblueprint.co.uk> - 2016-10-10 12:10 +0200
            Re: [PATCH] sched/fair: Do not decay new task load on first enqueue Wanpeng Li <kernellwp@gmail.com> - 2016-10-10 12:20 +0200
              Re: [PATCH] sched/fair: Do not decay new task load on first enqueue Matt Fleming <matt@codeblueprint.co.uk> - 2016-10-11 12:30 +0200
            Re: [PATCH] sched/fair: Do not decay new task load on first enqueue Vincent Guittot <vincent.guittot@linaro.org> - 2016-10-10 14:40 +0200
              Re: [PATCH] sched/fair: Do not decay new task load on first enqueue Dietmar Eggemann <dietmar.eggemann@arm.com> - 2016-10-10 16:00 +0200
                Re: [PATCH] sched/fair: Do not decay new task load on first enqueue Vincent Guittot <vincent.guittot@linaro.org> - 2016-10-10 20:30 +0200
                  Re: [PATCH] sched/fair: Do not decay new task load on first enqueue Dietmar Eggemann <dietmar.eggemann@arm.com> - 2016-10-11 11:50 +0200
                    Re: [PATCH] sched/fair: Do not decay new task load on first enqueue Matt Fleming <matt@codeblueprint.co.uk> - 2016-10-11 12:50 +0200
              Re: [PATCH] sched/fair: Do not decay new task load on first enqueue Vincent Guittot <vincent.guittot@linaro.org> - 2016-10-10 19:40 +0200
                Re: [PATCH] sched/fair: Do not decay new task load on first enqueue Matt Fleming <matt@codeblueprint.co.uk> - 2016-10-11 12:30 +0200
                  Re: [PATCH] sched/fair: Do not decay new task load on first enqueue Vincent Guittot <vincent.guittot@linaro.org> - 2016-10-11 15:20 +0200
                    Re: [PATCH] sched/fair: Do not decay new task load on first enqueue Matt Fleming <matt@codeblueprint.co.uk> - 2016-10-11 21:10 +0200
                      Re: [PATCH] sched/fair: Do not decay new task load on first enqueue Vincent Guittot <vincent.guittot@linaro.org> - 2016-10-12 09:50 +0200
      Re: [PATCH] sched/fair: Do not decay new task load on first enqueue Matt Fleming <matt@codeblueprint.co.uk> - 2016-10-04 22:20 +0200

Page 1 of 2  [1] 2  Next page →

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

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

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.

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.

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]

#1490171

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] | [prev] | [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] | [next] | [standalone]

#1492513

On Fri, Sep 23, 2016 at 12:58:08PM +0100, Matt Fleming wrote:
> 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);


Hrmm,.. so I see the problem, but I think we're working around it.

So the problem is that time moves between wake_up_new_task() doing
post_init_entity_util_avg(), which attaches us to the cfs_rq, and
activate_task() which enqueues us.

Part of the problem is that we do not in fact seem to do
update_rq_clock() before post_init_entity_util_avg(), which makes the
delta larger than it should be.

The other problem is that activate_task()->enqueue_task() does do
update_rq_clock() (again, after fixing), creating the delta.

Which suggests we do something like the below (not compile tested or
anything, also I ran out of tea again).

While staring at this, I don't think we can still hit
vruntime_normalized() with a new task, so I _think_ we can remove that
!se->sum_exec_runtime clause there (and rejoice), no?


---
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 7e7463aa399a..cc59bd4ab809 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -754,9 +754,16 @@ static void set_load_weight(struct task_struct *p)
 
 static inline void enqueue_task(struct rq *rq, struct task_struct *p, int flags)
 {
-	update_rq_clock(rq);
+	/*
+	 * For ENQUEUE_RESTORE, DEQUEUE_SAVE will have updated the rq-clock,
+	 * for ENQUEUE_NEW wake_up_new_task() will have.
+	 */
+	if (!(flags & (ENQUEUE_RESTORE | ENQUEUE_NEW)))
+		update_rq_clock(rq);
+
 	if (!(flags & ENQUEUE_RESTORE))
 		sched_info_queued(rq, p);
+
 	p->sched_class->enqueue_task(rq, p, flags);
 }
 
@@ -2577,9 +2584,11 @@ void wake_up_new_task(struct task_struct *p)
 	__set_task_cpu(p, select_task_rq(p, task_cpu(p), SD_BALANCE_FORK, 0));
 #endif
 	rq = __task_rq_lock(p, &rf);
+
+	update_rq_clock(rq);
 	post_init_entity_util_avg(&p->se);
+	activate_task(rq, p, ENQUEUE_NEW);
 
-	activate_task(rq, p, 0);
 	p->on_rq = TASK_ON_RQ_QUEUED;
 	trace_sched_wakeup_new(p);
 	check_preempt_curr(rq, p, WF_FORK);
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index 7c7e5745038b..3982d7dc9bff 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -1193,6 +1193,7 @@ extern const u32 sched_prio_to_wmult[40];
 #else
 #define ENQUEUE_MIGRATED	0x00
 #endif
+#define ENQUEUE_NEW		0x40
 
 #define RETRY_TASK		((void *)-1UL)
 

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

#1492532

On 28/09/16 11:14, Peter Zijlstra wrote:
> On Fri, Sep 23, 2016 at 12:58:08PM +0100, Matt Fleming wrote:
>> 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);
> 
> 
> Hrmm,.. so I see the problem, but I think we're working around it.
> 
> So the problem is that time moves between wake_up_new_task() doing
> post_init_entity_util_avg(), which attaches us to the cfs_rq, and
> activate_task() which enqueues us.
> 
> Part of the problem is that we do not in fact seem to do
> update_rq_clock() before post_init_entity_util_avg(), which makes the
> delta larger than it should be.

Yes, this is what I see as well. I always thought that the update is
done in task_fork_fair() so it's bounded but as I know now, this update
is only for the waker. In case the cpu was idle before the delta can be
pretty big.

> The other problem is that activate_task()->enqueue_task() does do
> update_rq_clock() (again, after fixing), creating the delta.

Not sure what you mean by 'after fixing' but the se is initialized with
a possibly stale 'now' value in post_init_entity_util_avg()->
attach_entity_load_avg() before the clock is updated in
activate_task()->enqueue_task().

> Which suggests we do something like the below (not compile tested or
> anything, also I ran out of tea again).

I'll give it a try. Plenty of coffee here ...

> 
> While staring at this, I don't think we can still hit
> vruntime_normalized() with a new task, so I _think_ we can remove that
> !se->sum_exec_runtime clause there (and rejoice), no?

I'm afraid that with accurate timing we will get the same situation that
we add and subtract the same amount of load (probably 1024 now and not
1002 (or less)) to/from cfs_rq->runnable_load_avg for the initial (fork)
hackbench run.
After all, it's 'runnable' based.

[...]

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

#1492534

On Wed, Sep 28, 2016 at 12:06:43PM +0100, Dietmar Eggemann wrote:
> On 28/09/16 11:14, Peter Zijlstra wrote:
> > On Fri, Sep 23, 2016 at 12:58:08PM +0100, Matt Fleming wrote:
> >> 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);
> > 
> > 
> > Hrmm,.. so I see the problem, but I think we're working around it.
> > 
> > So the problem is that time moves between wake_up_new_task() doing
> > post_init_entity_util_avg(), which attaches us to the cfs_rq, and
> > activate_task() which enqueues us.
> > 
> > Part of the problem is that we do not in fact seem to do
> > update_rq_clock() before post_init_entity_util_avg(), which makes the
> > delta larger than it should be.
> 
> Yes, this is what I see as well. I always thought that the update is
> done in task_fork_fair() so it's bounded but as I know now, this update
> is only for the waker. In case the cpu was idle before the delta can be
> pretty big.
> 
> > The other problem is that activate_task()->enqueue_task() does do
> > update_rq_clock() (again, after fixing), creating the delta.
> 
> Not sure what you mean by 'after fixing' but the se is initialized with
> a possibly stale 'now' value in post_init_entity_util_avg()->
> attach_entity_load_avg() before the clock is updated in
> activate_task()->enqueue_task().

I meant that after I fix the above issue of calling post_init with a
stale clock. So the + update_rq_clock(rq) in the patch.

> > Which suggests we do something like the below (not compile tested or
> > anything, also I ran out of tea again).
> 
> I'll give it a try. Plenty of coffee here ...
> 
> > 
> > While staring at this, I don't think we can still hit
> > vruntime_normalized() with a new task, so I _think_ we can remove that
> > !se->sum_exec_runtime clause there (and rejoice), no?
> 
> I'm afraid that with accurate timing we will get the same situation that
> we add and subtract the same amount of load (probably 1024 now and not
> 1002 (or less)) to/from cfs_rq->runnable_load_avg for the initial (fork)
> hackbench run.
> After all, it's 'runnable' based.

The idea was that since we now update rq clock before post_init and then
leave it be, both post_init and enqueue see the exact same timestamp,
and the delta is 0, resulting in no aging.

Or did I fail to make that happen?

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

#1492537

On 28/09/16 12:19, Peter Zijlstra wrote:
> On Wed, Sep 28, 2016 at 12:06:43PM +0100, Dietmar Eggemann wrote:
>> On 28/09/16 11:14, Peter Zijlstra wrote:
>>> On Fri, Sep 23, 2016 at 12:58:08PM +0100, Matt Fleming wrote:

[...]

>> Not sure what you mean by 'after fixing' but the se is initialized with
>> a possibly stale 'now' value in post_init_entity_util_avg()->
>> attach_entity_load_avg() before the clock is updated in
>> activate_task()->enqueue_task().
> 
> I meant that after I fix the above issue of calling post_init with a
> stale clock. So the + update_rq_clock(rq) in the patch.

OK.

[...]

>>> While staring at this, I don't think we can still hit
>>> vruntime_normalized() with a new task, so I _think_ we can remove that
>>> !se->sum_exec_runtime clause there (and rejoice), no?
>>
>> I'm afraid that with accurate timing we will get the same situation that
>> we add and subtract the same amount of load (probably 1024 now and not
>> 1002 (or less)) to/from cfs_rq->runnable_load_avg for the initial (fork)
>> hackbench run.
>> After all, it's 'runnable' based.
> 
> The idea was that since we now update rq clock before post_init and then
> leave it be, both post_init and enqueue see the exact same timestamp,
> and the delta is 0, resulting in no aging.
> 
> Or did I fail to make that happen?

No, but IMHO what Matt wants is ageing for the hackench tasks at the end
of their fork phase so there is a tiny amount of
cfs_rq->runnable_load_avg left on cpuX after the fork related dequeue so
the (load-based) fork-balancer chooses cpuY for the next hackbench task.
That's why he wanted to avoid the __update_load_avg(se) on enqueue (thus
adding 1024 to cfs_rq->runnable_load_avg) and do the ageing only on
dequeue (removing <1024 from cfs_rq->runnable_load_avg).

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

#1492546

On 28 September 2016 at 04:31, Dietmar Eggemann
<dietmar.eggemann@arm.com> wrote:
> On 28/09/16 12:19, Peter Zijlstra wrote:
>> On Wed, Sep 28, 2016 at 12:06:43PM +0100, Dietmar Eggemann wrote:
>>> On 28/09/16 11:14, Peter Zijlstra wrote:
>>>> On Fri, Sep 23, 2016 at 12:58:08PM +0100, Matt Fleming wrote:
>
> [...]
>
>>> Not sure what you mean by 'after fixing' but the se is initialized with
>>> a possibly stale 'now' value in post_init_entity_util_avg()->
>>> attach_entity_load_avg() before the clock is updated in
>>> activate_task()->enqueue_task().
>>
>> I meant that after I fix the above issue of calling post_init with a
>> stale clock. So the + update_rq_clock(rq) in the patch.
>
> OK.
>
> [...]
>
>>>> While staring at this, I don't think we can still hit
>>>> vruntime_normalized() with a new task, so I _think_ we can remove that
>>>> !se->sum_exec_runtime clause there (and rejoice), no?
>>>
>>> I'm afraid that with accurate timing we will get the same situation that
>>> we add and subtract the same amount of load (probably 1024 now and not
>>> 1002 (or less)) to/from cfs_rq->runnable_load_avg for the initial (fork)
>>> hackbench run.
>>> After all, it's 'runnable' based.
>>
>> The idea was that since we now update rq clock before post_init and then
>> leave it be, both post_init and enqueue see the exact same timestamp,
>> and the delta is 0, resulting in no aging.
>>
>> Or did I fail to make that happen?
>
> No, but IMHO what Matt wants is ageing for the hackench tasks at the end
> of their fork phase so there is a tiny amount of
> cfs_rq->runnable_load_avg left on cpuX after the fork related dequeue so
> the (load-based) fork-balancer chooses cpuY for the next hackbench task.
> That's why he wanted to avoid the __update_load_avg(se) on enqueue (thus
> adding 1024 to cfs_rq->runnable_load_avg) and do the ageing only on
> dequeue (removing <1024 from cfs_rq->runnable_load_avg).

ok so i'm a bit confused there
my understand of your explanation above  is that now we left a small
amount of load in runnable_load_avg after the dequeue so another cpu
will be chosen. But this explanation seems to be the opposite of what
Matt said in a previous email that:
"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."

>
>

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

#1492557

On 28 September 2016 at 04:46, Vincent Guittot
<vincent.guittot@linaro.org> wrote:
> On 28 September 2016 at 04:31, Dietmar Eggemann
> <dietmar.eggemann@arm.com> wrote:
>> On 28/09/16 12:19, Peter Zijlstra wrote:
>>> On Wed, Sep 28, 2016 at 12:06:43PM +0100, Dietmar Eggemann wrote:
>>>> On 28/09/16 11:14, Peter Zijlstra wrote:
>>>>> On Fri, Sep 23, 2016 at 12:58:08PM +0100, Matt Fleming wrote:
>>
>> [...]
>>
>>>> Not sure what you mean by 'after fixing' but the se is initialized with
>>>> a possibly stale 'now' value in post_init_entity_util_avg()->
>>>> attach_entity_load_avg() before the clock is updated in
>>>> activate_task()->enqueue_task().
>>>
>>> I meant that after I fix the above issue of calling post_init with a
>>> stale clock. So the + update_rq_clock(rq) in the patch.
>>
>> OK.
>>
>> [...]
>>
>>>>> While staring at this, I don't think we can still hit
>>>>> vruntime_normalized() with a new task, so I _think_ we can remove that
>>>>> !se->sum_exec_runtime clause there (and rejoice), no?
>>>>
>>>> I'm afraid that with accurate timing we will get the same situation that
>>>> we add and subtract the same amount of load (probably 1024 now and not
>>>> 1002 (or less)) to/from cfs_rq->runnable_load_avg for the initial (fork)
>>>> hackbench run.
>>>> After all, it's 'runnable' based.
>>>
>>> The idea was that since we now update rq clock before post_init and then
>>> leave it be, both post_init and enqueue see the exact same timestamp,
>>> and the delta is 0, resulting in no aging.
>>>
>>> Or did I fail to make that happen?
>>
>> No, but IMHO what Matt wants is ageing for the hackench tasks at the end
>> of their fork phase so there is a tiny amount of
>> cfs_rq->runnable_load_avg left on cpuX after the fork related dequeue so
>> the (load-based) fork-balancer chooses cpuY for the next hackbench task.
>> That's why he wanted to avoid the __update_load_avg(se) on enqueue (thus
>> adding 1024 to cfs_rq->runnable_load_avg) and do the ageing only on
>> dequeue (removing <1024 from cfs_rq->runnable_load_avg).
>
> ok so i'm a bit confused there
> my understand of your explanation above  is that now we left a small
> amount of load in runnable_load_avg after the dequeue so another cpu
> will be chosen. But this explanation seems to be the opposite of what
> Matt said in a previous email that:
> "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."

sorry forgot my question, i just misread your explanation.

Matt,

May be you can try this patch which uses utilization in
find_idlest_group. So even if runnable_load_avg is null, the
utilization should not and another cpu will be chosen
https://patchwork.kernel.org/patch/9306939/



>
>>
>>

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

#1495609

On Wed, 28 Sep, at 05:00:20AM, Vincent Guittot wrote:
> 
> Matt,
> 
> May be you can try this patch which uses utilization in
> find_idlest_group. So even if runnable_load_avg is null, the
> utilization should not and another cpu will be chosen
> https://patchwork.kernel.org/patch/9306939/

Unfortunately it doesn't restore performance for my tests. I'll dig
into why that is the case tomorrow.

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

#1495586

On Wed, 28 Sep, at 04:46:06AM, Vincent Guittot wrote:
> 
> ok so i'm a bit confused there
> my understand of your explanation above  is that now we left a small
> amount of load in runnable_load_avg after the dequeue so another cpu
> will be chosen. But this explanation seems to be the opposite of what
> Matt said in a previous email that:
> "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."

Right, we want to avoid the performance drop, which we can do by
leaving a small amount of load in runnable_load_avg. I think Dietmar
and me are saying the same thing.

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

#1492567

On 28 September 2016 at 04:31, Dietmar Eggemann
<dietmar.eggemann@arm.com> wrote:
> On 28/09/16 12:19, Peter Zijlstra wrote:
>> On Wed, Sep 28, 2016 at 12:06:43PM +0100, Dietmar Eggemann wrote:
>>> On 28/09/16 11:14, Peter Zijlstra wrote:
>>>> On Fri, Sep 23, 2016 at 12:58:08PM +0100, Matt Fleming wrote:
>
> [...]
>
>>> Not sure what you mean by 'after fixing' but the se is initialized with
>>> a possibly stale 'now' value in post_init_entity_util_avg()->
>>> attach_entity_load_avg() before the clock is updated in
>>> activate_task()->enqueue_task().
>>
>> I meant that after I fix the above issue of calling post_init with a
>> stale clock. So the + update_rq_clock(rq) in the patch.
>
> OK.
>
> [...]
>
>>>> While staring at this, I don't think we can still hit
>>>> vruntime_normalized() with a new task, so I _think_ we can remove that
>>>> !se->sum_exec_runtime clause there (and rejoice), no?
>>>
>>> I'm afraid that with accurate timing we will get the same situation that
>>> we add and subtract the same amount of load (probably 1024 now and not
>>> 1002 (or less)) to/from cfs_rq->runnable_load_avg for the initial (fork)
>>> hackbench run.
>>> After all, it's 'runnable' based.
>>
>> The idea was that since we now update rq clock before post_init and then
>> leave it be, both post_init and enqueue see the exact same timestamp,
>> and the delta is 0, resulting in no aging.
>>
>> Or did I fail to make that happen?
>
> No, but IMHO what Matt wants is ageing for the hackench tasks at the end
> of their fork phase so there is a tiny amount of
> cfs_rq->runnable_load_avg left on cpuX after the fork related dequeue so
> the (load-based) fork-balancer chooses cpuY for the next hackbench task.
> That's why he wanted to avoid the __update_load_avg(se) on enqueue (thus
> adding 1024 to cfs_rq->runnable_load_avg) and do the ageing only on
> dequeue (removing <1024 from cfs_rq->runnable_load_avg).

wanting  cfs_rq->runnable_load_avg to be not null when nothing is
runnable on the cfs_rq seems a bit odd.
We should better take into account cfs_rq->avg.load_avg or the
cfs_rq->avg.util_avg in the select_idlest_group in this case

>
>

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

#1492592

Le Wednesday 28 Sep 2016 à 05:27:54 (-0700), Vincent Guittot a écrit :
> On 28 September 2016 at 04:31, Dietmar Eggemann
> <dietmar.eggemann@arm.com> wrote:
> > On 28/09/16 12:19, Peter Zijlstra wrote:
> >> On Wed, Sep 28, 2016 at 12:06:43PM +0100, Dietmar Eggemann wrote:
> >>> On 28/09/16 11:14, Peter Zijlstra wrote:
> >>>> On Fri, Sep 23, 2016 at 12:58:08PM +0100, Matt Fleming wrote:
> >
> > [...]
> >
> >>> Not sure what you mean by 'after fixing' but the se is initialized with
> >>> a possibly stale 'now' value in post_init_entity_util_avg()->
> >>> attach_entity_load_avg() before the clock is updated in
> >>> activate_task()->enqueue_task().
> >>
> >> I meant that after I fix the above issue of calling post_init with a
> >> stale clock. So the + update_rq_clock(rq) in the patch.
> >
> > OK.
> >
> > [...]
> >
> >>>> While staring at this, I don't think we can still hit
> >>>> vruntime_normalized() with a new task, so I _think_ we can remove that
> >>>> !se->sum_exec_runtime clause there (and rejoice), no?
> >>>
> >>> I'm afraid that with accurate timing we will get the same situation that
> >>> we add and subtract the same amount of load (probably 1024 now and not
> >>> 1002 (or less)) to/from cfs_rq->runnable_load_avg for the initial (fork)
> >>> hackbench run.
> >>> After all, it's 'runnable' based.
> >>
> >> The idea was that since we now update rq clock before post_init and then
> >> leave it be, both post_init and enqueue see the exact same timestamp,
> >> and the delta is 0, resulting in no aging.
> >>
> >> Or did I fail to make that happen?
> >
> > No, but IMHO what Matt wants is ageing for the hackench tasks at the end
> > of their fork phase so there is a tiny amount of
> > cfs_rq->runnable_load_avg left on cpuX after the fork related dequeue so
> > the (load-based) fork-balancer chooses cpuY for the next hackbench task.
> > That's why he wanted to avoid the __update_load_avg(se) on enqueue (thus
> > adding 1024 to cfs_rq->runnable_load_avg) and do the ageing only on
> > dequeue (removing <1024 from cfs_rq->runnable_load_avg).
> 
> wanting  cfs_rq->runnable_load_avg to be not null when nothing is
> runnable on the cfs_rq seems a bit odd.
> We should better take into account cfs_rq->avg.load_avg or the
> cfs_rq->avg.util_avg in the select_idlest_group in this case

IIUC the problem raised by Matt, he see a regression because we now remove
during the dequeue the exact same load as during the enqueue so
cfs_rq->runnable_load_avg is null so we select a cfs_rq that might already have
a lot of hackbench blocked thread.
The fact that runnable_load_avg is null, when the cfs_rq doesn't have runnable
task, is quite correct and we should keep it. But when we look for the idlest
group, we have to take into account the blocked thread.

That's what i have tried to do below


---
 kernel/sched/fair.c | 30 +++++++++++++++++++++++-------
 1 file changed, 23 insertions(+), 7 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 06b3c47..702915e 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -5353,7 +5353,8 @@ find_idlest_group(struct sched_domain *sd, struct task_struct *p,
 		  int this_cpu, int sd_flag)
 {
 	struct sched_group *idlest = NULL, *group = sd->groups;
-	unsigned long min_load = ULONG_MAX, this_load = 0;
+	unsigned long min_runnable_load = ULONG_MAX, this_load = 0;
+	unsigned long min_avg_load = ULONG_MAX;
 	int load_idx = sd->forkexec_idx;
 	int imbalance = 100 + (sd->imbalance_pct-100)/2;
 
@@ -5361,7 +5362,7 @@ find_idlest_group(struct sched_domain *sd, struct task_struct *p,
 		load_idx = sd->wake_idx;
 
 	do {
-		unsigned long load, avg_load;
+		unsigned long load, avg_load, runnable_load;
 		int local_group;
 		int i;
 
@@ -5375,6 +5376,7 @@ find_idlest_group(struct sched_domain *sd, struct task_struct *p,
 
 		/* Tally up the load of all CPUs in the group */
 		avg_load = 0;
+		runnable_load = 0;
 
 		for_each_cpu(i, sched_group_cpus(group)) {
 			/* Bias balancing toward cpus of our domain */
@@ -5383,21 +5385,35 @@ find_idlest_group(struct sched_domain *sd, struct task_struct *p,
 			else
 				load = target_load(i, load_idx);
 
-			avg_load += load;
+			runnable_load += load;
+
+			avg_load += cfs_rq_load_avg(&cpu_rq(i)->cfs); 
 		}
 
 		/* Adjust by relative CPU capacity of the group */
 		avg_load = (avg_load * SCHED_CAPACITY_SCALE) / group->sgc->capacity;
+		runnable_load = (runnable_load * SCHED_CAPACITY_SCALE) / group->sgc->capacity;
 
 		if (local_group) {
-			this_load = avg_load;
-		} else if (avg_load < min_load) {
-			min_load = avg_load;
+			this_load = runnable_load;
+		} else if (runnable_load < min_runnable_load) {
+			min_runnable_load = runnable_load;
+			min_avg_load = avg_load;
+			idlest = group;
+		} else if ((runnable_load == min_runnable_load) && (avg_load < min_avg_load)) {
+		/*
+		 * In case that we have same runnable load (especially null
+		 *  runnable load), we select the group with smallest blocked
+		 *  load
+		 */
+			min_avg_load = avg_load;
+			min_runnable_load = runnable_load;
 			idlest = group;
 		}
+
 	} while (group = group->next, group != sd->groups);
 
-	if (!idlest || 100*this_load < imbalance*min_load)
+	if (!idlest || 100*this_load < imbalance*min_runnable_load)
 		return NULL;
 	return idlest;
 }


> 
> >
> >

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

#1493554

On 28/09/16 14:13, Vincent Guittot wrote:
> Le Wednesday 28 Sep 2016 à 05:27:54 (-0700), Vincent Guittot a écrit :
>> On 28 September 2016 at 04:31, Dietmar Eggemann
>> <dietmar.eggemann@arm.com> wrote:
>>> On 28/09/16 12:19, Peter Zijlstra wrote:
>>>> On Wed, Sep 28, 2016 at 12:06:43PM +0100, Dietmar Eggemann wrote:
>>>>> On 28/09/16 11:14, Peter Zijlstra wrote:
>>>>>> On Fri, Sep 23, 2016 at 12:58:08PM +0100, Matt Fleming wrote:

[...]

> IIUC the problem raised by Matt, he see a regression because we now remove
> during the dequeue the exact same load as during the enqueue so
> cfs_rq->runnable_load_avg is null so we select a cfs_rq that might already have
> a lot of hackbench blocked thread.

This is my understanding as well.

> The fact that runnable_load_avg is null, when the cfs_rq doesn't have runnable
> task, is quite correct and we should keep it. But when we look for the idlest
> group, we have to take into account the blocked thread.
> 
> That's what i have tried to do below

[...]

> +		/*
> +		 * In case that we have same runnable load (especially null
> +		 *  runnable load), we select the group with smallest blocked
> +		 *  load
> +		 */
> +			min_avg_load = avg_load;
> +			min_runnable_load = runnable_load;

Setting 'min_runnable_load' wouldn't be necessary here.

>  			idlest = group;
>  		}
> +
>  	} while (group = group->next, group != sd->groups);
>  
> -	if (!idlest || 100*this_load < imbalance*min_load)
> +	if (!idlest || 100*this_load < imbalance*min_runnable_load)
>  		return NULL;
>  	return idlest;

On the Hikey board (ARM64) (2 cluster, each 4 cpu's, so MC and DIE), the
first f_i_g (on DIE) is still based on rbl_load. So if the first
hackbench task (spawning all the worker task) runs on cluster1, and the
former worker p_X already blocks f_i_g returns cluster2, if p_X still
runs, it returns idlest=NULL and we continue with cluster1 for second
f_i_g on MC.

The additional 'else if' condition doesn't seem to help much because of
occurrences where an idle cpu (which never took a worker) still has a
small value of rbl_load (shouldn't actually happen, weighted_cpuload()
should be 0) so it is never chosen or it has even a negative impact in
the case where an idle cpu (which never took a worker) is not chosen
because its load (cfs->avg.load_avg) hasn't been updated for a long time
so another cpu with rbl_load = 0 and a smaller load is used (even though
a lot of worker where already placed on it).

There are also episodes where we 'pack' workers onto the cpu which is
initially picked in f_i_c (on DIE) because (100*this_load <
imbalance*min_load) is true in f_i_g on MC. Maybe we can get rid of this
for !sd->child ?

[...]

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

#1494874

On 29 September 2016 at 18:15, Dietmar Eggemann
<dietmar.eggemann@arm.com> wrote:
> On 28/09/16 14:13, Vincent Guittot wrote:
>> Le Wednesday 28 Sep 2016 à 05:27:54 (-0700), Vincent Guittot a écrit :
>>> On 28 September 2016 at 04:31, Dietmar Eggemann
>>> <dietmar.eggemann@arm.com> wrote:
>>>> On 28/09/16 12:19, Peter Zijlstra wrote:
>>>>> On Wed, Sep 28, 2016 at 12:06:43PM +0100, Dietmar Eggemann wrote:
>>>>>> On 28/09/16 11:14, Peter Zijlstra wrote:
>>>>>>> On Fri, Sep 23, 2016 at 12:58:08PM +0100, Matt Fleming wrote:
>
> [...]
>
>> IIUC the problem raised by Matt, he see a regression because we now remove
>> during the dequeue the exact same load as during the enqueue so
>> cfs_rq->runnable_load_avg is null so we select a cfs_rq that might already have
>> a lot of hackbench blocked thread.
>
> This is my understanding as well.
>
>> The fact that runnable_load_avg is null, when the cfs_rq doesn't have runnable
>> task, is quite correct and we should keep it. But when we look for the idlest
>> group, we have to take into account the blocked thread.
>>
>> That's what i have tried to do below
>
> [...]
>
>> +             /*
>> +              * In case that we have same runnable load (especially null
>> +              *  runnable load), we select the group with smallest blocked
>> +              *  load
>> +              */
>> +                     min_avg_load = avg_load;
>> +                     min_runnable_load = runnable_load;
>
> Setting 'min_runnable_load' wouldn't be necessary here.

fair enough

>
>>                       idlest = group;
>>               }
>> +
>>       } while (group = group->next, group != sd->groups);
>>
>> -     if (!idlest || 100*this_load < imbalance*min_load)
>> +     if (!idlest || 100*this_load < imbalance*min_runnable_load)
>>               return NULL;
>>       return idlest;
>
> On the Hikey board (ARM64) (2 cluster, each 4 cpu's, so MC and DIE), the
> first f_i_g (on DIE) is still based on rbl_load. So if the first
> hackbench task (spawning all the worker task) runs on cluster1, and the
> former worker p_X already blocks f_i_g returns cluster2, if p_X still
> runs, it returns idlest=NULL and we continue with cluster1 for second
> f_i_g on MC.
>
> The additional 'else if' condition doesn't seem to help much because of
> occurrences where an idle cpu (which never took a worker) still has a
> small value of rbl_load (shouldn't actually happen, weighted_cpuload()
> should be 0) so it is never chosen or it has even a negative impact in
> the case where an idle cpu (which never took a worker) is not chosen
> because its load (cfs->avg.load_avg) hasn't been updated for a long time
> so another cpu with rbl_load = 0 and a smaller load is used (even though
> a lot of worker where already placed on it).

So the elseif part is there to take care of the regression raised by
Matt where the runnable_load_avg is null because worker are blocked
and the same cpu is selected
This can be extended with a threshold in order to include small
differences that came from computation rounding

>
> There are also episodes where we 'pack' workers onto the cpu which is
> initially picked in f_i_c (on DIE) because (100*this_load <
> imbalance*min_load) is true in f_i_g on MC. Maybe we can get rid of this
> for !sd->child ?

This threshold is there to filter any small variations that are not
relevant. I'm going to extend the use of cfs_rq_load_avg() in all
conditions so we take into account blocked load everywhere instead of
only when runnable_load_avg is null

>
> [...]

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

#1492810

On 28/09/16 12:19, Peter Zijlstra wrote:
> On Wed, Sep 28, 2016 at 12:06:43PM +0100, Dietmar Eggemann wrote:
>> On 28/09/16 11:14, Peter Zijlstra wrote:
>>> On Fri, Sep 23, 2016 at 12:58:08PM +0100, Matt Fleming wrote:

[...]

>> I'm afraid that with accurate timing we will get the same situation that
>> we add and subtract the same amount of load (probably 1024 now and not
>> 1002 (or less)) to/from cfs_rq->runnable_load_avg for the initial (fork)
>> hackbench run.
>> After all, it's 'runnable' based.
> 
> The idea was that since we now update rq clock before post_init and then
> leave it be, both post_init and enqueue see the exact same timestamp,
> and the delta is 0, resulting in no aging.
> 
> Or did I fail to make that happen?

No, you're right the task load ages from 1024 (enqueue) to something
between 1002 and 1024 in (dequeue) for the initial fork-phase.

The call to __update_load_avg() in enqueue_task_fair() is now always
done with 'delta = now - sa->last_update_time' equal 0 so we bail out.

The following call to __update_load_avg() (from dequeue_task_fair(), or
set_next_entity() or even task_tick_fair()) let us enter the 'decayed =
1' path (even for a short runtime (>1us) since the initial value for
period_contrib is 1023 and with the initial values of load_avg=1024 and
load_sum = 1024*47742 = 48,887,808 (and a runtime < 1001us, so contrib
stays 0) we end up decaying load_avg to something between 1002 and 1024.

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

#1492928

On Wed, 28 Sep, at 12:14:22PM, Peter Zijlstra wrote:
> 
> Which suggests we do something like the below (not compile tested or
> anything, also I ran out of tea again).

I'm away on FTO right now. I can test this when I return on Friday.

Funnily enough, I now remember that I already sent a fix for the
missing update_rq_clock() in post_init_entity_util_avg(), but didn't
apply it when chasing this hackbench regression (oops),

  https://lkml.kernel.org/r/20160921133813.31976-3-matt@codeblueprint.co.uk

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

#1494261

On Wed, 28 Sep, at 08:37:31PM, Matt Fleming wrote:
> 
> I'm away on FTO right now. I can test this when I return on Friday.

I haven't had chance to review your patch or the other emails in this
thread yet, but I ran the patch on my test machine and it also
restores performance. I'll run it through the test grid on Monday.

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

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web