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

[Rafael J. Wysocki]

On Wed, Jan 15, 2025 at 9:48 PM Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote:
>
> On Wed, Jan 15, 2025 at 8:20 PM Christian Loehle
> <christian.loehle@xxxxxxx> wrote:
> >
> > On 1/15/25 15:54, Rafael J. Wysocki wrote:
> > > On Wed, Jan 15, 2025 at 3:46 PM Christian Loehle
> > > <christian.loehle@xxxxxxx> wrote:
> > >>
> > >> On 1/13/25 18:36, Rafael J. Wysocki wrote:
> > >>> From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> > >>>
> > >>> 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.
> >
> > I would hope constraints change nowhere near as frequently as
> > idle entry / exit happen, is your experience different?
>
> They don't, but they may change at any time and it is kind of good to
> have history in case this happens.
>
> > >
> > >>>
> > >>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> > >>> ---
> > >>>
> > >>> This is a rebased variant of
> > >>>
> > >>> https://lore.kernel.org/linux-pm/8476650.T7Z3S40VBb@xxxxxxxxxxxxx/
> > >>>
> > >>> ---
> > >>>  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?
> >
> > Saves querying the sleep length in case of constraint_idx == 0, i.e.
> > qos request to be very latency-sensitive and us actually adding latency
> > here.
>
> Fair enough, but before patch [7/9] leaving it where it is doesn't
> really cause it to skip the sleep length check unless state 0 is
> "polling".
>
> After patch [7/9] it is possible to add a constraint_idx check against
> 0 to the "goto out_tick" condition before the
> tick_nohz_get_sleep_length() call, that is
>
> if ((!idx || drv->states[idx].target_residency_ns < RESIDENCY_THRESHOLD_NS) &&
>     (2 * cpu_data->short_idle >= cpu_data->total || !constraint_idx))
>         goto out_tick;

Or even

if ((!idx || drv->states[idx].target_residency_ns < RESIDENCY_THRESHOLD_NS) &&
    (2 * cpu_data->short_idle >= cpu_data->total || latency_req <
A_SMALL_VALUE))
        goto out_tick;

for that matter.

> but that would be a separate patch if you will.