Message-ID: <1770754015.1979311.8126@nvidia.com>
Date: Tue, 10 Feb 2026 15:09:05 -0500
From: Joel Fernandes <joelagnelf@...dia.com>
To: Danilo Krummrich <dakr@...nel.org>
Cc: linux-kernel@...r.kernel.org,
	Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
	Maxime Ripard <mripard@...nel.org>,
	Thomas Zimmermann <tzimmermann@...e.de>,
	David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
	Jonathan Corbet <corbet@....net>,
	Alex Deucher <alexander.deucher@....com>,
	Christian König <christian.koenig@....com>,
	Jani Nikula <jani.nikula@...ux.intel.com>,
	Joonas Lahtinen <joonas.lahtinen@...ux.intel.com>,
	Vivi Rodrigo <rodrigo.vivi@...el.com>,
	Tvrtko Ursulin <tursulin@...ulin.net>,
	Rui Huang <ray.huang@....com>,
	Matthew Auld <matthew.auld@...el.com>,
	Matthew Brost <matthew.brost@...el.com>,
	Lucas De Marchi <lucas.demarchi@...el.com>,
	Thomas Hellström <thomas.hellstrom@...ux.intel.com>,
	Helge Deller <deller@....de>, Alice Ryhl <aliceryhl@...gle.com>,
	Miguel Ojeda <ojeda@...nel.org>,
	Alex Gaynor <alex.gaynor@...il.com>,
	Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
	Björn Roy Baron <bjorn3_gh@...tonmail.com>,
	Benno Lossin <lossin@...nel.org>,
	Andreas Hindborg <a.hindborg@...nel.org>,
	Trevor Gross <tmgross@...ch.edu>,
	John Hubbard <jhubbard@...dia.com>,
	Alistair Popple <apopple@...dia.com>, Timur Tabi <ttabi@...dia.com>,
	Edwin Peer <epeer@...dia.com>,
	Alexandre Courbot <acourbot@...dia.com>,
	Andrea Righi <arighi@...dia.com>, Andy Ritger <aritger@...dia.com>,
	Zhi Wang <zhiw@...dia.com>, Balbir Singh <balbirs@...dia.com>,
	Philipp Stanner <phasta@...nel.org>,
	Elle Rhumsaa <elle@...thered-steel.dev>,
	Daniel Almeida <daniel.almeida@...labora.com>,
	joel@...lfernandes.org, nouveau@...ts.freedesktop.org,
	dri-devel@...ts.freedesktop.org, rust-for-linux@...r.kernel.org,
	linux-doc@...r.kernel.org, amd-gfx@...ts.freedesktop.org,
	intel-gfx@...ts.freedesktop.org, intel-xe@...ts.freedesktop.org,
	linux-fbdev@...r.kernel.org
Subject: Re: [PATCH -next v8 2/3] rust: gpu: Add GPU buddy allocator bindings

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/20251029-nova-as-v3-5-6a30c7333ad9@nvidia.com/

>> +#[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+qWc4A41p7eqnNDLS1APUEeUbaQyJ46YDkipVw@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

Powered by blists - more mailing lists