[PATCH] drm/amdgpu: clear the entire mapping when partially unmapping

[PATCH] drm/amdgpu: clear the entire mapping when partially unmapping

[Thadeu Lima de Souza Cascardo]

When we partially clear a BO mapping, we only add the partially unmapped
region to the freed list. That will update the page table to clear that
mapping and leave the other endpoints mapped.

However, we move the endpoints to the invalids list. When later closing
that BO, it will simply free the mapping and not clear the GPU page
tables.

Drop the optimization and clear the entire mapping from the page tables.
As the endpoints are already in the invalids list, when that BO mapping
needs to be accessed, the page tables will be updated.

Though that may clear, then write page table entries and even lead to
new page table allocations in certain situations, those would already
happen in the clear path.

When measuring a given workload, it was noticed an extra allocation in
less than 10% of the cases, but no noticeable extra latency. In any
case, correctness is more important and this fixes the scenario
described.

Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index a99c2ef14be9..808b77083a5e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -2108,12 +2108,6 @@ int amdgpu_vm_bo_clear_mappings(struct amdgpu_device *adev,
 	list_for_each_entry_safe(tmp, next, &removed, list) {
 		amdgpu_vm_it_remove(tmp, &vm->va);
 		list_del(&tmp->list);
-
-		if (tmp->start < saddr)
-		    tmp->start = saddr;
-		if (tmp->last > eaddr)
-		    tmp->last = eaddr;
-
 		tmp->bo_va = NULL;
 		list_add(&tmp->list, &vm->freed);
 		trace_amdgpu_vm_bo_unmap(NULL, tmp);

---
base-commit: 7475ea8b58616b33c04afb976cf0fbaad9dd63fb
change-id: 20261111-amdgpu_partial_mappings_fix-c48d729ef590

Best regards,
--  
Thadeu Lima de Souza Cascardo <cascardo@igalia.com>

Re: [PATCH] drm/amdgpu: clear the entire mapping when partially unmapping

[Christian König]

On 11/11/26 19:28, Thadeu Lima de Souza Cascardo wrote:
> When we partially clear a BO mapping, we only add the partially unmapped
> region to the freed list. That will update the page table to clear that
> mapping and leave the other endpoints mapped.
> 
> However, we move the endpoints to the invalids list. When later closing
> that BO, it will simply free the mapping and not clear the GPU page
> tables.
> 
> Drop the optimization and clear the entire mapping from the page tables.
> As the endpoints are already in the invalids list, when that BO mapping
> needs to be accessed, the page tables will be updated.
> 
> Though that may clear, then write page table entries and even lead to
> new page table allocations in certain situations, those would already
> happen in the clear path.
> 
> When measuring a given workload, it was noticed an extra allocation in
> less than 10% of the cases, but no noticeable extra latency. In any
> case, correctness is more important and this fixes the scenario
> described.
> 
> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com>

Reviewed-by: Christian König <christian.koenig@amd.com>

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index a99c2ef14be9..808b77083a5e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -2108,12 +2108,6 @@ int amdgpu_vm_bo_clear_mappings(struct amdgpu_device *adev,
>  	list_for_each_entry_safe(tmp, next, &removed, list) {
>  		amdgpu_vm_it_remove(tmp, &vm->va);
>  		list_del(&tmp->list);
> -
> -		if (tmp->start < saddr)
> -		    tmp->start = saddr;
> -		if (tmp->last > eaddr)
> -		    tmp->last = eaddr;
> -
>  		tmp->bo_va = NULL;
>  		list_add(&tmp->list, &vm->freed);
>  		trace_amdgpu_vm_bo_unmap(NULL, tmp);
> 
> ---
> base-commit: 7475ea8b58616b33c04afb976cf0fbaad9dd63fb
> change-id: 20261111-amdgpu_partial_mappings_fix-c48d729ef590
> 
> Best regards,
> --  
> Thadeu Lima de Souza Cascardo <cascardo@igalia.com>
> 

Re: [PATCH] drm/amdgpu: clear the entire mapping when partially unmapping

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] drm/amdgpu: unmapped split endpoints in
  amdgpu_vm_bo_clear_mappings()
- [Low] drm/amdgpu: missing Fixes trailer in commit message
--

commit 193793bc04d0356a7faf15b1ecf5a2a8fdd585ce
Author: Thadeu Lima de Souza Cascardo <cascardo@igalia.com>

> drm/amdgpu: clear the entire mapping when partially unmapping
>
> When we partially clear a BO mapping, we only add the partially unmapped
> region to the freed list. That will update the page table to clear that
> mapping and leave the other endpoints mapped.
>
> However, we move the endpoints to the invalids list. When later closing
> that BO, it will simply free the mapping and not clear the GPU page
> tables.
>
> Drop the optimization and clear the entire mapping from the page tables.
> As the endpoints are already in the invalids list, when that BO mapping
> needs to be accessed, the page tables will be updated.
>
> Though that may clear, then write page table entries and even lead to
> new page table allocations in certain situations, those would already
> happen in the clear path.
>
> When measuring a given workload, it was noticed an extra allocation in
> less than 10% of the cases, but no noticeable extra latency. In any
> case, correctness is more important and this fixes the scenario
> described.
>
> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com>

[Severity: Low]
Since this fixes a page table leak on partial unmap, should it include a
Fixes: tag for 387f49e54672 ("drm/amdgpu: fix clear_all and replace
handling in the VM (v2)") or dc54d3d1744d ("drm/amdgpu: implement
AMDGPU_VA_OP_CLEAR v2")?

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index a99c2ef14be96..808b77083a5e4 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -2108,12 +2108,6 @@ int amdgpu_vm_bo_clear_mappings(struct amdgpu_device *adev,
>  	list_for_each_entry_safe(tmp, next, &removed, list) {
>  		amdgpu_vm_it_remove(tmp, &vm->va);
>  		list_del(&tmp->list);
> -
> -		if (tmp->start < saddr)
> -		    tmp->start = saddr;
> -		if (tmp->last > eaddr)
> -		    tmp->last = eaddr;
> -
>  		tmp->bo_va = NULL;
>  		list_add(&tmp->list, &vm->freed);
>  		trace_amdgpu_vm_bo_unmap(NULL, tmp);

[Severity: High]
Does queuing the unclamped mapping onto vm->freed leave the surviving
before and after endpoints unmapped in the hardware page tables?

When amdgpu_vm_clear_freed() processes vm->freed, it now zeroes the PTEs
for the entire original [tmp->start, tmp->last] range, wiping the PTEs of
the surviving before and after sub-ranges.  However, two paths fail to
re-populate those endpoints from bo_va->invalids:

1. In amdgpu_vm_bo_clear_mappings(), amdgpu_vm_bo_needs_update() is only
called when amdgpu_vm_is_bo_always_valid(vm, bo) is true:

drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:amdgpu_vm_bo_clear_mappings() {
    ...
	if (!list_empty(&before->list)) {
		struct amdgpu_bo *bo = before->bo_va->base.bo;
    ...
		if (amdgpu_vm_is_bo_always_valid(vm, bo) &&
		    !before->bo_va->base.moved)
			amdgpu_vm_bo_needs_update(&before->bo_va->base);
	}
    ...
}

For normal individual BOs, before->bo_va and after->bo_va stay on
vm->individual.idle instead of moving to vm->individual.needs_update, so
amdgpu_vm_handle_moved() and amdgpu_userq_bo_validate() skip them and
never restore their cleared PTEs unless the BO is explicitly passed in a
legacy CS bo_list.

2. On immediate updates without AMDGPU_VM_DELAY_UPDATE,
amdgpu_gem_va_update_vm() runs amdgpu_vm_clear_freed() and only calls
amdgpu_vm_bo_update() on the ioctl target bo_va for MAP or REPLACE:

drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c:amdgpu_gem_va_update_vm() {
    ...
	r = amdgpu_vm_clear_freed(adev, vm, &vm->last_update);
	if (r)
		goto error;

	/* For MAP/REPLACE we also need to update the BO mappings. */
	if (operation == AMDGPU_VA_OP_MAP ||
	    operation == AMDGPU_VA_OP_REPLACE) {
		r = amdgpu_vm_bo_update(adev, bo_va, false);
		if (r)
			goto error;
	}
    ...
}

When userspace partially unmaps via AMDGPU_VA_OP_CLEAR (where bo_va is
NULL) or splits a mapping belonging to another bo_va via
AMDGPU_VA_OP_REPLACE, the surviving endpoints are cleared by
amdgpu_vm_clear_freed() and never re-populated, causing GPU page faults
when accessed (for instance on usermode queues synchronizing via
vm_timeline_syncobj_out).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111-amdgpu_partial_mappings_fix-v1-1-02c5dfa20889@igalia.com?part=1