Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <011a2a34-69c4-0d12-0121-92d6f526a23b@bytedance.com>
Date:   Wed, 27 Jul 2022 18:55:31 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.0.3
Subject: Re: [PATCH v2 09/10] sched/fair: stop load tracking when task
 switched_from_fair()
Content-Language: en-US
To:     Vincent Guittot <vincent.guittot@linaro.org>
Cc:     mingo@redhat.com, peterz@infradead.org, dietmar.eggemann@arm.com,
        rostedt@goodmis.org, bsegall@google.com, vschneid@redhat.com,
        linux-kernel@vger.kernel.org
References: <20220713040430.25778-1-zhouchengming@bytedance.com>
 <20220713040430.25778-10-zhouchengming@bytedance.com>
 <CAKfTPtC4r1pon_QxHaa=3r85v4u1DL32_9FbTwL0zBZT017E-g@mail.gmail.com>
From:   Chengming Zhou <zhouchengming@bytedance.com>
In-Reply-To: <CAKfTPtC4r1pon_QxHaa=3r85v4u1DL32_9FbTwL0zBZT017E-g@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 2022/7/19 21:20, Vincent Guittot wrote:
> On Wed, 13 Jul 2022 at 06:05, Chengming Zhou
> <zhouchengming@bytedance.com> 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@bytedance.com>
>> ---
>>  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
>>