rust: time: Add clock read support

[PATCH 3/5] rust: time: Introduce clock reading framework

Posted by Boqun Feng 1 year, 10 months ago

To make sure Rust code doesn't mix timestamps from different clocks, a
type safe clock reading framework is introduced. It includes:

* A `Clock` trait that represents different clocks, to read a particular
  clock, one needs implement the `Clock::now()` function.

* A `Instant<Clock>` type that represents timestamps of different
  clocks, whose implementation is just a `ktime_t`, so all the
  calculation on `ktime_t` should apply to it as well.

Co-developed-by: Heghedus Razvan <heghedus.razvan@protonmail.com>
Signed-off-by: Heghedus Razvan <heghedus.razvan@protonmail.com>
Co-developed-by: Asahi Lina <lina@asahilina.net>
Signed-off-by: Asahi Lina <lina@asahilina.net>
Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
---
 rust/kernel/time.rs | 49 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
index b238b3a4e899..0f9f5605ed48 100644
--- a/rust/kernel/time.rs
+++ b/rust/kernel/time.rs
@@ -6,6 +6,9 @@
 //! have been ported or wrapped for usage by Rust code in the kernel.
 //!
 //! C header: [`include/linux/jiffies.h`](srctree/include/linux/jiffies.h).
+//! C header: [`include/linux/ktime.h`](srctree/include/linux/ktime.h).
+
+use core::marker::PhantomData;
 
 /// The number of nanoseconds per millisecond.
 pub const NSEC_PER_MSEC: i64 = bindings::NSEC_PER_MSEC as i64;
@@ -64,3 +67,49 @@ pub fn to_ns(self) -> i64 {
         self.inner
     }
 }
+
+/// Represents a clock, that is, a unique time source and it can be queried for the current time.
+pub trait Clock: Sized {
+    /// Returns the current time for this clock.
+    fn now() -> Instant<Self>;
+}
+
+/// Marker trait for clock sources that are guaranteed to be monotonic.
+pub trait Monotonic {}
+
+/// A timestamp of a given [`Clock`]
+#[repr(transparent)]
+#[derive(Debug)]
+pub struct Instant<T: Clock> {
+    inner: bindings::ktime_t,
+    clock: PhantomData<T>,
+}
+
+impl<T: Clock> Clone for Instant<T> {
+    fn clone(&self) -> Self {
+        *self
+    }
+}
+
+impl<T: Clock> Copy for Instant<T> {}
+
+impl<T: Clock> Instant<T> {
+    fn new(ktime: bindings::ktime_t) -> Self {
+        Self {
+            inner: ktime,
+            clock: PhantomData,
+        }
+    }
+}
+
+impl<T: Clock> core::ops::Sub for Instant<T> {
+    type Output = Duration;
+
+    /// Returns the difference of two timestamps.
+    #[inline]
+    fn sub(self, other: Self) -> Self::Output {
+        // `ktime_t` is an `i64` value of nanoseconds, and kernel defines signed overflow to behave
+        // like 2s-complement, hence `wrapping_sub()` is used here to mirror `ktime_sub()`.
+        Duration::new(self.inner.wrapping_sub(other.inner))
+    }
+}
-- 
2.44.0

Re: [PATCH 3/5] rust: time: Introduce clock reading framework

Posted by Benno Lossin 1 year, 10 months ago

On 24.03.24 23:33, Boqun Feng wrote:
> To make sure Rust code doesn't mix timestamps from different clocks, a
> type safe clock reading framework is introduced. It includes:
> 
> * A `Clock` trait that represents different clocks, to read a particular
>   clock, one needs implement the `Clock::now()` function.
> 
> * A `Instant<Clock>` type that represents timestamps of different
>   clocks, whose implementation is just a `ktime_t`, so all the
>   calculation on `ktime_t` should apply to it as well.
> 
> Co-developed-by: Heghedus Razvan <heghedus.razvan@protonmail.com>
> Signed-off-by: Heghedus Razvan <heghedus.razvan@protonmail.com>
> Co-developed-by: Asahi Lina <lina@asahilina.net>
> Signed-off-by: Asahi Lina <lina@asahilina.net>
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> ---
>  rust/kernel/time.rs | 49 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
> 
> diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
> index b238b3a4e899..0f9f5605ed48 100644
> --- a/rust/kernel/time.rs
> +++ b/rust/kernel/time.rs
> @@ -6,6 +6,9 @@
>  //! have been ported or wrapped for usage by Rust code in the kernel.
>  //!
>  //! C header: [`include/linux/jiffies.h`](srctree/include/linux/jiffies.h).
> +//! C header: [`include/linux/ktime.h`](srctree/include/linux/ktime.h).
> +
> +use core::marker::PhantomData;
> 
>  /// The number of nanoseconds per millisecond.
>  pub const NSEC_PER_MSEC: i64 = bindings::NSEC_PER_MSEC as i64;
> @@ -64,3 +67,49 @@ pub fn to_ns(self) -> i64 {
>          self.inner
>      }
>  }
> +
> +/// Represents a clock, that is, a unique time source and it can be queried for the current time.
> +pub trait Clock: Sized {
> +    /// Returns the current time for this clock.
> +    fn now() -> Instant<Self>;
> +}
> +
> +/// Marker trait for clock sources that are guaranteed to be monotonic.
> +pub trait Monotonic {}

