On 2025-03-09 20:51, Deng, Emily wrote:
[AMD Official Use Only - AMD Internal Distribution Only]
[AMD Official Use Only - AMD Internal Distribution Only]
*From:*Chen, Xiaogang <xiaogang.c...@amd.com>
*Sent:* Saturday, March 8, 2025 8:38 AM
*To:* Deng, Emily <emily.d...@amd.com>; amd-gfx@lists.freedesktop.org
*Subject:* Re: [PATCH v4] drm/amdgpu: Fix the race condition for
draining retry fault
On 3/6/2025 7:27 PM, Deng, Emily wrote:
[AMD Official Use Only - AMD Internal Distribution Only]
*From:*Chen, Xiaogang <xiaogang.c...@amd.com>
<mailto:xiaogang.c...@amd.com>
*Sent:* Friday, March 7, 2025 1:01 AM
*To:* Deng, Emily <emily.d...@amd.com>
<mailto:emily.d...@amd.com>; amd-gfx@lists.freedesktop.org
*Subject:* Re: [PATCH v4] drm/amdgpu: Fix the race condition for
draining retry fault
Thanks for catch up and fix this race condition. It looks good to
me. One minor thing below:
On 3/6/2025 12:03 AM, Emily Deng wrote:
Issue:
In the scenario where svm_range_restore_pages is called, but
svm->checkpoint_ts
has not been set and the retry fault has not been drained,
svm_range_unmap_from_cpu
is triggered and calls svm_range_free. Meanwhile,
svm_range_restore_pages
continues execution and reaches svm_range_from_addr. This results in
a "failed to find prange..." error, causing the page recovery to fail.
How to fix:
Move the timestamp check code under the protection of svm->lock.
v2:
Make sure all right locks are released before go out.
v3:
Directly goto out_unlock_svms, and return -EAGAIN.
v4:
Refine code.
Signed-off-by: Emily Deng<emily.d...@amd.com>
<mailto:emily.d...@amd.com>
---
drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 30 +++++++++++++++-------------
1 file changed, 16 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
index d04725583f19..83ac14bf7a7a 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
@@ -3008,19 +3008,6 @@ svm_range_restore_pages(struct amdgpu_device
*adev, unsigned int pasid,
goto out;
}
- /* 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])) {
- pr_debug("draining retry fault, drop fault
0x%llx\n", addr);
- r = 0;
- goto out;
- } else
- /* 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;
- }
-
if (!p->xnack_enabled) {
pr_debug("XNACK not enabled for pasid 0x%x\n", pasid);
r = -EFAULT;
@@ -3040,6 +3027,20 @@ svm_range_restore_pages(struct amdgpu_device
*adev, unsigned int pasid,
mmap_read_lock(mm);
retry_write_locked:
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])) {
+ pr_debug("draining retry fault, drop fault
0x%llx\n", addr);
+ r = -EAGAIN;
We drop page fault because it is stale, not mean to handle it
again. if return -EAGAIN we do amdgpu_gmc_filter_faults_remove. If
after unmap, user map same range again we should treat page fault
happened at same range as new one.
Regards
Xiaogang
Sorry, I didn't quite catch that. So, you think we shouldn't
remove the fault from amdgpu_gmc_filter_faults_remove?
I think return "-EAGAIN" means a page fault with an addr and a pasid
is not handled this time. Same page fault will come back again and kfd
will handle it, so remove it from filter to make sure it will be
handled next time.
Yes, I also think we should remove it to make sure it will be handled
next time. From this point of view, it should be removed from filter.
I agree. Without removing it from the filter, we would ignore later
faults on the same address, which would result in stalling the
application indefinitely.
Regards,
Felix
Emily Deng
Best Wishes
In this case this page fault is stale and we do not need or cannot
handle it. There is no indication it will come back again and we need
handle it. I am not sure if should remove it from filter. I just
concern if should return "-EAGAIN" in this case.
Regards
Xiaogang
Emily Deng
Best Wishes
+ goto out_unlock_svms;
+ } else
+ /* 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;
+ }
+
prange = svm_range_from_addr(svms, addr, NULL);
if (!prange) {
pr_debug("failed to find prange svms 0x%p address
[0x%llx]\n",
@@ -3165,7 +3166,8 @@ svm_range_restore_pages(struct amdgpu_device
*adev, unsigned int pasid,
mutex_unlock(&svms->lock);
mmap_read_unlock(mm);
- svm_range_count_fault(node, p, gpuidx);
+ if (r != -EAGAIN)
+ svm_range_count_fault(node, p, gpuidx);
mmput(mm);
out:
