Agreed with all these comments, will revise accordingly. Thanks.
> On Feb 10, 2026, at 4:10 PM, Danilo Krummrich <[email protected]> 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+qwc4a41p7eqnndls1apueeubaqyj46ydki...@mail.gmail.com/ >> >>>> + 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.
