[PATCH v1] sched: idle: Make skipping governor callbacks more consistent

Posted by Rafael J. Wysocki 1 month ago

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

If the cpuidle governor .select() callback is skipped because there
is only one idle state in the cpuidle driver, the .reflect() callback
should be skipped as well, at least for consistency (if not for
correctness), so do it.

Fixes: e5c9ffc6ae1b ("cpuidle: Skip governor when only one idle state is available")
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpuidle/cpuidle.c |   10 ----------
 kernel/sched/idle.c       |   11 ++++++++++-
 2 files changed, 10 insertions(+), 11 deletions(-)

--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -359,16 +359,6 @@ noinstr int cpuidle_enter_state(struct c
 int cpuidle_select(struct cpuidle_driver *drv, struct cpuidle_device *dev,
 		   bool *stop_tick)
 {
-	/*
-	 * If there is only a single idle state (or none), there is nothing
-	 * meaningful for the governor to choose. Skip the governor and
-	 * always use state 0 with the tick running.
-	 */
-	if (drv->state_count <= 1) {
-		*stop_tick = false;
-		return 0;
-	}
-
 	return cpuidle_curr_governor->select(drv, dev, stop_tick);
 }
 
--- a/kernel/sched/idle.c
+++ b/kernel/sched/idle.c
@@ -221,7 +221,7 @@ static void cpuidle_idle_call(void)
 
 		next_state = cpuidle_find_deepest_state(drv, dev, max_latency_ns);
 		call_cpuidle(drv, dev, next_state);
-	} else {
+	} else if (drv->state_count > 1) {
 		bool stop_tick = true;
 
 		/*
@@ -239,6 +239,15 @@ static void cpuidle_idle_call(void)
 		 * Give the governor an opportunity to reflect on the outcome
 		 */
 		cpuidle_reflect(dev, entered_state);
+	} else {
+		tick_nohz_idle_retain_tick();
+
+		/*
+		 * If there is only a single idle state (or none), there is
+		 * nothing meaningful for the governor to choose.  Skip the
+		 * governor and always use state 0.
+		 */
+		call_cpuidle(drv, dev, 0);
 	}
 
 exit_idle:

Re: [PATCH v1] sched: idle: Make skipping governor callbacks more consistent

Posted by Frederic Weisbecker 4 weeks, 1 day ago

Le Sat, Mar 07, 2026 at 05:12:05PM +0100, Rafael J. Wysocki a écrit :
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> If the cpuidle governor .select() callback is skipped because there
> is only one idle state in the cpuidle driver, the .reflect() callback
> should be skipped as well, at least for consistency (if not for
> correctness), so do it.
> 
> Fixes: e5c9ffc6ae1b ("cpuidle: Skip governor when only one idle state is available")
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Reviewed-by: Frederic Weisbecker <frederic@kernel.org>

-- 
Frederic Weisbecker
SUSE Labs

Re: [PATCH v1] sched: idle: Make skipping governor callbacks more consistent

Posted by Aboorva Devarajan 1 month ago

