Re: [PATCH v5 1/7] rust: time: make Delta generic over its time unit

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

FUJITA Tomonori <tomo@flapping.org> writes:

> From: FUJITA Tomonori <fujita.tomonori@gmail.com>
>
> Delta hardcodes its value as i64 nanoseconds. A later patch adds a
> jiffies span, whose natural representation is isize jiffies rather than
> i64 nanoseconds, and a separate type per unit would duplicate the
> arithmetic and comparison machinery.
>
> Make Delta generic over its time unit so the jiffies span can reuse that
> machinery. The nanosecond Delta keeps its current representation and API
> via the default unit parameter, so no functional change.
> users.
>
> Reviewed-by: Gary Guo <gary@garyguo.net>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> ---
>  rust/kernel/time.rs | 72 ++++++++++++++++++++++++++++++---------------
>  1 file changed, 48 insertions(+), 24 deletions(-)
>
> diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
> index b8463823aed9..589bd7827523 100644
> --- a/rust/kernel/time.rs
> +++ b/rust/kernel/time.rs
> @@ -246,7 +246,7 @@ impl<C: ClockSource> ops::Sub for Instant<C> {
>      #[inline]
>      fn sub(self, other: Instant<C>) -> Delta {
>          Delta {
> -            nanos: self.inner - other.inner,
> +            value: self.inner - other.inner,
>          }
>      }
>  }
> @@ -258,7 +258,7 @@ impl<T: ClockSource> ops::Add<Delta> for Instant<T> {
>      fn add(self, rhs: Delta) -> Self::Output {
>          // INVARIANT: With arithmetic over/underflow checks enabled, this will panic if we overflow
>          // (e.g. go above `KTIME_MAX`)
> -        let res = self.inner + rhs.nanos;
> +        let res = self.inner + rhs.value;
>  
>          // INVARIANT: With overflow checks enabled, we verify here that the value is >= 0
>          #[cfg(CONFIG_RUST_OVERFLOW_CHECKS)]
> @@ -278,7 +278,7 @@ impl<T: ClockSource> ops::Sub<Delta> for Instant<T> {
>      fn sub(self, rhs: Delta) -> Self::Output {
>          // INVARIANT: With arithmetic over/underflow checks enabled, this will panic if we overflow
>          // (e.g. go above `KTIME_MAX`)
> -        let res = self.inner - rhs.nanos;
> +        let res = self.inner - rhs.value;
>  
>          // INVARIANT: With overflow checks enabled, we verify here that the value is >= 0
>          #[cfg(CONFIG_RUST_OVERFLOW_CHECKS)]
> @@ -291,14 +291,38 @@ fn sub(self, rhs: Delta) -> Self::Output {
>      }
>  }
>  
> +mod private {
> +    pub trait Sealed {}
> +
> +    impl Sealed for super::Nsec {}
> +}
> +
> +/// A trait for time units.
> +pub trait TimeUnit: private::Sealed {
> +    /// The underlying representation of the time unit.
> +    type Repr: Copy + Clone + PartialEq + PartialOrd + Eq + Ord + core::fmt::Debug;
> +}
> +
> +/// A time unit of nanoseconds.
> +///
> +/// A [`Delta<Nsec>`] stores its value as `i64` nanoseconds and can represent
> +/// any `i64` value, including negative, zero, and positive numbers.
> +#[derive(Copy, Clone, PartialEq, PartialOrd, Eq, Ord, Debug)]
> +pub struct Nsec;

Should we make these enum with zero variants to indicate they should not
be constructed?

> +
> +impl TimeUnit for Nsec {
> +    type Repr = i64;
> +}
> +
>  /// A span of time.
>  ///
> -/// This struct represents a span of time, with its value stored as nanoseconds.
> -/// The value can represent any valid i64 value, including negative, zero, and
> -/// positive numbers.
> +/// The span is stored in the unit given by the type parameter `U` (see
> +/// [`TimeUnit`]); its value has type `U::Repr`. `U` defaults to [`Nsec`], so a
> +/// plain [`Delta`] is a span in nanoseconds. The value can be negative, zero, or
> +/// positive.
>  #[derive(Copy, Clone, PartialEq, PartialOrd, Eq, Ord, Debug)]
> -pub struct Delta {
> -    nanos: i64,
> +pub struct Delta<U: TimeUnit = Nsec> {
> +    value: U::Repr,
>  }
>  
>  impl ops::Add for Delta {

When you add `Jiffy` later, this impl block will only cover
`Delta<Nsec>`. Is that intentional, or did you intend to support
all these operations operations for `Delta<Jiffy>` as well?

Best regards,
Andreas Hindborg