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 lockheld, since
* on task freezing during suspend / hibernate, the callwill
* 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 ofrebind workers to be
* kicked on resume.*/struct list_head pm_activate_link;} preempt; /** @um: unified memory state */