On 10/3/26 08:38, Olivier Kaloudoff wrote:
> amdgpu_amdkfd_restore_userptr_worker() reschedules itself every
> AMDGPU_USERPTR_RESTORE_DELAY_MS whenever validation of invalidated
> userptr BOs fails. On PCIe surprise removal the VM update paths
> already fail fast: amdgpu_vm_update_pdes() and amdgpu_vm_update_range()
> return -ENODEV when drm_dev_enter() fails, which propagates up through
> process_update_pds()/update_gpuvm_pte()/validate_invalid_user_pages().
> The worker ignores the error code and reschedules unconditionally,
> producing an endless retry loop on a device that is permanently gone.
>
> Observed on MI50 eGPU TB3 surprise removal: ~11k rescheduled restore
> cycles, the worker holding reclaim-related locks until the system
> soft-locked up.
>
> Stop rescheduling when the validation failure is -ENODEV: the device
> is gone for good and the affected processes are being torn down
> anyway. Other errors keep the existing retry behavior, as they may be
> transient.
>
> Cc: Xiaogang Chen <[email protected]>
> Signed-off-by: Olivier Kaloudoff <[email protected]>
> ---
> .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 22 ++++++++++++++-----
> 1 file changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> index d668816..6c040ed 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> @@ -2827,6 +2827,7 @@ static void amdgpu_amdkfd_restore_userptr_worker(struct
> work_struct *work)
> struct task_struct *usertask;
> struct mm_struct *mm;
> uint32_t evicted_bos;
> + int ret = 0;
Please don't initialize return values when it isn't necessary.
Initialize it before the goto.
>
> mutex_lock(&process_info->notifier_lock);
> evicted_bos = process_info->evicted_bos;
> @@ -2846,14 +2847,16 @@ static void
> amdgpu_amdkfd_restore_userptr_worker(struct work_struct *work)
>
> mutex_lock(&process_info->lock);
>
> - if (update_invalid_user_pages(process_info, mm))
> + ret = update_invalid_user_pages(process_info, mm);
> + if (ret)
> goto unlock_out;
> /* userptr_inval_list can be empty if all evicted userptr BOs
> * have been freed. In that case there is nothing to validate
> * and we can just restart the queues.
> */
> if (!list_empty(&process_info->userptr_inval_list)) {
> - if (validate_invalid_user_pages(process_info))
> + ret = validate_invalid_user_pages(process_info);
> + if (ret)
> goto unlock_out;
> }
> /* Final check for concurrent evicton and atomic update. If
> @@ -2886,11 +2889,18 @@ unlock_out:
>
> /* If validation failed, reschedule another attempt */
> if (evicted_bos) {
> - queue_delayed_work(system_freezable_wq,
> - &process_info->restore_userptr_work,
> - msecs_to_jiffies(AMDGPU_USERPTR_RESTORE_DELAY_MS));
> + if (ret == -ENODEV) {
> + /* Device is gone for good, stop retrying */
> + pr_warn_once("%s: device unplugged, aborting userptr
> restore (pid %d, %s)\n",
> + __func__, pid_nr(process_info->pid),
> + usertask->comm);
That warning is most likely unnecessary and maybe even harmful.
That the device is hot removed should just be a perfectly normal abortion
condition for the worker.
Regards,
Christian.
> + } else {
> + queue_delayed_work(system_freezable_wq,
> + &process_info->restore_userptr_work,
> +
> msecs_to_jiffies(AMDGPU_USERPTR_RESTORE_DELAY_MS));
>
> - kfd_smi_event_queue_restore_rescheduled(mm);
> + kfd_smi_event_queue_restore_rescheduled(mm);
> + }
> }
> mmput(mm);
> put_task_struct(usertask);
