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

[Rafael J. Wysocki]

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;

but that would be a separate patch if you will.