Re: [PATCH 1/1] drm: nova: Align GEM memory allocation to system page size

["Alexandre Courbot" <acourbot@nvidia.com>]

On Wed Nov 26, 2025 at 10:36 PM JST, Alice Ryhl wrote:
> On Wed, Nov 26, 2025 at 10:22:14PM +0900, Alexandre Courbot wrote:
>> On Wed Nov 26, 2025 at 6:54 PM JST, Alice Ryhl wrote:
>> > On Wed, Nov 26, 2025 at 09:31:46AM +0900, Alexandre Courbot wrote:
>> >> On Tue Nov 25, 2025 at 11:59 PM JST, Alice Ryhl wrote:
>> >> > On Tue, Nov 25, 2025 at 3:55 PM Alexandre Courbot <acourbot@nvidia.com> wrote:
>> >> >>
>> >> >> On Tue Nov 25, 2025 at 11:41 PM JST, Alice Ryhl wrote:
>> >> >> > On Tue, Nov 25, 2025 at 3:29 PM Alexandre Courbot <acourbot@nvidia.com> wrote:
>> >> >> >>
>> >> >> >> On Fri Nov 21, 2025 at 1:04 PM JST, bshephar wrote:
>> >> >> >> >              return Err(EINVAL);
>> >> >> >> >          }
>> >> >> >> >
>> >> >> >> > +        let aligned_size = page_align(size);
>> >> >> >>
>> >> >> >> `page_align` won't panic on overflow, but it will still return an
>> >> >> >> invalid size. This is a job for `kernel::ptr::Alignment`, which let's
>> >> >> >> you return an error when an overflow occurs.
>> >> >> >
>> >> >> > The Rust implementation of page_align() is implemented as (addr +
>> >> >> > (PAGE_SIZE - 1)) & PAGE_MASK, which definitely will panic on overflow
>> >> >> > if the appropriate config options are enabled.
>> >> >>
>> >> >> That's right, I skimmed its code too fast. ^_^; All the more reason to
>> >> >> use `Alignment`.
>> >> >
>> >> > Alignment stores values that are powers of two, not multiples of PAGE_SIZE.
>> >> 
>> >> Isn't PAGE_SIZE always a power of two though?
>> >
>> > Yes it is. Maybe you can elaborate on how you wanted to use Alignment?
>> > It sounds like you have something different in mind than what I thought.
>> 
>> I thought we could just do something like this:
>> 
>>     use kernel::ptr::{Alignable, Alignment};
>> 
>>     let aligned_size = size
>>         .align_up(Alignment::new::<PAGE_SIZE>())
>>         .ok_or(EOVERFLOW)?;
>> 
>> (maybe we could also have that `Alignment<PAGE_SIZE>` stored as a const
>> in `page.rs` for convenience, as it might be used often)
>
> If you're trying to round up a number to the next multiple of PAGE_SIZE,
> then you should use page_align() because that's exactly what the
> function does.
>
> If you think there is something wrong with the design of page_align(),
> change it instead of reimplemtning it.

In that case I would suggest that `page_align` returns an `Option`
instead of potentially panicking. Does that sound reasonable? I cannot
find any user of it in the Rust code for now.