Re: [PATCH 3/5] drm/amdgpu/userq: bound the eviction fence rearm retry loop

["Christian König" <christian.koenig@amd.com>]

On 8/10/26 18:13, Junrui Luo via B4 Relay wrote:
> From: Junrui Luo <moonafterrain@outlook.com>
> 
> amdgpu_userq_ensure_ev_fence() loops until the eviction fence is both
> present and unsignaled.  The only producer of such a fence is
> amdgpu_evf_mgr_rearm(), which runs as the very last step of
> amdgpu_userq_vm_validate().  Every failure point ahead of it - the
> kzalloc() in the rearm itself, amdgpu_hmm_range_alloc(), the
> ttm_bo_validate() calls, the GART binding of the wptr BOs - makes
> amdgpu_userq_restore_worker() give up with only a drm_file_err().
> Nothing propagates that back, so the waiting thread reschedules the
> worker and flushes it again, forever.
> 
> Both flush_delayed_work() and mutex_lock() sleep in
> TASK_UNINTERRUPTIBLE, so the looping task cannot be killed and the OOM
> killer cannot reclaim it. An unprivileged render node client
> reaches this from both AMDGPU_USERQ and AMDGPU_USERQ_SIGNAL.
> 
> The eviction fence sequence number is already bumped by every
> successful rearm, so use it as the loop's progress condition: if a
> completed flush of the restore worker did not move it then no rearm
> happened and retrying cannot help.  Return -ENOMEM in that case and
> let both callers report it to userspace.
> 
> Fixes: a242a3e4b5be ("drm/amdgpu: simplify eviction fence suspend/resume")
> Reported-by: Yuhao Jiang <danisjiang@gmail.com>
> Assisted-by: Claude:claude-opus-5
> Cc: stable@vger.kernel.org
> Signed-off-by: Junrui Luo <moonafterrain@outlook.com>

Absolutely clear NAK!

This functions needs to loop forever should the rearm worker fails to re-arm the fence.

The only thing which could potentially get out of that is to kill the process or maybe that the eviction fence is signaled with an error.

Regards,
Christian.

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c       | 21 +++++++++++++++++++--
>  drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h       |  4 ++--
>  drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c | 10 +++++++++-
>  3 files changed, 30 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> index bec107216811..208b53ae5bd1 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> @@ -448,12 +448,16 @@ static void amdgpu_userq_cleanup(struct amdgpu_usermode_queue *queue)
>   * Ensures that a valid and not yet signaled eviction fence is attached to the
>   * usermode queue before any queue operations proceed. If it is signalled, then
>   * rearm a new eviction fence.
> + *
> + * Returns 0 with @uq_mgr->userq_mutex held, or -ENOMEM with the mutex released
> + * when the restore worker could not rearm the fence.
>   */
> -void
> +int
>  amdgpu_userq_ensure_ev_fence(struct amdgpu_userq_mgr *uq_mgr,
>  			     struct amdgpu_eviction_fence_mgr *evf_mgr)
>  {
>  	struct dma_fence *ev_fence;
> +	int seq, prev_seq = -1;
>  
>  retry:
>  	/* Flush any pending resume work to create ev_fence */
> @@ -463,7 +467,16 @@ amdgpu_userq_ensure_ev_fence(struct amdgpu_userq_mgr *uq_mgr,
>  	ev_fence = amdgpu_evf_mgr_get_fence(evf_mgr);
>  	if (dma_fence_is_signaled(ev_fence)) {
>  		dma_fence_put(ev_fence);
> +		seq = atomic_read(&evf_mgr->ev_fence_seq);
>  		mutex_unlock(&uq_mgr->userq_mutex);
> +		/*
> +		 * The sequence number is only bumped by a successful rearm, so
> +		 * if the flush above ran the worker without moving it then the
> +		 * restore failed and looping again would never terminate.
> +		 */
> +		if (seq == prev_seq)
> +			return -ENOMEM;
> +		prev_seq = seq;
>  		/*
>  		 * Looks like there was no pending resume work,
>  		 * add one now to create a valid eviction fence
> @@ -472,6 +485,8 @@ amdgpu_userq_ensure_ev_fence(struct amdgpu_userq_mgr *uq_mgr,
>  		goto retry;
>  	}
>  	dma_fence_put(ev_fence);
> +
> +	return 0;
>  }
>  
>  
> @@ -747,7 +762,9 @@ amdgpu_userq_create(struct drm_file *filp, union drm_amdgpu_userq *args)
>  	if (r)
>  		goto clean_mqd;
>  
> -	amdgpu_userq_ensure_ev_fence(&fpriv->userq_mgr, &fpriv->evf_mgr);
> +	r = amdgpu_userq_ensure_ev_fence(&fpriv->userq_mgr, &fpriv->evf_mgr);
> +	if (r)
> +		goto erase_doorbell;
>  
>  	/* don't map the queue if scheduling is halted */
>  	if (!adev->userq_halt_for_enforce_isolation ||
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h
> index 6412a7f7b6ef..c35909bf7ceb 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h
> @@ -164,8 +164,8 @@ void amdgpu_userq_mgr_fini(struct amdgpu_userq_mgr *userq_mgr);
>  
>  void amdgpu_userq_evict(struct amdgpu_userq_mgr *uq_mgr);
>  
> -void amdgpu_userq_ensure_ev_fence(struct amdgpu_userq_mgr *userq_mgr,
> -				  struct amdgpu_eviction_fence_mgr *evf_mgr);
> +int amdgpu_userq_ensure_ev_fence(struct amdgpu_userq_mgr *userq_mgr,
> +				 struct amdgpu_eviction_fence_mgr *evf_mgr);
>  
>  u32 amdgpu_userq_get_supported_ip_mask(struct amdgpu_device *adev);
>  bool amdgpu_userq_enabled(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
> index 7e80442ec3e5..1c287ce59736 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
> @@ -523,7 +523,15 @@ int amdgpu_userq_signal_ioctl(struct drm_device *dev, void *data,
>  		goto put_queue;
>  
>  	/* We are here means UQ is active, make sure the eviction fence is valid */
> -	amdgpu_userq_ensure_ev_fence(&fpriv->userq_mgr, &fpriv->evf_mgr);
> +	r = amdgpu_userq_ensure_ev_fence(&fpriv->userq_mgr, &fpriv->evf_mgr);
> +	if (r) {
> +		/* The fence is not initialized yet, so unwind it by hand */
> +		amdgpu_userq_fence_put_fence_drv_array(fence);
> +		amdgpu_userq_fence_driver_put(fence->fence_drv);
> +		kvfree(fence->fence_drv_array);
> +		kfree(fence);
> +		goto put_queue;
> +	}
>  
>  	/* Create the new fence */
>  	amdgpu_userq_fence_init(queue, fence, wptr);
>