[PATCH] cpuidle: Do not use CPUIDLE_DRIVER_STATE_START in cpuidle.c

[PATCH] cpuidle: Do not use CPUIDLE_DRIVER_STATE_START in cpuidle.c

[Rafael J. Wysocki]

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

The CPUIDLE_DRIVER_STATE_START symbol is defined as 1 only if
CONFIG_ARCH_HAS_CPU_RELAX is set, otherwise it is defined as 0.
However, if CONFIG_ARCH_HAS_CPU_RELAX is set, the first (index 0)
entry in the cpuidle driver's table of states is overwritten with
the default "poll" entry by the core.  The "state" defined by the
"poll" entry doesn't provide ->enter_dead and ->enter_freeze
callbacks and its exit_latency is 0.

For this reason, it is not necessary to use CPUIDLE_DRIVER_STATE_START
in cpuidle_play_dead() (->enter_dead is NULL, so the "poll state"
will be skipped by the loop) and in find_deepest_state() (since
exit_latency is 0, the "poll state" will become the default if the
"s->exit_latency <= latency_req" check is replaced with
"s->exit_latency < latency_req" which may only matter for drivers
providing different states with the same exit_latency).

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpuidle/cpuidle.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Index: linux-pm/drivers/cpuidle/cpuidle.c
===================================================================
--- linux-pm.orig/drivers/cpuidle/cpuidle.c
+++ linux-pm/drivers/cpuidle/cpuidle.c
@@ -65,7 +65,7 @@ int cpuidle_play_dead(void)
 		return -ENODEV;
 
 	/* Find lowest-power state that supports long-term idle */
-	for (i = drv->state_count - 1; i >= CPUIDLE_DRIVER_STATE_START; i--)
+	for (i = drv->state_count - 1; i >= 0; i--)
 		if (drv->states[i].enter_dead)
 			return drv->states[i].enter_dead(dev, i);
 
