[1/4] time: add ktime_get_cycles64() api

  add a method to retrieve raw cycles in the same fashion as there are
ktime_get_* methods available for supported time-bases. The method
continues using the 'struct timespec64' since the UAPI uses 'struct
ptp_clock_time'.

The caller can perform operation equivalent of timespec64_to_ns() to
retrieve raw-cycles value. The precision loss because of this conversion
should be none for 64 bit cycle counters and nominal at 96 bit counters
(considering UAPI of s64 + u32 of 'struct ptp_clock_time).

Signed-off-by: Mahesh Bandewar <maheshb@google.com>
CC: John Stultz <jstultz@google.com>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Stephen Boyd <sboyd@kernel.org>
CC: Richard Cochran <richardcochran@gmail.com>
CC: netdev@vger.kernel.org
CC: linux-kernel@vger.kernel.org
---
 include/linux/timekeeping.h |  1 +
 kernel/time/timekeeping.c   | 24 ++++++++++++++++++++++++
 2 files changed, 25 insertions(+)
  

On Thu, Sep 28, 2023 at 7:37 PM Mahesh Bandewar <maheshb@google.com> wrote:
>
> add a method to retrieve raw cycles in the same fashion as there are
> ktime_get_* methods available for supported time-bases. The method
> continues using the 'struct timespec64' since the UAPI uses 'struct
> ptp_clock_time'.
>
> The caller can perform operation equivalent of timespec64_to_ns() to
> retrieve raw-cycles value. The precision loss because of this conversion
> should be none for 64 bit cycle counters and nominal at 96 bit counters
> (considering UAPI of s64 + u32 of 'struct ptp_clock_time).
>
> Signed-off-by: Mahesh Bandewar <maheshb@google.com>
> CC: John Stultz <jstultz@google.com>
> CC: Thomas Gleixner <tglx@linutronix.de>
> CC: Stephen Boyd <sboyd@kernel.org>
> CC: Richard Cochran <richardcochran@gmail.com>
> CC: netdev@vger.kernel.org
> CC: linux-kernel@vger.kernel.org
> ---
>  include/linux/timekeeping.h |  1 +
>  kernel/time/timekeeping.c   | 24 ++++++++++++++++++++++++
>  2 files changed, 25 insertions(+)
>
> diff --git a/include/linux/timekeeping.h b/include/linux/timekeeping.h
> index fe1e467ba046..5537700ad113 100644
> --- a/include/linux/timekeeping.h
> +++ b/include/linux/timekeeping.h
> @@ -43,6 +43,7 @@ extern void ktime_get_ts64(struct timespec64 *ts);
>  extern void ktime_get_real_ts64(struct timespec64 *tv);
>  extern void ktime_get_coarse_ts64(struct timespec64 *ts);
>  extern void ktime_get_coarse_real_ts64(struct timespec64 *ts);
> +extern void ktime_get_cycles64(struct timespec64 *ts);
>
>  void getboottime64(struct timespec64 *ts);
>
> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> index 266d02809dbb..35d603d21bd5 100644
> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -989,6 +989,30 @@ void ktime_get_ts64(struct timespec64 *ts)
>  }
>  EXPORT_SYMBOL_GPL(ktime_get_ts64);
>
> +/**
> + * ktime_get_cycles64 - get the raw clock cycles in timespec64 format
> + * @ts:                pointer to timespec variable
> + *
> + * This function converts the raw clock cycles into timespce64 format
> + * in the varibale pointed to by @ts
> + */
> +void ktime_get_cycles64(struct timespec64 *ts)
> +{
> +       struct timekeeper *tk = &tk_core.timekeeper;
> +       unsigned int seq;
> +       u64 now;
> +
> +       WARN_ON_ONCE(timekeeping_suspended);
> +
> +       do {
> +               seq = read_seqcount_begin(&tk_core.seq);
> +               now = tk_clock_read(&tk->tkr_mono);
> +       } while (read_seqcount_retry(&tk_core.seq, seq));
> +
> +       *ts = ns_to_timespec64(now);
> +}

Hey Mahesh,
  Thanks for sending this out.  Unfortunately, I'm a bit confused by
this. It might be helpful to further explain what this would be used
for in more detail?

Some aspects that are particularly unclear:
1) You seem to be trying to stuff cycle values into a timespec64,
which is not very intuitive (and a type error of sorts). It's not
clear /why/ that type is useful.

