Re: [RFC v3 1/3] kernel/time/clockevents: initial support for mono to raw time conversion

[John Stultz <john.stultz@linaro.org>]

On Wed, Jul 13, 2016 at 6:00 AM, Nicolai Stange <nicstange@gmail.com> wrote:
> With NOHZ_FULL and one single well-isolated, CPU consumptive task, one
> would expect approximately one clockevent interrupt per second. However, on
> my Intel Haswell where the monotonic clock is the TSC monotonic clock and
> the clockevent device is the TSC deadline device, it turns out that every
> second, there are two such interrupts: the first one arrives always
> approximately ~50us before the scheduled deadline as programmed by
> tick_nohz_stop_sched_tick() through the hrtimer API. The
> __hrtimer_run_queues() called in this interrupt detects that the queued
> tick_sched_timer hasn't expired yet and simply does nothing except
> reprogramming the clock event device to fire shortly after again.
>
> These too early programmed deadlines are explained as follows:
> clockevents_program_event() programs the clockevent device to fire
> after
>   f_event * delta_t_progr
> clockevent device cycles where f_event is the clockevent device's hardware
> frequency and delta_t_progr is the requested time interval. After that many
> clockevent device cycles have elapsed, the device underlying the monotonic
> clock, that is the monotonic raw clock has seen f_raw / f_event as many
> cycles.
> The ktime_get() called from __hrtimer_run_queues() interprets those
> cycles to run at the frequency of the monotonic clock. Summarizing:
>   delta_t_perc = 1/f_mono * f_raw/f_event * f_event * delta_t_progr
>                = f_raw / f_mono * delta_t_progr
> with f_mono being the monotonic clock's frequency and delta_t_perc being
> the elapsed time interval as perceived by __hrtimer_run_queues().
>
> Now, f_mono is not a constant, but is dynamically adjusted in
> timekeeping_adjust() in order to compensate for the NTP error. With the
> large values of delta_t_progr of 10^9ns with NOHZ_FULL, the error made
> becomes significant and results in the double timer interrupts described
> above.
>
> Compensate for this error by multiplying the clockevent device's f_event
> by f_mono/f_raw.
>
> Namely:
> - Introduce a ->mult_mono member to the struct clock_event_device. It's
>   value is supposed to be equal to ->mult * f_mono/f_raw.
> - Introduce the timekeeping_get_mono_mult() helper which provides
>   the clockevent core with access to the timekeeping's current f_mono
>   and f_raw.
> - Introduce the helper __clockevents_adjust_freq() which
>   sets a clockevent device's ->mult_mono member as appropriate. It is
>   implemented with the help of the new __clockevents_calc_adjust_freq().
> - Call __clockevents_adjust_freq() at clockevent device registration time
>   as well as at frequency updates through clockevents_update_freq().
> - Finally, use the ->mult_mono rather than ->mult in the ns to cycle
>   conversion made in clockevents_program_event().
>
> Note that future adjustments of the monotonic clock are not taken into
> account yet. Furthemore, this patch assumes that after a clockevent
> device's registration, its ->mult changes only through calls to
> clockevents_update_freq().

Sorry for being a little slow to review here. Been swamped.

I was about to queue this but had a few nits that need addressing.


> Signed-off-by: Nicolai Stange <nicstange@gmail.com>
> ---
>  include/linux/clockchips.h  |  1 +
>  kernel/time/clockevents.c   | 49 ++++++++++++++++++++++++++++++++++++++++++++-
>  kernel/time/tick-internal.h |  1 +
>  kernel/time/timekeeping.c   |  8 ++++++++
>  4 files changed, 58 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/clockchips.h b/include/linux/clockchips.h
> index 0d442e3..2863742 100644
> --- a/include/linux/clockchips.h
> +++ b/include/linux/clockchips.h
> @@ -104,6 +104,7 @@ struct clock_event_device {
>         u64                     max_delta_ns;
>         u64                     min_delta_ns;
>         u32                     mult;
> +       u32                     mult_mono;

So in this context(for me at least), mult and mult_mono are a bit
confusing.  I tend to think of it as mult and mult_raw, but in this
case mult is the "raw" unmodified value and mult_mono is the adjusted
one.

I'd probably suggest mult_adjusted or some other name to make it more
clear how it differs from the clockevent mult.

>
> +void timekeeping_get_mono_mult(u32 *mult_cs_mono, u32 *mult_cs_raw)
> +{
> +       struct tk_read_base *tkr_mono = &tk_core.timekeeper.tkr_mono;
> +
> +       *mult_cs_mono = tkr_mono->mult;
> +       *mult_cs_raw = tkr_mono->clock->mult;
> +}

So.. you probably should have some locking here. Or at least a big
comment making it clear why locking isn't necessary.

thanks
-john