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]

Date: Tue, 25 Jun 2024 10:46:05 +0200
From: Vincent Guittot <vincent.guittot@...aro.org>
To: Xuewen Yan <xuewen.yan@...soc.com>
Cc: dietmar.eggemann@....com, mingo@...hat.com, peterz@...radead.org, 
	juri.lelli@...hat.com, qyousef@...alina.io, 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 Mon, 24 Jun 2024 at 10:21, Xuewen Yan <xuewen.yan@...soc.com> 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

s/althought/although/

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

actual_cpu_capacity() includes capacity pressure so this comment needs
to be changed.

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

In think that the pressure in this case is about DL, RT and Irq but
not OPP capping because of thermal pressure for example

Also, the returned value is not a boolean: fit or not fit but a
ternary for the case where it doesn't fit the uclamp min. In the later
case, the CPU is still considered as a candidate

>          *
> -        * 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.
>          *
>          * 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);
> +       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
>