A few years ago, we changed the code in cpuidle to replace ktime_get()
by a local_clock() to get rid of potential seq lock in the path and an
extra latency.
Meanwhile, the code evolved and we are getting the time in some other
places like the power domain governor and in the future break even
deadline proposal.
Unfortunately, as the time must be compared across the CPU, we have no
other option than using the ktime_get() again. Hopefully, we can
factor out all the calls to local_clock() and ktime_get() into a
single one when the CPU is entering idle as the value will be reuse in
different places.
We can assume the time to go through the code path distance is small
enough between ktime_get() call in the cpuidle_enter() function and
the other users inspecting the value.
Signed-off-by: Daniel Lezcano <[email protected]>
---
drivers/base/power/domain_governor.c | 4 +++-
drivers/cpuidle/cpuidle.c | 6 +++---
include/linux/cpuidle.h | 1 +
3 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/base/power/domain_governor.c b/drivers/base/power/domain_governor.c
index daa8c7689f7e..bee97f7b7b8d 100644
--- a/drivers/base/power/domain_governor.c
+++ b/drivers/base/power/domain_governor.c
@@ -279,8 +279,10 @@ static bool cpu_power_down_ok(struct dev_pm_domain *pd)
}
}
+ dev = per_cpu(cpuidle_devices, smp_processor_id());
+
/* The minimum idle duration is from now - until the next wakeup. */
- idle_duration_ns = ktime_to_ns(ktime_sub(domain_wakeup, ktime_get()));
+ idle_duration_ns = ktime_to_ns(ktime_sub(domain_wakeup, dev->idle_start));
if (idle_duration_ns <= 0)
return false;
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index c149d9e20dfd..9db14581759b 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -206,7 +206,7 @@ int cpuidle_enter_state(struct cpuidle_device *dev, struct cpuidle_driver *drv,
struct cpuidle_state *target_state = &drv->states[index];
bool broadcast = !!(target_state->flags & CPUIDLE_FLAG_TIMER_STOP);
- ktime_t time_start, time_end;
+ ktime_t time_end;
/*
* Tell the time framework to switch to a broadcast timer because our
@@ -228,14 +228,14 @@ int cpuidle_enter_state(struct cpuidle_device *dev, struct cpuidle_driver *drv,
sched_idle_set_state(target_state);
trace_cpu_idle_rcuidle(index, dev->cpu);
- time_start = ns_to_ktime(local_clock());
+ dev->idle_start = ktime_get();
stop_critical_timings();
entered_state = target_state->enter(dev, drv, index);
start_critical_timings();
sched_clock_idle_wakeup_event();
- time_end = ns_to_ktime(local_clock());
+ time_end = ktime_get();
trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, dev->cpu);
/* The cpu is no longer idle or about to enter idle. */
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index ec2ef63771f0..112494658e01 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -89,6 +89,7 @@ struct cpuidle_device {
unsigned int poll_time_limit:1;
unsigned int cpu;
ktime_t next_hrtimer;
+ ktime_t idle_start;
int last_state_idx;
u64 last_residency_ns;
--
2.17.1
On Monday, March 16, 2020 10:08:43 PM CET Daniel Lezcano wrote:
> A few years ago, we changed the code in cpuidle to replace ktime_get()
> by a local_clock() to get rid of potential seq lock in the path and an
> extra latency.
>
> Meanwhile, the code evolved and we are getting the time in some other
> places like the power domain governor and in the future break even
> deadline proposal.
Hmm?
Have any patches been posted for that?
> Unfortunately, as the time must be compared across the CPU, we have no
> other option than using the ktime_get() again. Hopefully, we can
> factor out all the calls to local_clock() and ktime_get() into a
> single one when the CPU is entering idle as the value will be reuse in
> different places.
So there are cases in which it is not necessary to synchronize the time
between CPUs and those would take the overhead unnecessarily.
This change looks premature to me at least.
Thanks!
On Mon, 16 Mar 2020 at 22:10, Daniel Lezcano <[email protected]> wrote:
>
> A few years ago, we changed the code in cpuidle to replace ktime_get()
> by a local_clock() to get rid of potential seq lock in the path and an
> extra latency.
>
> Meanwhile, the code evolved and we are getting the time in some other
> places like the power domain governor and in the future break even
> deadline proposal.
>
> Unfortunately, as the time must be compared across the CPU, we have no
> other option than using the ktime_get() again. Hopefully, we can
> factor out all the calls to local_clock() and ktime_get() into a
> single one when the CPU is entering idle as the value will be reuse in
> different places.
>
> We can assume the time to go through the code path distance is small
> enough between ktime_get() call in the cpuidle_enter() function and
> the other users inspecting the value.
>
> Signed-off-by: Daniel Lezcano <[email protected]>
> ---
> drivers/base/power/domain_governor.c | 4 +++-
> drivers/cpuidle/cpuidle.c | 6 +++---
> include/linux/cpuidle.h | 1 +
> 3 files changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/base/power/domain_governor.c b/drivers/base/power/domain_governor.c
> index daa8c7689f7e..bee97f7b7b8d 100644
> --- a/drivers/base/power/domain_governor.c
> +++ b/drivers/base/power/domain_governor.c
> @@ -279,8 +279,10 @@ static bool cpu_power_down_ok(struct dev_pm_domain *pd)
> }
> }
>
> + dev = per_cpu(cpuidle_devices, smp_processor_id());
> +
> /* The minimum idle duration is from now - until the next wakeup. */
> - idle_duration_ns = ktime_to_ns(ktime_sub(domain_wakeup, ktime_get()));
> + idle_duration_ns = ktime_to_ns(ktime_sub(domain_wakeup, dev->idle_start));
> if (idle_duration_ns <= 0)
> return false;
>
> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
> index c149d9e20dfd..9db14581759b 100644
> --- a/drivers/cpuidle/cpuidle.c
> +++ b/drivers/cpuidle/cpuidle.c
> @@ -206,7 +206,7 @@ int cpuidle_enter_state(struct cpuidle_device *dev, struct cpuidle_driver *drv,
>
> struct cpuidle_state *target_state = &drv->states[index];
> bool broadcast = !!(target_state->flags & CPUIDLE_FLAG_TIMER_STOP);
> - ktime_t time_start, time_end;
> + ktime_t time_end;
>
> /*
> * Tell the time framework to switch to a broadcast timer because our
> @@ -228,14 +228,14 @@ int cpuidle_enter_state(struct cpuidle_device *dev, struct cpuidle_driver *drv,
> sched_idle_set_state(target_state);
>
> trace_cpu_idle_rcuidle(index, dev->cpu);
> - time_start = ns_to_ktime(local_clock());
> + dev->idle_start = ktime_get();
I fully agree with Rafael, this is bad for all cases where the
local_clock is sufficient.
To avoid the ktime_get() in the cpu_power_down_ok() for the genpd
governor, I think a better option could be to use the
"ts->idle_entrytime", that has been set in tick_nohz_start_idle().
>
> stop_critical_timings();
> entered_state = target_state->enter(dev, drv, index);
> start_critical_timings();
>
> sched_clock_idle_wakeup_event();
> - time_end = ns_to_ktime(local_clock());
> + time_end = ktime_get();
> trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, dev->cpu);
>
> /* The cpu is no longer idle or about to enter idle. */
> diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
> index ec2ef63771f0..112494658e01 100644
> --- a/include/linux/cpuidle.h
> +++ b/include/linux/cpuidle.h
> @@ -89,6 +89,7 @@ struct cpuidle_device {
> unsigned int poll_time_limit:1;
> unsigned int cpu;
> ktime_t next_hrtimer;
> + ktime_t idle_start;
>
> int last_state_idx;
> u64 last_residency_ns;
> --
> 2.17.1
>
Kind regards
Uffe
Hi Rafael,
On 18/03/2020 11:17, Rafael J. Wysocki wrote:
> On Monday, March 16, 2020 10:08:43 PM CET Daniel Lezcano wrote:
>> A few years ago, we changed the code in cpuidle to replace ktime_get()
>> by a local_clock() to get rid of potential seq lock in the path and an
>> extra latency.
>>
>> Meanwhile, the code evolved and we are getting the time in some other
>> places like the power domain governor and in the future break even
>> deadline proposal.
>
> Hmm?
>
> Have any patches been posted for that?
https://lkml.org/lkml/2020/3/11/1113
https://lkml.org/lkml/2020/3/13/466
but there is no consensus yet if that has a benefit or not.
>> Unfortunately, as the time must be compared across the CPU, we have no
>> other option than using the ktime_get() again. Hopefully, we can
>> factor out all the calls to local_clock() and ktime_get() into a
>> single one when the CPU is entering idle as the value will be reuse in
>> different places.
>
> So there are cases in which it is not necessary to synchronize the time
> between CPUs and those would take the overhead unnecessarily.
>
> This change looks premature to me at least.
The idea is to call one time ktime_get() when entering idle and store
the result in the struct cpuidle_device, so we have the information when
we entered idle.
Moreover, ktime_get() is called in do_idle() via:
tick_nohz_idle_enter()
tick_nohz_start_idle()
ts->idle_entrytime = ktime_get();
This is called at the first loop level. The idle loop is exiting and
re-entering again without passing through tick_nohz_idle_enter() in the
second loop level in case of interrupt processing, thus the
idle_entrytime is not updated and the return of
tick_nohz_get_sleep_length() will be greater than what is expected.
May be we can consider ktime_get_mono_fast_ns() which is lockless with a
particular care of the non-monotonic aspect if needed. Given the
description at [1] the time jump could a few nanoseconds in case of NMI.
The local_clock() can no be inspected across CPUs, the gap is too big
and continues to increase during system lifetime.
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/time/timekeeping.c#n396
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
On 18/03/2020 12:04, Daniel Lezcano wrote:
>
> Hi Rafael,
>
> On 18/03/2020 11:17, Rafael J. Wysocki wrote:
>> On Monday, March 16, 2020 10:08:43 PM CET Daniel Lezcano wrote:
>>> A few years ago, we changed the code in cpuidle to replace ktime_get()
>>> by a local_clock() to get rid of potential seq lock in the path and an
>>> extra latency.
>>>
>>> Meanwhile, the code evolved and we are getting the time in some other
>>> places like the power domain governor and in the future break even
>>> deadline proposal.
>>
>> Hmm?
>>
>> Have any patches been posted for that?
>
> https://lkml.org/lkml/2020/3/11/1113
>
> https://lkml.org/lkml/2020/3/13/466
>
> but there is no consensus yet if that has a benefit or not.
>
>>> Unfortunately, as the time must be compared across the CPU, we have no
>>> other option than using the ktime_get() again. Hopefully, we can
>>> factor out all the calls to local_clock() and ktime_get() into a
>>> single one when the CPU is entering idle as the value will be reuse in
>>> different places.
>>
>> So there are cases in which it is not necessary to synchronize the time
>> between CPUs and those would take the overhead unnecessarily.
>>
>> This change looks premature to me at least.
>
> The idea is to call one time ktime_get() when entering idle and store
> the result in the struct cpuidle_device, so we have the information when
> we entered idle.
>
> Moreover, ktime_get() is called in do_idle() via:
>
> tick_nohz_idle_enter()
> tick_nohz_start_idle()
> ts->idle_entrytime = ktime_get();
>
> This is called at the first loop level. The idle loop is exiting and
> re-entering again without passing through tick_nohz_idle_enter() in the
> second loop level in case of interrupt processing, thus the
> idle_entrytime is not updated and the return of
> tick_nohz_get_sleep_length() will be greater than what is expected.
>
> May be we can consider ktime_get_mono_fast_ns() which is lockless with a
> particular care of the non-monotonic aspect if needed. Given the
> description at [1] the time jump could a few nanoseconds in case of NMI.
>
> The local_clock() can no be inspected across CPUs, the gap is too big
> and continues to increase during system lifetime.
I took the opportunity to measure the duration to a call to ktime_get,
ktime_get_mono_fast_ns and local_clock.
The result is an average of 10000 measurements and an average of 1000
run of those.
The duration is measured with local_clock(), ktime_get() and
ktime_get_mono_fast_ns()
Measurement with local_clock():
-------------------------------
ktime_get():
N min max sum mean stddev
1000 71 168 109052 109.052 13.0278
ktime_get_mono_fast_ns():
N min max sum mean stddev
1000 66 153 101135 101.135 11.9262
local_clock():
N min max sum mean stddev
1000 70 163 106896 106.896 12.8575
Measurement with ktime_get():
-----------------------------
ktime_get():
N min max sum mean stddev
1000 71 124 100465 100.465 10.0272
ktime_get_mono_fast_ns():
N min max sum mean stddev
1000 67 124 94451 94.451 9.67218
local_clock():
N min max sum mean stddev
1000 71 123 99765 99.765 10.0508
Measurement with ktime_get_mono_fast_ns():
------------------------------------------
ktime_get():
N min max sum mean stddev
1000 67 116 87562 87.562 4.38399
ktime_get_mono_fast_ns():
N min max sum mean stddev
1000 62 104 81017 81.017 4.12453
local_clock():
N min max sum mean stddev
1000 65 110 85919 85.919 4.24859
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
On Wed, Mar 18, 2020 at 3:32 PM Daniel Lezcano
<[email protected]> wrote:
>
> On 18/03/2020 12:04, Daniel Lezcano wrote:
> >
> > Hi Rafael,
> >
> > On 18/03/2020 11:17, Rafael J. Wysocki wrote:
> >> On Monday, March 16, 2020 10:08:43 PM CET Daniel Lezcano wrote:
> >>> A few years ago, we changed the code in cpuidle to replace ktime_get()
> >>> by a local_clock() to get rid of potential seq lock in the path and an
> >>> extra latency.
> >>>
> >>> Meanwhile, the code evolved and we are getting the time in some other
> >>> places like the power domain governor and in the future break even
> >>> deadline proposal.
> >>
> >> Hmm?
> >>
> >> Have any patches been posted for that?
> >
> > https://lkml.org/lkml/2020/3/11/1113
> >
> > https://lkml.org/lkml/2020/3/13/466
> >
> > but there is no consensus yet if that has a benefit or not.
> >
> >>> Unfortunately, as the time must be compared across the CPU, we have no
> >>> other option than using the ktime_get() again. Hopefully, we can
> >>> factor out all the calls to local_clock() and ktime_get() into a
> >>> single one when the CPU is entering idle as the value will be reuse in
> >>> different places.
> >>
> >> So there are cases in which it is not necessary to synchronize the time
> >> between CPUs and those would take the overhead unnecessarily.
> >>
> >> This change looks premature to me at least.
> >
> > The idea is to call one time ktime_get() when entering idle and store
> > the result in the struct cpuidle_device, so we have the information when
> > we entered idle.
> >
> > Moreover, ktime_get() is called in do_idle() via:
> >
> > tick_nohz_idle_enter()
> > tick_nohz_start_idle()
> > ts->idle_entrytime = ktime_get();
> >
> > This is called at the first loop level. The idle loop is exiting and
> > re-entering again without passing through tick_nohz_idle_enter() in the
> > second loop level in case of interrupt processing, thus the
> > idle_entrytime is not updated and the return of
> > tick_nohz_get_sleep_length() will be greater than what is expected.
> >
> > May be we can consider ktime_get_mono_fast_ns() which is lockless with a
> > particular care of the non-monotonic aspect if needed. Given the
> > description at [1] the time jump could a few nanoseconds in case of NMI.
> >
> > The local_clock() can no be inspected across CPUs, the gap is too big
> > and continues to increase during system lifetime.
>
> I took the opportunity to measure the duration to a call to ktime_get,
> ktime_get_mono_fast_ns and local_clock.
The results you get depend a good deal on the conditions of the test,
the system on which they were obtained and so on. Without this
information it is hard to draw any conclusions from those results. In
particular, ktime_get() is not significantly slower than local_clock()
if there is no contention AFAICS, and the lack of contention cannot be
guaranteed here.
Generally speaking, the problem is that it is not sufficient to
measure the time before running the governor and after the CPU wakes
up, because in the cases that really care about the latency of that
operation the time needed to run the governor may be a significant
fraction of the entire overhead. So it is necessary to take time
stamps in several places and putting ktime_get() in all of them
doesn't sound particularly attractive.
Anyway, there is no real need to make this change AFAICS, so I'm not
really sure what the entire argument is about.