Message-ID: <c16cbd82-a2a3-4f98-b42f-2a474f706d16@nvidia.com>
Date: Tue, 4 Nov 2025 17:57:12 -0500
From: Joel Fernandes <joelagnelf@...dia.com>
To: Alice Ryhl <aliceryhl@...gle.com>
Cc: linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org,
 dri-devel@...ts.freedesktop.org, dakr@...nel.org,
 David Airlie <airlied@...il.com>, acourbot@...dia.com,
 Alistair Popple <apopple@...dia.com>, Miguel Ojeda <ojeda@...nel.org>,
 Alex Gaynor <alex.gaynor@...il.com>, Boqun Feng <boqun.feng@...il.com>,
 Gary Guo <gary@...yguo.net>, bjorn3_gh@...tonmail.com,
 Benno Lossin <lossin@...nel.org>, Andreas Hindborg <a.hindborg@...nel.org>,
 Trevor Gross <tmgross@...ch.edu>, Simona Vetter <simona@...ll.ch>,
 Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
 Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>,
 John Hubbard <jhubbard@...dia.com>, Timur Tabi <ttabi@...dia.com>,
 joel@...lfernandes.org, Elle Rhumsaa <elle@...thered-steel.dev>,
 Daniel Almeida <daniel.almeida@...labora.com>,
 Andrea Righi <arighi@...dia.com>, Philipp Stanner <phasta@...nel.org>,
 nouveau@...ts.freedesktop.org
Subject: Re: [PATCH RFC 3/4] rust: drm: Add DRM buddy allocator bindings

Hi Alice,

On 10/31/2025 5:25 AM, Alice Ryhl wrote:
> On Thu, Oct 30, 2025 at 03:06:12PM -0400, Joel Fernandes wrote:
>> Add safe Rust abstractions over the Linux kernel's DRM buddy
>> allocator for physical memory management. The DRM buddy allocator
>> implements a binary buddy system for useful for GPU physical memory
>> allocation. nova-core will use it for physical memory allocation.
>>
>> Signed-off-by: Joel Fernandes <joelagnelf@...dia.com>
[...]>> +
>> +/// DRM buddy allocator instance.
>> +///
>> +/// This structure wraps the C `drm_buddy` allocator.
>> +///
>> +/// # Safety
>> +///
>> +/// Not thread-safe. Concurrent alloc/free operations require external
>> +/// synchronization (e.g., wrapping in `Arc<Mutex<DrmBuddy>>`).
>> +///
>> +/// # Invariants
>> +///
>> +/// - `mm` is initialized via `drm_buddy_init()` and remains valid until Drop.
> 
> Usually an invariant is a statement about the present rather than about
> the past. I would say that `mm` is a valid buddy allocator.
Noted, and I will change it to that, thanks!

