| Message ID | 1429092024-20498-1-git-send-email-daniel.lezcano@linaro.org |
|---|---|
| State | New |
| Headers | show |
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 ?
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.
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,
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(-)