Re: [PATCH 1/2] drm/gpuvm: add deferred vm_bo cleanup

[Boris Brezillon <boris.brezillon@collabora.com>]

On Mon, 08 Sep 2025 13:11:32 +0200
"Danilo Krummrich" <dakr@kernel.org> wrote:

> On Mon Sep 8, 2025 at 12:20 PM CEST, Boris Brezillon wrote:
> > I'm not following. Yes there's going to be a
> > drm_gpuva_unlink_defer_put() that skips the
> >
> >         va->vm_bo = NULL;
> >         drm_gpuvm_bo_put(vm_bo);
> >
> > and adds the gpuva to a list for deferred destruction. But I'm not
> > seeing where the leak is. Once the gpuva has been put in this list,
> > there should be no existing component referring to this object, and it's
> > going to be destroyed or recycled, but not re-used as-is.  
> 
> I'm saying exactly what you say: "has to be a special unlink function" ->
> drm_gpuva_unlink_defer_put(). :)

I don't see how calling drm_gpuva_unlink() instead of
drm_gpuva_unlink_defer_put() would leak the vm_bo though. I mean, it
would certainly be wrong because you'd be calling cleanup methods that
are expected to be called with the resv lock held from the
dma-signalling path, but that's a different issue, no? Anyway, if we're
going to allow gpuva cleanup/destruction deferral, we'll either need to
do that through a different function, or through some specialization of
drm_gpuva_unlink() that does things differently based on the
immediate/non-immediate mode (or some other flag).

> 
> >> Yeah, we really want to avoid that.  
> >
> > I think I agree that we want to avoid it, but I'm not too sure about
> > the solution that involves playing with vm_bo::kref. I'm particularly
> > worried by the fact drivers can iterate the evict/extobj lists
> > directly, and can thus see objects scheduled for destruction. I know
> > there's a gpuvm_bo_is_dead() helper, and drivers should be aware of the
> > risks, but I don't feel comfortable about this.  
> 
> No, drivers can't iterate the evict/extobj lists directly; or at least this is
> not intended by GPUVM's API and if drivers do so, this is considered peeking
> into GPUVM internals, so drivers are on their own anyways.
> 
> Iterators, such as for_each_vm_bo_in_list() are not exposed to drivers.

Okay, that's a good thing. I thought Xe was doing some funky stuff with
the list...

> 
> > And since we've mentioned the possibility of having to support
> > gpuva destruction deferral too, I'm wondering it wouldn't be cleaner
> > to just go for this approach from the start (gpuva owns a ref to a
> > vm_bo, which gets released when the gpuva object is released).