drivers/cpuidle/governors/teo.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
From: Rafael J. Wysocki <rafael.j.wysocki@intel.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
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@intel.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)
+ goto constraint;
}
/*
@@ -410,6 +419,7 @@ static int teo_select(struct cpuidle_dri
}
}
+constraint:
/*
* If there is a latency constraint, it may be necessary to select an
* idle state shallower than the current candidate one.On 1/14/26 19:44, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.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 > 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@intel.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. Other than that LGTM and with that check and the nit above: Reviewed-by: Christian Loehle <christian.loehle@arm.com> > + goto constraint; > } > > /* > @@ -410,6 +419,7 @@ static int teo_select(struct cpuidle_dri > } > } > > +constraint: > /* > * If there is a latency constraint, it may be necessary to select an > * idle state shallower than the current candidate one. > > >
On Wed, Jan 21, 2026 at 2:10 PM Christian Loehle <christian.loehle@arm.com> wrote: > > On 1/14/26 19:44, Rafael J. Wysocki wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.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@intel.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. So I think I only need to fix the typo above. > Other than that LGTM and with that check and the nit above: > > Reviewed-by: Christian Loehle <christian.loehle@arm.com> Thanks! > > + goto constraint; > > } > > > > /* > > @@ -410,6 +419,7 @@ static int teo_select(struct cpuidle_dri > > } > > } > > > > +constraint: > > /* > > * If there is a latency constraint, it may be necessary to select an > > * idle state shallower than the current candidate one. > > > > > > >
On 1/23/26 20:46, Rafael J. Wysocki wrote:
> On Wed, Jan 21, 2026 at 2:10 PM Christian Loehle
> <christian.loehle@arm.com> wrote:
>>
>> On 1/14/26 19:44, Rafael J. Wysocki wrote:
>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.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@intel.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?
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.
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
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@arm.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
--
2.34.1
On Mon, Jan 26, 2026 at 10:18 AM Christian Loehle
<christian.loehle@arm.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@arm.com> wrote:
> >>
> >> On 1/14/26 19:44, Rafael J. Wysocki wrote:
> >>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.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@intel.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@arm.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.
On 1/26/26 11:40, Rafael J. Wysocki wrote:
> On Mon, Jan 26, 2026 at 10:18 AM Christian Loehle
> <christian.loehle@arm.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@arm.com> wrote:
>>>>
>>>> On 1/14/26 19:44, Rafael J. Wysocki wrote:
>>>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.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@intel.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.
>
Duh you're right, sorry about the noise :/
© 2016 - 2026 Red Hat, Inc.