Re: [PATCH v2 3/3] CI-ONLY:Defer VM teardown until exec queue cleanup

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [High] drm/xe: deferred VM teardown outlives xe_pci_remove() workqueue
  flush
- [Medium] drm/xe: race between user bind queue tracking and VM teardown
  in xe_exec_queue_create_ioctl()
- [Medium] drm/xe: xe_exec_queue_destroy_ioctl() kills user bind queues
  and clears PTEs
- [Low] drm/xe: stale bind queue and wait comments in
  xe_vm_close_and_put_deferred()
--

commit 6c819ef5878fac66d046d56156bffc5871d7f007
Author: Arvind Yadav <arvind.yadav@intel.com>

CI-ONLY:Defer VM teardown until exec queue cleanup

This commit tracks the number of active exec queues using each VM and
defers VM teardown during VM destroy and file close onto xe_destroy_wq
until asynchronous exec queue cleanup completes.

> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> index 1ef9e276bbcad..609fdae907cae 100644
> --- a/drivers/gpu/drm/xe/xe_device.c
> +++ b/drivers/gpu/drm/xe/xe_device.c
> @@ -177,6 +177,7 @@ static void xe_file_close(struct drm_device *dev, struct drm_file *file)
>  	struct xe_file *xef = file->driver_priv;
>  	struct xe_vm *vm;
>  	struct xe_exec_queue *q;
> +	bool close_deferred = false;
>  	unsigned long idx;
>  
>  	guard(xe_pm_runtime)(xe);
> @@ -194,11 +195,26 @@ static void xe_file_close(struct drm_device *dev, struct drm_file *file)
>  	xa_for_each(&xef->exec_queue.xa, idx, q) {
>  		if (q->vm && q->hwe->hw_engine_group)
>  			xe_hw_engine_group_del_exec_queue(q->hwe->hw_engine_group, q);
> -		xe_exec_queue_kill(q);
> +		/*
> +		 * Killing a bind queue can force pending PT jobs to clear
> +		 * mappings while hardware exec queues are still running.
> +		 * Let bind jobs drain before their queue is destroyed.
> +		 */
> +		if (!(q->flags & EXEC_QUEUE_FLAG_VM))
> +			xe_exec_queue_kill(q);
>  		xe_exec_queue_put(q);
>  	}

[Severity: Medium]
Should xe_exec_queue_destroy_ioctl() also avoid calling
xe_exec_queue_kill(q) on EXEC_QUEUE_FLAG_VM queues?

drivers/gpu/drm/xe/xe_exec_queue.c:xe_exec_queue_destroy_ioctl() {
	...
	xe_exec_queue_kill(q);

	trace_xe_exec_queue_close(q);
	xe_exec_queue_put(q);
	...
}

When userspace destroys a user bind queue via
DRM_IOCTL_XE_EXEC_QUEUE_DESTROY while PT jobs are still pending on its
scheduler entity, xe_exec_queue_kill(q) sets EXEC_QUEUE_STATE_KILLED.
guc_exec_queue_run_job() then passes force_clear = true to run_pt_job(),
which clears PTEs instead of populating them while hardware exec queues on
the VM are still running or completing asynchronous GuC cleanup.

> +
> +	/*
> +	 * Do not wait in file close. Keep mappings alive until the remaining
> +	 * exec queues have completed their asynchronous cleanup.
> +	 */
>  	xa_for_each(&xef->vm.xa, idx, vm)
> -		xe_vm_close_and_put(vm);
> +		close_deferred |= xe_vm_close_and_put_deferred(vm);

[Severity: High]
Can deferred VM teardown outlive xe_pci_remove() when xe_file_close()
returns without waiting?

When a file with active exec queues is closed, xe_vm_close_and_put_deferred()
returns while the VM remains open (vm->size != 0) with its page-table BOs
pinned and userptr/SVM DMA mappings active. Teardown then proceeds through
multiple chained work items:

1. The last user queue's destroy work on xe_destroy_wq calls
   __xe_exec_queue_free() -> xe_vm_remove_close_queue(), which queues
   vm->close.work onto xe_destroy_wq.
