Re: [PATCH v3 3/3] drm/xe: Block exec and rebind worker while evicting for suspend / hibernate

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

On Thu, 2025-09-04 at 16:22 +0100, Matthew Auld wrote:
> On 04/09/2025 14:06, Thomas Hellström wrote:
> > When the xe pm_notifier evicts for suspend / hibernate, there might
> > be
> > racing tasks trying to re-validate again. This can lead to suspend
> > taking
> > excessive time or get stuck in a live-lock. This behaviour becomes
> > much worse with the fix that actually makes re-validation bring
> > back
> > bos to VRAM rather than letting them remain in TT.
> > 
> > Prevent that by having exec and the rebind worker waiting for a
> > completion
> > that is set to block by the pm_notifier before suspend and is
> > signaled
> > by the pm_notifier after resume / wakeup.
> > 
> > It's probably still possible to craft malicious applications that
> > block
> > suspending. More work is pending to fix that.
> > 
> > v3:
> > - Avoid wait_for_completion() in the kernel worker since it could
> >    potentially cause work item flushes from freezable processes to
> >    wait forever. Instead terminate the rebind workers if needed and
> >    re-launch at resume. (Matt Auld)
> > 
> > Link: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/4288
> > Fixes: c6a4d46ec1d7 ("drm/xe: evict user memory in PM notifier")
> > Cc: Matthew Auld <matthew.auld@intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: <stable@vger.kernel.org> # v6.16+
> > Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> > ---
> >   drivers/gpu/drm/xe/xe_device_types.h |  6 ++++
> >   drivers/gpu/drm/xe/xe_exec.c         |  9 ++++++
> >   drivers/gpu/drm/xe/xe_pm.c           | 20 ++++++++++++
> >   drivers/gpu/drm/xe/xe_vm.c           | 46
> > +++++++++++++++++++++++++++-
> >   drivers/gpu/drm/xe/xe_vm.h           |  2 ++
> >   drivers/gpu/drm/xe/xe_vm_types.h     |  5 +++
> >   6 files changed, 87 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_device_types.h
> > b/drivers/gpu/drm/xe/xe_device_types.h
> > index 092004d14db2..1e780f8a2a8c 100644
> > --- a/drivers/gpu/drm/xe/xe_device_types.h
> > +++ b/drivers/gpu/drm/xe/xe_device_types.h
> > @@ -507,6 +507,12 @@ struct xe_device {
> >   
> >   	/** @pm_notifier: Our PM notifier to perform actions in
> > response to various PM events. */
> >   	struct notifier_block pm_notifier;
> > +	/** @pm_block: Completion to block validating tasks on
> > suspend / hibernate prepare */
> > +	struct completion pm_block;
> > +	/** @rebind_resume_list: List of wq items to kick on
> > resume. */
> > +	struct list_head rebind_resume_list;
> > +	/** @rebind_resume_lock: Lock to protect the
> > rebind_resume_list */
> > +	struct mutex rebind_resume_lock;
> >   
> >   	/** @pmt: Support the PMT driver callback interface */
> >   	struct {
> > diff --git a/drivers/gpu/drm/xe/xe_exec.c
> > b/drivers/gpu/drm/xe/xe_exec.c
> > index 44364c042ad7..374c831e691b 100644
> > --- a/drivers/gpu/drm/xe/xe_exec.c
> > +++ b/drivers/gpu/drm/xe/xe_exec.c
> > @@ -237,6 +237,15 @@ int xe_exec_ioctl(struct drm_device *dev, void
> > *data, struct drm_file *file)
> >   		goto err_unlock_list;
> >   	}
> >   
> > +	/*
> > +	 * It's OK to block interruptible here with the vm lock
> > held, since
> > +	 * on task freezing during suspend / hibernate, the call
> > will
> > +	 * return -ERESTARTSYS and the IOCTL will be rerun.
> > +	 */
> > +	err = wait_for_completion_interruptible(&xe->pm_block);
> > +	if (err)
> > +		goto err_unlock_list;
> > +
> >   	vm_exec.vm = &vm->gpuvm;
> >   	vm_exec.flags = DRM_EXEC_INTERRUPTIBLE_WAIT;
> >   	if (xe_vm_in_lr_mode(vm)) {
> > diff --git a/drivers/gpu/drm/xe/xe_pm.c
> > b/drivers/gpu/drm/xe/xe_pm.c
> > index bee9aacd82e7..6d59990ff6ba 100644
> > --- a/drivers/gpu/drm/xe/xe_pm.c
> > +++ b/drivers/gpu/drm/xe/xe_pm.c
> > @@ -297,6 +297,18 @@ static u32 vram_threshold_value(struct
> > xe_device *xe)
> >   	return DEFAULT_VRAM_THRESHOLD;
> >   }
> >   
> > +static void xe_pm_wake_preempt_workers(struct xe_device *xe)
> > +{
> > +	struct list_head *link, *next;
> > +
> > +	mutex_lock(&xe->rebind_resume_lock);
> > +	list_for_each_safe(link, next, &xe->rebind_resume_list) {
> > +		list_del_init(link);
> > +		xe_vm_resume_preempt_worker(link);
> > +	}
> > +	mutex_unlock(&xe->rebind_resume_lock);
> > +}
> > +
> >   static int xe_pm_notifier_callback(struct notifier_block *nb,
> >   				   unsigned long action, void
> > *data)
> >   {
> > @@ -306,6 +318,7 @@ static int xe_pm_notifier_callback(struct
> > notifier_block *nb,
> >   	switch (action) {
> >   	case PM_HIBERNATION_PREPARE:
> >   	case PM_SUSPEND_PREPARE:
> > +		reinit_completion(&xe->pm_block);
> >   		xe_pm_runtime_get(xe);
> >   		err = xe_bo_evict_all_user(xe);
> >   		if (err)
> > @@ -322,6 +335,8 @@ static int xe_pm_notifier_callback(struct
> > notifier_block *nb,
> >   		break;
> >   	case PM_POST_HIBERNATION:
> >   	case PM_POST_SUSPEND:
> > +		complete_all(&xe->pm_block);
> > +		xe_pm_wake_preempt_workers(xe);
> >   		xe_bo_notifier_unprepare_all_pinned(xe);
> >   		xe_pm_runtime_put(xe);
> >   		break;
> > @@ -348,6 +363,11 @@ int xe_pm_init(struct xe_device *xe)
> >   	if (err)
> >   		return err;
> >   
> > +	init_completion(&xe->pm_block);
> > +	complete_all(&xe->pm_block);
> > +	mutex_init(&xe->rebind_resume_lock);
> 
> err = drmm_mutex_init(&xe->rebind_resume_lock);

Sure.

> 
> ?
> 
> > +	INIT_LIST_HEAD(&xe->rebind_resume_list);
> > +
> >   	/* For now suspend/resume is only allowed with GuC */
> >   	if (!xe_device_uc_enabled(xe))
> >   		return 0;
> > diff --git a/drivers/gpu/drm/xe/xe_vm.c
> > b/drivers/gpu/drm/xe/xe_vm.c
> > index f55f96bb240a..97aad1d53a8c 100644
> > --- a/drivers/gpu/drm/xe/xe_vm.c
> > +++ b/drivers/gpu/drm/xe/xe_vm.c
> > @@ -394,6 +394,9 @@ static int xe_gpuvm_validate(struct
> > drm_gpuvm_bo *vm_bo, struct drm_exec *exec)
> >   		list_move_tail(&gpuva_to_vma(gpuva)-
> > >combined_links.rebind,
> >   			       &vm->rebind_list);
> >   
> > +	if (!try_wait_for_completion(&vm->xe->pm_block))
> > +		return -EAGAIN;
> > +
> >   	ret = xe_bo_validate(gem_to_xe_bo(vm_bo->obj), vm, false);
> >   	if (ret)
> >   		return ret;
> > @@ -480,6 +483,37 @@ static int xe_preempt_work_begin(struct
> > drm_exec *exec, struct xe_vm *vm,
> >   	return xe_vm_validate_rebind(vm, exec, vm-
> > >preempt.num_exec_queues);
> >   }
> >   
> > +static bool vm_suspend_preempt_worker(struct xe_vm *vm)
> > +{
> > +	struct xe_device *xe = vm->xe;
> > +	bool ret = false;
> > +
> > +	mutex_lock(&xe->rebind_resume_lock);
> > +	if (!try_wait_for_completion(&vm->xe->pm_block)) {
> > +		ret = true;
> > +		list_move_tail(&vm->preempt.pm_activate_link, &xe-
> > >rebind_resume_list);
> > +	}
> > +	pr_info("Suspending %p\n", vm);
> > +	mutex_unlock(&xe->rebind_resume_lock);
> > +
> > +	return ret;
> > +}
> > +
> > +/**
> > + * xe_vm_resume_preempt_worker() - Resume the preempt worker.
> > + * @vm: The vm whose preempt worker to resume.
> > + *
> > + * Resume a preempt worker that was previously suspended by
> > + * vm_suspend_preempt_worker().
> > + */
> > +void xe_vm_resume_preempt_worker(struct list_head *link)
> 
> I guess should use vm arg here?

Yes I tried to avoid downcasting from within xe_pm.c but since we need
to include xe_vm.h anyway, I might as well do that.

Thanks for reviewing!

/Thomas




> 
> > +{
> > +	struct xe_vm *vm = container_of(link, typeof(*vm),
> > preempt.pm_activate_link);
> > +
> > +	pr_info("Resuming %p\n", vm);
> > +	queue_work(vm->xe->ordered_wq, &vm->preempt.rebind_work);
> > +}
> > +
> >   static void preempt_rebind_work_func(struct work_struct *w)
> >   {
> >   	struct xe_vm *vm = container_of(w, struct xe_vm,
> > preempt.rebind_work);
> > @@ -503,6 +537,11 @@ static void preempt_rebind_work_func(struct
> > work_struct *w)
> >   	}
> >   
> >   retry:
> > +	if (!try_wait_for_completion(&vm->xe->)) &&
> > vm_suspend_preempt_worker(vm)) {
> > +		up_write(&vm->lock);
> > +		return;
> > +	}
> > +
> >   	if (xe_vm_userptr_check_repin(vm)) {
> >   		err = xe_vm_userptr_pin(vm);
> >   		if (err)
> > @@ -1741,6 +1780,7 @@ struct xe_vm *xe_vm_create(struct xe_device
> > *xe, u32 flags, struct xe_file *xef)
> >   	if (flags & XE_VM_FLAG_LR_MODE) {
> >   		INIT_WORK(&vm->preempt.rebind_work,
> > preempt_rebind_work_func);
> >   		xe_pm_runtime_get_noresume(xe);
> > +		INIT_LIST_HEAD(&vm->preempt.pm_activate_link);
> >   	}
> >   
> >   	if (flags & XE_VM_FLAG_FAULT_MODE) {
> > @@ -1922,8 +1962,12 @@ void xe_vm_close_and_put(struct xe_vm *vm)
> >   	xe_assert(xe, !vm->preempt.num_exec_queues);
> >   
> >   	xe_vm_close(vm);
> > -	if (xe_vm_in_preempt_fence_mode(vm))
> > +	if (xe_vm_in_preempt_fence_mode(vm)) {
> > +		mutex_lock(&xe->rebind_resume_lock);
> > +		list_del_init(&vm->preempt.pm_activate_link);
> > +		mutex_unlock(&xe->rebind_resume_lock);
> >   		flush_work(&vm->preempt.rebind_work);
> > +	}
> >   	if (xe_vm_in_fault_mode(vm))
> >   		xe_svm_close(vm);
> >   
> > diff --git a/drivers/gpu/drm/xe/xe_vm.h
> > b/drivers/gpu/drm/xe/xe_vm.h
> > index b3e5bec0fa58..f2639794278b 100644
> > --- a/drivers/gpu/drm/xe/xe_vm.h
> > +++ b/drivers/gpu/drm/xe/xe_vm.h
> > @@ -281,6 +281,8 @@ struct dma_fence *xe_vm_bind_kernel_bo(struct
> > xe_vm *vm, struct xe_bo *bo,
> >   				       struct xe_exec_queue *q,
> > u64 addr,
> >   				       enum xe_cache_level
> > cache_lvl);
> >   
> > +void xe_vm_resume_preempt_worker(struct list_head *link);
> > +
> >   /**
> >    * xe_vm_resv() - Return's the vm's reservation object
> >    * @vm: The vm
> > diff --git a/drivers/gpu/drm/xe/xe_vm_types.h
> > b/drivers/gpu/drm/xe/xe_vm_types.h
> > index b5108d010786..e1a786db5f89 100644
> > --- a/drivers/gpu/drm/xe/xe_vm_types.h
> > +++ b/drivers/gpu/drm/xe/xe_vm_types.h
> > @@ -338,6 +338,11 @@ struct xe_vm {
> >   		 * BOs
> >   		 */
> >   		struct work_struct rebind_work;
> > +		/**
> > +		 * @preempt.pm_activate_link: Link to list of
> > rebind workers to be
> > +		 * kicked on resume.
> > +		 */
> > +		struct list_head pm_activate_link;
> >   	} preempt;
> >   
> >   	/** @um: unified memory state */
>