rust: time: New module for timekeeping functions

  This module is intended to contain functions related to kernel
timekeeping and time. Initially, this just wraps ktime_get() and
ktime_get_boottime() and returns them as core::time::Duration instances.
This is useful for drivers that need to implement simple retry loops and
timeouts.

Signed-off-by: Asahi Lina <lina@asahilina.net>
---
 rust/bindings/bindings_helper.h |  4 +++-
 rust/kernel/lib.rs              |  1 +
 rust/kernel/time.rs             | 25 +++++++++++++++++++++++++
 3 files changed, 29 insertions(+), 1 deletion(-)


---
base-commit: 89f5349e0673322857bd432fa23113af56673739
change-id: 20230221-gpu-up-time-ea9412204c3b

Thank you,
~~ Lina
  

On Tue, 21 Feb 2023 at 07:16, Asahi Lina <lina@asahilina.net> wrote:
>
> This module is intended to contain functions related to kernel
> timekeeping and time. Initially, this just wraps ktime_get() and
> ktime_get_boottime() and returns them as core::time::Duration instances.
> This is useful for drivers that need to implement simple retry loops and
> timeouts.
>
> Signed-off-by: Asahi Lina <lina@asahilina.net>
> ---

Nice and simple C interface to create Rust abstractions for.

Reviewed-by: Eric Curtin <ecurtin@redhat.com>

Is mise le meas/Regards,

Eric Curtin

>  rust/bindings/bindings_helper.h |  4 +++-
>  rust/kernel/lib.rs              |  1 +
>  rust/kernel/time.rs             | 25 +++++++++++++++++++++++++
>  3 files changed, 29 insertions(+), 1 deletion(-)
>
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index 75d85bd6c592..587f3d1c0c9f 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -6,8 +6,10 @@
>   * Sorted alphabetically.
>   */
>
> -#include <linux/slab.h>
> +#include <linux/ktime.h>
>  #include <linux/refcount.h>
> +#include <linux/slab.h>
> +#include <linux/timekeeping.h>
>
>  /* `bindgen` gets confused at certain things. */
>  const gfp_t BINDINGS_GFP_KERNEL = GFP_KERNEL;
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 223564f9f0cc..371b1b17570e 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -37,6 +37,7 @@ mod static_assert;
>  pub mod std_vendor;
>  pub mod str;
>  pub mod sync;
> +pub mod time;
>  pub mod types;
>
>  #[doc(hidden)]
> diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
> new file mode 100644
> index 000000000000..02844db47d34
> --- /dev/null
> +++ b/rust/kernel/time.rs
> @@ -0,0 +1,25 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Timekeeping functions.
> +//!
> +//! C header: [`include/linux/ktime.h`](../../../../include/linux/ktime.h)
> +//! C header: [`include/linux/timekeeping.h`](../../../../include/linux/timekeeping.h)
> +
> +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())
> +}
> +
> +/// 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) }
> +            .try_into()
> +            .unwrap(),
> +    )
> +}
>
> ---
> base-commit: 89f5349e0673322857bd432fa23113af56673739
> change-id: 20230221-gpu-up-time-ea9412204c3b
>
> Thank you,
> ~~ Lina
>
>
  

On Tuesday, February 21st, 2023 at 08:06, Asahi Lina <lina@asahilina.net> wrote:

