[1/3] cpuidle: Store the idle start time stamp

Message ID 1429092024-20498-1-git-send-email-daniel.lezcano@linaro.org
State New
Headers show

Commit Message

Daniel Lezcano April 15, 2015, 10 a.m.
The scheduler uses the idle timestamp stored in the struct rq to retrieve the
time when the cpu went idle in order to find the idlest cpu. Unfortunately
this information is wrong as it does not have the same meaning from the cpuidle
point of view. The idle_stamp in the struct rq gives the information when the
idle task has been scheduled while the idle task could be interrupted several
times and the cpu going through an idle/wakeup multiple times.

Add the idle start time in the idle state structure.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/cpuidle/cpuidle.c | 10 ++++++----
 include/linux/cpuidle.h   |  1 +
 2 files changed, 7 insertions(+), 4 deletions(-)

Comments

Daniel Lezcano April 15, 2015, 12:29 p.m. | #1
On 04/15/2015 12:20 PM, Peter Zijlstra wrote:
> On Wed, Apr 15, 2015 at 12:00:22PM +0200, Daniel Lezcano wrote:
>> +	target_state->idle_stamp = ktime_to_us(ktime_get());
>
> ktime_get_ns();
>

Hmm, sounds like I missed we are dealing with different units (us / ns) 
in cpuidle / sched.

Would it make sense to store the time into a ktime structure instead and 
use the relevant function depending on the place we are inspecting the 
value from ?
Daniel Lezcano April 15, 2015, 12:50 p.m. | #2
On 04/15/2015 02:42 PM, Peter Zijlstra wrote:
> On Wed, Apr 15, 2015 at 02:29:06PM +0200, Daniel Lezcano wrote:
>> On 04/15/2015 12:20 PM, Peter Zijlstra wrote:
>>> On Wed, Apr 15, 2015 at 12:00:22PM +0200, Daniel Lezcano wrote:
>>>> +	target_state->idle_stamp = ktime_to_us(ktime_get());
>>>
>>> ktime_get_ns();
>>>
>>
>> Hmm, sounds like I missed we are dealing with different units (us / ns) in
>> cpuidle / sched.
>>
>> Would it make sense to store the time into a ktime structure instead and use
>> the relevant function depending on the place we are inspecting the value
>> from ?
>
> Why would you ever want to use us? Does ARM enjoy calculating /1000?
>
> Ah, I see the !scalar ktime got whacked, good! At which point the u64 ns
> and ktime are the same. That said I prefer the u64 over ktime because
> its easier to manipulate.

Ok. I was trying to save one variable on the stack by reusing the 
idle_stamp value but if we store it in a different unit, then we have to 
keep it in order to have usecond for the governors.

I was thinking about converting to nanosecond the cpuidle framework but 
it is not worth to do that as the resolution is too high for the idle 
states.

Patch

diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index 080bd2d..1220dac 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -158,21 +158,23 @@  int cpuidle_enter_state(struct cpuidle_device *dev, struct cpuidle_driver *drv,
 	int entered_state;
 
 	struct cpuidle_state *target_state = &drv->states[index];
-	ktime_t time_start, time_end;
 	s64 diff;
 
 	trace_cpu_idle_rcuidle(index, dev->cpu);
-	time_start = ktime_get();
+
+	target_state->idle_stamp = ktime_to_us(ktime_get());
 
 	entered_state = target_state->enter(dev, drv, index);
 
-	time_end = ktime_get();
+	diff = ktime_to_us(ktime_sub_us(ktime_get(), target_state->idle_stamp));
+
+	target_state->idle_stamp = 0;
+
 	trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, dev->cpu);
 
 	if (!cpuidle_state_is_coupled(dev, drv, entered_state))
 		local_irq_enable();
 
-	diff = ktime_to_us(ktime_sub(time_end, time_start));
 	if (diff > INT_MAX)
 		diff = INT_MAX;
 
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index 306178d..2facce6 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -44,6 +44,7 @@  struct cpuidle_state {
 	int		power_usage; /* in mW */
 	unsigned int	target_residency; /* in US */
 	bool		disabled; /* disabled on all CPUs */
+	u64             idle_stamp;
 
 	int (*enter)	(struct cpuidle_device *dev,
 			struct cpuidle_driver *drv,