Re: [PATCH 1/2] Dynamic Tick: Prevent clocksource wrapping duringidle
From: Jon Hunter
Date: Tue Aug 18 2009 - 16:42:39 EST
Thomas Gleixner wrote:
On Tue, 18 Aug 2009, Jon Hunter wrote:
From: Jon Hunter <jon-hunter@xxxxxx>
The dynamic tick allows the kernel to sleep for periods longer
than a single tick. This patch prevents that the kernel from
sleeping for a period longer than the maximum time that the
current clocksource can count. This ensures that the kernel will
not lose track of time. This patch adds a function called
"clocksource_max_deferment()" that calculates the maximum time the
kernel can sleep for a given clocksource and function called
"timekeeping_max_deferment()" that returns maximum time the kernel
can sleep for the current clocksource.
Signed-off-by: Jon Hunter <jon-hunter@xxxxxx>
---
include/linux/clocksource.h | 2 +
include/linux/time.h | 1 +
kernel/time/clocksource.c | 47 +++++++++++++++++++++++++++++++++++
kernel/time/tick-sched.c | 57 ++++++++++++++++++++++++++++++++----------
kernel/time/timekeeping.c | 11 ++++++++
5 files changed, 104 insertions(+), 14 deletions(-)
diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h
index 9ea40ff..09ed7f1 100644
--- a/include/linux/clocksource.h
+++ b/include/linux/clocksource.h
@@ -151,6 +151,7 @@ extern u64 timecounter_cyc2time(struct timecounter *tc,
* subtraction of non 64 bit counters
* @mult: cycle to nanosecond multiplier
* @shift: cycle to nanosecond divisor (power of two)
+ * @max_idle_ns: max idle time permitted by the clocksource (nsecs)
* @flags: flags describing special properties
* @vread: vsyscall based read
* @resume: resume function for the clocksource, if necessary
@@ -168,6 +169,7 @@ struct clocksource {
cycle_t mask;
u32 mult;
u32 shift;
+ s64 max_idle_ns;
I don't think we should move this to the clocksource. That should go
into the new struct timekeeper and initialized when a clocksource is
selected for timekeeping.
diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
index e0f59a2..7a98e90 100644
--- a/kernel/time/tick-sched.c
+++ b/kernel/time/tick-sched.c
@@ -217,6 +217,7 @@ void tick_nohz_stop_sched_tick(int inidle)
ktime_t last_update, expires, now;
struct clock_event_device *dev = __get_cpu_var(tick_cpu_device).evtdev;
int cpu;
+ s64 time_delta, max_time_delta;
local_irq_save(flags);
@@ -270,6 +271,18 @@ void tick_nohz_stop_sched_tick(int inidle)
seq = read_seqbegin(&xtime_lock);
last_update = last_jiffies_update;
last_jiffies = jiffies;
+
+ /*
+ * On SMP we really should only care for the CPU which
+ * has the do_timer duty assigned. All other CPUs can
+ * sleep as long as they want.
+ */
+ if (cpu == tick_do_timer_cpu ||
+ tick_do_timer_cpu == TICK_DO_TIMER_NONE)
+ max_time_delta = timekeeping_max_deferment();
+ else
+ max_time_delta = KTIME_MAX;
+
Is it worth the extra check instead of always using
timekeeping_max_deferment() ?
} while (read_seqretry(&xtime_lock, seq));
/* Get the next timer wheel timer */
@@ -289,11 +302,30 @@ void tick_nohz_stop_sched_tick(int inidle)
if ((long)delta_jiffies >= 1) {
/*
- * calculate the expiry time for the next timer wheel
- * timer
- */
- expires = ktime_add_ns(last_update, tick_period.tv64 *
- delta_jiffies);
+ * calculate the expiry time for the next timer wheel
+ * timer. delta_jiffies >= NEXT_TIMER_MAX_DELTA signals
+ * that there is no timer pending or at least extremely
+ * far into the future (12 days for HZ=1000). In this
+ * case we set the expiry to the end of time.
+ */
+ if (likely(delta_jiffies < NEXT_TIMER_MAX_DELTA)) {
+
+ /*
+ * Calculate the time delta for the next timer event.
+ * If the time delta exceeds the maximum time delta
+ * permitted by the current clocksource then adjust
+ * the time delta accordingly to ensure the
+ * clocksource does not wrap.
+ */
+ time_delta = tick_period.tv64 * delta_jiffies;
+
+ if (time_delta > max_time_delta)
+ time_delta = max_time_delta;
+
+ expires = ktime_add_ns(last_update, time_delta);
+ } else {
+ expires.tv64 = KTIME_MAX;
+ }
This looks incorrect. You set expires to KTIME_MAX when no timer is
pending, but that defeats the purpose of this patch. When we hit this
code path and the next interrupt comes in after the timekeeping
clocksource wrapped we are bust.
Right, so this is a bit of a grey area for me. When I first started
looking at this I was questioning the purpose of the following code that
exists today in the tick_nohz_stop_sched_tick() function:
/*
* delta_jiffies >= NEXT_TIMER_MAX_DELTA signals that
* there is no timer pending or at least extremly far
* into the future (12 days for HZ=1000). In this case
* we simply stop the tick timer:
*/
if (unlikely(delta_jiffies >= NEXT_TIMER_MAX_DELTA)) {
ts->idle_expires.tv64 = KTIME_MAX;
if (ts->nohz_mode == NOHZ_MODE_HIGHRES)
hrtimer_cancel(&ts->sched_timer);
goto out;
}
The above code checks to see delta_jiffies is greater than
NEXT_TIMER_MAX_DELTA, if so then sets expires to KTIME_MAX and disables
the timer. I had questioned this a few months ago, but I don't think
that John and I knew the history here. So for right or wrong, I left
this code alone. In the above patch it is still do the same thing if
delta_jiffies is indeed greater than NEXT_TIMER_MAX_DELTA.
If you agree that this code is not needed and that in the case where we
have no timers we should simply make the next timer event always occur
in max_time_delta ns later, then I can re-work it to do this.
Thanks
Jon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/