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;
+}
+
+impl UsizeAlign for usize {
+ fn align_up(mut self, align: usize) -> usize {
+ self = (self + align - 1) & !(align - 1);
+ self
+ }
+}
+
+/// 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)
+}
--
2.49.0On 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.
Hello, Alex,
On 5/2/2025 12:57 AM, Alexandre Courbot wrote:
> 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, ...);
I actually tried this initially before settling for the simpler solution.
We can do this in 3 ways I think, generics, macros or both.
Unlike the last attempt, this time I did get generics to work. So how about this?
use core::ops::{Add, Sub, BitAnd, Not};
pub trait AlignUp {
fn align_up(self, alignment: Self) -> Self;
}
impl<T> AlignUp for T
where
T: Copy
+ Add<Output = T>
+ Sub<Output = T>
+ BitAnd<Output = T>
+ Not<Output = T>
+ From<u8>,
{
fn align_up(self, alignment: Self) -> Self {
let one = T::from(1u8);
(self + alignment - one) & !(alignment - one)
}
}
I know you must be screaming the word monomorphization, but it is clean code and
I'm willing to see just how much code the compiler generates and does not
requiring specifying any specific types at all!
This also could be simpler if we had num_traits in r4L like userspace, because
num_trait's PrimInt encapsulates all the needed ops.
use num_traits::PrimInt;
pub trait RoundUp {
fn round_up(self, alignment: Self) -> Self;
}
impl<T> RoundUp for T
where
T: PrimInt,
{
fn round_up(self, alignment: Self) -> Self {
(self + alignment - T::one()) & !(alignment - T::one())
}
}
fn main() {
let x: u32 = 1234;
let y: usize = 1234;
// Output 1536
println!("u32: {}", x.round_up(512));
println!("usize: {}", y.round_up(512));
}
For the monomorphization issues, I do wish Rust in general supported restricting
types further so if we could say "where T is u32, usize etc." but it does not
afaics, so maybe we can do this then?
/// This bit can go into separate module if we want to call it
/// 'num_traits' something.
ppub trait Alignable:
Copy
+ Add<Output = Self>
+ Sub<Output = Self>
+ BitAnd<Output = Self>
+ Not<Output = Self>
+ From<u8>
{
}
impl Alignable for usize {}
impl Alignable for u8 {}
impl Alignable for u16 {}
impl Alignable for u32 {}
impl Alignable for u64 {}
impl Alignable for u128 {}
//////////////////////
And then num.rs remains simple but restricted to those types:
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)
}
}
If we dislike that, we could go with the pure macro implementation as well. But
I do like the 'num_traits' approach better, since it keeps the align_up
implementation quite clean.
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?
>
>> +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)
Sure, that's fair enough. I am Ok with either.
>
> 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.
Good idea, makes sense to return Result and also check for power-of-2.
>
>> +/// 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.
Sure.
> 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?
Yeah I am Ok with this as well, however as you noted it is less cosmetic.
To clarify for r4l folks, Alex is asking do we add an impl on the types for
aligning them up as done by this patch, or do we provide a bunch of helpers
(possibly using generics or macros to keep the code clean)?
thanks,
- Joel
On Sat May 3, 2025 at 4:59 AM JST, Joel Fernandes wrote:
> Hello, Alex,
>
> On 5/2/2025 12:57 AM, Alexandre Courbot wrote:
>> 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, ...);
>
> I actually tried this initially before settling for the simpler solution.
>
> We can do this in 3 ways I think, generics, macros or both.
>
> Unlike the last attempt, this time I did get generics to work. So how about this?
>
> use core::ops::{Add, Sub, BitAnd, Not};
>
> pub trait AlignUp {
> fn align_up(self, alignment: Self) -> Self;
> }
>
> impl<T> AlignUp for T
> where
> T: Copy
> + Add<Output = T>
> + Sub<Output = T>
> + BitAnd<Output = T>
> + Not<Output = T>
> + From<u8>,
> {
> fn align_up(self, alignment: Self) -> Self {
> let one = T::from(1u8);
> (self + alignment - one) & !(alignment - one)
> }
> }
>
> I know you must be screaming the word monomorphization, but it is clean code and
> I'm willing to see just how much code the compiler generates and does not
> requiring specifying any specific types at all!
No, I think this is great - monomorphization only happens as the code is
used, so it won't be a problem. And the compiler should just inline that
anyway (let's add an `#[inline]` to be sure although I'm not convinced
it is entirely necessary).
>
> This also could be simpler if we had num_traits in r4L like userspace, because
> num_trait's PrimInt encapsulates all the needed ops.
>
> use num_traits::PrimInt;
>
> pub trait RoundUp {
> fn round_up(self, alignment: Self) -> Self;
> }
>
> impl<T> RoundUp for T
> where
> T: PrimInt,
> {
> fn round_up(self, alignment: Self) -> Self {
> (self + alignment - T::one()) & !(alignment - T::one())
> }
> }
>
> fn main() {
> let x: u32 = 1234;
> let y: usize = 1234;
>
> // Output 1536
> println!("u32: {}", x.round_up(512));
> println!("usize: {}", y.round_up(512));
> }
>
> For the monomorphization issues, I do wish Rust in general supported restricting
> types further so if we could say "where T is u32, usize etc." but it does not
> afaics, so maybe we can do this then?
>
> /// This bit can go into separate module if we want to call it
> /// 'num_traits' something.
>
> ppub trait Alignable:
> Copy
> + Add<Output = Self>
> + Sub<Output = Self>
> + BitAnd<Output = Self>
> + Not<Output = Self>
> + From<u8>
> {
> }
>
> impl Alignable for usize {}
> impl Alignable for u8 {}
> impl Alignable for u16 {}
> impl Alignable for u32 {}
> impl Alignable for u64 {}
> impl Alignable for u128 {}
>
> //////////////////////
>
> And then num.rs remains simple but restricted to those types:
>
> 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)
> }
> }
>
> If we dislike that, we could go with the pure macro implementation as well. But
> I do like the 'num_traits' approach better, since it keeps the align_up
> implementation quite clean.
This looks very rust-y and I like it. I guess Alignable (maybe with a
more generic name because I suspect these properties can be used for
other things than aligning as well) could be in the `num` module without
any problem.
>
> 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.
Cheers,
Alex.
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. I added the #[inline] and hopefully that
gives similar benefits to const that you're seeking:
use core::ops::{BitAnd, BitOr, Not, Add, Sub};
pub trait BitOps:
Copy
+ BitAnd<Output = Self>
+ BitOr<Output = Self>
+ Not<Output = Self>
{
}
pub trait Unsigned:
Copy
+ Add<Output = Self>
+ Sub<Output = Self>
+ From<u8>
{
}
macro_rules! impl_unsigned_traits {
($($t:ty),+) => {
$(
impl Unsigned for $t {}
impl BitOps for $t {}
)+
};
}
impl_unsigned_traits!(usize, u8, u16, u32, u64, u128);
pub trait AlignUp {
fn align_up(self, alignment: Self) -> Self;
}
impl<T> AlignUp for T
where
T: BitOps + Unsigned,
{
#[inline]
fn align_up(self, alignment: Self) -> Self {
let one = T::from(1u8);
(self + alignment - one) & !(alignment - one)
}
}
Thanks.
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.
Actually it may be a good idea to split 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 advised to take more inspiration
from them in order to avoid reinventing the wheel.
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.
> 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.
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.
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. :)
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...
> 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.
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.
On Tue May 6, 2025 at 12:25 AM JST, Joel Fernandes wrote:
>> 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?
Yeah maybe I was looking too far ahead. This can definitely grow
gradually.
>> 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 :)
`next_multiple_of` has the benefit of returning the correct result even
for non-powers of 2, but at the same time trying to align to something
that is not a power of 2 is probably a defect in the code itself. ^_^;
Another reason for not using it is to have things properly named, so
agreed that an extension trait with the functionality we need, with a
name that clearly carries our intent and implemented as efficiently as
the C equivalent is better than reusing standard library methods that
happen to provide the correct result.
>>> 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
Interesting, I would expect that to fail but "conditionally-const"?
After looking that up is appears we can constraint a generic type
against a const trait, but that feature is still experimental and not
enabled in the kernel. So agreed, let's consider that later.
On Thu, 2025-05-01 at 21:58 +0900, Alexandre Courbot wrote:
> +impl UsizeAlign for usize {
> + fn align_up(mut self, align: usize) -> usize {
> + self = (self + align - 1) & !(align - 1);
> + self
> + }
> +}
> +
> +/// 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)
> +}
Why not have usize_align_up() just return "val.align_up(align)"?
But why why two versions at all? Is there any context where you could use one
and not the other?
On Fri May 2, 2025 at 12:19 AM JST, Timur Tabi wrote:
> On Thu, 2025-05-01 at 21:58 +0900, Alexandre Courbot wrote:
>
>
>> +impl UsizeAlign for usize {
>> + fn align_up(mut self, align: usize) -> usize {
>> + self = (self + align - 1) & !(align - 1);
>> + self
>> + }
>> +}
>> +
>> +/// 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)
>> +}
>
> Why not have usize_align_up() just return "val.align_up(align)"?
>
> But why why two versions at all? Is there any context where you could use one
> and not the other?
The second version can be used in const context to create values at
compile-time, something the first one cannot do. If we want to factorize
things out we can probably make the first version call the second one
though.
On 5/1/2025 5:02 PM, Alexandre Courbot wrote:
> On Fri May 2, 2025 at 12:19 AM JST, Timur Tabi wrote:
>> On Thu, 2025-05-01 at 21:58 +0900, Alexandre Courbot wrote:
>>
>>
>>> +impl UsizeAlign for usize {
>>> + fn align_up(mut self, align: usize) -> usize {
>>> + self = (self + align - 1) & !(align - 1);
>>> + self
>>> + }
>>> +}
>>> +
>>> +/// 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)
>>> +}
>>
>> Why not have usize_align_up() just return "val.align_up(align)"?
>>
>> But why why two versions at all? Is there any context where you could use one
>> and not the other?
>
> The second version can be used in const context to create values at
> compile-time, something the first one cannot do. If we want to factorize
> things out we can probably make the first version call the second one
> though.
True. By the way, Timur suggested me to factor it out in a chat so I already did
that in my tree. thanks,
- Joel
On 5/1/2025 11:19 AM, Timur Tabi wrote:
> On Thu, 2025-05-01 at 21:58 +0900, Alexandre Courbot wrote:
>
>
>> +impl UsizeAlign for usize {
>> + fn align_up(mut self, align: usize) -> usize {
>> + self = (self + align - 1) & !(align - 1);
>> + self
>> + }
>> +}
>> +
>> +/// 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)
>> +}
>
> Why not have usize_align_up() just return "val.align_up(align)"?
>
> But why why two versions at all? Is there any context where you could use one
> and not the other?
>
I can't remember now but when I tried that, I got compiler errors (because val
is immutable?).
Also not mutating it like that matches the pattern in the rest of this file so
I'd leave it as-is.
Thanks.
On Thu, 2025-05-01 at 11:22 -0400, Joel Fernandes wrote: > Also not mutating it like that matches the pattern in the rest of this file > so > I'd leave it as-is. Oh I see now. One version changes a variable, and the other returns a new value.
On 5/1/2025 11:31 AM, Timur Tabi wrote: > On Thu, 2025-05-01 at 11:22 -0400, Joel Fernandes wrote: >> Also not mutating it like that matches the pattern in the rest of this file >> so >> I'd leave it as-is. > > Oh I see now. One version changes a variable, and the other returns a new > value. Yes, exactly! Thanks.
© 2016 - 2026 Red Hat, Inc.