Re: [RFC][PATCH 4/4] time: Clean up CLOCK_MONOTONIC_RAW time handling
From: Chris Wilson
Date: Fri Aug 25 2017 - 09:40:59 EST
Quoting John Stultz (2017-05-27 04:33:55)
> Now that we fixed the sub-ns handling for CLOCK_MONOTONIC_RAW,
> remove the duplicitive tk->raw_time.tv_nsec, which can be
> stored in tk->tkr_raw.xtime_nsec (similarly to how its handled
> for monotonic time).
This patch breaks tkr_raw pretty badly. It is now accumulating 2x faster
than tkr_mono, with the occasional wacky jump between two reads.
e.g:
@@ -838,6 +840,11 @@ ktime_t ktime_get_raw(void)
} while (read_seqcount_retry(&tk_core.seq, seq));
+ pr_err("%s: raw.base=%llu, mono.base=%llu: nsecs=%llu, mono.nsecs=%llu\n",
+ __func__,
+ ktime_to_ns(base), ktime_to_ns(tk->tkr_mono.base),
+ nsecs, timekeeping_get_ns(&tk->tkr_mono));
+
gave
ktime_get_raw: raw.base=40255680121, mono.base=39940532364: nsecs=261321742, mono.nsecs=318630514
ktime_get_raw: raw.base=40339680124, mono.base=39940532364: nsecs=345836800, mono.nsecs=403109209
https://bugs.freedesktop.org/show_bug.cgi?id=102336
The patch still reverts cleanly and aiui was not part of the bugfixes,
but a cleanup afterwards; so can be reapplied at leisure.
> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> Cc: Miroslav Lichvar <mlichvar@xxxxxxxxxx>
> Cc: Richard Cochran <richardcochran@xxxxxxxxx>
> Cc: Prarit Bhargava <prarit@xxxxxxxxxx>
> Cc: Stephen Boyd <stephen.boyd@xxxxxxxxxx>
> Cc: Kevin Brodsky <kevin.brodsky@xxxxxxx>
> Cc: Will Deacon <will.deacon@xxxxxxx>
> Cc: Daniel Mentz <danielmentz@xxxxxxxxxx>
> Signed-off-by: John Stultz <john.stultz@xxxxxxxxxx>
> ---
> arch/arm64/kernel/vdso.c | 6 ++---
> include/linux/timekeeper_internal.h | 4 ++--
> kernel/time/timekeeping.c | 47 ++++++++++++++++++++-----------------
> 3 files changed, 29 insertions(+), 28 deletions(-)
>
> diff --git a/arch/arm64/kernel/vdso.c b/arch/arm64/kernel/vdso.c
> index d0cb007..7492d90 100644
> --- a/arch/arm64/kernel/vdso.c
> +++ b/arch/arm64/kernel/vdso.c
> @@ -220,10 +220,8 @@ void update_vsyscall(struct timekeeper *tk)
> if (!use_syscall) {
> /* tkr_mono.cycle_last == tkr_raw.cycle_last */
> vdso_data->cs_cycle_last = tk->tkr_mono.cycle_last;
> - vdso_data->raw_time_sec = tk->raw_time.tv_sec;
> - vdso_data->raw_time_nsec = (tk->raw_time.tv_nsec <<
> - tk->tkr_raw.shift) +
> - tk->tkr_raw.xtime_nsec;
> + vdso_data->raw_time_sec = tk->raw_sec;
> + vdso_data->raw_time_nsec = tk->tkr_raw.xtime_nsec;
> vdso_data->xtime_clock_sec = tk->xtime_sec;
> vdso_data->xtime_clock_nsec = tk->tkr_mono.xtime_nsec;
> vdso_data->cs_mono_mult = tk->tkr_mono.mult;
> diff --git a/include/linux/timekeeper_internal.h b/include/linux/timekeeper_internal.h
> index 528cc86..8abf6df 100644
> --- a/include/linux/timekeeper_internal.h
> +++ b/include/linux/timekeeper_internal.h
> @@ -52,7 +52,7 @@ struct tk_read_base {
> * @clock_was_set_seq: The sequence number of clock was set events
> * @cs_was_changed_seq: The sequence number of clocksource change events
> * @next_leap_ktime: CLOCK_MONOTONIC time value of a pending leap-second
> - * @raw_time: Monotonic raw base time in timespec64 format
> + * @raw_sec: CLOCK_MONOTONIC_RAW time in seconds
> * @cycle_interval: Number of clock cycles in one NTP interval
> * @xtime_interval: Number of clock shifted nano seconds in one NTP
> * interval.
> @@ -94,7 +94,7 @@ struct timekeeper {
> unsigned int clock_was_set_seq;
> u8 cs_was_changed_seq;
> ktime_t next_leap_ktime;
> - struct timespec64 raw_time;
> + u64 raw_sec;
>
> /* The following members are for timekeeping internal use */
> u64 cycle_interval;
> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> index 35d3ba3..0c3b8c1 100644
> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -72,6 +72,10 @@ static inline void tk_normalize_xtime(struct timekeeper *tk)
> tk->tkr_mono.xtime_nsec -= (u64)NSEC_PER_SEC << tk->tkr_mono.shift;
> tk->xtime_sec++;
> }
> + while (tk->tkr_raw.xtime_nsec >= ((u64)NSEC_PER_SEC << tk->tkr_raw.shift)) {
> + tk->tkr_raw.xtime_nsec -= (u64)NSEC_PER_SEC << tk->tkr_raw.shift;
> + tk->raw_sec++;
> + }
> }
>
> static inline struct timespec64 tk_xtime(struct timekeeper *tk)
> @@ -287,12 +291,14 @@ static void tk_setup_internals(struct timekeeper *tk, struct clocksource *clock)
> /* if changing clocks, convert xtime_nsec shift units */
> if (old_clock) {
> int shift_change = clock->shift - old_clock->shift;
> - if (shift_change < 0)
> + if (shift_change < 0) {
> tk->tkr_mono.xtime_nsec >>= -shift_change;
> - else
> + tk->tkr_raw.xtime_nsec >>= -shift_change;
> + } else {
> tk->tkr_mono.xtime_nsec <<= shift_change;
> + tk->tkr_raw.xtime_nsec <<= shift_change;
> + }
> }
> - tk->tkr_raw.xtime_nsec = 0;
>
> tk->tkr_mono.shift = clock->shift;
> tk->tkr_raw.shift = clock->shift;
> @@ -617,9 +623,6 @@ static inline void tk_update_ktime_data(struct timekeeper *tk)
> nsec = (u32) tk->wall_to_monotonic.tv_nsec;
> tk->tkr_mono.base = ns_to_ktime(seconds * NSEC_PER_SEC + nsec);
>
> - /* Update the monotonic raw base */
> - tk->tkr_raw.base = timespec64_to_ktime(tk->raw_time);
> -
> /*
> * The sum of the nanoseconds portions of xtime and
> * wall_to_monotonic can be greater/equal one second. Take
> @@ -629,6 +632,11 @@ static inline void tk_update_ktime_data(struct timekeeper *tk)
> if (nsec >= NSEC_PER_SEC)
> seconds++;
> tk->ktime_sec = seconds;
> +
> + /* Update the monotonic raw base */
> + seconds = tk->raw_sec;
> + nsec = (u32)(tk->tkr_raw.xtime_nsec >> tk->tkr_raw.shift);
> + tk->tkr_raw.base = ns_to_ktime(seconds * NSEC_PER_SEC + nsec);
> }
>
> /* must hold timekeeper_lock */
> @@ -670,7 +678,6 @@ static void timekeeping_update(struct timekeeper *tk, unsigned int action)
> static void timekeeping_forward_now(struct timekeeper *tk)
> {
> u64 cycle_now, delta;
> - u64 nsec;
>
> cycle_now = tk_clock_read(&tk->tkr_mono);
> delta = clocksource_delta(cycle_now, tk->tkr_mono.cycle_last, tk->tkr_mono.mask);
> @@ -682,10 +689,13 @@ static void timekeeping_forward_now(struct timekeeper *tk)
> /* If arch requires, add in get_arch_timeoffset() */
> tk->tkr_mono.xtime_nsec += (u64)arch_gettimeoffset() << tk->tkr_mono.shift;
>
> - tk_normalize_xtime(tk);
>
> - nsec = clocksource_cyc2ns(delta, tk->tkr_raw.mult, tk->tkr_raw.shift);
> - timespec64_add_ns(&tk->raw_time, nsec);
> + tk->tkr_raw.xtime_nsec += delta * tk->tkr_raw.mult;
> +
> + /* If arch requires, add in get_arch_timeoffset() */
> + tk->tkr_raw.xtime_nsec += (u64)arch_gettimeoffset() << tk->tkr_raw.shift;
> +
> + tk_normalize_xtime(tk);
> }
>
> /**
> @@ -1371,19 +1381,18 @@ int timekeeping_notify(struct clocksource *clock)
> void getrawmonotonic64(struct timespec64 *ts)
> {
> struct timekeeper *tk = &tk_core.timekeeper;
> - struct timespec64 ts64;
> unsigned long seq;
> u64 nsecs;
>
> do {
> seq = read_seqcount_begin(&tk_core.seq);
> + ts->tv_sec = tk->raw_sec;
> nsecs = timekeeping_get_ns(&tk->tkr_raw);
> - ts64 = tk->raw_time;
>
> } while (read_seqcount_retry(&tk_core.seq, seq));
>
> - timespec64_add_ns(&ts64, nsecs);
> - *ts = ts64;
> + ts->tv_nsec = 0;
> + timespec64_add_ns(ts, nsecs);
> }
> EXPORT_SYMBOL(getrawmonotonic64);
>
> @@ -1507,8 +1516,7 @@ void __init timekeeping_init(void)
> tk_setup_internals(tk, clock);
>
> tk_set_xtime(tk, &now);
> - tk->raw_time.tv_sec = 0;
> - tk->raw_time.tv_nsec = 0;
> + tk->raw_sec = 0;
> if (boot.tv_sec == 0 && boot.tv_nsec == 0)
> boot = tk_xtime(tk);
>
> @@ -2009,17 +2017,12 @@ static u64 logarithmic_accumulation(struct timekeeper *tk, u64 offset,
> *clock_set |= accumulate_nsecs_to_secs(tk);
>
> /* Accumulate raw time */
> - tk->tkr_raw.xtime_nsec += (u64)tk->raw_time.tv_nsec
> - << tk->tkr_raw.shift;
> tk->tkr_raw.xtime_nsec += tk->raw_interval << shift;
> nsecps = (u64)NSEC_PER_SEC << tk->tkr_raw.shift;
> while (tk->tkr_raw.xtime_nsec >= nsecps) {
> tk->tkr_raw.xtime_nsec -= nsecps;
> - tk->raw_time.tv_sec++;
> + tk->raw_sec++;
> }
> - tk->raw_time.tv_nsec = tk->tkr_raw.xtime_nsec >> tk->tkr_raw.shift;
> - tk->tkr_raw.xtime_nsec -= (u64)tk->raw_time.tv_nsec
> - << tk->tkr_raw.shift;
>
> /* Accumulate error between NTP and clock interval */
> tk->ntp_error += tk->ntp_tick << shift;
> --
> 2.7.4
>