[PATCH 0/2] cpuidle: Avoid returning a disabled C-state from menu governor

[PATCH 0/2] cpuidle: Avoid returning a disabled C-state from menu governor

[Rafael J. Wysocki]

Hi,

The cpuidle menu governor has a problem that it returns 0 when it can't find
a suitable C-state, but on some systems in some situations 0 may be the
CPUIDLE_DRIVER_STATE_START index and it shouldn't be returned if the corresponding
C-state has been disabled, for example.

The following two patches avoid this issue by modifying the menu governor to
return an error code rather than 0 in those situations.

[1/2] Combine cpuidle_enabled() with cpuidle_select(), because [2/2] makes
      the latter return negative error codes sometimes, so keeping them separate
      is not useful any more.

[2/2] Modify the menu governor to return an error code if the extra "poll" state
      is not available.

Thanks!


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

[PATCH 1/2] cpuidle: Combine cpuidle_enabled() with cpuidle_select()

[Rafael J. Wysocki]

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

Since both cpuidle_enabled() and cpuidle_select() are only called by
cpuidle_idle_call(), it is not really useful to keep them separate
and combining them will help to avoid complicating cpuidle_idle_call()
even further if governors are changed to return error codes sometimes.

This code modification shouldn't lead to any functional changes.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpuidle/cpuidle.c |   26 ++++++--------------------
 include/linux/cpuidle.h   |    5 -----
 kernel/sched/idle.c       |   20 +++++++-------------
 3 files changed, 13 insertions(+), 38 deletions(-)

Index: linux-pm/kernel/sched/idle.c
===================================================================
--- linux-pm.orig/kernel/sched/idle.c
+++ linux-pm/kernel/sched/idle.c
@@ -100,19 +100,13 @@ static void cpuidle_idle_call(void)
 	rcu_idle_enter();
 
 	/*
-	 * Check if the cpuidle framework is ready, otherwise fallback
-	 * to the default arch specific idle method
+	 * Ask the cpuidle framework to choose a convenient idle state.
+	 * Fall back to the default arch specific idle method on errors.
 	 */
