Re: [PATCH v9 8/8] drm/gpusvm: Use hmm_range_fault_unlocked_timeout() for range faults

[Matthew Brost <matthew.brost@intel.com>]

On Wed, Jul 15, 2026 at 11:16:52AM -0700, Stanislav Kinsburskii wrote:
> Several GPU SVM paths take mmap_read_lock() only to call hmm_range_fault()
> and open-code mmu interval sequence setup before each HMM walk. They also
> retry -EBUSY until HMM_RANGE_DEFAULT_TIMEOUT expires.
> 
> Use hmm_range_fault_unlocked_timeout() for those faults. The HMM helper now
> owns mmap_lock acquisition and refreshes range->notifier_seq for its
> internal retries, while GPU SVM keeps its existing driver-lock validation
> with mmu_interval_read_retry() after a successful fault.
> 
> Pass HMM_RANGE_DEFAULT_TIMEOUT as the helper retry budget for each HMM
> fault attempt. This scopes the timeout to repeated HMM notifier retries
> while preserving the outer retry loops that restart when the interval is
> invalidated before GPU SVM updates or consumes the mapping state.
> 

This part doesn't seem right for get_pages(), see below.

> Leave drm_gpusvm_check_pages() on hmm_range_fault() because that path is
> called with the mmap lock already held by its caller.
> 
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Stanislav Kinsburskii <skinsburskii@gmail.com>
> ---
>  drivers/gpu/drm/drm_gpusvm.c |   61 +++++-------------------------------------
>  1 file changed, 7 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gpusvm.c b/drivers/gpu/drm/drm_gpusvm.c
> index 958cb605aedd..de5bbfe58ee9 100644
> --- a/drivers/gpu/drm/drm_gpusvm.c
> +++ b/drivers/gpu/drm/drm_gpusvm.c
> @@ -773,8 +773,7 @@ enum drm_gpusvm_scan_result drm_gpusvm_scan_mm(struct drm_gpusvm_range *range,
>  		.end = end,
>  		.dev_private_owner = dev_private_owner,
>  	};
> -	unsigned long timeout =
> -		jiffies + msecs_to_jiffies(HMM_RANGE_DEFAULT_TIMEOUT);
> +	unsigned long timeout = msecs_to_jiffies(HMM_RANGE_DEFAULT_TIMEOUT);
>  	enum drm_gpusvm_scan_result state = DRM_GPUSVM_SCAN_UNPOPULATED, new_state;
>  	unsigned long *pfns;
>  	unsigned long npages = npages_in_range(start, end);
> @@ -788,22 +787,7 @@ enum drm_gpusvm_scan_result drm_gpusvm_scan_mm(struct drm_gpusvm_range *range,
>  	hmm_range.hmm_pfns = pfns;
>  
>  retry:
> -	hmm_range.notifier_seq = mmu_interval_read_begin(notifier);
> -	mmap_read_lock(range->gpusvm->mm);
> -
> -	while (true) {
> -		err = hmm_range_fault(&hmm_range);
> -		if (err == -EBUSY) {
> -			if (time_after(jiffies, timeout))
> -				break;
> -
> -			hmm_range.notifier_seq =
> -				mmu_interval_read_begin(notifier);
> -			continue;
> -		}
> -		break;
> -	}
> -	mmap_read_unlock(range->gpusvm->mm);
> +	err = hmm_range_fault_unlocked_timeout(&hmm_range, timeout);
>  	if (err)
>  		goto err_free;
>  
> @@ -1406,8 +1390,7 @@ int drm_gpusvm_get_pages(struct drm_gpusvm *gpusvm,
>  		.dev_private_owner = ctx->device_private_page_owner,
>  	};
>  	void *zdd;
> -	unsigned long timeout =
> -		jiffies + msecs_to_jiffies(HMM_RANGE_DEFAULT_TIMEOUT);
> +	unsigned long timeout = msecs_to_jiffies(HMM_RANGE_DEFAULT_TIMEOUT);
>  	unsigned long i, j;
>  	unsigned long npages = npages_in_range(pages_start, pages_end);
>  	unsigned long num_dma_mapped;
> @@ -1422,9 +1405,6 @@ int drm_gpusvm_get_pages(struct drm_gpusvm *gpusvm,
>  	struct dma_iova_state *state = &svm_pages->state;
>  
>  retry:
> -	if (time_after(jiffies, timeout))
> -		return -EBUSY;
> -

I think that by deleting the code above, you have changed this function's
semantics by removing the hard cap of HMM_RANGE_DEFAULT_TIMEOUT. This
code was added because, on some non-production platforms, the timing in
this function could cause it to livelock.

Is there any reason this was remove aside from timeout variable not
being a deadline now? You likely should add the deadline back in.

Matt

>  	hmm_range.notifier_seq = mmu_interval_read_begin(notifier);
>  	if (drm_gpusvm_pages_valid_unlocked(gpusvm, svm_pages))
>  		goto set_seqno;
> @@ -1439,21 +1419,7 @@ int drm_gpusvm_get_pages(struct drm_gpusvm *gpusvm,
>  	}
>  
>  	hmm_range.hmm_pfns = pfns;
> -	while (true) {
> -		mmap_read_lock(mm);
> -		err = hmm_range_fault(&hmm_range);
> -		mmap_read_unlock(mm);
> -
> -		if (err == -EBUSY) {
> -			if (time_after(jiffies, timeout))
> -				break;
> -
> -			hmm_range.notifier_seq =
> -				mmu_interval_read_begin(notifier);
> -			continue;
> -		}
> -		break;
> -	}
> +	err = hmm_range_fault_unlocked_timeout(&hmm_range, timeout);
>  	mmput(mm);
>  	if (err)
>  		goto err_free;
> @@ -1720,8 +1686,7 @@ int drm_gpusvm_range_evict(struct drm_gpusvm *gpusvm,
>  		.end = drm_gpusvm_range_end(range),
>  		.dev_private_owner = NULL,
>  	};
> -	unsigned long timeout =
> -		jiffies + msecs_to_jiffies(HMM_RANGE_DEFAULT_TIMEOUT);
> +	unsigned long timeout = msecs_to_jiffies(HMM_RANGE_DEFAULT_TIMEOUT);
>  	unsigned long *pfns;
>  	unsigned long npages = npages_in_range(drm_gpusvm_range_start(range),
>  					       drm_gpusvm_range_end(range));
> @@ -1736,24 +1701,12 @@ int drm_gpusvm_range_evict(struct drm_gpusvm *gpusvm,
>  		return -ENOMEM;
>  
>  	hmm_range.hmm_pfns = pfns;
> -	while (!time_after(jiffies, timeout)) {
> -		hmm_range.notifier_seq = mmu_interval_read_begin(notifier);
> -		if (time_after(jiffies, timeout)) {
> -			err = -ETIME;
> -			break;
> -		}
> -
> -		mmap_read_lock(mm);
> -		err = hmm_range_fault(&hmm_range);
> -		mmap_read_unlock(mm);
> -		if (err != -EBUSY)
> -			break;
> -	}
> +	err = hmm_range_fault_unlocked_timeout(&hmm_range, timeout);
>  
>  	kvfree(pfns);
>  	mmput(mm);
>  
> -	return err;
> +	return err == -EBUSY ? -ETIME : err;
>  }
>  EXPORT_SYMBOL_GPL(drm_gpusvm_range_evict);
>  
> 
>