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;
MIME-Version: 1.0
References: <20220713040430.25778-1-zhouchengming@bytedance.com> <20220713040430.25778-10-zhouchengming@bytedance.com>
In-Reply-To: <20220713040430.25778-10-zhouchengming@bytedance.com>
From:   Vincent Guittot <vincent.guittot@linaro.org>
Date:   Tue, 19 Jul 2022 15:20:15 +0200
Message-ID: <CAKfTPtC4r1pon_QxHaa=3r85v4u1DL32_9FbTwL0zBZT017E-g@mail.gmail.com>
Subject: Re: [PATCH v2 09/10] sched/fair: stop load tracking when task switched_from_fair()
To:     Chengming Zhou <zhouchengming@bytedance.com>
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
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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

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