> This module is intended to contain functions related to kernel
> timekeeping and time. Initially, this just wraps ktime_get() and
> ktime_get_boottime() and returns them as core::time::Duration instances.
> This is useful for drivers that need to implement simple retry loops and
> timeouts.
> 
> Signed-off-by: Asahi Lina lina@asahilina.net
> 
> ---
>  rust/bindings/bindings_helper.h |  4 +++-
>  rust/kernel/lib.rs              |  1 +
>  rust/kernel/time.rs             | 25 +++++++++++++++++++++++++
>  3 files changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index 75d85bd6c592..587f3d1c0c9f 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -6,8 +6,10 @@
>   * Sorted alphabetically.
>   */
> 
> -#include <linux/slab.h>
> +#include <linux/ktime.h>
>  #include <linux/refcount.h>
> +#include <linux/slab.h>
> +#include <linux/timekeeping.h>
> 
>  /* `bindgen` gets confused at certain things. */
>  const gfp_t BINDINGS_GFP_KERNEL = GFP_KERNEL;
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 223564f9f0cc..371b1b17570e 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -37,6 +37,7 @@ mod static_assert;
>  pub mod std_vendor;
>  pub mod str;
>  pub mod sync;
> +pub mod time;
>  pub mod types;
> 
>  #[doc(hidden)]
> diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
> new file mode 100644
> index 000000000000..02844db47d34
> --- /dev/null
> +++ b/rust/kernel/time.rs
> @@ -0,0 +1,25 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Timekeeping functions.
> +//!
> +//! C header: [`include/linux/ktime.h`](../../../../include/linux/ktime.h)
> +//! C header: [`include/linux/timekeeping.h`](../../../../include/linux/timekeeping.h)
> +
> +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())

ktime_t seems to be a signed 64bit int, while Duration::from_nanos expects an unsigned 64bit int. Based on https://lore.kernel.org/all/alpine.DEB.2.21.1903231125480.2157@nanos.tec.linutronix.de/T/#u I think it is fine to cast directly to u64 without doing the overflow check of try_into. This would eliminate a potential BUG() call. I think it would be fine to keep as is if you prefer though. As such:

Reviewed-by: Björn Roy Baron <bjorn3_gh@protonmail.com>

Cheers,
Bjorn

> +}
> +
> +/// 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) }
> +            .try_into()
> +            .unwrap(),
> +    )
> +}
> 
> ---
> base-commit: 89f5349e0673322857bd432fa23113af56673739
> change-id: 20230221-gpu-up-time-ea9412204c3b
> 
> Thank you,
> ~~ Lina
  

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:

    Duration

        A Duration type to represent a span of time, typically used for
        system timeouts.

    Instant

        A measurement of a monotonically nondecreasing clock. Opaque and
        useful only with Duration.

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.

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.

This makes sense vs. the other representation:

     SystemTime
     
        A measurement of the system clock, useful for talking to
        external entities like the file system or other processes.

This maps to CLOCK_REALTIME and CLOCK_TAI, i.e. ktime_get_real_ns() and
ktime_get_clocktai().

Similar to 'Instant' 'SystemTime' is strictly bound to CLOCK_REALTIME
by specification and there is no way to read CLOCK_TAI.

Please fix this in the spec and do not try to work around that by
pretending that a clock read is a 'Duration'.

> +}
> +
> +/// 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.

Thanks,

        tglx
  

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..

>     Duration
> 
>         A Duration type to represent a span of time, typically used for
>         system timeouts.
> 
>     Instant
> 
>         A measurement of a monotonically nondecreasing clock. Opaque and
>         useful only with Duration.
> 
> 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.
> 
> 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.
> 
> This makes sense vs. the other representation:
> 
>      SystemTime
>      
>         A measurement of the system clock, useful for talking to
>         external entities like the file system or other processes.
> 
> This maps to CLOCK_REALTIME and CLOCK_TAI, i.e. ktime_get_real_ns() and
> ktime_get_clocktai().
> 
> Similar to 'Instant' 'SystemTime' is strictly bound to CLOCK_REALTIME
> by specification and there is no way to read CLOCK_TAI.
> 

..'Instant' and 'SystemTime' are in Rust std, we cannot use them
directly, similar as we cannot use userspace libc. 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?

Regards,
Boqun

> Please fix this in the spec and do not try to work around that by
> pretending that a clock read is a 'Duration'.
> 
> > +}
> > +
> > +/// 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.
> 
> Thanks,
> 
>         tglx
  

