Re: [PATCH v11 4/4] rust: gpu: Add GPU buddy allocator bindings

[Danilo Krummrich]

(Cc: Arun)

On Tue Feb 24, 2026 at 11:40 PM CET, Joel Fernandes wrote:
> +RUST [GPU BUDDY]
> +M:	Joel Fernandes <joelagnelf@xxxxxxxxxx>
> +L:	dri-devel@xxxxxxxxxxxxxxxxxxxxx
> +L:	rust-for-linux@xxxxxxxxxxxxxxx
> +S:	Maintained
> +F:	rust/helpers/gpu.c
> +F:	rust/kernel/gpu/

What about adding this to the existing GPU BUDDY ALLOCATOR entry? Maybe you can
offer Matthew and Arun your help.

Also, this wouldn't be a subentry of "RUST", but the rust version of GPU BUDDY,
so it would be "GPU BUDDY [RUST]".

Also, please add rust/kernel/gpu/ to the "DRM DRIVERS AND COMMON INFRASTRUCTURE
[RUST]" entry.

> +/// Inner structure holding the actual buddy allocator.
> +///
> +/// # Synchronization
> +///
> +/// The C `gpu_buddy` API requires synchronization (see `include/linux/gpu_buddy.h`).
> +/// The internal [`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.
> +///
> +/// # Invariants
> +///
> +/// The inner [`Opaque`] contains a valid, initialized buddy allocator.

The invariant should be justified in the constructor. Also, where is it used?

> +// SAFETY: GpuBuddyInner is `Sync` because the internal GpuBuddyGuard

I think it's more precise to refer to GpuBuddyInner::lock.

> +// serializes all access to the C allocator, preventing data races.
> +unsafe impl Sync for GpuBuddyInner {}
> +
> +/// Guard that proves the lock is held, enabling access to the allocator.
> +///
> +/// # Invariants
> +///
> +/// The inner `_guard` holds the lock for the duration of this guard's lifetime.
> +pub(crate) struct GpuBuddyGuard<'a> {
> +    inner: &'a GpuBuddyInner,
> +    _guard: MutexGuard<'a, ()>,
> +}
> +
> +impl GpuBuddyGuard<'_> {
> +    /// Get a raw pointer to the underlying C `gpu_buddy` structure.
> +    fn as_raw(&self) -> *mut bindings::gpu_buddy {
> +        self.inner.inner.get()
> +    }
> +}
> +
> +/// GPU buddy allocator instance.
> +///
> +/// This structure wraps the C `gpu_buddy` allocator using reference counting.
> +/// The allocator is automatically cleaned up when all references are dropped.
> +///
> +/// # Invariants
> +///
> +/// The inner [`Arc`] points to a valid, initialized GPU buddy allocator.

Do we need this invariant? Isn't this implied by the GpuBuddyInner type?

> +    /// Iterate over allocated blocks.
> +    ///
> +    /// Returns an iterator yielding [`AllocatedBlock`] values. Each [`AllocatedBlock`]
> +    /// borrows `self` and is only valid for the duration of that borrow.
> +    pub fn iter(&self) -> impl Iterator<Item = AllocatedBlock<'_>> + '_ {
> +        // SAFETY: list contains gpu_buddy_block items linked via __bindgen_anon_1.link.
> +        let clist = clist_create!(unsafe {

This macro has three separate safety requirements, please justify all of them.
Also, please also use markdown in safety comments. Personally, I don't care too
much, but it would be good to keep things consistent.

> +            self.list.as_raw(),
> +            Block,
> +            bindings::gpu_buddy_block,
> +            __bindgen_anon_1.link
> +        });
> +
> +        clist
> +            .iter()
> +            .map(|block| AllocatedBlock { block, alloc: self })
> +    }
> +}
> +
> +#[pinned_drop]
> +impl PinnedDrop for AllocatedBlocks {
> +    fn drop(self: Pin<&mut Self>) {
> +        let guard = self.buddy.lock();
> +
> +        // SAFETY:
> +        // - list is valid per the type's invariants.
> +        // - guard provides exclusive access to the allocator.
> +        // CAST: BuddyFlags were validated to fit in u32 at construction.
> +        unsafe {
> +            bindings::gpu_buddy_free_list(
> +                guard.as_raw(),
> +                self.list.as_raw(),
> +                self.flags.as_raw() as u32,
> +            );
> +        }
> +    }
> +}
> +
> +/// A GPU buddy block.
> +///
> +/// Transparent wrapper over C `gpu_buddy_block` structure. This type is returned
> +/// as references during iteration over [`AllocatedBlocks`].
> +///
> +/// # Invariants
> +///
> +/// The inner [`Opaque`] contains a valid, allocated `gpu_buddy_block`.
> +#[repr(transparent)]
> +pub struct Block(Opaque<bindings::gpu_buddy_block>);

Does this need to be public? I don't see it being exposed by any API.

> +
> +impl Block {
> +    /// Get a raw pointer to the underlying C block.
> +    fn as_raw(&self) -> *mut bindings::gpu_buddy_block {
> +        self.0.get()
> +    }
> +
> +    /// Get the block's offset in the address space.
> +    pub(crate) 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.
> +    pub(crate) fn order(&self) -> u32 {
> +        // SAFETY: self.as_raw() is valid per the type's invariants.
> +        unsafe { bindings::gpu_buddy_block_order(self.as_raw()) }
> +    }
> +}
> +
> +// SAFETY: `Block` is not modified after allocation for the lifetime
> +// of `AllocatedBlock`.
> +unsafe impl Send for Block {}
> +
> +// SAFETY: `Block` is not modified after allocation for the lifetime
> +// of `AllocatedBlock`.
> +unsafe impl Sync for Block {}
> +
> +/// An allocated block with access to the GPU buddy allocator.

This needs a better description, i.e. what makes an `AllocatedBlock different
compared to a "normal" Block.

> +///
> +/// It is returned by [`AllocatedBlocks::iter()`] and provides access to the
> +/// GPU buddy allocator required for some accessors.
> +///
> +/// # Invariants
> +///
> +/// - `block` is a valid reference to an allocated [`Block`].
> +/// - `alloc` is a valid reference to the [`AllocatedBlocks`] that owns this block.

References should always be valid, no need to mention that.

> +pub struct AllocatedBlock<'a> {
> +    block: &'a Block,

NIT: What about just naming it "this" and

> +    alloc: &'a AllocatedBlocks,

"blocks"?

> +}
> +
> +impl AllocatedBlock<'_> {
> +    /// Get the block's offset in the address space.
> +    ///
> +    /// Returns the absolute offset including the allocator's base offset.
> +    /// This is the actual address to use for accessing the allocated memory.
> +    pub fn offset(&self) -> u64 {
> +        self.alloc.buddy.base_offset + self.block.offset()
> +    }
> +
> +    /// Get the block order (size = chunk_size << order).
> +    pub fn order(&self) -> u32 {
> +        self.block.order()
> +    }
> +
> +    /// Get the block's size in bytes.
> +    pub fn size(&self) -> u64 {
> +        self.alloc.buddy.chunk_size << self.block.order()
> +    }
> +}