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

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

On Thu May 1, 2025 at 9:58 PM JST, Alexandre Courbot wrote:
> From: Joel Fernandes <joelagnelf@nvidia.com>
>
> This will be used in the nova-core driver where we need to upward-align
> the image size to get to the next image in the VBIOS ROM.
>
> [acourbot@nvidia.com: handled conflicts due to removal of patch creating
> num.rs]
>
> 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 | 21 +++++++++++++++++++++
>  2 files changed, 22 insertions(+)
>
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index ab0286857061d2de1be0279cbd2cd3490e5a48c3..be75b196aa7a29cf3eed7c902ed8fb98689bbb50 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -67,6 +67,7 @@
>  pub mod miscdevice;
>  #[cfg(CONFIG_NET)]
>  pub mod net;
> +pub mod num;
>  pub mod of;
>  pub mod page;
>  #[cfg(CONFIG_PCI)]
> diff --git a/rust/kernel/num.rs b/rust/kernel/num.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..953c6ab012601efb3c9387b4b299e22233670c4b
> --- /dev/null
> +++ b/rust/kernel/num.rs
> @@ -0,0 +1,21 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Numerical and binary utilities for primitive types.
> +
> +/// A trait providing alignment operations for `usize`.
> +pub trait UsizeAlign {
> +    /// Aligns `self` upwards to the nearest multiple of `align`.
> +    fn align_up(self, align: usize) -> usize;
> +}

As it turns out I will also need the same functionality for u64 in a
future patch. :) Could we turn this into a more generic trait? E.g:

    /// A trait providing alignment operations for `usize`.
    pub trait Align {
        /// Aligns `self` upwards to the nearest multiple of `align`.
        fn align_up(self, align: Self) -> Self;
    }

That way it can be implemented for all basic types. I'd suggest having a local
macro that takes an arbitrary number of arguments and generates the impl blocks
for each of them, which would be invoked as:

    impl_align!(i8, u8, i16, u16, ...);

> +impl UsizeAlign for usize {
> +    fn align_up(mut self, align: usize) -> usize {
> +        self = (self + align - 1) & !(align - 1);
> +        self
> +    }
> +}

Note that there is no need to mutate `self`, the body can just be:

    (self + align - 1) & !(align - 1)

This operation can trigger overflows/underflows, so I guess for safety it
should return a `Result` and use `checked_add` and `checked_sub`, after moving
`align - 1` into a local variable to avoid checking it twice.

There is also the interesting question of, what if `align` is not a power of 2.
We should probably document the fact that it is expected to be.

> +/// Aligns `val` upwards to the nearest multiple of `align`.
> +pub const fn usize_align_up(val: usize, align: usize) -> usize {
> +    (val + align - 1) & !(align - 1)
> +}

Let's add a statement in the documentation saying that this exists for use in
`const` contexts. The `impl` version can maybe call this one directly to avoid
repeating the same operation twice.

Actually I'm not sure whether we want the `impl` block at all since this
provides the same functionality, albeit in a slightly less cosmetic way. Can
core R4L folks provide guidance about that?

Cheers,
Alex.