On Tue, Feb 21 2023 at 06:06, Boqun Feng wrote:
> On Tue, Feb 21, 2023 at 01:32:51PM +0100, Thomas Gleixner wrote:
>> Similar to 'Instant' 'SystemTime' is strictly bound to CLOCK_REALTIME
>> by specification and there is no way to read CLOCK_TAI.
>> 
> ..'Instant' and 'SystemTime' are in Rust std, we cannot use them
> directly, similar as we cannot use userspace libc.

Sure. Was Rust std defined based on SysV from 30 years ago? :)

> 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 {
> 	        ...
> 	    }

I'm not rusty enough, but you really want two types:

    timestamp and timedelta

timestamp is an absolute time on a specific clock which is read via
now() and you can add time deltas to it. The latter is required for
arming an absolute timer on the clock.

timedelta is a relative time and completely independent of any
clock. That's what you get when you subtract two timestamps, but you can
also initialize it from a constant or some other source. timedelta can
be used to arm a relative timer on any clock.

Playing games with a single type is neither safe nor intuitive, right?

Thanks,

        tglx
  

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
  

On 22/02/2023 01.02, Thomas Gleixner wrote:
> On Tue, Feb 21 2023 at 06:06, Boqun Feng wrote:
>> 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 {
>> 	        ...
>> 	    }
> 
> I'm not rusty enough, but you really want two types:
> 
>     timestamp and timedelta
> 
> timestamp is an absolute time on a specific clock which is read via
> now() and you can add time deltas to it. The latter is required for
> arming an absolute timer on the clock.
> 
> timedelta is a relative time and completely independent of any
> clock. That's what you get when you subtract two timestamps, but you can
> also initialize it from a constant or some other source. timedelta can
> be used to arm a relative timer on any clock.

If all clocks end up as the same `timestamp` though, then this isn't
fully safe, because you could subtract `timestamp`s that came from
different clocks and the result would be meaningless. That's why the
Rust std Instant is specifically tied to one and only one system clock
on each platform.

~~ Lina
  

On 22/02/2023 01.27, Asahi Lina wrote:
> 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?

Also, it's probably worth mentioning that this kind of refactor can be
done without rewriting all the user code. For example, here is how I use
the APIs:

    let timeout = time::ktime_get() + Duration::from_millis(...);
    while time::ktime_get() < timeout {
        [...]
    }