-	ret = cpuidle_enabled(drv, dev);
-
-	if (!ret) {
-		/*
-		 * Ask the governor to choose an idle state it thinks
-		 * it is convenient to go to. There is *always* a
-		 * convenient idle state
-		 */
-		next_state = cpuidle_select(drv, dev);
+	next_state = cpuidle_select(drv, dev);
 
+	ret = next_state;
+	if (ret >= 0) {
 		/*
 		 * The idle task must be scheduled, it is pointless to
 		 * go to idle, just update no idle residency and get
@@ -139,7 +133,7 @@ static void cpuidle_idle_call(void)
 					CLOCK_EVT_NOTIFY_BROADCAST_ENTER,
 					&dev->cpu);
 
-			if (!ret) {
+			if (ret >= 0) {
 				trace_cpu_idle_rcuidle(next_state, dev->cpu);
 
 				/*
@@ -174,7 +168,7 @@ static void cpuidle_idle_call(void)
 	 * We can't use the cpuidle framework, let's use the default
 	 * idle routine
 	 */
-	if (ret)
+	if (ret < 0)
 		arch_cpu_idle();
 
 	__current_set_polling();
Index: linux-pm/drivers/cpuidle/cpuidle.c
===================================================================
--- linux-pm.orig/drivers/cpuidle/cpuidle.c
+++ linux-pm/drivers/cpuidle/cpuidle.c
@@ -65,26 +65,6 @@ int cpuidle_play_dead(void)
 }
 
 /**
- * cpuidle_enabled - check if the cpuidle framework is ready
- * @dev: cpuidle device for this cpu
- * @drv: cpuidle driver for this cpu
- *
- * Return 0 on success, otherwise:
- * -NODEV : the cpuidle framework is not available
- * -EBUSY : the cpuidle framework is not initialized
- */
-int cpuidle_enabled(struct cpuidle_driver *drv, struct cpuidle_device *dev)
-{
-	if (off || !initialized)
-		return -ENODEV;
-
-	if (!drv || !dev || !dev->enabled)
-		return -EBUSY;
-
-	return 0;
-}
-
-/**
  * cpuidle_enter_state - enter the state and update stats
  * @dev: cpuidle device for this cpu
  * @drv: cpuidle driver for this cpu
@@ -138,6 +118,12 @@ int cpuidle_enter_state(struct cpuidle_d
  */
 int cpuidle_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
 {
+	if (off || !initialized)
+		return -ENODEV;
+
+	if (!drv || !dev || !dev->enabled)
+		return -EBUSY;
+
 	return cpuidle_curr_governor->select(drv, dev);
 }
 
Index: linux-pm/include/linux/cpuidle.h
===================================================================
--- linux-pm.orig/include/linux/cpuidle.h
+++ linux-pm/include/linux/cpuidle.h
@@ -120,8 +120,6 @@ struct cpuidle_driver {
 #ifdef CONFIG_CPU_IDLE
 extern void disable_cpuidle(void);
 
-extern int cpuidle_enabled(struct cpuidle_driver *drv,
-			  struct cpuidle_device *dev);
 extern int cpuidle_select(struct cpuidle_driver *drv,
 			  struct cpuidle_device *dev);
 extern int cpuidle_enter(struct cpuidle_driver *drv,
@@ -149,9 +147,6 @@ extern int cpuidle_play_dead(void);
 extern struct cpuidle_driver *cpuidle_get_cpu_driver(struct cpuidle_device *dev);
 #else
 static inline void disable_cpuidle(void) { }
-static inline int cpuidle_enabled(struct cpuidle_driver *drv,
-				  struct cpuidle_device *dev)
-{return -ENODEV; }
 static inline int cpuidle_select(struct cpuidle_driver *drv,
 				 struct cpuidle_device *dev)
 {return -ENODEV; }

[PATCH 2/2] cpuidle / menu: Return error code if there are no suitable states

[Rafael J. Wysocki]

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

If there is a PM QoS latency limit and all of the sufficiently shallow
C-states are disabled, the cpuidle menu governor returns 0 which on
some systems is CPUIDLE_DRIVER_STATE_START and shouldn't be returned
if that C-state has been disabled.

Fix the issue by modifying the menu governor to return an error code
in such situations.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpuidle/governors/menu.c |    2 +-
 include/linux/cpuidle.h          |    2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

Index: linux-pm/drivers/cpuidle/governors/menu.c
===================================================================
--- linux-pm.orig/drivers/cpuidle/governors/menu.c
+++ linux-pm/drivers/cpuidle/governors/menu.c
@@ -296,7 +296,7 @@ static int menu_select(struct cpuidle_dr
 		data->needs_update = 0;
 	}
 
-	data->last_state_idx = 0;
+	data->last_state_idx = CPUIDLE_DRIVER_STATE_POLL;
 
 	/* Special case when user has set very strict latency requirement */
 	if (unlikely(latency_req == 0))
Index: linux-pm/include/linux/cpuidle.h
===================================================================
--- linux-pm.orig/include/linux/cpuidle.h
+++ linux-pm/include/linux/cpuidle.h
@@ -217,8 +217,10 @@ static inline int cpuidle_register_gover
 
 #ifdef CONFIG_ARCH_HAS_CPU_RELAX
 #define CPUIDLE_DRIVER_STATE_START	1
+#define CPUIDLE_DRIVER_STATE_POLL	0
 #else
 #define CPUIDLE_DRIVER_STATE_START	0
+#define CPUIDLE_DRIVER_STATE_POLL	(-ENXIO)
 #endif
 
 #endif /* _LINUX_CPUIDLE_H */

Re: [PATCH 2/2] cpuidle / menu: Return error code if there are no suitable states

[Daniel Lezcano]

On 04/27/2014 02:55 PM, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> If there is a PM QoS latency limit and all of the sufficiently shallow
> C-states are disabled, the cpuidle menu governor returns 0 which on
> some systems is CPUIDLE_DRIVER_STATE_START and shouldn't be returned
> if that C-state has been disabled.
>
> Fix the issue by modifying the menu governor to return an error code
> in such situations.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>   drivers/cpuidle/governors/menu.c |    2 +-
>   include/linux/cpuidle.h          |    2 ++
>   2 files changed, 3 insertions(+), 1 deletion(-)
>
> Index: linux-pm/drivers/cpuidle/governors/menu.c
> ===================================================================
> --- linux-pm.orig/drivers/cpuidle/governors/menu.c
> +++ linux-pm/drivers/cpuidle/governors/menu.c
> @@ -296,7 +296,7 @@ static int menu_select(struct cpuidle_dr
>   		data->needs_update = 0;
>   	}
>
> -	data->last_state_idx = 0;
> +	data->last_state_idx = CPUIDLE_DRIVER_STATE_POLL;
>
>   	/* Special case when user has set very strict latency requirement */
>   	if (unlikely(latency_req == 0))
> Index: linux-pm/include/linux/cpuidle.h
> ===================================================================
> --- linux-pm.orig/include/linux/cpuidle.h
> +++ linux-pm/include/linux/cpuidle.h
> @@ -217,8 +217,10 @@ static inline int cpuidle_register_gover
>
>   #ifdef CONFIG_ARCH_HAS_CPU_RELAX
>   #define CPUIDLE_DRIVER_STATE_START	1
> +#define CPUIDLE_DRIVER_STATE_POLL	0
>   #else
>   #define CPUIDLE_DRIVER_STATE_START	0
> +#define CPUIDLE_DRIVER_STATE_POLL	(-ENXIO)
>   #endif
>
>   #endif /* _LINUX_CPUIDLE_H */

Hi Rafael,

CPUIDLE_DRIVER_STATE_START is only for x86. It introduces some confusion 
in the code. As only two drivers are concerned by it, wouldn't make 
sense to add the poll state to those driver directly instead of having 
the code hacked around ? (eg. insert the poll state in the common 
cpuidle code).


-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 2/2] cpuidle / menu: Return error code if there are no suitable states

[Rafael J. Wysocki]

On Monday, April 28, 2014 01:14:32 PM Daniel Lezcano wrote:
> On 04/27/2014 02:55 PM, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > If there is a PM QoS latency limit and all of the sufficiently shallow
> > C-states are disabled, the cpuidle menu governor returns 0 which on
> > some systems is CPUIDLE_DRIVER_STATE_START and shouldn't be returned
> > if that C-state has been disabled.
> >
> > Fix the issue by modifying the menu governor to return an error code
> > in such situations.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >   drivers/cpuidle/governors/menu.c |    2 +-
> >   include/linux/cpuidle.h          |    2 ++
> >   2 files changed, 3 insertions(+), 1 deletion(-)
> >
> > Index: linux-pm/drivers/cpuidle/governors/menu.c
> > ===================================================================
> > --- linux-pm.orig/drivers/cpuidle/governors/menu.c
> > +++ linux-pm/drivers/cpuidle/governors/menu.c
> > @@ -296,7 +296,7 @@ static int menu_select(struct cpuidle_dr
> >   		data->needs_update = 0;
> >   	}
> >
> > -	data->last_state_idx = 0;
> > +	data->last_state_idx = CPUIDLE_DRIVER_STATE_POLL;
> >
> >   	/* Special case when user has set very strict latency requirement */
> >   	if (unlikely(latency_req == 0))
> > Index: linux-pm/include/linux/cpuidle.h
> > ===================================================================
> > --- linux-pm.orig/include/linux/cpuidle.h
> > +++ linux-pm/include/linux/cpuidle.h
> > @@ -217,8 +217,10 @@ static inline int cpuidle_register_gover
> >
> >   #ifdef CONFIG_ARCH_HAS_CPU_RELAX
> >   #define CPUIDLE_DRIVER_STATE_START	1
> > +#define CPUIDLE_DRIVER_STATE_POLL	0
> >   #else
> >   #define CPUIDLE_DRIVER_STATE_START	0
> > +#define CPUIDLE_DRIVER_STATE_POLL	(-ENXIO)
> >   #endif
> >
> >   #endif /* _LINUX_CPUIDLE_H */
> 
> Hi Rafael,
> 
> CPUIDLE_DRIVER_STATE_START is only for x86. It introduces some confusion 
> in the code.

I won't disagree with that.

> As only two drivers are concerned by it, wouldn't make 
> sense to add the poll state to those driver directly instead of having 
> the code hacked around ? (eg. insert the poll state in the common 
> cpuidle code).

Well, what about initializing data->last_state_idx to
(CPUIDLE_DRIVER_STATE_START - 1) in menu_select() instead of introducing the
new symbol for the time being and getting rid of CPUIDLE_DRIVER_STATE_START
separately?


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 2/2] cpuidle / menu: Return error code if there are no suitable states

[Rafael J. Wysocki]

On Tuesday, April 29, 2014 01:28:03 AM Rafael J. Wysocki wrote:
> On Monday, April 28, 2014 01:14:32 PM Daniel Lezcano wrote:
> > On 04/27/2014 02:55 PM, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > >
> > > If there is a PM QoS latency limit and all of the sufficiently shallow
> > > C-states are disabled, the cpuidle menu governor returns 0 which on
> > > some systems is CPUIDLE_DRIVER_STATE_START and shouldn't be returned
> > > if that C-state has been disabled.
> > >
> > > Fix the issue by modifying the menu governor to return an error code
> > > in such situations.
> > >
> > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > ---
> > >   drivers/cpuidle/governors/menu.c |    2 +-
> > >   include/linux/cpuidle.h          |    2 ++
> > >   2 files changed, 3 insertions(+), 1 deletion(-)
> > >
> > > Index: linux-pm/drivers/cpuidle/governors/menu.c
> > > ===================================================================
> > > --- linux-pm.orig/drivers/cpuidle/governors/menu.c
> > > +++ linux-pm/drivers/cpuidle/governors/menu.c
> > > @@ -296,7 +296,7 @@ static int menu_select(struct cpuidle_dr
> > >   		data->needs_update = 0;
> > >   	}
> > >
> > > -	data->last_state_idx = 0;
> > > +	data->last_state_idx = CPUIDLE_DRIVER_STATE_POLL;
> > >
> > >   	/* Special case when user has set very strict latency requirement */
> > >   	if (unlikely(latency_req == 0))
> > > Index: linux-pm/include/linux/cpuidle.h
> > > ===================================================================
> > > --- linux-pm.orig/include/linux/cpuidle.h
> > > +++ linux-pm/include/linux/cpuidle.h
> > > @@ -217,8 +217,10 @@ static inline int cpuidle_register_gover
> > >
> > >   #ifdef CONFIG_ARCH_HAS_CPU_RELAX
> > >   #define CPUIDLE_DRIVER_STATE_START	1
> > > +#define CPUIDLE_DRIVER_STATE_POLL	0
> > >   #else
> > >   #define CPUIDLE_DRIVER_STATE_START	0
> > > +#define CPUIDLE_DRIVER_STATE_POLL	(-ENXIO)
> > >   #endif
> > >
> > >   #endif /* _LINUX_CPUIDLE_H */
> > 
> > Hi Rafael,
> > 
> > CPUIDLE_DRIVER_STATE_START is only for x86. It introduces some confusion 
> > in the code.
> 
> I won't disagree with that.
> 
> > As only two drivers are concerned by it, wouldn't make 
> > sense to add the poll state to those driver directly instead of having 
> > the code hacked around ? (eg. insert the poll state in the common 
> > cpuidle code).
> 
> Well, what about initializing data->last_state_idx to
> (CPUIDLE_DRIVER_STATE_START - 1) in menu_select() instead of introducing the
> new symbol for the time being and getting rid of CPUIDLE_DRIVER_STATE_START
> separately?

Updated patch is appended for completness.

Thanks!

---
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Subject: cpuidle / menu: Return (-1) if there are no suitable states

If there is a PM QoS latency limit and all of the sufficiently shallow
C-states are disabled, the cpuidle menu governor returns 0 which on
some systems is CPUIDLE_DRIVER_STATE_START and shouldn't be returned
if that C-state has been disabled.

Fix the issue by modifying the menu governor to return (-1) in such
situations.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpuidle/governors/menu.c |    2 +-
 include/linux/cpuidle.h          |    2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

Index: linux-pm/drivers/cpuidle/governors/menu.c
===================================================================
--- linux-pm.orig/drivers/cpuidle/governors/menu.c
+++ linux-pm/drivers/cpuidle/governors/menu.c
@@ -296,7 +296,7 @@ static int menu_select(struct cpuidle_dr
 		data->needs_update = 0;
 	}
 
-	data->last_state_idx = 0;
+	data->last_state_idx = CPUIDLE_DRIVER_STATE_START - 1;
 
 	/* Special case when user has set very strict latency requirement */
 	if (unlikely(latency_req == 0))

Re: [PATCH 2/2] cpuidle / menu: Return error code if there are no suitable states

[Daniel Lezcano]

On 04/29/2014 01:28 AM, Rafael J. Wysocki wrote:
> On Monday, April 28, 2014 01:14:32 PM Daniel Lezcano wrote:
>> On 04/27/2014 02:55 PM, Rafael J. Wysocki wrote:
>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>
>>> If there is a PM QoS latency limit and all of the sufficiently shallow
>>> C-states are disabled, the cpuidle menu governor returns 0 which on
>>> some systems is CPUIDLE_DRIVER_STATE_START and shouldn't be returned
>>> if that C-state has been disabled.
>>>
>>> Fix the issue by modifying the menu governor to return an error code
>>> in such situations.
>>>
>>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>> ---
>>>    drivers/cpuidle/governors/menu.c |    2 +-
>>>    include/linux/cpuidle.h          |    2 ++
>>>    2 files changed, 3 insertions(+), 1 deletion(-)
>>>
>>> Index: linux-pm/drivers/cpuidle/governors/menu.c
>>> ===================================================================
>>> --- linux-pm.orig/drivers/cpuidle/governors/menu.c
>>> +++ linux-pm/drivers/cpuidle/governors/menu.c
>>> @@ -296,7 +296,7 @@ static int menu_select(struct cpuidle_dr
>>>    		data->needs_update = 0;
>>>    	}
>>>
>>> -	data->last_state_idx = 0;
>>> +	data->last_state_idx = CPUIDLE_DRIVER_STATE_POLL;
>>>
>>>    	/* Special case when user has set very strict latency requirement */
>>>    	if (unlikely(latency_req == 0))
>>> Index: linux-pm/include/linux/cpuidle.h
>>> ===================================================================
>>> --- linux-pm.orig/include/linux/cpuidle.h
>>> +++ linux-pm/include/linux/cpuidle.h
>>> @@ -217,8 +217,10 @@ static inline int cpuidle_register_gover
>>>
>>>    #ifdef CONFIG_ARCH_HAS_CPU_RELAX
>>>    #define CPUIDLE_DRIVER_STATE_START	1
>>> +#define CPUIDLE_DRIVER_STATE_POLL	0
>>>    #else
>>>    #define CPUIDLE_DRIVER_STATE_START	0
>>> +#define CPUIDLE_DRIVER_STATE_POLL	(-ENXIO)
>>>    #endif
>>>
>>>    #endif /* _LINUX_CPUIDLE_H */
>>
>> Hi Rafael,
>>
>> CPUIDLE_DRIVER_STATE_START is only for x86. It introduces some confusion
>> in the code.
>
> I won't disagree with that.
>
>> As only two drivers are concerned by it, wouldn't make
>> sense to add the poll state to those driver directly instead of having
>> the code hacked around ? (eg. insert the poll state in the common
>> cpuidle code).
>
> Well, what about initializing data->last_state_idx to
> (CPUIDLE_DRIVER_STATE_START - 1) in menu_select() instead of introducing the
> new symbol for the time being and getting rid of CPUIDLE_DRIVER_STATE_START
> separately?

+1


-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 2/2] cpuidle / menu: Return error code if there are no suitable states

