Message-ID: <CAJZ5v0iygK59hvYKfNy0AYXe4Nyn-kM3dnY-9ongeGWc_jZ+3Q@mail.gmail.com>
Date: Mon, 26 Jan 2026 12:40:37 +0100
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Christian Loehle <christian.loehle@....com>
Cc: "Rafael J. Wysocki" <rafael@...nel.org>, Linux PM <linux-pm@...r.kernel.org>, 
	LKML <linux-kernel@...r.kernel.org>, Doug Smythies <dsmythies@...us.net>
Subject: Re: [PATCH v1 1/5] cpuidle: governors: teo: Avoid selecting states
 with zero-size bins

On Mon, Jan 26, 2026 at 10:18 AM Christian Loehle
<christian.loehle@....com> wrote:
>
> On 1/23/26 20:46, Rafael J. Wysocki wrote:
> > On Wed, Jan 21, 2026 at 2:10 PM Christian Loehle
> > <christian.loehle@....com> wrote:
> >>
> >> On 1/14/26 19:44, Rafael J. Wysocki wrote:
> >>> From: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> >>>
> >>> If the last two enabled idle states have the same target residency which
> >>> is at least equal to TICK_NSET, teo may select the next-to-last one even
> >>
> >> s/TICK_NSET/TICK_NSEC
> >
> > Yup, thanks!
> >
> >>> though the size of that state's bin is 0, which is confusing.
> >>>
> >>> Prevent that from happening by adding a target residency check to the
> >>> relevant code path.
> >>>
> >>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> >>> ---
> >>>  drivers/cpuidle/governors/teo.c |   10 ++++++++++
> >>>  1 file changed, 10 insertions(+)
> >>>
> >>> --- a/drivers/cpuidle/governors/teo.c
> >>> +++ b/drivers/cpuidle/governors/teo.c
> >>> @@ -388,6 +388,15 @@ static int teo_select(struct cpuidle_dri
> >>>                       while (min_idx < idx &&
> >>>                              drv->states[min_idx].target_residency_ns < TICK_NSEC)
> >>>                               min_idx++;
> >>> +
> >>> +                     /*
> >>> +                      * Avoid selecting a state with a lower index, but with
> >>> +                      * the same target residency as the current candidate
> >>> +                      * one.
> >>> +                      */
> >>> +                     if (drv->states[min_idx].target_residency_ns ==
> >>> +                                     drv->states[idx].target_residency_ns)
> >>
> >> We need to check that min_idx isn't disabled though, otherwise we now skip a
> >> potential (enabled) idx==1 if min_idx==2 and min_idx is disabled.
> >
> > Not really because idx is the current candidate state and it is
> > enabled.  We'll use idx if this check is true, not min_idx.
> >
>
> Are you sure?

Yeah, pretty much.

> I meant initially:
>
>                 for (i = intercept_max_idx; i >= min_idx; i--) {
>                         intercept_sum += cpu_data->state_bins[i].intercepts;
>
>                         if (dev->states_usage[i].disable)
>                                 continue;
>
>                         idx = i;
>                         if (2 * intercept_sum > idx_intercept_sum)
>                                 break;
>                 }
> might skip an idx==3 if it enters with min_idx==2 (sorry, messed up the +-1 in the initial mail)
> even though idx==3 might have the same residency as idx==2.
> So if idx==2 is disabled we could've selected idx==3, but now won't and will go for idx==1 or
> whatever is the next shallower enabled state.

But that's after patch [5/5] that has problems (I have a new version
of it ready to send, will do that later today).

Look at the original 6.19-rc code and patch [1/1] by itself and you'll
see what I mean.

Patch [1/1] only adds a check and a jump to a new label (at which
point idx is going to be used), nothing more.

> Additionally an issue with this and patch 5/5:
>
>                 if (min_idx >= intercept_max_idx) {
>                         idx = min_idx;
>                         goto constraint; // CL: this will just select min_idx
>                 }
>
> will use min_idx even though it might be disabled and also the scenario

Again, after patch [5/5] which adds the code quoted above.

> I think we should just add something like
>
> ------8<-------
>
> cpuidle: teo: Fix intercept-logic selecting disabled
>
> Prevent min_idx to be set to a disabled state, which could lead to
> both a disabled state being returned by teo, but also an equally good
> state being skipped because it has a higher index than a disabled state.
>
> Signed-off-by: Christian Loehle <christian.loehle@....com>
> ---
>  drivers/cpuidle/governors/teo.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/cpuidle/governors/teo.c b/drivers/cpuidle/governors/teo.c
> index 4cf6302f99ad..94c5ef5df467 100644
> --- a/drivers/cpuidle/governors/teo.c
> +++ b/drivers/cpuidle/governors/teo.c
> @@ -420,9 +420,11 @@ static int teo_select(struct cpuidle_driver *drv, struct cpuidle_device *dev,
>                          * candidate one whose target residency is at least
>                          * equal to the tick period length.
>                          */
> -                       while (min_idx < idx &&
> -                              drv->states[min_idx].target_residency_ns < TICK_NSEC)
> -                               min_idx++;
> +                       while (i < idx && drv->states[i].target_residency_ns < TICK_NSEC) {
> +                               i++;
> +                               if (!dev->states_usage[i].disable)
> +                                       min_idx = i;
> +                       }
>
>                         /*
>                          * Avoid selecting a state with a lower index, but with
> --

After fixing patch [5/5] that's not needed any more.

Powered by blists - more mailing lists