Even if ktime_get() starts returning another type, as long as it can
interoperate with core::time::Duration the same way, it will continue to
compile (and if it only interoperates with a new kernel-specific
Duration, you'd only have to change the `use` statement at the top).

~~ Lina
  

On 2/21/23 4:32 AM, Thomas Gleixner wrote:
> 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.

That's not exactly *specified* -- it's meant to be opaque time. It is
documented that this currently uses clock_gettime monotonic on unix
targets, but "Disclaimer: These system calls might change over time."
CLOCK_MONOTONIC isn't even consistent across unix targets whether that
counts suspended time. It's been debated if we should switch to
CLOCK_BOOTTIME on Linux, but for now we're sticking to monotonic:

https://github.com/rust-lang/rust/pull/88714

But as others mentioned, this is in the std crate which isn't used by
the kernel. You could absolutely define your own with more specificity,
like kernel::time::BootTime, and copy an API similar to Instant's.
  

On Wed, Feb 22 2023 at 01:31, Asahi Lina wrote:
> On 22/02/2023 01.02, Thomas Gleixner wrote:
>> I'm not rusty enough, but you really want two types:
>> 
>>     timestamp and timedelta
>> 
>> timestamp is an absolute time on a specific clock which is read via
>> now() and you can add time deltas to it. The latter is required for
>> arming an absolute timer on the clock.
>> 
>> timedelta is a relative time and completely independent of any
>> clock. That's what you get when you subtract two timestamps, but you can
>> also initialize it from a constant or some other source. timedelta can
>> be used to arm a relative timer on any clock.
>
> If all clocks end up as the same `timestamp` though, then this isn't
> fully safe, because you could subtract `timestamp`s that came from
> different clocks and the result would be meaningless. That's why the
> Rust std Instant is specifically tied to one and only one system clock
> on each platform.

Fine, but do you agree that:

      ts1 = tboot.now()
      ...
      ts2 = tboot.now()

      xb = ts2 - ts1

then the result x1 cannot be the same data type as ts1, ts2.

From a typesafety perspective

      ts1 = treal.now()
      ...
      ts2 = tboot.now()

      x = ts2 - ts1

would be an invalid operation, but

      ts1 = treal.now()
      ...
      ts2 = treal.now()

      xr = ts2 - ts1

is obviously valid.

But xb abd xr are the same datatype because they represent a time delta.

That's the same the Rust std time semantics:

       Duration = Instance - Instance           valid
       Duration = Systemtime - SystemTime       valid
       Duration = Systemtime - Instance         invalid

No?

Thanks,

        tglx
  

Lina!

On Wed, Feb 22 2023 at 01:27, Asahi Lina wrote:
> 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:
>>>> +    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...

Sure. I'm understanding that part and fully agree.

>>> 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.

I can understand the idea, but I'm pretty sure that this will run into
issues sooner than later if someone tries to rewrite Linux userspace low
level system components in Rust.

> 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.

Sure, but ktime_t is also prone to issues which allow things you want to
prevent, i.e. substracting different clocks to calculate a duration.

>> 	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...

That's a patently bad approach, really. You are doing exactly what Rust
is trying to avoid and writing ad hoc APIs is generally frowned upon
even on the C side of the kernel. The requirements for kernel time(r)
interfaces are pretty well known.

> 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).

As you said yourself: The kernel can't use Rust std lib. So you better
implement sensible interfaces which are straight forward and simple to
use in the context you are aiming for.

>>>> +}
>>>> +
>>>> +/// 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...

I can understand that, but please add proper comments and an explanation
to the changelog then. That would have avoided tripping my taste sensor.

Be aware that my Rust foo is not even rusty it's close to non-existant.
That's probaly true for many maintainers you need to interact with.

Thanks,

        tglx
  

On Tue, Feb 21, 2023 at 08:00:52PM +0100, Thomas Gleixner wrote:
[...]
> > 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).
> 
> As you said yourself: The kernel can't use Rust std lib. So you better
> implement sensible interfaces which are straight forward and simple to
> use in the context you are aiming for.
> 

Agreed!

Lina, my suggestion is just to go ahead and add the minimal timestamp
abstraction, surely you may make some bad decisions about APIs (e.g.
panic vs returning a Result), but kernel doesn't have a stable internal
API, so we can always fix things later.

Myself actually do something like below based on your patch, because
nothing like `now()` ;-)

I'm sure you can do better because as you said, you're the first user
;-)

Regards,
Boqun

---------->8
diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
index 02844db47d34..3398388de0e1 100644
--- a/rust/kernel/time.rs
+++ b/rust/kernel/time.rs
@@ -6,16 +6,61 @@
 //! C header: [`include/linux/timekeeping.h`](../../../../include/linux/timekeeping.h)
 
 use crate::bindings;
