Hi, Vincent,
Vincent Guittot <vincent.guit...@linaro.org> writes:
> On 4 January 2017 at 04:08, Huang, Ying <ying.hu...@intel.com> wrote:
>> Vincent Guittot <vincent.guit...@linaro.org> writes:
>>
>>>>
>>>> Vincent, like we discussed in September last year, the proper fix would
>>>> probably be a cfs-rq->nr_attached which IMHO is not doable w/o being an
>>>> atomic because of migrate_task_rq_fair()->remove_entity_load_avg() not
>>>> holding the rq lock.
>>>
>>> I remember the discussion and even if I agree that a large number of
>>> taskgroup
>>> increases the number of loop in update_blocked_averages() and as a result
>>> the
>>> time spent in the update, I don't think that this is the root cause of
>>> this regression because the patch "sched/fair: Propagate asynchrous detach"
>>> doesn't add more loops to update_blocked_averages but it adds more thing to
>>> do
>>> per loop.
>>>
>>> Then, I think I'm still too conservative in the condition for calling
>>> update_load_avg(cfs_rq->tg->se[cpu], 0). This call has been added to
>>> propagate gcfs_rq->propagate_avg flag to parent so we don't need to call it
>>> even if load_avg is not null but only when propagate_avg flag is set. The
>>> patch below should improve thing compare to the previous version because
>>> it will call update_load_avg(cfs_rq->tg->se[cpu], 0) only if an
>>> asynchrounous
>>> detach happened (propagate_avg is set).
>>>
>>> Ying, could you test the patch below instead of the previous one ?
>>>
>>> ---
>>> kernel/sched/fair.c | 8 +++++---
>>> 1 file changed, 5 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>>> index 6559d19..a4f5c35 100644
>>> --- a/kernel/sched/fair.c
>>> +++ b/kernel/sched/fair.c
>>> @@ -6915,6 +6915,7 @@ static void update_blocked_averages(int cpu)
>>> {
>>> struct rq *rq = cpu_rq(cpu);
>>> struct cfs_rq *cfs_rq;
>>> + struct sched_entity *se;
>>> unsigned long flags;
>>>
>>> raw_spin_lock_irqsave(&rq->lock, flags);
>>> @@ -6932,9 +6933,10 @@ static void update_blocked_averages(int cpu)
>>> if (update_cfs_rq_load_avg(cfs_rq_clock_task(cfs_rq), cfs_rq,
>>> true))
>>> update_tg_load_avg(cfs_rq, 0);
>>>
>>> - /* Propagate pending load changes to the parent */
>>> - if (cfs_rq->tg->se[cpu])
>>> - update_load_avg(cfs_rq->tg->se[cpu], 0);
>>> + /* Propagate pending load changes to the parent if any */
>>> + se = cfs_rq->tg->se[cpu];
>>> + if (se && cfs_rq->propagate_avg)
>>> + update_load_avg(se, 0);
>>> }
>>> raw_spin_unlock_irqrestore(&rq->lock, flags);
>>> }
>>
>> Here is the test result,
>>
>> =========================================================================================
>> compiler/cpufreq_governor/freq/kconfig/nr_task/rootfs/samples/tbox_group/test/testcase:
>>
>> gcc-6/powersave/20/x86_64-rhel-7.2/100%/debian-x86_64-2016-08-31.cgz/6000ss/lkp-hsw-d01/cache/ftq
>>
>> commit:
>> 4e5160766fcc9f41bbd38bac11f92dce993644aa: first bad commit
>> 09a43ace1f986b003c118fdf6ddf1fd685692d49: parent of first bad commit
>> b524060933c546fd2410c5a09360ba23a0fef846: with fix patch above
>>
>> 4e5160766fcc9f41 09a43ace1f986b003c118fdf6d b524060933c546fd2410c5a093
>> ---------------- -------------------------- --------------------------
>> %stddev %change %stddev %change %stddev
>> \ | \ | \
>> 3463 ± 10% -61.4% 1335 ± 17% -3.0% 3359 ± 2%
>> ftq.noise.50%
>> 1116 ± 23% -73.7% 293.90 ± 30% -23.8% 850.69 ± 17%
>> ftq.noise.75%
>
> To be honest, I was expecting at least the same level of improvement
> as the previous patch if not better but i was not expecting worse
> results
What's your next plan for this regression? At least your previous patch
could recover part of it.
Best Regards,
Huang, Ying
