Re: [PATCH 3/5] sched/fair: Rework feec() to use cost instead of spare capacity

[Vincent Guittot]

On Wed, 4 Sept 2024 at 17:07, Pierre Gondois <pierre.gondois@xxxxxxx> wrote:
>
>
>
> On 8/30/24 15:03, Vincent Guittot wrote:
> > feec() looks for the CPU with highest spare capacity in a PD assuming that
> > it will be the best CPU from a energy efficiency PoV because it will
> > require the smallest increase of OPP. Although this is true generally
> > speaking, this policy also filters some others CPUs which will be as
> > efficients because of using the same OPP.
> > In fact, we really care about the cost of the new OPP that will be
> > selected to handle the waking task. In many cases, several CPUs will end
> > up selecting the same OPP and as a result using the same energy cost. In
> > these cases, we can use other metrics to select the best CPU for the same
> > energy cost.
> >
> > Rework feec() to look 1st for the lowest cost in a PD and then the most
> > performant CPU between CPUs.
> >
> > Signed-off-by: Vincent Guittot <vincent.guittot@xxxxxxxxxx>
> > ---
> >   kernel/sched/fair.c | 466 +++++++++++++++++++++++---------------------
> >   1 file changed, 244 insertions(+), 222 deletions(-)
> >
> > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > index e67d6029b269..2273eecf6086 100644
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > -/*
> > - * compute_energy(): Use the Energy Model to estimate the energy that @pd would
> > - * consume for a given utilization landscape @eenv. When @dst_cpu < 0, the task
> > - * contribution is ignored.
> > - */
> > -static inline unsigned long
> > -compute_energy(struct energy_env *eenv, struct perf_domain *pd,
> > -            struct cpumask *pd_cpus, struct task_struct *p, int dst_cpu)
> > +/*Check if the CPU can handle the waking task */
> > +static int check_cpu_with_task(struct task_struct *p, int cpu)
> >   {
> > -     unsigned long max_util = eenv_pd_max_util(eenv, pd_cpus, p, dst_cpu);
> > -     unsigned long busy_time = eenv->pd_busy_time;
> > -     unsigned long energy;
> > +     unsigned long p_util_min = uclamp_is_used() ? uclamp_eff_value(p, UCLAMP_MIN) : 0;
> > +     unsigned long p_util_max = uclamp_is_used() ? uclamp_eff_value(p, UCLAMP_MAX) : 1024;
> > +     unsigned long util_min = p_util_min;
> > +     unsigned long util_max = p_util_max;
> > +     unsigned long util = cpu_util(cpu, p, cpu, 0);
> > +     struct rq *rq = cpu_rq(cpu);
> >
> > -     if (dst_cpu >= 0)
> > -             busy_time = min(eenv->pd_cap, busy_time + eenv->task_busy_time);
> > +     /*
> > +      * Skip CPUs that cannot satisfy the capacity request.
> > +      * IOW, placing the task there would make the CPU
> > +      * overutilized. Take uclamp into account to see how
> > +      * much capacity we can get out of the CPU; this is
> > +      * aligned with sched_cpu_util().
> > +      */
> > +     if (uclamp_is_used() && !uclamp_rq_is_idle(rq)) {
> > +             unsigned long rq_util_min, rq_util_max;
> > +             /*
> > +              * Open code uclamp_rq_util_with() except for
> > +              * the clamp() part. I.e.: apply max aggregation
> > +              * only. util_fits_cpu() logic requires to
> > +              * operate on non clamped util but must use the
> > +              * max-aggregated uclamp_{min, max}.
> > +              */
> > +             rq_util_min = uclamp_rq_get(rq, UCLAMP_MIN);
> > +             rq_util_max = uclamp_rq_get(rq, UCLAMP_MAX);
> > +             util_min = max(rq_util_min, p_util_min);
> > +             util_max = max(rq_util_max, p_util_max);
> > +     }
> > +     return util_fits_cpu(util, util_min, util_max, cpu);
> > +}
> >
> > -     energy = em_cpu_energy(pd->em_pd, max_util, busy_time, eenv->cpu_cap);
>
> I think em_cpu_energy() would need to be removed with this patch,
> if there are no more references to it.

Yes, I will add a patch to cleanup unused function

>