Re: [PATCH -next v8 2/3] rust: gpu: Add GPU buddy allocator bindings

[Joel Fernandes]

Agreed with all these comments, will revise accordingly.

Thanks.

> On Feb 10, 2026, at 4:10 PM, Danilo Krummrich <dakr@xxxxxxxxxx> wrote:
> 
> On Tue Feb 10, 2026 at 9:09 PM CET, Joel Fernandes wrote:
>>>> +impl GpuBuddyInner {
>>>> +    /// Create a pin-initializer for the buddy allocator.
>>>> +    fn new(params: &GpuBuddyParams) -> impl PinInit<Self, Error> {
>>> 
>>> I think we can just pass them by value, they shouldn't be needed anymore after
>>> the GpuBuddy instance has been constructed.
>> 
>> Dave Airlie specifically reviewed this in RFC v6 and recommended passing by
>> reference rather than by value [2]:
>> 
>>  "maybe we should pass them as non-mutable references, but I don't think
>>   there is any point in passing them by value ever."
>> 
>> The params are also reused in practice -- the doc examples show the same
>> `GpuBuddyParams` being used repeatedly. References
>> avoid unnecessary copies for this reuse pattern.
> 
> Well, that's for GpuBuddyAllocParams, those are indeed reused and shouldn't be
> copied all the time.
> 
> But my comment was about GpuBuddyParams, I don't see a reason those are reused
> (neither are they in the example), so it makes more sense to pass them by value,
> such that they are consumed. (I.e.  I'm not asking for adding Copy/Clone.)
> 
>> 
>> [2] https://lore.kernel.org/all/CAPM=9tyL_Cq3+qWc4A41p7eqnNDLS1APUEeUbaQyJ46YDkipVw@xxxxxxxxxxxxxx/
>> 
>>>> +    pub fn new(params: &GpuBuddyParams) -> Result<Self> {
>>> 
>>> Same here, we should be able to take this by value.
>> 
>> Same reasoning as above.
>> 
>>>> +    pub fn alloc_blocks(&self, params: &GpuBuddyAllocParams) -> Result<Arc<AllocatedBlocks>> {
>>> 
>>> Why do we force a reference count here? I think we should just return
>>> impl PinInit<AllocatedBlocks, Error> and let the driver decide where to
>>> initialize the object, no?
>>> 
>>> I.e. what if the driver wants to store additional data in a driver private
>>> structure? Then we'd need two allocations otherwise and another reference count
>>> in the worst case.
>> 
>> Good point. The reason I had `Arc` was to anticipate potential shared ownership
>> use cases, but at the moment there is no such use case
>> in nova-core -- each `AllocatedBlocks` has a single owner.
> 
> Sure, but drivers can always pass an impl PinInit to Arc::pin_init() themselves.
> 
>> I'll change this to return `impl PinInit<AllocatedBlocks, Error>` in the next
>> version. If a shared ownership use case arises later, we
>> can always add an `Arc`-returning convenience wrapper.
> 
> I don't think we should, don't give drivers a reason to go for more allocations
> they actually need for convinience.