Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1500174 > unrolled thread
| Started by | Vincent Guittot <vincent.guittot@linaro.org> |
|---|---|
| First post | 2016-10-13 13:10 +0200 |
| Last post | 2016-10-18 14:10 +0200 |
| Articles | 15 on this page of 35 — 6 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [v4.8-rc1 Regression] sched/fair: Apply more PELT fixes Vincent Guittot <vincent.guittot@linaro.org> - 2016-10-13 13:10 +0200
Re: [v4.8-rc1 Regression] sched/fair: Apply more PELT fixes Joseph Salisbury <joseph.salisbury@canonical.com> - 2016-10-13 18:00 +0200
Re: [v4.8-rc1 Regression] sched/fair: Apply more PELT fixes Vincent Guittot <vincent.guittot@linaro.org> - 2016-10-13 19:00 +0200
Re: [v4.8-rc1 Regression] sched/fair: Apply more PELT fixes Dietmar Eggemann <dietmar.eggemann@arm.com> - 2016-10-13 21:00 +0200
Re: [v4.8-rc1 Regression] sched/fair: Apply more PELT fixes Vincent Guittot <vincent.guittot@linaro.org> - 2016-10-13 23:40 +0200
Re: [v4.8-rc1 Regression] sched/fair: Apply more PELT fixes Vincent Guittot <vincent.guittot@linaro.org> - 2016-10-14 10:30 +0200
Re: [v4.8-rc1 Regression] sched/fair: Apply more PELT fixes Dietmar Eggemann <dietmar.eggemann@arm.com> - 2016-10-14 15:20 +0200
Re: [v4.8-rc1 Regression] sched/fair: Apply more PELT fixes Vincent Guittot <vincent.guittot@linaro.org> - 2016-10-14 17:20 +0200
Re: [v4.8-rc1 Regression] sched/fair: Apply more PELT fixes Joseph Salisbury <joseph.salisbury@canonical.com> - 2016-10-14 18:10 +0200
Re: [v4.8-rc1 Regression] sched/fair: Apply more PELT fixes Vincent Guittot <vincent.guittot@linaro.org> - 2016-10-17 11:20 +0200
Re: [v4.8-rc1 Regression] sched/fair: Apply more PELT fixes Dietmar Eggemann <dietmar.eggemann@arm.com> - 2016-10-17 14:00 +0200
Re: [v4.8-rc1 Regression] sched/fair: Apply more PELT fixes Peter Zijlstra <peterz@infradead.org> - 2016-10-17 15:30 +0200
Re: [v4.8-rc1 Regression] sched/fair: Apply more PELT fixes Vincent Guittot <vincent.guittot@linaro.org> - 2016-10-17 16:00 +0200
Re: [v4.8-rc1 Regression] sched/fair: Apply more PELT fixes Dietmar Eggemann <dietmar.eggemann@arm.com> - 2016-10-18 01:00 +0200
Re: [v4.8-rc1 Regression] sched/fair: Apply more PELT fixes Vincent Guittot <vincent.guittot@linaro.org> - 2016-10-18 10:50 +0200
Re: [v4.8-rc1 Regression] sched/fair: Apply more PELT fixes Peter Zijlstra <peterz@infradead.org> - 2016-10-18 11:10 +0200
Re: [v4.8-rc1 Regression] sched/fair: Apply more PELT fixes Vincent Guittot <vincent.guittot@linaro.org> - 2016-10-18 11:50 +0200
Re: [v4.8-rc1 Regression] sched/fair: Apply more PELT fixes Peter Zijlstra <peterz@infradead.org> - 2016-10-18 12:40 +0200
Re: [v4.8-rc1 Regression] sched/fair: Apply more PELT fixes Vincent Guittot <vincent.guittot@linaro.org> - 2016-10-18 14:00 +0200
Re: [v4.8-rc1 Regression] sched/fair: Apply more PELT fixes Joonwoo Park <joonwoop@codeaurora.org> - 2016-10-19 00:00 +0200
Re: [v4.8-rc1 Regression] sched/fair: Apply more PELT fixes Vincent Guittot <vincent.guittot@linaro.org> - 2016-10-19 08:50 +0200
Re: [v4.8-rc1 Regression] sched/fair: Apply more PELT fixes Vincent Guittot <vincent.guittot@linaro.org> - 2016-10-19 16:50 +0200
Re: [v4.8-rc1 Regression] sched/fair: Apply more PELT fixes Joseph Salisbury <joseph.salisbury@canonical.com> - 2016-10-19 17:00 +0200
Re: [v4.8-rc1 Regression] sched/fair: Apply more PELT fixes Vincent Guittot <vincent.guittot@linaro.org> - 2016-10-19 17:00 +0200
Re: [v4.8-rc1 Regression] sched/fair: Apply more PELT fixes Dietmar Eggemann <dietmar.eggemann@arm.com> - 2016-10-19 17:40 +0200
Re: [v4.8-rc1 Regression] sched/fair: Apply more PELT fixes Joonwoo Park <joonwoop@codeaurora.org> - 2016-10-19 19:40 +0200
Re: [v4.8-rc1 Regression] sched/fair: Apply more PELT fixes Vincent Guittot <vincent.guittot@linaro.org> - 2016-10-19 20:00 +0200
Re: [v4.8-rc1 Regression] sched/fair: Apply more PELT fixes Vincent Guittot <vincent.guittot@linaro.org> - 2016-10-19 17:50 +0200
Re: [v4.8-rc1 Regression] sched/fair: Apply more PELT fixes Dietmar Eggemann <dietmar.eggemann@arm.com> - 2016-10-19 17:50 +0200
Re: [v4.8-rc1 Regression] sched/fair: Apply more PELT fixes Peter Zijlstra <peterz@infradead.org> - 2016-10-19 18:20 +0200
Re: [v4.8-rc1 Regression] sched/fair: Apply more PELT fixes Morten Rasmussen <morten.rasmussen@arm.com> - 2016-10-19 18:40 +0200
Re: [v4.8-rc1 Regression] sched/fair: Apply more PELT fixes Vincent Guittot <vincent.guittot@linaro.org> - 2016-10-19 19:50 +0200
Re: [v4.8-rc1 Regression] sched/fair: Apply more PELT fixes Morten Rasmussen <morten.rasmussen@arm.com> - 2016-10-20 10:00 +0200
Re: [v4.8-rc1 Regression] sched/fair: Apply more PELT fixes Dietmar Eggemann <dietmar.eggemann@arm.com> - 2016-10-18 13:20 +0200
Re: [v4.8-rc1 Regression] sched/fair: Apply more PELT fixes Peter Zijlstra <peterz@infradead.org> - 2016-10-18 14:10 +0200
Page 2 of 2 — ← Prev page 1 [2]
| From | Vincent Guittot <vincent.guittot@linaro.org> |
|---|---|
| Date | 2016-10-19 08:50 +0200 |
| Message-ID | <stSRj-5NC-13@gated-at.bofh.it> |
| In reply to | #1503387 |
On 18 October 2016 at 23:58, Joonwoo Park <joonwoop@codeaurora.org> wrote:
>
>
> On 10/18/2016 04:56 AM, Vincent Guittot wrote:
>>
>> Le Tuesday 18 Oct 2016 à 12:34:12 (+0200), Peter Zijlstra a écrit :
>>>
>>> On Tue, Oct 18, 2016 at 11:45:48AM +0200, Vincent Guittot wrote:
>>>>
>>>> On 18 October 2016 at 11:07, Peter Zijlstra <peterz@infradead.org>
>>>> wrote:
>>>>>
>>>>> So aside from funny BIOSes, this should also show up when creating
>>>>> cgroups when you have offlined a few CPUs, which is far more common I'd
>>>>> think.
>>>>
>>>>
>>>> The problem is also that the load of the tg->se[cpu] that represents
>>>> the tg->cfs_rq[cpu] is initialized to 1024 in:
>>>> alloc_fair_sched_group
>>>> for_each_possible_cpu(i) {
>>>> init_entity_runnable_average(se);
>>>> sa->load_avg = scale_load_down(se->load.weight);
>>>>
>>>> Initializing sa->load_avg to 1024 for a newly created task makes
>>>> sense as we don't know yet what will be its real load but i'm not sure
>>>> that we have to do the same for se that represents a task group. This
>>>> load should be initialized to 0 and it will increase when task will be
>>>> moved/attached into task group
>>>
>>>
>>> Yes, I think that makes sense, not sure how horrible that is with the
>>
>>
>> That should not be that bad because this initial value is only useful for
>> the few dozens of ms that follow the creation of the task group
>>
>>>
>>> current state of things, but after your propagate patch, that
>>> reinstates the interactivity hack that should work for sure.
>>
>>
>> The patch below fixes the issue on my platform:
>>
>> Dietmar, Omer can you confirm that this fix the problem of your platform
>> too ?
>
>
> I just noticed this thread after posting
> https://lkml.org/lkml/2016/10/18/719...
> Noticed this bug while a ago and had the patch above at least a week but
> unfortunately didn't have time to post...
> I think Omer had same problem I was trying to fix and I believe patch I post
> should address it.
>
> Vincent, your version fixes my test case as well.
Thanks for testing.
Can i consider this as a Tested-by ?
> This is sched_stat from the same test case I had in my changelog.
> Note dd-2030 which is in root cgroup had same runtime as dd-2033 which is in
> child cgroup.
>
> dd (2030, #threads: 1)
> -------------------------------------------------------------------
> se.exec_start : 275700.024137
> se.vruntime : 10589.114654
> se.sum_exec_runtime : 1576.837993
> se.nr_migrations : 0
> nr_switches : 159
> nr_voluntary_switches : 0
> nr_involuntary_switches : 159
> se.load.weight : 1048576
> se.avg.load_sum : 48840575
> se.avg.util_sum : 19741820
> se.avg.load_avg : 1022
> se.avg.util_avg : 413
> se.avg.last_update_time : 275700024137
> policy : 0
> prio : 120
> clock-delta : 34
> dd (2033, #threads: 1)
> -------------------------------------------------------------------
> se.exec_start : 275710.037178
> se.vruntime : 2383.802868
> se.sum_exec_runtime : 1576.547591
> se.nr_migrations : 0
> nr_switches : 162
> nr_voluntary_switches : 0
> nr_involuntary_switches : 162
> se.load.weight : 1048576
> se.avg.load_sum : 48316646
> se.avg.util_sum : 21235249
> se.avg.load_avg : 1011
> se.avg.util_avg : 444
> se.avg.last_update_time : 275710037178
> policy : 0
> prio : 120
> clock-delta : 36
>
> Thanks,
> Joonwoo
>
>
>>
>> ---
>> kernel/sched/fair.c | 9 ++++++++-
>> 1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> index 8b03fb5..89776ac 100644
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -690,7 +690,14 @@ void init_entity_runnable_average(struct sched_entity
>> *se)
>> * will definitely be update (after enqueue).
>> */
>> sa->period_contrib = 1023;
>> - sa->load_avg = scale_load_down(se->load.weight);
>> + /*
>> + * Tasks are intialized with full load to be seen as heavy task
>> until
>> + * they get a chance to stabilize to their real load level.
>> + * group entity are intialized with null load to reflect the fact
>> that
>> + * nothing has been attached yet to the task group.
>> + */
>> + if (entity_is_task(se))
>> + sa->load_avg = scale_load_down(se->load.weight);
>> sa->load_sum = sa->load_avg * LOAD_AVG_MAX;
>> /*
>> * At this point, util_avg won't be used in select_task_rq_fair
>> anyway
>>
>>
>>
>>
>
[toc] | [prev] | [next] | [standalone]
| From | Vincent Guittot <vincent.guittot@linaro.org> |
|---|---|
| Date | 2016-10-19 16:50 +0200 |
| Message-ID | <su0lQ-2yX-25@gated-at.bofh.it> |
| In reply to | #1502936 |
On 19 October 2016 at 13:33, Peter Zijlstra <peterz@infradead.org> wrote: > On Tue, Oct 18, 2016 at 01:56:51PM +0200, Vincent Guittot wrote: > >> --- >> kernel/sched/fair.c | 9 ++++++++- >> 1 file changed, 8 insertions(+), 1 deletion(-) >> >> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c >> index 8b03fb5..89776ac 100644 >> --- a/kernel/sched/fair.c >> +++ b/kernel/sched/fair.c >> @@ -690,7 +690,14 @@ void init_entity_runnable_average(struct sched_entity *se) >> * will definitely be update (after enqueue). >> */ >> sa->period_contrib = 1023; >> - sa->load_avg = scale_load_down(se->load.weight); >> + /* >> + * Tasks are intialized with full load to be seen as heavy task until >> + * they get a chance to stabilize to their real load level. >> + * group entity are intialized with null load to reflect the fact that >> + * nothing has been attached yet to the task group. >> + */ >> + if (entity_is_task(se)) >> + sa->load_avg = scale_load_down(se->load.weight); >> sa->load_sum = sa->load_avg * LOAD_AVG_MAX; >> /* >> * At this point, util_avg won't be used in select_task_rq_fair anyway >> > > Vince, could you post a proper version of this patch with changelogs and > tags so that we can get that merged into Linus' tree and stable for 4.8? yes. i just have to finish the changelog and i sent it >
[toc] | [prev] | [next] | [standalone]
| From | Joseph Salisbury <joseph.salisbury@canonical.com> |
|---|---|
| Date | 2016-10-19 17:00 +0200 |
| Message-ID | <su0vx-2D0-73@gated-at.bofh.it> |
| In reply to | #1502936 |
On 10/18/2016 07:56 AM, Vincent Guittot wrote:
> Le Tuesday 18 Oct 2016 à 12:34:12 (+0200), Peter Zijlstra a écrit :
>> On Tue, Oct 18, 2016 at 11:45:48AM +0200, Vincent Guittot wrote:
>>> On 18 October 2016 at 11:07, Peter Zijlstra <peterz@infradead.org> wrote:
>>>> So aside from funny BIOSes, this should also show up when creating
>>>> cgroups when you have offlined a few CPUs, which is far more common I'd
>>>> think.
>>> The problem is also that the load of the tg->se[cpu] that represents
>>> the tg->cfs_rq[cpu] is initialized to 1024 in:
>>> alloc_fair_sched_group
>>> for_each_possible_cpu(i) {
>>> init_entity_runnable_average(se);
>>> sa->load_avg = scale_load_down(se->load.weight);
>>>
>>> Initializing sa->load_avg to 1024 for a newly created task makes
>>> sense as we don't know yet what will be its real load but i'm not sure
>>> that we have to do the same for se that represents a task group. This
>>> load should be initialized to 0 and it will increase when task will be
>>> moved/attached into task group
>> Yes, I think that makes sense, not sure how horrible that is with the
> That should not be that bad because this initial value is only useful for
> the few dozens of ms that follow the creation of the task group
>
>> current state of things, but after your propagate patch, that
>> reinstates the interactivity hack that should work for sure.
> The patch below fixes the issue on my platform:
>
> Dietmar, Omer can you confirm that this fix the problem of your platform too ?
>
> ---
> kernel/sched/fair.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)Vinc
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 8b03fb5..89776ac 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -690,7 +690,14 @@ void init_entity_runnable_average(struct sched_entity *se)
> * will definitely be update (after enqueue).
> */
> sa->period_contrib = 1023;
> - sa->load_avg = scale_load_down(se->load.weight);
> + /*
> + * Tasks are intialized with full load to be seen as heavy task until
> + * they get a chance to stabilize to their real load level.
> + * group entity are intialized with null load to reflect the fact that
> + * nothing has been attached yet to the task group.
> + */
> + if (entity_is_task(se))
> + sa->load_avg = scale_load_down(se->load.weight);
> sa->load_sum = sa->load_avg * LOAD_AVG_MAX;
> /*
> * At this point, util_avg won't be used in select_task_rq_fair anyway
>
>
>
>
Omer also reports that this patch fixes the bug for him as well. Thanks
for the great work, Vincent!
[toc] | [prev] | [next] | [standalone]
| From | Vincent Guittot <vincent.guittot@linaro.org> |
|---|---|
| Date | 2016-10-19 17:00 +0200 |
| Message-ID | <su0vx-2D0-75@gated-at.bofh.it> |
| In reply to | #1503794 |
On 19 October 2016 at 16:49, Joseph Salisbury
<joseph.salisbury@canonical.com> wrote:
> On 10/18/2016 07:56 AM, Vincent Guittot wrote:
>> Le Tuesday 18 Oct 2016 à 12:34:12 (+0200), Peter Zijlstra a écrit :
>>> On Tue, Oct 18, 2016 at 11:45:48AM +0200, Vincent Guittot wrote:
>>>> On 18 October 2016 at 11:07, Peter Zijlstra <peterz@infradead.org> wrote:
>>>>> So aside from funny BIOSes, this should also show up when creating
>>>>> cgroups when you have offlined a few CPUs, which is far more common I'd
>>>>> think.
>>>> The problem is also that the load of the tg->se[cpu] that represents
>>>> the tg->cfs_rq[cpu] is initialized to 1024 in:
>>>> alloc_fair_sched_group
>>>> for_each_possible_cpu(i) {
>>>> init_entity_runnable_average(se);
>>>> sa->load_avg = scale_load_down(se->load.weight);
>>>>
>>>> Initializing sa->load_avg to 1024 for a newly created task makes
>>>> sense as we don't know yet what will be its real load but i'm not sure
>>>> that we have to do the same for se that represents a task group. This
>>>> load should be initialized to 0 and it will increase when task will be
>>>> moved/attached into task group
>>> Yes, I think that makes sense, not sure how horrible that is with the
>> That should not be that bad because this initial value is only useful for
>> the few dozens of ms that follow the creation of the task group
>>
>>> current state of things, but after your propagate patch, that
>>> reinstates the interactivity hack that should work for sure.
>> The patch below fixes the issue on my platform:
>>
>> Dietmar, Omer can you confirm that this fix the problem of your platform too ?
>>
>> ---
>> kernel/sched/fair.c | 9 ++++++++-
>> 1 file changed, 8 insertions(+), 1 deletion(-)Vinc
>>
>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> index 8b03fb5..89776ac 100644
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -690,7 +690,14 @@ void init_entity_runnable_average(struct sched_entity *se)
>> * will definitely be update (after enqueue).
>> */
>> sa->period_contrib = 1023;
>> - sa->load_avg = scale_load_down(se->load.weight);
>> + /*
>> + * Tasks are intialized with full load to be seen as heavy task until
>> + * they get a chance to stabilize to their real load level.
>> + * group entity are intialized with null load to reflect the fact that
>> + * nothing has been attached yet to the task group.
>> + */
>> + if (entity_is_task(se))
>> + sa->load_avg = scale_load_down(se->load.weight);
>> sa->load_sum = sa->load_avg * LOAD_AVG_MAX;
>> /*
>> * At this point, util_avg won't be used in select_task_rq_fair anyway
>>
>>
>>
>>
> Omer also reports that this patch fixes the bug for him as well. Thanks
> for the great work, Vincent!
Thanks
>
[toc] | [prev] | [next] | [standalone]
| From | Dietmar Eggemann <dietmar.eggemann@arm.com> |
|---|---|
| Date | 2016-10-19 17:40 +0200 |
| Message-ID | <su18e-37s-29@gated-at.bofh.it> |
| In reply to | #1502936 |
On 19/10/16 12:25, Vincent Guittot wrote: > On 19 October 2016 at 11:46, Dietmar Eggemann <dietmar.eggemann@arm.com> wrote: >> On 18/10/16 12:56, Vincent Guittot wrote: >>> Le Tuesday 18 Oct 2016 à 12:34:12 (+0200), Peter Zijlstra a écrit : >>>> On Tue, Oct 18, 2016 at 11:45:48AM +0200, Vincent Guittot wrote: >>>>> On 18 October 2016 at 11:07, Peter Zijlstra <peterz@infradead.org> wrote: [...] >> But this test only makes sure that we don't see any ghost contribution >> (from non-existing cpus) any more. >> >> We should study the tg->se[i]->avg.load_avg for the hierarchy of tg's >> (with the highest tg having a task enqueued) a little bit more, with and >> without your v5 'sched: reflect sched_entity move into task_group's load'. > > Can you elaborate ? I try :-) I thought I will see some different behaviour because of the fact that the tg se's are initialized differently [1024 versus 0]. But I can't spot any difference. The test case is running a sysbench thread affine to cpu1 in tg_root/tg_1/tg_11/tg_111 on tip/sched/core on an ARM64 Juno (6 logical cpus). The moment the sysbench task is put into tg_111 tg_111->se[1]->avg.load_avg gets updated to 0 any way because of the huge time difference between creating this tg and attaching a task to it. So the tg->se[2]->avg.load_avg signals for tg_111, tg_11 and tg_1 look exactly the same w/o and w/ your patch. But your patch helps in this (very synthetic) test case as well. W/o your patch I see remaining tg->load_avg for tg_1 and tg_11 after the test case has finished because the tg's were exclusively used on cpu1. # cat /proc/sched_debug cfs_rq[1]:/tg_1 .tg_load_avg_contrib : 0 .tg_load_avg : 5120 (5 (unused cpus) * 1024 * 1) cfs_rq[1]:/tg_1/tg_11/tg_111 .tg_load_avg_contrib : 0 .tg_load_avg : 0 cfs_rq[1]:/tg_1/tg_11 .tg_load_avg_contrib : 0 .tg_load_avg : 5120 With your patch applied all the .tg_load_avg are 0.
[toc] | [prev] | [next] | [standalone]
| From | Joonwoo Park <joonwoop@codeaurora.org> |
|---|---|
| Date | 2016-10-19 19:40 +0200 |
| Message-ID | <su30l-4pj-5@gated-at.bofh.it> |
| In reply to | #1503932 |
On Wed, Oct 19, 2016 at 04:33:03PM +0100, Dietmar Eggemann wrote: > On 19/10/16 12:25, Vincent Guittot wrote: > > On 19 October 2016 at 11:46, Dietmar Eggemann <dietmar.eggemann@arm.com> wrote: > >> On 18/10/16 12:56, Vincent Guittot wrote: > >>> Le Tuesday 18 Oct 2016 à 12:34:12 (+0200), Peter Zijlstra a écrit : > >>>> On Tue, Oct 18, 2016 at 11:45:48AM +0200, Vincent Guittot wrote: > >>>>> On 18 October 2016 at 11:07, Peter Zijlstra <peterz@infradead.org> wrote: > > [...] > > >> But this test only makes sure that we don't see any ghost contribution > >> (from non-existing cpus) any more. > >> > >> We should study the tg->se[i]->avg.load_avg for the hierarchy of tg's > >> (with the highest tg having a task enqueued) a little bit more, with and > >> without your v5 'sched: reflect sched_entity move into task_group's load'. > > > > Can you elaborate ? > > I try :-) > > I thought I will see some different behaviour because of the fact that > the tg se's are initialized differently [1024 versus 0]. This is the exact thing I was also worried about and that's the reason I tried to fix this in a different way. However I didn't find any behaviour difference once any task attached to child cfs_rq which is the point we really care about. I found this bug while making patch at https://lkml.org/lkml/2016/10/18/841 which will fail with wrong task_group load_avg. I tested Vincent's patch and above together, confirmed it's still good. Though I know Ingo already sent out pull request. Anyway. Tested-by: Joonwoo Park <joonwoop@codeaurora.org> Thanks, Joonwoo > > But I can't spot any difference. The test case is running a sysbench > thread affine to cpu1 in tg_root/tg_1/tg_11/tg_111 on tip/sched/core on > an ARM64 Juno (6 logical cpus). > The moment the sysbench task is put into tg_111 > tg_111->se[1]->avg.load_avg gets updated to 0 any way because of the > huge time difference between creating this tg and attaching a task to > it. So the tg->se[2]->avg.load_avg signals for tg_111, tg_11 and tg_1 > look exactly the same w/o and w/ your patch. > > But your patch helps in this (very synthetic) test case as well. W/o > your patch I see remaining tg->load_avg for tg_1 and tg_11 after the > test case has finished because the tg's were exclusively used on cpu1. > > # cat /proc/sched_debug > > cfs_rq[1]:/tg_1 > .tg_load_avg_contrib : 0 > .tg_load_avg : 5120 (5 (unused cpus) * 1024 * 1) > cfs_rq[1]:/tg_1/tg_11/tg_111 > .tg_load_avg_contrib : 0 > .tg_load_avg : 0 > cfs_rq[1]:/tg_1/tg_11 > .tg_load_avg_contrib : 0 > .tg_load_avg : 5120 > > With your patch applied all the .tg_load_avg are 0.
[toc] | [prev] | [next] | [standalone]
| From | Vincent Guittot <vincent.guittot@linaro.org> |
|---|---|
| Date | 2016-10-19 20:00 +0200 |
| Message-ID | <su3jH-4wR-5@gated-at.bofh.it> |
| In reply to | #1503932 |
On 19 October 2016 at 17:33, Dietmar Eggemann <dietmar.eggemann@arm.com> wrote: > On 19/10/16 12:25, Vincent Guittot wrote: >> On 19 October 2016 at 11:46, Dietmar Eggemann <dietmar.eggemann@arm.com> wrote: >>> On 18/10/16 12:56, Vincent Guittot wrote: >>>> Le Tuesday 18 Oct 2016 à 12:34:12 (+0200), Peter Zijlstra a écrit : >>>>> On Tue, Oct 18, 2016 at 11:45:48AM +0200, Vincent Guittot wrote: >>>>>> On 18 October 2016 at 11:07, Peter Zijlstra <peterz@infradead.org> wrote: > > [...] > >>> But this test only makes sure that we don't see any ghost contribution >>> (from non-existing cpus) any more. >>> >>> We should study the tg->se[i]->avg.load_avg for the hierarchy of tg's >>> (with the highest tg having a task enqueued) a little bit more, with and >>> without your v5 'sched: reflect sched_entity move into task_group's load'. >> >> Can you elaborate ? > > I try :-) > > I thought I will see some different behaviour because of the fact that > the tg se's are initialized differently [1024 versus 0]. This difference should be noticeable (if noticeable) only during few hundreds of ms after the creation of the task group until the load_avg has reached its real value. > > But I can't spot any difference. The test case is running a sysbench > thread affine to cpu1 in tg_root/tg_1/tg_11/tg_111 on tip/sched/core on > an ARM64 Juno (6 logical cpus). > The moment the sysbench task is put into tg_111 > tg_111->se[1]->avg.load_avg gets updated to 0 any way because of the > huge time difference between creating this tg and attaching a task to > it. So the tg->se[2]->avg.load_avg signals for tg_111, tg_11 and tg_1 > look exactly the same w/o and w/ your patch. > > But your patch helps in this (very synthetic) test case as well. W/o > your patch I see remaining tg->load_avg for tg_1 and tg_11 after the > test case has finished because the tg's were exclusively used on cpu1. > > # cat /proc/sched_debug > > cfs_rq[1]:/tg_1 > .tg_load_avg_contrib : 0 > .tg_load_avg : 5120 (5 (unused cpus) * 1024 * 1) > cfs_rq[1]:/tg_1/tg_11/tg_111 > .tg_load_avg_contrib : 0 > .tg_load_avg : 0 > cfs_rq[1]:/tg_1/tg_11 > .tg_load_avg_contrib : 0 > .tg_load_avg : 5120 > > With your patch applied all the .tg_load_avg are 0.
[toc] | [prev] | [next] | [standalone]
| From | Vincent Guittot <vincent.guittot@linaro.org> |
|---|---|
| Date | 2016-10-19 17:50 +0200 |
| Message-ID | <su18e-37s-33@gated-at.bofh.it> |
| In reply to | #1502936 |
On 19 October 2016 at 11:46, Dietmar Eggemann <dietmar.eggemann@arm.com> wrote: > On 18/10/16 12:56, Vincent Guittot wrote: >> Le Tuesday 18 Oct 2016 à 12:34:12 (+0200), Peter Zijlstra a écrit : >>> On Tue, Oct 18, 2016 at 11:45:48AM +0200, Vincent Guittot wrote: >>>> On 18 October 2016 at 11:07, Peter Zijlstra <peterz@infradead.org> wrote: > > [...] > >> >> The patch below fixes the issue on my platform: >> >> Dietmar, Omer can you confirm that this fix the problem of your platform too ? > > It fixes this broken BIOS issue on my T430 ( cpu_possible_mask > > cpu_online_mask). I ran the original test with the cpu hogs (stress -c > 4). Launch time of applications becomes normal again. > > Tested-by: Dietmar Eggemann <dietmar.eggemann@arm.com> Thanks > > But this test only makes sure that we don't see any ghost contribution > (from non-existing cpus) any more. > > We should study the tg->se[i]->avg.load_avg for the hierarchy of tg's > (with the highest tg having a task enqueued) a little bit more, with and > without your v5 'sched: reflect sched_entity move into task_group's load'. Can you elaborate ? Vincent > >> --- >> kernel/sched/fair.c | 9 ++++++++- >> 1 file changed, 8 insertions(+), 1 deletion(-) >> >> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c >> index 8b03fb5..89776ac 100644 >> --- a/kernel/sched/fair.c >> +++ b/kernel/sched/fair.c >> @@ -690,7 +690,14 @@ void init_entity_runnable_average(struct sched_entity *se) >> * will definitely be update (after enqueue). >> */ >> sa->period_contrib = 1023; >> - sa->load_avg = scale_load_down(se->load.weight); >> + /* >> + * Tasks are intialized with full load to be seen as heavy task until >> + * they get a chance to stabilize to their real load level. >> + * group entity are intialized with null load to reflect the fact that >> + * nothing has been attached yet to the task group. >> + */ >> + if (entity_is_task(se)) >> + sa->load_avg = scale_load_down(se->load.weight); >> sa->load_sum = sa->load_avg * LOAD_AVG_MAX; >> /* >> * At this point, util_avg won't be used in select_task_rq_fair anyway
[toc] | [prev] | [next] | [standalone]
| From | Dietmar Eggemann <dietmar.eggemann@arm.com> |
|---|---|
| Date | 2016-10-19 17:50 +0200 |
| Message-ID | <su18e-37s-31@gated-at.bofh.it> |
| In reply to | #1502936 |
On 18/10/16 12:56, Vincent Guittot wrote: > Le Tuesday 18 Oct 2016 à 12:34:12 (+0200), Peter Zijlstra a écrit : >> On Tue, Oct 18, 2016 at 11:45:48AM +0200, Vincent Guittot wrote: >>> On 18 October 2016 at 11:07, Peter Zijlstra <peterz@infradead.org> wrote: [...] > > The patch below fixes the issue on my platform: > > Dietmar, Omer can you confirm that this fix the problem of your platform too ? It fixes this broken BIOS issue on my T430 ( cpu_possible_mask > cpu_online_mask). I ran the original test with the cpu hogs (stress -c 4). Launch time of applications becomes normal again. Tested-by: Dietmar Eggemann <dietmar.eggemann@arm.com> But this test only makes sure that we don't see any ghost contribution (from non-existing cpus) any more. We should study the tg->se[i]->avg.load_avg for the hierarchy of tg's (with the highest tg having a task enqueued) a little bit more, with and without your v5 'sched: reflect sched_entity move into task_group's load'. > --- > kernel/sched/fair.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c > index 8b03fb5..89776ac 100644 > --- a/kernel/sched/fair.c > +++ b/kernel/sched/fair.c > @@ -690,7 +690,14 @@ void init_entity_runnable_average(struct sched_entity *se) > * will definitely be update (after enqueue). > */ > sa->period_contrib = 1023; > - sa->load_avg = scale_load_down(se->load.weight); > + /* > + * Tasks are intialized with full load to be seen as heavy task until > + * they get a chance to stabilize to their real load level. > + * group entity are intialized with null load to reflect the fact that > + * nothing has been attached yet to the task group. > + */ > + if (entity_is_task(se)) > + sa->load_avg = scale_load_down(se->load.weight); > sa->load_sum = sa->load_avg * LOAD_AVG_MAX; > /* > * At this point, util_avg won't be used in select_task_rq_fair anyway
[toc] | [prev] | [next] | [standalone]
| From | Peter Zijlstra <peterz@infradead.org> |
|---|---|
| Date | 2016-10-19 18:20 +0200 |
| Message-ID | <su0lQ-2yX-27@gated-at.bofh.it> |
| In reply to | #1502936 |
On Tue, Oct 18, 2016 at 01:56:51PM +0200, Vincent Guittot wrote: > --- > kernel/sched/fair.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c > index 8b03fb5..89776ac 100644 > --- a/kernel/sched/fair.c > +++ b/kernel/sched/fair.c > @@ -690,7 +690,14 @@ void init_entity_runnable_average(struct sched_entity *se) > * will definitely be update (after enqueue). > */ > sa->period_contrib = 1023; > - sa->load_avg = scale_load_down(se->load.weight); > + /* > + * Tasks are intialized with full load to be seen as heavy task until > + * they get a chance to stabilize to their real load level. > + * group entity are intialized with null load to reflect the fact that > + * nothing has been attached yet to the task group. > + */ > + if (entity_is_task(se)) > + sa->load_avg = scale_load_down(se->load.weight); > sa->load_sum = sa->load_avg * LOAD_AVG_MAX; > /* > * At this point, util_avg won't be used in select_task_rq_fair anyway > Vince, could you post a proper version of this patch with changelogs and tags so that we can get that merged into Linus' tree and stable for 4.8?
[toc] | [prev] | [next] | [standalone]
| From | Morten Rasmussen <morten.rasmussen@arm.com> |
|---|---|
| Date | 2016-10-19 18:40 +0200 |
| Message-ID | <su24j-3OH-57@gated-at.bofh.it> |
| In reply to | #1502936 |
On Tue, Oct 18, 2016 at 01:56:51PM +0200, Vincent Guittot wrote:
> Le Tuesday 18 Oct 2016 à 12:34:12 (+0200), Peter Zijlstra a écrit :
> > On Tue, Oct 18, 2016 at 11:45:48AM +0200, Vincent Guittot wrote:
> > > On 18 October 2016 at 11:07, Peter Zijlstra <peterz@infradead.org> wrote:
> > > > So aside from funny BIOSes, this should also show up when creating
> > > > cgroups when you have offlined a few CPUs, which is far more common I'd
> > > > think.
> > >
> > > The problem is also that the load of the tg->se[cpu] that represents
> > > the tg->cfs_rq[cpu] is initialized to 1024 in:
> > > alloc_fair_sched_group
> > > for_each_possible_cpu(i) {
> > > init_entity_runnable_average(se);
> > > sa->load_avg = scale_load_down(se->load.weight);
> > >
> > > Initializing sa->load_avg to 1024 for a newly created task makes
> > > sense as we don't know yet what will be its real load but i'm not sure
> > > that we have to do the same for se that represents a task group. This
> > > load should be initialized to 0 and it will increase when task will be
> > > moved/attached into task group
> >
> > Yes, I think that makes sense, not sure how horrible that is with the
>
> That should not be that bad because this initial value is only useful for
> the few dozens of ms that follow the creation of the task group
IMHO, it doesn't make much sense to initialize empty containers, which
group sched_entities really are, to 1024. It is meant to represent what
is in it, and a creation it is empty, so in my opinion initializing it
to zero make sense.
> > current state of things, but after your propagate patch, that
> > reinstates the interactivity hack that should work for sure.
It actually works on mainline/tip as well.
As I see it, the fundamental problem is keeping group entities up to
date. Because the load_weight and hence se->avg.load_avg each per-cpu
group sched_entity depends on the group cfs_rq->tg_load_avg_contrib for
all cpus (tg->load_avg), including those that might be empty and
therefore not enqueued, we must ensure that they are updated some other
way. Most naturally as part of update_blocked_averages().
To guarantee that, it basically boils down to making sure:
Any cfs_rq with a non-zero tg_load_avg_contrib must be on the
leaf_cfs_rq_list.
We can do that in different ways: 1) Add all cfs_rqs to the
leaf_cfs_rq_list at task group creation, or 2) initialize group
sched_entity contributions to zero and make sure that they are added to
leaf_cfs_rq_list as soon as a sched_entity (task or group) is enqueued
on it.
Vincent patch below gives us the second option.
> kernel/sched/fair.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 8b03fb5..89776ac 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -690,7 +690,14 @@ void init_entity_runnable_average(struct sched_entity *se)
> * will definitely be update (after enqueue).
> */
> sa->period_contrib = 1023;
> - sa->load_avg = scale_load_down(se->load.weight);
> + /*
> + * Tasks are intialized with full load to be seen as heavy task until
> + * they get a chance to stabilize to their real load level.
> + * group entity are intialized with null load to reflect the fact that
> + * nothing has been attached yet to the task group.
> + */
> + if (entity_is_task(se))
> + sa->load_avg = scale_load_down(se->load.weight);
> sa->load_sum = sa->load_avg * LOAD_AVG_MAX;
> /*
> * At this point, util_avg won't be used in select_task_rq_fair anyway
I would suggest adding a comment somewhere stating that we need to keep
group cfs_rqs up to date:
-----
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index abb3763dff69..2b820d489be0 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -6641,6 +6641,11 @@ static void update_blocked_averages(int cpu)
if (throttled_hierarchy(cfs_rq))
continue;
+ /*
+ * Note that _any_ leaf cfs_rq with a non-zero tg_load_avg_contrib
+ * _must_ be on the leaf_cfs_rq_list to ensure that group shares
+ * are updated correctly.
+ */
if (update_cfs_rq_load_avg(cfs_rq_clock_task(cfs_rq), cfs_rq, true))
update_tg_load_avg(cfs_rq, 0);
}
-----
I did a couple of simple tests on tip/sched/core to test whether
Vincent's fix works even without reflecting group load/util in the group
hierarchy:
Juno (2xA57+4xA53)
tip:
grouped hog(1) alone: 2841
non-grouped hogs(6) alone: 40830
grouped hog(1): 218
non-grouped hogs(6): 40580
tip+vg:
grouped hog alone: 2849
non-grouped hogs(6) alone: 40831
grouped hog: 2363
non-grouped hogs: 38418
See script below for details, but we basically see that the grouped task
is not getting its 'fair' share on tip, while it does with Vincent's
patch.
To summarize, I think Vincent's patch makes sense and works :-) More
testing is needed of cause to see if there are other problems.
-----
# Create 100 task groups:
for i in `seq 1 100`;
do
cgcreate -g cpu:/root/test$i
done
NCPUS=$(grep -c ^processor /proc/cpuinfo)
# Run single cpu hog inside task group on first cpu _alone_:
cgexec -g cpu:/root/test100 taskset 0x01 sysbench --test=cpu \
--num-threads=1 --max-time=5 --max-requests=1000000 run | \
awk '{if ($4=="events:") {print "grouped hog(1) alone: " $5}}'
# Run cpu hogs outside task group _alone_:
sysbench --test=cpu --num-threads=$NCPUS --max-time=10 \
--max-requests=1000000 run | awk '{if ($4=="events:") \
{print "non-grouped hogs('$NCPUS') alone: " $5}}'
# Run cpu hogs outside task group:
sysbench --test=cpu --num-threads=$NCPUS --max-time=10 \
--max-requests=1000000 run | awk '{if ($4=="events:") \
{print "non-grouped hogs('$NCPUS'): " $5}}' &
# Run single cpu hog inside task group on first cpu:
cgexec -g cpu:/root/test100 taskset 0x01 sysbench \
--test=cpu --num-threads=1 --max-time=5 \
--max-requests=1000000 run | awk '{if ($4=="events:") \
{print "grouped hog(1): " $5}}'
wait
# Delete task groups:
for i in `seq 1 100`;
do
cgdelete -g cpu:/root/test$i
done
[toc] | [prev] | [next] | [standalone]
| From | Vincent Guittot <vincent.guittot@linaro.org> |
|---|---|
| Date | 2016-10-19 19:50 +0200 |
| Message-ID | <su3a2-4te-47@gated-at.bofh.it> |
| In reply to | #1504125 |
On 19 October 2016 at 15:30, Morten Rasmussen <morten.rasmussen@arm.com> wrote:
> On Tue, Oct 18, 2016 at 01:56:51PM +0200, Vincent Guittot wrote:
>> Le Tuesday 18 Oct 2016 à 12:34:12 (+0200), Peter Zijlstra a écrit :
>> > On Tue, Oct 18, 2016 at 11:45:48AM +0200, Vincent Guittot wrote:
>> > > On 18 October 2016 at 11:07, Peter Zijlstra <peterz@infradead.org> wrote:
>> > > > So aside from funny BIOSes, this should also show up when creating
>> > > > cgroups when you have offlined a few CPUs, which is far more common I'd
>> > > > think.
>> > >
>> > > The problem is also that the load of the tg->se[cpu] that represents
>> > > the tg->cfs_rq[cpu] is initialized to 1024 in:
>> > > alloc_fair_sched_group
>> > > for_each_possible_cpu(i) {
>> > > init_entity_runnable_average(se);
>> > > sa->load_avg = scale_load_down(se->load.weight);
>> > >
>> > > Initializing sa->load_avg to 1024 for a newly created task makes
>> > > sense as we don't know yet what will be its real load but i'm not sure
>> > > that we have to do the same for se that represents a task group. This
>> > > load should be initialized to 0 and it will increase when task will be
>> > > moved/attached into task group
>> >
>> > Yes, I think that makes sense, not sure how horrible that is with the
>>
>> That should not be that bad because this initial value is only useful for
>> the few dozens of ms that follow the creation of the task group
>
> IMHO, it doesn't make much sense to initialize empty containers, which
> group sched_entities really are, to 1024. It is meant to represent what
> is in it, and a creation it is empty, so in my opinion initializing it
> to zero make sense.
>
>> > current state of things, but after your propagate patch, that
>> > reinstates the interactivity hack that should work for sure.
>
> It actually works on mainline/tip as well.
>
> As I see it, the fundamental problem is keeping group entities up to
> date. Because the load_weight and hence se->avg.load_avg each per-cpu
> group sched_entity depends on the group cfs_rq->tg_load_avg_contrib for
> all cpus (tg->load_avg), including those that might be empty and
> therefore not enqueued, we must ensure that they are updated some other
> way. Most naturally as part of update_blocked_averages().
>
> To guarantee that, it basically boils down to making sure:
> Any cfs_rq with a non-zero tg_load_avg_contrib must be on the
> leaf_cfs_rq_list.
>
> We can do that in different ways: 1) Add all cfs_rqs to the
> leaf_cfs_rq_list at task group creation, or 2) initialize group
> sched_entity contributions to zero and make sure that they are added to
> leaf_cfs_rq_list as soon as a sched_entity (task or group) is enqueued
> on it.
>
> Vincent patch below gives us the second option.
>
>> kernel/sched/fair.c | 9 ++++++++-
>> 1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> index 8b03fb5..89776ac 100644
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -690,7 +690,14 @@ void init_entity_runnable_average(struct sched_entity *se)
>> * will definitely be update (after enqueue).
>> */
>> sa->period_contrib = 1023;
>> - sa->load_avg = scale_load_down(se->load.weight);
>> + /*
>> + * Tasks are intialized with full load to be seen as heavy task until
>> + * they get a chance to stabilize to their real load level.
>> + * group entity are intialized with null load to reflect the fact that
>> + * nothing has been attached yet to the task group.
>> + */
>> + if (entity_is_task(se))
>> + sa->load_avg = scale_load_down(se->load.weight);
>> sa->load_sum = sa->load_avg * LOAD_AVG_MAX;
>> /*
>> * At this point, util_avg won't be used in select_task_rq_fair anyway
>
> I would suggest adding a comment somewhere stating that we need to keep
> group cfs_rqs up to date:
>
> -----
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index abb3763dff69..2b820d489be0 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -6641,6 +6641,11 @@ static void update_blocked_averages(int cpu)
> if (throttled_hierarchy(cfs_rq))
> continue;
>
> + /*
> + * Note that _any_ leaf cfs_rq with a non-zero tg_load_avg_contrib
> + * _must_ be on the leaf_cfs_rq_list to ensure that group shares
> + * are updated correctly.
> + */
As discussed on IRC, the point is that even if the leaf cfs_rq is
added to the leaf_cfs_rq_list, it doesn't ensure that it will be
updated correctly for unplugged CPUs
> if (update_cfs_rq_load_avg(cfs_rq_clock_task(cfs_rq), cfs_rq, true))
> update_tg_load_avg(cfs_rq, 0);
> }
> -----
>
> I did a couple of simple tests on tip/sched/core to test whether
> Vincent's fix works even without reflecting group load/util in the group
> hierarchy:
>
> Juno (2xA57+4xA53)
>
> tip:
> grouped hog(1) alone: 2841
> non-grouped hogs(6) alone: 40830
> grouped hog(1): 218
> non-grouped hogs(6): 40580
>
> tip+vg:
> grouped hog alone: 2849
> non-grouped hogs(6) alone: 40831
> grouped hog: 2363
> non-grouped hogs: 38418
>
> See script below for details, but we basically see that the grouped task
> is not getting its 'fair' share on tip, while it does with Vincent's
> patch.
>
> To summarize, I think Vincent's patch makes sense and works :-) More
> testing is needed of cause to see if there are other problems.
>
> -----
>
> # Create 100 task groups:
> for i in `seq 1 100`;
> do
> cgcreate -g cpu:/root/test$i
> done
>
> NCPUS=$(grep -c ^processor /proc/cpuinfo)
>
> # Run single cpu hog inside task group on first cpu _alone_:
> cgexec -g cpu:/root/test100 taskset 0x01 sysbench --test=cpu \
> --num-threads=1 --max-time=5 --max-requests=1000000 run | \
> awk '{if ($4=="events:") {print "grouped hog(1) alone: " $5}}'
>
> # Run cpu hogs outside task group _alone_:
> sysbench --test=cpu --num-threads=$NCPUS --max-time=10 \
> --max-requests=1000000 run | awk '{if ($4=="events:") \
> {print "non-grouped hogs('$NCPUS') alone: " $5}}'
>
> # Run cpu hogs outside task group:
> sysbench --test=cpu --num-threads=$NCPUS --max-time=10 \
> --max-requests=1000000 run | awk '{if ($4=="events:") \
> {print "non-grouped hogs('$NCPUS'): " $5}}' &
>
> # Run single cpu hog inside task group on first cpu:
> cgexec -g cpu:/root/test100 taskset 0x01 sysbench \
> --test=cpu --num-threads=1 --max-time=5 \
> --max-requests=1000000 run | awk '{if ($4=="events:") \
> {print "grouped hog(1): " $5}}'
>
> wait
>
> # Delete task groups:
> for i in `seq 1 100`;
> do
> cgdelete -g cpu:/root/test$i
> done
[toc] | [prev] | [next] | [standalone]
| From | Morten Rasmussen <morten.rasmussen@arm.com> |
|---|---|
| Date | 2016-10-20 10:00 +0200 |
| Message-ID | <sugqB-4Bq-1@gated-at.bofh.it> |
| In reply to | #1504187 |
On Wed, Oct 19, 2016 at 07:41:36PM +0200, Vincent Guittot wrote:
> On 19 October 2016 at 15:30, Morten Rasmussen <morten.rasmussen@arm.com> wrote:
> > On Tue, Oct 18, 2016 at 01:56:51PM +0200, Vincent Guittot wrote:
> >> Le Tuesday 18 Oct 2016 à 12:34:12 (+0200), Peter Zijlstra a écrit :
> >> > On Tue, Oct 18, 2016 at 11:45:48AM +0200, Vincent Guittot wrote:
> >> > > On 18 October 2016 at 11:07, Peter Zijlstra <peterz@infradead.org> wrote:
> >> > > > So aside from funny BIOSes, this should also show up when creating
> >> > > > cgroups when you have offlined a few CPUs, which is far more common I'd
> >> > > > think.
> >> > >
> >> > > The problem is also that the load of the tg->se[cpu] that represents
> >> > > the tg->cfs_rq[cpu] is initialized to 1024 in:
> >> > > alloc_fair_sched_group
> >> > > for_each_possible_cpu(i) {
> >> > > init_entity_runnable_average(se);
> >> > > sa->load_avg = scale_load_down(se->load.weight);
> >> > >
> >> > > Initializing sa->load_avg to 1024 for a newly created task makes
> >> > > sense as we don't know yet what will be its real load but i'm not sure
> >> > > that we have to do the same for se that represents a task group. This
> >> > > load should be initialized to 0 and it will increase when task will be
> >> > > moved/attached into task group
> >> >
> >> > Yes, I think that makes sense, not sure how horrible that is with the
> >>
> >> That should not be that bad because this initial value is only useful for
> >> the few dozens of ms that follow the creation of the task group
> >
> > IMHO, it doesn't make much sense to initialize empty containers, which
> > group sched_entities really are, to 1024. It is meant to represent what
> > is in it, and a creation it is empty, so in my opinion initializing it
> > to zero make sense.
> >
> >> > current state of things, but after your propagate patch, that
> >> > reinstates the interactivity hack that should work for sure.
> >
> > It actually works on mainline/tip as well.
> >
> > As I see it, the fundamental problem is keeping group entities up to
> > date. Because the load_weight and hence se->avg.load_avg each per-cpu
> > group sched_entity depends on the group cfs_rq->tg_load_avg_contrib for
> > all cpus (tg->load_avg), including those that might be empty and
> > therefore not enqueued, we must ensure that they are updated some other
> > way. Most naturally as part of update_blocked_averages().
> >
> > To guarantee that, it basically boils down to making sure:
> > Any cfs_rq with a non-zero tg_load_avg_contrib must be on the
> > leaf_cfs_rq_list.
> >
> > We can do that in different ways: 1) Add all cfs_rqs to the
> > leaf_cfs_rq_list at task group creation, or 2) initialize group
> > sched_entity contributions to zero and make sure that they are added to
> > leaf_cfs_rq_list as soon as a sched_entity (task or group) is enqueued
> > on it.
> >
> > Vincent patch below gives us the second option.
> >
> >> kernel/sched/fair.c | 9 ++++++++-
> >> 1 file changed, 8 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> >> index 8b03fb5..89776ac 100644
> >> --- a/kernel/sched/fair.c
> >> +++ b/kernel/sched/fair.c
> >> @@ -690,7 +690,14 @@ void init_entity_runnable_average(struct sched_entity *se)
> >> * will definitely be update (after enqueue).
> >> */
> >> sa->period_contrib = 1023;
> >> - sa->load_avg = scale_load_down(se->load.weight);
> >> + /*
> >> + * Tasks are intialized with full load to be seen as heavy task until
> >> + * they get a chance to stabilize to their real load level.
> >> + * group entity are intialized with null load to reflect the fact that
> >> + * nothing has been attached yet to the task group.
> >> + */
> >> + if (entity_is_task(se))
> >> + sa->load_avg = scale_load_down(se->load.weight);
> >> sa->load_sum = sa->load_avg * LOAD_AVG_MAX;
> >> /*
> >> * At this point, util_avg won't be used in select_task_rq_fair anyway
> >
> > I would suggest adding a comment somewhere stating that we need to keep
> > group cfs_rqs up to date:
> >
> > -----
> > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > index abb3763dff69..2b820d489be0 100644
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -6641,6 +6641,11 @@ static void update_blocked_averages(int cpu)
> > if (throttled_hierarchy(cfs_rq))
> > continue;
> >
> > + /*
> > + * Note that _any_ leaf cfs_rq with a non-zero tg_load_avg_contrib
> > + * _must_ be on the leaf_cfs_rq_list to ensure that group shares
> > + * are updated correctly.
> > + */
>
> As discussed on IRC, the point is that even if the leaf cfs_rq is
> added to the leaf_cfs_rq_list, it doesn't ensure that it will be
> updated correctly for unplugged CPUs
Agreed. We have to ensure that tg_load_avg_contrib is zeroed for leaf
cfs_rqs belonging to unplugged cpus. And if modify the above to say
leaf_cfs_rq_list of an online cpu, then we should be covered I think.
[toc] | [prev] | [next] | [standalone]
| From | Dietmar Eggemann <dietmar.eggemann@arm.com> |
|---|---|
| Date | 2016-10-18 13:20 +0200 |
| Message-ID | <stAB3-T8-1@gated-at.bofh.it> |
| In reply to | #1502815 |
On 18/10/16 10:07, Peter Zijlstra wrote: > On Mon, Oct 17, 2016 at 11:52:39PM +0100, Dietmar Eggemann wrote: [...] >> Using for_each_online_cpu(i) instead of for_each_possible_cpu(i) in >> online_fair_sched_group() works on this machine, i.e. the .tg_load_avg >> of system.slice tg is 0 after startup. > > Right, so the reason for using present_mask is that it avoids having to > deal with hotplug, also all the per-cpu memory is allocated and present > for !online CPUs anyway, so might as well set it up properly anyway. > > (You might want to start booting your laptop with "possible_cpus=4" to > save some memory FWIW.) The question for me is could this be the reason for the X1 Carbon platform as well? The initial pastebin from Joseph (http://paste.ubuntu.com/23312351) showed .tg_load_avg : 381697 on a 4 logical cpu thing. With a couple of more services than 80 this might be the problem. > > But yes, we have a bug here too... /me ponders > > So aside from funny BIOSes, this should also show up when creating > cgroups when you have offlined a few CPUs, which is far more common I'd > think. Yes. > On IRC you mentioned that adding list_add_leaf_cfs_rq() to > online_fair_sched_group() cures this, this would actually match with > unregister_fair_sched_group() doing list_del_leaf_cfs_rq() and avoid > a few instructions on the enqueue path, so that's all good. Yes, I was able to recreate a similar problem (not related to the cpu masks) on ARM64 (6 logical cpus). I created 100 2. level tg's but only put one task (no cpu affinity, so it could run on multiple cpus) in one of these tg's (mainly to see the related cfs_rq's in /proc/sched_debug). I get a remaining .tg_load_avg : 49898 for cfs_rq[x]:/tg_1 > I'm just not immediately seeing how that cures things. The only relevant > user of the leaf_cfs_rq list seems to be update_blocked_averages() which > is called from the balance code (idle_balance() and > rebalance_domains()). But neither should call that for offline (or > !present) CPUs. Assuming this is load from the 99 2. level tg's which never had a task running, putting list_add_leaf_cfs_rq() into online_fair_sched_group() for all cpus makes sure that all the 'blocked load' get's decayed. Doing what Vincent just suggested, not initializing tg se's w/ 1024 but w/ 0 instead prevents this from being necessary. [...]
[toc] | [prev] | [next] | [standalone]
| From | Peter Zijlstra <peterz@infradead.org> |
|---|---|
| Date | 2016-10-18 14:10 +0200 |
| Message-ID | <stBnr-1re-25@gated-at.bofh.it> |
| In reply to | #1502897 |
On Tue, Oct 18, 2016 at 12:15:11PM +0100, Dietmar Eggemann wrote: > On 18/10/16 10:07, Peter Zijlstra wrote: > > On Mon, Oct 17, 2016 at 11:52:39PM +0100, Dietmar Eggemann wrote: > > On IRC you mentioned that adding list_add_leaf_cfs_rq() to > > online_fair_sched_group() cures this, this would actually match with > > unregister_fair_sched_group() doing list_del_leaf_cfs_rq() and avoid > > a few instructions on the enqueue path, so that's all good. > > Yes, I was able to recreate a similar problem (not related to the cpu > masks) on ARM64 (6 logical cpus). I created 100 2. level tg's but only > put one task (no cpu affinity, so it could run on multiple cpus) in one > of these tg's (mainly to see the related cfs_rq's in /proc/sched_debug). > > I get a remaining .tg_load_avg : 49898 for cfs_rq[x]:/tg_1 Ah, and since all those CPUs are online, we decay all that load away. OK makes sense now. > > I'm just not immediately seeing how that cures things. The only relevant > > user of the leaf_cfs_rq list seems to be update_blocked_averages() which > > is called from the balance code (idle_balance() and > > rebalance_domains()). But neither should call that for offline (or > > !present) CPUs. > > Assuming this is load from the 99 2. level tg's which never had a task > running, putting list_add_leaf_cfs_rq() into online_fair_sched_group() > for all cpus makes sure that all the 'blocked load' get's decayed. > > Doing what Vincent just suggested, not initializing tg se's w/ 1024 but > w/ 0 instead prevents this from being necessary. Indeed. I just worry about the cases where we do no propagate the load up, eg. the stuff fixed by: 1476695653-12309-5-git-send-email-vincent.guittot@linaro.org If we hit an intermediary cgroup with 0 load, we might get some interactivity issues. But it could be I got lost again :-)
[toc] | [prev] | [standalone]
Page 2 of 2 — ← Prev page 1 [2]
Back to top | Article view | linux.kernel
csiph-web