2. vm_close_work_func() calls xe_vm_close_and_put() ->
   xe_vm_kill_bind_queues(), which starts asynchronous cleanup of vm->q
   (via gt->ordered_wq and then xe_destroy_wq) while vm->q->user_vm still
   holds a reference to vm.
3. When __xe_exec_queue_free(vm->q) runs on xe_destroy_wq and drops
   vm->q->user_vm, xe_vm_free() queues vm->destroy_work onto
   xe_destroy_wq.

During PCI removal, xe_pci_remove() runs:

drivers/gpu/drm/xe/xe_pci.c:xe_pci_remove() {
	...
	xe_device_remove(xe);

	/*
	 * Preserve remove-time flush after moving destroy work to module
	 * lifetime.
	 */
	xe_destroy_wq_flush();
	xe_pm_fini(xe);
}

xe_device_remove() calls xe_bo_pci_dev_remove_all(xe) before
xe_destroy_wq_flush() while deferred VMs are still open, and
xe_destroy_wq_flush() only calls flush_workqueue(xe_destroy_wq) once,
which only waits for work items already queued at entry and does not drain
the chained stages above. As a result, vm_close_work_func(), vm->q
teardown, and vm_destroy_work_func() can run after xe_pm_fini(),
pci_dma_cleanup(), and devres release (including VRAM BAR unmapping),
allowing runtime PM calls after xe_pm_fini() and MMU notifier PTE zaps to
write to unmapped VRAM BARs.

[ ... ]

> diff --git a/drivers/gpu/drm/xe/xe_exec_queue.c b/drivers/gpu/drm/xe/xe_exec_queue.c
> index 7ade915c59a85..da2d437fffb6b 100644
> --- a/drivers/gpu/drm/xe/xe_exec_queue.c
> +++ b/drivers/gpu/drm/xe/xe_exec_queue.c
[ ... ]
> @@ -1466,6 +1473,14 @@ int xe_exec_queue_create_ioctl(struct drm_device *dev, void *data,
>  	}
>  
>  	q->xef = xe_file_get(xef);
> +	/*
> +	 * Track user bind queues until their final free. VM-owned bind
> +	 * queues have no xef and must remain alive until xe_vm_close()
> +	 * has waited for pending bind fences.
> +	 */
> +	if (q->user_vm)
> +		xe_vm_add_close_queue(q->user_vm);

[Severity: Medium]
Can xe_vm_destroy_ioctl() race with user bind queue creation before
xe_vm_add_close_queue(q->user_vm) is called?

For normal exec queues, __xe_exec_queue_alloc() calls
xe_vm_add_close_queue(vm) while xe_exec_queue_create_ioctl() still holds
vm->lock. For DRM_XE_ENGINE_CLASS_VM_BIND queues, __xe_exec_queue_alloc()
receives migrate_vm (where migrate_vm->xef is NULL), and
xe_exec_queue_create_ioctl() drops vm->lock right after
xe_exec_queue_create_bind() before incrementing q->user_vm's counter here:

CPU0                                  CPU1
xe_exec_queue_create_ioctl()
  down_read_interruptible(&vm->lock)
  xe_exec_queue_create_bind(..., vm)
    q->user_vm = xe_vm_get(vm)
  up_read(&vm->lock)
                                      xe_vm_destroy_ioctl()
                                        xe_vm_close_start(vm)
                                        xe_vm_close_and_put_deferred(vm)
                                          !atomic_read(&num_exec_queues)
                                          xe_vm_close_and_put(vm)  <-- tears down VM
  xe_vm_add_close_queue(q->user_vm)
  xa_alloc(&xef->exec_queue.xa, ..., q)

Calling xe_vm_add_close_queue(q->user_vm) before up_read(&vm->lock) is
released (and pairing it with xe_vm_remove_close_queue(q->user_vm) in
__xe_exec_queue_free() unconditionally whenever q->user_vm is set, or
setting q->xef before releasing vm->lock) would close this window.

> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> index e6e53912448f2..51ec754f36a18 100644
> --- a/drivers/gpu/drm/xe/xe_vm.c
> +++ b/drivers/gpu/drm/xe/xe_vm.c
[ ... ]
> +/**
> + * xe_vm_close_and_put_deferred() - Close a VM immediately or defer the close
> + * @vm: The VM reference to consume
> + *
> + * The VM must already be marked closing, and callers must serialize teardown.
> + * Release the VM-owned bind queues before checking for remaining queues.
> + * Callers may release them earlier to start cleanup for several VMs.
> + *
> + * This function does not wait for queue cleanup. If queues remain, defer VM
> + * close until the last tracked queue is freed. The caller's VM reference
> + * is consumed on both the immediate and deferred paths.
> + *
> + * Return: %true if VM close was deferred, or %false if it completed now.
> + */
> +bool xe_vm_close_and_put_deferred(struct xe_vm *vm)
> +{
> +	bool queue_close = false;
> +
> +	/*
> +	 * Keep the VM-owned bind queue alive. xe_vm_close_and_put() waits
> +	 * for pending binds in xe_vm_close() before killing that queue.
> +	 */
> +	if (!atomic_read(&vm->close.num_exec_queues)) {
[ ... ]
> @@ -2289,7 +2410,14 @@ int xe_vm_destroy_ioctl(struct drm_device *dev, void *data,
>  
>  	if (!err) {
>  		xe_vm_close_start(vm);
> -		xe_vm_close_and_put(vm);
> +
> +		/*
> +		 * Internal bind queue cleanup is asynchronous, and user exec
> +		 * queues can outlive the VM handle. Retain the mappings until
> +		 * all tracked queues finish cleanup without waiting here.
> +		 * The helper consumes the caller's VM reference.
> +		 */
> +		xe_vm_close_and_put_deferred(vm);
>  	}
>  
>  	return err;
> diff --git a/drivers/gpu/drm/xe/xe_vm.h b/drivers/gpu/drm/xe/xe_vm.h
> index c90c9dc1351c7..19eb8dfc914e8 100644
> --- a/drivers/gpu/drm/xe/xe_vm.h
> +++ b/drivers/gpu/drm/xe/xe_vm.h
> @@ -221,6 +221,11 @@ int xe_vm_get_property_ioctl(struct drm_device *dev, void *data,
>  
>  void xe_vm_close_and_put(struct xe_vm *vm);
>  void xe_vm_close_start(struct xe_vm *vm);
> +bool xe_vm_close_and_put_deferred(struct xe_vm *vm);
> +void xe_vm_kill_bind_queues(struct xe_vm *vm);
[ ... ]
> diff --git a/drivers/gpu/drm/xe/xe_vm_types.h b/drivers/gpu/drm/xe/xe_vm_types.h
> index 6ca9ef9d0c0b8..953180e99e518 100644
> --- a/drivers/gpu/drm/xe/xe_vm_types.h
> +++ b/drivers/gpu/drm/xe/xe_vm_types.h
> @@ -14,6 +14,7 @@
>  #include <linux/kref.h>
>  #include <linux/mmu_notifier.h>
>  #include <linux/scatterlist.h>
> +#include <linux/wait.h>

[Severity: Low]
This isn't a bug, but several comments and declarations appear to be left
over from an earlier version of the patch:

- The kernel-doc for xe_vm_close_and_put_deferred() says "Release the
  VM-owned bind queues before checking for remaining queues. Callers may
  release them earlier to start cleanup for several VMs", which
  contradicts the inline comment right below it stating that the VM-owned
  bind queue must be kept alive until xe_vm_close_and_put().
- The comment in xe_vm_destroy_ioctl() and paragraphs 3 and 8 of the
  commit message say that internal VM-owned bind queues are counted in
  vm->close.num_exec_queues and can defer VM destroy even when no user
  exec queues remain, whereas VM-owned bind queues have no xef and are
  not tracked in num_exec_queues.
- Paragraph 4 of the commit message still describes a five-second wait
  budget during file close, contradicting paragraph 7 and xe_file_close().
- xe_vm_kill_bind_queues() is only called within xe_vm.c, so it can be
  static rather than declared in xe_vm.h, and <linux/wait.h> in
  xe_vm_types.h is unused.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114083859.1007095-1-arvind.yadav@intel.com?part=3