Re: [PATCH v3 3/3] drm/xe: Route deferred xe_vma/xe_vm teardown off system_dfl_wq

["Thomas Hellström" <thomas.hellstrom@linux.intel.com>]

On Fri, 2026-09-25 at 13:18 -0700, Matthew Brost wrote:
> On Fri, Sep 25, 2026 at 12:56:43PM -0700, Matthew Brost wrote:
> > On Fri, Sep 25, 2026 at 03:33:35PM +0200, Thomas Hellström wrote:
> > > xe_vma_destroy() can defer the final teardown of a struct xe_vma
> > > to a
> > > dma_fence completion callback (vma_destroy_cb()), and
> > > xe_vm_free() (the
> > > drm_gpuvm_ops.vm_free callback) always defers struct xe_vm
> > > teardown to
> > > a work item, since destroying a VM needs to sleep. Both used to
> > > queue
> > > their work on system_dfl_wq, a global, kernel-wide workqueue that
> > > xe
> > > has no control over and never waits on during module unload.
> > > 
> > > drm_gpuvm_free() drops its drm_device reference immediately after
> > > calling xe_vm_free(), without waiting for the deferred work to
> > > run.
> > > The same applies one level down: whichever xe_vma or xe_vm
> > > reference
> > > happens to be the last one can trigger this chain from a
> > > dma_fence
> > > callback that may fire at an arbitrary time, including after the
> > > owning file has already been closed and its own module reference
> > > dropped. Since nothing tracks or waits for work queued on
> > > system_dfl_wq, `rmmod xe` could succeed and free the module's
> > > text
> > > while vma_destroy_work_func() or vm_destroy_work_func() is still
> > > queued or running on it, jumping into freed code.
> > > 
> > > Fix this by queueing this work on xe_destroy_wq instead, the
> > > existing
> > > module-lifetime workqueue already used for GuC exec queue
> > > teardown.
> > > Unlike a per-device workqueue, this requires no dereference of a
> > > struct xe_device that may already be gone by the time a deferred
> > > callback fires, and unlike system_dfl_wq it is guaranteed to be
> > > drained by xe_destroy_wq_module_exit() before the module is
> > > unloaded,
> > > following the drm_pagemap_dev_hold()/unhold_work precedent of
> > > using a
> > > workqueue that is waited on at module unload rather than a bare
> > > module
> > > reference. The previous commit's reordering of
> > > xe_destroy_wq_exit()
> > > to run after xe_device_exit() guarantees that xe_destroy_wq is
> > > only
> > > torn down once the device-count has reached zero, i.e. after any
> > > xe_vma or xe_vm whose teardown queues work here has already
> > > dropped
> > > its drm_device reference and thus already queued that work.
> > > 
> > > Signed-off-by: Thomas Hellström
> > > <thomas.hellstrom@linux.intel.com>
> > > Assisted-by: LLM
> > > Reviewed-by: Matthew Brost <matthew.brost@intel.com>
> > > ---
> > >  drivers/gpu/drm/xe/xe_module.c | 6 ++++--
> > >  drivers/gpu/drm/xe/xe_vm.c     | 5 +++--
> > >  2 files changed, 7 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/xe/xe_module.c
> > > b/drivers/gpu/drm/xe/xe_module.c
> > > index c61bd33546f2..897724cb5cfb 100644
> > > --- a/drivers/gpu/drm/xe/xe_module.c
> > > +++ b/drivers/gpu/drm/xe/xe_module.c
> > > @@ -114,8 +114,10 @@ static void xe_destroy_wq_module_exit(void)
> > >   * xe_destroy_wq_queue() - Queue work on the destroy workqueue
> > >   * @work: work item to queue
> > >   *
> > > - * The destroy workqueue has module lifetime and is used for GuC
> > > exec queue
> > > - * teardown that can outlive a single xe_device. SVM pagemap
> > > destroy uses the
> > > + * The destroy workqueue has module lifetime, and is guaranteed
> > > to outlive
> > > + * any xe_device, and to be drained before the module is
> > > unloaded. It is used
> > > + * for GuC exec queue and xe_vm/xe_vma teardown that can be
> > > deferred past the
> > > + * lifetime of the xe_device that triggered it. SVM pagemap
> > > destroy uses the
> > >   * per-device xe->destroy_wq instead.
> > >   *
> > >   * Return: %true if @work was queued, %false if it was already
> > > pending.
> > > diff --git a/drivers/gpu/drm/xe/xe_vm.c
> > > b/drivers/gpu/drm/xe/xe_vm.c
> > > index 390da884c727..ee369e6c3b28 100644
> > > --- a/drivers/gpu/drm/xe/xe_vm.c
> > > +++ b/drivers/gpu/drm/xe/xe_vm.c
> > > @@ -29,6 +29,7 @@
> > >  #include "xe_exec_queue.h"
> > >  #include "xe_gt.h"
> > >  #include "xe_migrate.h"
> > > +#include "xe_module.h"
> > >  #include "xe_pagefault.h"
> > >  #include "xe_pat.h"
> > >  #include "xe_pm.h"
> > > @@ -1249,7 +1250,7 @@ static void vma_destroy_cb(struct dma_fence
> > > *fence,
> > >  	struct xe_vma *vma = container_of(cb, struct xe_vma,
> > > destroy_cb);
> > >  
> > >  	INIT_WORK(&vma->destroy_work, vma_destroy_work_func);
> > > -	queue_work(system_dfl_wq, &vma->destroy_work);
> > > +	xe_destroy_wq_queue(&vma->destroy_work);
> > >  }
> > >  
> > >  static void xe_vm_assert_write_mode_or_garbage_collector(struct
> > > xe_vm *vm)
> > > @@ -2059,7 +2060,7 @@ static void xe_vm_free(struct drm_gpuvm
> > > *gpuvm)
> > >  	struct xe_vm *vm = container_of(gpuvm, struct xe_vm,
> > > gpuvm);
> > >  
> > >  	/* To destroy the VM we need to be able to sleep */
> > > -	queue_work(system_dfl_wq, &vm->destroy_work);
> > > +	xe_destroy_wq_queue(&vm->destroy_work);
> > 
> > Actually this is still unsafe, right?
> > 
> > destroy_work touches vm->xe which could be gone after gpuvm drops
> > potentially the final drm_dev_put, right?
> > 
> 
> Ignore this - we flush this queue before destorying any device.

Actually I think you have a point. The module-wide queue is destroyed
as the very last thing the module does. Having both a per-device
destroy queue and a module-wide one is confusing. Let me double-check
this. With this series we could just grab a device reference and
release it when we're done.

Thanks,
Thomas




> 
> Matt
> 
> > Matt 
> > 
> > >  }
> > >  
> > >  struct xe_vm *xe_vm_lookup(struct xe_file *xef, u32 id)
> > > -- 
> > > 2.55.0
> > >