Re: [PATCH v2 1/2] drm/amdkfd: Add helper svm_range_update_checkpoint_timestamp

On 2026-07-27 15:39, Philip Yang wrote:
> Extract svm_range_update_checkpoint_timestamp() from
> svm_range_unmap_from_cpu(). The next patch calls it when the app sets
> the no-access attribute.
>
> Change checkpoint_ts in svm_range_list from uint64_t to atomic64_t so
> svm_range_restore_pages() can read it from the page fault handler
> without holding the svms lock.
>
> No functional change, preparation for the next patch.
>
> Signed-off-by: Philip Yang <[email protected]>
> ---
>   drivers/gpu/drm/amd/amdkfd/kfd_priv.h |  2 +-
>   drivers/gpu/drm/amd/amdkfd/kfd_svm.c  | 83 +++++++++++++++------------
>   2 files changed, 48 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> index 88191a4c1657..bcb929002839 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> @@ -895,7 +895,7 @@ struct svm_range_list {
>   	DECLARE_BITMAP(bitmap_supported, MAX_GPU_INSTANCE);
>   	struct task_struct		*faulting_task;
>   	/* check point ts decides if page fault recovery need be dropped */
> -	uint64_t			checkpoint_ts[MAX_GPU_INSTANCE];
> +	atomic64_t			checkpoint_ts[MAX_GPU_INSTANCE];
>   
>   	/* Default granularity to use in buffer migration
>   	 * and restoration of backing memory while handling
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> index 30ad10bbd47e..c10d4edc8813 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> @@ -759,6 +759,48 @@ svm_range_check_attr(struct kfd_process *p,
>   	return 0;
>   }
>   
> +static void svm_range_update_checkpoint_timestamp(struct kfd_process *p)
> +{
> +	struct svm_range_list *svms;
> +	int i;
> +
> +	svms = &p->svms;
> +
> +	/* calculate time stamps that are used to decide which page faults need be
> +	 * dropped or handled before unmap pages from gpu vm
> +	 */
> +	for_each_set_bit(i, svms->bitmap_supported, p->n_pdds) {
> +		struct kfd_process_device *pdd;
> +		struct amdgpu_device *adev;
> +		struct amdgpu_ih_ring *ih;
> +		uint32_t checkpoint_wptr;
> +
> +		pdd = p->pdds[i];
> +		if (!pdd)
> +			continue;
> +
> +		adev = pdd->dev->adev;
> +
> +		/* Check and drain ih1 ring if cam not available */
> +		if (!adev->irq.retry_cam_enabled && adev->irq.ih1.ring_size) {
> +			ih = &adev->irq.ih1;
> +			checkpoint_wptr = amdgpu_ih_get_wptr(adev, ih);
> +			if (ih->rptr != checkpoint_wptr) {
> +				atomic64_set(&svms->checkpoint_ts[i],
> +					amdgpu_ih_decode_iv_ts(adev, ih, checkpoint_wptr, -1));
> +				continue;
> +			}
> +		}
> +
> +		/* check if dev->irq.ih_soft is not empty */
> +		ih = &adev->irq.ih_soft;
> +		checkpoint_wptr = amdgpu_ih_get_wptr(adev, ih);
> +		if (ih->rptr != checkpoint_wptr)
> +			atomic64_set(&svms->checkpoint_ts[i],
> +				     amdgpu_ih_decode_iv_ts(adev, ih, checkpoint_wptr, -1));
> +	}
> +}
> +
>   static void
>   svm_range_apply_attrs(struct kfd_process *p, struct svm_range *prange,
>   		      uint32_t nattr, struct kfd_ioctl_svm_attribute *attrs,
> @@ -2556,7 +2598,6 @@ svm_range_unmap_from_cpu(struct mm_struct *mm, struct svm_range *prange,
>   	struct kfd_process *p;
>   	unsigned long s, l;
>   	bool unmap_parent;
> -	uint32_t i;
>   
>   	if (atomic_read(&prange->queue_refcount)) {
>   		int r;
> @@ -2576,38 +2617,7 @@ svm_range_unmap_from_cpu(struct mm_struct *mm, struct svm_range *prange,
>   	pr_debug("svms 0x%p prange 0x%p [0x%lx 0x%lx] [0x%lx 0x%lx]\n", svms,
>   		 prange, prange->start, prange->last, start, last);
>   
> -	/* calculate time stamps that are used to decide which page faults need be
> -	 * dropped or handled before unmap pages from gpu vm
> -	 */
> -	for_each_set_bit(i, svms->bitmap_supported, p->n_pdds) {
> -		struct kfd_process_device *pdd;
> -		struct amdgpu_device *adev;
> -		struct amdgpu_ih_ring *ih;
> -		uint32_t checkpoint_wptr;
> -
> -		pdd = p->pdds[i];
> -		if (!pdd)
> -			continue;
> -
> -		adev = pdd->dev->adev;
> -
> -		/* Check and drain ih1 ring if cam not available */
> -		if (!adev->irq.retry_cam_enabled && adev->irq.ih1.ring_size) {
> -			ih = &adev->irq.ih1;
> -			checkpoint_wptr = amdgpu_ih_get_wptr(adev, ih);
> -			if (ih->rptr != checkpoint_wptr) {
> -				svms->checkpoint_ts[i] =
> -					amdgpu_ih_decode_iv_ts(adev, ih, checkpoint_wptr, -1);
> -				continue;
> -			}
> -		}
> -
> -		/* check if dev->irq.ih_soft is not empty */
> -		ih = &adev->irq.ih_soft;
> -		checkpoint_wptr = amdgpu_ih_get_wptr(adev, ih);
> -		if (ih->rptr != checkpoint_wptr)
> -			svms->checkpoint_ts[i] = amdgpu_ih_decode_iv_ts(adev, ih, checkpoint_wptr, -1);
> -	}
> +	svm_range_update_checkpoint_timestamp(p);
>   
>   	unmap_parent = start <= prange->start && last >= prange->last;
>   
> @@ -3117,8 +3127,9 @@ svm_range_restore_pages(struct amdgpu_device *adev, unsigned int pasid,
>   	mutex_lock(&svms->lock);
>   
>   	/* check if this page fault time stamp is before svms->checkpoint_ts */
> -	if (svms->checkpoint_ts[gpuidx] != 0) {
> -		if (amdgpu_ih_ts_after_or_equal(ts,  svms->checkpoint_ts[gpuidx])) {
> +	if (atomic64_read(&svms->checkpoint_ts[gpuidx]) != 0) {
> +		if (amdgpu_ih_ts_after_or_equal(ts,
> +		    atomic64_read(&svms->checkpoint_ts[gpuidx]))) {

You lose atomicity here by reading the checkpoint twice. It would be 
better to read it into a local variable once only and then using the 
same value twice.

With that fixed, the patch is

Reviewed-by: Felix Kuehling <[email protected]>


>   			pr_debug("draining retry fault, drop fault 0x%llx\n", addr);
>   			if (write_locked)
>   				mmap_write_downgrade(mm);
> @@ -3128,7 +3139,7 @@ svm_range_restore_pages(struct amdgpu_device *adev, unsigned int pasid,
>   			/* ts is after svms->checkpoint_ts now, reset svms->checkpoint_ts
>   			 * to zero to avoid following ts wrap around give wrong comparing
>   			 */
> -			svms->checkpoint_ts[gpuidx] = 0;
> +			atomic64_set(&svms->checkpoint_ts[gpuidx], 0);
>   		}
>   	}
>