Re: [PATCH v12.1 1/1] rust: gpu: Add GPU buddy allocator bindings

[Joel Fernandes <joelagnelf@nvidia.com>]

Hi Alex,

On Mon Mar 16, 2026 at 10:12 PM JST, Alexandre Courbot wrote:
> This is getting close IMHO. But `make rustdoc` fails on the examples,
> and there are still clippy warnings - please make sure to address them.

Fixed.

>> +//! use kernel::{
>> +//!     gpu::buddy::{GpuBuddy, GpuBuddyAllocMode, GpuBuddyAllocFlags, GpuBuddyParams},
>
> nit: should also use kernel formatting style.

Fixed.

>> +//!     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: SZ_4K,
>
> `chunk_size` is an interesting case. The C API uses a `u64`, but I think
> we can reasonably consider that we won't ever need chunks larger than
> 4GB (or can we :O). I'm actually ok with using a `usize` for this one.
>
> One of the first things the C code does is throwing an error if it is
> not a power of 2, so maybe we can even request an `Alignment`?
>
> I'm a bit torn as to whether we should use a `u64` to conform with the C
> API, but doing so would mean we cannot use an `Alignment`...

I prefer to keep it simple and use `usize` for now. I cannot imagine
chunk_size ever exceeding 4GB, and given our stance on rejecting invalid
inputs, this sounds reasonable. Regarding `Alignment`, I still prefer
`usize` here since it makes the caller-side simpler and as you noted the
C code already does error-checking. Let's revisit if needed once this
lands.

>> +//!         GpuBuddyAllocMode::Range { start: 0, end: 0 },
>
> This zero-sized range looks confusing for the example. I understand the
> C API allows this, but I don't think we should. Is there a difference
> with just using `GpuBuddyAllocMode::Simple`? If not, let's switch to
> that, and reject zero-sized ranges in the same spirit as we don't allow
> invalid flag combinations.

Good point. Switched to use `GpuBuddyAllocMode::Simple` and
added validation.

>> +//! assert_eq!(block.offset(), 0);
>> +//! assert_eq!(block.order(), 12); // 2^12 pages = 16MB
>> +//! assert_eq!(block.size(), SZ_16M);
>
> Here we should also check that there is not a second block.

Added.

>> +//! // Dropping the allocation returns the memory to the buddy allocator.
>
> s/memory/range - technically we are not returning physical memory.

Fixed.

>> +//! let (mut count, mut total) = (0u32, 0usize);
>> +//! for block in fragmented.iter() {
>> +//!     assert_eq!(block.size(), SZ_4M);
>> +//!     total += block.size();
>> +//!     count += 1;
>> +//! }
>
> Note that we can avoid mutable variables with this:
>
> //! let total_size: usize = fragmented.iter()
> //!      .inspect(|block| assert_eq!(block.size(), SZ_4M))
> //!      .map(|block| block.size())
> //!      .sum();
> //! assert_eq!(total_size, SZ_8M);
> //! assert_eq!(fragmented.iter().count(), 2);
>
> But your call as to whether this is an improvement.

I feel the current for-loop version is slightly more readable,
especially in a doc example aimed at new users, so I'd like to keep
it as-is.

>> +//! # };
>> +//! # let buddy = GpuBuddy::new(GpuBuddyParams {
>> +//! #     base_offset: 0,
>> +//! #     physical_memory_size: SZ_1G as u64,
>> +//! #     chunk_size: SZ_4K,
>> +//! # })?;
>> +//! # let initial_free = buddy.free_memory();
>
> `make rustdoc` fails to build:
>
> error[E0433]: failed to resolve: use of undeclared type `GpuBuddyAllocFlags`

Fixed. I'll try to run this before submissions henceforth.

>> +/// # Synchronization
>> +///
>> +/// The C `gpu_buddy` API requires synchronization (see `include/linux/gpu_buddy.h`).
>> +/// [`GpuBuddyGuard`] ensures that the lock is held for all
>> +/// allocator and free operations, preventing races between concurrent allocations
>> +/// and the freeing that occurs when [`AllocatedBlocks`] is dropped.
>
> `GpuBuddyGuard` is now private, so we should avoid mentioning it in the
> public documentation as it will just confuse users.
>
> Users won't care about such implementation details - we can just say
> that internal locking ensures all operations are properly synchronized.

Done.

>> +    pub fn new(params: GpuBuddyParams) -> Result<Self> {
>> +        Ok(Self(Arc::pin_init(GpuBuddyInner::new(params), GFP_KERNEL)?))
>
> Can be written as:
>
>         Arc::pin_init(GpuBuddyInner::new(params), GFP_KERNEL).map(Self)
>
> I prefer this form as it avoids the `?` and re-wrapping into `Ok` for
> something that is already a `Result`.

Done.

>> +    /// Allocate blocks from the buddy allocator.
>> +    ///
>> +    /// Returns a pin-initializer for [`AllocatedBlocks`].
>> +    ///
>> +    /// Takes `&self` instead of `&mut self` because the internal [`Mutex`] provides
>> +    /// synchronization - no external `&mut` exclusivity needed.
>
> This is another implementation detail - the fact this takes `&self` and
> is not `unsafe` is already proof that synchronization is taken care of.

Removed the comment.

>> +    pub fn alloc_blocks(
>> +        &self,
>> +        mode: GpuBuddyAllocMode,
>> +        size: usize,
>
> For this parameter I am pretty sure we want to conform to the C API and
> use a `u64` - there is no benefit in not doing so, and buffers larger
> than 4GB *are* a reality nowadays, (maybe not for graphics, but this
> will also be used in compute scenarios).

Agreed. Though, note this adds 7 more `as` usages, but I guess there's
nothing we can do till the IntoSafe stuff is moved to core rust, I think.

>> +    fn offset(&self) -> u64 {
>> +        // SAFETY: `self.as_raw()` is valid per the type's invariants.
>> +        unsafe { bindings::gpu_buddy_block_offset(self.as_raw()) }
>> +    }
>> +
>> +    /// Get the block order.
>> +    fn order(&self) -> u32 {
>> +        // SAFETY: `self.as_raw()` is valid per the type's invariants.
>> +        unsafe { bindings::gpu_buddy_block_order(self.as_raw()) }
>> +    }
>
> Speaking of synchronization - I only had a quick look at the C API, but
> are we sure these methods can all be called without holding the lock?

Yes, sure. Locking is only required around alloc/free operations. Additionally,
`AllocatedBlock` borrows a reference to `AllocatedBlocks`, so the borrow checker
prevents `AllocatedBlocks` from being dropped.

>> diff --git a/rust/kernel/gpu/mod.rs b/rust/kernel/gpu/mod.rs
>
> Let's use `gpu.rs` as the file for this module.

Done, renamed and also updated the MAINTAINERS entries.

>> +pub mod buddy;
>
> IMHO we should have a `#[cfg(CONFIG_GPU_BUDDY = "y")]` here for
> defensiveness...

Added.

>> +#[cfg(CONFIG_GPU_BUDDY = "y")]
>> +pub mod gpu;
>
> ... because in the future I suspect the condition for enabling that
> module will become broader. I think it's fine to keep it as-is for now
> though.

Noted, agreed, keeping it as-is for now.

Thanks for the thorough review! Will respin and send likely tomorrow or day after.

thanks,

--
Joel Fernandes