by Qais Yousef

[permalink] [raw]

Subject: Re: [PATCH 1/7] sched/uclamp: Fix relationship between uclamp and migration margin

On 07/22/22 17:13, Vincent Guittot wrote:

[...]

> Using capacity_orig_of(cpu) - thermal_load_avg(rq_of(cpu)) seems like
> a simple solution to cover thermal mitigation
>
> Also I was looking more deeply at your condition and get hard time to
> understand why uclamp_max_fits needs to be false when both
> (capacity_orig == SCHED_CAPACITY_SCALE) && (uclamp_max == SCHED_CAPACITY_SCALE) ?
>
> + max_capacity = (capacity_orig == SCHED_CAPACITY_SCALE) &&
> (uclamp_max == SCHED_CAPACITY_SCALE);
> + uclamp_max_fits = !max_capacity && (uclamp_max <= capacity_orig);
> + fits = fits || uclamp_max_fits;
>
> For task I would have done only :
>
> + capacity_orig = capacity_orig_of(cpu) - thermal_load_avg(rq_of(cpu));
> + uclamp_max_fits = (uclamp_max <= capacity_orig);
> fits = fits || uclamp_max_fits;

I just sent v2, and it's good to clarify what I have considered so far:

uclamp_max shouldn't care about thermal pressure except for capacity inversion
case. The goal of uclamp_max is to cap the task and the weak affinity part of
the hint is important to honour. So transient thermal pressure is not a problem
from fitness point of view. uclamp_max means it shouldn't exceed this perf
level, it's okay to be capped at a less value.

And ignoring the max_capacity check for tasks will actually create problems
because feec() will wrongly force fit tasks on the biggest cores only for
overutilized state to trigger later.

To preserve the current behavior, feec() should bailout and let the other logic
in select_task_rq_fair() fallback to the next best thing.

To do that, we need both call sites to behave the same.

>
> and I would use a different one for cpu_overutlized in orde to discard the test
> with uclamp_max if uclamp_max one equals SCHED_CAPACITY_SCALE
>
> + uclamp_max_fits = (uclamp_max <= capacity_orig) && (uclamp_max != SCHED_CAPACITY_SCALE);

I opted to keep the logic encapsulated in util_fits_cpu(). I was wary that not
having coherent logic for all call sites will lead to random behavior changes.
Especially in the wake up path.

> and I don't think that we should compare uclamp_min <= capacity_orig for
> cpu_overutlized() but only for task to detect misfit one because uclamp_min is
> a performance hint not a bandwidth as you said previously.

I'd agree only for the corner case where capacity_orig == SCHED_CAPACITY_SCALE.

But for others it actually defeats the purpose of uclamp_min. If the user
dynamically controls uclamp_min (there are already users in android), then we
should detect if we need to migrate the task to a bigger CPU at the tick,
otherwise the new uclamp_min will only be honoured on the next wake up.

This doesn't contradict the performance hint nature of uclamp. If it requests
a uclamp_min = 1024 for example but it's already running on a little or medium
CPU, then by not triggering a misfit migration we prevent the task from
obtaining the performance level it asked for until the next wake up. Which
might end up being too late and impact the user experience already.

Thanks!

--
Qais Yousef

2022-08-17 10:35:10

by Vincent Guittot

[permalink] [raw]

Subject: Re: [PATCH 2/7] sched/uclamp: Make task_fits_capacity() use util_fits_cpu()

