diff mbox

[linux-pm,RFC,V3,1/4] cpuidle: Move dev->last_residency update to driver enter routine; remove dev->last_state

Message ID 87pqog6dfw.fsf@ti.com (mailing list archive)
State RFC
Headers show

Commit Message

Kevin Hilman April 20, 2011, 5:27 p.m. UTC
Trinabh Gupta <trinabh@linux.vnet.ibm.com> writes:

> Cpuidle subsystem only suggests the state to enter and does not
> guarantee if the suggested state is entered. The actual entered state
> may be different because of software or hardware demotion. Software
> demotion is done by the back-end cpuidle driver and can be accounted
> correctly. Current cpuidle code uses last_state field to capture the
> actual state entered and based on that updates the statistics for the
> state entered.
>
> Ideally the driver enter routine should update the counters,
> and it should return the state actually entered rather than the time
> spent there. 

OK, the return type was changed to return the state index instead of the
time, but since the governors are still relying on dev->last_residency,
drivers are required to update it.

Because of that, why not keep the update of the  time/usage counters
in common code rather than duplicating it (9 times in this patch) into 
all the drivers?

Something like the patch below should suffice.

Kevin


--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Trinabh Gupta April 21, 2011, 4:42 a.m. UTC | #1
On 04/20/2011 10:57 PM, Kevin Hilman wrote:
> Trinabh Gupta<trinabh@linux.vnet.ibm.com>  writes:
>
>> Cpuidle subsystem only suggests the state to enter and does not
>> guarantee if the suggested state is entered. The actual entered state
>> may be different because of software or hardware demotion. Software
>> demotion is done by the back-end cpuidle driver and can be accounted
>> correctly. Current cpuidle code uses last_state field to capture the
>> actual state entered and based on that updates the statistics for the
>> state entered.
>>
>> Ideally the driver enter routine should update the counters,
>> and it should return the state actually entered rather than the time
>> spent there.
>
> OK, the return type was changed to return the state index instead of the
> time, but since the governors are still relying on dev->last_residency,
> drivers are required to update it.
>
> Because of that, why not keep the update of the  time/usage counters
> in common code rather than duplicating it (9 times in this patch) into
> all the drivers?

Hi Kevin,

Thanks for your review. Yes, we can do like this and it would prevent
duplication of code. I just wanted to make cpuidle handle only selection
and entering of state, and leave everything else to the driver which
knows best about these things.

But both are functionally same and definitely statistics update inside
cpuidle_idle_call() prevents duplication of code.

Thanks,
-Trinabh
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index 845d3ef..875d241 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -91,6 +91,11 @@  static void cpuidle_idle_call(void)
 
 	entered_state = target_state->enter(dev, drv, next_state);
 
+	/* Update cpuidle counters */
+	dev->states_usage[entered_state].time += 
+		(unsigned long long)dev->last_residency;
+	dev->states_usage[entered_state].usage++;
+
 	trace_power_end(dev->cpu);
 	trace_cpu_idle(PWR_EVENT_EXIT, dev->cpu);