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 2 of 2 — ← Prev page 1 [2]

#1498333

On 10/10/16 13:29, Vincent Guittot wrote:
> 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

Wasn't this about the two patches mentioned in this thread? The one from
Matt using 'se->sum_exec_runtime' in the if condition in
enqueue_entity_load_avg() and Peterz's conditional call to
update_rq_clock(rq) in enqueue_task()?

> 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

Hikey  (ARM64 2x4 cpus) board: cpufreq: performance, cpuidle: disabled

Performance counter stats for 'perf bench sched messaging -g 20 -l 500'
(10 runs):

(1) tip/sched/core: commit 447976ef4fd0

    5.902209533 seconds time elapsed ( +- 0.31% )

(2) tip/sched/core + original patch on the 'sched/fair: Do not decay
    new task load on first enqueue' thread (23/09/16)

    5.919933030 seconds time elapsed ( +- 0.44% )

(3) tip/sched/core + Peter's ENQUEUE_NEW patch on the 'sched/fair: Do
    not decay new task load on first enqueue' thread (28/09/16)

    5.970195534 seconds time elapsed ( +- 0.37% )

Not sure if we can call this a regression but it also shows no
performance gain.

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

We carry two trace events locally, one for PELT on se and one for
cfs_rq's (I have to add the runnable bits here) which work for
CONFIG_FAIR_GROUP_SCHED and !CONFIG_FAIR_GROUP_SCHED. I put them into
__update_load_avg(), attach_entity_load_avg() and
detach_entity_load_avg(). I could post them but so far mainline has been
reluctant to see the need for PELT related trace events ...

[...]

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

#1498476

On 10 October 2016 at 15:54, Dietmar Eggemann <dietmar.eggemann@arm.com> wrote:
> On 10/10/16 13:29, Vincent Guittot wrote:
>> 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
>
> Wasn't this about the two patches mentioned in this thread? The one from
> Matt using 'se->sum_exec_runtime' in the if condition in
> enqueue_entity_load_avg() and Peterz's conditional call to
> update_rq_clock(rq) in enqueue_task()?

I was trying to reproduce the regression that Matt mentioned at the
beg of the thread not those linked to proposed fixes

>
>> 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
>
> Hikey  (ARM64 2x4 cpus) board: cpufreq: performance, cpuidle: disabled
>
> Performance counter stats for 'perf bench sched messaging -g 20 -l 500'
> (10 runs):
>
> (1) tip/sched/core: commit 447976ef4fd0
>
>     5.902209533 seconds time elapsed ( +- 0.31% )

This seems to be too long to test the impact of the forking phase of hackbench

>
> (2) tip/sched/core + original patch on the 'sched/fair: Do not decay
>     new task load on first enqueue' thread (23/09/16)
>
>     5.919933030 seconds time elapsed ( +- 0.44% )
>
> (3) tip/sched/core + Peter's ENQUEUE_NEW patch on the 'sched/fair: Do
>     not decay new task load on first enqueue' thread (28/09/16)
>
>     5.970195534 seconds time elapsed ( +- 0.37% )
>
> Not sure if we can call this a regression but it also shows no
> performance gain.
>
>>>
>>> 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
>
> We carry two trace events locally, one for PELT on se and one for
> cfs_rq's (I have to add the runnable bits here) which work for
> CONFIG_FAIR_GROUP_SCHED and !CONFIG_FAIR_GROUP_SCHED. I put them into
> __update_load_avg(), attach_entity_load_avg() and
> detach_entity_load_avg(). I could post them but so far mainline has been
> reluctant to see the need for PELT related trace events ...
>
> [...]

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

#1498693

On 10/10/16 19:29, Vincent Guittot wrote:
> On 10 October 2016 at 15:54, Dietmar Eggemann <dietmar.eggemann@arm.com> wrote:
>> On 10/10/16 13:29, Vincent Guittot wrote:
>>> 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:

[...]

