Messages in this thread |  | | Date | Mon, 8 Sep 2025 08:26:13 +0000 | | Subject | Re: [PATCH 1/2] drm/gpuvm: add deferred vm_bo cleanup | | From | Alice Ryhl <> |
| |
On Mon, Sep 08, 2025 at 09:11:40AM +0200, Boris Brezillon wrote: > Hi Alice, > > On Sun, 7 Sep 2025 11:39:41 +0000 > Alice Ryhl <aliceryhl@google.com> wrote: > > > Yeah I guess we could have unlink remove the gpuva, but then allow the > > end-user to attach the gpuva to a list of gpuvas to kfree deferred. That > > way, the drm_gpuva_unlink() is not deferred but any resources it has can > > be. > > This ^. > > > > > Of course, this approach also makes deferred gpuva cleanup somewhat > > orthogonal to this patch. > > Well, yes and no, because if you go for gpuva deferred cleanup, you > don't really need the fancy kref_put() you have in this patch, it's > just a regular vm_bo_put() that's called in the deferred gpuva path on > the vm_bo attached to the gpuva being released.
Ok, so what you suggest is that on gpuva_unlink() we remove the gpuva from the vm_bo's list, but then instead of putting the vm_bo's refcount, we add the gpuva to a list, and in the deferred cleanup codepath we iterate gpuvas and drop vm_bo refcounts *at that point*. Is that understood correctly?
That means we don't immediately remove the vm_bo from the gem.gpuva list, but the gpuva list in the vm_bo will be empty. I guess you already have to handle such vm_bos anyway since you can already have an empty vm_bo in between vm_bo_obtain() and the first call to gpuva_link().
One disadvantage is that we might end up preparing or unevicting a GEM object that doesn't have any VAs left, which the current approach avoids.
> > One annoying part is that we don't have an gpuvm ops operation for > > freeing gpuva, and if we add one for this, it would *only* be used in > > this case as most drivers explicitly kfree gpuvas, which could be > > confusing for end-users. > > Also not sure ::vm_bo_free() was meant to be used like that. It was for > drivers that need to control the drm_gpuvm_bo allocation, not those > that rely on the default implementation (kmalloc). Given how things > are described in the the doc, it feels weird to have a ::vm_bo_free() > without ::vm_bo_alloc(). So, if we decide to go this way (which I'm > still not convinced we should, given ultimately we might want to defer > gpuvas cleanup), the ::vm_bo_free() doc should be extended to cover > this 'deferred vm_bo free' case.
I can implement vm_bo_alloc() too, but I think it seems like a pretty natural way to use vm_bo_free().
Alice
|  |