linux-kernel - Re: [PATCH v1 2/9] cpuidle: teo: Reorder candidate state index checks

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: <CAJZ5v0iVOg5CnYo8OQ5E8VGLdn4cvVdFFQqpOgpWvij4a4cdxQ@mail.gmail.com>
Date: Wed, 15 Jan 2025 16:54:53 +0100
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Christian Loehle <christian.loehle@....com>
Cc: "Rafael J. Wysocki" <rjw@...ysocki.net>, Linux PM <linux-pm@...r.kernel.org>, 
	LKML <linux-kernel@...r.kernel.org>, Daniel Lezcano <daniel.lezcano@...aro.org>, 
	Artem Bityutskiy <artem.bityutskiy@...ux.intel.com>
Subject: Re: [PATCH v1 2/9] cpuidle: teo: Reorder candidate state index checks

On Wed, Jan 15, 2025 at 3:46 PM Christian Loehle
<christian.loehle@....com> wrote:
>
> On 1/13/25 18:36, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> >
> > Since constraint_idx may be 0, the candidate state index may change to 0
> > after assigning constraint_idx to it, so first check if it is greater
> > than constraint_idx (and update it if so) and then check it against 0.
>
> So the reason I've left this where it was is because the prev_intercept_idx
> was supposed to query the sleep length if we're in an majority-intercept
> period and then it makes sense to query the sleep length (to detect such
> a period being over).
> A constraint_idx == 0 scenario doesn't need the intercept-machinery to
> work at all, why are we querying the sleep length then?

In case the constraint is different next time and it's better to know
the sleep length to properly classify the wakeup.

> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> > ---
> >
> > This is a rebased variant of
> >
> > https://lore.kernel.org/linux-pm/8476650.T7Z3S40VBb@rjwysocki.net/
> >
> > ---
> >  drivers/cpuidle/governors/teo.c |   15 ++++++++-------
> >  1 file changed, 8 insertions(+), 7 deletions(-)
> >
> > --- a/drivers/cpuidle/governors/teo.c
> > +++ b/drivers/cpuidle/governors/teo.c
> > @@ -428,6 +428,14 @@
> >                               break;
> >               }
> >       }
> > +
> > +     /*
> > +      * If there is a latency constraint, it may be necessary to select an
> > +      * idle state shallower than the current candidate one.
> > +      */
> > +     if (idx > constraint_idx)
> > +             idx = constraint_idx;
> > +
> >       if (!idx && prev_intercept_idx) {
> >               /*
> >                * We have to query the sleep length here otherwise we don't
> > @@ -439,13 +447,6 @@
> >       }
> >
> >       /*
> > -      * If there is a latency constraint, it may be necessary to select an
> > -      * idle state shallower than the current candidate one.
> > -      */
> > -     if (idx > constraint_idx)
> > -             idx = constraint_idx;
> > -
> > -     /*
>
> We could leave this here and just do goto end;?

Why would this be better?