>>> 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
>>
>> Wasn't this about the two patches mentioned in this thread? The one from
>> Matt using 'se->sum_exec_runtime' in the if condition in
>> enqueue_entity_load_avg() and Peterz's conditional call to
>> update_rq_clock(rq) in enqueue_task()?
> 
> I was trying to reproduce the regression that Matt mentioned at the
> beg of the thread not those linked to proposed fixes

OK.

> 
>>
>>> 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
>>
>> Hikey  (ARM64 2x4 cpus) board: cpufreq: performance, cpuidle: disabled
>>
>> Performance counter stats for 'perf bench sched messaging -g 20 -l 500'
>> (10 runs):
>>
>> (1) tip/sched/core: commit 447976ef4fd0
>>
>>     5.902209533 seconds time elapsed ( +- 0.31% )
> 
> This seems to be too long to test the impact of the forking phase of hackbench

[...]

Yeah, you're right. But I can't see any significant difference. IMHO,
it's all in the noise.

(A) Performance counter stats for 'perf bench sched messaging -g 100 -l
    1 -t'
    # 20 sender and receiver threads per group
    # 100 groups == 4000 threads run

(1) tip/sched/core: commit 447976ef4fd0

    Total time: 0.188 [sec]

(2) tip/sched/core + original patch on the 'sched/fair: Do not decay
    new task load on first enqueue' thread (23/09/16)

    Total time: 0.199 [sec]

(3) tip/sched/core + Peter's ENQUEUE_NEW patch on the 'sched/fair: Do
    not decay new task load on first enqueue' thread (28/09/16)

    Total time: 0.178 [sec]

(B) hackbench -P -g 1
    Running in process mode with 1 groups using 40 file descriptors
    each (== 40 tasks)
    Each sender will pass 100 messages of 100 bytes

(1) 0.067

(2) 0.083

(3) 0.073

(C) hackbench -T -g 1
    Running in threaded mode with 1 groups using 40 file descriptors
    each (== 40 tasks)
    Each sender will pass 100 messages of 100 bytes

(1) 0.077

(2) 0.079

(3) 0.072

Maybe, instead of the performance gov, I should pin the frequency to a
lower one to eliminate the thermal influence on this Hikey board.

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

#1498748

On Tue, 11 Oct, at 10:44:25AM, Dietmar Eggemann wrote:
> 
> [...]
> 
> Yeah, you're right. But I can't see any significant difference. IMHO,
> it's all in the noise.
> 
> (A) Performance counter stats for 'perf bench sched messaging -g 100 -l
>     1 -t'
>     # 20 sender and receiver threads per group
>     # 100 groups == 4000 threads run
> 

FWIW, our tests run with 1000 loops, not 1, and we don't use 100
groups for low cpu machines. We tend to test upto $((nproc * 4)).

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

#1498448

Le Monday 10 Oct 2016 à 14:29:28 (+0200), Vincent Guittot a écrit :
> 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
>

Subject: [PATCH] sched: use load_avg for selecting idlest group

select_busiest_group only compares the runnable_load_avg when looking for
the idlest group. But on fork intensive use case like hackbenchw here task
blocked quickly after the fork, this can lead to selecting the same CPU
whereas other CPUs, which have similar runnable load but a lower load_avg,
could be chosen instead.

When the runnable_load_avg of 2 CPUs are close, we now take into account
the amount of blocked load as a 2nd selection factor.

For use case like hackbench, this enable the scheduler to select different
CPUs during the fork sequence and to spread tasks across the system.

Tests have been done on a Hikey board (ARM based octo cores) for several
kernel. The result below gives min, max, avg and stdev values of 18 runs
with each configuration.

The v4.8+patches configuration also includes the changes below which is part of the
proposal made by Peter to ensure that the clock will be up to date when the
fork task will be attached to the rq.

@@ -2568,6 +2568,7 @@ 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, 0);

hackbench -P -g 1 

       ea86cb4b7621  7dc603c9028e  v4.8        v4.8+patches
min    0.049         0.050         0.051       0,048
avg    0.057         0.057(0%)     0.057(0%)   0,055(+5%)
max    0.066         0.068         0.070       0,063
stdev  +/-9%         +/-9%         +/-8%       +/-9%

Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
---
 kernel/sched/fair.c | 40 ++++++++++++++++++++++++++++++++--------
 1 file changed, 32 insertions(+), 8 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 039de34..628b00b 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -5166,15 +5166,16 @@ 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_runnable_load = 0;
