Messages in this thread | | | From | Mahesh Bandewar (महेश बंडेवार) <> | Date | Thu, 28 Sep 2023 23:34:37 -0700 | Subject | Re: [PATCH 1/4] time: add ktime_get_cycles64() api |
| |
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
| |