Re: [PATCH v2 17/21] rust: num: Add an upward alignment helper for usize

[Joel Fernandes <joelagnelf@nvidia.com>]

Hello, Alexandre,

On 5/3/2025 10:37 AM, Alexandre Courbot wrote:
> On Sat May 3, 2025 at 12:02 PM JST, Joel Fernandes wrote:
>>
>>
>> On 5/2/2025 9:59 PM, Alexandre Courbot wrote:
>>>> pub trait AlignUp {
>>>>     fn align_up(self, alignment: Self) -> Self;
>>>> }
>>>>
>>>> macro_rules! align_up_impl {
>>>>     ($($t:ty),+) => {
>>>>         $(
>>>>             impl AlignUp for $t {
>>>>                 fn align_up(self, alignment: Self) -> Self {
>>>>                     (self + alignment - 1) & !(alignment - 1)
>>>>                 }
>>>>             }
>>>>         )+
>>>>     }
>>>> }
>>>>
>>>> align_up_impl!(usize, u8, u16, u32, u64, u128);
>>>>
>>>> Or, we can even combine the 2 approaches. Use macros for the "impl Alignable"
>>>> and use generics on the Alignable trait.
>>>>
>>>> macro_rules! impl_alignable {
>>>>     ($($t:ty),+) => {
>>>>         $(
>>>>             impl Alignable for $t {}
>>>>         )+
>>>>     };
>>>> }
>>>>
>>>> impl_alignable!(usize, u8, u16, u32, u64, u128);
>>>>
>>>> pub trait AlignUp {
>>>>     fn align_up(self, alignment: Self) -> Self;
>>>> }
>>>>
>>>> impl<T> AlignUp for T
>>>> where
>>>>     T: Alignable,
>>>> {
>>>>     fn align_up(self, alignment: Self) -> Self {
>>>>         let one = T::from(1u8);
>>>>         (self + alignment - one) & !(alignment - one)
>>>>     }
>>>> }
>>>>
>>>> Thoughts?
>>> I think that's the correct way to do it and am fully on board with this
>>> approach.
>>>
>>> The only thing this doesn't solve is that it doesn't provide `const`
>>> functions. But maybe for that purpose we can use a single macro that
>>> nicely panics at build-time should an overflow occur.
>>
>> Great, thanks. I split the traits as follows and it is cleaner and works. I will
>> look into the build-time overflow check and the returning of Result change on
>> Monday. Let me know if any objections.
> 
> Looking good IMHO, apart maybe from the names of the `BitOps` and
> `Unsigned` traits that are not super descriptive and don't need to be
> split for the moment anyway.

Sounds good, actually I already switched to keeping them in one trait
"Unsigned". I agree that is cleaner (see below).

> Actually it may be a good idea to move this into its own patch/series so
> it gets more attention as this is starting to look like the `num` or
> `num_integer` crates and we might be well-advised to take more
> inspiration from them in order to avoid reinventing the wheel. It is
> basically asking the question "how do we want to extend the integer
> types in a useful way for the kernel", so it's actually pretty important
> that we get our answer right. :)

I am not sure if we want to split the series for a simple change like this,
because then the whole series gets blocked? It may also be better to pair the
user of the function with the function itself IMHO since the function is also
quite small. I am also Ok with keeping the original patch in the series and
extending on that in the future (with just usize) to not block the series.

Regarding for the full blown num module, I looked over the weekend and its
actually a bunch of modules working together, with dozens of numeric APIs, so I
am not sure if we should pull everything or try to copy parts of it. The R4l
guidelines have something to say here. A good approach IMO is to just do it
incrementally, like I'm doing with this patch.

I think defining a "Unsigned" trait does make sense, and then for future
expansion, it can be expanded on in the new num module?

> 
> To address our immediate needs of an `align_up`, it just occurred to me
> that we could simply use the `next_multiple_of` method, at least
> temporarily. It is implemented with a modulo and will therefore probably
> result in less efficient code than a version optimized for powers of
> two, but it will do the trick until we figure out how we want to extend
> the primitive types for the kernel, which is really what this patch is
> about - we will also need an `align_down` for instance, and I don't know
> of a standard library equivalent for it...

Why do we want to trade off for "less efficient code"? :) I think that's worse
than the original change (before this series) I had which had no function call
at all, but hardcoded the expression at the call site. The suggestion is also
less desirable than having a local helper in the vbios module itself. I am not
much a fan of the idea "lets call this temporarily and have sub optimal code"
when the alternative is to just do it in-place, in-module, or via a num module
extension :)

> 
>> I added the #[inline] and hopefully that
>> gives similar benefits to const that you're seeking:
> 
> A `const` version is still going to be needed, `#[inline]` encourages the
> compiler to try and inline the function, but AFAIK it doesn't allow use
> in const context.

Right, so for the vbios use case there is no use of a const function. The only
reason I added it is because there were other functions at the time which were
used (by the now dropped timer module). I suggest let us add the const function
once there is a user of it, I also don't know right how to do it. Like if I use
generics for the const fn, I get this:

const fn align_up_unsigned<T: Unsigned>(value: T, alignment: T) -> T {
    let one = T::from(1u8);
    (value + alignment - one) & !(alignment - one)
}

error[E0658]: cannot call conditionally-const method `<T as Add>::add` in
constant functions

I tried to do this with macros as well, but no luck. If you can share a macro, I
can incorporate it into the patch.

I can respin this patch again on conclusion of the discussion, but any guidance
from rust-for-linux folks is also much appreciated. Below is currently the patch
that I am considering so far (without the const function and Result changes).

// num.rs
//! Numerical and binary utilities for primitive types.

/// A trait providing alignment operations for `usize`.
use core::ops::{Add, BitAnd, BitOr, Not, Sub};

/// Traits for unsigned integers
pub trait Unsigned:
    Copy
    + BitAnd<Output = Self>
    + BitOr<Output = Self>
    + Not<Output = Self>
    + Add<Output = Self>
    + Sub<Output = Self>
    + From<u8>
{
}

macro_rules! unsigned_trait_impl {
    ($($t:ty),+) => {
        $(
            impl Unsigned for $t {}
        )+
    };
}
unsigned_trait_impl!(usize, u8, u16, u32, u64, u128);

/// Trait for unsigned integer alignment
pub trait UnsignedAlign {
    /// Implement upward power-of-2 alignment for unsigned ints
    fn align_up(self, alignment: Self) -> Self;
}

impl<T> UnsignedAlign for T
where
    T: Unsigned,
{
    #[inline]
    fn align_up(self, alignment: Self) -> Self {
        let one = T::from(1u8);
        (self + alignment - one) & !(alignment - one)
    }
}

Thanks.