Messages in this thread | ![/](/images/icornerl.gif) | | Date | Tue, 18 Aug 2009 15:42:09 -0500 | From | Jon Hunter <> | Subject | Re: [PATCH 1/2] Dynamic Tick: Prevent clocksource wrapping during idle |
| |
Thomas Gleixner wrote: > On Tue, 18 Aug 2009, Jon Hunter wrote: > >> From: Jon Hunter <jon-hunter@ti.com> >> >> 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@ti.com> >> --- >> 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
| ![\](/images/icornerr.gif) |