linux-kernel - Re: [PATCH V2 2/2] sched/fair: Use actual_cpu_capacity everywhere in util_fits

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240628012832.37swdtxr4ds2kkp7@airbuntu>
Date: Fri, 28 Jun 2024 02:28:32 +0100
From: Qais Yousef <qyousef@...alina.io>
To: Xuewen Yan <xuewen.yan@...soc.com>
Cc: vincent.guittot@...aro.org, dietmar.eggemann@....com, mingo@...hat.com,
	peterz@...radead.org, juri.lelli@...hat.com, rostedt@...dmis.org,
	bsegall@...gle.com, mgorman@...e.de, bristot@...hat.com,
	vschneid@...hat.com, christian.loehle@....com,
	vincent.donnefort@....com, ke.wang@...soc.com, di.shen@...soc.com,
	xuewen.yan94@...il.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH V2 2/2] sched/fair: Use actual_cpu_capacity everywhere in
 util_fits_cpu()

On 06/24/24 16:20, Xuewen Yan wrote:
> Commit f1f8d0a22422 ("sched/cpufreq: Take cpufreq feedback into account")
> introduced get_actual_cpu_capacity(), and it had aggregated the
> different pressures applied on the capacity of CPUs.
> And in util_fits_cpu(), it would return true when uclamp_max
> is smaller than SCHED_CAPACITY_SCALE, althought the uclamp_max
> is bigger than actual_cpu_capacity.
> 
> So use actual_cpu_capacity everywhere in util_fits_cpu() to
> cover all cases.
> 
> Signed-off-by: Xuewen Yan <xuewen.yan@...soc.com>
> ---
>  kernel/sched/fair.c | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 5ca6396ef0b7..9c16ae192217 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -4980,7 +4980,7 @@ static inline int util_fits_cpu(unsigned long util,
>  				int cpu)
>  {
>  	unsigned long capacity = capacity_of(cpu);
> -	unsigned long capacity_orig;
> +	unsigned long capacity_actual;
>  	bool fits, uclamp_max_fits;
>  
>  	/*
> @@ -4992,15 +4992,15 @@ static inline int util_fits_cpu(unsigned long util,
>  		return fits;
>  
>  	/*
> -	 * We must use arch_scale_cpu_capacity() for comparing against uclamp_min and
> +	 * We must use actual_cpu_capacity() for comparing against uclamp_min and
>  	 * uclamp_max. We only care about capacity pressure (by using
>  	 * capacity_of()) for comparing against the real util.
>  	 *
>  	 * If a task is boosted to 1024 for example, we don't want a tiny
>  	 * pressure to skew the check whether it fits a CPU or not.
>  	 *
> -	 * Similarly if a task is capped to arch_scale_cpu_capacity(little_cpu), it
> -	 * should fit a little cpu even if there's some pressure.
> +	 * Similarly if a task is capped to actual_cpu_capacity, it should fit
> +	 * the cpu even if there's some pressure.

This statement is not clear now. We need to be specific since
actual_cpu_capacity() includes thermal pressure and cpufreq limits.

/even if there's some pressure/even if there is non OPP based pressure ie: RT,
DL or irq/?

>  	 *
>  	 * Only exception is for HW or cpufreq pressure since it has a direct impact
>  	 * on available OPP of the system.
> @@ -5011,7 +5011,7 @@ static inline int util_fits_cpu(unsigned long util,
>  	 * For uclamp_max, we can tolerate a drop in performance level as the
>  	 * goal is to cap the task. So it's okay if it's getting less.
>  	 */
> -	capacity_orig = arch_scale_cpu_capacity(cpu);
> +	capacity_actual = get_actual_cpu_capacity(cpu);
>  
>  	/*
>  	 * We want to force a task to fit a cpu as implied by uclamp_max.
> @@ -5039,7 +5039,7 @@ static inline int util_fits_cpu(unsigned long util,
>  	 *     uclamp_max request.
>  	 *
>  	 *   which is what we're enforcing here. A task always fits if
> -	 *   uclamp_max <= capacity_orig. But when uclamp_max > capacity_orig,
> +	 *   uclamp_max <= capacity_actual. But when uclamp_max > capacity_actual,
>  	 *   the normal upmigration rules should withhold still.
>  	 *
>  	 *   Only exception is when we are on max capacity, then we need to be
> @@ -5050,8 +5050,8 @@ static inline int util_fits_cpu(unsigned long util,
>  	 *     2. The system is being saturated when we're operating near
>  	 *        max capacity, it doesn't make sense to block overutilized.
>  	 */
> -	uclamp_max_fits = (capacity_orig == SCHED_CAPACITY_SCALE) && (uclamp_max == SCHED_CAPACITY_SCALE);
> -	uclamp_max_fits = !uclamp_max_fits && (uclamp_max <= capacity_orig);
> +	uclamp_max_fits = (capacity_actual == SCHED_CAPACITY_SCALE) && (uclamp_max == SCHED_CAPACITY_SCALE);

We should use capacity_orig here. We are checking if the CPU is the max
capacity CPU.

> +	uclamp_max_fits = !uclamp_max_fits && (uclamp_max <= capacity_actual);
>  	fits = fits || uclamp_max_fits;
>  
>  	/*
> @@ -5086,8 +5086,7 @@ static inline int util_fits_cpu(unsigned long util,
>  	 * handle the case uclamp_min > uclamp_max.
>  	 */
>  	uclamp_min = min(uclamp_min, uclamp_max);
> -	if (fits && (util < uclamp_min) &&
> -	    (uclamp_min > get_actual_cpu_capacity(cpu)))
> +	if (fits && (util < uclamp_min) && (uclamp_min > capacity_actual))
>  		return -1;
>  
>  	return fits;
> -- 
> 2.25.1
>