-use core::time::Duration;
+// Re-exports [`Duration`], so that it's easy to provide kernel's own implemention in the future.
+pub use core::time::Duration;
+
+/// A timestamp
+pub trait TimeStamp: Copy {
+    /// Gets the current stamp.
+    fn now() -> Self;
+
+    /// Calculates the passed duration since `another`.
+    fn duration_since(&self, another: Self) -> Duration;
+
+
+    /// Return the duration passed since this stamp was created.
+    fn elapsed(&self) -> Duration {
+        let created = self.clone();
+        self.duration_since(created)
+    }
+}
+
+/// CLOCK_MONOTONIC timestamps.
+#[derive(Clone, Copy)]
+pub struct MonoTime(Duration);
+
+impl TimeStamp for MonoTime {
+    fn now() -> Self {
+        Self(ktime_get())
+    }
+
+    fn duration_since(&self, another: Self) -> Duration {
+        ktime_get() - another.0
+    }
+}
+
+/// CLOCK_BOOTTIME timestamps.
+#[derive(Clone, Copy)]
+pub struct BootTime(Duration);
+
+impl TimeStamp for BootTime {
+    fn now() -> Self {
+        Self(ktime_get_boottime())
+    }
+
+    fn duration_since(&self, another: Self) -> Duration {
+        ktime_get_boottime() - another.0
+    }
+}
 
 /// Returns the kernel time elapsed since boot, excluding time spent sleeping, as a [`Duration`].
-pub fn ktime_get() -> Duration {
+fn ktime_get() -> Duration {
     // SAFETY: Function has no side effects and no inputs.
     Duration::from_nanos(unsafe { bindings::ktime_get() }.try_into().unwrap())
 }
 
 /// Returns the kernel time elapsed since boot, including time spent sleeping, as a [`Duration`].
-pub fn ktime_get_boottime() -> Duration {
+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) }
  

On Tuesday, February 21st, 2023 at 8:45 PM, Thomas Gleixner <tglx@linutronix.de> wrote:


> On Wed, Feb 22 2023 at 01:31, Asahi Lina wrote:
> 
> > On 22/02/2023 01.02, Thomas Gleixner wrote:
> > 
> > > I'm not rusty enough, but you really want two types:
> > > 
> > > timestamp and timedelta
> > > 
> > > timestamp is an absolute time on a specific clock which is read via
> > > now() and you can add time deltas to it. The latter is required for
> > > arming an absolute timer on the clock.
> > > 
> > > timedelta is a relative time and completely independent of any
> > > clock. That's what you get when you subtract two timestamps, but you can
> > > also initialize it from a constant or some other source. timedelta can
> > > be used to arm a relative timer on any clock.
> > 
> > If all clocks end up as the same `timestamp` though, then this isn't
> > fully safe, because you could subtract `timestamp`s that came from
> > different clocks and the result would be meaningless. That's why the
> > Rust std Instant is specifically tied to one and only one system clock
> > on each platform.
> 
> 
> Fine, but do you agree that:
> 
> ts1 = tboot.now()
> ...
> ts2 = tboot.now()
> 
> xb = ts2 - ts1
> 
> then the result x1 cannot be the same data type as ts1, ts2.
> 
> From a typesafety perspective
> 
> ts1 = treal.now()
> ...
> ts2 = tboot.now()
> 
> x = ts2 - ts1
> 
> would be an invalid operation, but
> 
> ts1 = treal.now()
> ...
> ts2 = treal.now()
> 
> xr = ts2 - ts1
> 
> is obviously valid.
> 
> But xb abd xr are the same datatype because they represent a time delta.
> 
> That's the same the Rust std time semantics:
> 
> Duration = Instance - Instance valid
> Duration = Systemtime - SystemTime valid
> Duration = Systemtime - Instance invalid
> 
> No?
> 
I agree with Thomas on this one. The Rust type system is really powerful and we should take advantage of it. Time deltas can be enforced to be from the same clock at compile time.
Just for the sake of it, I wrote a small example on how this can be achieve: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=1d0f70bb5329b181f203ce7270e2957a


-- Heghedus Razvan (heghedus.razvan@protonmail.com)

> Thanks,
> 
> tglx
  

On Tue, Feb 21 2023 at 09:13, Josh Stone wrote:

> On 2/21/23 4:32 AM, Thomas Gleixner wrote:
>> 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.
>
> That's not exactly *specified* -- it's meant to be opaque time. It is
> documented that this currently uses clock_gettime monotonic on unix
> targets, but "Disclaimer: These system calls might change over time."
> CLOCK_MONOTONIC isn't even consistent across unix targets whether that
> counts suspended time. It's been debated if we should switch to
> CLOCK_BOOTTIME on Linux, but for now we're sticking to monotonic:

You'll need both when you want to implement substantial parts of the low
level user space stack in Rust. Same for CLOCK_TAI.

Thanks,

        tglx
  

On Tue, Feb 21, 2023 at 7:45 PM Thomas Gleixner <tglx@linutronix.de> 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.

Cheers,
Miguel
  

On Tue, Feb 21 2023 at 21:33, Heghedus Razvan wrote:
> On Tuesday, February 21st, 2023 at 8:45 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
>> That's the same the Rust std time semantics:
>> 
>> Duration = Instance - Instance valid
>> Duration = Systemtime - SystemTime valid
>> Duration = Systemtime - Instance invalid
>> 
>> No?
>> 
> I agree with Thomas on this one. The Rust type system is really
> powerful and we should take advantage of it. Time deltas can be
> enforced to be from the same clock at compile time.  Just for the sake
> of it, I wrote a small example on how this can be achieve:
> https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=1d0f70bb5329b181f203ce7270e2957a

Cute. This code makes even sense to Rustagnostics like me.

Thanks,

        tglx
  

Miguel!

On Tue, Feb 21 2023 at 23:29, Miguel Ojeda wrote:
> On Tue, Feb 21, 2023 at 7:45 PM Thomas Gleixner <tglx@linutronix.de> 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.

Thanks,

        tglx
  

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@linutronix.de> 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?

Regards,
Boqun

> Thanks,
> 
>         tglx
  

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@linutronix.de> 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? ^^

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
  

On 22/02/2023 04.00, Thomas Gleixner wrote:
>>>>> +}
>>>>> +
>>>>> +/// 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...
> 
> I can understand that, but please add proper comments and an explanation
> to the changelog then. That would have avoided tripping my taste sensor.
> 
> Be aware that my Rust foo is not even rusty it's close to non-existant.
> That's probaly true for many maintainers you need to interact with.

I'll add a comment ^^

Please do feel free to reach out and ask any questions about all this
crazy Rust stuff stuff! We're here to help and I know this is all new to
a lot of maintainers. I want people to be comfortable that we aren't
just creating more maintenance burden for everyone else.

That's also another conversation that we probably need to have, how do
we handle maintainership of Rust abstractions? I think Miguel mentioned
that ideally existing subsystem maintainers take over their bits of the
Rust side too over time, but of course a lot of people aren't going to
be comfortable with that if they don't have a lot of Rust experience
yet... personally I'm happy to sign up as co-maintainer or supporter of
the abstractions I contribute, or maybe we can just pool resources and
have people interested in Rust agree to help support this stuff for
every subsystem?

~~ Lina
  

On Wed, Feb 22, 2023 at 01:45:58PM +0900, Asahi Lina 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@linutronix.de> 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? ^^
> 
> 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

Sounds even better ;-) It means Instant and Duration have the exact same
behavior about sub and so on.

Regards,
Boqun

> the same as my original patch behind the scenes, but wrapped in type
> safety).
> 
> ~~ Lina
  

------- Original Message -------
On Wednesday, February 22nd, 2023 at 6:45 AM, Asahi Lina <lina@asahilina.net> 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@linutronix.de 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@protonmail.com)

> 
> 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
  

Lina !

On Wed, Feb 22 2023 at 13:56, Asahi Lina wrote:
> On 22/02/2023 04.00, Thomas Gleixner wrote:
>> Be aware that my Rust foo is not even rusty it's close to non-existant.
>> That's probaly true for many maintainers you need to interact with.
>
> Please do feel free to reach out and ask any questions about all this
> crazy Rust stuff stuff! We're here to help and I know this is all new to
> a lot of maintainers. I want people to be comfortable that we aren't
> just creating more maintenance burden for everyone else.

I can only speak for myself. I'm comfortable and sufficiently curious
about this particular flavour of crazy.

