Message-ID: <20251105005931.GA2278646@joelbox2>
Date: Tue, 4 Nov 2025 19:59:31 -0500
From: Joel Fernandes <joelagnelf@...dia.com>
To: Alexandre Courbot <acourbot@...dia.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>,
	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>,
	Alice Ryhl <aliceryhl@...gle.com>, 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,
	Nouveau <nouveau-bounces@...ts.freedesktop.org>
Subject: Re: [PATCH RFC 3/4] rust: drm: Add DRM buddy allocator bindings

On Sat, Nov 01, 2025 at 02:08:56PM +0900, Alexandre Courbot wrote:
> On Fri Oct 31, 2025 at 4:06 AM JST, Joel Fernandes wrote:
> <snip>
> > +/// 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.
> > +pub struct DrmBuddy {
> > +    mm: Opaque<bindings::drm_buddy>,
> > +}
> 
> not a big deal, but usually such wrapping structures are defined as
> follows:
> 
> pub struct DrmBuddy(Opaque<bindings::drm_buddy>);

Sure.

> 
> > +
> > +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(),
> 
> Isn't `Opaque::uninit` more appropriate here, since `drm_buddy_init`
> below will overwrite the data?

Sure.

> 
> <snip>
> > +// 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 {}
> > +
> > +/// 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,
> > +}
> 
> Isn't the lifetime going to severely restrict how this can be used?
> 
> For instance, after allocating a list of blocks I suppose you will want
> to store it somewhere, do some other business, and free it much later in
> a completely different code path. The lifetime is going to make this
> very difficult.
> 
> For instance, try and adapt the unit test in the following patch to
> allocate some driver object on the heap (representing a bound device),
> and store both the `DrmBuddy` and the allocated blocks into it. I don't
> think the borrow checker will let you do that.
> 
> I think this calls for a reference-counted design instead - this will
> move lifetime management to runtime, and solve the issue.
> 

Agreed, I will use refcounting. I am also looking into Alice's suggestion
about doing the same between the individual blocks and the AllocatedBlocks.

> > +
> > +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 also looks like a good change to use a transparent struct with an
> opaque. I guess once you adapt the CList design as suggested it will
> come to this naturally.
> 

Sure, sounds good, thanks!

 - Joel

Powered by blists - more mailing lists