Hi Danilo, Thanks for the review!
On Tue, Feb 10, 2026 at 12:55:01PM +0100, Danilo Krummrich wrote: > On Mon Feb 9, 2026 at 10:42 PM CET, Joel Fernandes wrote: > > [...] > >> +//! params.size_bytes = SZ_8M as u64; > > It looks there are ~30 occurences of `as u64` in this example code, which > seems > quite inconvinient for drivers. > > In nova-core I proposed to have FromSafeCast / IntoSafeCast for usize, u32 and > u64, which would help here as well, once factored out. > > But even this seems pretty annoying. I wonder if we should just have separate > 64-bit size constants, as they'd be pretty useful in other places as well, > e.g. > GPUVM. Agreed, the `as u64` casts are verbose. Note that these are only in the doc examples -- actual driver code (e.g. nova-core) already uses FromSafeCast/IntoSafeCast from your nova-core patches [1]. Once those traits are factored out of nova-core into a shared kernel crate location, I can update the examples to use them instead. Since the doc examples live outside nova-core, I suggest let us keep it as using as for now. Thoughts? [1] https://lore.kernel.org/all/[email protected]/ >> +#[pin_data(PinnedDrop)] >> +struct GpuBuddyInner { >> + #[pin] >> + inner: Opaque<bindings::gpu_buddy>, >> + #[pin] >> + lock: Mutex<()>, > > Why don't we have the mutex around the Opaque<bindings::gpu_buddy>? It's the > only field the mutex does protect. > > Is it because mutex does not take an impl PinInit? If so, we should add a > comment with a proper TODO. Correct, that is the reason. I'll add a TODO comment in the next version explaining this limitation. >> +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. [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. 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. For the nova-core side, the field would change from `KVec<Arc<AllocatedBlocks>>` to `KVec<Pin<KBox<AllocatedBlocks>>>`, which works fine I think. -- Joel Fernandes