+	unsigned long min_avg_load = ULONG_MAX, this_avg_load = 0;
 	int load_idx = sd->forkexec_idx;
-	int imbalance = 100 + (sd->imbalance_pct-100)/2;
+	unsigned long imbalance = (scale_load_down(NICE_0_LOAD)*(sd->imbalance_pct-100))/100;
 
 	if (sd_flag & SD_BALANCE_WAKE)
 		load_idx = sd->wake_idx;
 
 	do {
-		unsigned long load, avg_load;
+		unsigned long load, avg_load, runnable_load;
 		int local_group;
 		int i;
 
@@ -5188,6 +5189,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 */
@@ -5196,21 +5198,43 @@ 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_runnable_load = runnable_load;
+			this_avg_load = avg_load;
+		} else if (min_runnable_load > (runnable_load + imbalance)) {
+			/*
+			 * The runnable load is significantly smaller so we
+			 * can pick this new cpu
+			 */
+			min_runnable_load = runnable_load;
+			min_avg_load = avg_load;
+			idlest = group;
+		} else if ((runnable_load < (min_runnable_load + imbalance)) &&
+				(100*min_avg_load > sd->imbalance_pct*avg_load)) {
+			/*
+			 * The runnable loads are close so we take into account
+			 * blocked load throught avg_load which is blocked +
+			 * runnable load
+			 */
+			min_avg_load = avg_load;
 			idlest = group;
 		}
+
 	} while (group = group->next, group != sd->groups);
 
-	if (!idlest || 100*this_load < imbalance*min_load)
+	if (!idlest ||
+	    (min_runnable_load > (this_runnable_load + imbalance)) ||
+	    ((this_runnable_load < (min_runnable_load + imbalance)) &&
+			(100*min_avg_load > sd->imbalance_pct*this_avg_load)))
 		return NULL;
 	return idlest;
 }
-- 
2.7.4

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

#1498742

On Mon, 10 Oct, at 07:34:40PM, Vincent Guittot wrote:
> 
> Subject: [PATCH] sched: use load_avg for selecting idlest group
> 
> select_busiest_group only compares the runnable_load_avg when looking for
> the idlest group. But on fork intensive use case like hackbenchw here task
> blocked quickly after the fork, this can lead to selecting the same CPU
> whereas other CPUs, which have similar runnable load but a lower load_avg,
> could be chosen instead.
> 
> When the runnable_load_avg of 2 CPUs are close, we now take into account
> the amount of blocked load as a 2nd selection factor.
> 
> For use case like hackbench, this enable the scheduler to select different
> CPUs during the fork sequence and to spread tasks across the system.
> 
> Tests have been done on a Hikey board (ARM based octo cores) for several
> kernel. The result below gives min, max, avg and stdev values of 18 runs
> with each configuration.
> 
> The v4.8+patches configuration also includes the changes below which is part of the
> proposal made by Peter to ensure that the clock will be up to date when the
> fork task will be attached to the rq.
> 
> @@ -2568,6 +2568,7 @@ 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, 0);
> 
> hackbench -P -g 1 
> 
>        ea86cb4b7621  7dc603c9028e  v4.8        v4.8+patches
> min    0.049         0.050         0.051       0,048
> avg    0.057         0.057(0%)     0.057(0%)   0,055(+5%)
> max    0.066         0.068         0.070       0,063
> stdev  +/-9%         +/-9%         +/-8%       +/-9%
> 
> Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
> ---
>  kernel/sched/fair.c | 40 ++++++++++++++++++++++++++++++++--------
>  1 file changed, 32 insertions(+), 8 deletions(-)

This patch looks pretty good to me and this 2-socket 48-cpu Xeon
(domain0 SMT, domain1 MC, domain2 NUMA) shows a few nice performance
improvements, and no regressions for various combinations of hackbench
sockets/pipes and group numbers.