Why is this trait not an extension of `Clock`?

> +
> +/// A timestamp of a given [`Clock`]

Missing '.'.

> +#[repr(transparent)]
> +#[derive(Debug)]
> +pub struct Instant<T: Clock> {
> +    inner: bindings::ktime_t,
> +    clock: PhantomData<T>,
> +}
> +
> +impl<T: Clock> Clone for Instant<T> {
> +    fn clone(&self) -> Self {
> +        *self
> +    }
> +}
> +
> +impl<T: Clock> Copy for Instant<T> {}
> +
> +impl<T: Clock> Instant<T> {
> +    fn new(ktime: bindings::ktime_t) -> Self {

When compiling, this function is marked as dead-code in this patch.

-- 
Cheers,
Benno

> +        Self {
> +            inner: ktime,
> +            clock: PhantomData,
> +        }
> +    }
> +}
> +
> +impl<T: Clock> core::ops::Sub for Instant<T> {
> +    type Output = Duration;
> +
> +    /// Returns the difference of two timestamps.
> +    #[inline]
> +    fn sub(self, other: Self) -> Self::Output {
> +        // `ktime_t` is an `i64` value of nanoseconds, and kernel defines signed overflow to behave
> +        // like 2s-complement, hence `wrapping_sub()` is used here to mirror `ktime_sub()`.
> +        Duration::new(self.inner.wrapping_sub(other.inner))
> +    }
> +}
> --
> 2.44.0
>

Re: [PATCH 3/5] rust: time: Introduce clock reading framework

Posted by Boqun Feng 1 year, 10 months ago

On Tue, Mar 26, 2024 at 05:00:39PM +0000, Benno Lossin wrote:
> On 24.03.24 23:33, Boqun Feng wrote:
> > To make sure Rust code doesn't mix timestamps from different clocks, a
> > type safe clock reading framework is introduced. It includes:
> > 
> > * A `Clock` trait that represents different clocks, to read a particular
> >   clock, one needs implement the `Clock::now()` function.
> > 
> > * A `Instant<Clock>` type that represents timestamps of different
> >   clocks, whose implementation is just a `ktime_t`, so all the
> >   calculation on `ktime_t` should apply to it as well.
> > 
> > Co-developed-by: Heghedus Razvan <heghedus.razvan@protonmail.com>
> > Signed-off-by: Heghedus Razvan <heghedus.razvan@protonmail.com>
> > Co-developed-by: Asahi Lina <lina@asahilina.net>
> > Signed-off-by: Asahi Lina <lina@asahilina.net>
> > Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> > ---
> >  rust/kernel/time.rs | 49 +++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 49 insertions(+)
> > 
> > diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
> > index b238b3a4e899..0f9f5605ed48 100644
> > --- a/rust/kernel/time.rs
> > +++ b/rust/kernel/time.rs
> > @@ -6,6 +6,9 @@
> >  //! have been ported or wrapped for usage by Rust code in the kernel.
> >  //!
> >  //! C header: [`include/linux/jiffies.h`](srctree/include/linux/jiffies.h).
> > +//! C header: [`include/linux/ktime.h`](srctree/include/linux/ktime.h).
> > +
> > +use core::marker::PhantomData;
> > 
> >  /// The number of nanoseconds per millisecond.
> >  pub const NSEC_PER_MSEC: i64 = bindings::NSEC_PER_MSEC as i64;
> > @@ -64,3 +67,49 @@ pub fn to_ns(self) -> i64 {
> >          self.inner
> >      }
> >  }
> > +
> > +/// Represents a clock, that is, a unique time source and it can be queried for the current time.
> > +pub trait Clock: Sized {
> > +    /// Returns the current time for this clock.
> > +    fn now() -> Instant<Self>;
> > +}
> > +
> > +/// Marker trait for clock sources that are guaranteed to be monotonic.
> > +pub trait Monotonic {}
> 
> Why is this trait not an extension of `Clock`?
> 

This was carried over from the old version, for myself, it doesn't make
much difference between:

	trait A { .. }
	trait B { .. }

	impl<T: A + B> ...

vs

	trait A { .. }
	trait B: A { .. }

	impl<T: B> ...

hence I kept it as it is, but yes, it a `Monotonic` *`Clock`*, so I will
change it in the next version.

> > +
> > +/// A timestamp of a given [`Clock`]
> 
> Missing '.'.
> 

Fixed locally.

> > +#[repr(transparent)]
> > +#[derive(Debug)]
> > +pub struct Instant<T: Clock> {
> > +    inner: bindings::ktime_t,
> > +    clock: PhantomData<T>,
> > +}
> > +
> > +impl<T: Clock> Clone for Instant<T> {
> > +    fn clone(&self) -> Self {
> > +        *self
> > +    }
> > +}
> > +
> > +impl<T: Clock> Copy for Instant<T> {}
> > +
> > +impl<T: Clock> Instant<T> {
> > +    fn new(ktime: bindings::ktime_t) -> Self {
> 
> When compiling, this function is marked as dead-code in this patch.
> 

Hmm... I cannot trigger any compile errors with this patch. If you see
an error, could you share the build command? It's not a `pub` function
though.

Regards,
Boqun

> -- 
> Cheers,
> Benno
> 
> > +        Self {
> > +            inner: ktime,
> > +            clock: PhantomData,
> > +        }
> > +    }
> > +}
> > +
> > +impl<T: Clock> core::ops::Sub for Instant<T> {
> > +    type Output = Duration;
> > +
> > +    /// Returns the difference of two timestamps.
> > +    #[inline]
> > +    fn sub(self, other: Self) -> Self::Output {
> > +        // `ktime_t` is an `i64` value of nanoseconds, and kernel defines signed overflow to behave
> > +        // like 2s-complement, hence `wrapping_sub()` is used here to mirror `ktime_sub()`.
> > +        Duration::new(self.inner.wrapping_sub(other.inner))
> > +    }
> > +}
> > --
> > 2.44.0
> >

Re: [PATCH 3/5] rust: time: Introduce clock reading framework

Posted by Benno Lossin 1 year, 10 months ago

On 26.03.24 20:19, Boqun Feng wrote:
> On Tue, Mar 26, 2024 at 05:00:39PM +0000, Benno Lossin wrote:
>> On 24.03.24 23:33, Boqun Feng wrote:
>>> To make sure Rust code doesn't mix timestamps from different clocks, a
>>> type safe clock reading framework is introduced. It includes:
>>>
>>> * A `Clock` trait that represents different clocks, to read a particular
>>>    clock, one needs implement the `Clock::now()` function.
>>>
>>> * A `Instant<Clock>` type that represents timestamps of different
>>>    clocks, whose implementation is just a `ktime_t`, so all the
>>>    calculation on `ktime_t` should apply to it as well.
>>>
>>> Co-developed-by: Heghedus Razvan <heghedus.razvan@protonmail.com>
>>> Signed-off-by: Heghedus Razvan <heghedus.razvan@protonmail.com>
>>> Co-developed-by: Asahi Lina <lina@asahilina.net>
>>> Signed-off-by: Asahi Lina <lina@asahilina.net>
>>> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
>>> ---
>>>   rust/kernel/time.rs | 49 +++++++++++++++++++++++++++++++++++++++++++++
>>>   1 file changed, 49 insertions(+)
>>>
>>> diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
>>> index b238b3a4e899..0f9f5605ed48 100644
>>> --- a/rust/kernel/time.rs
>>> +++ b/rust/kernel/time.rs
>>> @@ -6,6 +6,9 @@
>>>   //! have been ported or wrapped for usage by Rust code in the kernel.
>>>   //!
>>>   //! C header: [`include/linux/jiffies.h`](srctree/include/linux/jiffies.h).
>>> +//! C header: [`include/linux/ktime.h`](srctree/include/linux/ktime.h).
>>> +
>>> +use core::marker::PhantomData;
>>>
>>>   /// The number of nanoseconds per millisecond.
>>>   pub const NSEC_PER_MSEC: i64 = bindings::NSEC_PER_MSEC as i64;
>>> @@ -64,3 +67,49 @@ pub fn to_ns(self) -> i64 {
>>>           self.inner
>>>       }
>>>   }
>>> +
>>> +/// Represents a clock, that is, a unique time source and it can be queried for the current time.
>>> +pub trait Clock: Sized {
>>> +    /// Returns the current time for this clock.
>>> +    fn now() -> Instant<Self>;
>>> +}
>>> +
>>> +/// Marker trait for clock sources that are guaranteed to be monotonic.
>>> +pub trait Monotonic {}
>>
>> Why is this trait not an extension of `Clock`?
>>
> 
> This was carried over from the old version, for myself, it doesn't make
> much difference between:
> 
> 	trait A { .. }
> 	trait B { .. }
> 
> 	impl<T: A + B> ...
> 
> vs
> 
> 	trait A { .. }
> 	trait B: A { .. }
> 
> 	impl<T: B> ...
> 
> hence I kept it as it is, but yes, it a `Monotonic` *`Clock`*, so I will
> change it in the next version.

