Re: [PATCH v2 09/10] sched/fair: stop load tracking when task switched_from_fair()

From: Chengming Zhou
Date: Wed Jul 27 2022 - 06:55:48 EST


On 2022/7/19 21:20, Vincent Guittot wrote:
> On Wed, 13 Jul 2022 at 06:05, Chengming Zhou
> <zhouchengming@xxxxxxxxxxxxx> wrote:
>>
>> The same reason as the previous commit, if we don't reset the
>> sched_avg last_update_time to 0, after a while in switched_to_fair():
>>
>> switched_to_fair
>> attach_task_cfs_rq
>> attach_entity_cfs_rq
>> update_load_avg
>> __update_load_avg_se(now, cfs_rq, se)
>>
>> The delta (now - sa->last_update_time) will wrongly contribute/decay
>> sched_avg depends on the task running/runnable status at that time.
>
> I think that we always decay the time spent in !fair class and never contribute
>
> This one is a bit more complex than the new task one. Generally
> speaking, I would say that we should decay the load while running in
> !fair case because the value becomes less and less relevant over time.
> What's the meaning of pelt values when the task has run seconds as a
> fifo ? The only case that would need a particular attention, is the pi
> boost case but there is no way to now how long is runs and sleep when
> !fair

After thought for a while, I agree with you on that the PELT values become
meaningless after running as a FIFO task. So yes it's best for now to always
decay them when switched_to_fair().

So in the next version:
1. don't set for !fair in post_init_entity_util_avg()
2. keep last_update_time when switched_from_fair()
3. should we still keep SKIP_AGE_LOAD ? I don't understand what's its usage
in switched_to_fair(). Or should we just delete it?

Thanks!

>
>>
>> This patch reset it's sched_avg last_update_time to 0, stop load
>> tracking for !fair task, later in switched_to_fair() ->
>> update_load_avg(), we can use its saved sched_avg.
>>
>> Signed-off-by: Chengming Zhou <zhouchengming@xxxxxxxxxxxxx>
>> ---
>> kernel/sched/fair.c | 5 +++++
>> 1 file changed, 5 insertions(+)
>>
>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> index 50f65a2ede32..576028f5a09e 100644
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -11552,6 +11552,11 @@ static void attach_task_cfs_rq(struct task_struct *p)
>> static void switched_from_fair(struct rq *rq, struct task_struct *p)
>> {
>> detach_task_cfs_rq(p);
>> +
>> +#ifdef CONFIG_SMP
>> + /* Stop load tracking for !fair task */
>> + p->se.avg.last_update_time = 0;
>> +#endif
>> }
>>
>> static void switched_to_fair(struct rq *rq, struct task_struct *p)
>> --
>> 2.36.1
>>