Hi Alice,
On Thu May 29, 2025 at 4:56 AM JST, Alice Ryhl wrote:
> On Wed, May 21, 2025 at 8:45 AM Alexandre Courbot <acour...@nvidia.com> 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 <joelagn...@nvidia.com>
>> Signed-off-by: Joel Fernandes <joelagn...@nvidia.com>
>> Signed-off-by: Alexandre Courbot <acour...@nvidia.com>
>> ---
>> rust/kernel/lib.rs | 1 +
>> rust/kernel/num.rs | 82
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 83 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..05d45b59313d830876c1a7b452827689a6dd5400
>> --- /dev/null
>> +++ b/rust/kernel/num.rs
>> @@ -0,0 +1,82 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +//! Numerical and binary utilities for primitive types.
>> +
>> +/// Extension trait providing useful methods for the kernel on integers.
>> +pub trait NumExt {
>
> I wonder if these should just be standalone methods instead of an
> extension trait?
Standalone methods would need to have different names depending on the
type (e.g. `align_down_u8`, `align_down_u16`, etc.) as I don't think
they can be all made generic (there is for instance no trait for
`wrapping_add`). I thought an extension trait would be cleaner in that
respect.
>
>> + /// Align `self` down to `alignment`.
>> + ///
>> + /// `alignment` must be a power of 2 for accurate results.
>> + ///
>> + /// # Examples
>> + ///
>> + /// ```
>> + /// use kernel::num::NumExt;
>> + ///
>> + /// assert_eq!(0x4fffu32.align_down(0x1000), 0x4000);
>> + /// assert_eq!(0x4fffu32.align_down(0x0), 0x0);
>> + /// ```
>> + fn align_down(self, alignment: Self) -> Self;
>> +
>> + /// 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;
>
> I would probably make alignment into a const parameter.
>
> fn align_up<ALIGN: usize>(value: usize) -> usize {
> const { assert!(ALIGN.is_power_of_two()) };
> self.wrapping_add(ALIGN.wrapping_sub(1)).align_down(ALIGN)
> }
>
> Here the check for power-of-two happens at compile time. Unless you
> have cases where the alignment is a dynamic parameter?
It's very tempting, and I agree that in 99% of the cases the alignment
parameter is a clear constant. There might be the odd case where e.g. a
particular device supports different page sizes for its DMA mappings,
but even there the sizes would come from a limited list.
Worst case, providing two versions, or having the user fall back to
`next_multiple_of` is also an option.
