Re: [RFC][PATCH 1/2] time: Add ktime_get_mono_raw_fast_ns()

[John Stultz]

On Fri, Feb 20, 2015 at 6:29 AM, Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
> By popular demand, add a MONOTONIC_RAW variant.
>
> The implementation is up for discussion; but given the need for the
> dummy thing I thought it was easiest to just always update the
> mono_raw clock state in timekeeping_update() for now, even though its
> mostly wasted cycles.
>
> I suppose the right place would be a resume callback somewhere.
>
> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Cc: John Stultz <john.stultz@xxxxxxxxxx>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
> ---
>  include/linux/timekeeping.h |    1 +
>  kernel/time/timekeeping.c   |   42 +++++++++++++++++++++++++++++++++++-------
>  2 files changed, 36 insertions(+), 7 deletions(-)
>
> --- a/include/linux/timekeeping.h
> +++ b/include/linux/timekeeping.h
> @@ -220,6 +220,7 @@ static inline u64 ktime_get_raw_ns(void)
>  }
>
>  extern u64 ktime_get_mono_fast_ns(void);
> +extern u64 ktime_get_mono_raw_fast_ns(void);
>
>  /*
>   * Timespec interfaces utilizing the ktime based ones
> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -58,7 +58,8 @@ struct tk_fast {
>         struct tk_read_base     base[2];
>  };
>
> -static struct tk_fast tk_fast_mono ____cacheline_aligned;
> +static struct tk_fast tk_fast_mono     ____cacheline_aligned;
> +static struct tk_fast tk_fast_mono_raw ____cacheline_aligned;
>
>  /* flag for if timekeeping is suspended */
>  int __read_mostly timekeeping_suspended;
> @@ -267,18 +268,18 @@ static inline s64 timekeeping_get_ns_raw
>   * slightly wrong timestamp (a few nanoseconds). See
>   * @ktime_get_mono_fast_ns.
>   */
> -static void update_fast_timekeeper(struct tk_read_base *tkr)
> +static void update_fast_timekeeper(struct tk_fast *fast, struct tk_read_base *tkr)
>  {
> -       struct tk_read_base *base = tk_fast_mono.base;
> +       struct tk_read_base *base = fast->base;
>
>         /* Force readers off to base[1] */
> -       raw_write_seqcount_latch(&tk_fast_mono.seq);
> +       raw_write_seqcount_latch(&fast->seq);
>
>         /* Update base[0] */
>         memcpy(base, tkr, sizeof(*base));
>
>         /* Force readers back to base[0] */
> -       raw_write_seqcount_latch(&tk_fast_mono.seq);
> +       raw_write_seqcount_latch(&fast->seq);
>
>         /* Update base[1] */
>         memcpy(base + 1, base, sizeof(*base));
> @@ -332,6 +333,22 @@ u64 notrace ktime_get_mono_fast_ns(void)
>  }
>  EXPORT_SYMBOL_GPL(ktime_get_mono_fast_ns);
>
> +u64 notrace ktime_get_mono_raw_fast_ns(void)
> +{
> +       struct tk_read_base *tkr;
> +       unsigned int seq;
> +       u64 now;
> +
> +       do {
> +               seq = raw_read_seqcount(&tk_fast_mono_raw.seq);
> +               tkr = tk_fast_mono_raw.base + (seq & 0x01);
> +               now = ktime_to_ns(tkr->base_mono) + timekeeping_get_ns(tkr);


So this doesn't look right. I think you want to use tk->base_raw and
timekeeping_get_ns_raw() here?


> +       } while (read_seqcount_retry(&tk_fast_mono_raw.seq, seq));
> +       return now;
> +}
> +EXPORT_SYMBOL_GPL(ktime_get_mono_raw_fast_ns);
> +
>  /* Suspend-time cycles value for halted fast timekeeper. */
>  static cycle_t cycles_at_suspend;
>
> @@ -358,7 +375,11 @@ static void halt_fast_timekeeper(struct
>         memcpy(&tkr_dummy, tkr, sizeof(tkr_dummy));
>         cycles_at_suspend = tkr->read(tkr->clock);
>         tkr_dummy.read = dummy_clock_read;
> -       update_fast_timekeeper(&tkr_dummy);
> +       update_fast_timekeeper(&tk_fast_mono,     &tkr_dummy);
> +
> +       tkr_dummy.mult  = tkr->clock->mult;
> +       tkr_dummy.shift = tkr->clock->shift;
> +       update_fast_timekeeper(&tk_fast_mono_raw, &tkr_dummy);
>  }
>
>  #ifdef CONFIG_GENERIC_TIME_VSYSCALL_OLD
> @@ -475,6 +496,8 @@ static inline void tk_update_ktime_data(
>  /* must hold timekeeper_lock */
>  static void timekeeping_update(struct timekeeper *tk, unsigned int action)
>  {
> +       static struct tk_read_base tkr;
> +
>         if (action & TK_CLEAR_NTP) {
>                 tk->ntp_error = 0;
>                 ntp_clear();
> @@ -489,7 +512,12 @@ static void timekeeping_update(struct ti
>                 memcpy(&shadow_timekeeper, &tk_core.timekeeper,
>                        sizeof(tk_core.timekeeper));
>
> -       update_fast_timekeeper(&tk->tkr);
> +       update_fast_timekeeper(&tk_fast_mono, &tk->tkr);
> +
> +       tkr = tk->tkr;
> +       tkr.mult  = tk->tkr.clock->mult;
> +       tkr.shift = tk->tkr.clock->shift;
> +       update_fast_timekeeper(&tk_fast_mono_raw, &tkr);

So this is sort of sneaky and subtle here, which will surely cause
problems later on. You're copying the original mult/shift pair into a
copy of the tkr, so you get similar results from timekeeping_get_ns()
as you'd want from timekeeping_get_ns_raw().  This results in multiple
ways of getting the raw clock.

I think it would be better to either add a new tkr structure for the
raw clock in the timekeeper, so you can directly copy it over, or
extend the tkr structure so it can contain the raw values as well.

Also, there's no real reason to have fast/non-fast versions of the raw
clock. Since it isn't affected by frequency changes, it can't have the
inconsistency issues the monotonic clock can see (which are documented
in the comment near ktime_get_mono_fast_ns()). So we can probably
condense these and avoid duplicative code.

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/