2) Depending on your clocksource, this would have very strange
wrapping behavior, so I'm not sure it is generally safe to use.

3) Nit: The interface is called ktime_get_cycles64 (timespec64
returning interfaces usually are postfixed with ts64).

I guess could you clarify why you need this instead of using
CLOCK_MONOTONIC_RAW which tries to abstract raw cycles in a way that
is safe and avoids wrapping across various clocksources?

thanks
-john
  

On Thu, Sep 28, 2023 at 10:15 PM John Stultz <jstultz@google.com> wrote:
>
> On Thu, Sep 28, 2023 at 7:37 PM Mahesh Bandewar <maheshb@google.com> wrote:
> >
> > add a method to retrieve raw cycles in the same fashion as there are
> > ktime_get_* methods available for supported time-bases. The method
> > continues using the 'struct timespec64' since the UAPI uses 'struct
> > ptp_clock_time'.
> >
> > The caller can perform operation equivalent of timespec64_to_ns() to
> > retrieve raw-cycles value. The precision loss because of this conversion
> > should be none for 64 bit cycle counters and nominal at 96 bit counters
> > (considering UAPI of s64 + u32 of 'struct ptp_clock_time).
> >
> > Signed-off-by: Mahesh Bandewar <maheshb@google.com>
> > CC: John Stultz <jstultz@google.com>
> > CC: Thomas Gleixner <tglx@linutronix.de>
> > CC: Stephen Boyd <sboyd@kernel.org>
> > CC: Richard Cochran <richardcochran@gmail.com>
> > CC: netdev@vger.kernel.org
> > CC: linux-kernel@vger.kernel.org
> > ---
> >  include/linux/timekeeping.h |  1 +
> >  kernel/time/timekeeping.c   | 24 ++++++++++++++++++++++++
> >  2 files changed, 25 insertions(+)
> >
> > diff --git a/include/linux/timekeeping.h b/include/linux/timekeeping.h
> > index fe1e467ba046..5537700ad113 100644
> > --- a/include/linux/timekeeping.h
> > +++ b/include/linux/timekeeping.h
> > @@ -43,6 +43,7 @@ extern void ktime_get_ts64(struct timespec64 *ts);
> >  extern void ktime_get_real_ts64(struct timespec64 *tv);
> >  extern void ktime_get_coarse_ts64(struct timespec64 *ts);
> >  extern void ktime_get_coarse_real_ts64(struct timespec64 *ts);
> > +extern void ktime_get_cycles64(struct timespec64 *ts);
> >
> >  void getboottime64(struct timespec64 *ts);
> >
> > diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> > index 266d02809dbb..35d603d21bd5 100644
> > --- a/kernel/time/timekeeping.c
> > +++ b/kernel/time/timekeeping.c
> > @@ -989,6 +989,30 @@ void ktime_get_ts64(struct timespec64 *ts)
> >  }
> >  EXPORT_SYMBOL_GPL(ktime_get_ts64);
> >
> > +/**
> > + * ktime_get_cycles64 - get the raw clock cycles in timespec64 format
> > + * @ts:                pointer to timespec variable
> > + *
> > + * This function converts the raw clock cycles into timespce64 format
> > + * in the varibale pointed to by @ts
> > + */
> > +void ktime_get_cycles64(struct timespec64 *ts)
> > +{
> > +       struct timekeeper *tk = &tk_core.timekeeper;
> > +       unsigned int seq;
> > +       u64 now;
> > +
> > +       WARN_ON_ONCE(timekeeping_suspended);
> > +
> > +       do {
> > +               seq = read_seqcount_begin(&tk_core.seq);
> > +               now = tk_clock_read(&tk->tkr_mono);
> > +       } while (read_seqcount_retry(&tk_core.seq, seq));
> > +
> > +       *ts = ns_to_timespec64(now);
> > +}
>
> Hey Mahesh,
>   Thanks for sending this out.  Unfortunately, I'm a bit confused by
> this. It might be helpful to further explain what this would be used
> for in more detail?
>
Thanks for looking at this John. I think my cover-letter wasn't sent
to all reviewers and that's my mistake.

