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 <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(+) >> >> 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 > 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. >>> + >>> + /// Find Last Set Bit: return the 1-based index of the last (i.e. most >>> significant) set bit in >>> + /// `self`. >>> + /// >>> + /// Equivalent to the C `fls` function. >>> + /// >>> + /// # Examples >>> + /// >>> + /// ``` >>> + /// use kernel::num::NumExt; >>> + /// >>> + /// assert_eq!(0x0u32.fls(), 0); >>> + /// assert_eq!(0x1u32.fls(), 1); >>> + /// assert_eq!(0x10u32.fls(), 5); >>> + /// assert_eq!(0xffffu32.fls(), 16); >>> + /// assert_eq!(0x8000_0000u32.fls(), 32); >>> + /// ``` >>> + fn fls(self) -> u32; >> >> Isn't this just `trailing_zeros` [2]? >> >> [2]: https://doc.rust-lang.org/std/primitive.u32.html#method.trailing_zeros > > No, `trailing_zeros` counts from the LSB up to the first bit set to 1, > whereas fls does that from the MSB. For instance, `0xffffu32.fls() == > 16` but `0xffffu32.trailing_zeros() == 0`. Ah right... Then maybe add that to the upstream suggestion :) --- Cheers, Benno