On Sat, 2026-03-07 at 17:12 +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> If the cpuidle governor .select() callback is skipped because there
> is only one idle state in the cpuidle driver, the .reflect() callback
> should be skipped as well, at least for consistency (if not for
> correctness), so do it.
> 
> Fixes: e5c9ffc6ae1b ("cpuidle: Skip governor when only one idle state
> is available")
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/cpuidle/cpuidle.c |   10 ----------
>  kernel/sched/idle.c       |   11 ++++++++++-
>  2 files changed, 10 insertions(+), 11 deletions(-)
> 
> --- a/drivers/cpuidle/cpuidle.c
> +++ b/drivers/cpuidle/cpuidle.c
> @@ -359,16 +359,6 @@ noinstr int cpuidle_enter_state(struct c
>  int cpuidle_select(struct cpuidle_driver *drv, struct cpuidle_device
> *dev,
>  		   bool *stop_tick)
>  {
> -	/*
> -	 * If there is only a single idle state (or none), there is
> nothing
> -	 * meaningful for the governor to choose. Skip the governor
> and
> -	 * always use state 0 with the tick running.
> -	 */
> -	if (drv->state_count <= 1) {
> -		*stop_tick = false;
> -		return 0;
> -	}
> -
>  	return cpuidle_curr_governor->select(drv, dev, stop_tick);
>  }
>  
> --- a/kernel/sched/idle.c
> +++ b/kernel/sched/idle.c
> @@ -221,7 +221,7 @@ static void cpuidle_idle_call(void)
>  
>  		next_state = cpuidle_find_deepest_state(drv, dev,
> max_latency_ns);
>  		call_cpuidle(drv, dev, next_state);
> -	} else {
> +	} else if (drv->state_count > 1) {
>  		bool stop_tick = true;
>  
>  		/*
> @@ -239,6 +239,15 @@ static void cpuidle_idle_call(void)
>  		 * Give the governor an opportunity to reflect on
> the outcome
>  		 */
>  		cpuidle_reflect(dev, entered_state);
> +	} else {
> +		tick_nohz_idle_retain_tick();
> +
> +		/*
> +		 * If there is only a single idle state (or none),
> there is
> +		 * nothing meaningful for the governor to choose. 
> Skip the
> +		 * governor and always use state 0.
> +		 */
> +		call_cpuidle(drv, dev, 0);
>  	}
>  
>  exit_idle:
> 
> 

Hi Rafael,

Thanks for fixing this, sorry I missed it earlier.

Reviewed-by: Aboorva Devarajan <aboorvad@linux.ibm.com>

Regards,
Aboorva

Re: [PATCH v1] sched: idle: Make skipping governor callbacks more consistent

Posted by Christian Loehle 1 month ago

On 3/7/26 16:12, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> If the cpuidle governor .select() callback is skipped because there
> is only one idle state in the cpuidle driver, the .reflect() callback
> should be skipped as well, at least for consistency (if not for
> correctness), so do it.
> 
> Fixes: e5c9ffc6ae1b ("cpuidle: Skip governor when only one idle state is available")
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/cpuidle/cpuidle.c |   10 ----------
>  kernel/sched/idle.c       |   11 ++++++++++-
>  2 files changed, 10 insertions(+), 11 deletions(-)
> 
> --- a/drivers/cpuidle/cpuidle.c
> +++ b/drivers/cpuidle/cpuidle.c
> @@ -359,16 +359,6 @@ noinstr int cpuidle_enter_state(struct c
>  int cpuidle_select(struct cpuidle_driver *drv, struct cpuidle_device *dev,
>  		   bool *stop_tick)
>  {
> -	/*
> -	 * If there is only a single idle state (or none), there is nothing
> -	 * meaningful for the governor to choose. Skip the governor and
> -	 * always use state 0 with the tick running.
> -	 */
> -	if (drv->state_count <= 1) {
> -		*stop_tick = false;
> -		return 0;
> -	}
> -
>  	return cpuidle_curr_governor->select(drv, dev, stop_tick);
>  }
>  
> --- a/kernel/sched/idle.c
> +++ b/kernel/sched/idle.c
> @@ -221,7 +221,7 @@ static void cpuidle_idle_call(void)
>  
>  		next_state = cpuidle_find_deepest_state(drv, dev, max_latency_ns);
>  		call_cpuidle(drv, dev, next_state);
> -	} else {
> +	} else if (drv->state_count > 1) {
>  		bool stop_tick = true;
>  
>  		/*
> @@ -239,6 +239,15 @@ static void cpuidle_idle_call(void)
>  		 * Give the governor an opportunity to reflect on the outcome
>  		 */
>  		cpuidle_reflect(dev, entered_state);
> +	} else {
> +		tick_nohz_idle_retain_tick();
> +
> +		/*
> +		 * If there is only a single idle state (or none), there is
> +		 * nothing meaningful for the governor to choose.  Skip the
> +		 * governor and always use state 0.
> +		 */
> +		call_cpuidle(drv, dev, 0);
>  	}
>  
>  exit_idle:
> 
> 
> 

Duh, good catch.
Reviewed-by: Christian Loehle <christian.loehle@arm.com>

Re: [PATCH v1] sched: idle: Make skipping governor callbacks more consistent

Posted by Rafael J. Wysocki 1 month ago

On Mon, Mar 9, 2026 at 10:13 AM Christian Loehle
<christian.loehle@arm.com> wrote:
>
> On 3/7/26 16:12, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > If the cpuidle governor .select() callback is skipped because there
> > is only one idle state in the cpuidle driver, the .reflect() callback
> > should be skipped as well, at least for consistency (if not for
> > correctness), so do it.
> >
> > Fixes: e5c9ffc6ae1b ("cpuidle: Skip governor when only one idle state is available")
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  drivers/cpuidle/cpuidle.c |   10 ----------
> >  kernel/sched/idle.c       |   11 ++++++++++-
> >  2 files changed, 10 insertions(+), 11 deletions(-)
> >
> > --- a/drivers/cpuidle/cpuidle.c
> > +++ b/drivers/cpuidle/cpuidle.c
> > @@ -359,16 +359,6 @@ noinstr int cpuidle_enter_state(struct c
> >  int cpuidle_select(struct cpuidle_driver *drv, struct cpuidle_device *dev,
> >                  bool *stop_tick)
> >  {
> > -     /*
> > -      * If there is only a single idle state (or none), there is nothing
> > -      * meaningful for the governor to choose. Skip the governor and
> > -      * always use state 0 with the tick running.
> > -      */
> > -     if (drv->state_count <= 1) {
> > -             *stop_tick = false;
> > -             return 0;
> > -     }
> > -
> >       return cpuidle_curr_governor->select(drv, dev, stop_tick);
> >  }
> >
> > --- a/kernel/sched/idle.c
> > +++ b/kernel/sched/idle.c
> > @@ -221,7 +221,7 @@ static void cpuidle_idle_call(void)
> >
> >               next_state = cpuidle_find_deepest_state(drv, dev, max_latency_ns);
> >               call_cpuidle(drv, dev, next_state);
> > -     } else {
> > +     } else if (drv->state_count > 1) {
> >               bool stop_tick = true;
> >
> >               /*
> > @@ -239,6 +239,15 @@ static void cpuidle_idle_call(void)
> >                * Give the governor an opportunity to reflect on the outcome
> >                */
> >               cpuidle_reflect(dev, entered_state);
> > +     } else {
> > +             tick_nohz_idle_retain_tick();
> > +
> > +             /*
> > +              * If there is only a single idle state (or none), there is
> > +              * nothing meaningful for the governor to choose.  Skip the
> > +              * governor and always use state 0.
> > +              */
> > +             call_cpuidle(drv, dev, 0);
> >       }
> >
> >  exit_idle:
> >
> >
> >
>
> Duh, good catch.
> Reviewed-by: Christian Loehle <christian.loehle@arm.com>

OK, so any objections or concerns from anyone?

I'm about to queue this up for the next -rc.

Re: [PATCH v1] sched: idle: Make skipping governor callbacks more consistent

Posted by Qais Yousef 1 month ago

On 03/09/26 13:26, Rafael J. Wysocki wrote:
> On Mon, Mar 9, 2026 at 10:13 AM Christian Loehle
> <christian.loehle@arm.com> wrote:
> >
> > On 3/7/26 16:12, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > >
> > > If the cpuidle governor .select() callback is skipped because there
> > > is only one idle state in the cpuidle driver, the .reflect() callback
> > > should be skipped as well, at least for consistency (if not for
> > > correctness), so do it.
> > >
> > > Fixes: e5c9ffc6ae1b ("cpuidle: Skip governor when only one idle state is available")
> > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > ---
> > >  drivers/cpuidle/cpuidle.c |   10 ----------
> > >  kernel/sched/idle.c       |   11 ++++++++++-
> > >  2 files changed, 10 insertions(+), 11 deletions(-)
> > >
> > > --- a/drivers/cpuidle/cpuidle.c
> > > +++ b/drivers/cpuidle/cpuidle.c
> > > @@ -359,16 +359,6 @@ noinstr int cpuidle_enter_state(struct c
> > >  int cpuidle_select(struct cpuidle_driver *drv, struct cpuidle_device *dev,
> > >                  bool *stop_tick)
> > >  {
> > > -     /*
> > > -      * If there is only a single idle state (or none), there is nothing
> > > -      * meaningful for the governor to choose. Skip the governor and
> > > -      * always use state 0 with the tick running.
> > > -      */
> > > -     if (drv->state_count <= 1) {
> > > -             *stop_tick = false;
> > > -             return 0;
> > > -     }
> > > -
> > >       return cpuidle_curr_governor->select(drv, dev, stop_tick);
> > >  }
> > >
> > > --- a/kernel/sched/idle.c
> > > +++ b/kernel/sched/idle.c
> > > @@ -221,7 +221,7 @@ static void cpuidle_idle_call(void)
> > >
> > >               next_state = cpuidle_find_deepest_state(drv, dev, max_latency_ns);
> > >               call_cpuidle(drv, dev, next_state);
> > > -     } else {
> > > +     } else if (drv->state_count > 1) {
> > >               bool stop_tick = true;
> > >
> > >               /*
> > > @@ -239,6 +239,15 @@ static void cpuidle_idle_call(void)
> > >                * Give the governor an opportunity to reflect on the outcome
> > >                */
> > >               cpuidle_reflect(dev, entered_state);
> > > +     } else {
> > > +             tick_nohz_idle_retain_tick();
> > > +
> > > +             /*
> > > +              * If there is only a single idle state (or none), there is
> > > +              * nothing meaningful for the governor to choose.  Skip the
> > > +              * governor and always use state 0.
> > > +              */
> > > +             call_cpuidle(drv, dev, 0);
> > >       }
> > >
> > >  exit_idle:
> > >
> > >
> > >
> >
> > Duh, good catch.
> > Reviewed-by: Christian Loehle <christian.loehle@arm.com>
> 
> OK, so any objections or concerns from anyone?
> 
> I'm about to queue this up for the next -rc.

LGTM too