Re: [PATCH v2 2/2] rust: time: Implement basic arithmetic operations for Delta

["Benno Lossin" <lossin@kernel.org>]

On Thu Aug 7, 2025 at 9:06 PM CEST, Lyude Paul wrote:
> While rvkms is only going to be using a few of these, since Deltas are
> basically the same as i64 it's easy enough to just implement all of the
> basic arithmetic operations for Delta types.
>
> Keep in mind there's one quirk here - the kernel has no support for
> i64 % i64 on 32 bit platforms, the closest we have is i64 % i32 through
> div_s64_rem(). So, instead of implementing ops::Rem or ops::RemAssign we
> simply provide Delta::rem_nanos().

We could still provide the trait implementations on CONFIG_64BIT? WDYT?

> +impl ops::Div for Delta {
> +    type Output = Self;
> +
> +    #[inline]
> +    fn div(self, rhs: Self) -> Self::Output {
> +        #[cfg(CONFIG_64BIT)]
> +        {

This pattern seems to be rather common in this patchset & in general I
think I've also seen it elsewhere. We should think about adding a
`if_cfg!` macro:

    Self {
        nanos: if_cfg! {
            if CONFIG_64BIT {
                self.nanos / rhs.nanos
            } else {
                unsafe { ... }
            }
        },
    }

But we can do that later. I'll file a good-first-issue.

> +            Self {
> +                nanos: self.nanos / rhs.nanos,
> +            }
> +        }
> +
> +        #[cfg(not(CONFIG_64BIT))]
> +        {
> +            Self {
> +                // SAFETY: This function is always safe to call regardless of the input values
> +                nanos: unsafe { bindings::div64_s64(self.nanos, rhs.nanos) },
> +            }
> +        }
> +    }
> +}
> +
> +impl ops::DivAssign for Delta {
> +    #[inline]
> +    fn div_assign(&mut self, rhs: Self) {
> +        self.nanos = self.nanos / rhs.nanos
> +    }
> +}
> +
>  impl Delta {
>      /// A span of time equal to zero.
>      pub const ZERO: Self = Self { nanos: 0 };
> @@ -353,4 +436,30 @@ pub fn as_millis(self) -> i64 {
>              bindings::ktime_to_ms(self.as_nanos())
>          }
>      }
> +
> +    /// Return `self % dividend` where `dividend` is in nanoseconds.
> +    ///
> +    /// The kernel doesn't have any emulation for `s64 % s64` on 32 bit platforms, so this is

I would say `i64` instead of `s64`.

> +    /// limited to 32 bit dividends.
> +    #[inline]
> +    pub fn rem_nanos(self, ns: i32) -> Self {
> +        #[cfg(CONFIG_64BIT)]
> +        {
> +            Self {
> +                nanos: self.as_nanos() % i64::from(ns),
> +            }
> +        }
> +
> +        #[cfg(not(CONFIG_64BIT))]
> +        {
> +            let mut rem = 0;
> +
> +            // SAFETY: `rem` is in the stack, so we can always provide a valid pointer to it.

I'd just say "`&mut rem` is a valid pointer.".

---
Cheers,
Benno

> +            unsafe { bindings::div_s64_rem(self.as_nanos(), ns, &mut rem) };
> +
> +            Self {
> +                nanos: i64::from(rem),
> +            }
> +        }
> +    }
>  }