Received-SPF: pass (google.com: domain of linux-kernel+bounces-207535-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Sun, 9 Jun 2024 23:55:20 +0100
From: Qais Yousef <qyousef@layalina.io>
To: Xuewen Yan <xuewen.yan@unisoc.com>
Cc: vincent.guittot@linaro.org, mingo@redhat.com, peterz@infradead.org,
	juri.lelli@redhat.com, dietmar.eggemann@arm.com,
	rostedt@goodmis.org, bsegall@google.com, mgorman@suse.de,
	bristot@redhat.com, vschneid@redhat.com, vincent.donnefort@arm.com,
	ke.wang@unisoc.com, xuewen.yan94@gmail.com,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] sched/fair: Prevent cpu_busy_time from exceeding
 actual_cpu_capacity
Message-ID: <20240609225520.6gnmx2wjhxghcxfo@airbuntu>
References: <20240606070645.3295-1-xuewen.yan@unisoc.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20240606070645.3295-1-xuewen.yan@unisoc.com>

On 06/06/24 15:06, Xuewen Yan wrote:
> Because the effective_cpu_util() would return a util which
> maybe bigger than the actual_cpu_capacity, this could cause
> the pd_busy_time calculation errors.
> So clamp the cpu_busy_time with the eenv->cpu_cap, which is
> the actual_cpu_capacity.

I actually think capping by pd_cap is something we should remove. Saturated
systems aren't calculated properly especially when uclamp_max is used.

But this might a bigger change and out of scope of what you're proposing..

Did this 'wrong' calculation cause an actual problem for task placement?
I assume the pd looked 'busier' because some CPUs were too busy.

Was the system in overutilzied state? Usually if one CPU is that busy
overutilized should be set and we'd skip EAS - unless uclamp_max was used.

> 
> Fixes: 3e8c6c9aac42 ("sched/fair: Remove task_util from effective utilization in feec()")
> Signed-off-by: Xuewen Yan <xuewen.yan@unisoc.com>
> ---
>  kernel/sched/fair.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 8a5b1ae0aa55..8939d725023a 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -7870,7 +7870,9 @@ static inline void eenv_pd_busy_time(struct energy_env *eenv,
>  	for_each_cpu(cpu, pd_cpus) {
>  		unsigned long util = cpu_util(cpu, p, -1, 0);
>  
> -		busy_time += effective_cpu_util(cpu, util, NULL, NULL);
> +		util = effective_cpu_util(cpu, util, NULL, NULL);
> +		util = min(eenv->cpu_cap, util);
> +		busy_time += util;
>  	}
>  
>  	eenv->pd_busy_time = min(eenv->pd_cap, busy_time);
> -- 
> 2.25.1
>