Re: [RFC v5 3/6] Add history to cross timestamp interface supporting slower devices

[John Stultz]

On Mon, Jan 4, 2016 at 4:45 AM, Christopher S. Hall
<christopher.s.hall@xxxxxxxxx> 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
--
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/