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

[Joel Fernandes <joelagnelf@nvidia.com>]

Hi Danilo,

On Fri, Feb 27, 2026 at 12:30:20PM +0100, Danilo Krummrich wrote:
> >
> > +RUST [GPU BUDDY]
> > +M:	Joel Fernandes <joelagnelf@nvidia.com>
> > +L:	dri-devel@lists.freedesktop.org
> > +L:	rust-for-linux@vger.kernel.org
> > +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.

Both these additions sound good to me. Just to list all the options below
so we save time on what we all agree on:

1. New GPU BUDDY [RUST] entry.

2. Add it under the existing "DRM DRIVERS AND COMMON INFRASTRUCTURE
   [RUST]"

3. Add the rust files under the existing "GPU BUDDY ALLOCATOR" and add
   myself and Alex as R:/M:

Which of these options make sense? Personally, I think #2 makes sense
since we are already suggesting to add it to the existing DRM RUST entry
anyway - we can just drop the new record and add rust/helpers/gpu.c
there as well. That's what I've done for now. Does that work for you/others?

> > +/// # Invariants
> > +///
> > +/// The inner [`Opaque`] contains a valid, initialized buddy allocator.
>
> The invariant should be justified in the constructor. Also, where is it used?

Done. Added `// INVARIANT:` justification in `GpuBuddyInner::new()` and
cited at all usage sites (ex, alloc_blocks).

> > +// SAFETY: GpuBuddyInner is `Sync` because the internal GpuBuddyGuard
> > +// serializes all access to the C allocator, preventing data races.
>
> I think it's more precise to refer to GpuBuddyInner::lock.

Agreed. Fixed to reference `GpuBuddyInner::lock`.

> > +/// # 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?

Right, removed.

> > +        // 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.

Done. Expanded to justify all three safety requirements.

> > +pub struct Block(Opaque<bindings::gpu_buddy_block>);
>
> Does this need to be public? I don't see it being exposed by any API.

You're right. Made it private.

> > +/// 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.

Updated to the following:

/// A [`Block`] paired with its owning [`AllocatedBlocks`] context.
///
/// Unlike a raw [`Block`], which only knows its offset within the buddy address
/// space, an [`AllocatedBlock`] also has access to the allocator's
/// `base_offset` and `chunk_size`, enabling it to compute absolute offsets and
/// byte sizes.
///
/// Returned by [`AllocatedBlocks::iter()`].
pub struct AllocatedBlock<'a> {
    this: &'a Block,
    blocks: &'a AllocatedBlocks,
}


> > +/// - `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.

Removed.

> > +    block: &'a Block,
>
> NIT: What about just naming it "this" and
>
> > +    alloc: &'a AllocatedBlocks,
>
> "blocks"?

Done, that's better.

thanks,

--
Joel Fernandes