[PATCH 0/2] drm/gpuvm: avoid deferred cleanup on GpuVmBoAlloc drop

[PATCH 0/2] drm/gpuvm: avoid deferred cleanup on GpuVmBoAlloc drop

[Daniel Pesic]

GpuVmBoAlloc::drop() (rust/kernel/drm/gpuvm/vm_bo.rs) currently releases
its drm_gpuvm_bo via drm_gpuvm_bo_put_deferred(), which exists to handle
destruction from contexts where the GEM's gpuva lock cannot be taken
synchronously. However, a GpuVmBoAlloc has a refcount of one and is
never added to the gem, extobj, or evict lists by construction.
Therefore, a GpuVmBoAlloc that is dropped without being consumed by
obtain() does not require a deferred put.

drm_gpuvm_bo_obtain_prealloc() currently handles this situation
internally, where it discards the losing preallocated drm_gpuvm_bo by
unlocking obj->gpuva.lock and calling
drm_gpuvm_bo_destroy_not_in_lists() directly, which skips the deferred
path.

This series lets GpuVmBoAlloc::drop() behave the same:

	1/2: exports drm_gpuvm_bo_destroy_not_in_lists() so it can be
	called from the Rust binding, and promotes its comment to
	kernel-doc.

	2/2: switches GpuVmBoAlloc::drop() to call it directly, and
	includes the safety argument for why its precondition holds.
	Drops the TODO left previously.

Note: there is currently no in-tree caller of GpuVm::obtain(), so the
drop path isn't currently exercised by an existing driver.

Daniel Pesic (2):
  drm/gpuvm: export drm_gpuvm_bo_destroy_not_in_lists()
  rust: drm: gpuvm: call drm_gpuvm_bo_destroy_not_in_lists() in Drop

 drivers/gpu/drm/drm_gpuvm.c    |  5 +++--
 include/drm/drm_gpuvm.h        |  2 ++
 rust/kernel/drm/gpuvm/vm_bo.rs | 10 +++++++---
 3 files changed, 12 insertions(+), 5 deletions(-)

-- 
2.55.0

[PATCH 1/2] drm/gpuvm: export drm_gpuvm_bo_destroy_not_in_lists()

[Daniel Pesic]

drm_gpuvm_bo_obtain_prealloc() currently calls this function directly to
destroy a losing preallocated drm_gpuvm_bo. Export it and promote its
comment to kernel-doc so the upcoming Rust GpuVmBoAlloc binding can call
it the same way.

Signed-off-by: Daniel Pesic <danny.pesic@gmail.com>
---
 drivers/gpu/drm/drm_gpuvm.c | 5 +++--
 include/drm/drm_gpuvm.h     | 2 ++
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_gpuvm.c b/drivers/gpu/drm/drm_gpuvm.c
index d1c80ad3dead..30c134455547 100644
--- a/drivers/gpu/drm/drm_gpuvm.c
+++ b/drivers/gpu/drm/drm_gpuvm.c
@@ -1608,7 +1608,7 @@ drm_gpuvm_bo_create(struct drm_gpuvm *gpuvm,
 }
 EXPORT_SYMBOL_GPL(drm_gpuvm_bo_create);
 
-/*
+/**
  * drm_gpuvm_bo_destroy_not_in_lists() - final part of drm_gpuvm_bo cleanup
  * @vm_bo: the &drm_gpuvm_bo to destroy
  *
@@ -1619,7 +1619,7 @@ EXPORT_SYMBOL_GPL(drm_gpuvm_bo_create);
  * object if the refcount reaches zero. It's illegal for this to happen if the
  * caller holds the GEMs gpuva mutex because it would free the mutex.
  */
-static void
+void
 drm_gpuvm_bo_destroy_not_in_lists(struct drm_gpuvm_bo *vm_bo)
 {
 	struct drm_gpuvm *gpuvm = vm_bo->vm;
@@ -1634,6 +1634,7 @@ drm_gpuvm_bo_destroy_not_in_lists(struct drm_gpuvm_bo *vm_bo)
 	drm_gpuvm_put(gpuvm);
 	drm_gem_object_put(obj);
 }
