[PATCH 1/2] drm/amdgpu: nuke most amdgpu_vm_eviction_(try)lock uses

[PATCH 1/2] drm/amdgpu: nuke most amdgpu_vm_eviction_(try)lock uses

[Christian König]

Use scoped_guard/scoped_cond_guard instead.

Saving and restoring the GFP flags is not necessary in those use case.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c        | 27 +++++++++----------
 .../gpu/drm/amd/amdgpu/amdgpu_vm_internal.h   |  9 -------
 2 files changed, 12 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 47cde54594693..7ced26c9c651b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -616,9 +616,8 @@ int amdgpu_vm_validate(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 	 * As soon as all page tables are in place we can start updating them
 	 * again.
 	 */
-	amdgpu_vm_eviction_lock(vm);
-	vm->evicting = false;
-	amdgpu_vm_eviction_unlock(vm);
+	scoped_guard(mutex, &vm->eviction_lock)
+		vm->evicting = false;
 
 	list_for_each_entry_safe(bo_base, tmp, &vm->always_valid.evicted,
 				 vm_status) {
@@ -678,9 +677,8 @@ bool amdgpu_vm_ready(struct amdgpu_vm *vm)
 
 	amdgpu_vm_assert_locked(vm);
 
-	amdgpu_vm_eviction_lock(vm);
-	ret = !vm->evicting;
-	amdgpu_vm_eviction_unlock(vm);
+	scoped_guard(mutex, &vm->eviction_lock)
+		ret = !vm->evicting;
 
 	ret &= list_empty(&vm->kernel.evicted);
 
@@ -2274,6 +2272,7 @@ void amdgpu_vm_bo_del(struct amdgpu_device *adev,
 bool amdgpu_vm_evictable(struct amdgpu_bo *bo)
 {
 	struct amdgpu_vm_bo_base *bo_base = bo->vm_bo;
+	struct amdgpu_vm *vm;
 
 	/* Page tables of a destroyed VM can go away immediately */
 	if (!bo_base || !bo_base->vm)
@@ -2284,17 +2283,15 @@ bool amdgpu_vm_evictable(struct amdgpu_bo *bo)
 		return false;
 
 	/* Try to block ongoing updates */
-	if (!amdgpu_vm_eviction_trylock(bo_base->vm))
-		return false;
+	vm = bo_base->vm;
+	scoped_cond_guard(mutex_try, return false, &vm->eviction_lock) {
 
-	/* Don't evict VM page tables while they are updated */
-	if (!dma_fence_is_signaled(bo_base->vm->last_unlocked)) {
-		amdgpu_vm_eviction_unlock(bo_base->vm);
-		return false;
-	}
+		/* Don't evict VM page tables while they are updated */
+		if (!dma_fence_is_signaled(vm->last_unlocked))
+			return false;
 
-	bo_base->vm->evicting = true;
-	amdgpu_vm_eviction_unlock(bo_base->vm);
+		vm->evicting = true;
+	}
 	return true;
 }
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_internal.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_internal.h
index 447195cc45932..ca86eaac75235 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_internal.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_internal.h
@@ -137,15 +137,6 @@ static inline void amdgpu_vm_eviction_lock(struct amdgpu_vm *vm)
 	vm->saved_flags = memalloc_noreclaim_save();
 }
 
-static inline bool amdgpu_vm_eviction_trylock(struct amdgpu_vm *vm)
-{
-	if (mutex_trylock(&vm->eviction_lock)) {
-		vm->saved_flags = memalloc_noreclaim_save();
-		return true;
-	}
-	return false;
-}
-
 static inline void amdgpu_vm_eviction_unlock(struct amdgpu_vm *vm)
 {
 	memalloc_noreclaim_restore(vm->saved_flags);
-- 
2.43.0

[PATCH 2/2] drm/amdgpu: rework eviction lock handling into critical section v2

[Christian König]

Taking the eviction lock is actually just one step which we need to do
in the critical section handling.

Rename the functions to reflect that, use the update parameters instead of the
vm to save the GFP flags.

v2: rebased and reordered

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c        | 21 +++++++---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h        |  1 -
 .../gpu/drm/amd/amdgpu/amdgpu_vm_internal.h   | 41 ++++++++++++++-----
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c     | 35 ++++++++--------
 4 files changed, 63 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 7ced26c9c651b..67dc7da1a02fa 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -1169,11 +1169,9 @@ int amdgpu_vm_update_range(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 	params.override_pte = allow_override && adev->gmc.override_pte;
 	INIT_LIST_HEAD(&params.tlb_flush_waitlist);
 
-	amdgpu_vm_eviction_lock(vm);
-	if (vm->evicting) {
-		r = -EBUSY;
+	r = amdgpu_vm_begin_critical(&params);
+	if (r)
 		goto error_free;
-	}
 
 	if (!unlocked && !dma_fence_is_signaled(vm->last_unlocked)) {
 		struct dma_fence *tmp = dma_fence_get_stub();
@@ -1257,7 +1255,7 @@ int amdgpu_vm_update_range(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 
 error_free:
 	kfree(tlb_cb);
-	amdgpu_vm_eviction_unlock(vm);
+	amdgpu_vm_end_critical(&params);
 	drm_dev_exit(idx);
 	return r;
 }
@@ -3044,6 +3042,7 @@ bool amdgpu_vm_handle_fault(struct amdgpu_device *adev, u32 pasid,
 			    u32 vmid, u32 node_id, uint64_t addr,
 			    uint64_t ts, bool write_fault)
 {
+	struct amdgpu_vm_update_params params;
 	bool is_compute_context = false;
 	struct drm_exec exec;
 	uint64_t value, flags;
@@ -3117,6 +3116,15 @@ bool amdgpu_vm_handle_fault(struct amdgpu_device *adev, u32 pasid,
 		goto error_unlock;
 	}
 
+	memset(&params, 0, sizeof(params));
+	params.adev = adev;
+	params.vm = vm;
+	params.immediate = true;
+
+	r = amdgpu_vm_begin_critical(&params);
+	if (r)
+		goto error_end_critical;
+
 	r = amdgpu_vm_update_range(adev, vm, true, false, false, false,
 				   NULL, addr, addr, flags, value, 0, NULL, NULL, NULL);
 	if (r)
@@ -3124,6 +3132,9 @@ bool amdgpu_vm_handle_fault(struct amdgpu_device *adev, u32 pasid,
 
 	r = amdgpu_vm_update_pdes(adev, vm, true);
 
+error_end_critical:
+	amdgpu_vm_end_critical(&params);
+
 error_unlock:
 	drm_exec_fini(&exec);
 	if (r < 0)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
index 0f6634b749746..98cdd7e3475fb 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -289,7 +289,6 @@ struct amdgpu_vm {
 	 */
 	struct mutex		eviction_lock;
 	bool			evicting;
-	unsigned int		saved_flags;
 
 	/* Memory statistics for this vm, protected by stats_lock */
 	spinlock_t		stats_lock;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_internal.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_internal.h
index ca86eaac75235..77f3942d9533b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_internal.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_internal.h
@@ -93,6 +93,11 @@ struct amdgpu_vm_update_params {
 	 */
 	bool override_pte;
 
+	/**
+	 * @saved_flags: Saved flags for GFP reduction.
+	 */
+	unsigned int saved_flags;
+
 	/**
 	 * @tlb_flush_waitlist: temporary storage for BOs until tlb_flush
 	 */
@@ -126,21 +131,37 @@ void amdgpu_vm_pt_free_list(struct amdgpu_device *adev,
 			    struct amdgpu_vm_update_params *params);
 int amdgpu_vm_pt_map_tables(struct amdgpu_device *adev, struct amdgpu_vm *vm);
 
-/*
- * vm eviction_lock can be taken in MMU notifiers. Make sure no reclaim-FS
- * happens while holding this lock anywhere to prevent deadlocks when
- * an MMU notifier runs in reclaim-FS context.
+/**
+ * amdgpu_vm_begin_critical - start the critical section of the update
+ * @p: The update parameters
+ *
+ * Serialize all updates, check parameters and make sure that memory allocations
+ * don't enter the reclaim path so that we don't deadlock with MMU notifiers.
+ *
+ * Returns:
+ *
+ * 0 on success or a negative error code on failure.
+ * Even on error amdgpu_vm_end_critical() must still be called to clean up!
  */
-static inline void amdgpu_vm_eviction_lock(struct amdgpu_vm *vm)
+static inline int amdgpu_vm_begin_critical(struct amdgpu_vm_update_params *p)
 {
-	mutex_lock(&vm->eviction_lock);
-	vm->saved_flags = memalloc_noreclaim_save();
+	mutex_lock(&p->vm->eviction_lock);
+	p->saved_flags = memalloc_noreclaim_save();
+	if (p->vm->evicting)
+		return -EBUSY;
+	return 0;
 }
 
-static inline void amdgpu_vm_eviction_unlock(struct amdgpu_vm *vm)
+/**
+ * amdgpu_vm_end_critical - end the critical section of the update
+ * @p: The update parameters
+ *
+ * Restore the GFP flags and drop the lock.
+ */
+static inline void amdgpu_vm_end_critical(struct amdgpu_vm_update_params *p)
 {
-	memalloc_noreclaim_restore(vm->saved_flags);
-	mutex_unlock(&vm->eviction_lock);
+	memalloc_noreclaim_restore(p->saved_flags);
+	mutex_unlock(&p->vm->eviction_lock);
 }
 
 #endif
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
index 1cdf2b854f261..3f78202a60585 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
@@ -505,51 +505,49 @@ int amdgpu_vm_pt_create(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 /**
  * amdgpu_vm_pt_alloc - Allocate a specific page table
  *
- * @adev: amdgpu_device pointer
- * @vm: VM to allocate page tables for
+ * @p: see amdgpu_vm_update_params definition
  * @cursor: Which page table to allocate
- * @immediate: use an immediate update
  *
  * Make sure a specific page table or directory is allocated.
  *
  * Returns:
- * 1 if page table needed to be allocated, 0 if page table was already
- * allocated, negative errno if an error occurred.
+ *
+ * 0 on success or a negative error code on failure.
  */
-static int amdgpu_vm_pt_alloc(struct amdgpu_device *adev,
-			      struct amdgpu_vm *vm,
-			      struct amdgpu_vm_pt_cursor *cursor,
-			      bool immediate)
+static int amdgpu_vm_pt_alloc(struct amdgpu_vm_update_params *p,
+			      struct amdgpu_vm_pt_cursor *cursor)
 {
 	struct amdgpu_vm_bo_base *entry = cursor->entry;
 	struct amdgpu_bo *pt_bo;
 	struct amdgpu_bo_vm *pt;
-	int r;
+	int r, r2;
 
 	if (entry->bo)
 		return 0;
 
-	amdgpu_vm_eviction_unlock(vm);
-	r = amdgpu_vm_pt_create(adev, vm, cursor->level, immediate, &pt,
-				vm->root.bo->xcp_id);
-	amdgpu_vm_eviction_lock(vm);
+	amdgpu_vm_end_critical(p);
+	r = amdgpu_vm_pt_create(p->adev, p->vm, cursor->level, p->immediate,
+				&pt, p->vm->root.bo->xcp_id);
+	r2 = amdgpu_vm_begin_critical(p);
 	if (r)
 		return r;
+	if (r2)
+		return r2;
 
 	/* Keep a reference to the root directory to avoid
 	 * freeing them up in the wrong order.
 	 */
 	pt_bo = &pt->bo;
 	pt_bo->parent = amdgpu_bo_ref(cursor->parent->bo);
-	amdgpu_vm_bo_base_init(entry, vm, pt_bo);
-	r = amdgpu_vm_pt_clear(adev, vm, pt, immediate);
+	amdgpu_vm_bo_base_init(entry, p->vm, pt_bo);
+	r = amdgpu_vm_pt_clear(p->adev, p->vm, pt, p->immediate);
 	if (r)
 		goto error_free_pt;
 
 	return 0;
 
 error_free_pt:
-	if (vm->is_npa)
+	if (p->vm->is_npa)
 		amdgpu_bo_unpin(pt_bo);
 	amdgpu_bo_unref(&pt_bo);
 	return r;
@@ -838,8 +836,7 @@ int amdgpu_vm_ptes_update(struct amdgpu_vm_update_params *params,
 			/* make sure that the page tables covering the
 			 * address range are actually allocated
 			 */
-			r = amdgpu_vm_pt_alloc(params->adev, params->vm,
-					       &cursor, params->immediate);
+			r = amdgpu_vm_pt_alloc(params, &cursor);
 			if (r)
 				return r;
 		}
-- 
2.43.0

Re: [PATCH 2/2] drm/amdgpu: rework eviction lock handling into critical section v2

[Natalie Vock]

Hi,

On 9/11/26 18:48, Christian König wrote:
> Taking the eviction lock is actually just one step which we need to do
> in the critical section handling.
> 
> Rename the functions to reflect that, use the update parameters instead of the
> vm to save the GFP flags.
> 
> v2: rebased and reordered
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c        | 21 +++++++---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h        |  1 -
>   .../gpu/drm/amd/amdgpu/amdgpu_vm_internal.h   | 41 ++++++++++++++-----
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c     | 35 ++++++++--------
>   4 files changed, 63 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 7ced26c9c651b..67dc7da1a02fa 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -1169,11 +1169,9 @@ int amdgpu_vm_update_range(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>   	params.override_pte = allow_override && adev->gmc.override_pte;
>   	INIT_LIST_HEAD(&params.tlb_flush_waitlist);
>   
> -	amdgpu_vm_eviction_lock(vm);
> -	if (vm->evicting) {
> -		r = -EBUSY;
> +	r = amdgpu_vm_begin_critical(&params);
> +	if (r)
>   		goto error_free;
> -	}
>   
>   	if (!unlocked && !dma_fence_is_signaled(vm->last_unlocked)) {
>   		struct dma_fence *tmp = dma_fence_get_stub();
> @@ -1257,7 +1255,7 @@ int amdgpu_vm_update_range(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>   
>   error_free:
>   	kfree(tlb_cb);
> -	amdgpu_vm_eviction_unlock(vm);
> +	amdgpu_vm_end_critical(&params);
>   	drm_dev_exit(idx);
>   	return r;
>   }
> @@ -3044,6 +3042,7 @@ bool amdgpu_vm_handle_fault(struct amdgpu_device *adev, u32 pasid,
>   			    u32 vmid, u32 node_id, uint64_t addr,
>   			    uint64_t ts, bool write_fault)
>   {
> +	struct amdgpu_vm_update_params params;
>   	bool is_compute_context = false;
>   	struct drm_exec exec;
>   	uint64_t value, flags;
> @@ -3117,6 +3116,15 @@ bool amdgpu_vm_handle_fault(struct amdgpu_device *adev, u32 pasid,
>   		goto error_unlock;
>   	}
>   
> +	memset(&params, 0, sizeof(params));
> +	params.adev = adev;
> +	params.vm = vm;
> +	params.immediate = true;
> +
> +	r = amdgpu_vm_begin_critical(&params);
> +	if (r)
> +		goto error_end_critical;
> +
>   	r = amdgpu_vm_update_range(adev, vm, true, false, false, false,
>   				   NULL, addr, addr, flags, value, 0, NULL, NULL, NULL);
>   	if (r)
> @@ -3124,6 +3132,9 @@ bool amdgpu_vm_handle_fault(struct amdgpu_device *adev, u32 pasid,
>   
>   	r = amdgpu_vm_update_pdes(adev, vm, true);
>   
> +error_end_critical:
> +	amdgpu_vm_end_critical(&params);
> +
>   error_unlock:
>   	drm_exec_fini(&exec);
>   	if (r < 0)
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> index 0f6634b749746..98cdd7e3475fb 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> @@ -289,7 +289,6 @@ struct amdgpu_vm {
>   	 */
>   	struct mutex		eviction_lock;
>   	bool			evicting;
> -	unsigned int		saved_flags;
>   
>   	/* Memory statistics for this vm, protected by stats_lock */
>   	spinlock_t		stats_lock;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_internal.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_internal.h
> index ca86eaac75235..77f3942d9533b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_internal.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_internal.h

It's not shown in the diff here, but the comment above
amdgpu_vm_update_params.vm suggests that the VM is an optional
parameter, which, IIUC, isn't the case anymore. The "optional" should
probably be removed from the comment

This small nitpick aside, the series looks good to me.

Reviewed-by: Natalie Vock <nat@pixelcluster.dev>

Best,
Natalie

> @@ -93,6 +93,11 @@ struct amdgpu_vm_update_params {
>   	 */
>   	bool override_pte;
>   
> +	/**
> +	 * @saved_flags: Saved flags for GFP reduction.
> +	 */
> +	unsigned int saved_flags;
> +
>   	/**
>   	 * @tlb_flush_waitlist: temporary storage for BOs until tlb_flush
>   	 */
> @@ -126,21 +131,37 @@ void amdgpu_vm_pt_free_list(struct amdgpu_device *adev,
>   			    struct amdgpu_vm_update_params *params);
>   int amdgpu_vm_pt_map_tables(struct amdgpu_device *adev, struct amdgpu_vm *vm);
>   
> -/*
> - * vm eviction_lock can be taken in MMU notifiers. Make sure no reclaim-FS
> - * happens while holding this lock anywhere to prevent deadlocks when
> - * an MMU notifier runs in reclaim-FS context.
> +/**
> + * amdgpu_vm_begin_critical - start the critical section of the update
> + * @p: The update parameters
> + *
> + * Serialize all updates, check parameters and make sure that memory allocations
> + * don't enter the reclaim path so that we don't deadlock with MMU notifiers.
> + *
> + * Returns:
> + *
> + * 0 on success or a negative error code on failure.
> + * Even on error amdgpu_vm_end_critical() must still be called to clean up!
>    */
> -static inline void amdgpu_vm_eviction_lock(struct amdgpu_vm *vm)
> +static inline int amdgpu_vm_begin_critical(struct amdgpu_vm_update_params *p)
>   {
> -	mutex_lock(&vm->eviction_lock);
> -	vm->saved_flags = memalloc_noreclaim_save();
> +	mutex_lock(&p->vm->eviction_lock);
> +	p->saved_flags = memalloc_noreclaim_save();
> +	if (p->vm->evicting)
> +		return -EBUSY;
> +	return 0;
>   }
>   
> -static inline void amdgpu_vm_eviction_unlock(struct amdgpu_vm *vm)
> +/**
> + * amdgpu_vm_end_critical - end the critical section of the update
> + * @p: The update parameters
> + *
> + * Restore the GFP flags and drop the lock.
> + */
> +static inline void amdgpu_vm_end_critical(struct amdgpu_vm_update_params *p)
>   {
> -	memalloc_noreclaim_restore(vm->saved_flags);
> -	mutex_unlock(&vm->eviction_lock);
> +	memalloc_noreclaim_restore(p->saved_flags);
> +	mutex_unlock(&p->vm->eviction_lock);
>   }
>   
>   #endif
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
> index 1cdf2b854f261..3f78202a60585 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
> @@ -505,51 +505,49 @@ int amdgpu_vm_pt_create(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>   /**
>    * amdgpu_vm_pt_alloc - Allocate a specific page table
>    *
> - * @adev: amdgpu_device pointer
> - * @vm: VM to allocate page tables for
> + * @p: see amdgpu_vm_update_params definition
>    * @cursor: Which page table to allocate
> - * @immediate: use an immediate update
>    *
>    * Make sure a specific page table or directory is allocated.
>    *
>    * Returns:
> - * 1 if page table needed to be allocated, 0 if page table was already
> - * allocated, negative errno if an error occurred.
> + *
> + * 0 on success or a negative error code on failure.
>    */
> -static int amdgpu_vm_pt_alloc(struct amdgpu_device *adev,
> -			      struct amdgpu_vm *vm,
> -			      struct amdgpu_vm_pt_cursor *cursor,
> -			      bool immediate)
> +static int amdgpu_vm_pt_alloc(struct amdgpu_vm_update_params *p,
> +			      struct amdgpu_vm_pt_cursor *cursor)
>   {
>   	struct amdgpu_vm_bo_base *entry = cursor->entry;
>   	struct amdgpu_bo *pt_bo;
>   	struct amdgpu_bo_vm *pt;
> -	int r;
> +	int r, r2;
>   
>   	if (entry->bo)
>   		return 0;
>   
> -	amdgpu_vm_eviction_unlock(vm);
> -	r = amdgpu_vm_pt_create(adev, vm, cursor->level, immediate, &pt,
> -				vm->root.bo->xcp_id);
> -	amdgpu_vm_eviction_lock(vm);
> +	amdgpu_vm_end_critical(p);
> +	r = amdgpu_vm_pt_create(p->adev, p->vm, cursor->level, p->immediate,
> +				&pt, p->vm->root.bo->xcp_id);
> +	r2 = amdgpu_vm_begin_critical(p);
>   	if (r)
>   		return r;
> +	if (r2)
> +		return r2;
>   
>   	/* Keep a reference to the root directory to avoid
>   	 * freeing them up in the wrong order.
>   	 */
>   	pt_bo = &pt->bo;
>   	pt_bo->parent = amdgpu_bo_ref(cursor->parent->bo);
> -	amdgpu_vm_bo_base_init(entry, vm, pt_bo);
> -	r = amdgpu_vm_pt_clear(adev, vm, pt, immediate);
> +	amdgpu_vm_bo_base_init(entry, p->vm, pt_bo);
> +	r = amdgpu_vm_pt_clear(p->adev, p->vm, pt, p->immediate);
>   	if (r)
>   		goto error_free_pt;
>   
>   	return 0;
>   
>   error_free_pt:
> -	if (vm->is_npa)
> +	if (p->vm->is_npa)
>   		amdgpu_bo_unpin(pt_bo);
>   	amdgpu_bo_unref(&pt_bo);
>   	return r;
> @@ -838,8 +836,7 @@ int amdgpu_vm_ptes_update(struct amdgpu_vm_update_params *params,
>   			/* make sure that the page tables covering the
>   			 * address range are actually allocated
>   			 */
> -			r = amdgpu_vm_pt_alloc(params->adev, params->vm,
> -					       &cursor, params->immediate);
> +			r = amdgpu_vm_pt_alloc(params, &cursor);
>   			if (r)
>   				return r;
>   		}

Re: [PATCH 1/2] drm/amdgpu: nuke most amdgpu_vm_eviction_(try)lock uses

[Christian König]

Hi Honglei,

this is just to unblock your SVM work, the rest of my patch set needs more work than I thought because of the new ualink files.

Regards,
Christian.

On 9/11/26 18:48, Christian König wrote:
> Use scoped_guard/scoped_cond_guard instead.
> 
> Saving and restoring the GFP flags is not necessary in those use case.
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c        | 27 +++++++++----------
>  .../gpu/drm/amd/amdgpu/amdgpu_vm_internal.h   |  9 -------
>  2 files changed, 12 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 47cde54594693..7ced26c9c651b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -616,9 +616,8 @@ int amdgpu_vm_validate(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>  	 * As soon as all page tables are in place we can start updating them
>  	 * again.
>  	 */
> -	amdgpu_vm_eviction_lock(vm);
> -	vm->evicting = false;
> -	amdgpu_vm_eviction_unlock(vm);
> +	scoped_guard(mutex, &vm->eviction_lock)
> +		vm->evicting = false;
>  
>  	list_for_each_entry_safe(bo_base, tmp, &vm->always_valid.evicted,
>  				 vm_status) {
> @@ -678,9 +677,8 @@ bool amdgpu_vm_ready(struct amdgpu_vm *vm)
>  
>  	amdgpu_vm_assert_locked(vm);
>  
> -	amdgpu_vm_eviction_lock(vm);
> -	ret = !vm->evicting;
> -	amdgpu_vm_eviction_unlock(vm);
> +	scoped_guard(mutex, &vm->eviction_lock)
> +		ret = !vm->evicting;
>  
>  	ret &= list_empty(&vm->kernel.evicted);
>  
> @@ -2274,6 +2272,7 @@ void amdgpu_vm_bo_del(struct amdgpu_device *adev,
>  bool amdgpu_vm_evictable(struct amdgpu_bo *bo)
>  {
>  	struct amdgpu_vm_bo_base *bo_base = bo->vm_bo;
> +	struct amdgpu_vm *vm;
>  
>  	/* Page tables of a destroyed VM can go away immediately */
>  	if (!bo_base || !bo_base->vm)
> @@ -2284,17 +2283,15 @@ bool amdgpu_vm_evictable(struct amdgpu_bo *bo)
>  		return false;
>  
>  	/* Try to block ongoing updates */
> -	if (!amdgpu_vm_eviction_trylock(bo_base->vm))
> -		return false;
> +	vm = bo_base->vm;
> +	scoped_cond_guard(mutex_try, return false, &vm->eviction_lock) {
>  
> -	/* Don't evict VM page tables while they are updated */
> -	if (!dma_fence_is_signaled(bo_base->vm->last_unlocked)) {
> -		amdgpu_vm_eviction_unlock(bo_base->vm);
> -		return false;
> -	}
> +		/* Don't evict VM page tables while they are updated */
> +		if (!dma_fence_is_signaled(vm->last_unlocked))
> +			return false;
>  
> -	bo_base->vm->evicting = true;
> -	amdgpu_vm_eviction_unlock(bo_base->vm);
> +		vm->evicting = true;
> +	}
>  	return true;
>  }
>  
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_internal.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_internal.h
> index 447195cc45932..ca86eaac75235 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_internal.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_internal.h
> @@ -137,15 +137,6 @@ static inline void amdgpu_vm_eviction_lock(struct amdgpu_vm *vm)
>  	vm->saved_flags = memalloc_noreclaim_save();
>  }
>  
> -static inline bool amdgpu_vm_eviction_trylock(struct amdgpu_vm *vm)
> -{
> -	if (mutex_trylock(&vm->eviction_lock)) {
> -		vm->saved_flags = memalloc_noreclaim_save();
> -		return true;
> -	}
> -	return false;
> -}
> -
>  static inline void amdgpu_vm_eviction_unlock(struct amdgpu_vm *vm)
>  {
>  	memalloc_noreclaim_restore(vm->saved_flags);

Re: [PATCH 1/2] drm/amdgpu: nuke most amdgpu_vm_eviction_(try)lock uses

[Huang, Honglei]

Will rebase on this series, and forword Junhua and Yu, casue the rebae, 
fix and testing are a relatively big work, requires multiple people to 
cowork.

Regrds,
Honglei

On 9/12/2026 12:48 AM, Christian König wrote:
> Use scoped_guard/scoped_cond_guard instead.
> 
> Saving and restoring the GFP flags is not necessary in those use case.
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c        | 27 +++++++++----------
>   .../gpu/drm/amd/amdgpu/amdgpu_vm_internal.h   |  9 -------
>   2 files changed, 12 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 47cde54594693..7ced26c9c651b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -616,9 +616,8 @@ int amdgpu_vm_validate(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>   	 * As soon as all page tables are in place we can start updating them
>   	 * again.
>   	 */
> -	amdgpu_vm_eviction_lock(vm);
> -	vm->evicting = false;
> -	amdgpu_vm_eviction_unlock(vm);
> +	scoped_guard(mutex, &vm->eviction_lock)
> +		vm->evicting = false;
>   
>   	list_for_each_entry_safe(bo_base, tmp, &vm->always_valid.evicted,
>   				 vm_status) {
> @@ -678,9 +677,8 @@ bool amdgpu_vm_ready(struct amdgpu_vm *vm)
>   
>   	amdgpu_vm_assert_locked(vm);
>   
> -	amdgpu_vm_eviction_lock(vm);
> -	ret = !vm->evicting;
> -	amdgpu_vm_eviction_unlock(vm);
> +	scoped_guard(mutex, &vm->eviction_lock)
> +		ret = !vm->evicting;
>   
>   	ret &= list_empty(&vm->kernel.evicted);
>   
> @@ -2274,6 +2272,7 @@ void amdgpu_vm_bo_del(struct amdgpu_device *adev,
>   bool amdgpu_vm_evictable(struct amdgpu_bo *bo)
>   {
>   	struct amdgpu_vm_bo_base *bo_base = bo->vm_bo;
> +	struct amdgpu_vm *vm;
>   
>   	/* Page tables of a destroyed VM can go away immediately */
>   	if (!bo_base || !bo_base->vm)
> @@ -2284,17 +2283,15 @@ bool amdgpu_vm_evictable(struct amdgpu_bo *bo)
>   		return false;
>   
>   	/* Try to block ongoing updates */
> -	if (!amdgpu_vm_eviction_trylock(bo_base->vm))
> -		return false;
> +	vm = bo_base->vm;
> +	scoped_cond_guard(mutex_try, return false, &vm->eviction_lock) {
>   
> -	/* Don't evict VM page tables while they are updated */
> -	if (!dma_fence_is_signaled(bo_base->vm->last_unlocked)) {
> -		amdgpu_vm_eviction_unlock(bo_base->vm);
> -		return false;
> -	}
> +		/* Don't evict VM page tables while they are updated */
> +		if (!dma_fence_is_signaled(vm->last_unlocked))
> +			return false;
>   
> -	bo_base->vm->evicting = true;
> -	amdgpu_vm_eviction_unlock(bo_base->vm);
> +		vm->evicting = true;
> +	}
>   	return true;
>   }
>   
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_internal.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_internal.h
> index 447195cc45932..ca86eaac75235 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_internal.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_internal.h
> @@ -137,15 +137,6 @@ static inline void amdgpu_vm_eviction_lock(struct amdgpu_vm *vm)
>   	vm->saved_flags = memalloc_noreclaim_save();
>   }
>   
> -static inline bool amdgpu_vm_eviction_trylock(struct amdgpu_vm *vm)
> -{
> -	if (mutex_trylock(&vm->eviction_lock)) {
> -		vm->saved_flags = memalloc_noreclaim_save();
> -		return true;
> -	}
> -	return false;
> -}
> -
>   static inline void amdgpu_vm_eviction_unlock(struct amdgpu_vm *vm)
>   {
>   	memalloc_noreclaim_restore(vm->saved_flags);

Re: [PATCH 1/2] drm/amdgpu: nuke most amdgpu_vm_eviction_(try)lock uses

[Huang, Honglei]

Hi Christian,

We are preparing the amdgpu drmsvm V10 series, have some questions:

1. The patches organization will be:

	drmsvm notifier and eviction lock unify patches
	amdgpu drmsvm patches
	your amdgpu vm fixes

And during the test process, we found some places need to modify, we 
will fold these changes into your amdgpu VM fixes,
  do you need to review them before we do so?

2.  In previous meetings, we cancelled the attributes remove under unmap 
mmu notifier event,
Then we found some issue in kfd and HIP test, do we need the flag: 
AMDGPU_SVM_ATTR_AUTO_RESET like the auto reset flag
In XE driver for backward compatibility?

3. The KFD doesn't support the multi-CPU prefetch/migration (numa 
support), this is a major missing feature in KFD.
And there is an information form ROCR/HIP, UMD have the multi CPU 
prefetch/migration APIs and requirements , and they want to kernel
to support it natively,  the current KFD can not support it, so they 
have to make some workaround for this feature.

So, for support the numa feature, we may need change the AMDGPU UAPI let 
UMD pass the target CPU preferred/prefetch location. Here is the draft 
design for UAPI:

enum amdgpu_ioctl_svm_attr_type {
	AMDGPU_SVM_ATTR_PREFERRED_LOC		= 0,
	AMDGPU_SVM_ATTR_PREFETCH_LOC		= 1,
	AMDGPU_SVM_ATTR_ACCESS				= 2,
	AMDGPU_SVM_ATTR_GRANULARITY			= 3,
	AMDGPU_SVM_ATTR_HOST_ACCESS			= 4,
	AMDGPU_SVM_ATTR_COHERENT			= 5,
	AMDGPU_SVM_ATTR_EXT_COHERENT		= 6,
	AMDGPU_SVM_ATTR_HIVE_LOCAL			= 7,
	AMDGPU_SVM_ATTR_GPU_RO				= 8,
	AMDGPU_SVM_ATTR_GPU_EXEC			= 9,
	AMDGPU_SVM_ATTR_GPU_READ_MOSTLY		= 10,
	AMDGPU_SVM_ATTR_AUTO_RESET		= 11,
	/* new attr for multi cpu/numa  */
     	AMDGPU_SVM_ATTR_PREFERRED_NUMA_NODE = 12,
     	AMDGPU_SVM_ATTR_PREFETCH_NUMA_NODE  = 13,
};

And the value can reuse the raw Linux nid.

Is that ok for you?

Regards,
Honglei



On 9/14/2026 4:04 PM, Huang, Honglei wrote:
> 
> 
> Will rebase on this series, and forword Junhua and Yu, casue the rebae, 
> fix and testing are a relatively big work, requires multiple people to 
> cowork.
> 
> Regrds,
> Honglei
> 
> On 9/12/2026 12:48 AM, Christian König wrote:
>> Use scoped_guard/scoped_cond_guard instead.
>>
>> Saving and restoring the GFP flags is not necessary in those use case.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c        | 27 +++++++++----------
>>   .../gpu/drm/amd/amdgpu/amdgpu_vm_internal.h   |  9 -------
>>   2 files changed, 12 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/ 
>> amd/amdgpu/amdgpu_vm.c
>> index 47cde54594693..7ced26c9c651b 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -616,9 +616,8 @@ int amdgpu_vm_validate(struct amdgpu_device *adev, 
>> struct amdgpu_vm *vm,
>>        * As soon as all page tables are in place we can start updating 
>> them
>>        * again.
>>        */
>> -    amdgpu_vm_eviction_lock(vm);
>> -    vm->evicting = false;
>> -    amdgpu_vm_eviction_unlock(vm);
>> +    scoped_guard(mutex, &vm->eviction_lock)
>> +        vm->evicting = false;
>>       list_for_each_entry_safe(bo_base, tmp, &vm->always_valid.evicted,
>>                    vm_status) {
>> @@ -678,9 +677,8 @@ bool amdgpu_vm_ready(struct amdgpu_vm *vm)
>>       amdgpu_vm_assert_locked(vm);
>> -    amdgpu_vm_eviction_lock(vm);
>> -    ret = !vm->evicting;
>> -    amdgpu_vm_eviction_unlock(vm);
>> +    scoped_guard(mutex, &vm->eviction_lock)
>> +        ret = !vm->evicting;
>>       ret &= list_empty(&vm->kernel.evicted);
>> @@ -2274,6 +2272,7 @@ void amdgpu_vm_bo_del(struct amdgpu_device *adev,
>>   bool amdgpu_vm_evictable(struct amdgpu_bo *bo)
>>   {
>>       struct amdgpu_vm_bo_base *bo_base = bo->vm_bo;
>> +    struct amdgpu_vm *vm;
>>       /* Page tables of a destroyed VM can go away immediately */
>>       if (!bo_base || !bo_base->vm)
>> @@ -2284,17 +2283,15 @@ bool amdgpu_vm_evictable(struct amdgpu_bo *bo)
>>           return false;
>>       /* Try to block ongoing updates */
>> -    if (!amdgpu_vm_eviction_trylock(bo_base->vm))
>> -        return false;
>> +    vm = bo_base->vm;
>> +    scoped_cond_guard(mutex_try, return false, &vm->eviction_lock) {
>> -    /* Don't evict VM page tables while they are updated */
>> -    if (!dma_fence_is_signaled(bo_base->vm->last_unlocked)) {
>> -        amdgpu_vm_eviction_unlock(bo_base->vm);
>> -        return false;
>> -    }
>> +        /* Don't evict VM page tables while they are updated */
>> +        if (!dma_fence_is_signaled(vm->last_unlocked))
>> +            return false;
>> -    bo_base->vm->evicting = true;
>> -    amdgpu_vm_eviction_unlock(bo_base->vm);
>> +        vm->evicting = true;
>> +    }
>>       return true;
>>   }
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_internal.h b/ 
>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm_internal.h
>> index 447195cc45932..ca86eaac75235 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_internal.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_internal.h
>> @@ -137,15 +137,6 @@ static inline void amdgpu_vm_eviction_lock(struct 
>> amdgpu_vm *vm)
>>       vm->saved_flags = memalloc_noreclaim_save();
>>   }
>> -static inline bool amdgpu_vm_eviction_trylock(struct amdgpu_vm *vm)
>> -{
>> -    if (mutex_trylock(&vm->eviction_lock)) {
>> -        vm->saved_flags = memalloc_noreclaim_save();
>> -        return true;
>> -    }
>> -    return false;
>> -}
>> -
>>   static inline void amdgpu_vm_eviction_unlock(struct amdgpu_vm *vm)
>>   {
>>       memalloc_noreclaim_restore(vm->saved_flags);
>