Message-Id: <D9PZN1E5H5I4.WXP8OB3ANJ02@nvidia.com>
Date: Wed, 07 May 2025 23:11:59 +0900
From: "Alexandre Courbot" <acourbot@...dia.com>
To: "Joel Fernandes" <joelagnelf@...dia.com>, "Miguel Ojeda"
 <ojeda@...nel.org>, "Alex Gaynor" <alex.gaynor@...il.com>, "Boqun Feng"
 <boqun.feng@...il.com>, "Gary Guo" <gary@...yguo.net>,
 Björn Roy Baron <bjorn3_gh@...tonmail.com>, "Benno Lossin"
 <benno.lossin@...ton.me>, "Andreas Hindborg" <a.hindborg@...nel.org>,
 "Alice Ryhl" <aliceryhl@...gle.com>, "Trevor Gross" <tmgross@...ch.edu>,
 "Danilo Krummrich" <dakr@...nel.org>, "David Airlie" <airlied@...il.com>,
 "Simona Vetter" <simona@...ll.ch>, "Maarten Lankhorst"
 <maarten.lankhorst@...ux.intel.com>, "Maxime Ripard" <mripard@...nel.org>,
 "Thomas Zimmermann" <tzimmermann@...e.de>, "Jonathan Corbet"
 <corbet@....net>
Cc: "John Hubbard" <jhubbard@...dia.com>, "Ben Skeggs" <bskeggs@...dia.com>,
 "Timur Tabi" <ttabi@...dia.com>, "Alistair Popple" <apopple@...dia.com>,
 <linux-kernel@...r.kernel.org>, <rust-for-linux@...r.kernel.org>,
 <nouveau@...ts.freedesktop.org>, <dri-devel@...ts.freedesktop.org>
Subject: Re: [PATCH v2 17/21] rust: num: Add an upward alignment helper for
 usize

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.

Powered by blists - more mailing lists