Hi Danilo,
On Fri, Feb 27, 2026 at 12:30:20PM +0100, Danilo Krummrich wrote:
> >
> > +RUST [GPU BUDDY]
> > +M: Joel Fernandes <[email protected]>
> > +L: [email protected]
> > +L: [email protected]
> > +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
