[patch 0/2] cpuidle: Time accounting fix

[patch 0/2] cpuidle: Time accounting fix

[Venkatesh Pallipadi]

As noted in PM minisummit notes here
http://lwn.net/Articles/292447/

Richard Woodruff <r-woodruff2@ti.com> reported a bug
"the CPUIDLE bug that if target is avoided due to BM activity the original
target state is still accounted the time."

Following provides a mechanism to fix the problem for all drivers and fixes
the problem for acpi cpuidle driver.

-- 

[patch 1/2] cpuidle: use last_state which can reflect the actual state entered

[Venkatesh Pallipadi]

[-- Attachment #1: cpuidle_proper_time_accounting.patch --]
[-- Type: text/plain, Size: 1338 bytes --]

cpuidle accounts the idle time for the C-state it was trying to enter and
not to the actual state that the driver eventually entered. The driver may
select a different state than the one chosen by cpuidle due to
constraints like bus-mastering, etc.

Change the time acounting code to look at the dev->last_state after
returning from target_state->enter(). Driver can modify dev->last_state
internally, inside the enter routine to reflect the actual C-state
entered.

Signed-off-by: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>

---
 drivers/cpuidle/cpuidle.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Index: tip/drivers/cpuidle/cpuidle.c
===================================================================
--- tip.orig/drivers/cpuidle/cpuidle.c	2008-09-23 13:52:59.000000000 -0700
+++ tip/drivers/cpuidle/cpuidle.c	2008-09-23 14:22:43.000000000 -0700
@@ -71,8 +71,11 @@ static void cpuidle_idle_call(void)
 	target_state = &dev->states[next_state];
 
 	/* enter the state and update stats */
-	dev->last_residency = target_state->enter(dev, target_state);
 	dev->last_state = target_state;
+	dev->last_residency = target_state->enter(dev, target_state);
+	if (dev->last_state)
+		target_state = dev->last_state;
+
 	target_state->time += (unsigned long long)dev->last_residency;
 	target_state->usage++;
 

-- 

[patch 2/2] cpuidle: update the last_state acpi cpuidle reflecting actual state entered

[Venkatesh Pallipadi]

[-- Attachment #1: acpi_cpuidle_driver_set_last_state.patch --]
[-- Type: text/plain, Size: 778 bytes --]

reflect the actual state entered in dev->last_state, when actaul state entered
is different from intended one.

Signed-off-by: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>

---
 drivers/acpi/processor_idle.c |    1 +
 1 file changed, 1 insertion(+)

Index: tip/drivers/acpi/processor_idle.c
===================================================================
--- tip.orig/drivers/acpi/processor_idle.c	2008-09-23 11:40:50.000000000 -0700
+++ tip/drivers/acpi/processor_idle.c	2008-09-23 14:22:48.000000000 -0700
@@ -1587,6 +1587,7 @@ static int acpi_idle_enter_bm(struct cpu
 
 	if (acpi_idle_bm_check()) {
 		if (dev->safe_state) {
+			dev->last_state = dev->safe_state;
 			return dev->safe_state->enter(dev, dev->safe_state);
 		} else {
 			local_irq_disable();

-- 

Re: [patch 1/2] cpuidle: use last_state which can reflect the actual state entered

[Kevin Hilman]

Venkatesh Pallipadi <venkatesh.pallipadi@intel.com> writes:

> cpuidle accounts the idle time for the C-state it was trying to enter and
> not to the actual state that the driver eventually entered. The driver may
> select a different state than the one chosen by cpuidle due to
> constraints like bus-mastering, etc.
>
> Change the time acounting code to look at the dev->last_state after
> returning from target_state->enter(). Driver can modify dev->last_state
> internally, inside the enter routine to reflect the actual C-state
> entered.
>
> Signed-off-by: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>
>
> ---
>  drivers/cpuidle/cpuidle.c |    5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> Index: tip/drivers/cpuidle/cpuidle.c
> ===================================================================
> --- tip.orig/drivers/cpuidle/cpuidle.c	2008-09-23 13:52:59.000000000 -0700
> +++ tip/drivers/cpuidle/cpuidle.c	2008-09-23 14:22:43.000000000 -0700
> @@ -71,8 +71,11 @@ static void cpuidle_idle_call(void)
>  	target_state = &dev->states[next_state];
>  
>  	/* enter the state and update stats */
> -	dev->last_residency = target_state->enter(dev, target_state);
>  	dev->last_state = target_state;
> +	dev->last_residency = target_state->enter(dev, target_state);
> +	if (dev->last_state)
> +		target_state = dev->last_state;
> +
>  	target_state->time += (unsigned long long)dev->last_residency;
>  	target_state->usage++;

Under what conditions would the enter hook set dev->last_sate to NULL?
Having the check seems to indicate it's possilble.

A minor nit-pick...  why not explicitly do the accounting using
'last_state' like below.  While functionally the same as above, this
makes it makes it more explicit when reading the code that the
accounting is done using 'last_state' and not 'target_state.'

Kevin


diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index 5ce07b5..c1294f5 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -67,10 +67,10 @@ static void cpuidle_idle_call(void)
 	target_state = &dev->states[next_state];
 
 	/* enter the state and update stats */
-	dev->last_residency = target_state->enter(dev, target_state);
 	dev->last_state = target_state;
-	target_state->time += (unsigned long long)dev->last_residency;
-	target_state->usage++;
+	dev->last_residency = target_state->enter(dev, target_state);
+	dev->last_state->time += (unsigned long long)dev->last_residency;
+	dev->last_state->usage++;
 
 	/* give the governor an opportunity to reflect on the outcome */
 	if (cpuidle_curr_governor->reflect)

Re: [patch 1/2] cpuidle: use last_state which can reflect the actual state entered

[Pallipadi, Venkatesh]

>-----Original Message-----
>From: Kevin Hilman [mailto:khilman@deeprootsystems.com]
>Sent: Wednesday, October 01, 2008 4:09 AM
>To: Pallipadi, Venkatesh
>Cc: lenb@kernel.org; linux-pm@lists.linux-foundation.org
>Subject: Re: [linux-pm] [patch 1/2] cpuidle: use last_state
>which can reflect the actual state entered
>
>Venkatesh Pallipadi <venkatesh.pallipadi@intel.com> writes:
>
>> cpuidle accounts the idle time for the C-state it was trying
>to enter and
>> not to the actual state that the driver eventually entered.
>The driver may
>> select a different state than the one chosen by cpuidle due to
>> constraints like bus-mastering, etc.
>>
>> Change the time acounting code to look at the dev->last_state after
>> returning from target_state->enter(). Driver can modify
>dev->last_state
>> internally, inside the enter routine to reflect the actual C-state
>> entered.
>>
>> Signed-off-by: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>
>>
>> ---
>>  drivers/cpuidle/cpuidle.c |    5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> Index: tip/drivers/cpuidle/cpuidle.c
>> ===================================================================
>> --- tip.orig/drivers/cpuidle/cpuidle.c        2008-09-23
>13:52:59.000000000 -0700
>> +++ tip/drivers/cpuidle/cpuidle.c     2008-09-23
>14:22:43.000000000 -0700
>> @@ -71,8 +71,11 @@ static void cpuidle_idle_call(void)
>>       target_state = &dev->states[next_state];
>>
>>       /* enter the state and update stats */
>> -     dev->last_residency = target_state->enter(dev, target_state);
>>       dev->last_state = target_state;
>> +     dev->last_residency = target_state->enter(dev, target_state);
>> +     if (dev->last_state)
>> +             target_state = dev->last_state;
>> +
>>       target_state->time += (unsigned long long)dev->last_residency;
>>       target_state->usage++;
>
>Under what conditions would the enter hook set dev->last_sate to NULL?
>Having the check seems to indicate it's possilble.

Sorry about the delayed response.

Yes. I added the check after finding out that last_state can be possibly NULL.
That happens when the governor changes while one core is in idle and also during
CPU offline/online.

>A minor nit-pick...  why not explicitly do the accounting using
>'last_state' like below.  While functionally the same as above, this
>makes it makes it more explicit when reading the code that the
>accounting is done using 'last_state' and not 'target_state.'
>

Yes. It is cleaner. But, we still have to check for last_state being NULL.

>
>
>diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
>index 5ce07b5..c1294f5 100644
>--- a/drivers/cpuidle/cpuidle.c
>+++ b/drivers/cpuidle/cpuidle.c
>@@ -67,10 +67,10 @@ static void cpuidle_idle_call(void)
>        target_state = &dev->states[next_state];
>
>        /* enter the state and update stats */
>-       dev->last_residency = target_state->enter(dev, target_state);
>        dev->last_state = target_state;
>-       target_state->time += (unsigned long long)dev->last_residency;
>-       target_state->usage++;
>+       dev->last_residency = target_state->enter(dev, target_state);
>+       dev->last_state->time += (unsigned long
>long)dev->last_residency;
>+       dev->last_state->usage++;
>
>        /* give the governor an opportunity to reflect on the
>outcome */
>        if (cpuidle_curr_governor->reflect)
>

Thanks,
Venki

Re: [patch 1/2] cpuidle: use last_state which can reflect the actual state entered

[Kevin Hilman]

"Pallipadi, Venkatesh" <venkatesh.pallipadi@intel.com> writes:

>>Venkatesh Pallipadi <venkatesh.pallipadi@intel.com> writes:
>>
>>> cpuidle accounts the idle time for the C-state it was trying
>>to enter and
>>> not to the actual state that the driver eventually entered.
>>The driver may
>>> select a different state than the one chosen by cpuidle due to
>>> constraints like bus-mastering, etc.
>>>
>>> Change the time acounting code to look at the dev->last_state after
>>> returning from target_state->enter(). Driver can modify
>>dev->last_state
>>> internally, inside the enter routine to reflect the actual C-state
>>> entered.
>>>
>>> Signed-off-by: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>
>>>
>>> ---
>>>  drivers/cpuidle/cpuidle.c |    5 ++++-
>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> Index: tip/drivers/cpuidle/cpuidle.c
>>> ===================================================================
>>> --- tip.orig/drivers/cpuidle/cpuidle.c        2008-09-23
>>13:52:59.000000000 -0700
>>> +++ tip/drivers/cpuidle/cpuidle.c     2008-09-23
>>14:22:43.000000000 -0700
>>> @@ -71,8 +71,11 @@ static void cpuidle_idle_call(void)
>>>       target_state = &dev->states[next_state];
>>>
>>>       /* enter the state and update stats */
>>> -     dev->last_residency = target_state->enter(dev, target_state);
>>>       dev->last_state = target_state;
>>> +     dev->last_residency = target_state->enter(dev, target_state);
>>> +     if (dev->last_state)
>>> +             target_state = dev->last_state;
>>> +
>>>       target_state->time += (unsigned long long)dev->last_residency;
>>>       target_state->usage++;
>>
>>Under what conditions would the enter hook set dev->last_sate to NULL?
>>Having the check seems to indicate it's possilble.
>
> Sorry about the delayed response.
>
> Yes. I added the check after finding out that last_state can be
> possibly NULL.  That happens when the governor changes while one
> core is in idle and also during CPU offline/online.

OK, thanks for the clarification.

>>A minor nit-pick...  why not explicitly do the accounting using
>>'last_state' like below.  While functionally the same as above, this
>>makes it makes it more explicit when reading the code that the
>>accounting is done using 'last_state' and not 'target_state.'
>>
>
> Yes. It is cleaner. But, we still have to check for last_state being NULL.

Agreed.

I've switched to your version and tested this on OMAP3 and it's
working well.

Tested-by: Kevin Hilman <khilman@deeprootsystems.com>


>>
>>
>>diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
>>index 5ce07b5..c1294f5 100644
>>--- a/drivers/cpuidle/cpuidle.c
>>+++ b/drivers/cpuidle/cpuidle.c
>>@@ -67,10 +67,10 @@ static void cpuidle_idle_call(void)
>>        target_state = &dev->states[next_state];
>>
>>        /* enter the state and update stats */
>>-       dev->last_residency = target_state->enter(dev, target_state);
>>        dev->last_state = target_state;
>>-       target_state->time += (unsigned long long)dev->last_residency;
>>-       target_state->usage++;
>>+       dev->last_residency = target_state->enter(dev, target_state);
>>+       dev->last_state->time += (unsigned long
>>long)dev->last_residency;
>>+       dev->last_state->usage++;
>>
>>        /* give the governor an opportunity to reflect on the
>>outcome */
>>        if (cpuidle_curr_governor->reflect)
>>
>
> Thanks,
> Venki

Re: [patch 1/2] cpuidle: use last_state which can reflect the actual state entered

[Len Brown]

1,2 applied.

thanks,
-len

On Mon, 29 Sep 2008, Venkatesh Pallipadi wrote:

> cpuidle accounts the idle time for the C-state it was trying to enter and
> not to the actual state that the driver eventually entered. The driver may
> select a different state than the one chosen by cpuidle due to
> constraints like bus-mastering, etc.
> 
> Change the time acounting code to look at the dev->last_state after
> returning from target_state->enter(). Driver can modify dev->last_state
> internally, inside the enter routine to reflect the actual C-state
> entered.
> 
> Signed-off-by: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>
> 
> ---
>  drivers/cpuidle/cpuidle.c |    5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> Index: tip/drivers/cpuidle/cpuidle.c
> ===================================================================
> --- tip.orig/drivers/cpuidle/cpuidle.c	2008-09-23 13:52:59.000000000 -0700
> +++ tip/drivers/cpuidle/cpuidle.c	2008-09-23 14:22:43.000000000 -0700
> @@ -71,8 +71,11 @@ static void cpuidle_idle_call(void)
>  	target_state = &dev->states[next_state];
>  
>  	/* enter the state and update stats */
> -	dev->last_residency = target_state->enter(dev, target_state);
>  	dev->last_state = target_state;
> +	dev->last_residency = target_state->enter(dev, target_state);
> +	if (dev->last_state)
> +		target_state = dev->last_state;
> +
>  	target_state->time += (unsigned long long)dev->last_residency;
>  	target_state->usage++;
>  
> 
> -- 
>