But on a 2-socket 8-cpu Xeon (domain0 MC, domain1 DIE) running,

  perf stat --null -r 25 -- hackbench -pipe 30 process 1000

I see a regression,

  baseline: 2.41228
  patched : 2.64528 (-9.7%)

Even though the spread of tasks during fork[0] is improved,

  baseline CV: 0.478%
  patched CV : 0.042%

Clearly the spread wasn't *that* bad to begin with on this machine for
this workload. I consider the baseline spread to be pretty well
distributed. Some other factor must be at play.

Patched runqueue latencies are higher (max9* are percentiles),

  baseline: mean: 615932.69 max90: 75272.00 max95: 175985.00 max99: 5884778.00 max: 1694084747.00 
  patched: mean : 882026.28 max90: 92015.00 max95: 291760.00 max99: 7590167.00 max: 1841154776.00

And there are more migrations of hackbench tasks,

  baseline: total: 5390 cross-MC: 3810 cross-DIE: 1580
  patched : total: 7222 cross-MC: 4591 cross-DIE: 2631
                 (+34.0%)       (+20.5%)        (+66.5%)

That's a lot more costly cross-DIE migrations. I think this patch is
along the right lines, but there's something fishy happening on this
box.

[0] - Fork task placement spread measurement:

      cat /tmp/trace.$1 | grep -E "wakeup_new.*comm=hackbench" | \
	sed -e 's/.*target_cpu=//' | sort | uniq -c | awk '{print $1}' 

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

#1498878

On 11 October 2016 at 12:24, Matt Fleming <matt@codeblueprint.co.uk> wrote:
> On Mon, 10 Oct, at 07:34:40PM, Vincent Guittot wrote:
>>
>> Subject: [PATCH] sched: use load_avg for selecting idlest group
>>
>> select_busiest_group only compares the runnable_load_avg when looking for
>> the idlest group. But on fork intensive use case like hackbenchw here task
>> blocked quickly after the fork, this can lead to selecting the same CPU
>> whereas other CPUs, which have similar runnable load but a lower load_avg,
>> could be chosen instead.
>>
>> When the runnable_load_avg of 2 CPUs are close, we now take into account
>> the amount of blocked load as a 2nd selection factor.
>>
>> For use case like hackbench, this enable the scheduler to select different
>> CPUs during the fork sequence and to spread tasks across the system.
>>
>> Tests have been done on a Hikey board (ARM based octo cores) for several
>> kernel. The result below gives min, max, avg and stdev values of 18 runs
>> with each configuration.
>>
>> The v4.8+patches configuration also includes the changes below which is part of the
>> proposal made by Peter to ensure that the clock will be up to date when the
>> fork task will be attached to the rq.
>>
>> @@ -2568,6 +2568,7 @@ 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, 0);
>>
>> hackbench -P -g 1
>>
>>        ea86cb4b7621  7dc603c9028e  v4.8        v4.8+patches
>> min    0.049         0.050         0.051       0,048
>> avg    0.057         0.057(0%)     0.057(0%)   0,055(+5%)
>> max    0.066         0.068         0.070       0,063
>> stdev  +/-9%         +/-9%         +/-8%       +/-9%
>>
>> Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
>> ---
>>  kernel/sched/fair.c | 40 ++++++++++++++++++++++++++++++++--------
>>  1 file changed, 32 insertions(+), 8 deletions(-)
>
> This patch looks pretty good to me and this 2-socket 48-cpu Xeon
> (domain0 SMT, domain1 MC, domain2 NUMA) shows a few nice performance
> improvements, and no regressions for various combinations of hackbench
> sockets/pipes and group numbers.

Good

>
> But on a 2-socket 8-cpu Xeon (domain0 MC, domain1 DIE) running,
>
>   perf stat --null -r 25 -- hackbench -pipe 30 process 1000

I'm going to have a look at this use case on my hikey board made 8
CPUs (2 cluster of quad core) (domain0 MC, domain1 DIE) which seems to
have a similar topology

>
> I see a regression,
>
>   baseline: 2.41228
>   patched : 2.64528 (-9.7%)

Just to be sure; By baseline you mean v4.8 ?