@@ -79,13 +79,13 @@ static int find_deepest_state(struct cpu
 			      bool freeze)
 {
 	unsigned int latency_req = 0;
-	int i, ret = freeze ? -1 : CPUIDLE_DRIVER_STATE_START - 1;
+	int i, ret = -ENXIO;
 
-	for (i = CPUIDLE_DRIVER_STATE_START; i < drv->state_count; i++) {
+	for (i = 0; i < drv->state_count; i++) {
 		struct cpuidle_state *s = &drv->states[i];
 		struct cpuidle_state_usage *su = &dev->states_usage[i];
 
-		if (s->disabled || su->disable || s->exit_latency <= latency_req
+		if (s->disabled || su->disable || s->exit_latency < latency_req
 		    || s->exit_latency > max_latency
 		    || (s->flags & forbidden_flags)
 		    || (freeze && !cpuidle_has_enter_freeze(s)))

Re: [PATCH] cpuidle: Do not use CPUIDLE_DRIVER_STATE_START in cpuidle.c

[Daniel Lezcano]

On 05/27/2015 03:36 AM, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> The CPUIDLE_DRIVER_STATE_START symbol is defined as 1 only if
> CONFIG_ARCH_HAS_CPU_RELAX is set, otherwise it is defined as 0.
> However, if CONFIG_ARCH_HAS_CPU_RELAX is set, the first (index 0)
> entry in the cpuidle driver's table of states is overwritten with
> the default "poll" entry by the core.  The "state" defined by the
> "poll" entry doesn't provide ->enter_dead and ->enter_freeze
> callbacks and its exit_latency is 0.
>
> For this reason, it is not necessary to use CPUIDLE_DRIVER_STATE_START
> in cpuidle_play_dead() (->enter_dead is NULL, so the "poll state"
> will be skipped by the loop) and in find_deepest_state() (since
> exit_latency is 0, the "poll state" will become the default if the
> "s->exit_latency <= latency_req" check is replaced with
> "s->exit_latency < latency_req" which may only matter for drivers
> providing different states with the same exit_latency).
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

I was about to send *exactly* the same patch :)

Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>


-- 
  <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] cpuidle: Do not use CPUIDLE_DRIVER_STATE_START in cpuidle.c

[Preeti U Murthy]

On 05/27/2015 07:06 AM, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> The CPUIDLE_DRIVER_STATE_START symbol is defined as 1 only if
> CONFIG_ARCH_HAS_CPU_RELAX is set, otherwise it is defined as 0.
> However, if CONFIG_ARCH_HAS_CPU_RELAX is set, the first (index 0)
> entry in the cpuidle driver's table of states is overwritten with
> the default "poll" entry by the core.  The "state" defined by the
> "poll" entry doesn't provide ->enter_dead and ->enter_freeze
> callbacks and its exit_latency is 0.
> 
> For this reason, it is not necessary to use CPUIDLE_DRIVER_STATE_START
> in cpuidle_play_dead() (->enter_dead is NULL, so the "poll state"
> will be skipped by the loop) and in find_deepest_state() (since
> exit_latency is 0, the "poll state" will become the default if the
> "s->exit_latency <= latency_req" check is replaced with
> "s->exit_latency < latency_req" which may only matter for drivers
> providing different states with the same exit_latency).
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/cpuidle/cpuidle.c |    8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
<snip>

> 
> @@ -79,13 +79,13 @@ static int find_deepest_state(struct cpu
>  			      bool freeze)
>  {
>  	unsigned int latency_req = 0;
> -	int i, ret = freeze ? -1 : CPUIDLE_DRIVER_STATE_START - 1;
> +	int i, ret = -ENXIO;
> 
> -	for (i = CPUIDLE_DRIVER_STATE_START; i < drv->state_count; i++) {
> +	for (i = 0; i < drv->state_count; i++) {
>  		struct cpuidle_state *s = &drv->states[i];
>  		struct cpuidle_state_usage *su = &dev->states_usage[i];
> 
> -		if (s->disabled || su->disable || s->exit_latency <= latency_req
> +		if (s->disabled || su->disable || s->exit_latency < latency_req

Prior to this patch,

For drivers on which CPUIDLE_DRIVER_STATE_START takes a value 0 and
whose first idle state has an exit_latency of 0, find_deepest_state()
would return -1 if it failed to find a deeper idle state.

But as an effect of this patch, find_deepest_state() returns 0 in the
above circumstance.

My concern is if these drivers do not intend to enter a polling state
during suspend, this will cause an issue, won't it? This also gets me
wondering if polling state is an acceptable idle state during suspend,
given that the drivers with ARCH_HAS_CPU_RELAX permit entry into it
during suspend today. I would expect the cpus to be in a hardware
defined idle state.

Regards
Preeti U Murthy

Re: [PATCH] cpuidle: Do not use CPUIDLE_DRIVER_STATE_START in cpuidle.c

[Daniel Lezcano]

On 05/27/2015 01:31 PM, Preeti U Murthy wrote:
> On 05/27/2015 07:06 AM, Rafael J. Wysocki wrote:
>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>
>> The CPUIDLE_DRIVER_STATE_START symbol is defined as 1 only if
>> CONFIG_ARCH_HAS_CPU_RELAX is set, otherwise it is defined as 0.
>> However, if CONFIG_ARCH_HAS_CPU_RELAX is set, the first (index 0)
>> entry in the cpuidle driver's table of states is overwritten with
>> the default "poll" entry by the core.  The "state" defined by the
>> "poll" entry doesn't provide ->enter_dead and ->enter_freeze
>> callbacks and its exit_latency is 0.
>>
>> For this reason, it is not necessary to use CPUIDLE_DRIVER_STATE_START
>> in cpuidle_play_dead() (->enter_dead is NULL, so the "poll state"
>> will be skipped by the loop) and in find_deepest_state() (since
>> exit_latency is 0, the "poll state" will become the default if the
>> "s->exit_latency <= latency_req" check is replaced with
>> "s->exit_latency < latency_req" which may only matter for drivers
>> providing different states with the same exit_latency).
>>
>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> ---
>>   drivers/cpuidle/cpuidle.c |    8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
> <snip>
>
>>
>> @@ -79,13 +79,13 @@ static int find_deepest_state(struct cpu
>>   			      bool freeze)
>>   {
>>   	unsigned int latency_req = 0;
>> -	int i, ret = freeze ? -1 : CPUIDLE_DRIVER_STATE_START - 1;
>> +	int i, ret = -ENXIO;
>>
>> -	for (i = CPUIDLE_DRIVER_STATE_START; i < drv->state_count; i++) {
>> +	for (i = 0; i < drv->state_count; i++) {
>>   		struct cpuidle_state *s = &drv->states[i];
>>   		struct cpuidle_state_usage *su = &dev->states_usage[i];
>>
>> -		if (s->disabled || su->disable || s->exit_latency <= latency_req
>> +		if (s->disabled || su->disable || s->exit_latency < latency_req
>
> Prior to this patch,
>
> For drivers on which CPUIDLE_DRIVER_STATE_START takes a value 0 and
> whose first idle state has an exit_latency of 0, find_deepest_state()
> would return -1 if it failed to find a deeper idle state.
> But as an effect of this patch, find_deepest_state() returns 0 in the
> above circumstance.

Except I am missing something, with an exit_latency = 0, the state will 
be never selected, because of the "s->exit_latency < latency_req" 
condition (strictly greater than).

> My concern is if these drivers do not intend to enter a polling state
> during suspend, this will cause an issue, won't it? This also gets me
> wondering if polling state is an acceptable idle state during suspend,
> given that the drivers with ARCH_HAS_CPU_RELAX permit entry into it
> during suspend today.

Definitively poll can cause thermal issues, especially when suspending. 
It is a dangerous state (let's imagine you close your laptop => 
suspend/poll and then put it in your bag for a travel).

I don't think with the code above we can reach this situation but I 
agree this is something we have to take care carefully.

Actually, I am in favour of removing poll at all from the cpuidle driver 
and poll only when a cpuidle state selection fails under certain condition.

So I fully agree with your statement below.

> I would expect the cpus to be in a hardware
> defined idle state.




-- 
  <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] cpuidle: Do not use CPUIDLE_DRIVER_STATE_START in cpuidle.c

[Rafael J. Wysocki]

On Wed, May 27, 2015 at 2:25 PM, Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
> On 05/27/2015 01:31 PM, Preeti U Murthy wrote:
>>
>> On 05/27/2015 07:06 AM, Rafael J. Wysocki wrote:
>>>
>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>
>>> The CPUIDLE_DRIVER_STATE_START symbol is defined as 1 only if
>>> CONFIG_ARCH_HAS_CPU_RELAX is set, otherwise it is defined as 0.
>>> However, if CONFIG_ARCH_HAS_CPU_RELAX is set, the first (index 0)
>>> entry in the cpuidle driver's table of states is overwritten with
>>> the default "poll" entry by the core.  The "state" defined by the
>>> "poll" entry doesn't provide ->enter_dead and ->enter_freeze
>>> callbacks and its exit_latency is 0.
>>>
>>> For this reason, it is not necessary to use CPUIDLE_DRIVER_STATE_START
>>> in cpuidle_play_dead() (->enter_dead is NULL, so the "poll state"
>>> will be skipped by the loop) and in find_deepest_state() (since
>>> exit_latency is 0, the "poll state" will become the default if the
>>> "s->exit_latency <= latency_req" check is replaced with
>>> "s->exit_latency < latency_req" which may only matter for drivers
>>> providing different states with the same exit_latency).
>>>
>>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>> ---
>>>   drivers/cpuidle/cpuidle.c |    8 ++++----
>>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> <snip>
>>
>>>
>>> @@ -79,13 +79,13 @@ static int find_deepest_state(struct cpu
>>>                               bool freeze)
>>>   {
>>>         unsigned int latency_req = 0;
>>> -       int i, ret = freeze ? -1 : CPUIDLE_DRIVER_STATE_START - 1;
>>> +       int i, ret = -ENXIO;
>>>
>>> -       for (i = CPUIDLE_DRIVER_STATE_START; i < drv->state_count; i++) {
>>> +       for (i = 0; i < drv->state_count; i++) {
>>>                 struct cpuidle_state *s = &drv->states[i];
>>>                 struct cpuidle_state_usage *su = &dev->states_usage[i];
>>>
>>> -               if (s->disabled || su->disable || s->exit_latency <=
>>> latency_req
>>> +               if (s->disabled || su->disable || s->exit_latency <
>>> latency_req
>>
>>
>> Prior to this patch,
>>
>> For drivers on which CPUIDLE_DRIVER_STATE_START takes a value 0 and
>> whose first idle state has an exit_latency of 0, find_deepest_state()
>> would return -1 if it failed to find a deeper idle state.
>> But as an effect of this patch, find_deepest_state() returns 0 in the
>> above circumstance.
>
>
> Except I am missing something, with an exit_latency = 0, the state will be
> never selected, because of the "s->exit_latency < latency_req" condition
> (strictly greater than).

No, this is the condition to skip the state, so previously it wouldn't
be selected, but after the patch it will.

So yes, the patch changes behavior for systems with
CONFIG_ARCH_HAS_CPU_RELAX unset.

>> My concern is if these drivers do not intend to enter a polling state
>> during suspend, this will cause an issue, won't it?

The change in behavior happens for architectures where
CONFIG_ARCH_HAS_CPU_RELAX is not set.  In those cases the 0-index
state is supposed to be provided by the driver.  Is there a reason to
expect that this may not be a genuine idle state?

>> This also gets me
>> wondering if polling state is an acceptable idle state during suspend,
>> given that the drivers with ARCH_HAS_CPU_RELAX permit entry into it
>> during suspend today.

That actually is a good question.

> Definitively poll can cause thermal issues, especially when suspending. It
> is a dangerous state (let's imagine you close your laptop => suspend/poll
> and then put it in your bag for a travel).

With ARCH_HAS_CPU_RELAX set the "poll state" is supposed to be thermally safe.

> I don't think with the code above we can reach this situation but I agree
> this is something we have to take care carefully.
>
> Actually, I am in favour of removing poll at all from the cpuidle driver and
> poll only when a cpuidle state selection fails under certain condition.
>
> So I fully agree with your statement below.
>
>> I would expect the cpus to be in a hardware
>> defined idle state.

Well, except for the degenerate case in which all of them are disabled
and for the "broadcast timer stopping aviodance" use case for
find_deepest_state().

So there are two questions in my view:
(1) Should find_deepest_state() ever return states with exit_latency equal to 0?
(2) If the answer to (1) is "yes", should the "poll state" be ever
returned by find_deepest_state()?

In any case, find_deepest_state() will only return a state with
exit_latency equal to 0 if there's no other choice and if it returns
nothing, we'll fall back to the architecture idle method.  So the
question really is whether or not falling back to arch idle is any
better than using any state we have in the table.

The patch is based on the assumption that any state from the table
will be better than arch idle, including the "polling state".  If that
does not hold, we'll need to rethink a couple of other things in my
view.

Thanks,
Rafael

Re: [PATCH] cpuidle: Do not use CPUIDLE_DRIVER_STATE_START in cpuidle.c

[Preeti U Murthy]

On 05/27/2015 07:27 PM, Rafael J. Wysocki wrote:
> On Wed, May 27, 2015 at 2:25 PM, Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>> On 05/27/2015 01:31 PM, Preeti U Murthy wrote:
>>>
>>> On 05/27/2015 07:06 AM, Rafael J. Wysocki wrote:
>>>>
>>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>>
>>>> The CPUIDLE_DRIVER_STATE_START symbol is defined as 1 only if
>>>> CONFIG_ARCH_HAS_CPU_RELAX is set, otherwise it is defined as 0.
>>>> However, if CONFIG_ARCH_HAS_CPU_RELAX is set, the first (index 0)
>>>> entry in the cpuidle driver's table of states is overwritten with
>>>> the default "poll" entry by the core.  The "state" defined by the
>>>> "poll" entry doesn't provide ->enter_dead and ->enter_freeze
>>>> callbacks and its exit_latency is 0.
>>>>
>>>> For this reason, it is not necessary to use CPUIDLE_DRIVER_STATE_START
>>>> in cpuidle_play_dead() (->enter_dead is NULL, so the "poll state"
>>>> will be skipped by the loop) and in find_deepest_state() (since
>>>> exit_latency is 0, the "poll state" will become the default if the
>>>> "s->exit_latency <= latency_req" check is replaced with
>>>> "s->exit_latency < latency_req" which may only matter for drivers
>>>> providing different states with the same exit_latency).
>>>>
>>>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>> ---
>>>>   drivers/cpuidle/cpuidle.c |    8 ++++----
>>>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> <snip>
>>>
>>>>
>>>> @@ -79,13 +79,13 @@ static int find_deepest_state(struct cpu
>>>>                               bool freeze)
>>>>   {
>>>>         unsigned int latency_req = 0;
>>>> -       int i, ret = freeze ? -1 : CPUIDLE_DRIVER_STATE_START - 1;
>>>> +       int i, ret = -ENXIO;
>>>>
>>>> -       for (i = CPUIDLE_DRIVER_STATE_START; i < drv->state_count; i++) {
>>>> +       for (i = 0; i < drv->state_count; i++) {
>>>>                 struct cpuidle_state *s = &drv->states[i];
>>>>                 struct cpuidle_state_usage *su = &dev->states_usage[i];
>>>>
>>>> -               if (s->disabled || su->disable || s->exit_latency <=
>>>> latency_req
>>>> +               if (s->disabled || su->disable || s->exit_latency <
>>>> latency_req
>>>
>>>
>>> Prior to this patch,
>>>
>>> For drivers on which CPUIDLE_DRIVER_STATE_START takes a value 0 and
>>> whose first idle state has an exit_latency of 0, find_deepest_state()
>>> would return -1 if it failed to find a deeper idle state.
>>> But as an effect of this patch, find_deepest_state() returns 0 in the
>>> above circumstance.
>>
>>
>> Except I am missing something, with an exit_latency = 0, the state will be
>> never selected, because of the "s->exit_latency < latency_req" condition
>> (strictly greater than).
> 
> No, this is the condition to skip the state, so previously it wouldn't
> be selected, but after the patch it will.
> 
> So yes, the patch changes behavior for systems with
> CONFIG_ARCH_HAS_CPU_RELAX unset.
> 
>>> My concern is if these drivers do not intend to enter a polling state
>>> during suspend, this will cause an issue, won't it?
> 
> The change in behavior happens for architectures where
> CONFIG_ARCH_HAS_CPU_RELAX is not set.  In those cases the 0-index
> state is supposed to be provided by the driver.  Is there a reason to
> expect that this may not be a genuine idle state?

On PowerPC, we have the 0-index idle state, whose exit_latency is 0 and
all that the CPU does in this state, is poll on need_resched(), except
at a lower priority from the hardware's standpoint. Nevertheless, the
CPU is busy polling. So, I would not consider it a genuine idle state.

On a side note, we do not yet support suspend on Power servers, but we
may in the future. Hence the concern.

>> Definitively poll can cause thermal issues, especially when suspending. It
>> is a dangerous state (let's imagine you close your laptop => suspend/poll
>> and then put it in your bag for a travel).
> 
> With ARCH_HAS_CPU_RELAX set the "poll state" is supposed to be thermally safe.
> 
>> I don't think with the code above we can reach this situation but I agree
>> this is something we have to take care carefully.
>>
>> Actually, I am in favour of removing poll at all from the cpuidle driver and
>> poll only when a cpuidle state selection fails under certain condition.
>>
>> So I fully agree with your statement below.
>>
>>> I would expect the cpus to be in a hardware
>>> defined idle state.
> 
> Well, except for the degenerate case in which all of them are disabled
> and for the "broadcast timer stopping aviodance" use case for
> find_deepest_state().

During suspend, the CPUs can very well enter states where the timer
stops since we stop timer interrupts anyway. So unless the idle states
are explicitly disabled by the user/hardware for some reason, deeper
idle states will still be available during suspend as far as I can see.

> 
> So there are two questions in my view:
> (1) Should find_deepest_state() ever return states with exit_latency equal to 0?

I would say no, since such an idle state would mostly be polling on a
wakeup event. Atleast, there is one such case in PowerPC.

> (2) If the answer to (1) is "yes", should the "poll state" be ever
> returned by find_deepest_state()?
> 
> In any case, find_deepest_state() will only return a state with
> exit_latency equal to 0 if there's no other choice and if it returns
> nothing, we'll fall back to the architecture idle method.  So the
> question really is whether or not falling back to arch idle is any
> better than using any state we have in the table.

My suggestion is to *not* fall back to arch idle code, since that is a
black box from the core cpuidle's perspective.

> 
> The patch is based on the assumption that any state from the table
> will be better than arch idle, including the "polling state".  If that
> does not hold, we'll need to rethink a couple of other things in my
> view.

We could fail suspend if a non-polling idle state is not available perhaps ?

Regards
Preeti U Murthy
> 
> Thanks,
> Rafael
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] cpuidle: Do not use CPUIDLE_DRIVER_STATE_START in cpuidle.c

[Rafael J. Wysocki]

On Wednesday, May 27, 2015 09:49:54 PM Preeti U Murthy wrote:
> On 05/27/2015 07:27 PM, Rafael J. Wysocki wrote:
> > On Wed, May 27, 2015 at 2:25 PM, Daniel Lezcano
> > <daniel.lezcano@linaro.org> wrote:
> >> On 05/27/2015 01:31 PM, Preeti U Murthy wrote:
> >>>
> >>> On 05/27/2015 07:06 AM, Rafael J. Wysocki wrote:
> >>>>
> >>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >>>>
> >>>> The CPUIDLE_DRIVER_STATE_START symbol is defined as 1 only if
> >>>> CONFIG_ARCH_HAS_CPU_RELAX is set, otherwise it is defined as 0.
> >>>> However, if CONFIG_ARCH_HAS_CPU_RELAX is set, the first (index 0)
> >>>> entry in the cpuidle driver's table of states is overwritten with
> >>>> the default "poll" entry by the core.  The "state" defined by the
> >>>> "poll" entry doesn't provide ->enter_dead and ->enter_freeze
> >>>> callbacks and its exit_latency is 0.
> >>>>
> >>>> For this reason, it is not necessary to use CPUIDLE_DRIVER_STATE_START
> >>>> in cpuidle_play_dead() (->enter_dead is NULL, so the "poll state"
> >>>> will be skipped by the loop) and in find_deepest_state() (since
> >>>> exit_latency is 0, the "poll state" will become the default if the
> >>>> "s->exit_latency <= latency_req" check is replaced with
> >>>> "s->exit_latency < latency_req" which may only matter for drivers
> >>>> providing different states with the same exit_latency).
> >>>>
> >>>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >>>> ---
> >>>>   drivers/cpuidle/cpuidle.c |    8 ++++----
> >>>>   1 file changed, 4 insertions(+), 4 deletions(-)
> >>>
> >>> <snip>
> >>>
> >>>>
> >>>> @@ -79,13 +79,13 @@ static int find_deepest_state(struct cpu
> >>>>                               bool freeze)
> >>>>   {
> >>>>         unsigned int latency_req = 0;
> >>>> -       int i, ret = freeze ? -1 : CPUIDLE_DRIVER_STATE_START - 1;
> >>>> +       int i, ret = -ENXIO;
> >>>>
> >>>> -       for (i = CPUIDLE_DRIVER_STATE_START; i < drv->state_count; i++) {
> >>>> +       for (i = 0; i < drv->state_count; i++) {
> >>>>                 struct cpuidle_state *s = &drv->states[i];
> >>>>                 struct cpuidle_state_usage *su = &dev->states_usage[i];
> >>>>
> >>>> -               if (s->disabled || su->disable || s->exit_latency <=
> >>>> latency_req
> >>>> +               if (s->disabled || su->disable || s->exit_latency <
> >>>> latency_req
> >>>
> >>>
> >>> Prior to this patch,
> >>>
> >>> For drivers on which CPUIDLE_DRIVER_STATE_START takes a value 0 and
> >>> whose first idle state has an exit_latency of 0, find_deepest_state()
> >>> would return -1 if it failed to find a deeper idle state.
> >>> But as an effect of this patch, find_deepest_state() returns 0 in the
> >>> above circumstance.
> >>
> >>
> >> Except I am missing something, with an exit_latency = 0, the state will be
> >> never selected, because of the "s->exit_latency < latency_req" condition
> >> (strictly greater than).
> > 
> > No, this is the condition to skip the state, so previously it wouldn't
> > be selected, but after the patch it will.
> > 
> > So yes, the patch changes behavior for systems with
> > CONFIG_ARCH_HAS_CPU_RELAX unset.
> > 
> >>> My concern is if these drivers do not intend to enter a polling state
> >>> during suspend, this will cause an issue, won't it?
> > 
> > The change in behavior happens for architectures where
> > CONFIG_ARCH_HAS_CPU_RELAX is not set.  In those cases the 0-index
> > state is supposed to be provided by the driver.  Is there a reason to
> > expect that this may not be a genuine idle state?
> 
> On PowerPC, we have the 0-index idle state, whose exit_latency is 0 and
> all that the CPU does in this state, is poll on need_resched(), except
> at a lower priority from the hardware's standpoint. Nevertheless, the
> CPU is busy polling. So, I would not consider it a genuine idle state.

OK, that's good to know.

Arguably, then, returning states with exit_latency equal to 0 from
find_deepest_state() may not be safe in general.

Well, we can make that rule, so I'll send an updated patch with that taken
into account.

> On a side note, we do not yet support suspend on Power servers, but we
> may in the future. Hence the concern.
> 
> >> Definitively poll can cause thermal issues, especially when suspending. It
> >> is a dangerous state (let's imagine you close your laptop => suspend/poll
> >> and then put it in your bag for a travel).
> > 
> > With ARCH_HAS_CPU_RELAX set the "poll state" is supposed to be thermally safe.
> > 
> >> I don't think with the code above we can reach this situation but I agree
> >> this is something we have to take care carefully.
> >>
> >> Actually, I am in favour of removing poll at all from the cpuidle driver and
> >> poll only when a cpuidle state selection fails under certain condition.
> >>
> >> So I fully agree with your statement below.
> >>
> >>> I would expect the cpus to be in a hardware
> >>> defined idle state.
> > 
> > Well, except for the degenerate case in which all of them are disabled
> > and for the "broadcast timer stopping aviodance" use case for
> > find_deepest_state().
> 
> During suspend, the CPUs can very well enter states where the timer
> stops since we stop timer interrupts anyway.

That's if you provide ->enter_freeze callbacks, otherwise it works like
runtime idle.

> So unless the idle states
> are explicitly disabled by the user/hardware for some reason, deeper
> idle states will still be available during suspend as far as I can see.

The case at hand is when ->enter_freeze callbacks are not available or
all states having them are disabled.

> > So there are two questions in my view:
> > (1) Should find_deepest_state() ever return states with exit_latency equal to 0?
> 
> I would say no, since such an idle state would mostly be polling on a
> wakeup event. Atleast, there is one such case in PowerPC.

OK

> > (2) If the answer to (1) is "yes", should the "poll state" be ever
> > returned by find_deepest_state()?
> > 
> > In any case, find_deepest_state() will only return a state with
> > exit_latency equal to 0 if there's no other choice and if it returns
> > nothing, we'll fall back to the architecture idle method.  So the
> > question really is whether or not falling back to arch idle is any
> > better than using any state we have in the table.
> 
> My suggestion is to *not* fall back to arch idle code, since that is a
> black box from the core cpuidle's perspective.

Well, we do that today in some cases.

> > The patch is based on the assumption that any state from the table
> > will be better than arch idle, including the "polling state".  If that
> > does not hold, we'll need to rethink a couple of other things in my
> > view.
> 
> We could fail suspend if a non-polling idle state is not available perhaps ?

The only thing we can do is to wake up the system immediately if there's
no "genuine" state to enter.  I can send a (separate) patch for that too.


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

[PATCH v2] cpuidle: Do not use CPUIDLE_DRIVER_STATE_START in cpuidle.c

[Rafael J. Wysocki]

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

The CPUIDLE_DRIVER_STATE_START symbol is defined as 1 only if
CONFIG_ARCH_HAS_CPU_RELAX is set, otherwise it is defined as 0.
However, if CONFIG_ARCH_HAS_CPU_RELAX is set, the first (index 0)
entry in the cpuidle driver's table of states is overwritten with
the default "poll" entry by the core.  The "state" defined by the
"poll" entry doesn't provide ->enter_dead and ->enter_freeze
callbacks and its exit_latency is 0.

For this reason, it is not necessary to use CPUIDLE_DRIVER_STATE_START
in cpuidle_play_dead() (->enter_dead is NULL, so the "poll state"
will be skipped by the loop).

It also is arguably unuseful to return states with exit_latency
equal to 0 from find_deepest_state(), so the function can be modified
to start the loop from index 0 and the "poll state" will be skipped by
it as a result of the check against latency_req.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpuidle/cpuidle.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Index: linux-pm/drivers/cpuidle/cpuidle.c
===================================================================
--- linux-pm.orig/drivers/cpuidle/cpuidle.c
+++ linux-pm/drivers/cpuidle/cpuidle.c
@@ -65,7 +65,7 @@ int cpuidle_play_dead(void)
 		return -ENODEV;
 
 	/* Find lowest-power state that supports long-term idle */
-	for (i = drv->state_count - 1; i >= CPUIDLE_DRIVER_STATE_START; i--)
+	for (i = drv->state_count - 1; i >= 0; i--)
 		if (drv->states[i].enter_dead)
 			return drv->states[i].enter_dead(dev, i);
 
@@ -79,9 +79,9 @@ static int find_deepest_state(struct cpu
 			      bool freeze)
 {
 	unsigned int latency_req = 0;
-	int i, ret = freeze ? -1 : CPUIDLE_DRIVER_STATE_START - 1;
+	int i, ret = -ENXIO;
 
-	for (i = CPUIDLE_DRIVER_STATE_START; i < drv->state_count; i++) {
+	for (i = 0; i < drv->state_count; i++) {
 		struct cpuidle_state *s = &drv->states[i];
 		struct cpuidle_state_usage *su = &dev->states_usage[i];
 

Re: [PATCH v2] cpuidle: Do not use CPUIDLE_DRIVER_STATE_START in cpuidle.c

[Preeti U Murthy]

On 05/28/2015 07:39 AM, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> The CPUIDLE_DRIVER_STATE_START symbol is defined as 1 only if
> CONFIG_ARCH_HAS_CPU_RELAX is set, otherwise it is defined as 0.
> However, if CONFIG_ARCH_HAS_CPU_RELAX is set, the first (index 0)
> entry in the cpuidle driver's table of states is overwritten with
> the default "poll" entry by the core.  The "state" defined by the
> "poll" entry doesn't provide ->enter_dead and ->enter_freeze
> callbacks and its exit_latency is 0.
> 
> For this reason, it is not necessary to use CPUIDLE_DRIVER_STATE_START
> in cpuidle_play_dead() (->enter_dead is NULL, so the "poll state"
> will be skipped by the loop).
> 
> It also is arguably unuseful to return states with exit_latency
> equal to 0 from find_deepest_state(), so the function can be modified
> to start the loop from index 0 and the "poll state" will be skipped by
> it as a result of the check against latency_req.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/cpuidle/cpuidle.c |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> Index: linux-pm/drivers/cpuidle/cpuidle.c
> ===================================================================
> --- linux-pm.orig/drivers/cpuidle/cpuidle.c
> +++ linux-pm/drivers/cpuidle/cpuidle.c
> @@ -65,7 +65,7 @@ int cpuidle_play_dead(void)
>  		return -ENODEV;
> 
>  	/* Find lowest-power state that supports long-term idle */
> -	for (i = drv->state_count - 1; i >= CPUIDLE_DRIVER_STATE_START; i--)
> +	for (i = drv->state_count - 1; i >= 0; i--)
>  		if (drv->states[i].enter_dead)
>  			return drv->states[i].enter_dead(dev, i);
> 
> @@ -79,9 +79,9 @@ static int find_deepest_state(struct cpu
>  			      bool freeze)
>  {
>  	unsigned int latency_req = 0;
> -	int i, ret = freeze ? -1 : CPUIDLE_DRIVER_STATE_START - 1;
> +	int i, ret = -ENXIO;
> 
> -	for (i = CPUIDLE_DRIVER_STATE_START; i < drv->state_count; i++) {
> +	for (i = 0; i < drv->state_count; i++) {
>  		struct cpuidle_state *s = &drv->states[i];
>  		struct cpuidle_state_usage *su = &dev->states_usage[i];
> 

Reviewed-by: Preeti U Murthy <preeti@linux.vnet.ibm.com>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>