Hi Christian,
thanks for the review. I understand that re-adding the mapping to
vm->freed is not a valid approach, so I will drop this patch.
The failure path I had in mind was the interruptible CPU update wait in
amdgpu_vm_cpu_prepare():
amdgpu_sync_wait(sync, true)
-> dma_fence_wait(fence, true)
I do not have a minimal reproducer yet. I will investigate the actual
call path further before proposing another change.
Regards,
oushinnyo
At 2026-09-07 20:04:16, "Christian König" <[email protected]> wrote:
>On 9/5/26 04:31, oushinnyo wrote:
>> amdgpu_vm_clear_freed() removes a mapping from vm->freed before
>> clearing its page table entries. If amdgpu_vm_update_range() fails,
>> the mapping is currently freed even though the PTE update might not
>> have completed.
>>
>> This is particularly problematic with CPU VM updates. Waiting for an
>> earlier submission is interruptible, so a signal can abort the wait
>> before any PTE is cleared. The old mapping then exists neither in the
>> VA tree nor on the freed list, allowing a later VM update to reuse the
>> virtual address while an earlier submission still references it.
>
>That would be indeed problematic, where exactly do you see that? Do you have a
>test case for this?
>> Put the mapping back on vm->freed on every update error. This keeps the
>> pending clear operation alive and makes later VM updates retry it before
>> installing new PTEs. It also covers partial CPU or SDMA updates
>> conservatively.
>
>Clear NAK to that approach.
>
>Freed up areas of the page tables must be cleared when that function is called
>or otherwise we run into tons of problems reverting previous changes to the
>housekeeping structures.
>
>The only valid reason this function can fail is because the device is hot
>removed and in that case it doesn't matter any more what we do here.
>
>Regards,
>Christian.
>
>>
>> Fixes: b4d42511b7f2 ("drm/amdgpu: Support page table update via CPU")
>> Cc: [email protected]
>> Assisted-by: Codex:gpt-5
>> Signed-off-by: oushinnyo <[email protected]>
>> ---
>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index 47cde5459469..e20905d2d90c 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -1606,11 +1606,12 @@ int amdgpu_vm_clear_freed(struct amdgpu_device *adev,
>> r = amdgpu_vm_update_range(adev, vm, false, false, true,
>> false,
>> &sync, mapping->start,
>> mapping->last,
>> 0, 0, 0, NULL, NULL, &f);
>> - amdgpu_vm_free_mapping(adev, vm, mapping, f);
>> if (r) {
>> + list_add(&mapping->list, &vm->freed);
>> dma_fence_put(f);
>> goto error_free;
>> }
>> + amdgpu_vm_free_mapping(adev, vm, mapping, f);
>> }
>>
>> if (fence && f) {
>> --
>> 2.25.1
>>
