[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAB8ipk-p_vT3mq4+BD0AxzgsLiRxpAnHJ1QaTWr1t619JeaL+w@mail.gmail.com>
Date: Mon, 1 Jul 2024 20:13:12 +0800
From: Xuewen Yan <xuewen.yan94@...il.com>
To: Qais Yousef <qyousef@...alina.io>
Cc: Xuewen Yan <xuewen.yan@...soc.com>, 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, linux-kernel@...r.kernel.org
Subject: Re: [PATCH V2 2/2] sched/fair: Use actual_cpu_capacity everywhere in util_fits_cpu()
On Fri, Jun 28, 2024 at 9:28 AM Qais Yousef <qyousef@...alina.io> wrote:
>
> 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.
Maybe we could remove the uclamp_max_fits = (capacity_orig ==
SCHED_CAPACITY_SCALE) && (uclamp_max == SCHED_CAPACITY_SCALE);
and just judge the uclamp_max <= capacity_actual?
- 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 = (uclamp_max <= capacity_actual);
>
> > + 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
> >
Powered by blists - more mailing lists