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

[Alice Ryhl <aliceryhl@google.com>]

On Thu, Jul 24, 2025 at 02:54:07PM -0400, 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.
> 
> Note that for division and remainders, we currently limit these operations
> to CONFIG_64BIT as u64 / u64 and u64 % u64 is not supported on all 32 bit
> platforms natively. The correct solution we want to aim for here in the
> future is to use the kernel's math library for performing these operations
> so they're emulated on 32 bit platforms.

The CONFIG_64BIT restriction seems annoying. Could we not support 32-bit
from the get-go? Where is this going to be used?

After all, we have stuff like this:
https://lore.kernel.org/r/20250724165441.2105632-1-ojeda@kernel.org

> +impl ops::Mul for Delta {
> +    type Output = Self;
> +
> +    fn mul(self, rhs: Self) -> Self::Output {
> +        Self {
> +            nanos: self.nanos * rhs.nanos,
> +        }
> +    }
> +}
> +
> +impl ops::MulAssign for Delta {
> +    fn mul_assign(&mut self, rhs: Self) {
> +        self.nanos *= rhs.nanos;
> +    }
> +}

The units here do not make sense. I would not add multiplication of
Delta*Delta. It makes sense to have Delta*int, but it does not make
sense to multiply two Deltas together.

I would change the second type for both multiplication operators to be a
normal integer.

> +// TODO: When we get support for u64/u64 division and remainders helpers remove this, until then
> +// these ops only work on 64bit platforms.
> +#[cfg(CONFIG_64BIT)]
> +impl ops::Div for Delta {
> +    type Output = Self;
> +
> +    fn div(self, rhs: Self) -> Self::Output {
> +        Self {
> +            nanos: self.nanos / rhs.nanos,
> +        }
> +    }
> +}
> +
> +#[cfg(CONFIG_64BIT)]
> +impl ops::DivAssign for Delta {
> +    fn div_assign(&mut self, rhs: Self) {
> +        self.nanos /= rhs.nanos;
> +    }
> +}

Same here. The units don't work. If you divide two deltas by each other,
the correct unit is to return a kind of integer, not another Delta.

I would change Div to have an integer type as output and get rid of
DivAssign.

> +#[cfg(CONFIG_64BIT)]
> +impl ops::Rem for Delta {
> +    type Output = Self;
> +
> +    fn rem(self, rhs: Self) -> Self::Output {
> +        Self {
> +            nanos: self.nanos % rhs.nanos,
> +        }
> +    }
> +}
> +
> +#[cfg(CONFIG_64BIT)]
> +impl ops::RemAssign for Delta {
> +    fn rem_assign(&mut self, rhs: Self) {
> +        self.nanos %= rhs.nanos;
> +    }
> +}

The units here do make sense, so these are fine.

Alice