>> +pub struct DrmBuddy {
>> +    mm: Opaque<bindings::drm_buddy>,
>> +}
>> +
>> +impl DrmBuddy {
>> +    /// Create a new buddy allocator.
>> +    ///
>> +    /// Creates a buddy allocator that manages a contiguous address space of the given
>> +    /// size, with the specified minimum allocation unit (chunk_size must be at least 4KB).
>> +    ///
>> +    /// # Examples
>> +    ///
>> +    /// See the complete example in the documentation comments for [`AllocatedBlocks`].
>> +    pub fn new(size: usize, chunk_size: usize) -> Result<Self> {
>> +        // Create buddy allocator with zeroed memory.
>> +        let buddy = Self {
>> +            mm: Opaque::zeroed(),
>> +        };
>> +
>> +        // Initialize the C buddy structure.
>> +        // SAFETY: buddy.mm points to valid, zeroed memory.
>> +        unsafe {
>> +            to_result(bindings::drm_buddy_init(
>> +                buddy.mm.get(),
> 
> After this call to drm_buddy_init, you return it which moves the struct.
> Is the struct safe to move from one location to another?

It is safe to move this struct since it does not contain anything
self-referential or DMA buffers. But we should pin it for more robustness/future
proofing. I will do that.

> Also I usually put the to_result outside of the unsafe block.

Will do.

[...]
>> +
>> +    /// Allocate blocks from the buddy allocator.
>> +    ///
>> +    /// Returns an [`AllocatedBlocks`] structure that owns the allocated blocks and automatically
>> +    /// frees them when dropped. Allocation of `list_head` uses the `gfp` flags passed.
>> +    pub fn alloc_blocks(
>> +        &self,
>> +        start: usize,
>> +        end: usize,
>> +        size: usize,
>> +        min_block_size: usize,
>> +        flags: BuddyFlags,
>> +        gfp: Flags,
>> +    ) -> Result<AllocatedBlocks<'_>> {
>> +        // Allocate list_head on the heap.
>> +        let mut list_head = KBox::new(bindings::list_head::default(), gfp)?;
>> +
>> +        // SAFETY: list_head is valid and heap-allocated.
>> +        unsafe {
>> +            bindings::INIT_LIST_HEAD(&mut *list_head as *mut _);
>> +        }
>> +
>> +        // SAFETY: mm is a valid DrmBuddy object per the type's invariants.
>> +        unsafe {
>> +            to_result(bindings::drm_buddy_alloc_blocks(
>> +                self.as_raw(),
>> +                start as u64,
>> +                end as u64,
>> +                size as u64,
>> +                min_block_size as u64,
>> +                &mut *list_head as *mut _,
>> +                flags.as_raw() as usize,
>> +            ))?;
>> +        }
>> +
>> +        // `list_head` is now the head of a list that contains allocated blocks
>> +        // from C code. The allocated blocks will be automatically freed when
>> +        // `AllocatedBlocks` is dropped.
>> +        Ok(AllocatedBlocks {
>> +            list_head,
>> +            buddy: self,
>> +        })
>> +    }
>> +}
>> +
>> +impl Drop for DrmBuddy {
>> +    fn drop(&mut self) {
>> +        // SAFETY: self.mm is initialized and valid. drm_buddy_fini properly
>> +        // cleans up all resources. This is called exactly once during Drop.
>> +        unsafe {
>> +            bindings::drm_buddy_fini(self.as_raw());
>> +        }
>> +    }
>> +}
>> +
>> +// SAFETY: DrmBuddy can be sent between threads. Caller is responsible for
>> +// ensuring thread-safe access if needed (e.g., via Mutex).
>> +unsafe impl Send for DrmBuddy {}
> 
> Generally, we should implement both Send and Sync unless we really can't
> do so. If methods require external synchronization, then those methods
> should be marked &mut self and then you implement Sync.

Thanks for letting me know the convention. Just to clarify, when you say "then
you implement Sync", you mean "then you implement mutual exclusion, say via
locks" correct?
> If you instead omit Sync and make the methods &self, then the caller is
> severely restricted and can't e.g. store it in an Arc.
Ok so I will implement Sync and keep the methods as &self since at the moment,
mutation is not required. Let me know if I missed something though.

>> +/// Allocated blocks from the buddy allocator with automatic cleanup.
>> +///
>> +/// This structure owns a list of allocated blocks and ensures they are
>> +/// automatically freed when dropped. Blocks may be iterated over and are
>> +/// read-only after allocation (iteration via [`IntoIterator`] and
>> +/// automatic cleanup via [`Drop`] only). To share across threads, wrap
>> +/// in `Arc<AllocatedBlocks>`. Rust owns the head list head of the
>> +/// allocated blocks; C allocates blocks and links them to the head
>> +/// list head. Clean up of the allocated blocks is handled by C code.
>> +///
>> +/// # Invariants
>> +///
>> +/// - `list_head` is an owned, valid, initialized list_head.
>> +/// - `buddy` points to a valid, initialized [`DrmBuddy`].
>> +pub struct AllocatedBlocks<'a> {
>> +    list_head: KBox<bindings::list_head>,
>> +    buddy: &'a DrmBuddy,
>> +}
>> +
>> +impl Drop for AllocatedBlocks<'_> {
>> +    fn drop(&mut self) {
>> +        // Free all blocks automatically when dropped.
>> +        // SAFETY: list_head is a valid list of blocks per the type's invariants.
>> +        unsafe {
>> +            bindings::drm_buddy_free_list(self.buddy.as_raw(), &mut *self.list_head as *mut _, 0);
>> +        }
>> +    }
>> +}
>> +
>> +impl<'a> AllocatedBlocks<'a> {
>> +    /// Check if the block list is empty.
>> +    pub fn is_empty(&self) -> bool {
>> +        // SAFETY: list_head is a valid list of blocks per the type's invariants.
>> +        unsafe { clist::list_empty(&*self.list_head as *const _) }
>> +    }
>> +
>> +    /// Iterate over allocated blocks.
>> +    pub fn iter(&self) -> clist::ClistIter<'_, Block> {
>> +        // SAFETY: list_head is a valid list of blocks per the type's invariants.
>> +        clist::iter_list_head::<Block>(&*self.list_head)
>> +    }
>> +}
>> +
>> +/// Iteration support for allocated blocks.
>> +///
>> +/// # Examples
>> +///
>> +/// ```ignore
>> +/// for block in &allocated_blocks {
>> +///     // Use block.
>> +/// }
>> +/// ```
>> +impl<'a> IntoIterator for &'a AllocatedBlocks<'_> {
>> +    type Item = Block;
>> +    type IntoIter = clist::ClistIter<'a, Block>;
>> +
>> +    fn into_iter(self) -> Self::IntoIter {
>> +        self.iter()
>> +    }
>> +}
>> +
>> +/// A DRM buddy block.
>> +///
>> +/// Wraps a pointer to a C `drm_buddy_block` structure. This is returned
>> +/// from allocation operations and used to free blocks.
>> +///
>> +/// # Invariants
>> +///
>> +/// `drm_buddy_block_ptr` points to a valid `drm_buddy_block` managed by the buddy allocator.
>> +pub struct Block {
>> +    drm_buddy_block_ptr: NonNull<bindings::drm_buddy_block>,
>> +}
> 
> This type is exposed to the user by ownership (as opposed to being
> exposed behind a reference), and has no lifetime annotation. This
> implies that the caller is allowed to keep it alive for arbitrary
> amounts of time.
> 
> However, it looks like dropping AllocatedBlock would also free this
> Block object. That is a problem.
> 
> The ownership of Block should probably be tied to AllocatedBlock so that
> the borrow-checker prevents dropping AllocatedBlock while Block objects
> exist. Or this code should be changed so that Block keeps the underlying
> AllocatedBlock alive using a refcount. Or similar. It depends on how it
> will be used - if Block is stored long-term in structs, then you should
> avoid lifetimes, but if it's a view into AllocatedBlock that is not
> stored long-term, then lifetimes are the right choice.
> 
You're right! Thanks for catching this, I am leaning to the ref count approach,
but I'll look into the use cases more and designing it accordingly.

thanks,

 - Joel

Powered by blists - more mailing lists