Alice Ryhl
2025-Nov-26 13:36 UTC
[PATCH 1/1] drm: nova: Align GEM memory allocation to system page size
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 at 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 at 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. Alice
Alexandre Courbot
2025-Nov-26 14:00 UTC
[PATCH 1/1] drm: nova: Align GEM memory allocation to system page size
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 at 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 at 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.