Alexandre Courbot
2025-Jun-16 06:41 UTC
[PATCH v5 05/23] rust: num: add the `fls` operation
On Sun Jun 15, 2025 at 4:16 AM JST, Benno Lossin wrote:> On Thu Jun 12, 2025 at 4:01 PM CEST, Alexandre Courbot wrote: >> Add an equivalent to the `fls` (Find Last Set bit) C function to Rust >> unsigned types. > > Have you tried to upstream this?I will consider alongside `prev_multiple_of` that we discussed during v4. :)> >> It is to be first used by the nova-core driver. >> >> Signed-off-by: Alexandre Courbot <acourbot at nvidia.com> >> --- >> rust/kernel/num.rs | 31 +++++++++++++++++++++++++++++++ >> 1 file changed, 31 insertions(+) >> >> diff --git a/rust/kernel/num.rs b/rust/kernel/num.rs >> index ee0f67ad1a89e69f5f8d2077eba5541b472e7d8a..934afe17719f789c569dbd54534adc2e26fe59f2 100644 >> --- a/rust/kernel/num.rs >> +++ b/rust/kernel/num.rs >> @@ -171,3 +171,34 @@ fn borrow(&self) -> &T { >> &self.0 >> } >> } >> + >> +macro_rules! impl_fls { >> + ($($t:ty),+) => { >> + $( >> + ::kernel::macros::paste! { >> + /// Find Last Set Bit: return the 1-based index of the last (i.e. most significant) set >> + /// bit in `v`. >> + /// >> + /// Equivalent to the C `fls` function. >> + /// >> + /// # Examples >> + /// >> + /// ``` >> + /// use kernel::num::fls_u32; >> + /// >> + /// assert_eq!(fls_u32(0x0), 0); >> + /// assert_eq!(fls_u32(0x1), 1); >> + /// assert_eq!(fls_u32(0x10), 5); >> + /// assert_eq!(fls_u32(0xffff), 16); >> + /// assert_eq!(fls_u32(0x8000_0000), 32); >> + /// ``` >> + #[inline(always)] >> + pub const fn [<fls_ $t>](v: $t) -> u32 { > > Can we name this `find_last_set_bit_ $t`? When the upstream function > lands, we should also rename this one.We can - but as for `align_up`/`next_multiple_of`, I am not sure which naming scheme (kernel-like or closer to Rust conventions) is favored in such cases, and so far it seems to come down to personal preference. I tend to think that staying close to kernel conventions make it easier to understand when a function is the equivalent of a C one, but whichever policy we adopt it would be nice to codify it somewhere (apologies if it is already and I missed it).
On Mon Jun 16, 2025 at 8:41 AM CEST, Alexandre Courbot wrote:> On Sun Jun 15, 2025 at 4:16 AM JST, Benno Lossin wrote: >> On Thu Jun 12, 2025 at 4:01 PM CEST, Alexandre Courbot wrote: >>> + #[inline(always)] >>> + pub const fn [<fls_ $t>](v: $t) -> u32 { >> >> Can we name this `find_last_set_bit_ $t`? When the upstream function >> lands, we should also rename this one. > > We can - but as for `align_up`/`next_multiple_of`, I am not sure which > naming scheme (kernel-like or closer to Rust conventions) is favored in > such cases, and so far it seems to come down to personal preference. I > tend to think that staying close to kernel conventions make it easier to > understand when a function is the equivalent of a C one, but whichever > policy we adopt it would be nice to codify it somewhere (apologies if it > is already and I missed it).I don't think we have it written down anywhere. I don't think that we should have a global rule for this. Certain things are more in the purview of the kernel and others are more on the Rust side. My opinion is that this, since it will hopefully be in `core` at some point, should go with the Rust naming. --- Cheers, Benno