On 5/28/2026 4:06 PM, Christian König wrote:
On 5/28/26 09:22, Huang, Honglei1 wrote:
AMD General
-----Original Message-----
From: Koenig, Christian <[email protected]>
Sent: Thursday, May 28, 2026 2:54 PM
To: Huang, Honglei1 <[email protected]>; [email protected]
Cc: Deucher, Alexander <[email protected]>; Huang, Ray <[email protected]>;
Prosyak, Vitaly <[email protected]>; Liu, Jenny (Jing) <[email protected]>
Subject: Re: [PATCH] drm/amdgpu: move notifier_seq read back inside retry loop
On 5/28/26 08:29, Honglei Huang wrote:
Align with drm_gpusvm_get_pages() (drm_gpusvm.c line 1416, 1440) which
refreshes notifier_seq via mmu_interval_read_begin() on each retry
iteration. Without this, a stale sequence number causes
hmm_range_fault() to perpetually return -EBUSY, leading to an infinite
retry loop at the caller level.
Absolutely clear NAK.
This is exactly the bug 144ba981783f ("drm/amdgpu: fix
amdgpu_hmm_range_get_pages") tries to fix.
The sequence *must* be grabbed before the loop because it protects all pages
and not just the one from the current grabbed chunk.
What probably needs to happen is that we need to move the retry label or just
completely stop retrying at all.
Got it.
But in my local test, the test KFDMemoryTest.LargestSysBufferTest always fail.
I mean that's pretty much intentional. Testing is for falsification and not
validation.
Either the use case is not valid in the first place or the test is not valid.
Yes, under the current pattern this case is hard to pass for large
system memory platform.
It can pass before. This case needs RAM size big enough to reproduce,
for some large RAM size CI platform, it can reproduce easily.
Maybe someone can else can double check.
And I understand your concern about the seq scope.
Would it be acceptable to just remove the internal retry entirely and
propagate -EBUSY to the caller?
Yeah I had a similar thought. I don't think the code should retry at all.
-EBUSY means userspace did something in parallel which resulting in the
operation to not be able to complete.
So the only good reaction I can see is to abort and let user space or higher
level retry.
Yes, and actually I think maybe there are two points need to be changed.
If we want to keep the retry tag, the retry need to be moved to the
start of do while loop, the entire fault path need to be done again,
the previous 2G size chunks should not be skipped. The retry tag in the
middle of the loop is meaningless. The whole range should be retry again.
And the latest seq number should be get when restart the retry. Or there
will be a -EBUSY infinite loop when a retry is needed, cause the seq
number never get updated.
This can be done in inside a function or outside a function.
If a retry is needed at a high level calling, modifications will also be
required at other call points, and there are many places need to be
modified.
However, even with the above fixes, faults in large intervals may still
get stuck in endless retries. I got a the pages from a system with 60G
of memory, memory changes are inevitable in my local tests.
Perhaps the better approach is to split the huge range into smaller
chunks, like drm_gpusvm does with 2 MiB ranges, and get the pages and
insert each one into the MMU notifier individually.
In this way, registration for a large area can be completed, although it
will be time consuming.
But it's better than the situation of constantly trying again and again.
Regards,
Honglei
Regards,
Christian.
The caller already handles retry at a
higher level. Something like:
hmm_range->notifier_seq = mmu_interval_read_begin(notifier);
r = hmm_range_fault(hmm_range);
if (unlikely(r))
goto out_free_pfns;
This keeps your seq placement while eliminating the infinite loop on
-EBUSY.
Regards,
Christian.
Fixes: 144ba981783f ("drm/amdgpu: fix amdgpu_hmm_range_get_pages")
Signed-off-by: Honglei Huang <[email protected]>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c
index 5d72878c8..ec0fe9044 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c
@@ -192,7 +192,6 @@ int amdgpu_hmm_range_get_pages(struct mmu_interval_notifier
*notifier,
end = start + npages * PAGE_SIZE;
hmm_range->dev_private_owner = owner;
- hmm_range->notifier_seq = mmu_interval_read_begin(notifier);
do {
hmm_range->end = min(hmm_range->start + max_bytes, end);
@@ -202,6 +201,7 @@ int amdgpu_hmm_range_get_pages(struct mmu_interval_notifier
*notifier,
timeout = jiffies + msecs_to_jiffies(HMM_RANGE_DEFAULT_TIMEOUT);
retry:
+ hmm_range->notifier_seq = mmu_interval_read_begin(notifier);
r = hmm_range_fault(hmm_range);
if (unlikely(r)) {
if (r == -EBUSY && !time_after(jiffies, timeout))