I think it would also make sense to rename it to `MonotonicClock`.

> 
>>> +
>>> +/// A timestamp of a given [`Clock`]
>>
>> Missing '.'.
>>
> 
> Fixed locally.
> 
>>> +#[repr(transparent)]
>>> +#[derive(Debug)]
>>> +pub struct Instant<T: Clock> {
>>> +    inner: bindings::ktime_t,
>>> +    clock: PhantomData<T>,
>>> +}
>>> +
>>> +impl<T: Clock> Clone for Instant<T> {
>>> +    fn clone(&self) -> Self {
>>> +        *self
>>> +    }
>>> +}
>>> +
>>> +impl<T: Clock> Copy for Instant<T> {}
>>> +
>>> +impl<T: Clock> Instant<T> {
>>> +    fn new(ktime: bindings::ktime_t) -> Self {
>>
>> When compiling, this function is marked as dead-code in this patch.
>>
> 
> Hmm... I cannot trigger any compile errors with this patch. If you see
> an error, could you share the build command? It's not a `pub` function
> though.

I created a fresh config using `menuconfig` and enabled CONFIG_RUST.
Then I get this error:

    error: associated function `new` is never used
      --> rust/kernel/time.rs:97:8
       |
    96 | impl<T: Clock> Instant<T> {
       | ------------------------- associated function in this implementation
    97 |     fn new(ktime: bindings::ktime_t) -> Self {
       |        ^^^
       |
       = note: `-D dead-code` implied by `-D warnings`
       = help: to override `-D warnings` add `#[allow(dead_code)]`
    
    error: aborting due to 1 previous error

Note that the error vanishes in the next patch, since there you do have
a user for `new`.

-- 
Cheers,
Benno

Re: [PATCH 3/5] rust: time: Introduce clock reading framework

Posted by Boqun Feng 1 year, 10 months ago

On Wed, Mar 27, 2024 at 12:50:50PM +0000, Benno Lossin wrote:
> On 26.03.24 20:19, Boqun Feng wrote:
> > On Tue, Mar 26, 2024 at 05:00:39PM +0000, Benno Lossin wrote:
> >> On 24.03.24 23:33, Boqun Feng wrote:
> >>> To make sure Rust code doesn't mix timestamps from different clocks, a
> >>> type safe clock reading framework is introduced. It includes:
> >>>
> >>> * A `Clock` trait that represents different clocks, to read a particular
> >>>    clock, one needs implement the `Clock::now()` function.
> >>>
> >>> * A `Instant<Clock>` type that represents timestamps of different
> >>>    clocks, whose implementation is just a `ktime_t`, so all the
> >>>    calculation on `ktime_t` should apply to it as well.
> >>>
> >>> Co-developed-by: Heghedus Razvan <heghedus.razvan@protonmail.com>
> >>> Signed-off-by: Heghedus Razvan <heghedus.razvan@protonmail.com>
> >>> Co-developed-by: Asahi Lina <lina@asahilina.net>
> >>> Signed-off-by: Asahi Lina <lina@asahilina.net>
> >>> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> >>> ---
> >>>   rust/kernel/time.rs | 49 +++++++++++++++++++++++++++++++++++++++++++++
> >>>   1 file changed, 49 insertions(+)
> >>>
> >>> diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
> >>> index b238b3a4e899..0f9f5605ed48 100644
> >>> --- a/rust/kernel/time.rs
> >>> +++ b/rust/kernel/time.rs
> >>> @@ -6,6 +6,9 @@
> >>>   //! have been ported or wrapped for usage by Rust code in the kernel.
> >>>   //!
> >>>   //! C header: [`include/linux/jiffies.h`](srctree/include/linux/jiffies.h).
> >>> +//! C header: [`include/linux/ktime.h`](srctree/include/linux/ktime.h).
> >>> +
> >>> +use core::marker::PhantomData;
> >>>
> >>>   /// The number of nanoseconds per millisecond.
> >>>   pub const NSEC_PER_MSEC: i64 = bindings::NSEC_PER_MSEC as i64;
> >>> @@ -64,3 +67,49 @@ pub fn to_ns(self) -> i64 {
> >>>           self.inner
> >>>       }
> >>>   }
> >>> +
> >>> +/// Represents a clock, that is, a unique time source and it can be queried for the current time.
> >>> +pub trait Clock: Sized {
> >>> +    /// Returns the current time for this clock.
> >>> +    fn now() -> Instant<Self>;
> >>> +}
> >>> +
> >>> +/// Marker trait for clock sources that are guaranteed to be monotonic.
> >>> +pub trait Monotonic {}
> >>
> >> Why is this trait not an extension of `Clock`?
> >>
> > 
> > This was carried over from the old version, for myself, it doesn't make
> > much difference between:
> > 
> > 	trait A { .. }
> > 	trait B { .. }
> > 
> > 	impl<T: A + B> ...
> > 
> > vs
> > 
> > 	trait A { .. }
> > 	trait B: A { .. }
> > 
> > 	impl<T: B> ...
> > 
> > hence I kept it as it is, but yes, it a `Monotonic` *`Clock`*, so I will
> > change it in the next version.
> 
> I think it would also make sense to rename it to `MonotonicClock`.
> 
> > 
> >>> +
> >>> +/// A timestamp of a given [`Clock`]
> >>
> >> Missing '.'.
> >>
> > 
> > Fixed locally.
> > 
> >>> +#[repr(transparent)]
> >>> +#[derive(Debug)]
> >>> +pub struct Instant<T: Clock> {
> >>> +    inner: bindings::ktime_t,
> >>> +    clock: PhantomData<T>,
> >>> +}
> >>> +
> >>> +impl<T: Clock> Clone for Instant<T> {
> >>> +    fn clone(&self) -> Self {
> >>> +        *self
> >>> +    }
> >>> +}
> >>> +
> >>> +impl<T: Clock> Copy for Instant<T> {}
> >>> +
> >>> +impl<T: Clock> Instant<T> {
> >>> +    fn new(ktime: bindings::ktime_t) -> Self {
> >>
> >> When compiling, this function is marked as dead-code in this patch.
> >>
> > 
> > Hmm... I cannot trigger any compile errors with this patch. If you see
> > an error, could you share the build command? It's not a `pub` function
> > though.
> 
> I created a fresh config using `menuconfig` and enabled CONFIG_RUST.
> Then I get this error:
> 
>     error: associated function `new` is never used
>       --> rust/kernel/time.rs:97:8
>        |
>     96 | impl<T: Clock> Instant<T> {
>        | ------------------------- associated function in this implementation
>     97 |     fn new(ktime: bindings::ktime_t) -> Self {
>        |        ^^^
>        |
>        = note: `-D dead-code` implied by `-D warnings`
>        = help: to override `-D warnings` add `#[allow(dead_code)]`
>     
>     error: aborting due to 1 previous error
> 
> Note that the error vanishes in the next patch, since there you do have
> a user for `new`.
> 

Seems I didn't select CONFIG_WERROR=y, so I missed it as a warning.
and I can hit it now with that config. Will fix it in the next version.
Thanks!

Regards

> -- 
> Cheers,
> Benno

[PATCH 1/5] rust: time: doc: Add missing C header link to jiffies
[PATCH 2/5] rust: time: Introduce Duration type
[PATCH 3/5] rust: time: Introduce clock reading framework
[PATCH 4/5] rust: time: Support reading CLOCK_MONOTONIC
[PATCH 5/5] rust: time: Add Instant::elapsed() for monotonic clocks