Messages in this thread | | | Date | Wed, 22 Feb 2023 01:27:22 +0900 | Subject | Re: [PATCH] rust: time: New module for timekeeping functions | From | Asahi Lina <> |
| |
On 21/02/2023 23.06, Boqun Feng wrote: > On Tue, Feb 21, 2023 at 01:32:51PM +0100, Thomas Gleixner wrote: >> On Tue, Feb 21 2023 at 16:06, Asahi Lina wrote: >>> + >>> +use crate::bindings; >>> +use core::time::Duration; >>> + >>> +/// Returns the kernel time elapsed since boot, excluding time spent sleeping, as a [`Duration`]. >>> +pub fn ktime_get() -> Duration { >>> + // SAFETY: Function has no side effects and no inputs. >>> + Duration::from_nanos(unsafe { bindings::ktime_get() }.try_into().unwrap()) >> >> Why is this a Duration? From the spec: >> > > I agree that returning a Duration may not be ideal, but..
>> In my understanding 'Duration' is a time span between two points, while >> ktime_get() and ktime_get_boottime() return the current time of >> monotonically nondecreasing clocks, i.e. they fall into the 'Instant' >> category.
The return values are analogous to `Instant` (which is not available in the kernel, so we can't use it anyway), but they can't be the same type in that case. `Instant` in std refers to a specific clock and its invariants only hold if all instances of the type refer to the same clock. So if we want to do something analogous here, we need separate types for each clock as Boqun mentioned...
>> Now the problem is that 'Instant' in it's specification is bound to >> CLOCK_MONOTONIC and there is no way to express CLOCK_BOOTTIME, but >> that's a shortcoming of the spec which ignores CLOCK_BOOTTIME >> completely. IOW, that's also a problem for user space.
It's sort of inherent in the idea that all `Instant` instances must share the same clock, so there can only be one canonical clock on any given platform that is represented by `Instant`.
Of course std could have separate types to support more than one clock, but then you'd end up with platform-specific variants... and I don't think the goal was to support all possible platform-specific clocks in that std API.
That's also why I went with Duration, since that doesn't try to claim those invariants and represents "a time duration since boot" in this case (measured according to different rules depending on what API you call). Basically it punts the problem of not mixing clocks to the API user... which is not ideal, but it's exactly what the C API does. ktime_t naturally maps well to Duration since it does not encode any clock/epoch information in the type.
> To me, there seems > two options to provide Rust types for kernel time management: > > * Use KTime which maps to ktime_t, then we have the similar > semantics around it: sometimes it's a duration, sometimes it's > a point of time.. but I know "this is a safe language, you > should do more" ;-) > > * Introduce kernel's own types, e.g. BootTime, RawTime, TAI, > RealTime, and make them play with Duration (actually I'd prefer > we have own Duration, because Rust core::time::Duration takes > more than u64), something like below: > > > pub struct BootTime { > d: Duration > } > > impl BootTime { > fn now() -> Self { > unsafe { BootTime { d: ktime_to_duration(ktime_get_boottime())} } > } > fn add(self, d: Duration) -> Self { > <Add a duration, similar to ktime_add> > } > fn sub(self, other: Self) -> Duration { > ... > } > ... > } > > Thoughts?
I think that's the better approach, but I was hoping to leave that for a future patch, especially since right now I'm the only user of this API in the upcoming Apple GPU driver and it only uses it to implement some really simple timeouts for polled operations, which isn't much API coverage... I figured we might get a better idea for what to do once a second user comes along. For example, do we want straight methods like that or std::ops trait impls? And do we make everything fallible or panic on overflow or just wrap?
It also means we basically have to reimplement all of core::time::Duration if we want to offer an equally ergonomic API with a 64-bit type (for reference, Duration is a struct with u64 secs and u32 nanoseconds).
>>> +} >>> + >>> +/// Returns the kernel time elapsed since boot, including time spent sleeping, as a [`Duration`]. >>> +pub fn ktime_get_boottime() -> Duration { >>> + Duration::from_nanos( >>> + // SAFETY: Function has no side effects and no variable inputs. >>> + unsafe { bindings::ktime_get_with_offset(bindings::tk_offsets_TK_OFFS_BOOT) } >> >> No. Please use ktime_get_boottime() and not the timekeeping internal function.
`ktime_get_boottime()` is static inline, so it will need a manual helper wrapper to be callable from Rust (at least until bindgen implements automatic helper generation, which I hear is coming soon). I was trying to avoid introducing even more helpers, since helpers.c is kind of already getting out of hand in my branch with all the stuff that's getting added... but I can add it if you don't want me to use ktime_get_with_offset(). It'll also be slower this way though (since the helper introduces another layer of function calling), and I figured of all things we probably want time functions to be fast, since they tend to get called a lot...
~~ Lina
| |