Re: [PATCH] rust: time: New module for timekeeping functions

[Heghedus Razvan]

------- Original Message -------
On Wednesday, February 22nd, 2023 at 6:45 AM, Asahi Lina <lina@xxxxxxxxxxxxx> wrote:


> On 22/02/2023 11.54, Boqun Feng wrote:
> 
> > On Wed, Feb 22, 2023 at 01:24:53AM +0100, Thomas Gleixner wrote:
> > 
> > > Miguel!
> > > 
> > > On Tue, Feb 21 2023 at 23:29, Miguel Ojeda wrote:
> > > 
> > > > On Tue, Feb 21, 2023 at 7:45 PM Thomas Gleixner tglx@xxxxxxxxxxxxx wrote:
> > > > 
> > > > > But xb abd xr are the same datatype because they represent a time delta.
> > > > 
> > > > In principle, one could also have different duration types too. For
> > > > instance, C++'s `std::chrono::duration` type is parametrized on the
> > > > representation type and the tick period, and thus an operation between
> > > > two time points like t1 - t0 returns a duration type that depends on
> > > > the type of the time points, i.e. which clock they were obtained from.
> > > 
> > > Correct, but for practical purposes I'd assume that the timestamps
> > > retrieved via ktime_get*() have the same granularity, i.e. 1ns.
> > > 
> > > TBH, that's not entirely correct because:
> > > 
> > > - the underlying hardware clocksource might not have a 1ns
> > > resolution
> > > 
> > > - the CLOCK_*_COARSE implementations are only advanced once per
> > > tick, but are executing significantly faster because they avoid
> > > the hardware counter access.
> > > 
> > > But that's an assumption which has proven to be workable and correct
> > > with the full zoo of hardware supported by the kernel.
> > > 
> > > The point is that all CLOCK_* variants, except CLOCK_REALTIME and
> > > CLOCK_TAI are guaranteed to never go backwards.
> > > 
> > > CLOCK_REALTIME and CLOCK_TAI are special as they can be set by user
> > > space and CLOCK_REALTIME has the extra oddities of leap seconds. But
> > > that's a well understood issue and is not specific to the kernel.
> > > 
> > > Back to time deltas (or duration types). Independent of the above it
> > > might make sense to be type strict about these as well. Especially if we
> > > go one step further and have timers based on CLOCK_* which need to be
> > > armed by either timestamps for absolute expiry or time deltas for
> > > relative to now expiry. I definitely can see a point for requiring
> > > matching time delta types there.
> > > 
> > > That said, I have no strong opinions about this particular detail and
> > > leave it to the Rusties to agree on something sensible.
> > 
> > I'd like to propose something below to make thing forward quickly:
> > 
> > Given Lina only uses CLOCK_BOOTTIME and CLOCK_MONOTONIC, I'd say we
> > reuse core::time::Duration and probably remain its ">=0" semantics even
> > in the future we change its internal representation to u64.
> > 
> > For timestamp type, use Instant semantics and use different types for
> > different clocks, i.e. similar to the implementation from Heghedus (much
> > better than mine!). But we can avoid implementing a fully version of
> > Instant, and focus on just the piece that Lina needs, which I believe
> > it's elapsed()?
> > 
> > For the future, if we were to support non-monotonic timestamp, maybe we
> > use the different type name like TimeStamp and TimeDelta.
> > 
> > In short:
> > 
> > * For monotonic clocks, Instant + Duration, and keep them similar
> > to std semantics.
> > 
> > * For non-monotonic clocks, don't worry it right now, and
> > probably different types for both stamps and deltas.
> > 
> > Thoughts?
> 
> 
> I actually only used CLOCK_MONOTONIC in the end, so I could even leave
> CLOCK_BOOTTIME for later, though I like the idea of having scaffolding
> for several clock types even if we only implement one initially.
> 
> This works for me, if you're happy with the idea I'll give it a spin
> based on Heghedus' example. Heghedus, is it okay if I put you down as
> Co-developed-by and can I get a signoff? ^^
Yes, of course. You have my support.

-- Heghedus Razvan (heghedus.razvan@xxxxxxxxxxxxxx)

> 
> For the actual Instant type, I was thinking it makes sense to just
> internally represent it as a newtype of Duration as well. Then all the
> math becomes trivial based on Duration operations, and when we replace
> Duration with a new u64 type it'll all work out the same. Fundamentally
> that means Instant types are internally stored as the Duration between
> the epoch (e.g. system boot) subject to the way that clock ticks, which
> I think is a reasonable internal representation? (In other words, it's
> the same as my original patch behind the scenes, but wrapped in type
> safety).
> 
> ~~ Lina