Re: [PATCH v11 4/8] rust: time: Introduce Instant type

[Andreas Hindborg <a.hindborg@kernel.org>]

FUJITA Tomonori <fujita.tomonori@gmail.com> writes:

> Introduce a type representing a specific point in time. We could use
> the Ktime type but C's ktime_t is used for both timestamp and
> timedelta. To avoid confusion, introduce a new Instant type for
> timestamp.
>
> Rename Ktime to Instant and modify their methods for timestamp.
>
> Implement the subtraction operator for Instant:
>
> Delta = Instant A - Instant B
>
> Tested-by: Daniel Almeida <daniel.almeida@collabora.com>
> Reviewed-by: Boqun Feng <boqun.feng@gmail.com>
> Reviewed-by: Gary Guo <gary@garyguo.net>
> Reviewed-by: Fiona Behrens <me@kloenk.dev>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>


Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>


As Boqun mentioned, we should make this generic over `ClockId` when the
hrtimer patches land.

One question regarding overflow below.

> ---
>  rust/kernel/time.rs | 77 +++++++++++++++++++++++----------------------
>  1 file changed, 39 insertions(+), 38 deletions(-)
>
> diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
> index 622cd01e24d7..d64a05a4f4d1 100644
> --- a/rust/kernel/time.rs
> +++ b/rust/kernel/time.rs
> @@ -5,6 +5,22 @@
>  //! This module contains the kernel APIs related to time and timers that
>  //! have been ported or wrapped for usage by Rust code in the kernel.
>  //!
> +//! There are two types in this module:
> +//!
> +//! - The [`Instant`] type represents a specific point in time.
> +//! - The [`Delta`] type represents a span of time.
> +//!
> +//! Note that the C side uses `ktime_t` type to represent both. However, timestamp
> +//! and timedelta are different. To avoid confusion, we use two different types.
> +//!
> +//! A [`Instant`] object can be created by calling the [`Instant::now()`] function.
> +//! It represents a point in time at which the object was created.
> +//! By calling the [`Instant::elapsed()`] method, a [`Delta`] object representing
> +//! the elapsed time can be created. The [`Delta`] object can also be created
> +//! by subtracting two [`Instant`] objects.
> +//!
> +//! A [`Delta`] type supports methods to retrieve the duration in various units.
> +//!
>  //! C header: [`include/linux/jiffies.h`](srctree/include/linux/jiffies.h).
>  //! C header: [`include/linux/ktime.h`](srctree/include/linux/ktime.h).
>  
> @@ -31,59 +47,44 @@ pub fn msecs_to_jiffies(msecs: Msecs) -> Jiffies {
>      unsafe { bindings::__msecs_to_jiffies(msecs) }
>  }
>  
> -/// A Rust wrapper around a `ktime_t`.
> +/// A specific point in time.
> +///
> +/// # Invariants
> +///
> +/// The `inner` value is in the range from 0 to `KTIME_MAX`.
>  #[repr(transparent)]
>  #[derive(Copy, Clone, PartialEq, PartialOrd, Eq, Ord)]
> -pub struct Ktime {
> +pub struct Instant {
>      inner: bindings::ktime_t,
>  }
>  
> -impl Ktime {
> -    /// Create a `Ktime` from a raw `ktime_t`.
> -    #[inline]
> -    pub fn from_raw(inner: bindings::ktime_t) -> Self {
> -        Self { inner }
> -    }
> -
> +impl Instant {
>      /// Get the current time using `CLOCK_MONOTONIC`.
>      #[inline]
> -    pub fn ktime_get() -> Self {
> -        // SAFETY: It is always safe to call `ktime_get` outside of NMI context.
> -        Self::from_raw(unsafe { bindings::ktime_get() })
> -    }
> -
> -    /// Divide the number of nanoseconds by a compile-time constant.
> -    #[inline]
> -    fn divns_constant<const DIV: i64>(self) -> i64 {
> -        self.to_ns() / DIV
> -    }
> -
> -    /// Returns the number of nanoseconds.
> -    #[inline]
> -    pub fn to_ns(self) -> i64 {
> -        self.inner
> +    pub fn now() -> Self {
> +        // INVARIANT: The `ktime_get()` function returns a value in the range
> +        // from 0 to `KTIME_MAX`.
> +        Self {
> +            // SAFETY: It is always safe to call `ktime_get()` outside of NMI context.
> +            inner: unsafe { bindings::ktime_get() },
> +        }
>      }
>  
> -    /// Returns the number of milliseconds.
> +    /// Return the amount of time elapsed since the [`Instant`].
>      #[inline]
> -    pub fn to_ms(self) -> i64 {
> -        self.divns_constant::<NSEC_PER_MSEC>()
> +    pub fn elapsed(&self) -> Delta {
> +        Self::now() - *self
>      }
>  }
>  
> -/// Returns the number of milliseconds between two ktimes.
> -#[inline]
> -pub fn ktime_ms_delta(later: Ktime, earlier: Ktime) -> i64 {
> -    (later - earlier).to_ms()
> -}
> -
> -impl core::ops::Sub for Ktime {
> -    type Output = Ktime;
> +impl core::ops::Sub for Instant {
> +    type Output = Delta;
>  
> +    // By the type invariant, it never overflows.
>      #[inline]
> -    fn sub(self, other: Ktime) -> Ktime {
> -        Self {
> -            inner: self.inner - other.inner,
> +    fn sub(self, other: Instant) -> Delta {
> +        Delta {
> +            nanos: self.inner - other.inner,

If this never overflows by invariant, would it make sense to use
`unchecked_sub` or `wraping_sub`? That would remove the overflow check.


Best regards,
Andreas Hindborg