On Mon, 08 Sep 2025 13:11:32 +0200 "Danilo Krummrich" <d...@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).
