Hi Alex,
On Mon Mar 16, 2026 at 10:12 PM JST, Alexandre Courbot wrote:
> This is getting close IMHO. But `make rustdoc` fails on the examples,
> and there are still clippy warnings - please make sure to address them.
Fixed.
>> +//! use kernel::{
>> +//! gpu::buddy::{GpuBuddy, GpuBuddyAllocMode, GpuBuddyAllocFlags,
>> GpuBuddyParams},
>
> nit: should also use kernel formatting style.
Fixed.
>> +//! ptr::Alignment,
>> +//! sizes::*, //
>> +//! };
>> +//!
>> +//! // Create a 1GB buddy allocator with 4KB minimum chunk size.
>> +//! let buddy = GpuBuddy::new(GpuBuddyParams {
>> +//! base_offset: 0,
>> +//! physical_memory_size: SZ_1G as u64,
>> +//! chunk_size: SZ_4K,
>
> `chunk_size` is an interesting case. The C API uses a `u64`, but I think
> we can reasonably consider that we won't ever need chunks larger than
> 4GB (or can we :O). I'm actually ok with using a `usize` for this one.
>
> One of the first things the C code does is throwing an error if it is
> not a power of 2, so maybe we can even request an `Alignment`?
>
> I'm a bit torn as to whether we should use a `u64` to conform with the C
> API, but doing so would mean we cannot use an `Alignment`...
I prefer to keep it simple and use `usize` for now. I cannot imagine
chunk_size ever exceeding 4GB, and given our stance on rejecting invalid
inputs, this sounds reasonable. Regarding `Alignment`, I still prefer
`usize` here since it makes the caller-side simpler and as you noted the
C code already does error-checking. Let's revisit if needed once this
lands.
>> +//! GpuBuddyAllocMode::Range { start: 0, end: 0 },
>
> This zero-sized range looks confusing for the example. I understand the
> C API allows this, but I don't think we should. Is there a difference
> with just using `GpuBuddyAllocMode::Simple`? If not, let's switch to
> that, and reject zero-sized ranges in the same spirit as we don't allow
> invalid flag combinations.
Good point. Switched to use `GpuBuddyAllocMode::Simple` and
added validation.
>> +//! assert_eq!(block.offset(), 0);
>> +//! assert_eq!(block.order(), 12); // 2^12 pages = 16MB
>> +//! assert_eq!(block.size(), SZ_16M);
>
> Here we should also check that there is not a second block.
Added.
>> +//! // Dropping the allocation returns the memory to the buddy allocator.
>
> s/memory/range - technically we are not returning physical memory.
Fixed.
>> +//! let (mut count, mut total) = (0u32, 0usize);
>> +//! for block in fragmented.iter() {
>> +//! assert_eq!(block.size(), SZ_4M);
>> +//! total += block.size();
>> +//! count += 1;
>> +//! }
>
> Note that we can avoid mutable variables with this:
>
> //! let total_size: usize = fragmented.iter()
> //! .inspect(|block| assert_eq!(block.size(), SZ_4M))
> //! .map(|block| block.size())
> //! .sum();
> //! assert_eq!(total_size, SZ_8M);
> //! assert_eq!(fragmented.iter().count(), 2);
>
> But your call as to whether this is an improvement.
I feel the current for-loop version is slightly more readable,
especially in a doc example aimed at new users, so I'd like to keep
it as-is.
>> +//! # };
>> +//! # let buddy = GpuBuddy::new(GpuBuddyParams {
>> +//! # base_offset: 0,
>> +//! # physical_memory_size: SZ_1G as u64,
>> +//! # chunk_size: SZ_4K,
>> +//! # })?;
>> +//! # let initial_free = buddy.free_memory();
>
> `make rustdoc` fails to build:
>
> error[E0433]: failed to resolve: use of undeclared type `GpuBuddyAllocFlags`
Fixed. I'll try to run this before submissions henceforth.
>> +/// # Synchronization
>> +///
>> +/// The C `gpu_buddy` API requires synchronization (see
>> `include/linux/gpu_buddy.h`).
>> +/// [`GpuBuddyGuard`] ensures that the lock is held for all
>> +/// allocator and free operations, preventing races between concurrent
>> allocations
>> +/// and the freeing that occurs when [`AllocatedBlocks`] is dropped.
>
> `GpuBuddyGuard` is now private, so we should avoid mentioning it in the
> public documentation as it will just confuse users.
>
> Users won't care about such implementation details - we can just say
> that internal locking ensures all operations are properly synchronized.
Done.
>> + pub fn new(params: GpuBuddyParams) -> Result<Self> {
>> + Ok(Self(Arc::pin_init(GpuBuddyInner::new(params), GFP_KERNEL)?))
>
> Can be written as:
>
> Arc::pin_init(GpuBuddyInner::new(params), GFP_KERNEL).map(Self)
>
> I prefer this form as it avoids the `?` and re-wrapping into `Ok` for
> something that is already a `Result`.
Done.
>> + /// Allocate blocks from the buddy allocator.
>> + ///
>> + /// Returns a pin-initializer for [`AllocatedBlocks`].
>> + ///
>> + /// Takes `&self` instead of `&mut self` because the internal [`Mutex`]
>> provides
>> + /// synchronization - no external `&mut` exclusivity needed.
>
> This is another implementation detail - the fact this takes `&self` and
> is not `unsafe` is already proof that synchronization is taken care of.
Removed the comment.
>> + pub fn alloc_blocks(
>> + &self,
>> + mode: GpuBuddyAllocMode,
>> + size: usize,
>
> For this parameter I am pretty sure we want to conform to the C API and
> use a `u64` - there is no benefit in not doing so, and buffers larger
> than 4GB *are* a reality nowadays, (maybe not for graphics, but this
> will also be used in compute scenarios).
Agreed. Though, note this adds 7 more `as` usages, but I guess there's
nothing we can do till the IntoSafe stuff is moved to core rust, I think.
>> + fn offset(&self) -> u64 {
>> + // SAFETY: `self.as_raw()` is valid per the type's invariants.
>> + unsafe { bindings::gpu_buddy_block_offset(self.as_raw()) }
>> + }
>> +
>> + /// Get the block order.
>> + fn order(&self) -> u32 {
>> + // SAFETY: `self.as_raw()` is valid per the type's invariants.
>> + unsafe { bindings::gpu_buddy_block_order(self.as_raw()) }
>> + }
>
> Speaking of synchronization - I only had a quick look at the C API, but
> are we sure these methods can all be called without holding the lock?
Yes, sure. Locking is only required around alloc/free operations. Additionally,
`AllocatedBlock` borrows a reference to `AllocatedBlocks`, so the borrow checker
prevents `AllocatedBlocks` from being dropped.
>> diff --git a/rust/kernel/gpu/mod.rs b/rust/kernel/gpu/mod.rs
>
> Let's use `gpu.rs` as the file for this module.
Done, renamed and also updated the MAINTAINERS entries.
>> +pub mod buddy;
>
> IMHO we should have a `#[cfg(CONFIG_GPU_BUDDY = "y")]` here for
> defensiveness...
Added.
>> +#[cfg(CONFIG_GPU_BUDDY = "y")]
>> +pub mod gpu;
>
> ... because in the future I suspect the condition for enabling that
> module will become broader. I think it's fine to keep it as-is for now
> though.
Noted, agreed, keeping it as-is for now.
Thanks for the thorough review! Will respin and send likely tomorrow or day
after.
thanks,
--
Joel Fernandes
