[PATCH] cpuidle/powernv: Enter fastsleep on checking if deep idle states are allowed

[PATCH] cpuidle/powernv: Enter fastsleep on checking if deep idle states are allowed

[Preeti U Murthy]

Today the procfs interface /proc/sys/kernel/powersave-nap is used to control
entry into deep idle states beyond snooze. Check for the value of this
parameter before entering fastsleep. We already do this check for nap in
power7_idle().

Signed-off-by: Preeti U Murthy <preeti@linux.vnet.ibm.com>
---

 drivers/cpuidle/cpuidle-powernv.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/cpuidle/cpuidle-powernv.c b/drivers/cpuidle/cpuidle-powernv.c
index a64be57..b8ba52e 100644
--- a/drivers/cpuidle/cpuidle-powernv.c
+++ b/drivers/cpuidle/cpuidle-powernv.c
@@ -69,6 +69,12 @@ static int fastsleep_loop(struct cpuidle_device *dev,
 	unsigned long old_lpcr = mfspr(SPRN_LPCR);
 	unsigned long new_lpcr;
 
+	/*
+	 * Verify if snooze is the only valid cpuidle state
+	 */
+	if (!(powersave_nap > 0))
+		return index;
+
 	if (unlikely(system_state < SYSTEM_RUNNING))
 		return index;
 

Re: [PATCH] cpuidle/powernv: Enter fastsleep on checking if deep idle states are allowed

[Michael Ellerman]

On Fri, 2014-09-12 at 16:31 +0530, Preeti U Murthy wrote:
> Today the procfs interface /proc/sys/kernel/powersave-nap is used to control
> entry into deep idle states beyond snooze. Check for the value of this
> parameter before entering fastsleep. We already do this check for nap in
> power7_idle().
> 
> Signed-off-by: Preeti U Murthy <preeti@linux.vnet.ibm.com>
> ---
> 
>  drivers/cpuidle/cpuidle-powernv.c |    6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/cpuidle/cpuidle-powernv.c b/drivers/cpuidle/cpuidle-powernv.c
> index a64be57..b8ba52e 100644
> --- a/drivers/cpuidle/cpuidle-powernv.c
> +++ b/drivers/cpuidle/cpuidle-powernv.c
> @@ -69,6 +69,12 @@ static int fastsleep_loop(struct cpuidle_device *dev,
>  	unsigned long old_lpcr = mfspr(SPRN_LPCR);
>  	unsigned long new_lpcr;
>  
> +	/*
> +	 * Verify if snooze is the only valid cpuidle state
> +	 */
> +	if (!(powersave_nap > 0))
> +		return index;
> +
>  	if (unlikely(system_state < SYSTEM_RUNNING))
>  		return index;

Doesn't the above mean we are just going to keep trying to go into fastsleep
again and again? Or does the idle code work out that it didn't work based on
the fact that we didn't sleep for the right period?

We were talking about getting rid of powersave_nap altogether, but I think we
decided we couldn't, I forget.

cheers

Re: [PATCH] cpuidle/powernv: Enter fastsleep on checking if deep idle states are allowed

[Preeti U Murthy]

On 09/15/2014 12:29 PM, Michael Ellerman wrote:
> On Fri, 2014-09-12 at 16:31 +0530, Preeti U Murthy wrote:
>> Today the procfs interface /proc/sys/kernel/powersave-nap is used to control
>> entry into deep idle states beyond snooze. Check for the value of this
>> parameter before entering fastsleep. We already do this check for nap in
>> power7_idle().
>>
>> Signed-off-by: Preeti U Murthy <preeti@linux.vnet.ibm.com>
>> ---
>>
>>  drivers/cpuidle/cpuidle-powernv.c |    6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/cpuidle/cpuidle-powernv.c b/drivers/cpuidle/cpuidle-powernv.c
>> index a64be57..b8ba52e 100644
>> --- a/drivers/cpuidle/cpuidle-powernv.c
>> +++ b/drivers/cpuidle/cpuidle-powernv.c
>> @@ -69,6 +69,12 @@ static int fastsleep_loop(struct cpuidle_device *dev,
>>  	unsigned long old_lpcr = mfspr(SPRN_LPCR);
>>  	unsigned long new_lpcr;
>>  
>> +	/*
>> +	 * Verify if snooze is the only valid cpuidle state
>> +	 */
>> +	if (!(powersave_nap > 0))
>> +		return index;
>> +
>>  	if (unlikely(system_state < SYSTEM_RUNNING))
>>  		return index;
> 
> Doesn't the above mean we are just going to keep trying to go into fastsleep
> again and again? Or does the idle code work out that it didn't work based on
> the fact that we didn't sleep for the right period?

Thats right. The idle code figures that its judgment to enter fastsleep
was not correct and applies a correction factor to its future decisions
. This correction factor is intended to influence the cpuidle governor's
decision on choosing an idle state for the cpu based on the history of
wakeups. Hence a shallower idle state will be chosen here on in the
above circumstance.

> 
> We were talking about getting rid of powersave_nap altogether, but I think we
> decided we couldn't, I forget.

Isn't this a helpful knob to disable cpuidle at runtime? Currently we
check the value of powersave_nap before entering both nap and fastsleep.

Regards
Preeti U Murthy
> 
> cheers
> 
> 
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
> 

Re: [PATCH] cpuidle/powernv: Enter fastsleep on checking if deep idle states are allowed

[Benjamin Herrenschmidt]

On Mon, 2014-09-15 at 14:22 +0530, Preeti U Murthy wrote:
> > We were talking about getting rid of powersave_nap altogether, but I
> think we
> > decided we couldn't, I forget.
> 
> Isn't this a helpful knob to disable cpuidle at runtime? Currently we
> check the value of powersave_nap before entering both nap and
> fastsleep.

It's useful when diagnosing some problems that can be caused by idle
states.

Cheers,
Ben.

Re: [PATCH] cpuidle/powernv: Enter fastsleep on checking if deep idle states are allowed

[Preeti U Murthy]

On 09/15/2014 02:22 PM, Preeti U Murthy wrote:
> On 09/15/2014 12:29 PM, Michael Ellerman wrote:
>> On Fri, 2014-09-12 at 16:31 +0530, Preeti U Murthy wrote:
>>> Today the procfs interface /proc/sys/kernel/powersave-nap is used to control
>>> entry into deep idle states beyond snooze. Check for the value of this
>>> parameter before entering fastsleep. We already do this check for nap in
>>> power7_idle().
>>>
>>> Signed-off-by: Preeti U Murthy <preeti@linux.vnet.ibm.com>
>>> ---
>>>
>>>  drivers/cpuidle/cpuidle-powernv.c |    6 ++++++
>>>  1 file changed, 6 insertions(+)
>>>
>>> diff --git a/drivers/cpuidle/cpuidle-powernv.c b/drivers/cpuidle/cpuidle-powernv.c
>>> index a64be57..b8ba52e 100644
>>> --- a/drivers/cpuidle/cpuidle-powernv.c
>>> +++ b/drivers/cpuidle/cpuidle-powernv.c
>>> @@ -69,6 +69,12 @@ static int fastsleep_loop(struct cpuidle_device *dev,
>>>  	unsigned long old_lpcr = mfspr(SPRN_LPCR);
>>>  	unsigned long new_lpcr;
>>>  
>>> +	/*
>>> +	 * Verify if snooze is the only valid cpuidle state
>>> +	 */
>>> +	if (!(powersave_nap > 0))
>>> +		return index;
>>> +
>>>  	if (unlikely(system_state < SYSTEM_RUNNING))
>>>  		return index;
>>
>> Doesn't the above mean we are just going to keep trying to go into fastsleep
>> again and again? Or does the idle code work out that it didn't work based on
>> the fact that we didn't sleep for the right period?
> 
> Thats right. The idle code figures that its judgment to enter fastsleep
> was not correct and applies a correction factor to its future decisions
> . This correction factor is intended to influence the cpuidle governor's
> decision on choosing an idle state for the cpu based on the history of
> wakeups. Hence a shallower idle state will be chosen here on in the
> above circumstance.

On second thoughts, I feel that this is not the best way to disable deep
idle states. Going down this path means the governor should identify
that fastsleep is not being entered into and take corrective action from
then on. If the governor is buggy, we pointlessly enter and exit
fastsleep_loop() during long idle periods.

So a better way to disable fastsleep during runtime is through the
disable flag in sysfs. To disable deep idle states at bootup one could
use powersave=off kernel cmd line parameter.

The functionality provided by powersave_nap and smt_snooze_delay (to
quote another example) carried value during the days that powernv did
not have a cpuidle driver. We retain them so as to not break userspace,
although they do not carry value today. Any future users of these
parameters should be avoided IMO. Hence I retract this patch.

Regards
Preeti U Murthy

> 
>>
>> We were talking about getting rid of powersave_nap altogether, but I think we
>> decided we couldn't, I forget.
> 
> Isn't this a helpful knob to disable cpuidle at runtime? Currently we
> check the value of powersave_nap before entering both nap and fastsleep.
> 
> Regards
> Preeti U Murthy
>>
>> cheers
>>
>>
>> _______________________________________________
>> Linuxppc-dev mailing list
>> Linuxppc-dev@lists.ozlabs.org
>> https://lists.ozlabs.org/listinfo/linuxppc-dev
>>
> 
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
>