>
> Even though the spread of tasks during fork[0] is improved,
>
>   baseline CV: 0.478%
>   patched CV : 0.042%
>
> Clearly the spread wasn't *that* bad to begin with on this machine for
> this workload. I consider the baseline spread to be pretty well
> distributed. Some other factor must be at play.
>
> Patched runqueue latencies are higher (max9* are percentiles),
>
>   baseline: mean: 615932.69 max90: 75272.00 max95: 175985.00 max99: 5884778.00 max: 1694084747.00
>   patched: mean : 882026.28 max90: 92015.00 max95: 291760.00 max99: 7590167.00 max: 1841154776.00
>
> And there are more migrations of hackbench tasks,
>
>   baseline: total: 5390 cross-MC: 3810 cross-DIE: 1580
>   patched : total: 7222 cross-MC: 4591 cross-DIE: 2631
>                  (+34.0%)       (+20.5%)        (+66.5%)
>
> That's a lot more costly cross-DIE migrations. I think this patch is
> along the right lines, but there's something fishy happening on this
> box.
>
> [0] - Fork task placement spread measurement:
>
>       cat /tmp/trace.$1 | grep -E "wakeup_new.*comm=hackbench" | \
>         sed -e 's/.*target_cpu=//' | sort | uniq -c | awk '{print $1}'

nice command to evaluate spread

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

#1499159

On Tue, 11 Oct, at 03:14:47PM, Vincent Guittot wrote:
> >
> > I see a regression,
> >
> >   baseline: 2.41228
> >   patched : 2.64528 (-9.7%)
> 
> Just to be sure; By baseline you mean v4.8 ?
 
Baseline is actually tip/sched/core commit 447976ef4fd0
("sched/irqtime: Consolidate irqtime flushing code") but I could try
out v4.8 instead if you'd prefer that.

> >       cat /tmp/trace.$1 | grep -E "wakeup_new.*comm=hackbench" | \
> >         sed -e 's/.*target_cpu=//' | sort | uniq -c | awk '{print $1}'
> 
> nice command to evaluate spread

Thanks!

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

#1499407

On 11 October 2016 at 20:57, Matt Fleming <matt@codeblueprint.co.uk> wrote:
> On Tue, 11 Oct, at 03:14:47PM, Vincent Guittot wrote:
>> >
>> > I see a regression,
>> >
>> >   baseline: 2.41228
>> >   patched : 2.64528 (-9.7%)
>>
>> Just to be sure; By baseline you mean v4.8 ?
>
> Baseline is actually tip/sched/core commit 447976ef4fd0
> ("sched/irqtime: Consolidate irqtime flushing code") but I could try
> out v4.8 instead if you'd prefer that.

ok. In fact, I have noticed another regression with tip/sched/core and
hackbench while looking at yours.
I have bisect to :
10e2f1acd0 ("sched/core: Rewrite and improve select_idle_siblings")

hackbench -P -g 1

       v4.8        tip/sched/core  tip/sched/core+revert 10e2f1acd010
and 1b568f0aabf2
min 0.051       0,052               0.049
avg 0.057(0%)   0,062(-7%)   0.056(+1%)
max 0.070       0,073      0.067
stdev  +/-8%       +/-10%    +/-9%

The issue seems to be that it prevents some migration at wake up at
the end of hackbench test so we have last tasks that compete for the
same CPU whereas other CPUs are idle in the same MC domain. I haven't
to look more deeply which part of the patch do the regression yet

 >
>> >       cat /tmp/trace.$1 | grep -E "wakeup_new.*comm=hackbench" | \
>> >         sed -e 's/.*target_cpu=//' | sort | uniq -c | awk '{print $1}'
>>
>> nice command to evaluate spread
>
> Thanks!

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

#1495585

On Wed, 28 Sep, at 12:14:22PM, 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.
> 
> 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).
 
This patch causes some low cpu machines (4 or 8) to regress. It turns
out they regress with my patch too. I'm running the below patch
without the enqueue_task() hunk now to see if that makes a difference.

> 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?

Looks that way to me, yeah.

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web