I don't think these abstractions are a huge burden as long as the folks
who implement them talk to the relevant maintainers so we don't end
up with Rust inflicted burdens or ill defined abstractions.

> That's also another conversation that we probably need to have, how do
> we handle maintainership of Rust abstractions? I think Miguel mentioned
> that ideally existing subsystem maintainers take over their bits of the
> Rust side too over time, but of course a lot of people aren't going to
> be comfortable with that if they don't have a lot of Rust experience
> yet... personally I'm happy to sign up as co-maintainer or supporter of
> the abstractions I contribute, or maybe we can just pool resources and
> have people interested in Rust agree to help support this stuff for
> every subsystem?

Having subsystem maintainer teams supplemented with a Rust wizard, is
probably the best option at the moment. Time will tell as always.

Thanks,

        tglx
  

> On Feb 21, 2023, at 9:46 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> On Tue, Feb 21 2023 at 09:13, Josh Stone wrote:
> 
>> On 2/21/23 4:32 AM, Thomas Gleixner wrote:
>>> 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.
>> 
>> That's not exactly *specified* -- it's meant to be opaque time. It is
>> documented that this currently uses clock_gettime monotonic on unix
>> targets, but "Disclaimer: These system calls might change over time."
>> CLOCK_MONOTONIC isn't even consistent across unix targets whether that
>> counts suspended time. It's been debated if we should switch to
>> CLOCK_BOOTTIME on Linux, but for now we're sticking to monotonic:
> 
> You'll need both when you want to implement substantial parts of the low
> level user space stack in Rust. Same for CLOCK_TAI.
> 
> Thanks,
> 
>        tglx

std isn’t really designed to provide full coverage of any particular OS
interface - it has to provide concepts that map cleanly onto Windows and
every flavor of Unix.[1] Low-level Unix Rust code typically uses the libc
crate, which just exports everything from libc as an unsafe function, or
one of several safe wrappers (nix is the most popular one, I’m partial to
rustix), alongside std.

[1]: std does in cases provide OS-specific functions - for example,
std::fs::Metadata (~= struct stat) has Unix-specific ways to get the
mode - but again, the goal here is to be broadly useful, not full
coverage.

Best wishes,
Gaelan
  

On Wed, Feb 22, 2023 at 1:24 AM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> 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.

Yeah, if you think having several delta types would make sense for
some use case, or at least prevent some bugs statically (especially if
you have been similar issues in the past), then I think we should
eventually do it. Not necessarily now, of course. We should just keep
it in mind before the types get a lot of use, because it can be easier
to merge types (if they end up being unneeded) than to split them
later (double-checking each instance).

Thanks for all your feedback on this!

Cheers,
Miguel
  

On Wed, Feb 22, 2023 at 5:46 AM Asahi Lina <lina@asahilina.net> wrote:
>
> 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?

Yeah, I think that is the way to go -- that is also how C++ represents
time points as well (including libstdc++, libc++ and STL from a quick
look), as well as Abseil too.

Cheers,
Miguel
  

On Tue, 21 Feb 2023 21:33:42 +0000
Heghedus Razvan <heghedus.razvan@protonmail.com> wrote:

> On Tuesday, February 21st, 2023 at 8:45 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> > That's the same the Rust std time semantics:
> > 
> > Duration = Instance - Instance valid
> > Duration = Systemtime - SystemTime valid
> > Duration = Systemtime - Instance invalid
> > 
> > No?
> >   
> I agree with Thomas on this one. The Rust type system is really powerful and we should take advantage of it. Time deltas can be enforced to be from the same clock at compile time.
> Just for the sake of it, I wrote a small example on how this can be achieve: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=1d0f70bb5329b181f203ce7270e2957a
> 

`NowTime` don't need the type parameter. Here's a slightly more polished
version (also with names changed a bit so it looks more "Rusty"):

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=83702f491050da1c67ab9aa129103f7d

Best,
Gary