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
