linux-kernel - Re: [PATCH 2/5] energy model: Add a get previous state function

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: <CAKfTPtDHTrXmXuJsC9OEwib2brFs87YMjrkVcH=eb08GRwVw+A@mail.gmail.com>
Date: Fri, 6 Sep 2024 08:55:11 +0200
From: Vincent Guittot <vincent.guittot@...aro.org>
To: Lukasz Luba <lukasz.luba@....com>
Cc: qyousef@...alina.io, hongyan.xia2@....com, mingo@...hat.com, 
	mgorman@...e.de, peterz@...radead.org, dietmar.eggemann@....com, 
	bsegall@...gle.com, vschneid@...hat.com, rostedt@...dmis.org, 
	rafael.j.wysocki@...el.com, linux-kernel@...r.kernel.org, 
	juri.lelli@...hat.com
Subject: Re: [PATCH 2/5] energy model: Add a get previous state function

On Thu, 5 Sept 2024 at 11:20, Lukasz Luba <lukasz.luba@....com> wrote:
>
> Hi Vincent,
>
> On 8/30/24 14:03, Vincent Guittot wrote:
> > Instead of parsing all EM table everytime, add a function to get the
> > previous state.
> >
> > Will be used in the scheduler feec() function.
> >
> > Signed-off-by: Vincent Guittot <vincent.guittot@...aro.org>
> > ---
> >   include/linux/energy_model.h | 18 ++++++++++++++++++
> >   1 file changed, 18 insertions(+)
> >
> > diff --git a/include/linux/energy_model.h b/include/linux/energy_model.h
> > index 1ff52020cf75..ea8ea7e031c0 100644
> > --- a/include/linux/energy_model.h
> > +++ b/include/linux/energy_model.h
> > @@ -207,6 +207,24 @@ em_pd_get_efficient_state(struct em_perf_state *table, int nr_perf_states,
> >       return nr_perf_states - 1;
> >   }
> >
> > +static inline int
> > +em_pd_get_previous_state(struct em_perf_state *table, int nr_perf_states,
> > +                       int idx, unsigned long pd_flags)
> > +{
> > +     struct em_perf_state *ps;
> > +     int i;
> > +
> > +     for (i = idx - 1; i >= 0; i--) {
> > +             ps = &table[i];
> > +             if (pd_flags & EM_PERF_DOMAIN_SKIP_INEFFICIENCIES &&
> > +                 ps->flags & EM_PERF_STATE_INEFFICIENT)
> > +                     continue;
> > +             return i;
> > +     }
>
> Would you mind to add a comment on top of that for loop?

Yes I will

> Or maybe a bit more detail in the patch header what would you like to
> find (e.g. 1st efficient OPP which is lower).
>
> It's looking for a first OPP (don't forget it's ascending 'table') which
> is lower or equal to the 'idx' state.
>
> If uclamp_max is set and that OPP is inefficient, don't we choose
> a higher OPP which is efficient?

I use this function to get the capacity range of an OPP at index idx.
uclamp has already been checked before when selecting OPP idx. Now we
want to capacity range to know when we need to look for a lower OPP

>
> I'm not against this function.
>
> BTW, I wonder if this design is still valid with the uclamp_max.
>
> Regards,
> Lukasz