Re: [PATCH v13 1/2] rust: gpu: Add GPU buddy allocator bindings

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

Hi Joel,

On Wed Mar 18, 2026 at 7:03 AM JST, Joel Fernandes wrote:
> Add safe Rust abstractions over the Linux kernel's GPU buddy
> allocator for physical memory management. The GPU buddy allocator
> implements a binary buddy system useful for GPU physical memory
> allocation. nova-core will use it for physical memory allocation.
>
> Cc: Nikola Djukic <ndjukic@nvidia.com>
> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>

A few things came to mind when re-reading this again. I believe these
will be my final comments on this patch though (famous last words).

<snip>
> +//! # Examples
> +//!
> +//! Create a buddy allocator and perform a basic range allocation:
> +//!
> +//! ```
> +//! use kernel::{
> +//!     gpu::buddy::{
> +//!         GpuBuddy,
> +//!         GpuBuddyAllocFlags,
> +//!         GpuBuddyAllocMode,
> +//!         GpuBuddyParams, //
> +//!     },
> +//!     prelude::*,
> +//!     ptr::Alignment,
> +//!     sizes::*, //
> +//! };
> +//!
> +//! // Create a 1GB buddy allocator with 4KB minimum chunk size.
> +//! let buddy = GpuBuddy::new(GpuBuddyParams {
> +//!     base_offset: 0,
> +//!     physical_memory_size: SZ_1G as u64,
> +//!     chunk_size: Alignment::new::<SZ_4K>(),
> +//! })?;
> +//!
> +//! assert_eq!(buddy.size(), SZ_1G as u64);
> +//! assert_eq!(buddy.chunk_size(), Alignment::new::<SZ_4K>());

Note that you can also use

  assert_eq!(buddy.chunk_size().as_usize(), SZ_4K);

To avoid the `Alignment` constructor.

<snip>
> +impl GpuBuddyAllocMode {
> +    // Returns the C flags corresponding to the allocation mode.
> +    fn into_flags(self) -> usize {
> +        match self {
> +            Self::Simple => 0,
> +            Self::Range { .. } => bindings::GPU_BUDDY_RANGE_ALLOCATION,
> +            Self::TopDown => bindings::GPU_BUDDY_TOPDOWN_ALLOCATION,
> +        }
> +    }
> +
> +    // Extracts the range start/end, defaulting to (0, 0) for non-range modes.

Let's use `(0, 0)` so they are properly formatted (I know it's not a
doccomment, but the convention also applies to regular comments).

> +    fn range(self) -> (u64, u64) {
> +        match self {
> +            Self::Range { start, end } => (start, end),
> +            _ => (0, 0),
> +        }
> +    }
> +}
> +
> +crate::impl_flags!(
> +    /// Modifier flags for GPU buddy allocation.
> +    ///
> +    /// These flags can be combined with any [`GpuBuddyAllocMode`] to control
> +    /// additional allocation behavior.
> +    #[derive(Clone, Copy, Default, PartialEq, Eq)]
> +    pub struct GpuBuddyAllocFlags(u32);

I've realized a bit late that this should actually be
`GpuBuddyAllocFlags(usize)`.

The values are defined in the bindings as `usize`, and we convert them
to `u32`, only to convert them back into `usize` in `alloc_blocks`. I
know it goes against the idea that flags should not have a size
dependent on the architecture, but in this case it's just a consequence
of the C API not doing it - and in the end we have to conform, so there
is no point in resisting. Actually, `GpuBuddyAllocMode::into_flags`
already return a `usize`, so we're already halfway there.

Just going with the flow and using `usize` removes quite a few `as` in
the code. Ideally we would fix the C API and switch back to `u32` in the
near future but for now that's the best course of action imho.

I've checked whether it worked, and it does - here is a diff for reference:

https://github.com/Gnurou/linux/commit/2e1bfc2d8e1f93a76343c7c563b1f4b85a69ab8b

<snip>
> +    /// Get the base offset for allocations.
> +    pub fn base_offset(&self) -> u64 {
> +        self.0.params.base_offset
> +    }
> +
> +    /// Get the chunk size (minimum allocation unit).
> +    pub fn chunk_size(&self) -> Alignment {
> +        self.0.params.chunk_size
> +    }
> +
> +    /// Get the total managed size.
> +    pub fn size(&self) -> u64 {
> +        self.0.params.physical_memory_size
> +    }
> +
> +    /// Get the available (free) memory in bytes.
> +    pub fn free_memory(&self) -> u64 {

This name is a bit confusing as it can be interpreted as this method
frees memory, whereas it doesn't free anything, and doesn't even deal
with memory (but an address space that may or may not represent memory).

In the C `struct gpu_buddy`, the member representing the chunk size is
named `chunk_size`, and the total size `size`, making the two methods
above this one adopt the same name (by a happy coincidence maybe :)).

Let's do the same here - since we are querying `avail`, this method can
just be called `avail` to align with the C API.

In the same spirit, we should rename
`GpuBuddyParams::physical_memory_size` into just `size` because that's
the name of the corresponding field in `struct gpu_buddy` and again, we
are not limited to managing physical memory with this allocator.

> +        let guard = self.0.lock();
> +
> +        // SAFETY: Per the type invariant, `inner` contains an initialized allocator.
> +        // `guard` provides exclusive access.
> +        unsafe { (*guard.as_raw()).avail }
> +    }
> +
> +    /// Allocate blocks from the buddy allocator.
> +    ///
> +    /// Returns a pin-initializer for [`AllocatedBlocks`].
> +    pub fn alloc_blocks(
> +        &self,
> +        mode: GpuBuddyAllocMode,
> +        size: u64,
> +        min_block_size: Alignment,
> +        flags: impl Into<GpuBuddyAllocFlags>,
> +    ) -> impl PinInit<AllocatedBlocks, Error> {
> +        let buddy_arc = Arc::clone(&self.0);
> +        let (start, end) = mode.range();
> +        let mode_flags = mode.into_flags();
> +        let modifier_flags = u32::from(flags.into()) as usize;
> +
> +        // Create pin-initializer that initializes list and allocates blocks.
> +        try_pin_init!(AllocatedBlocks {
> +            buddy: buddy_arc,
> +            list <- CListHead::new(),
> +            _: {
> +                // Reject zero-sized or inverted ranges.
> +                if let GpuBuddyAllocMode::Range { start, end } = mode {
> +                    if end <= start {
> +                        Err::<(), Error>(EINVAL)?;
> +                    }
> +                }

Ah, indeed we want to disallow decreasing ranges. Actually, why not
prevent them from even being expressed by using an actual Rust `Range`?

This lets you turn this test into an `is_empty()` and removes 10 LoCs
overall. You lose the ability to copy `GpuBuddyAllocMode`, but we don't
need it in the first place.

Here is a diff showing what it looks like, feel free to pick it:

https://github.com/Gnurou/linux/commit/7f9348f6a64d0fbec7ddf99b78ca727a1ac1cd06