[Daniel Lezcano]

On 04/30/2014 01:16 AM, Rafael J. Wysocki wrote:
> On Tuesday, April 29, 2014 01:28:03 AM Rafael J. Wysocki wrote:
>> On Monday, April 28, 2014 01:14:32 PM Daniel Lezcano wrote:
>>> On 04/27/2014 02:55 PM, Rafael J. Wysocki wrote:

[ ... ]

> ---
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Subject: cpuidle / menu: Return (-1) if there are no suitable states
>
> If there is a PM QoS latency limit and all of the sufficiently shallow
> C-states are disabled, the cpuidle menu governor returns 0 which on
> some systems is CPUIDLE_DRIVER_STATE_START and shouldn't be returned
> if that C-state has been disabled.
>
> Fix the issue by modifying the menu governor to return (-1) in such
> situations.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>   drivers/cpuidle/governors/menu.c |    2 +-
>   include/linux/cpuidle.h          |    2 ++
>   2 files changed, 3 insertions(+), 1 deletion(-)
>
> Index: linux-pm/drivers/cpuidle/governors/menu.c
> ===================================================================
> --- linux-pm.orig/drivers/cpuidle/governors/menu.c
> +++ linux-pm/drivers/cpuidle/governors/menu.c
> @@ -296,7 +296,7 @@ static int menu_select(struct cpuidle_dr
>   		data->needs_update = 0;
>   	}
>
> -	data->last_state_idx = 0;
> +	data->last_state_idx = CPUIDLE_DRIVER_STATE_START - 1;

