Re: [PATCH] CPUidle: increment state counter for state actually entered

Re: [PATCH] CPUidle: increment state counter for state actually entered

[Kevin Hilman]

Kevin Hilman <khilman@deeprootsystems.com> writes:

> Currently, the count for state that the governor chooses is always
> incremented after the 'state_enter' hook is called.  However, the
> target's enter hook may choose a different state based on BM activity
> or other factors.
>
> This patch does the accounting use 'dev->last_state' state instead of
> the governor-chosen state.  If the target's enter_idle hook enters a
> different state than it was asked, it should update dev->last_state.
>

Oops, I sent this before I saw Venkatesh's earlier patch to do the
same.

Kevin

> ---
>  drivers/cpuidle/cpuidle.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
>
> 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)
> -- 
> 1.6.0

Re: [PATCH] CPUidle: increment state counter for state actually entered

[Woodruff, Richard]

[-- Attachment #1: Type: text/plain, Size: 1084 bytes --]


> owner@vger.kernel.org] On Behalf Of Kevin Hilman
> Sent: Wednesday, October 01, 2008 5:47 AM

> This patch does the accounting use 'dev->last_state' state instead of
> the governor-chosen state.  If the target's enter_idle hook enters a
> different state than it was asked, it should update dev->last_state.

Isn't this the same as the patch Venki pushed on the pm list?  I've been using a version of it for a while from him and haven't seen any issue.

I had also done a version which was much more evasive.  In the version I was hacking at I tried to push the aborted state information back into the governor as it locally caches the last state tried.

Doing this simpler way like in patch does make sure the accounting is correct which is a plus, powertop doesn't 'lie' any more.  It however, does not stop a following likely mis-prediction based on the local state kept in the governor.

Doing at least this much is necessary.  However, if you don't want to feed the false state data into the predictor more linkage has to be done.

Regards,
Richard W.




[-- Attachment #2: Type: message/rfc822, Size: 4498 bytes --]

From: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>
To: "lenb@kernel.org" <lenb@kernel.org>
Cc: "linux-pm@lists.linux-foundation.org" <linux-pm@lists.linux-foundation.org>
Subject: [linux-pm] [patch 0/2] cpuidle: Time accounting fix
Date: Mon, 29 Sep 2008 17:24:26 -0500
Message-ID: <20080929222426.914988000@intel.com>

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.

--

_______________________________________________
linux-pm mailing list
linux-pm@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/linux-pm


[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

[PATCH] CPUidle: increment state counter for state actually entered

[Kevin Hilman]

Currently, the count for state that the governor chooses is always
incremented after the 'state_enter' hook is called.  However, the
target's enter hook may choose a different state based on BM activity
or other factors.

This patch does the accounting use 'dev->last_state' state instead of
the governor-chosen state.  If the target's enter_idle hook enters a
different state than it was asked, it should update dev->last_state.

Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
---
 drivers/cpuidle/cpuidle.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

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)
-- 
1.6.0