Re: [PATCH v4 04/20] rust: add new `num` module with useful integer operations

["Alexandre Courbot" <acourbot@nvidia.com>]

On Thu May 29, 2025 at 4:27 PM JST, Benno Lossin wrote:
> On Thu May 29, 2025 at 3:18 AM CEST, Alexandre Courbot wrote:
>> On Thu May 29, 2025 at 5:17 AM JST, Benno Lossin wrote:
>>> On Wed May 21, 2025 at 8:44 AM CEST, Alexandre Courbot wrote:
>>>> Introduce the `num` module, featuring the `NumExt` extension trait
>>>> that expands unsigned integers with useful operations for the kernel.
>>>>
>>>> These are to be used by the nova-core driver, but they are so ubiquitous
>>>> that other drivers should be able to take advantage of them as well.
>>>>
>>>> The currently implemented operations are:
>>>>
>>>> - align_down()
>>>> - align_up()
>>>> - fls()
>>>>
>>>> But this trait is expected to be expanded further.
>>>>
>>>> `NumExt` is on unsigned types using a macro. An approach using another
>>>> trait constrained by the operator traits that we need (`Add`, `Sub`,
>>>> etc) was also considered, but had to be dropped as we need to use
>>>> wrapping operations, which are not provided by any trait.
>>>>
>>>> Co-developed-by: Joel Fernandes <joelagnelf@nvidia.com>
>>>> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
>>>> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
>>>> ---
>>>>  rust/kernel/lib.rs |  1 +
>>>>  rust/kernel/num.rs | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>  2 files changed, 83 insertions(+)
>>>
>>> Have you proposed `align_down` to upstream rust? Not saying that we
>>> shouldn't do it here, but if we haven't tried yet, it might be a good
>>> idea to just get them upstreamed. (if you do, it should probably be
>>> named `prev_multiple_of`)
>>
>> I haven't yet - haven't ever contributed anything to upstream Rust, so
>> I'll have to look that one up first. :) But I agree a `prev_multiple_of`
>> could be useful.
>
> I'd recommend opening a thread on Zulip before you go implement stuff.
> Then you can also get a more rusty name for `fls` :)
>
>>>> +    /// Align `self` up to `alignment`.
>>>> +    ///
>>>> +    /// `alignment` must be a power of 2 for accurate results.
>>>> +    ///
>>>> +    /// Wraps around to `0` if the requested alignment pushes the result above the type's limits.
>>>> +    ///
>>>> +    /// # Examples
>>>> +    ///
>>>> +    /// ```
>>>> +    /// use kernel::num::NumExt;
>>>> +    ///
>>>> +    /// assert_eq!(0x4fffu32.align_up(0x1000), 0x5000);
>>>> +    /// assert_eq!(0x4000u32.align_up(0x1000), 0x4000);
>>>> +    /// assert_eq!(0x0u32.align_up(0x1000), 0x0);
>>>> +    /// assert_eq!(0xffffu16.align_up(0x100), 0x0);
>>>> +    /// assert_eq!(0x4fffu32.align_up(0x0), 0x0);
>>>> +    /// ```
>>>> +    fn align_up(self, alignment: Self) -> Self;
>>>
>>> Isn't this `next_multiple_of` [1] (it also allows non power of 2
>>> inputs).
>>>
>>> [1]: https://doc.rust-lang.org/std/primitive.u32.html#method.next_multiple_of
>>
>> It is, however the fact that `next_multiple_of` works with non powers of
>> two also means it needs to perform a modulo operation. That operation
>> might well be optimized away by the compiler, but ACAICT we have no way
>> of proving it will always be the case, hence the always-optimal
>> implementation here.
>
> When you use a power of 2 constant, then I'm very sure that it will get
> optimized [1]. Even with non-powers of 2, you don't get a division [2].
> If you find some code that is not optimized, then sure add a custom
> function.
>
> [1]: https://godbolt.org/z/57M9e36T3
> [2]: https://godbolt.org/z/9P4P8zExh

That's impressive and would definitely work well with a constant. But
when the value is not known at compile-time, the division does occur
unfortunately: https://godbolt.org/z/WK1bPMeEx

So I think we will still need a kernel-optimized version of these
alignment functions.

>
>> Also in the kernel we tend to use the `align` nomenclature and I think we
>> should preserve that for clarity.
>
> That's also fair, but we lose the constness of `next_multiple_of`, so
> you can't use `align_up` in a const function. That might confuse people
> and then they write their own const helper function... I'd prefer we use
> all functions that are available in the stdlib.

We definitely want const variants of these, one way or the other (const
methods in traits are not available yet unfortunately). And yes, on
principle I am aligned (haha) with using stdlib functions when possible.