In case of x86, CPUIDLE_DRIVER_STATE_START will be 1, so the select 
function could return 0 even this one is disabled and this is not what 
you want to happen, no ?


>   	/* Special case when user has set very strict latency requirement */
>   	if (unlikely(latency_req == 0))
>


-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 2/2] cpuidle / menu: Return error code if there are no suitable states

[Rafael J. Wysocki]

On Friday, May 02, 2014 10:47:48 AM Daniel Lezcano wrote:
> On 04/30/2014 01:16 AM, Rafael J. Wysocki wrote:
> > On Tuesday, April 29, 2014 01:28:03 AM Rafael J. Wysocki wrote:
> >> On Monday, April 28, 2014 01:14:32 PM Daniel Lezcano wrote:
> >>> On 04/27/2014 02:55 PM, Rafael J. Wysocki wrote:
> 
> [ ... ]
> 
> > ---
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > Subject: cpuidle / menu: Return (-1) if there are no suitable states
> >
> > If there is a PM QoS latency limit and all of the sufficiently shallow
> > C-states are disabled, the cpuidle menu governor returns 0 which on
> > some systems is CPUIDLE_DRIVER_STATE_START and shouldn't be returned
> > if that C-state has been disabled.
> >
> > Fix the issue by modifying the menu governor to return (-1) in such
> > situations.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >   drivers/cpuidle/governors/menu.c |    2 +-
> >   include/linux/cpuidle.h          |    2 ++
> >   2 files changed, 3 insertions(+), 1 deletion(-)
> >
> > Index: linux-pm/drivers/cpuidle/governors/menu.c
> > ===================================================================
> > --- linux-pm.orig/drivers/cpuidle/governors/menu.c
> > +++ linux-pm/drivers/cpuidle/governors/menu.c
> > @@ -296,7 +296,7 @@ static int menu_select(struct cpuidle_dr
> >   		data->needs_update = 0;
> >   	}
> >
> > -	data->last_state_idx = 0;
> > +	data->last_state_idx = CPUIDLE_DRIVER_STATE_START - 1;
> 
> In case of x86, CPUIDLE_DRIVER_STATE_START will be 1, so the select 
> function could return 0 even this one is disabled and this is not what 
> you want to happen, no ?

