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

Groups > linux.kernel > #1492513 > 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 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 →

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

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] | [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]

#1497856

2016-09-29 3:37 GMT+08:00 Matt Fleming <matt@codeblueprint.co.uk>:
> 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

The difference between this patch and Peterz's is your patch have a
delta since activate_task()->enqueue_task() does do update_rq_clock(),
so why don't have the delta will cause low cpu machines (4 or 8) to
regress against your another reply in this thread?

Regards,
Wanpeng Li

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

#1498205

On Sun, 09 Oct, at 11:39:27AM, Wanpeng Li wrote:
> 
> The difference between this patch and Peterz's is your patch have a
> delta since activate_task()->enqueue_task() does do update_rq_clock(),
> so why don't have the delta will cause low cpu machines (4 or 8) to
> regress against your another reply in this thread?

Both my patch and Peter's patch cause issues with low cpu machines. In
<20161004201105.GP16071@codeblueprint.co.uk> I said,

 "This patch causes some low cpu machines (4 or 8) to regress. It turns
  out they regress with my patch too."

Have I misunderstood your question?

I ran out of time to investigate this last week, though I did try all
proposed patches, including Vincent's, and none of them produced wins
across the board.

I should get a bit further this week.

Vincent, Dietmar, did you guys ever get around to submitting your PELT
tracepoint patches? Getting some introspection into the scheduler's
load balancing decisions would speed up this sort of research.

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

#1498216

2016-10-10 18:01 GMT+08:00 Matt Fleming <matt@codeblueprint.co.uk>:
> On Sun, 09 Oct, at 11:39:27AM, Wanpeng Li wrote:
>>
>> The difference between this patch and Peterz's is your patch have a
>> delta since activate_task()->enqueue_task() does do update_rq_clock(),
>> so why don't have the delta will cause low cpu machines (4 or 8) to
>> regress against your another reply in this thread?
>
> Both my patch and Peter's patch cause issues with low cpu machines. In
> <20161004201105.GP16071@codeblueprint.co.uk> I said,
>
>  "This patch causes some low cpu machines (4 or 8) to regress. It turns
>   out they regress with my patch too."
>
> Have I misunderstood your question?

I compare this one https://marc.info/?l=linux-kernel&m=147446511924571
with Peterz's patch.

>
> I ran out of time to investigate this last week, though I did try all
> proposed patches, including Vincent's, and none of them produced wins
> across the board.
>
> I should get a bit further this week.
>
> Vincent, Dietmar, did you guys ever get around to submitting your PELT
> tracepoint patches? Getting some introspection into the scheduler's
> load balancing decisions would speed up this sort of research.

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

#1498736

On Mon, 10 Oct, at 06:09:14PM, Wanpeng Li wrote:
> 2016-10-10 18:01 GMT+08:00 Matt Fleming <matt@codeblueprint.co.uk>:
> > On Sun, 09 Oct, at 11:39:27AM, Wanpeng Li wrote:
> >>
> >> The difference between this patch and Peterz's is your patch have a
> >> delta since activate_task()->enqueue_task() does do update_rq_clock(),
> >> so why don't have the delta will cause low cpu machines (4 or 8) to
> >> regress against your another reply in this thread?
> >
> > Both my patch and Peter's patch cause issues with low cpu machines. In
> > <20161004201105.GP16071@codeblueprint.co.uk> I said,
> >
> >  "This patch causes some low cpu machines (4 or 8) to regress. It turns
> >   out they regress with my patch too."
> >
> > Have I misunderstood your question?
> 
> I compare this one https://marc.info/?l=linux-kernel&m=147446511924571
> with Peterz's patch.

Oh. Low cpu machines probably do regress with my patch. At least, I
can't find the notes to prove that they don't.

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

#1498270

On 10 October 2016 at 12:01, Matt Fleming <matt@codeblueprint.co.uk> wrote:
> On Sun, 09 Oct, at 11:39:27AM, Wanpeng Li wrote:
>>
>> The difference between this patch and Peterz's is your patch have a
>> delta since activate_task()->enqueue_task() does do update_rq_clock(),
>> so why don't have the delta will cause low cpu machines (4 or 8) to
>> regress against your another reply in this thread?
>
> Both my patch and Peter's patch cause issues with low cpu machines. In
> <20161004201105.GP16071@codeblueprint.co.uk> I said,
>
>  "This patch causes some low cpu machines (4 or 8) to regress. It turns
>   out they regress with my patch too."
>
> Have I misunderstood your question?
>
> I ran out of time to investigate this last week, though I did try all
> proposed patches, including Vincent's, and none of them produced wins
> across the board.

I have tried to reprocude your issue on my target an hikey board (ARM
based octo cores) but i failed to see a regression with commit
7dc603c9028e. Neverthless, i can see tasks not been well  spread
during fork as you mentioned. So I have studied a bit more the
spreading issue during fork last week and i have a new version of my
proposed patch that i'm going to send soon. With this patch, i can see
a good spread of tasks  during the fork sequence and some kind of perf
improvement even if it's bit difficult as the variance is quite
important with hackbench test so it's mainly an improvement of
repeatability of the result

>
> I should get a bit further this week.
>
> Vincent, Dietmar, did you guys ever get around to submitting your PELT
> tracepoint patches? Getting some introspection into the scheduler's

My tarcepoint are not in a shape to be submitted and would need a
cleanup as some are more hacks for debugging than real trace events.
Nevertheless, i can push them on a git branch if they can be useful
for someone


> load balancing decisions would speed up this sort of research.

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

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web