cpuidle C-state accounting

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:

@@ -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

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.

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.

Hi Surinder,

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 :

index 9ab50be..a835bc2 100644
@@ -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;

index 0f3515e..eaf21a1 100644
@@ -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);

index c4e0016..71fe0ac 100644
@@ -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