OK, so that's a choice.  We can choose to do the above or to return an error
code if the 0 state is disabled too.  The above is arguably simpler and
matches the idea that 0 is a "fallback" state on x86.

Of course, it also is confusing, because user space *can* set "disable" for
the 0 state on x86, but that actually has no effect today AFAICS.

I'm mostly worried about systems where CPUIDLE_DRIVER_STATE_START is 0
and where menu_select() explicitly checks "disabled" and then it returns
0 anyway if it cannot find any other suitable state.

In my opinion that needs to be made consistent, but I don't care too much about
which way as long as the change is not too intrusive.


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 2/2] cpuidle / menu: Return error code if there are no suitable states

[Daniel Lezcano]

On 05/02/2014 02:20 PM, Rafael J. Wysocki wrote:
> On Friday, May 02, 2014 10:47:48 AM Daniel Lezcano wrote:
>> On 04/30/2014 01:16 AM, Rafael J. Wysocki wrote:
>>> On Tuesday, April 29, 2014 01:28:03 AM Rafael J. Wysocki wrote:
>>>> On Monday, April 28, 2014 01:14:32 PM Daniel Lezcano wrote:
>>>>> On 04/27/2014 02:55 PM, Rafael J. Wysocki wrote:
>>
>> [ ... ]
>>
>>> ---
>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>> Subject: cpuidle / menu: Return (-1) if there are no suitable states
>>>
>>> If there is a PM QoS latency limit and all of the sufficiently shallow
>>> C-states are disabled, the cpuidle menu governor returns 0 which on
>>> some systems is CPUIDLE_DRIVER_STATE_START and shouldn't be returned
>>> if that C-state has been disabled.
>>>
>>> Fix the issue by modifying the menu governor to return (-1) in such
>>> situations.
>>>
>>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>> ---
>>>    drivers/cpuidle/governors/menu.c |    2 +-
>>>    include/linux/cpuidle.h          |    2 ++
>>>    2 files changed, 3 insertions(+), 1 deletion(-)
>>>
>>> Index: linux-pm/drivers/cpuidle/governors/menu.c
>>> ===================================================================
>>> --- linux-pm.orig/drivers/cpuidle/governors/menu.c
>>> +++ linux-pm/drivers/cpuidle/governors/menu.c
>>> @@ -296,7 +296,7 @@ static int menu_select(struct cpuidle_dr
>>>    		data->needs_update = 0;
>>>    	}
>>>
>>> -	data->last_state_idx = 0;
>>> +	data->last_state_idx = CPUIDLE_DRIVER_STATE_START - 1;
>>
>> In case of x86, CPUIDLE_DRIVER_STATE_START will be 1, so the select
>> function could return 0 even this one is disabled and this is not what
>> you want to happen, no ?
>
> OK, so that's a choice.  We can choose to do the above or to return an error
> code if the 0 state is disabled too.  The above is arguably simpler and
> matches the idea that 0 is a "fallback" state on x86.
>
> Of course, it also is confusing, because user space *can* set "disable" for
> the 0 state on x86, but that actually has no effect today AFAICS.