+EXPORT_SYMBOL_GPL(drm_gpuvm_bo_destroy_not_in_lists);
 
 static void
 drm_gpuvm_bo_destroy_not_in_lists_kref(struct kref *kref)
diff --git a/include/drm/drm_gpuvm.h b/include/drm/drm_gpuvm.h
index 38221d83285b..d014a389405c 100644
--- a/include/drm/drm_gpuvm.h
+++ b/include/drm/drm_gpuvm.h
@@ -760,6 +760,8 @@ bool drm_gpuvm_bo_put(struct drm_gpuvm_bo *vm_bo);
 bool drm_gpuvm_bo_put_deferred(struct drm_gpuvm_bo *vm_bo);
 void drm_gpuvm_bo_deferred_cleanup(struct drm_gpuvm *gpuvm);
 
+void drm_gpuvm_bo_destroy_not_in_lists(struct drm_gpuvm_bo *vm_bo);
+
 struct drm_gpuvm_bo *
 drm_gpuvm_bo_find(struct drm_gpuvm *gpuvm,
 		  struct drm_gem_object *obj);
-- 
2.55.0

[PATCH 2/2] rust: drm: gpuvm: call drm_gpuvm_bo_destroy_not_in_lists() in Drop

[Daniel Pesic]

GpuVmBoAlloc's type invariant guarantees a refcount of one and absence
from the gem, extobj, and evict lists for as long as the value exists as
itself. The only way to consume it is via obtain(), which moves it out
via ManuallyDrop, so the invariant holds when Drop::drop runs. Since
this is the required precondition for
drm_gpuvm_bo_destroy_not_in_lists(), call it directly rather than going
through the deferred put, and drop the TODO.

The GEM's gpuva lock must not be held when this runs, since freeing the
last reference to the GEM object would free the lock embedded in it.
Drop::drop has a fixed safe signature, so this precondition cannot be
enforced by the type system and is documented instead, matching the
existing note on GpuVmBoAlloc::obtain(). It currently holds as
lock_gpuva()'s two call sites, in sm_ops.rs, operate on a GpuVmBo that
is already obtained and never construct or drop a GpuVmBoAlloc.

Suggested-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Daniel Pesic <danny.pesic@gmail.com>
---
 rust/kernel/drm/gpuvm/vm_bo.rs | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/rust/kernel/drm/gpuvm/vm_bo.rs b/rust/kernel/drm/gpuvm/vm_bo.rs
index a30f838c11b8..7c0f5846cf39 100644
--- a/rust/kernel/drm/gpuvm/vm_bo.rs
+++ b/rust/kernel/drm/gpuvm/vm_bo.rs
@@ -249,10 +249,14 @@ fn deref(&self) -> &GpuVmBo<T> {
 }
 
 impl<T: DriverGpuVm> Drop for GpuVmBoAlloc<T> {
+    /// Must not be dropped while holding the `drm_gem_object` gpuva lock.
     #[inline]
     fn drop(&mut self) {
-        // TODO: Call drm_gpuvm_bo_destroy_not_in_lists() directly.
-        // SAFETY: It's safe to perform a deferred put in any context.
-        unsafe { bindings::drm_gpuvm_bo_put_deferred(self.as_raw()) };
+        // SAFETY: By the type invariant, `drm_gpuvm_bo` has a refcount
+        // of one and is absent from the gem, extobj, and evict lists.
+        // Per the precondition documented in impl, the caller does not
+        // hold the object's gpuva lock. Therefore, the preconditions of
+        // `drm_gpuvm_bo_destroy_not_in_lists()` are satisfied.
+        unsafe { bindings::drm_gpuvm_bo_destroy_not_in_lists(self.as_raw()) };
     }
 }
-- 
2.55.0