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);
          }
      }

Reply via email to