Yes, the poll state is very rarely selected.

Regarding the description of this patch, I think it would make sense to 
move the duplicate pm qos checks to the cpuidle_idle_call function 
directly and pass the latency req to the select function, so the zero 
latency check could be done by the caller before entering select.

> I'm mostly worried about systems where CPUIDLE_DRIVER_STATE_START is 0
> and where menu_select() explicitly checks "disabled" and then it returns
> 0 anyway if it cannot find any other suitable state.

For the ARM platform, the state0 and the default idle function are the 
same, so disabling this state will result in calling the same idle function.

> In my opinion that needs to be made consistent, but I don't care too much about
> which way as long as the change is not too intrusive.

I think we can live with this patch until we remove the 
CPUIDLE_DRIVER_STATE_START macro. It was introduced to factor out a 
couple of drivers and now it results in a confusing-hard-to-fix-code.


-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 2/2] cpuidle / menu: Return error code if there are no suitable states

[Rafael J. Wysocki]

On Friday, May 02, 2014 03:19:55 PM Daniel Lezcano wrote:
> On 05/02/2014 02:20 PM, Rafael J. Wysocki wrote:
> > On Friday, May 02, 2014 10:47:48 AM Daniel Lezcano wrote:
> >> On 04/30/2014 01:16 AM, Rafael J. Wysocki wrote:
> >>> On Tuesday, April 29, 2014 01:28:03 AM Rafael J. Wysocki wrote:
> >>>> On Monday, April 28, 2014 01:14:32 PM Daniel Lezcano wrote:
> >>>>> On 04/27/2014 02:55 PM, Rafael J. Wysocki wrote:
> >>
> >> [ ... ]
> >>
> >>> ---
> >>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >>> Subject: cpuidle / menu: Return (-1) if there are no suitable states
> >>>
> >>> If there is a PM QoS latency limit and all of the sufficiently shallow
> >>> C-states are disabled, the cpuidle menu governor returns 0 which on
> >>> some systems is CPUIDLE_DRIVER_STATE_START and shouldn't be returned
> >>> if that C-state has been disabled.
> >>>
> >>> Fix the issue by modifying the menu governor to return (-1) in such
> >>> situations.
> >>>
> >>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >>> ---
> >>>    drivers/cpuidle/governors/menu.c |    2 +-
> >>>    include/linux/cpuidle.h          |    2 ++
> >>>    2 files changed, 3 insertions(+), 1 deletion(-)
> >>>
> >>> Index: linux-pm/drivers/cpuidle/governors/menu.c
> >>> ===================================================================
> >>> --- linux-pm.orig/drivers/cpuidle/governors/menu.c
> >>> +++ linux-pm/drivers/cpuidle/governors/menu.c
> >>> @@ -296,7 +296,7 @@ static int menu_select(struct cpuidle_dr
> >>>    		data->needs_update = 0;
> >>>    	}
> >>>
> >>> -	data->last_state_idx = 0;
> >>> +	data->last_state_idx = CPUIDLE_DRIVER_STATE_START - 1;
> >>
> >> In case of x86, CPUIDLE_DRIVER_STATE_START will be 1, so the select
> >> function could return 0 even this one is disabled and this is not what
> >> you want to happen, no ?
> >
> > OK, so that's a choice.  We can choose to do the above or to return an error
> > code if the 0 state is disabled too.  The above is arguably simpler and
> > matches the idea that 0 is a "fallback" state on x86.
> >
> > Of course, it also is confusing, because user space *can* set "disable" for
> > the 0 state on x86, but that actually has no effect today AFAICS.
> 
> Yes, the poll state is very rarely selected.
> 
> Regarding the description of this patch, I think it would make sense to 
> move the duplicate pm qos checks to the cpuidle_idle_call function 
> directly and pass the latency req to the select function, so the zero 
> latency check could be done by the caller before entering select.

I would prefer to have them in cpuidle_select() for various reasons (one
of them being to avoid the need to pass latency_req from cpuidle_idle_call()
to cpuidle_select() which isn't necessary).

> > I'm mostly worried about systems where CPUIDLE_DRIVER_STATE_START is 0
> > and where menu_select() explicitly checks "disabled" and then it returns
> > 0 anyway if it cannot find any other suitable state.
> 
> For the ARM platform, the state0 and the default idle function are the 
> same, so disabling this state will result in calling the same idle function.
> 
> > In my opinion that needs to be made consistent, but I don't care too much about
> > which way as long as the change is not too intrusive.
> 
> I think we can live with this patch until we remove the 
> CPUIDLE_DRIVER_STATE_START macro. It was introduced to factor out a 
> couple of drivers and now it results in a confusing-hard-to-fix-code.

OK

Thanks!

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.