> Some aspects that are particularly unclear:
> 1) You seem to be trying to stuff cycle values into a timespec64,
> which is not very intuitive (and a type error of sorts). It's not
> clear /why/ that type is useful.
>
The primary idea is to build a PTP API similar to gettimex64() that
gives us a sandwich timestamp of a given timebase instead of just
sys-time. Since sys-time is disciplined (adjustment / steps), it's not
really suitable for all possible use cases. For the same reasons
CLOCK_MONOTONIC is also not suitable in a subset of use cases while
some do want to use it. So this API gives user a choice to select the
timebase. The ioctl() interface uses 'struct ptp_clock_time' (similar
to timespec64) hence the interface.

> 2) Depending on your clocksource, this would have very strange
> wrapping behavior, so I'm not sure it is generally safe to use.
>
The uapi does provide other alternatives like sys, mono, mono-raw
along with raw-cycles and users can choose.

> 3) Nit: The interface is called ktime_get_cycles64 (timespec64
> returning interfaces usually are postfixed with ts64).
>
Ah, thanks for the explanation. I can change to comply with the
convention. Does ktime_get_cycles_ts64() make more sense?

> I guess could you clarify why you need this instead of using
> CLOCK_MONOTONIC_RAW which tries to abstract raw cycles in a way that
> is safe and avoids wrapping across various clocksources?
>
My impression was that CLOCK_MONOTONIC_RAW (as the same suggests) does
provide you the raw / undisciplined cycles. However, code like below
does show that monotonic-raw is subjected to certain changes.
"""
int do_adjtimex(struct __kernel_timex *txc)
{
      [...]
        /*
         * The timekeeper keeps its own mult values for the currently
         * active clocksource. These value will be adjusted via NTP
         * to counteract clock drifting.
         */
        tk->tkr_mono.mult = clock->mult;
        tk->tkr_raw.mult = clock->mult;
        tk->ntp_err_mult = 0;
        tk->skip_second_overflow = 0;
}
"""
and that was the reason why I have added raw-cycles as another option.
Of course the user can always choose mono-raw if it satisfies their
use-case.

> thanks
> -john
  

On Thu, Sep 28, 2023 at 11:35 PM Mahesh Bandewar (महेश बंडेवार)
<maheshb@google.com> wrote:
>
> On Thu, Sep 28, 2023 at 10:15 PM John Stultz <jstultz@google.com> wrote:
> >
> >   Thanks for sending this out.  Unfortunately, I'm a bit confused by
> > this. It might be helpful to further explain what this would be used
> > for in more detail?
> >
> Thanks for looking at this John. I think my cover-letter wasn't sent
> to all reviewers and that's my mistake.

No worries, I was able to find it on lore. Though it looks like your
mail threading isn't quite right?


> > 2) Depending on your clocksource, this would have very strange
> > wrapping behavior, so I'm not sure it is generally safe to use.
> >
> The uapi does provide other alternatives like sys, mono, mono-raw
> along with raw-cycles and users can choose.

Sure, but how does userland know if it's safe to use raw cycles? How
do we avoid userland applications written against raw cycles from
breaking if they run on a different machine?
To me this doesn't feel like the interface has been abstracted enough.

> > 3) Nit: The interface is called ktime_get_cycles64 (timespec64
> > returning interfaces usually are postfixed with ts64).
> >
> Ah, thanks for the explanation. I can change to comply with the
> convention. Does ktime_get_cycles_ts64() make more sense?

Maybe a little (it at least looks consistent), but not really if
you're sticking raw cycles in the timespec :)

