Message-Id: <DDX3K8BNA4DW.12U3WTKDD5GCF@nvidia.com>
Date: Sat, 01 Nov 2025 14:08:56 +0900
From: "Alexandre Courbot" <acourbot@...dia.com>
To: "Joel Fernandes" <joelagnelf@...dia.com>,
 <linux-kernel@...r.kernel.org>, <rust-for-linux@...r.kernel.org>,
 <dri-devel@...ts.freedesktop.org>, <dakr@...nel.org>, "David Airlie"
 <airlied@...il.com>
Cc: <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>, "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 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>);

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

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

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

Powered by blists - more mailing lists