cpuidle C-state accounting

cpuidle C-state accounting

[Surinder P Singh]

Hi,

I have a question about cpuidle C-state accounting in
cpuidle_idle_call() as implemented in kernel v2.6.29.

I understand it is possible for the target state entering callback to
return 0, if for some reason it could not enter the requested C-state
as decided by the governor. In this case, shouldn't the usage count be
updated conditionally to reflect the fact that the state may or may
not actually have been entered ? essentially, something like this:

--- cpuidle.c   2009-03-24 04:42:14.000000000 +0530
+++ cpuidle_modified.c  2009-05-19 11:39:58.000000000 +0530
@@ -86,7 +86,8 @@
                target_state = dev->last_state;

        target_state->time += (unsigned long long)dev->last_residency;
-       target_state->usage++;
+       if (dev->last_residency)
+               target_state->usage++;

        /* give the governor an opportunity to reflect on the outcome */
        if (cpuidle_curr_governor->reflect)


Cheers,
surinder

Re: cpuidle C-state accounting

[Woodruff, Richard]

> From: linux-pm-bounces@lists.linux-foundation.org [mailto:linux-pm-
> bounces@lists.linux-foundation.org] On Behalf Of Surinder P Singh
> Sent: Tuesday, May 19, 2009 11:25 AM


> I have a question about cpuidle C-state accounting in
> cpuidle_idle_call() as implemented in kernel v2.6.29.
>
> I understand it is possible for the target state entering callback to
> return 0, if for some reason it could not enter the requested C-state
> as decided by the governor. In this case, shouldn't the usage count be
> updated conditionally to reflect the fact that the state may or may
> not actually have been entered ? essentially, something like this:

If it fails state due to bus master check you can do like the reference acpi driver does.  I validated the same on OMAP3 a while back.

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=addbad46ed0906cd584784423b9d0babc7476446

In code I've used I don't demote other states.  But there are state aborts.  In those cases residency is low or 0.

Regards,
Richard W.

cpuidle C-state accounting

[Surinder P Singh]

> But there are state aborts.  In those cases residency is low or 0.

Indeed, its the residency=0 cases that the patch I sent in my last
mail attempts to fix. The idea being, not to increment
target_state->usage if dev->last_residency was 0, i.e. the state
(whether it was the governor recommended state or  safe_state or any
other state) was  not entered or used.

Cheers,
surinder

Re: cpuidle C-state accounting

[Surinder P Singh]

> In code I've used I don't demote other states.  But there are state aborts.  In those cases residency is low or 0.

Indeed, its the residency=0 cases that the patch I sent in my last
mail attempts to fix. The idea being, not to increment
target_state->usage if dev->last_residency was 0, i.e. the state
(whether it was the governor recommended state or  safe_state or any
other state) was  not entered or used.

Cheers,
surinder

P.S Richard, pls ignore my other mail with the same content. I goofed
up & missed your email in my inbox and sent the reply out of context.
This mail should link it back to thread.

Re: cpuidle C-state accounting

[Aneesh Bhasin]

Hi Surinder,

On Wed, May 20, 2009 at 3:06 PM, Surinder P Singh <srplsnh@gmail.com> wrote:
>> In code I've used I don't demote other states.  But there are state aborts.  In those cases residency is low or 0.
>
> Indeed, its the residency=0 cases that the patch I sent in my last
> mail attempts to fix. The idea being, not to increment
> target_state->usage if dev->last_residency was 0, i.e. the state
> (whether it was the governor recommended state or  safe_state or any
> other state) was  not entered or used.

That being the case , perhaps it will be useful to then have an extra
'usage' stat which shows such scenarios too for analysis purpose.
Patch below :

diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index 9ab50be..a835bc2 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -57,7 +57,10 @@ static void cpuidle_idle_call(void)
        dev->last_residency = target_state->enter(dev, target_state);
        dev->last_state = target_state;
        target_state->time += dev->last_residency;
-       target_state->usage++;
+  if(dev->last_residency)
+    target_state->usage++;
+  else
+    target_state->usage_nosuccess++;

        /* give the governor an opportunity to reflect on the outcome */
        if (cpuidle_curr_governor->reflect)
@@ -137,6 +140,7 @@ int cpuidle_enable_device(struct cpuidle_device *dev)

        for (i = 0; i < dev->state_count; i++) {
                dev->states[i].usage = 0;
+               dev->states[i].usage_nosuccess = 0;
                dev->states[i].time = 0;
        }
        dev->last_residency = 0;
diff --git a/drivers/cpuidle/sysfs.c b/drivers/cpuidle/sysfs.c
index 0f3515e..eaf21a1 100644
--- a/drivers/cpuidle/sysfs.c
+++ b/drivers/cpuidle/sysfs.c
@@ -226,6 +226,7 @@ static ssize_t show_state_name(struct
cpuidle_state *state, char *buf)
 define_show_state_function(exit_latency)
 define_show_state_function(power_usage)
 define_show_state_function(usage)
+define_show_state_function(usage_nosuccess)
 define_show_state_function(time)
 define_one_state_ro(name, show_state_name);
 define_one_state_ro(latency, show_state_exit_latency);
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index c4e0016..71fe0ac 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -37,6 +37,7 @@ struct cpuidle_state {
        unsigned int    target_residency; /* in US */

        unsigned int    usage;
+       unsigned int    usage_nosuccess;
        unsigned int    time; /* in US */

        int (*enter)    (struct cpuidle_device *dev,

Regards,
Aneesh