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

[John Stultz]

On Thu, Sep 28, 2023 at 11:35 PM Mahesh Bandewar (महेश बंडेवार)
<maheshb@xxxxxxxxxx> wrote:
>
> On Thu, Sep 28, 2023 at 10:15 PM John Stultz <jstultz@xxxxxxxxxx> 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