> > I guess could you clarify why you need this instead of using
> > CLOCK_MONOTONIC_RAW which tries to abstract raw cycles in a way that
> > is safe and avoids wrapping across various clocksources?
> >
> My impression was that CLOCK_MONOTONIC_RAW (as the same suggests) does
> provide you the raw / undisciplined cycles. However, code like below
> does show that monotonic-raw is subjected to certain changes.
> """
> int do_adjtimex(struct __kernel_timex *txc)
> {
>       [...]

Err. The bit below is from tk_setup_internals() not do_adjtimex(), no?

>         /*
>          * The timekeeper keeps its own mult values for the currently
>          * active clocksource. These value will be adjusted via NTP
>          * to counteract clock drifting.
>          */
>         tk->tkr_mono.mult = clock->mult;
>         tk->tkr_raw.mult = clock->mult;
>         tk->ntp_err_mult = 0;
>         tk->skip_second_overflow = 0;

So the comment is correct, except for the tkr_raw.mult value (I can
see how that is confusing). The raw mult is set to the clocksource
mult value and should not modified (unless the clocksource changes).

> """
> and that was the reason why I have added raw-cycles as another option.
> Of course the user can always choose mono-raw if it satisfies their
> use-case.

Having raw monotonic as an option seems reasonable to me (as it was
introduced to provide a generic abstraction for logic that was using
raw TSC values in an unportable way).

But the raw cycles interface still worries me, as I want to make sure
we're not creating user visible interfaces that expose raw hardware
details (making it very difficult to maintain long term).

thanks
-john
  

On Thu, Sep 28, 2023 at 11:56 PM John Stultz <jstultz@google.com> wrote:
> On Thu, Sep 28, 2023 at 11:35 PM Mahesh Bandewar (महेश बंडेवार)
> <maheshb@google.com> wrote:
> > On Thu, Sep 28, 2023 at 10:15 PM John Stultz <jstultz@google.com> wrote:
> > > 3) Nit: The interface is called ktime_get_cycles64 (timespec64
> > > returning interfaces usually are postfixed with ts64).
> > >
> > Ah, thanks for the explanation. I can change to comply with the
> > convention. Does ktime_get_cycles_ts64() make more sense?
>
> Maybe a little (it at least looks consistent), but not really if
> you're sticking raw cycles in the timespec :)
>

Despite my concerns that it's a bad idea, If one was going to expose
raw cycles from the timekeeping core, I'd suggest doing so directly as
a u64 (`u64 ktime_get_cycles(void)`).

That may mean widening (or maybe using a union in) your PTP ioctl data
structure to have a explicit cycles field.
Or introducing a separate ioctl that deals with cycles instead of timespec64s.

Squeezing data into types that are canonically used for something else
should always be avoided if possible (there are some cases where
you're stuck with an existing interface, but that's not the case
here).

But I still think we should avoid exporting the raw cycle values
unless there is some extremely strong argument for it (and if we can,
they should be abstracted into some sort of cookie value to avoid
userland using it as a raw clock).

thanks
-john
  

On Fri, Sep 29, 2023 at 12:06:46AM -0700, John Stultz wrote:
> But I still think we should avoid exporting the raw cycle values
> unless there is some extremely strong argument for it

Looks like the argument was based on a misunderstanding of what
CLOCK_MONOTONIC_RAW actually is.  So, please, let's not expose the raw
cycle counter value.

Thanks,
Richard
  

