Message-ID: <CAF2d9jgeGLCzbFZhptGzpUnmMgLaRysyzBmpZ+dK4sxWdmR5ZQ@mail.gmail.com>
Date:   Thu, 28 Sep 2023 23:34:37 -0700
From:   Mahesh Bandewar (महेश बंडेवार) 
        <maheshb@...gle.com>
To:     John Stultz <jstultz@...gle.com>
Cc:     Netdev <netdev@...r.kernel.org>,
        Linux <linux-kernel@...r.kernel.org>,
        David Miller <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Eric Dumazet <edumazet@...gle.com>,
        Paolo Abeni <pabeni@...hat.com>,
        Jonathan Corbet <corbet@....net>,
        Don Hatchett <hatch@...gle.com>,
        Yuliang Li <yuliangli@...gle.com>,
        Mahesh Bandewar <mahesh@...dewar.net>,
        Thomas Gleixner <tglx@...utronix.de>,
        Stephen Boyd <sboyd@...nel.org>,
        Richard Cochran <richardcochran@...il.com>
Subject: Re: [PATCH 1/4] time: add ktime_get_cycles64() api

On Thu, Sep 28, 2023 at 10:15 PM John Stultz <jstultz@...gle.com> wrote:
>
> On Thu, Sep 28, 2023 at 7:37 PM Mahesh Bandewar <maheshb@...gle.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@...gle.com>
> > CC: John Stultz <jstultz@...gle.com>
> > CC: Thomas Gleixner <tglx@...utronix.de>
> > CC: Stephen Boyd <sboyd@...nel.org>
> > CC: Richard Cochran <richardcochran@...il.com>
> > CC: netdev@...r.kernel.org
> > CC: linux-kernel@...r.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

Powered by blists - more mailing lists