Re: [PATCH 2/2] drm/amdkfd: Drain retry faults before SVM range no-access unmap

On 2026-06-17 13:04, Kuehling, Felix wrote:
>
> On 2026-06-16 17:16, Philip Yang wrote:
>> When svm_range_needs_unmap() unmaps a range, retry faults queued in the
>> soft IH ring before set_attr no-access was called must be dropped, since
>> those faults arrived before the app chose to revoke GPU access and 
>> should
>> not re-map the range.
>>
>> Extract and add helper svm_range_update_checkpoint_timestamp() from
>> svm_range_unmap_from_cpu() so both callers share the same logic.
>>
>> Change checkpoint_ts in svm_range_list from uint64_t to atomic64_t so
>> svm_range_restore_pages() can read it safely from the page fault handler
>> without holding the svms lock.
>>
>> Signed-off-by: Philip Yang <[email protected]>
>> Acked-by: Kent Russell <[email protected]>
>
> Looks good in general. Two small suggestions inline.
>
>
>> ---
>>   drivers/gpu/drm/amd/amdkfd/kfd_priv.h |  2 +-
>>   drivers/gpu/drm/amd/amdkfd/kfd_svm.c  | 85 +++++++++++++++------------
>>   2 files changed, 50 insertions(+), 37 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h 
>> b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>> index f00c522fba74..d3dcc3b8d546 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>> @@ -893,7 +893,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 e039b6f2942f..acaa364244d0 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,
>> @@ -784,6 +826,8 @@ svm_range_apply_attrs(struct kfd_process *p, 
>> struct svm_range *prange,
>>               gpuidx = kfd_process_gpuidx_from_gpuid(p,
>>                                      attrs[i].value);
>>               if (attrs[i].type == KFD_IOCTL_SVM_ATTR_NO_ACCESS) {
>> +                svm_range_update_checkpoint_timestamp(p);
>> +
>
> I think we should only do this if we're actually unmapping something. 
> So maybe move this into svm_range_needs_unmap.
yes, then this patch only extract and add helper function 
svm_range_update_checkpoint_timestamp, move this patch as the first patch.
>
>
>> bitmap_clear(prange->bitmap_access, gpuidx, 1);
>>                   bitmap_clear(prange->bitmap_aip, gpuidx, 1);
>>                   bitmap_set(prange->bitmap_needs_unmap, gpuidx, 1);
>
> And maybe make "bitmap_needs_unmap" conditional on whether it's 
> currently mapped. (in the previous patch).
add check if (test_bit(gpuidx, prange->bitmap_mapped)) condition in the 
previous patch.

Will send v2 patch series.

Regards,
Philip
>
> Regards,
>   Felix
>
>
>> @@ -2560,7 +2604,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;
>> @@ -2580,38 +2623,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;
>>   @@ -3121,8 +3133,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]))) {
>>               pr_debug("draining retry fault, drop fault 0x%llx\n", 
>> addr);
>>               if (write_locked)
>>                   mmap_write_downgrade(mm);
>> @@ -3132,7 +3145,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);
>>           }
>>       }