On Wed, 27 Jul 2022 at 18:05, Qais Yousef <[email protected]> wrote:
>
> Hi Vincent
>
> On 07/22/22 10:19, Vincent Guittot wrote:
> > Le jeudi 21 juil. 2022 ï¿½ 15:29:49 (+0100), Qais Yousef a ï¿½crit :
> > > On 07/12/22 11:48, Qais Yousef wrote:
> > > > On 07/11/22 15:09, Vincent Guittot wrote:
> > > > > On Wed, 29 Jun 2022 at 21:48, Qais Yousef <[email protected]> wrote:
> > > >
> >
> > [...]
> >
> > > > > > @@ -9108,7 +9125,7 @@ static inline void update_sg_wakeup_stats(struct sched_domain *sd,
> > > > > >
> > > > > > /* Check if task fits in the group */
> > > > > > if (sd->flags & SD_ASYM_CPUCAPACITY &&
> > > > > > - !task_fits_capacity(p, group->sgc->max_capacity)) {
> > > > > > + !task_fits_cpu(p, group->sgc->max_capacity_cpu)) {
> > > > >
> > > > > All the changes and added complexity above for this line. Can't you
> > > > > find another way ?
> > > >
> > > > You're right, I might have got carried away trying to keep the logic the same.
> > > >
> > > > Can we use group->asym_prefer_cpu or pick a cpu from group->sgc->cpumask
> > > > instead?
> > > >
> > > > I'll dig more into it anyway and try to come up with simpler alternative.
> > >
> > > Actually we can't.
> > >
> > > I can keep the current {max,min}_capacity field and just add the new
> > > {max,min}_capacity_cpu and use them where needed. Should address your concerns
> > > this way? That was actually the first version of the code, but then it seemed
> > > redundant to keep both {max,min}_capacity and {max,min}_capacity_cpu.
> > >
> > > OR
> > >
> > > I can add a new function to search for max spare capacity cpu in the group.
> > >
> > > Preference?
> > >
> >
> > Isn't the below enough and much simpler ?
>
> Thanks for that!
>
> >
> > [PATCH] sched/uclamp: Make task_fits_capacity() use util_fits_cpu()
> >
> > So that the new uclamp rules in regard to migration margin and capacity
> > pressure are taken into account correctly.
> > ---
> > kernel/sched/fair.c | 25 +++++++++++++++----------
> > kernel/sched/sched.h | 9 +++++++++
> > 2 files changed, 24 insertions(+), 10 deletions(-)
> >
> > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > index 5eecae32a0f6..3e0c7cc490be 100644
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -4317,10 +4317,12 @@ static inline int util_fits_cpu(unsigned long util,
> > return fits;
> > }
> >
> > -static inline int task_fits_capacity(struct task_struct *p,
> > - unsigned long capacity)
> > +static inline int task_fits_cpu(struct task_struct *p, int cpu)
> > {
> > - return fits_capacity(uclamp_task_util(p), capacity);
> > + unsigned long uclamp_min = uclamp_eff_value(p, UCLAMP_MIN);
> > + unsigned long uclamp_max = uclamp_eff_value(p, UCLAMP_MAX);
> > + unsigned long util = task_util_est(p);
> > + return util_fits_cpu(util, uclamp_min, uclamp_max, cpu);
> > }
> >
> > static inline void update_misfit_status(struct task_struct *p, struct rq *rq)
> > @@ -4333,7 +4335,7 @@ static inline void update_misfit_status(struct task_struct *p, struct rq *rq)
> > return;
> > }
> >
> > - if (task_fits_capacity(p, capacity_of(cpu_of(rq)))) {
> > + if (task_fits_cpu(p, cpu_of(rq))) {
> > rq->misfit_task_load = 0;
> > return;
> > }
> > @@ -8104,7 +8106,7 @@ static int detach_tasks(struct lb_env *env)
> >
> > case migrate_misfit:
> > /* This is not a misfit task */
> > - if (task_fits_capacity(p, capacity_of(env->src_cpu)))
> > + if (task_fits_cpu(p, env->src_cpu))
> > goto next;
> >
> > env->imbalance = 0;
> > @@ -9085,6 +9087,10 @@ static inline void update_sg_wakeup_stats(struct sched_domain *sd,
> >
> > memset(sgs, 0, sizeof(*sgs));
> >
> > + /* Assume that task can't fit any CPU of the group */
> > + if (sd->flags & SD_ASYM_CPUCAPACITY)
> > + sgs->group_misfit_task_load = 0;
>
> Should this be
>
> sgs->group_misfit_task_load = 1
>
> to indicate it doesn't fit?

Yes

>
> > +
> > for_each_cpu(i, sched_group_span(group)) {
> > struct rq *rq = cpu_rq(i);
> > unsigned int local;
> > @@ -9104,12 +9110,11 @@ static inline void update_sg_wakeup_stats(struct sched_domain *sd,
> > if (!nr_running && idle_cpu_without(i, p))
> > sgs->idle_cpus++;
> >
> > - }
> > + /* Check if task fits in the CPU */
> > + if (sd->flags & SD_ASYM_CPUCAPACITY &&
> > + task_fits_cpu(p, i))
> > + sgs->group_misfit_task_load = 0;
>
> So we clear the flag if there's any cpu that fits, I think that should work,
> yes and much better too. I got tunneled visioned and didn't take a step back to
> look at the big picture. Thanks for the suggestion :-)
>
> I think we can make it more efficient by checking if
> sgs->group_misfit_task_load is set
>
> /* Check if task fits in the CPU */
> if (sd->flags & SD_ASYM_CPUCAPACITY &&
> sgs->group_misfit_task_load &&
> task_fits_cpu(p, i))
> sgs->group_misfit_task_load = 0;
>
> which will avoid calling task_fits_cpu() repeatedly if we got a hit already.

yes, looks better

>
>
> Thanks!
>
> --
> Qais Yousef
>
> >
> > - /* Check if task fits in the group */
> > - if (sd->flags & SD_ASYM_CPUCAPACITY &&
> > - !task_fits_capacity(p, group->sgc->max_capacity)) {
> > - sgs->group_misfit_task_load = 1;
> > }
> >
> > sgs->group_capacity = group->sgc->capacity;
> > diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> > index 02c970501295..3292ad2db4ac 100644
> > --- a/kernel/sched/sched.h
> > +++ b/kernel/sched/sched.h
> > @@ -2988,6 +2988,15 @@ static inline bool uclamp_is_used(void)
> > return static_branch_likely(&sched_uclamp_used);
> > }
> > #else /* CONFIG_UCLAMP_TASK */
> > +static inline unsigned long uclamp_eff_value(struct task_struct *p,
> > + enum uclamp_id clamp_id)
> > +{
> > + if (clamp_id == UCLAMP_MIN)
> > + return 0;
> > +
> > + return SCHED_CAPACITY_SCALE;
> > +}
> > +
> > static inline
> > unsigned long uclamp_rq_util_with(struct rq *rq, unsigned long util,
> > struct task_struct *p)
> > --
> > 2.17.1
> >
> > >
> > > Thanks!
> > >
> > > --
> > > Qais Yousef