| Message ID | 20230929023737.1610865-1-maheshb@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | add ptp_gettimex64any() API | expand |
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
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); +} +EXPORT_SYMBOL_GPL(ktime_get_cycles64); + /** * ktime_get_seconds - Get the seconds portion of CLOCK_MONOTONIC *
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(+)