On Fri, Sep 29, 2023 at 12:07 AM John Stultz <jstultz@google.com> wrote:
>
> On Thu, Sep 28, 2023 at 11:56 PM John Stultz <jstultz@google.com> wrote:
> > On Thu, Sep 28, 2023 at 11:35 PM Mahesh Bandewar (महेश बंडेवार)
> > <maheshb@google.com> wrote:
> > > On Thu, Sep 28, 2023 at 10:15 PM John Stultz <jstultz@google.com> wrote:
> > > > 3) Nit: The interface is called ktime_get_cycles64 (timespec64
> > > > returning interfaces usually are postfixed with ts64).
> > > >
> > > Ah, thanks for the explanation. I can change to comply with the
> > > convention. Does ktime_get_cycles_ts64() make more sense?
> >
> > Maybe a little (it at least looks consistent), but not really if
> > you're sticking raw cycles in the timespec :)
> >
>
> Despite my concerns that it's a bad idea, If one was going to expose
> raw cycles from the timekeeping core, I'd suggest doing so directly as
> a u64 (`u64 ktime_get_cycles(void)`).
>
> That may mean widening (or maybe using a union in) your PTP ioctl data
> structure to have a explicit cycles field.
> Or introducing a separate ioctl that deals with cycles instead of timespec64s.
>
> Squeezing data into types that are canonically used for something else
> should always be avoided if possible (there are some cases where
> you're stuck with an existing interface, but that's not the case
> here).
>
> But I still think we should avoid exporting the raw cycle values
> unless there is some extremely strong argument for it (and if we can,
> they should be abstracted into some sort of cookie value to avoid
> userland using it as a raw clock).
>
Thanks for the input John. This change is basically to address the API
gap and allow it to give a user-given timebase for the sandwich time.
I will remove this RAW-CYCLES option for now. If it's deemed
necessary, we can always add it later into the same API.

> thanks
> -john
  

On Mon, Oct 2, 2023 at 5:13 PM Mahesh Bandewar (महेश बंडेवार)
<maheshb@google.com> wrote:
>
> On Fri, Sep 29, 2023 at 12:07 AM John Stultz <jstultz@google.com> wrote:
> >
> > On Thu, Sep 28, 2023 at 11:56 PM John Stultz <jstultz@google.com> wrote:
> > > On Thu, Sep 28, 2023 at 11:35 PM Mahesh Bandewar (महेश बंडेवार)
> > > <maheshb@google.com> wrote:
> > > > On Thu, Sep 28, 2023 at 10:15 PM John Stultz <jstultz@google.com> wrote:
> > > > > 3) Nit: The interface is called ktime_get_cycles64 (timespec64
> > > > > returning interfaces usually are postfixed with ts64).
> > > > >
> > > > Ah, thanks for the explanation. I can change to comply with the
> > > > convention. Does ktime_get_cycles_ts64() make more sense?
> > >
> > > Maybe a little (it at least looks consistent), but not really if
> > > you're sticking raw cycles in the timespec :)
> > >
> >
> > Despite my concerns that it's a bad idea, If one was going to expose
> > raw cycles from the timekeeping core, I'd suggest doing so directly as
> > a u64 (`u64 ktime_get_cycles(void)`).
> >
> > That may mean widening (or maybe using a union in) your PTP ioctl data
> > structure to have a explicit cycles field.
> > Or introducing a separate ioctl that deals with cycles instead of timespec64s.
> >
> > Squeezing data into types that are canonically used for something else
> > should always be avoided if possible (there are some cases where
> > you're stuck with an existing interface, but that's not the case
> > here).
> >
> > But I still think we should avoid exporting the raw cycle values
> > unless there is some extremely strong argument for it (and if we can,
> > they should be abstracted into some sort of cookie value to avoid
> > userland using it as a raw clock).
> >
> Thanks for the input John. This change is basically to address the API
> gap and allow it to give a user-given timebase for the sandwich time.
> I will remove this RAW-CYCLES option for now. If it's deemed
> necessary, we can always add it later into the same API.

Sounds reasonable to me.

thanks
-john
  

On Sat, Sep 30 2023 at 14:15, Richard Cochran wrote:
> On Fri, Sep 29, 2023 at 12:06:46AM -0700, John Stultz wrote:
>> But I still think we should avoid exporting the raw cycle values
>> unless there is some extremely strong argument for it
>
> Looks like the argument was based on a misunderstanding of what
> CLOCK_MONOTONIC_RAW actually is.  So, please, let's not expose the raw
> cycle counter value.

Correct. Exposing the raw counter value is broken if the counter wraps
around on a regular base.

Thanks,

        tglx