[PATCH v1 1/4] cpuidle: governors: teo: Drop incorrect target residency check

Posted by Rafael J. Wysocki 2 months, 3 weeks ago

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

When the target residency of the current candidate idle state is
greater than the expected time till the closest timer (the sleep
length), it does not matter whether or not the tick has already
been stopped or if it is going to be stopped.  The closest timer
will trigger anyway at its due time, so it does not make sense to
select an idle state with target residency above the sleep length.

Accordingly, drop the teo_state_ok() check done in that case and
let the governor use the teo_find_shallower_state() return value
as the new candidate idle state index.

Fixes: 21d28cd2fa5f ("cpuidle: teo: Do not call tick_nohz_get_sleep_length() upfront")
Cc: All applicable <stable@vger.kernel.org>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpuidle/governors/teo.c |    7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

--- a/drivers/cpuidle/governors/teo.c
+++ b/drivers/cpuidle/governors/teo.c
@@ -458,11 +458,8 @@ static int teo_select(struct cpuidle_dri
 	 * If the closest expected timer is before the target residency of the
 	 * candidate state, a shallower one needs to be found.
 	 */
-	if (drv->states[idx].target_residency_ns > duration_ns) {
-		i = teo_find_shallower_state(drv, dev, idx, duration_ns, false);
-		if (teo_state_ok(i, drv))
-			idx = i;
-	}
+	if (drv->states[idx].target_residency_ns > duration_ns)
+		idx = teo_find_shallower_state(drv, dev, idx, duration_ns, false);
 
 	/*
 	 * If the selected state's target residency is below the tick length

Re: [PATCH v1 1/4] cpuidle: governors: teo: Drop incorrect target residency check

Posted by Christian Loehle 2 months, 3 weeks ago

On 11/12/25 16:22, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> When the target residency of the current candidate idle state is
> greater than the expected time till the closest timer (the sleep
> length), it does not matter whether or not the tick has already
> been stopped or if it is going to be stopped.  The closest timer
> will trigger anyway at its due time, so it does not make sense to
> select an idle state with target residency above the sleep length.
> 
> Accordingly, drop the teo_state_ok() check done in that case and
> let the governor use the teo_find_shallower_state() return value
> as the new candidate idle state index.
> 
> Fixes: 21d28cd2fa5f ("cpuidle: teo: Do not call tick_nohz_get_sleep_length() upfront")
> Cc: All applicable <stable@vger.kernel.org>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/cpuidle/governors/teo.c |    7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> --- a/drivers/cpuidle/governors/teo.c
> +++ b/drivers/cpuidle/governors/teo.c
> @@ -458,11 +458,8 @@ static int teo_select(struct cpuidle_dri
>  	 * If the closest expected timer is before the target residency of the
>  	 * candidate state, a shallower one needs to be found.
>  	 */
> -	if (drv->states[idx].target_residency_ns > duration_ns) {
> -		i = teo_find_shallower_state(drv, dev, idx, duration_ns, false);
> -		if (teo_state_ok(i, drv))
> -			idx = i;
> -	}
> +	if (drv->states[idx].target_residency_ns > duration_ns)
> +		idx = teo_find_shallower_state(drv, dev, idx, duration_ns, false);
>  
>  	/*
>  	 * If the selected state's target residency is below the tick length
> 
> 
> 

AFAICT this check was to not be stuck in a shallow state when tick is already disabled.
There might be a timer armed in t+500us but that might still get cancelled, which
is why we didn't think a below TICK_NSEC 'shallow' state is acceptable?

Re: [PATCH v1 1/4] cpuidle: governors: teo: Drop incorrect target residency check

Posted by Rafael J. Wysocki 2 months, 3 weeks ago

On Thu, Nov 13, 2025 at 12:32 PM Christian Loehle
<christian.loehle@arm.com> wrote:
>
> On 11/12/25 16:22, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > When the target residency of the current candidate idle state is
> > greater than the expected time till the closest timer (the sleep
> > length), it does not matter whether or not the tick has already
> > been stopped or if it is going to be stopped.  The closest timer
> > will trigger anyway at its due time, so it does not make sense to
> > select an idle state with target residency above the sleep length.
> >
> > Accordingly, drop the teo_state_ok() check done in that case and
> > let the governor use the teo_find_shallower_state() return value
> > as the new candidate idle state index.
> >
> > Fixes: 21d28cd2fa5f ("cpuidle: teo: Do not call tick_nohz_get_sleep_length() upfront")
> > Cc: All applicable <stable@vger.kernel.org>
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  drivers/cpuidle/governors/teo.c |    7 ++-----
> >  1 file changed, 2 insertions(+), 5 deletions(-)
> >
> > --- a/drivers/cpuidle/governors/teo.c
> > +++ b/drivers/cpuidle/governors/teo.c
> > @@ -458,11 +458,8 @@ static int teo_select(struct cpuidle_dri
> >        * If the closest expected timer is before the target residency of the
> >        * candidate state, a shallower one needs to be found.
> >        */
> > -     if (drv->states[idx].target_residency_ns > duration_ns) {
> > -             i = teo_find_shallower_state(drv, dev, idx, duration_ns, false);
> > -             if (teo_state_ok(i, drv))
> > -                     idx = i;
> > -     }
> > +     if (drv->states[idx].target_residency_ns > duration_ns)
> > +             idx = teo_find_shallower_state(drv, dev, idx, duration_ns, false);
> >
> >       /*
> >        * If the selected state's target residency is below the tick length
> >
> >
> >
>
> AFAICT this check was to not be stuck in a shallow state when tick is already disabled.
> There might be a timer armed in t+500us but that might still get cancelled, which
> is why we didn't think a below TICK_NSEC 'shallow' state is acceptable?

This is all about hrtimers which are not expected to be canceled too
often and real energy is wasted here by going too deep if the timer is
not canceled.

Re: [PATCH v1 1/4] cpuidle: governors: teo: Drop incorrect target residency check

Posted by Rafael J. Wysocki 2 months, 3 weeks ago

On Thu, Nov 13, 2025 at 12:41 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Thu, Nov 13, 2025 at 12:32 PM Christian Loehle
> <christian.loehle@arm.com> wrote:
> >
> > On 11/12/25 16:22, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > >
> > > When the target residency of the current candidate idle state is
> > > greater than the expected time till the closest timer (the sleep
> > > length), it does not matter whether or not the tick has already
> > > been stopped or if it is going to be stopped.  The closest timer
> > > will trigger anyway at its due time, so it does not make sense to
> > > select an idle state with target residency above the sleep length.
> > >
> > > Accordingly, drop the teo_state_ok() check done in that case and
> > > let the governor use the teo_find_shallower_state() return value
> > > as the new candidate idle state index.
> > >
> > > Fixes: 21d28cd2fa5f ("cpuidle: teo: Do not call tick_nohz_get_sleep_length() upfront")
> > > Cc: All applicable <stable@vger.kernel.org>
> > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > ---
> > >  drivers/cpuidle/governors/teo.c |    7 ++-----
> > >  1 file changed, 2 insertions(+), 5 deletions(-)
> > >
> > > --- a/drivers/cpuidle/governors/teo.c
> > > +++ b/drivers/cpuidle/governors/teo.c
> > > @@ -458,11 +458,8 @@ static int teo_select(struct cpuidle_dri
> > >        * If the closest expected timer is before the target residency of the
> > >        * candidate state, a shallower one needs to be found.
> > >        */
> > > -     if (drv->states[idx].target_residency_ns > duration_ns) {
> > > -             i = teo_find_shallower_state(drv, dev, idx, duration_ns, false);
> > > -             if (teo_state_ok(i, drv))
> > > -                     idx = i;
> > > -     }
> > > +     if (drv->states[idx].target_residency_ns > duration_ns)
> > > +             idx = teo_find_shallower_state(drv, dev, idx, duration_ns, false);
> > >
> > >       /*
> > >        * If the selected state's target residency is below the tick length
> > >
> > >
> > >
> >
> > AFAICT this check was to not be stuck in a shallow state when tick is already disabled.
> > There might be a timer armed in t+500us but that might still get cancelled, which
> > is why we didn't think a below TICK_NSEC 'shallow' state is acceptable?
>
> This is all about hrtimers which are not expected to be canceled too
> often and real energy is wasted here by going too deep if the timer is
> not canceled.

Overall, both teo and menu assume that the timers reported by
tick_nohz_get_sleep_length() will trigger.  Otherwise, calling it
would be kind of pointless ...

Re: [PATCH v1 1/4] cpuidle: governors: teo: Drop incorrect target residency check

Posted by Rafael J. Wysocki 2 months, 3 weeks ago

On Thu, Nov 13, 2025 at 12:47 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Thu, Nov 13, 2025 at 12:41 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Thu, Nov 13, 2025 at 12:32 PM Christian Loehle
> > <christian.loehle@arm.com> wrote:
> > >
> > > On 11/12/25 16:22, Rafael J. Wysocki wrote:
> > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > >
> > > > When the target residency of the current candidate idle state is
> > > > greater than the expected time till the closest timer (the sleep
> > > > length), it does not matter whether or not the tick has already
> > > > been stopped or if it is going to be stopped.  The closest timer
> > > > will trigger anyway at its due time, so it does not make sense to
> > > > select an idle state with target residency above the sleep length.
> > > >
> > > > Accordingly, drop the teo_state_ok() check done in that case and
> > > > let the governor use the teo_find_shallower_state() return value
> > > > as the new candidate idle state index.
> > > >
> > > > Fixes: 21d28cd2fa5f ("cpuidle: teo: Do not call tick_nohz_get_sleep_length() upfront")
> > > > Cc: All applicable <stable@vger.kernel.org>
> > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > ---
> > > >  drivers/cpuidle/governors/teo.c |    7 ++-----
> > > >  1 file changed, 2 insertions(+), 5 deletions(-)
> > > >
> > > > --- a/drivers/cpuidle/governors/teo.c
> > > > +++ b/drivers/cpuidle/governors/teo.c
> > > > @@ -458,11 +458,8 @@ static int teo_select(struct cpuidle_dri
> > > >        * If the closest expected timer is before the target residency of the
> > > >        * candidate state, a shallower one needs to be found.
> > > >        */
> > > > -     if (drv->states[idx].target_residency_ns > duration_ns) {
> > > > -             i = teo_find_shallower_state(drv, dev, idx, duration_ns, false);
> > > > -             if (teo_state_ok(i, drv))
> > > > -                     idx = i;
> > > > -     }
> > > > +     if (drv->states[idx].target_residency_ns > duration_ns)
> > > > +             idx = teo_find_shallower_state(drv, dev, idx, duration_ns, false);
> > > >
> > > >       /*
> > > >        * If the selected state's target residency is below the tick length
> > > >
> > > >
> > > >
> > >
> > > AFAICT this check was to not be stuck in a shallow state when tick is already disabled.
> > > There might be a timer armed in t+500us but that might still get cancelled, which
> > > is why we didn't think a below TICK_NSEC 'shallow' state is acceptable?
> >
> > This is all about hrtimers which are not expected to be canceled too
> > often and real energy is wasted here by going too deep if the timer is
> > not canceled.
>
> Overall, both teo and menu assume that the timers reported by
> tick_nohz_get_sleep_length() will trigger.  Otherwise, calling it
> would be kind of pointless ...

Anyway, I've sent a v2 of the $subject patch with a more elaborate changelog:

https://lore.kernel.org/linux-pm/5955081.DvuYhMxLoT@rafael.j.wysocki/

Hopefully, it is more convincing.

[PATCH v2 1/4] cpuidle: governors: teo: Drop misguided target residency check

Posted by Rafael J. Wysocki 2 months, 3 weeks ago

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

When the target residency of the current candidate idle state is
greater than the expected time till the closest timer (the sleep
length), it does not matter whether or not the tick has already been
stopped or if it is going to be stopped.  The closest timer will
trigger anyway at its due time, so if an idle state with target
residency above the sleep length is selected, energy will be wasted
and there may be excess latency.

Of course, if the closest timer were canceled before it could trigger,
a deeper idle state would be more suitable, but this is not expected
to happen (generally speaking, hrtimers are not expected to be
canceled as a rule).

Accordingly, the teo_state_ok() check done in that case causes energy to
be wasted more often than it allows any energy to be saved (if it allows
any energy to be saved at all), so drop it and let the governor use the
teo_find_shallower_state() return value as the new candidate idle state
index.

Fixes: 21d28cd2fa5f ("cpuidle: teo: Do not call tick_nohz_get_sleep_length() upfront")
Cc: All applicable <stable@vger.kernel.org>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

v1 -> v2: Subject and changelog modifications

---
 drivers/cpuidle/governors/teo.c |    7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

--- a/drivers/cpuidle/governors/teo.c
+++ b/drivers/cpuidle/governors/teo.c
@@ -458,11 +458,8 @@ static int teo_select(struct cpuidle_dri
 	 * If the closest expected timer is before the target residency of the
 	 * candidate state, a shallower one needs to be found.
 	 */
-	if (drv->states[idx].target_residency_ns > duration_ns) {
-		i = teo_find_shallower_state(drv, dev, idx, duration_ns, false);
-		if (teo_state_ok(i, drv))
-			idx = i;
-	}
+	if (drv->states[idx].target_residency_ns > duration_ns)
+		idx = teo_find_shallower_state(drv, dev, idx, duration_ns, false);
 
 	/*
 	 * If the selected state's target residency is below the tick length

Re: [PATCH v2 1/4] cpuidle: governors: teo: Drop misguided target residency check

Posted by Christian Loehle 2 months, 3 weeks ago

On 11/13/25 13:24, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> When the target residency of the current candidate idle state is
> greater than the expected time till the closest timer (the sleep
> length), it does not matter whether or not the tick has already been
> stopped or if it is going to be stopped.  The closest timer will
> trigger anyway at its due time, so if an idle state with target
> residency above the sleep length is selected, energy will be wasted
> and there may be excess latency.
> 
> Of course, if the closest timer were canceled before it could trigger,
> a deeper idle state would be more suitable, but this is not expected
> to happen (generally speaking, hrtimers are not expected to be
> canceled as a rule).
> 
> Accordingly, the teo_state_ok() check done in that case causes energy to
> be wasted more often than it allows any energy to be saved (if it allows
> any energy to be saved at all), so drop it and let the governor use the
> teo_find_shallower_state() return value as the new candidate idle state
> index.
> 
> Fixes: 21d28cd2fa5f ("cpuidle: teo: Do not call tick_nohz_get_sleep_length() upfront")
> Cc: All applicable <stable@vger.kernel.org>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> 
> v1 -> v2: Subject and changelog modifications
> 
> ---
>  drivers/cpuidle/governors/teo.c |    7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> --- a/drivers/cpuidle/governors/teo.c
> +++ b/drivers/cpuidle/governors/teo.c
> @@ -458,11 +458,8 @@ static int teo_select(struct cpuidle_dri
>  	 * If the closest expected timer is before the target residency of the
>  	 * candidate state, a shallower one needs to be found.
>  	 */
> -	if (drv->states[idx].target_residency_ns > duration_ns) {
> -		i = teo_find_shallower_state(drv, dev, idx, duration_ns, false);
> -		if (teo_state_ok(i, drv))
> -			idx = i;
> -	}
> +	if (drv->states[idx].target_residency_ns > duration_ns)
> +		idx = teo_find_shallower_state(drv, dev, idx, duration_ns, false);
>  
>  	/*
>  	 * If the selected state's target residency is below the tick length
> 

Reviewed-by: Christian Loehle <christian.loehle@arm.com>