Messages in this thread | | | Date | Wed, 6 Jan 2016 11:37:23 -0800 | Subject | Re: [RFC v5 3/6] Add history to cross timestamp interface supporting slower devices | From | John Stultz <> |
| |
On Mon, Jan 4, 2016 at 4:45 AM, Christopher S. Hall <christopher.s.hall@intel.com> wrote: > @@ -13,6 +13,9 @@ > /** > * struct tk_read_base - base structure for timekeeping readout > * @clock: Current clocksource used for timekeeping. > + * @cs_seq: Clocksource sequence is incremented per clocksource change. > + * It's used to determine whether past system time can be related to > + * current system time > * @read: Read function of @clock > * @mask: Bitmask for two's complement subtraction of non 64bit clocks > * @cycle_last: @clock cycle value at last update > @@ -29,6 +32,7 @@ > */ > struct tk_read_base { > struct clocksource *clock; > + u8 cs_seq; > cycle_t (*read)(struct clocksource *cs); > cycle_t mask; > cycle_t cycle_last;
So Thomas optimized the tk_read_bases to fit in a cacheline, and so I worry about the u8 being added there. I'm also cautious about exporting these seq values out further via the system_time_snapshot. But I may just need some more time to warm up to the idea.
> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c > index 9c1ddc3..5a7f784 100644 > --- a/kernel/time/timekeeping.c > +++ b/kernel/time/timekeeping.c > @@ -235,11 +235,13 @@ static void tk_setup_internals(struct timekeeper *tk, struct clocksource *clock) > > old_clock = tk->tkr_mono.clock; > tk->tkr_mono.clock = clock; > + ++tk->tkr_mono.cs_seq; > tk->tkr_mono.read = clock->read; > tk->tkr_mono.mask = clock->mask; > tk->tkr_mono.cycle_last = tk->tkr_mono.read(clock); > > tk->tkr_raw.clock = clock; > + ++tk->tkr_raw.cs_seq; > tk->tkr_raw.read = clock->read; > tk->tkr_raw.mask = clock->mask; > tk->tkr_raw.cycle_last = tk->tkr_mono.cycle_last; > @@ -862,6 +864,39 @@ time64_t ktime_get_real_seconds(void) > } > EXPORT_SYMBOL_GPL(ktime_get_real_seconds); > > +/** > + * ktime_get_snapshot - snapshots the realtime/monotonic raw clocks with counter > + * @snapshot: pointer to struct receiving the system time snapshot > + */ > +void ktime_get_snapshot(struct system_time_snapshot *systime_snapshot) > +{ > + struct timekeeper *tk = &tk_core.timekeeper; > + unsigned long seq; > + ktime_t base_raw; > + ktime_t base_real; > + s64 nsec_raw; > + s64 nsec_real; > + cycle_t now; > + > + do { > + seq = read_seqcount_begin(&tk_core.seq); > + > + now = tk->tkr_mono.read(tk->tkr_mono.clock); > + systime_snapshot->cs_seq = tk->tkr_mono.cs_seq; > + systime_snapshot->clock_set_seq = tk->clock_was_set_seq; > + base_real = ktime_add(tk->tkr_mono.base, > + tk_core.timekeeper.offs_real); > + base_raw = tk->tkr_raw.base; > + nsec_real = timekeeping_cycles_to_ns(&tk->tkr_mono, now); > + nsec_raw = timekeeping_cycles_to_ns(&tk->tkr_raw, now); > + } while (read_seqcount_retry(&tk_core.seq, seq)); > + > + systime_snapshot->cycles = now; > + systime_snapshot->real = ktime_add_ns(base_real, nsec_real); > + systime_snapshot->raw = ktime_add_ns(base_raw, nsec_raw); > +} > +EXPORT_SYMBOL_GPL(ktime_get_snapshot);
So can you split out this adding of ktime_get_snapshot() (maybe skipping the seqcount bits initially) into a separate patch?
> @@ -936,19 +1044,63 @@ int get_device_system_crosststamp(struct system_device_crosststamp *xtstamp, > */ > if (tk->tkr_mono.clock != raw_sys.cs) > return -ENODEV; > + cycles = raw_sys.cycles; > + > + /* > + * Check whether the system counter value provided by the > + * device driver is on the current interval. > + */ > + now = tk->tkr_mono.read(tk->tkr_mono.clock); > + interval_start = tk->tkr_mono.cycle_last; > + if (!cycle_between(interval_start, cycles, now)) { > + cs_seq = tk->tkr_mono.cs_seq; > + clock_was_set_seq = tk->clock_was_set_seq; > + cycles = interval_start; > + do_interp = true; > + } else { > + do_interp = false; > + } > > base_real = ktime_add(tk->tkr_mono.base, > tk_core.timekeeper.offs_real); > base_raw = tk->tkr_raw.base; > > - nsec_real = timekeeping_cycles_to_ns(&tk->tkr_mono, > - raw_sys.cycles); > - nsec_raw = timekeeping_cycles_to_ns(&tk->tkr_raw, > - raw_sys.cycles); > + nsec_real = timekeeping_cycles_to_ns(&tk->tkr_mono, cycles); > + nsec_raw = timekeeping_cycles_to_ns(&tk->tkr_raw, cycles); > } while (read_seqcount_retry(&tk_core.seq, seq)); > > xtstamp->sys_realtime = ktime_add_ns(base_real, nsec_real); > xtstamp->sys_monoraw = ktime_add_ns(base_raw, nsec_raw); > + > + /* > + * Interpolate if necessary, working back from the start of the current > + * interval > + */ > + if (do_interp) { > + cycle_t total_history_cycles; > + ktime_t history_monoraw; > + ktime_t history_realtime; > + bool discontinuity; > + cycle_t partial_history_cycles = cycles - raw_sys.cycles; > + > + if (!history_ref || history_ref->cs_seq != cs_seq ||
I've not done a super close reading here. But is it very likely the the history_ref->cs_seq is the same as the captured seq? I thought this history_ref was to allow old cross stamps to be used to improve the back-calculation of the time at the given cycle value. So throwing them out if they are older then the last tick seems strange.
thanks -john
| |