[PATCH 1/2] drm/amdgpu: free pasid early before converting a vm

[PATCH 1/2] drm/amdgpu: free pasid early before converting a vm

[Nirmoy Das]

VM code should not be responsible for freeing pasid as pasid
gets allocated outside of VM code, before initializing a vm.

Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 6 ++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c           | 5 -----
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
index f96598279593..42e22b1fdfee 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -1287,6 +1287,12 @@ int amdgpu_amdkfd_gpuvm_acquire_process_vm(struct kgd_dev *kgd,
 	if (avm->process_info)
 		return -EINVAL;
 
+	/* Free the original amdgpu allocated pasid,
+	 * will be replaced with kfd allocated pasid
+	 */
+	if (avm->pasid)
+		amdgpu_pasid_free(avm->pasid);
+
 	/* Convert VM into a compute VM */
 	ret = amdgpu_vm_make_compute(adev, avm, pasid);
 	if (ret)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 63975bda8e76..be841d62a1d4 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -3094,11 +3094,6 @@ int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 		spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
 		idr_remove(&adev->vm_manager.pasid_idr, vm->pasid);
 		spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
-
-		/* Free the original amdgpu allocated pasid
-		 * Will be replaced with kfd allocated pasid
-		 */
-		amdgpu_pasid_free(vm->pasid);
 		vm->pasid = 0;
 	}
 
-- 
2.32.0

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 2/2] drm/amdgpu: use xarray for storing pasid in vm

[Nirmoy Das]

Replace idr with xarray as we actually need hash functionality.
Cleanup code related to vm pasid by adding helper function.

Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 134 +++++++++++--------------
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h |   3 +-
 2 files changed, 60 insertions(+), 77 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index be841d62a1d4..e047e56a4be2 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -87,6 +87,39 @@ struct amdgpu_prt_cb {
 	struct dma_fence_cb cb;
 };
 
+static int amdgpu_vm_set_pasid(struct amdgpu_device *adev,
+			       struct amdgpu_vm *vm,
+			       unsigned long pasid)
+{
+	unsigned long flags;
+	int r;
+
+	if (pasid) {
+		xa_lock_irqsave(&adev->vm_manager.pasids, flags);
+		r = xa_err(__xa_store(&adev->vm_manager.pasids, pasid, vm,
+				      GFP_ATOMIC));
+		xa_unlock_irqrestore(&adev->vm_manager.pasids, flags);
+		if (r < 0)
+			return r;
+	} else {
+		unsigned long index;
+		struct amdgpu_vm *res;
+
+		xa_lock_irqsave(&adev->vm_manager.pasids, flags);
+		xa_for_each(&adev->vm_manager.pasids, index, res) {
+			if (res == vm) {
+				__xa_erase(&adev->vm_manager.pasids, index);
+				break;
+			}
+		}
+		xa_unlock_irqrestore(&adev->vm_manager.pasids, flags);
+	}
+
+	vm->pasid = pasid;
+
+	return 0;
+}
+
 /*
  * vm eviction_lock can be taken in MMU notifiers. Make sure no reclaim-FS
  * happens while holding this lock anywhere to prevent deadlocks when
@@ -2940,18 +2973,9 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm, u32 pasid)
 
 	amdgpu_bo_unreserve(vm->root.bo);
 
-	if (pasid) {
-		unsigned long flags;
-
-		spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
-		r = idr_alloc(&adev->vm_manager.pasid_idr, vm, pasid, pasid + 1,
-			      GFP_ATOMIC);
-		spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
-		if (r < 0)
-			goto error_free_root;
-
-		vm->pasid = pasid;
-	}
+	r = amdgpu_vm_set_pasid(adev, vm, pasid);
+	if (r)
+		goto error_free_root;
 
 	INIT_KFIFO(vm->faults);
 
@@ -3039,18 +3063,11 @@ int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 	if (r)
 		goto unreserve_bo;
 
-	if (pasid) {
-		unsigned long flags;
-
-		spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
-		r = idr_alloc(&adev->vm_manager.pasid_idr, vm, pasid, pasid + 1,
-			      GFP_ATOMIC);
-		spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
-
-		if (r == -ENOSPC)
-			goto unreserve_bo;
-		r = 0;
-	}
+	/* remove previous {pasid:vm} entry first */
+	r = amdgpu_vm_set_pasid(adev, vm, 0);
+	r = amdgpu_vm_set_pasid(adev, vm, pasid);
+	if (r)
+		goto unreserve_bo;
 
 	/* Check if PD needs to be reinitialized and do it before
 	 * changing any other state, in case it fails.
@@ -3061,7 +3078,7 @@ int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 				       to_amdgpu_bo_vm(vm->root.bo),
 				       false);
 		if (r)
-			goto free_idr;
+			goto free_pasid_entry;
 	}
 
 	/* Update VM state */
@@ -3078,7 +3095,7 @@ int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 		r = amdgpu_bo_sync_wait(vm->root.bo,
 					AMDGPU_FENCE_OWNER_UNDEFINED, true);
 		if (r)
-			goto free_idr;
+			goto free_pasid_entry;
 
 		vm->update_funcs = &amdgpu_vm_cpu_funcs;
 	} else {
@@ -3088,31 +3105,14 @@ int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 	vm->last_update = NULL;
 	vm->is_compute_context = true;
 
-	if (vm->pasid) {
-		unsigned long flags;
-
-		spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
-		idr_remove(&adev->vm_manager.pasid_idr, vm->pasid);
-		spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
-		vm->pasid = 0;
-	}
-
 	/* Free the shadow bo for compute VM */
 	amdgpu_bo_unref(&to_amdgpu_bo_vm(vm->root.bo)->shadow);
 
-	if (pasid)
-		vm->pasid = pasid;
-
 	goto unreserve_bo;
 
-free_idr:
-	if (pasid) {
-		unsigned long flags;
+free_pasid_entry:
+	amdgpu_vm_set_pasid(adev, vm, 0);
 
-		spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
-		idr_remove(&adev->vm_manager.pasid_idr, pasid);
-		spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
-	}
 unreserve_bo:
 	amdgpu_bo_unreserve(vm->root.bo);
 	return r;
@@ -3128,14 +3128,7 @@ int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm,
  */
 void amdgpu_vm_release_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm)
 {
-	if (vm->pasid) {
-		unsigned long flags;
-
-		spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
-		idr_remove(&adev->vm_manager.pasid_idr, vm->pasid);
-		spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
-	}
-	vm->pasid = 0;
+	amdgpu_vm_set_pasid(adev, vm, 0);
 	vm->is_compute_context = false;
 }
 
@@ -3159,15 +3152,7 @@ void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm)
 
 	root = amdgpu_bo_ref(vm->root.bo);
 	amdgpu_bo_reserve(root, true);
-	if (vm->pasid) {
-		unsigned long flags;
-
-		spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
-		idr_remove(&adev->vm_manager.pasid_idr, vm->pasid);
-		spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
-		vm->pasid = 0;
-	}
-
+	amdgpu_vm_set_pasid(adev, vm, 0);
 	dma_fence_wait(vm->last_unlocked, false);
 	dma_fence_put(vm->last_unlocked);
 
@@ -3249,8 +3234,7 @@ void amdgpu_vm_manager_init(struct amdgpu_device *adev)
 	adev->vm_manager.vm_update_mode = 0;
 #endif
 
-	idr_init(&adev->vm_manager.pasid_idr);
-	spin_lock_init(&adev->vm_manager.pasid_lock);
+	xa_init_flags(&adev->vm_manager.pasids, XA_FLAGS_LOCK_IRQ);
 }
 
 /**
@@ -3262,8 +3246,8 @@ void amdgpu_vm_manager_init(struct amdgpu_device *adev)
  */
 void amdgpu_vm_manager_fini(struct amdgpu_device *adev)
 {
-	WARN_ON(!idr_is_empty(&adev->vm_manager.pasid_idr));
-	idr_destroy(&adev->vm_manager.pasid_idr);
+	WARN_ON(!xa_empty(&adev->vm_manager.pasids));
+	xa_destroy(&adev->vm_manager.pasids);
 
 	amdgpu_vmid_mgr_fini(adev);
 }
@@ -3332,13 +3316,13 @@ void amdgpu_vm_get_task_info(struct amdgpu_device *adev, u32 pasid,
 	struct amdgpu_vm *vm;
 	unsigned long flags;
 
-	spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
+	xa_lock_irqsave(&adev->vm_manager.pasids, flags);
 
-	vm = idr_find(&adev->vm_manager.pasid_idr, pasid);
+	vm = xa_load(&adev->vm_manager.pasids, pasid);
 	if (vm)
 		*task_info = vm->task_info;
 
-	spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
+	xa_unlock_irqrestore(&adev->vm_manager.pasids, flags);
 }
 
 /**
@@ -3380,15 +3364,15 @@ bool amdgpu_vm_handle_fault(struct amdgpu_device *adev, u32 pasid,
 	struct amdgpu_vm *vm;
 	int r;
 
-	spin_lock_irqsave(&adev->vm_manager.pasid_lock, irqflags);
-	vm = idr_find(&adev->vm_manager.pasid_idr, pasid);
+	xa_lock_irqsave(&adev->vm_manager.pasids, irqflags);
+	vm = xa_load(&adev->vm_manager.pasids, pasid);
 	if (vm) {
 		root = amdgpu_bo_ref(vm->root.bo);
 		is_compute_context = vm->is_compute_context;
 	} else {
 		root = NULL;
 	}
-	spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, irqflags);
+	xa_unlock_irqrestore(&adev->vm_manager.pasids, irqflags);
 
 	if (!root)
 		return false;
@@ -3406,11 +3390,11 @@ bool amdgpu_vm_handle_fault(struct amdgpu_device *adev, u32 pasid,
 		goto error_unref;
 
 	/* Double check that the VM still exists */
-	spin_lock_irqsave(&adev->vm_manager.pasid_lock, irqflags);
-	vm = idr_find(&adev->vm_manager.pasid_idr, pasid);
+	xa_lock_irqsave(&adev->vm_manager.pasids, irqflags);
+	vm = xa_load(&adev->vm_manager.pasids, pasid);
 	if (vm && vm->root.bo != root)
 		vm = NULL;
-	spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, irqflags);
+	xa_unlock_irqrestore(&adev->vm_manager.pasids, irqflags);
 	if (!vm)
 		goto error_unlock;
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
index ddb85a85cbba..31c467764162 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -359,8 +359,7 @@ struct amdgpu_vm_manager {
 	/* PASID to VM mapping, will be used in interrupt context to
 	 * look up VM of a page fault
 	 */
-	struct idr				pasid_idr;
-	spinlock_t				pasid_lock;
+	struct xarray				pasids;
 };
 
 struct amdgpu_bo_va_mapping;
-- 
2.32.0

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/2] drm/amdgpu: free pasid early before converting a vm

[Christian König]

Am 23.06.21 um 14:25 schrieb Nirmoy Das:
> VM code should not be responsible for freeing pasid as pasid
> gets allocated outside of VM code, before initializing a vm.
>
> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>

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

> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 6 ++++++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c           | 5 -----
>   2 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> index f96598279593..42e22b1fdfee 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> @@ -1287,6 +1287,12 @@ int amdgpu_amdkfd_gpuvm_acquire_process_vm(struct kgd_dev *kgd,
>   	if (avm->process_info)
>   		return -EINVAL;
>   
> +	/* Free the original amdgpu allocated pasid,
> +	 * will be replaced with kfd allocated pasid
> +	 */
> +	if (avm->pasid)
> +		amdgpu_pasid_free(avm->pasid);
> +
>   	/* Convert VM into a compute VM */
>   	ret = amdgpu_vm_make_compute(adev, avm, pasid);
>   	if (ret)
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 63975bda8e76..be841d62a1d4 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -3094,11 +3094,6 @@ int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>   		spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
>   		idr_remove(&adev->vm_manager.pasid_idr, vm->pasid);
>   		spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
> -
> -		/* Free the original amdgpu allocated pasid
> -		 * Will be replaced with kfd allocated pasid
> -		 */
> -		amdgpu_pasid_free(vm->pasid);
>   		vm->pasid = 0;
>   	}
>   

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/2] drm/amdgpu: use xarray for storing pasid in vm

[Christian König]

Am 23.06.21 um 14:25 schrieb Nirmoy Das:
> Replace idr with xarray as we actually need hash functionality.
> Cleanup code related to vm pasid by adding helper function.
>
> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 134 +++++++++++--------------
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h |   3 +-
>   2 files changed, 60 insertions(+), 77 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index be841d62a1d4..e047e56a4be2 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -87,6 +87,39 @@ struct amdgpu_prt_cb {
>   	struct dma_fence_cb cb;
>   };
>   
> +static int amdgpu_vm_set_pasid(struct amdgpu_device *adev,
> +			       struct amdgpu_vm *vm,
> +			       unsigned long pasid)

Some kerneldoc please describing why we have that function.

> +{
> +	unsigned long flags;
> +	int r;
> +
> +	if (pasid) {

You should probably reorder the code so that the old pasid is first 
removed and then eventually the new one added.

> +		xa_lock_irqsave(&adev->vm_manager.pasids, flags);
> +		r = xa_err(__xa_store(&adev->vm_manager.pasids, pasid, vm,
> +				      GFP_ATOMIC));
> +		xa_unlock_irqrestore(&adev->vm_manager.pasids, flags);

As far as I can see this can just use xa_store() which also drops the 
need for GFP_ATOMIC here.

> +		if (r < 0)
> +			return r;
> +	} else {
> +		unsigned long index;
> +		struct amdgpu_vm *res;
> +
> +		xa_lock_irqsave(&adev->vm_manager.pasids, flags);
> +		xa_for_each(&adev->vm_manager.pasids, index, res) {
> +			if (res == vm) {
> +				__xa_erase(&adev->vm_manager.pasids, index);
> +				break;
> +			}
> +		}
> +		xa_unlock_irqrestore(&adev->vm_manager.pasids, flags);

That is really ugly, why is that necessary?

Christian

> +	}
> +
> +	vm->pasid = pasid;
> +
> +	return 0;
> +}
> +
>   /*
>    * vm eviction_lock can be taken in MMU notifiers. Make sure no reclaim-FS
>    * happens while holding this lock anywhere to prevent deadlocks when
> @@ -2940,18 +2973,9 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm, u32 pasid)
>   
>   	amdgpu_bo_unreserve(vm->root.bo);
>   
> -	if (pasid) {
> -		unsigned long flags;
> -
> -		spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
> -		r = idr_alloc(&adev->vm_manager.pasid_idr, vm, pasid, pasid + 1,
> -			      GFP_ATOMIC);
> -		spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
> -		if (r < 0)
> -			goto error_free_root;
> -
> -		vm->pasid = pasid;
> -	}
> +	r = amdgpu_vm_set_pasid(adev, vm, pasid);
> +	if (r)
> +		goto error_free_root;
>   
>   	INIT_KFIFO(vm->faults);
>   
> @@ -3039,18 +3063,11 @@ int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>   	if (r)
>   		goto unreserve_bo;
>   
> -	if (pasid) {
> -		unsigned long flags;
> -
> -		spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
> -		r = idr_alloc(&adev->vm_manager.pasid_idr, vm, pasid, pasid + 1,
> -			      GFP_ATOMIC);
> -		spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
> -
> -		if (r == -ENOSPC)
> -			goto unreserve_bo;
> -		r = 0;
> -	}
> +	/* remove previous {pasid:vm} entry first */
> +	r = amdgpu_vm_set_pasid(adev, vm, 0);
> +	r = amdgpu_vm_set_pasid(adev, vm, pasid);
> +	if (r)
> +		goto unreserve_bo;
>   
>   	/* Check if PD needs to be reinitialized and do it before
>   	 * changing any other state, in case it fails.
> @@ -3061,7 +3078,7 @@ int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>   				       to_amdgpu_bo_vm(vm->root.bo),
>   				       false);
>   		if (r)
> -			goto free_idr;
> +			goto free_pasid_entry;
>   	}
>   
>   	/* Update VM state */
> @@ -3078,7 +3095,7 @@ int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>   		r = amdgpu_bo_sync_wait(vm->root.bo,
>   					AMDGPU_FENCE_OWNER_UNDEFINED, true);
>   		if (r)
> -			goto free_idr;
> +			goto free_pasid_entry;
>   
>   		vm->update_funcs = &amdgpu_vm_cpu_funcs;
>   	} else {
> @@ -3088,31 +3105,14 @@ int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>   	vm->last_update = NULL;
>   	vm->is_compute_context = true;
>   
> -	if (vm->pasid) {
> -		unsigned long flags;
> -
> -		spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
> -		idr_remove(&adev->vm_manager.pasid_idr, vm->pasid);
> -		spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
> -		vm->pasid = 0;
> -	}
> -
>   	/* Free the shadow bo for compute VM */
>   	amdgpu_bo_unref(&to_amdgpu_bo_vm(vm->root.bo)->shadow);
>   
> -	if (pasid)
> -		vm->pasid = pasid;
> -
>   	goto unreserve_bo;
>   
> -free_idr:
> -	if (pasid) {
> -		unsigned long flags;
> +free_pasid_entry:
> +	amdgpu_vm_set_pasid(adev, vm, 0);
>   
> -		spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
> -		idr_remove(&adev->vm_manager.pasid_idr, pasid);
> -		spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
> -	}
>   unreserve_bo:
>   	amdgpu_bo_unreserve(vm->root.bo);
>   	return r;
> @@ -3128,14 +3128,7 @@ int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>    */
>   void amdgpu_vm_release_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm)
>   {
> -	if (vm->pasid) {
> -		unsigned long flags;
> -
> -		spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
> -		idr_remove(&adev->vm_manager.pasid_idr, vm->pasid);
> -		spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
> -	}
> -	vm->pasid = 0;
> +	amdgpu_vm_set_pasid(adev, vm, 0);
>   	vm->is_compute_context = false;
>   }
>   
> @@ -3159,15 +3152,7 @@ void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm)
>   
>   	root = amdgpu_bo_ref(vm->root.bo);
>   	amdgpu_bo_reserve(root, true);
> -	if (vm->pasid) {
> -		unsigned long flags;
> -
> -		spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
> -		idr_remove(&adev->vm_manager.pasid_idr, vm->pasid);
> -		spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
> -		vm->pasid = 0;
> -	}
> -
> +	amdgpu_vm_set_pasid(adev, vm, 0);
>   	dma_fence_wait(vm->last_unlocked, false);
>   	dma_fence_put(vm->last_unlocked);
>   
> @@ -3249,8 +3234,7 @@ void amdgpu_vm_manager_init(struct amdgpu_device *adev)
>   	adev->vm_manager.vm_update_mode = 0;
>   #endif
>   
> -	idr_init(&adev->vm_manager.pasid_idr);
> -	spin_lock_init(&adev->vm_manager.pasid_lock);
> +	xa_init_flags(&adev->vm_manager.pasids, XA_FLAGS_LOCK_IRQ);
>   }
>   
>   /**
> @@ -3262,8 +3246,8 @@ void amdgpu_vm_manager_init(struct amdgpu_device *adev)
>    */
>   void amdgpu_vm_manager_fini(struct amdgpu_device *adev)
>   {
> -	WARN_ON(!idr_is_empty(&adev->vm_manager.pasid_idr));
> -	idr_destroy(&adev->vm_manager.pasid_idr);
> +	WARN_ON(!xa_empty(&adev->vm_manager.pasids));
> +	xa_destroy(&adev->vm_manager.pasids);
>   
>   	amdgpu_vmid_mgr_fini(adev);
>   }
> @@ -3332,13 +3316,13 @@ void amdgpu_vm_get_task_info(struct amdgpu_device *adev, u32 pasid,
>   	struct amdgpu_vm *vm;
>   	unsigned long flags;
>   
> -	spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
> +	xa_lock_irqsave(&adev->vm_manager.pasids, flags);
>   
> -	vm = idr_find(&adev->vm_manager.pasid_idr, pasid);
> +	vm = xa_load(&adev->vm_manager.pasids, pasid);
>   	if (vm)
>   		*task_info = vm->task_info;
>   
> -	spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
> +	xa_unlock_irqrestore(&adev->vm_manager.pasids, flags);
>   }
>   
>   /**
> @@ -3380,15 +3364,15 @@ bool amdgpu_vm_handle_fault(struct amdgpu_device *adev, u32 pasid,
>   	struct amdgpu_vm *vm;
>   	int r;
>   
> -	spin_lock_irqsave(&adev->vm_manager.pasid_lock, irqflags);
> -	vm = idr_find(&adev->vm_manager.pasid_idr, pasid);
> +	xa_lock_irqsave(&adev->vm_manager.pasids, irqflags);
> +	vm = xa_load(&adev->vm_manager.pasids, pasid);
>   	if (vm) {
>   		root = amdgpu_bo_ref(vm->root.bo);
>   		is_compute_context = vm->is_compute_context;
>   	} else {
>   		root = NULL;
>   	}
> -	spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, irqflags);
> +	xa_unlock_irqrestore(&adev->vm_manager.pasids, irqflags);
>   
>   	if (!root)
>   		return false;
> @@ -3406,11 +3390,11 @@ bool amdgpu_vm_handle_fault(struct amdgpu_device *adev, u32 pasid,
>   		goto error_unref;
>   
>   	/* Double check that the VM still exists */
> -	spin_lock_irqsave(&adev->vm_manager.pasid_lock, irqflags);
> -	vm = idr_find(&adev->vm_manager.pasid_idr, pasid);
> +	xa_lock_irqsave(&adev->vm_manager.pasids, irqflags);
> +	vm = xa_load(&adev->vm_manager.pasids, pasid);
>   	if (vm && vm->root.bo != root)
>   		vm = NULL;
> -	spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, irqflags);
> +	xa_unlock_irqrestore(&adev->vm_manager.pasids, irqflags);
>   	if (!vm)
>   		goto error_unlock;
>   
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> index ddb85a85cbba..31c467764162 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> @@ -359,8 +359,7 @@ struct amdgpu_vm_manager {
>   	/* PASID to VM mapping, will be used in interrupt context to
>   	 * look up VM of a page fault
>   	 */
> -	struct idr				pasid_idr;
> -	spinlock_t				pasid_lock;
> +	struct xarray				pasids;
>   };
>   
>   struct amdgpu_bo_va_mapping;

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/2] drm/amdgpu: use xarray for storing pasid in vm

[Das, Nirmoy]

On 6/23/2021 2:50 PM, Christian König wrote:
>
>
> Am 23.06.21 um 14:25 schrieb Nirmoy Das:
>> Replace idr with xarray as we actually need hash functionality.
>> Cleanup code related to vm pasid by adding helper function.
>>
>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 134 +++++++++++--------------
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h |   3 +-
>>   2 files changed, 60 insertions(+), 77 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index be841d62a1d4..e047e56a4be2 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -87,6 +87,39 @@ struct amdgpu_prt_cb {
>>       struct dma_fence_cb cb;
>>   };
>>   +static int amdgpu_vm_set_pasid(struct amdgpu_device *adev,
>> +                   struct amdgpu_vm *vm,
>> +                   unsigned long pasid)
>
> Some kerneldoc please describing why we have that function.


Alright.


>
>> +{
>> +    unsigned long flags;
>> +    int r;
>> +
>> +    if (pasid) {
>
> You should probably reorder the code so that the old pasid is first 
> removed and then eventually the new one added.
>
>> + xa_lock_irqsave(&adev->vm_manager.pasids, flags);
>> +        r = xa_err(__xa_store(&adev->vm_manager.pasids, pasid, vm,
>> +                      GFP_ATOMIC));
>> +        xa_unlock_irqrestore(&adev->vm_manager.pasids, flags);
>
> As far as I can see this can just use xa_store() which also drops the 
> need for GFP_ATOMIC here.


Do we need to have this irqsave/restore to keep passids safe for 
amdgpu_vm_handle_fault() ?

xa_store() takes the spinlock without irq flags so I wanted to keep old 
behavior.


>
>> +        if (r < 0)
>> +            return r;
>> +    } else {
>> +        unsigned long index;
>> +        struct amdgpu_vm *res;
>> +
>> +        xa_lock_irqsave(&adev->vm_manager.pasids, flags);
>> +        xa_for_each(&adev->vm_manager.pasids, index, res) {
>> +            if (res == vm) {
>> +                __xa_erase(&adev->vm_manager.pasids, index);
>> +                break;
>> +            }
>> +        }
>> +        xa_unlock_irqrestore(&adev->vm_manager.pasids, flags);
>
> That is really ugly, why is that necessary?

Do you mean the lock around xa_for_each() ? I think I can just used lock 
around __xa_erase() or just xa_erase() if just simple spinlock without 
flags is enough.


Regards,

Nirmoy


>
> Christian
>
>> +    }
>> +
>> +    vm->pasid = pasid;
>> +
>> +    return 0;
>> +}
>> +
>>   /*
>>    * vm eviction_lock can be taken in MMU notifiers. Make sure no 
>> reclaim-FS
>>    * happens while holding this lock anywhere to prevent deadlocks when
>> @@ -2940,18 +2973,9 @@ int amdgpu_vm_init(struct amdgpu_device *adev, 
>> struct amdgpu_vm *vm, u32 pasid)
>>         amdgpu_bo_unreserve(vm->root.bo);
>>   -    if (pasid) {
>> -        unsigned long flags;
>> -
>> -        spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
>> -        r = idr_alloc(&adev->vm_manager.pasid_idr, vm, pasid, pasid 
>> + 1,
>> -                  GFP_ATOMIC);
>> - spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
>> -        if (r < 0)
>> -            goto error_free_root;
>> -
>> -        vm->pasid = pasid;
>> -    }
>> +    r = amdgpu_vm_set_pasid(adev, vm, pasid);
>> +    if (r)
>> +        goto error_free_root;
>>         INIT_KFIFO(vm->faults);
>>   @@ -3039,18 +3063,11 @@ int amdgpu_vm_make_compute(struct 
>> amdgpu_device *adev, struct amdgpu_vm *vm,
>>       if (r)
>>           goto unreserve_bo;
>>   -    if (pasid) {
>> -        unsigned long flags;
>> -
>> -        spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
>> -        r = idr_alloc(&adev->vm_manager.pasid_idr, vm, pasid, pasid 
>> + 1,
>> -                  GFP_ATOMIC);
>> - spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
>> -
>> -        if (r == -ENOSPC)
>> -            goto unreserve_bo;
>> -        r = 0;
>> -    }
>> +    /* remove previous {pasid:vm} entry first */
>> +    r = amdgpu_vm_set_pasid(adev, vm, 0);
>> +    r = amdgpu_vm_set_pasid(adev, vm, pasid);
>> +    if (r)
>> +        goto unreserve_bo;
>>         /* Check if PD needs to be reinitialized and do it before
>>        * changing any other state, in case it fails.
>> @@ -3061,7 +3078,7 @@ int amdgpu_vm_make_compute(struct amdgpu_device 
>> *adev, struct amdgpu_vm *vm,
>>                          to_amdgpu_bo_vm(vm->root.bo),
>>                          false);
>>           if (r)
>> -            goto free_idr;
>> +            goto free_pasid_entry;
>>       }
>>         /* Update VM state */
>> @@ -3078,7 +3095,7 @@ int amdgpu_vm_make_compute(struct amdgpu_device 
>> *adev, struct amdgpu_vm *vm,
>>           r = amdgpu_bo_sync_wait(vm->root.bo,
>>                       AMDGPU_FENCE_OWNER_UNDEFINED, true);
>>           if (r)
>> -            goto free_idr;
>> +            goto free_pasid_entry;
>>             vm->update_funcs = &amdgpu_vm_cpu_funcs;
>>       } else {
>> @@ -3088,31 +3105,14 @@ int amdgpu_vm_make_compute(struct 
>> amdgpu_device *adev, struct amdgpu_vm *vm,
>>       vm->last_update = NULL;
>>       vm->is_compute_context = true;
>>   -    if (vm->pasid) {
>> -        unsigned long flags;
>> -
>> -        spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
>> -        idr_remove(&adev->vm_manager.pasid_idr, vm->pasid);
>> - spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
>> -        vm->pasid = 0;
>> -    }
>> -
>>       /* Free the shadow bo for compute VM */
>> amdgpu_bo_unref(&to_amdgpu_bo_vm(vm->root.bo)->shadow);
>>   -    if (pasid)
>> -        vm->pasid = pasid;
>> -
>>       goto unreserve_bo;
>>   -free_idr:
>> -    if (pasid) {
>> -        unsigned long flags;
>> +free_pasid_entry:
>> +    amdgpu_vm_set_pasid(adev, vm, 0);
>>   - spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
>> -        idr_remove(&adev->vm_manager.pasid_idr, pasid);
>> - spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
>> -    }
>>   unreserve_bo:
>>       amdgpu_bo_unreserve(vm->root.bo);
>>       return r;
>> @@ -3128,14 +3128,7 @@ int amdgpu_vm_make_compute(struct 
>> amdgpu_device *adev, struct amdgpu_vm *vm,
>>    */
>>   void amdgpu_vm_release_compute(struct amdgpu_device *adev, struct 
>> amdgpu_vm *vm)
>>   {
>> -    if (vm->pasid) {
>> -        unsigned long flags;
>> -
>> -        spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
>> -        idr_remove(&adev->vm_manager.pasid_idr, vm->pasid);
>> - spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
>> -    }
>> -    vm->pasid = 0;
>> +    amdgpu_vm_set_pasid(adev, vm, 0);
>>       vm->is_compute_context = false;
>>   }
>>   @@ -3159,15 +3152,7 @@ void amdgpu_vm_fini(struct amdgpu_device 
>> *adev, struct amdgpu_vm *vm)
>>         root = amdgpu_bo_ref(vm->root.bo);
>>       amdgpu_bo_reserve(root, true);
>> -    if (vm->pasid) {
>> -        unsigned long flags;
>> -
>> -        spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
>> -        idr_remove(&adev->vm_manager.pasid_idr, vm->pasid);
>> - spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
>> -        vm->pasid = 0;
>> -    }
>> -
>> +    amdgpu_vm_set_pasid(adev, vm, 0);
>>       dma_fence_wait(vm->last_unlocked, false);
>>       dma_fence_put(vm->last_unlocked);
>>   @@ -3249,8 +3234,7 @@ void amdgpu_vm_manager_init(struct 
>> amdgpu_device *adev)
>>       adev->vm_manager.vm_update_mode = 0;
>>   #endif
>>   -    idr_init(&adev->vm_manager.pasid_idr);
>> -    spin_lock_init(&adev->vm_manager.pasid_lock);
>> +    xa_init_flags(&adev->vm_manager.pasids, XA_FLAGS_LOCK_IRQ);
>>   }
>>     /**
>> @@ -3262,8 +3246,8 @@ void amdgpu_vm_manager_init(struct 
>> amdgpu_device *adev)
>>    */
>>   void amdgpu_vm_manager_fini(struct amdgpu_device *adev)
>>   {
>> -    WARN_ON(!idr_is_empty(&adev->vm_manager.pasid_idr));
>> -    idr_destroy(&adev->vm_manager.pasid_idr);
>> +    WARN_ON(!xa_empty(&adev->vm_manager.pasids));
>> +    xa_destroy(&adev->vm_manager.pasids);
>>         amdgpu_vmid_mgr_fini(adev);
>>   }
>> @@ -3332,13 +3316,13 @@ void amdgpu_vm_get_task_info(struct 
>> amdgpu_device *adev, u32 pasid,
>>       struct amdgpu_vm *vm;
>>       unsigned long flags;
>>   -    spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
>> +    xa_lock_irqsave(&adev->vm_manager.pasids, flags);
>>   -    vm = idr_find(&adev->vm_manager.pasid_idr, pasid);
>> +    vm = xa_load(&adev->vm_manager.pasids, pasid);
>>       if (vm)
>>           *task_info = vm->task_info;
>>   - spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
>> +    xa_unlock_irqrestore(&adev->vm_manager.pasids, flags);
>>   }
>>     /**
>> @@ -3380,15 +3364,15 @@ bool amdgpu_vm_handle_fault(struct 
>> amdgpu_device *adev, u32 pasid,
>>       struct amdgpu_vm *vm;
>>       int r;
>>   -    spin_lock_irqsave(&adev->vm_manager.pasid_lock, irqflags);
>> -    vm = idr_find(&adev->vm_manager.pasid_idr, pasid);
>> +    xa_lock_irqsave(&adev->vm_manager.pasids, irqflags);
>> +    vm = xa_load(&adev->vm_manager.pasids, pasid);
>>       if (vm) {
>>           root = amdgpu_bo_ref(vm->root.bo);
>>           is_compute_context = vm->is_compute_context;
>>       } else {
>>           root = NULL;
>>       }
>> -    spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, irqflags);
>> +    xa_unlock_irqrestore(&adev->vm_manager.pasids, irqflags);
>>         if (!root)
>>           return false;
>> @@ -3406,11 +3390,11 @@ bool amdgpu_vm_handle_fault(struct 
>> amdgpu_device *adev, u32 pasid,
>>           goto error_unref;
>>         /* Double check that the VM still exists */
>> -    spin_lock_irqsave(&adev->vm_manager.pasid_lock, irqflags);
>> -    vm = idr_find(&adev->vm_manager.pasid_idr, pasid);
>> +    xa_lock_irqsave(&adev->vm_manager.pasids, irqflags);
>> +    vm = xa_load(&adev->vm_manager.pasids, pasid);
>>       if (vm && vm->root.bo != root)
>>           vm = NULL;
>> -    spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, irqflags);
>> +    xa_unlock_irqrestore(&adev->vm_manager.pasids, irqflags);
>>       if (!vm)
>>           goto error_unlock;
>>   diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> index ddb85a85cbba..31c467764162 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> @@ -359,8 +359,7 @@ struct amdgpu_vm_manager {
>>       /* PASID to VM mapping, will be used in interrupt context to
>>        * look up VM of a page fault
>>        */
>> -    struct idr                pasid_idr;
>> -    spinlock_t                pasid_lock;
>> +    struct xarray                pasids;
>>   };
>>     struct amdgpu_bo_va_mapping;
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/2] drm/amdgpu: use xarray for storing pasid in vm

[Christian König]

Am 23.06.21 um 15:11 schrieb Das, Nirmoy:
>
> On 6/23/2021 2:50 PM, Christian König wrote:
>>
>>
>> Am 23.06.21 um 14:25 schrieb Nirmoy Das:
>>> Replace idr with xarray as we actually need hash functionality.
>>> Cleanup code related to vm pasid by adding helper function.
>>>
>>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 134 
>>> +++++++++++--------------
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h |   3 +-
>>>   2 files changed, 60 insertions(+), 77 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> index be841d62a1d4..e047e56a4be2 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> @@ -87,6 +87,39 @@ struct amdgpu_prt_cb {
>>>       struct dma_fence_cb cb;
>>>   };
>>>   +static int amdgpu_vm_set_pasid(struct amdgpu_device *adev,
>>> +                   struct amdgpu_vm *vm,
>>> +                   unsigned long pasid)
>>
>> Some kerneldoc please describing why we have that function.
>
>
> Alright.
>
>
>>
>>> +{
>>> +    unsigned long flags;
>>> +    int r;
>>> +
>>> +    if (pasid) {
>>
>> You should probably reorder the code so that the old pasid is first 
>> removed and then eventually the new one added.
>>
>>> + xa_lock_irqsave(&adev->vm_manager.pasids, flags);
>>> +        r = xa_err(__xa_store(&adev->vm_manager.pasids, pasid, vm,
>>> +                      GFP_ATOMIC));
>>> +        xa_unlock_irqrestore(&adev->vm_manager.pasids, flags);
>>
>> As far as I can see this can just use xa_store() which also drops the 
>> need for GFP_ATOMIC here.
>
>
> Do we need to have this irqsave/restore to keep passids safe for 
> amdgpu_vm_handle_fault() ?

No, we need the VM safe not the pasid.

> xa_store() takes the spinlock without irq flags so I wanted to keep 
> old behavior.

Yeah, that's indeed problematic. You need to keep that straight or 
lockdep will complain.

IIRC there is also a function to reserve an entry before you take the 
lock. Maybe use that one.

>
>
>>
>>> +        if (r < 0)
>>> +            return r;
>>> +    } else {
>>> +        unsigned long index;
>>> +        struct amdgpu_vm *res;
>>> +
>>> +        xa_lock_irqsave(&adev->vm_manager.pasids, flags);
>>> +        xa_for_each(&adev->vm_manager.pasids, index, res) {
>>> +            if (res == vm) {
>>> +                __xa_erase(&adev->vm_manager.pasids, index);
>>> +                break;
>>> +            }
>>> +        }
>>> +        xa_unlock_irqrestore(&adev->vm_manager.pasids, flags);
>>
>> That is really ugly, why is that necessary?
>
> Do you mean the lock around xa_for_each() ? I think I can just used 
> lock around __xa_erase() or just xa_erase() if just simple spinlock 
> without flags is enough.

I mean why you use xa_for_each() here?

Just __xa_erase should be enough.

Christian.

>
>
> Regards,
>
> Nirmoy
>
>
>>
>> Christian
>>
>>> +    }
>>> +
>>> +    vm->pasid = pasid;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>>   /*
>>>    * vm eviction_lock can be taken in MMU notifiers. Make sure no 
>>> reclaim-FS
>>>    * happens while holding this lock anywhere to prevent deadlocks when
>>> @@ -2940,18 +2973,9 @@ int amdgpu_vm_init(struct amdgpu_device 
>>> *adev, struct amdgpu_vm *vm, u32 pasid)
>>>         amdgpu_bo_unreserve(vm->root.bo);
>>>   -    if (pasid) {
>>> -        unsigned long flags;
>>> -
>>> - spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
>>> -        r = idr_alloc(&adev->vm_manager.pasid_idr, vm, pasid, pasid 
>>> + 1,
>>> -                  GFP_ATOMIC);
>>> - spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
>>> -        if (r < 0)
>>> -            goto error_free_root;
>>> -
>>> -        vm->pasid = pasid;
>>> -    }
>>> +    r = amdgpu_vm_set_pasid(adev, vm, pasid);
>>> +    if (r)
>>> +        goto error_free_root;
>>>         INIT_KFIFO(vm->faults);
>>>   @@ -3039,18 +3063,11 @@ int amdgpu_vm_make_compute(struct 
>>> amdgpu_device *adev, struct amdgpu_vm *vm,
>>>       if (r)
>>>           goto unreserve_bo;
>>>   -    if (pasid) {
>>> -        unsigned long flags;
>>> -
>>> - spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
>>> -        r = idr_alloc(&adev->vm_manager.pasid_idr, vm, pasid, pasid 
>>> + 1,
>>> -                  GFP_ATOMIC);
>>> - spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
>>> -
>>> -        if (r == -ENOSPC)
>>> -            goto unreserve_bo;
>>> -        r = 0;
>>> -    }
>>> +    /* remove previous {pasid:vm} entry first */
>>> +    r = amdgpu_vm_set_pasid(adev, vm, 0);
>>> +    r = amdgpu_vm_set_pasid(adev, vm, pasid);
>>> +    if (r)
>>> +        goto unreserve_bo;
>>>         /* Check if PD needs to be reinitialized and do it before
>>>        * changing any other state, in case it fails.
>>> @@ -3061,7 +3078,7 @@ int amdgpu_vm_make_compute(struct 
>>> amdgpu_device *adev, struct amdgpu_vm *vm,
>>>                          to_amdgpu_bo_vm(vm->root.bo),
>>>                          false);
>>>           if (r)
>>> -            goto free_idr;
>>> +            goto free_pasid_entry;
>>>       }
>>>         /* Update VM state */
>>> @@ -3078,7 +3095,7 @@ int amdgpu_vm_make_compute(struct 
>>> amdgpu_device *adev, struct amdgpu_vm *vm,
>>>           r = amdgpu_bo_sync_wait(vm->root.bo,
>>>                       AMDGPU_FENCE_OWNER_UNDEFINED, true);
>>>           if (r)
>>> -            goto free_idr;
>>> +            goto free_pasid_entry;
>>>             vm->update_funcs = &amdgpu_vm_cpu_funcs;
>>>       } else {
>>> @@ -3088,31 +3105,14 @@ int amdgpu_vm_make_compute(struct 
>>> amdgpu_device *adev, struct amdgpu_vm *vm,
>>>       vm->last_update = NULL;
>>>       vm->is_compute_context = true;
>>>   -    if (vm->pasid) {
>>> -        unsigned long flags;
>>> -
>>> - spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
>>> -        idr_remove(&adev->vm_manager.pasid_idr, vm->pasid);
>>> - spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
>>> -        vm->pasid = 0;
>>> -    }
>>> -
>>>       /* Free the shadow bo for compute VM */
>>> amdgpu_bo_unref(&to_amdgpu_bo_vm(vm->root.bo)->shadow);
>>>   -    if (pasid)
>>> -        vm->pasid = pasid;
>>> -
>>>       goto unreserve_bo;
>>>   -free_idr:
>>> -    if (pasid) {
>>> -        unsigned long flags;
>>> +free_pasid_entry:
>>> +    amdgpu_vm_set_pasid(adev, vm, 0);
>>>   - spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
>>> -        idr_remove(&adev->vm_manager.pasid_idr, pasid);
>>> - spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
>>> -    }
>>>   unreserve_bo:
>>>       amdgpu_bo_unreserve(vm->root.bo);
>>>       return r;
>>> @@ -3128,14 +3128,7 @@ int amdgpu_vm_make_compute(struct 
>>> amdgpu_device *adev, struct amdgpu_vm *vm,
>>>    */
>>>   void amdgpu_vm_release_compute(struct amdgpu_device *adev, struct 
>>> amdgpu_vm *vm)
>>>   {
>>> -    if (vm->pasid) {
>>> -        unsigned long flags;
>>> -
>>> - spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
>>> -        idr_remove(&adev->vm_manager.pasid_idr, vm->pasid);
>>> - spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
>>> -    }
>>> -    vm->pasid = 0;
>>> +    amdgpu_vm_set_pasid(adev, vm, 0);
>>>       vm->is_compute_context = false;
>>>   }
>>>   @@ -3159,15 +3152,7 @@ void amdgpu_vm_fini(struct amdgpu_device 
>>> *adev, struct amdgpu_vm *vm)
>>>         root = amdgpu_bo_ref(vm->root.bo);
>>>       amdgpu_bo_reserve(root, true);
>>> -    if (vm->pasid) {
>>> -        unsigned long flags;
>>> -
>>> - spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
>>> -        idr_remove(&adev->vm_manager.pasid_idr, vm->pasid);
>>> - spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
>>> -        vm->pasid = 0;
>>> -    }
>>> -
>>> +    amdgpu_vm_set_pasid(adev, vm, 0);
>>>       dma_fence_wait(vm->last_unlocked, false);
>>>       dma_fence_put(vm->last_unlocked);
>>>   @@ -3249,8 +3234,7 @@ void amdgpu_vm_manager_init(struct 
>>> amdgpu_device *adev)
>>>       adev->vm_manager.vm_update_mode = 0;
>>>   #endif
>>>   -    idr_init(&adev->vm_manager.pasid_idr);
>>> -    spin_lock_init(&adev->vm_manager.pasid_lock);
>>> +    xa_init_flags(&adev->vm_manager.pasids, XA_FLAGS_LOCK_IRQ);
>>>   }
>>>     /**
>>> @@ -3262,8 +3246,8 @@ void amdgpu_vm_manager_init(struct 
>>> amdgpu_device *adev)
>>>    */
>>>   void amdgpu_vm_manager_fini(struct amdgpu_device *adev)
>>>   {
>>> - WARN_ON(!idr_is_empty(&adev->vm_manager.pasid_idr));
>>> -    idr_destroy(&adev->vm_manager.pasid_idr);
>>> +    WARN_ON(!xa_empty(&adev->vm_manager.pasids));
>>> +    xa_destroy(&adev->vm_manager.pasids);
>>>         amdgpu_vmid_mgr_fini(adev);
>>>   }
>>> @@ -3332,13 +3316,13 @@ void amdgpu_vm_get_task_info(struct 
>>> amdgpu_device *adev, u32 pasid,
>>>       struct amdgpu_vm *vm;
>>>       unsigned long flags;
>>>   -    spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
>>> +    xa_lock_irqsave(&adev->vm_manager.pasids, flags);
>>>   -    vm = idr_find(&adev->vm_manager.pasid_idr, pasid);
>>> +    vm = xa_load(&adev->vm_manager.pasids, pasid);
>>>       if (vm)
>>>           *task_info = vm->task_info;
>>>   - spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
>>> +    xa_unlock_irqrestore(&adev->vm_manager.pasids, flags);
>>>   }
>>>     /**
>>> @@ -3380,15 +3364,15 @@ bool amdgpu_vm_handle_fault(struct 
>>> amdgpu_device *adev, u32 pasid,
>>>       struct amdgpu_vm *vm;
>>>       int r;
>>>   -    spin_lock_irqsave(&adev->vm_manager.pasid_lock, irqflags);
>>> -    vm = idr_find(&adev->vm_manager.pasid_idr, pasid);
>>> +    xa_lock_irqsave(&adev->vm_manager.pasids, irqflags);
>>> +    vm = xa_load(&adev->vm_manager.pasids, pasid);
>>>       if (vm) {
>>>           root = amdgpu_bo_ref(vm->root.bo);
>>>           is_compute_context = vm->is_compute_context;
>>>       } else {
>>>           root = NULL;
>>>       }
>>> - spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, irqflags);
>>> +    xa_unlock_irqrestore(&adev->vm_manager.pasids, irqflags);
>>>         if (!root)
>>>           return false;
>>> @@ -3406,11 +3390,11 @@ bool amdgpu_vm_handle_fault(struct 
>>> amdgpu_device *adev, u32 pasid,
>>>           goto error_unref;
>>>         /* Double check that the VM still exists */
>>> -    spin_lock_irqsave(&adev->vm_manager.pasid_lock, irqflags);
>>> -    vm = idr_find(&adev->vm_manager.pasid_idr, pasid);
>>> +    xa_lock_irqsave(&adev->vm_manager.pasids, irqflags);
>>> +    vm = xa_load(&adev->vm_manager.pasids, pasid);
>>>       if (vm && vm->root.bo != root)
>>>           vm = NULL;
>>> - spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, irqflags);
>>> +    xa_unlock_irqrestore(&adev->vm_manager.pasids, irqflags);
>>>       if (!vm)
>>>           goto error_unlock;
>>>   diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>> index ddb85a85cbba..31c467764162 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>> @@ -359,8 +359,7 @@ struct amdgpu_vm_manager {
>>>       /* PASID to VM mapping, will be used in interrupt context to
>>>        * look up VM of a page fault
>>>        */
>>> -    struct idr                pasid_idr;
>>> -    spinlock_t                pasid_lock;
>>> +    struct xarray                pasids;
>>>   };
>>>     struct amdgpu_bo_va_mapping;
>>

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/2] drm/amdgpu: use xarray for storing pasid in vm

[Das, Nirmoy]

On 6/23/2021 3:40 PM, Christian König wrote:
>
>
> Am 23.06.21 um 15:11 schrieb Das, Nirmoy:
>>
>> On 6/23/2021 2:50 PM, Christian König wrote:
>>>
>>>
>>> Am 23.06.21 um 14:25 schrieb Nirmoy Das:
>>>> Replace idr with xarray as we actually need hash functionality.
>>>> Cleanup code related to vm pasid by adding helper function.
>>>>
>>>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
>>>> ---
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 134 
>>>> +++++++++++--------------
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h |   3 +-
>>>>   2 files changed, 60 insertions(+), 77 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> index be841d62a1d4..e047e56a4be2 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> @@ -87,6 +87,39 @@ struct amdgpu_prt_cb {
>>>>       struct dma_fence_cb cb;
>>>>   };
>>>>   +static int amdgpu_vm_set_pasid(struct amdgpu_device *adev,
>>>> +                   struct amdgpu_vm *vm,
>>>> +                   unsigned long pasid)
>>>
>>> Some kerneldoc please describing why we have that function.
>>
>>
>> Alright.
>>
>>
>>>
>>>> +{
>>>> +    unsigned long flags;
>>>> +    int r;
>>>> +
>>>> +    if (pasid) {
>>>
>>> You should probably reorder the code so that the old pasid is first 
>>> removed and then eventually the new one added.
>>>
>>>> + xa_lock_irqsave(&adev->vm_manager.pasids, flags);
>>>> +        r = xa_err(__xa_store(&adev->vm_manager.pasids, pasid, vm,
>>>> +                      GFP_ATOMIC));
>>>> + xa_unlock_irqrestore(&adev->vm_manager.pasids, flags);
>>>
>>> As far as I can see this can just use xa_store() which also drops 
>>> the need for GFP_ATOMIC here.
>>
>>
>> Do we need to have this irqsave/restore to keep passids safe for 
>> amdgpu_vm_handle_fault() ?
>
> No, we need the VM safe not the pasid.


Would spin_lock_irq be enough to keep the VM safe then I can use 
xa_store_irq() and remove some extra locking code?


>
>> xa_store() takes the spinlock without irq flags so I wanted to keep 
>> old behavior.
>
> Yeah, that's indeed problematic. You need to keep that straight or 
> lockdep will complain.
>
> IIRC there is also a function to reserve an entry before you take the 
> lock. Maybe use that one.


xa_reserve() also takes a spin lock so I think this won't work either 
with gfp_kernel flag.


>
>>
>>
>>>
>>>> +        if (r < 0)
>>>> +            return r;
>>>> +    } else {
>>>> +        unsigned long index;
>>>> +        struct amdgpu_vm *res;
>>>> +
>>>> +        xa_lock_irqsave(&adev->vm_manager.pasids, flags);
>>>> +        xa_for_each(&adev->vm_manager.pasids, index, res) {
>>>> +            if (res == vm) {
>>>> +                __xa_erase(&adev->vm_manager.pasids, index);
>>>> +                break;
>>>> +            }
>>>> +        }
>>>> + xa_unlock_irqrestore(&adev->vm_manager.pasids, flags);
>>>
>>> That is really ugly, why is that necessary?
>>
>> Do you mean the lock around xa_for_each() ? I think I can just used 
>> lock around __xa_erase() or just xa_erase() if just simple spinlock 
>> without flags is enough.
>
> I mean why you use xa_for_each() here?


amdgpu_vm_set_pasid() removes pasid:vm entry when pasid 0 is passed. I 
need xa_for_each()  to find the index of that vm pointer

so that I can pass that to __xa_erase(). I couldn't find an API which 
removes an entry  by value.


Regards,

Nirmoy

>
> Just __xa_erase should be enough.
>
> Christian.
>
>>
>>
>> Regards,
>>
>> Nirmoy
>>
>>
>>>
>>> Christian
>>>
>>>> +    }
>>>> +
>>>> +    vm->pasid = pasid;
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>>   /*
>>>>    * vm eviction_lock can be taken in MMU notifiers. Make sure no 
>>>> reclaim-FS
>>>>    * happens while holding this lock anywhere to prevent deadlocks 
>>>> when
>>>> @@ -2940,18 +2973,9 @@ int amdgpu_vm_init(struct amdgpu_device 
>>>> *adev, struct amdgpu_vm *vm, u32 pasid)
>>>>         amdgpu_bo_unreserve(vm->root.bo);
>>>>   -    if (pasid) {
>>>> -        unsigned long flags;
>>>> -
>>>> - spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
>>>> -        r = idr_alloc(&adev->vm_manager.pasid_idr, vm, pasid, 
>>>> pasid + 1,
>>>> -                  GFP_ATOMIC);
>>>> - spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
>>>> -        if (r < 0)
>>>> -            goto error_free_root;
>>>> -
>>>> -        vm->pasid = pasid;
>>>> -    }
>>>> +    r = amdgpu_vm_set_pasid(adev, vm, pasid);
>>>> +    if (r)
>>>> +        goto error_free_root;
>>>>         INIT_KFIFO(vm->faults);
>>>>   @@ -3039,18 +3063,11 @@ int amdgpu_vm_make_compute(struct 
>>>> amdgpu_device *adev, struct amdgpu_vm *vm,
>>>>       if (r)
>>>>           goto unreserve_bo;
>>>>   -    if (pasid) {
>>>> -        unsigned long flags;
>>>> -
>>>> - spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
>>>> -        r = idr_alloc(&adev->vm_manager.pasid_idr, vm, pasid, 
>>>> pasid + 1,
>>>> -                  GFP_ATOMIC);
>>>> - spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
>>>> -
>>>> -        if (r == -ENOSPC)
>>>> -            goto unreserve_bo;
>>>> -        r = 0;
>>>> -    }
>>>> +    /* remove previous {pasid:vm} entry first */
>>>> +    r = amdgpu_vm_set_pasid(adev, vm, 0);
>>>> +    r = amdgpu_vm_set_pasid(adev, vm, pasid);
>>>> +    if (r)
>>>> +        goto unreserve_bo;
>>>>         /* Check if PD needs to be reinitialized and do it before
>>>>        * changing any other state, in case it fails.
>>>> @@ -3061,7 +3078,7 @@ int amdgpu_vm_make_compute(struct 
>>>> amdgpu_device *adev, struct amdgpu_vm *vm,
>>>>                          to_amdgpu_bo_vm(vm->root.bo),
>>>>                          false);
>>>>           if (r)
>>>> -            goto free_idr;
>>>> +            goto free_pasid_entry;
>>>>       }
>>>>         /* Update VM state */
>>>> @@ -3078,7 +3095,7 @@ int amdgpu_vm_make_compute(struct 
>>>> amdgpu_device *adev, struct amdgpu_vm *vm,
>>>>           r = amdgpu_bo_sync_wait(vm->root.bo,
>>>>                       AMDGPU_FENCE_OWNER_UNDEFINED, true);
>>>>           if (r)
>>>> -            goto free_idr;
>>>> +            goto free_pasid_entry;
>>>>             vm->update_funcs = &amdgpu_vm_cpu_funcs;
>>>>       } else {
>>>> @@ -3088,31 +3105,14 @@ int amdgpu_vm_make_compute(struct 
>>>> amdgpu_device *adev, struct amdgpu_vm *vm,
>>>>       vm->last_update = NULL;
>>>>       vm->is_compute_context = true;
>>>>   -    if (vm->pasid) {
>>>> -        unsigned long flags;
>>>> -
>>>> - spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
>>>> -        idr_remove(&adev->vm_manager.pasid_idr, vm->pasid);
>>>> - spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
>>>> -        vm->pasid = 0;
>>>> -    }
>>>> -
>>>>       /* Free the shadow bo for compute VM */
>>>> amdgpu_bo_unref(&to_amdgpu_bo_vm(vm->root.bo)->shadow);
>>>>   -    if (pasid)
>>>> -        vm->pasid = pasid;
>>>> -
>>>>       goto unreserve_bo;
>>>>   -free_idr:
>>>> -    if (pasid) {
>>>> -        unsigned long flags;
>>>> +free_pasid_entry:
>>>> +    amdgpu_vm_set_pasid(adev, vm, 0);
>>>>   - spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
>>>> -        idr_remove(&adev->vm_manager.pasid_idr, pasid);
>>>> - spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
>>>> -    }
>>>>   unreserve_bo:
>>>>       amdgpu_bo_unreserve(vm->root.bo);
>>>>       return r;
>>>> @@ -3128,14 +3128,7 @@ int amdgpu_vm_make_compute(struct 
>>>> amdgpu_device *adev, struct amdgpu_vm *vm,
>>>>    */
>>>>   void amdgpu_vm_release_compute(struct amdgpu_device *adev, struct 
>>>> amdgpu_vm *vm)
>>>>   {
>>>> -    if (vm->pasid) {
>>>> -        unsigned long flags;
>>>> -
>>>> - spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
>>>> -        idr_remove(&adev->vm_manager.pasid_idr, vm->pasid);
>>>> - spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
>>>> -    }
>>>> -    vm->pasid = 0;
>>>> +    amdgpu_vm_set_pasid(adev, vm, 0);
>>>>       vm->is_compute_context = false;
>>>>   }
>>>>   @@ -3159,15 +3152,7 @@ void amdgpu_vm_fini(struct amdgpu_device 
>>>> *adev, struct amdgpu_vm *vm)
>>>>         root = amdgpu_bo_ref(vm->root.bo);
>>>>       amdgpu_bo_reserve(root, true);
>>>> -    if (vm->pasid) {
>>>> -        unsigned long flags;
>>>> -
>>>> - spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
>>>> -        idr_remove(&adev->vm_manager.pasid_idr, vm->pasid);
>>>> - spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
>>>> -        vm->pasid = 0;
>>>> -    }
>>>> -
>>>> +    amdgpu_vm_set_pasid(adev, vm, 0);
>>>>       dma_fence_wait(vm->last_unlocked, false);
>>>>       dma_fence_put(vm->last_unlocked);
>>>>   @@ -3249,8 +3234,7 @@ void amdgpu_vm_manager_init(struct 
>>>> amdgpu_device *adev)
>>>>       adev->vm_manager.vm_update_mode = 0;
>>>>   #endif
>>>>   -    idr_init(&adev->vm_manager.pasid_idr);
>>>> -    spin_lock_init(&adev->vm_manager.pasid_lock);
>>>> +    xa_init_flags(&adev->vm_manager.pasids, XA_FLAGS_LOCK_IRQ);
>>>>   }
>>>>     /**
>>>> @@ -3262,8 +3246,8 @@ void amdgpu_vm_manager_init(struct 
>>>> amdgpu_device *adev)
>>>>    */
>>>>   void amdgpu_vm_manager_fini(struct amdgpu_device *adev)
>>>>   {
>>>> - WARN_ON(!idr_is_empty(&adev->vm_manager.pasid_idr));
>>>> -    idr_destroy(&adev->vm_manager.pasid_idr);
>>>> +    WARN_ON(!xa_empty(&adev->vm_manager.pasids));
>>>> +    xa_destroy(&adev->vm_manager.pasids);
>>>>         amdgpu_vmid_mgr_fini(adev);
>>>>   }
>>>> @@ -3332,13 +3316,13 @@ void amdgpu_vm_get_task_info(struct 
>>>> amdgpu_device *adev, u32 pasid,
>>>>       struct amdgpu_vm *vm;
>>>>       unsigned long flags;
>>>>   - spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
>>>> +    xa_lock_irqsave(&adev->vm_manager.pasids, flags);
>>>>   -    vm = idr_find(&adev->vm_manager.pasid_idr, pasid);
>>>> +    vm = xa_load(&adev->vm_manager.pasids, pasid);
>>>>       if (vm)
>>>>           *task_info = vm->task_info;
>>>>   - spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
>>>> +    xa_unlock_irqrestore(&adev->vm_manager.pasids, flags);
>>>>   }
>>>>     /**
>>>> @@ -3380,15 +3364,15 @@ bool amdgpu_vm_handle_fault(struct 
>>>> amdgpu_device *adev, u32 pasid,
>>>>       struct amdgpu_vm *vm;
>>>>       int r;
>>>>   - spin_lock_irqsave(&adev->vm_manager.pasid_lock, irqflags);
>>>> -    vm = idr_find(&adev->vm_manager.pasid_idr, pasid);
>>>> +    xa_lock_irqsave(&adev->vm_manager.pasids, irqflags);
>>>> +    vm = xa_load(&adev->vm_manager.pasids, pasid);
>>>>       if (vm) {
>>>>           root = amdgpu_bo_ref(vm->root.bo);
>>>>           is_compute_context = vm->is_compute_context;
>>>>       } else {
>>>>           root = NULL;
>>>>       }
>>>> - spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, irqflags);
>>>> +    xa_unlock_irqrestore(&adev->vm_manager.pasids, irqflags);
>>>>         if (!root)
>>>>           return false;
>>>> @@ -3406,11 +3390,11 @@ bool amdgpu_vm_handle_fault(struct 
>>>> amdgpu_device *adev, u32 pasid,
>>>>           goto error_unref;
>>>>         /* Double check that the VM still exists */
>>>> -    spin_lock_irqsave(&adev->vm_manager.pasid_lock, irqflags);
>>>> -    vm = idr_find(&adev->vm_manager.pasid_idr, pasid);
>>>> +    xa_lock_irqsave(&adev->vm_manager.pasids, irqflags);
>>>> +    vm = xa_load(&adev->vm_manager.pasids, pasid);
>>>>       if (vm && vm->root.bo != root)
>>>>           vm = NULL;
>>>> - spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, irqflags);
>>>> +    xa_unlock_irqrestore(&adev->vm_manager.pasids, irqflags);
>>>>       if (!vm)
>>>>           goto error_unlock;
>>>>   diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h 
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>>> index ddb85a85cbba..31c467764162 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>>> @@ -359,8 +359,7 @@ struct amdgpu_vm_manager {
>>>>       /* PASID to VM mapping, will be used in interrupt context to
>>>>        * look up VM of a page fault
>>>>        */
>>>> -    struct idr                pasid_idr;
>>>> -    spinlock_t                pasid_lock;
>>>> +    struct xarray                pasids;
>>>>   };
>>>>     struct amdgpu_bo_va_mapping;
>>>
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/2] drm/amdgpu: use xarray for storing pasid in vm

[Christian König]

Am 23.06.21 um 16:54 schrieb Das, Nirmoy:
>
> On 6/23/2021 3:40 PM, Christian König wrote:
>>
>>
>> Am 23.06.21 um 15:11 schrieb Das, Nirmoy:
>>>
>>> On 6/23/2021 2:50 PM, Christian König wrote:
>>>>
>>>>
>>>> Am 23.06.21 um 14:25 schrieb Nirmoy Das:
>>>>> Replace idr with xarray as we actually need hash functionality.
>>>>> Cleanup code related to vm pasid by adding helper function.
>>>>>
>>>>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
>>>>> ---
>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 134 
>>>>> +++++++++++--------------
>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h |   3 +-
>>>>>   2 files changed, 60 insertions(+), 77 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>> index be841d62a1d4..e047e56a4be2 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>> @@ -87,6 +87,39 @@ struct amdgpu_prt_cb {
>>>>>       struct dma_fence_cb cb;
>>>>>   };
>>>>>   +static int amdgpu_vm_set_pasid(struct amdgpu_device *adev,
>>>>> +                   struct amdgpu_vm *vm,
>>>>> +                   unsigned long pasid)
>>>>
>>>> Some kerneldoc please describing why we have that function.
>>>
>>>
>>> Alright.
>>>
>>>
>>>>
>>>>> +{
>>>>> +    unsigned long flags;
>>>>> +    int r;
>>>>> +
>>>>> +    if (pasid) {
>>>>
>>>> You should probably reorder the code so that the old pasid is first 
>>>> removed and then eventually the new one added.
>>>>
>>>>> + xa_lock_irqsave(&adev->vm_manager.pasids, flags);
>>>>> +        r = xa_err(__xa_store(&adev->vm_manager.pasids, pasid, vm,
>>>>> +                      GFP_ATOMIC));
>>>>> + xa_unlock_irqrestore(&adev->vm_manager.pasids, flags);
>>>>
>>>> As far as I can see this can just use xa_store() which also drops 
>>>> the need for GFP_ATOMIC here.
>>>
>>>
>>> Do we need to have this irqsave/restore to keep passids safe for 
>>> amdgpu_vm_handle_fault() ?
>>
>> No, we need the VM safe not the pasid.
>
>
> Would spin_lock_irq be enough to keep the VM safe then I can use 
> xa_store_irq() and remove some extra locking code?

Yes, when when amdgpu_vm_set_pasid() is called we can be 100% sure that 
the VM won't be freed inside the function or otherwise I really question 
the saneness of the code.

>>
>>> xa_store() takes the spinlock without irq flags so I wanted to keep 
>>> old behavior.
>>
>> Yeah, that's indeed problematic. You need to keep that straight or 
>> lockdep will complain.
>>
>> IIRC there is also a function to reserve an entry before you take the 
>> lock. Maybe use that one.
>
>
> xa_reserve() also takes a spin lock so I think this won't work either 
> with gfp_kernel flag.

No, I just double checked. You can use xa_store_irq() with GFP_KERNEL.

The implementation makes sure to not allocate while holding the lock.

>
>
>>
>>>
>>>
>>>>
>>>>> +        if (r < 0)
>>>>> +            return r;
>>>>> +    } else {
>>>>> +        unsigned long index;
>>>>> +        struct amdgpu_vm *res;
>>>>> +
>>>>> +        xa_lock_irqsave(&adev->vm_manager.pasids, flags);
>>>>> +        xa_for_each(&adev->vm_manager.pasids, index, res) {
>>>>> +            if (res == vm) {
>>>>> + __xa_erase(&adev->vm_manager.pasids, index);
>>>>> +                break;
>>>>> +            }
>>>>> +        }
>>>>> + xa_unlock_irqrestore(&adev->vm_manager.pasids, flags);
>>>>
>>>> That is really ugly, why is that necessary?
>>>
>>> Do you mean the lock around xa_for_each() ? I think I can just used 
>>> lock around __xa_erase() or just xa_erase() if just simple spinlock 
>>> without flags is enough.
>>
>> I mean why you use xa_for_each() here?
>
>
> amdgpu_vm_set_pasid() removes pasid:vm entry when pasid 0 is passed. I 
> need xa_for_each()  to find the index of that vm pointer
>
> so that I can pass that to __xa_erase(). I couldn't find an API which 
> removes an entry  by value.

Ok sounds like you don't understand what semantics I suggest. Let me try 
once more:

if (vm->pasid == pasid)
     return 0;

if (vm->pasid)
     xa_erase_irq(..., vm->pasid);

if (pasid)
     xa_store_irq(..., pasid, vm);

vm->pasid = pasid;

You of course need to add error handling and everything, but in general 
that should do it.

Thanks,
Christian.

>
>
> Regards,
>
> Nirmoy
>
>>
>> Just __xa_erase should be enough.
>>
>> Christian.
>>
>>>
>>>
>>> Regards,
>>>
>>> Nirmoy
>>>
>>>
>>>>
>>>> Christian
>>>>
>>>>> +    }
>>>>> +
>>>>> +    vm->pasid = pasid;
>>>>> +
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>>   /*
>>>>>    * vm eviction_lock can be taken in MMU notifiers. Make sure no 
>>>>> reclaim-FS
>>>>>    * happens while holding this lock anywhere to prevent deadlocks 
>>>>> when
>>>>> @@ -2940,18 +2973,9 @@ int amdgpu_vm_init(struct amdgpu_device 
>>>>> *adev, struct amdgpu_vm *vm, u32 pasid)
>>>>>         amdgpu_bo_unreserve(vm->root.bo);
>>>>>   -    if (pasid) {
>>>>> -        unsigned long flags;
>>>>> -
>>>>> - spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
>>>>> -        r = idr_alloc(&adev->vm_manager.pasid_idr, vm, pasid, 
>>>>> pasid + 1,
>>>>> -                  GFP_ATOMIC);
>>>>> - spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
>>>>> -        if (r < 0)
>>>>> -            goto error_free_root;
>>>>> -
>>>>> -        vm->pasid = pasid;
>>>>> -    }
>>>>> +    r = amdgpu_vm_set_pasid(adev, vm, pasid);
>>>>> +    if (r)
>>>>> +        goto error_free_root;
>>>>>         INIT_KFIFO(vm->faults);
>>>>>   @@ -3039,18 +3063,11 @@ int amdgpu_vm_make_compute(struct 
>>>>> amdgpu_device *adev, struct amdgpu_vm *vm,
>>>>>       if (r)
>>>>>           goto unreserve_bo;
>>>>>   -    if (pasid) {
>>>>> -        unsigned long flags;
>>>>> -
>>>>> - spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
>>>>> -        r = idr_alloc(&adev->vm_manager.pasid_idr, vm, pasid, 
>>>>> pasid + 1,
>>>>> -                  GFP_ATOMIC);
>>>>> - spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
>>>>> -
>>>>> -        if (r == -ENOSPC)
>>>>> -            goto unreserve_bo;
>>>>> -        r = 0;
>>>>> -    }
>>>>> +    /* remove previous {pasid:vm} entry first */
>>>>> +    r = amdgpu_vm_set_pasid(adev, vm, 0);
>>>>> +    r = amdgpu_vm_set_pasid(adev, vm, pasid);
>>>>> +    if (r)
>>>>> +        goto unreserve_bo;
>>>>>         /* Check if PD needs to be reinitialized and do it before
>>>>>        * changing any other state, in case it fails.
>>>>> @@ -3061,7 +3078,7 @@ int amdgpu_vm_make_compute(struct 
>>>>> amdgpu_device *adev, struct amdgpu_vm *vm,
>>>>>                          to_amdgpu_bo_vm(vm->root.bo),
>>>>>                          false);
>>>>>           if (r)
>>>>> -            goto free_idr;
>>>>> +            goto free_pasid_entry;
>>>>>       }
>>>>>         /* Update VM state */
>>>>> @@ -3078,7 +3095,7 @@ int amdgpu_vm_make_compute(struct 
>>>>> amdgpu_device *adev, struct amdgpu_vm *vm,
>>>>>           r = amdgpu_bo_sync_wait(vm->root.bo,
>>>>>                       AMDGPU_FENCE_OWNER_UNDEFINED, true);
>>>>>           if (r)
>>>>> -            goto free_idr;
>>>>> +            goto free_pasid_entry;
>>>>>             vm->update_funcs = &amdgpu_vm_cpu_funcs;
>>>>>       } else {
>>>>> @@ -3088,31 +3105,14 @@ int amdgpu_vm_make_compute(struct 
>>>>> amdgpu_device *adev, struct amdgpu_vm *vm,
>>>>>       vm->last_update = NULL;
>>>>>       vm->is_compute_context = true;
>>>>>   -    if (vm->pasid) {
>>>>> -        unsigned long flags;
>>>>> -
>>>>> - spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
>>>>> -        idr_remove(&adev->vm_manager.pasid_idr, vm->pasid);
>>>>> - spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
>>>>> -        vm->pasid = 0;
>>>>> -    }
>>>>> -
>>>>>       /* Free the shadow bo for compute VM */
>>>>> amdgpu_bo_unref(&to_amdgpu_bo_vm(vm->root.bo)->shadow);
>>>>>   -    if (pasid)
>>>>> -        vm->pasid = pasid;
>>>>> -
>>>>>       goto unreserve_bo;
>>>>>   -free_idr:
>>>>> -    if (pasid) {
>>>>> -        unsigned long flags;
>>>>> +free_pasid_entry:
>>>>> +    amdgpu_vm_set_pasid(adev, vm, 0);
>>>>>   - spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
>>>>> -        idr_remove(&adev->vm_manager.pasid_idr, pasid);
>>>>> - spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
>>>>> -    }
>>>>>   unreserve_bo:
>>>>>       amdgpu_bo_unreserve(vm->root.bo);
>>>>>       return r;
>>>>> @@ -3128,14 +3128,7 @@ int amdgpu_vm_make_compute(struct 
>>>>> amdgpu_device *adev, struct amdgpu_vm *vm,
>>>>>    */
>>>>>   void amdgpu_vm_release_compute(struct amdgpu_device *adev, 
>>>>> struct amdgpu_vm *vm)
>>>>>   {
>>>>> -    if (vm->pasid) {
>>>>> -        unsigned long flags;
>>>>> -
>>>>> - spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
>>>>> -        idr_remove(&adev->vm_manager.pasid_idr, vm->pasid);
>>>>> - spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
>>>>> -    }
>>>>> -    vm->pasid = 0;
>>>>> +    amdgpu_vm_set_pasid(adev, vm, 0);
>>>>>       vm->is_compute_context = false;
>>>>>   }
>>>>>   @@ -3159,15 +3152,7 @@ void amdgpu_vm_fini(struct amdgpu_device 
>>>>> *adev, struct amdgpu_vm *vm)
>>>>>         root = amdgpu_bo_ref(vm->root.bo);
>>>>>       amdgpu_bo_reserve(root, true);
>>>>> -    if (vm->pasid) {
>>>>> -        unsigned long flags;
>>>>> -
>>>>> - spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
>>>>> -        idr_remove(&adev->vm_manager.pasid_idr, vm->pasid);
>>>>> - spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
>>>>> -        vm->pasid = 0;
>>>>> -    }
>>>>> -
>>>>> +    amdgpu_vm_set_pasid(adev, vm, 0);
>>>>>       dma_fence_wait(vm->last_unlocked, false);
>>>>>       dma_fence_put(vm->last_unlocked);
>>>>>   @@ -3249,8 +3234,7 @@ void amdgpu_vm_manager_init(struct 
>>>>> amdgpu_device *adev)
>>>>>       adev->vm_manager.vm_update_mode = 0;
>>>>>   #endif
>>>>>   -    idr_init(&adev->vm_manager.pasid_idr);
>>>>> -    spin_lock_init(&adev->vm_manager.pasid_lock);
>>>>> +    xa_init_flags(&adev->vm_manager.pasids, XA_FLAGS_LOCK_IRQ);
>>>>>   }
>>>>>     /**
>>>>> @@ -3262,8 +3246,8 @@ void amdgpu_vm_manager_init(struct 
>>>>> amdgpu_device *adev)
>>>>>    */
>>>>>   void amdgpu_vm_manager_fini(struct amdgpu_device *adev)
>>>>>   {
>>>>> - WARN_ON(!idr_is_empty(&adev->vm_manager.pasid_idr));
>>>>> -    idr_destroy(&adev->vm_manager.pasid_idr);
>>>>> +    WARN_ON(!xa_empty(&adev->vm_manager.pasids));
>>>>> +    xa_destroy(&adev->vm_manager.pasids);
>>>>>         amdgpu_vmid_mgr_fini(adev);
>>>>>   }
>>>>> @@ -3332,13 +3316,13 @@ void amdgpu_vm_get_task_info(struct 
>>>>> amdgpu_device *adev, u32 pasid,
>>>>>       struct amdgpu_vm *vm;
>>>>>       unsigned long flags;
>>>>>   - spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
>>>>> +    xa_lock_irqsave(&adev->vm_manager.pasids, flags);
>>>>>   -    vm = idr_find(&adev->vm_manager.pasid_idr, pasid);
>>>>> +    vm = xa_load(&adev->vm_manager.pasids, pasid);
>>>>>       if (vm)
>>>>>           *task_info = vm->task_info;
>>>>>   - spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
>>>>> +    xa_unlock_irqrestore(&adev->vm_manager.pasids, flags);
>>>>>   }
>>>>>     /**
>>>>> @@ -3380,15 +3364,15 @@ bool amdgpu_vm_handle_fault(struct 
>>>>> amdgpu_device *adev, u32 pasid,
>>>>>       struct amdgpu_vm *vm;
>>>>>       int r;
>>>>>   - spin_lock_irqsave(&adev->vm_manager.pasid_lock, irqflags);
>>>>> -    vm = idr_find(&adev->vm_manager.pasid_idr, pasid);
>>>>> +    xa_lock_irqsave(&adev->vm_manager.pasids, irqflags);
>>>>> +    vm = xa_load(&adev->vm_manager.pasids, pasid);
>>>>>       if (vm) {
>>>>>           root = amdgpu_bo_ref(vm->root.bo);
>>>>>           is_compute_context = vm->is_compute_context;
>>>>>       } else {
>>>>>           root = NULL;
>>>>>       }
>>>>> - spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, irqflags);
>>>>> +    xa_unlock_irqrestore(&adev->vm_manager.pasids, irqflags);
>>>>>         if (!root)
>>>>>           return false;
>>>>> @@ -3406,11 +3390,11 @@ bool amdgpu_vm_handle_fault(struct 
>>>>> amdgpu_device *adev, u32 pasid,
>>>>>           goto error_unref;
>>>>>         /* Double check that the VM still exists */
>>>>> - spin_lock_irqsave(&adev->vm_manager.pasid_lock, irqflags);
>>>>> -    vm = idr_find(&adev->vm_manager.pasid_idr, pasid);
>>>>> +    xa_lock_irqsave(&adev->vm_manager.pasids, irqflags);
>>>>> +    vm = xa_load(&adev->vm_manager.pasids, pasid);
>>>>>       if (vm && vm->root.bo != root)
>>>>>           vm = NULL;
>>>>> - spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, irqflags);
>>>>> +    xa_unlock_irqrestore(&adev->vm_manager.pasids, irqflags);
>>>>>       if (!vm)
>>>>>           goto error_unlock;
>>>>>   diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h 
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>>>> index ddb85a85cbba..31c467764162 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>>>> @@ -359,8 +359,7 @@ struct amdgpu_vm_manager {
>>>>>       /* PASID to VM mapping, will be used in interrupt context to
>>>>>        * look up VM of a page fault
>>>>>        */
>>>>> -    struct idr                pasid_idr;
>>>>> -    spinlock_t                pasid_lock;
>>>>> +    struct xarray                pasids;
>>>>>   };
>>>>>     struct amdgpu_bo_va_mapping;
>>>>
>>

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/2] drm/amdgpu: use xarray for storing pasid in vm

[Das, Nirmoy]

On 6/23/2021 5:02 PM, Christian König wrote:
>
>
> Am 23.06.21 um 16:54 schrieb Das, Nirmoy:
>>
>> On 6/23/2021 3:40 PM, Christian König wrote:
>>>
>>>
>>> Am 23.06.21 um 15:11 schrieb Das, Nirmoy:
>>>>
>>>> On 6/23/2021 2:50 PM, Christian König wrote:
>>>>>
>>>>>
>>>>> Am 23.06.21 um 14:25 schrieb Nirmoy Das:
>>>>>> Replace idr with xarray as we actually need hash functionality.
>>>>>> Cleanup code related to vm pasid by adding helper function.
>>>>>>
>>>>>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
>>>>>> ---
>>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 134 
>>>>>> +++++++++++--------------
>>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h |   3 +-
>>>>>>   2 files changed, 60 insertions(+), 77 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>>> index be841d62a1d4..e047e56a4be2 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>>> @@ -87,6 +87,39 @@ struct amdgpu_prt_cb {
>>>>>>       struct dma_fence_cb cb;
>>>>>>   };
>>>>>>   +static int amdgpu_vm_set_pasid(struct amdgpu_device *adev,
>>>>>> +                   struct amdgpu_vm *vm,
>>>>>> +                   unsigned long pasid)
>>>>>
>>>>> Some kerneldoc please describing why we have that function.
>>>>
>>>>
>>>> Alright.
>>>>
>>>>
>>>>>
>>>>>> +{
>>>>>> +    unsigned long flags;
>>>>>> +    int r;
>>>>>> +
>>>>>> +    if (pasid) {
>>>>>
>>>>> You should probably reorder the code so that the old pasid is 
>>>>> first removed and then eventually the new one added.
>>>>>
>>>>>> + xa_lock_irqsave(&adev->vm_manager.pasids, flags);
>>>>>> +        r = xa_err(__xa_store(&adev->vm_manager.pasids, pasid, vm,
>>>>>> +                      GFP_ATOMIC));
>>>>>> + xa_unlock_irqrestore(&adev->vm_manager.pasids, flags);
>>>>>
>>>>> As far as I can see this can just use xa_store() which also drops 
>>>>> the need for GFP_ATOMIC here.
>>>>
>>>>
>>>> Do we need to have this irqsave/restore to keep passids safe for 
>>>> amdgpu_vm_handle_fault() ?
>>>
>>> No, we need the VM safe not the pasid.
>>
>>
>> Would spin_lock_irq be enough to keep the VM safe then I can use 
>> xa_store_irq() and remove some extra locking code?
>
> Yes, when when amdgpu_vm_set_pasid() is called we can be 100% sure 
> that the VM won't be freed inside the function or otherwise I really 
> question the saneness of the code.
>
>>>
>>>> xa_store() takes the spinlock without irq flags so I wanted to keep 
>>>> old behavior.
>>>
>>> Yeah, that's indeed problematic. You need to keep that straight or 
>>> lockdep will complain.
>>>
>>> IIRC there is also a function to reserve an entry before you take 
>>> the lock. Maybe use that one.
>>
>>
>> xa_reserve() also takes a spin lock so I think this won't work either 
>> with gfp_kernel flag.
>
> No, I just double checked. You can use xa_store_irq() with GFP_KERNEL.
>
> The implementation makes sure to not allocate while holding the lock.
>
>>
>>
>>>
>>>>
>>>>
>>>>>
>>>>>> +        if (r < 0)
>>>>>> +            return r;
>>>>>> +    } else {
>>>>>> +        unsigned long index;
>>>>>> +        struct amdgpu_vm *res;
>>>>>> +
>>>>>> + xa_lock_irqsave(&adev->vm_manager.pasids, flags);
>>>>>> +        xa_for_each(&adev->vm_manager.pasids, index, res) {
>>>>>> +            if (res == vm) {
>>>>>> + __xa_erase(&adev->vm_manager.pasids, index);
>>>>>> +                break;
>>>>>> +            }
>>>>>> +        }
>>>>>> + xa_unlock_irqrestore(&adev->vm_manager.pasids, flags);
>>>>>
>>>>> That is really ugly, why is that necessary?
>>>>
>>>> Do you mean the lock around xa_for_each() ? I think I can just used 
>>>> lock around __xa_erase() or just xa_erase() if just simple spinlock 
>>>> without flags is enough.
>>>
>>> I mean why you use xa_for_each() here?
>>
>>
>> amdgpu_vm_set_pasid() removes pasid:vm entry when pasid 0 is passed. 
>> I need xa_for_each()  to find the index of that vm pointer
>>
>> so that I can pass that to __xa_erase(). I couldn't find an API which 
>> removes an entry  by value.
>
> Ok sounds like you don't understand what semantics I suggest. Let me 
> try once more:
>
> if (vm->pasid == pasid)
>     return 0;
>
> if (vm->pasid)
>     xa_erase_irq(..., vm->pasid);
>
> if (pasid)
>     xa_store_irq(..., pasid, vm);
>
> vm->pasid = pasid;
>
> You of course need to add error handling and everything, but in 
> general that should do it.


I guess I was over thinking. This looks much straight forward than what 
I was thinking.


Thanks,

Nirmoy


>
> Thanks,
> Christian.
>
>>
>>
>> Regards,
>>
>> Nirmoy
>>
>>>
>>> Just __xa_erase should be enough.
>>>
>>> Christian.
>>>
>>>>
>>>>
>>>> Regards,
>>>>
>>>> Nirmoy
>>>>
>>>>
>>>>>
>>>>> Christian
>>>>>
>>>>>> +    }
>>>>>> +
>>>>>> +    vm->pasid = pasid;
>>>>>> +
>>>>>> +    return 0;
>>>>>> +}
>>>>>> +
>>>>>>   /*
>>>>>>    * vm eviction_lock can be taken in MMU notifiers. Make sure no 
>>>>>> reclaim-FS
>>>>>>    * happens while holding this lock anywhere to prevent 
>>>>>> deadlocks when
>>>>>> @@ -2940,18 +2973,9 @@ int amdgpu_vm_init(struct amdgpu_device 
>>>>>> *adev, struct amdgpu_vm *vm, u32 pasid)
>>>>>>         amdgpu_bo_unreserve(vm->root.bo);
>>>>>>   -    if (pasid) {
>>>>>> -        unsigned long flags;
>>>>>> -
>>>>>> - spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
>>>>>> -        r = idr_alloc(&adev->vm_manager.pasid_idr, vm, pasid, 
>>>>>> pasid + 1,
>>>>>> -                  GFP_ATOMIC);
>>>>>> - spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
>>>>>> -        if (r < 0)
>>>>>> -            goto error_free_root;
>>>>>> -
>>>>>> -        vm->pasid = pasid;
>>>>>> -    }
>>>>>> +    r = amdgpu_vm_set_pasid(adev, vm, pasid);
>>>>>> +    if (r)
>>>>>> +        goto error_free_root;
>>>>>>         INIT_KFIFO(vm->faults);
>>>>>>   @@ -3039,18 +3063,11 @@ int amdgpu_vm_make_compute(struct 
>>>>>> amdgpu_device *adev, struct amdgpu_vm *vm,
>>>>>>       if (r)
>>>>>>           goto unreserve_bo;
>>>>>>   -    if (pasid) {
>>>>>> -        unsigned long flags;
>>>>>> -
>>>>>> - spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
>>>>>> -        r = idr_alloc(&adev->vm_manager.pasid_idr, vm, pasid, 
>>>>>> pasid + 1,
>>>>>> -                  GFP_ATOMIC);
>>>>>> - spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
>>>>>> -
>>>>>> -        if (r == -ENOSPC)
>>>>>> -            goto unreserve_bo;
>>>>>> -        r = 0;
>>>>>> -    }
>>>>>> +    /* remove previous {pasid:vm} entry first */
>>>>>> +    r = amdgpu_vm_set_pasid(adev, vm, 0);
>>>>>> +    r = amdgpu_vm_set_pasid(adev, vm, pasid);
>>>>>> +    if (r)
>>>>>> +        goto unreserve_bo;
>>>>>>         /* Check if PD needs to be reinitialized and do it before
>>>>>>        * changing any other state, in case it fails.
>>>>>> @@ -3061,7 +3078,7 @@ int amdgpu_vm_make_compute(struct 
>>>>>> amdgpu_device *adev, struct amdgpu_vm *vm,
>>>>>> to_amdgpu_bo_vm(vm->root.bo),
>>>>>>                          false);
>>>>>>           if (r)
>>>>>> -            goto free_idr;
>>>>>> +            goto free_pasid_entry;
>>>>>>       }
>>>>>>         /* Update VM state */
>>>>>> @@ -3078,7 +3095,7 @@ int amdgpu_vm_make_compute(struct 
>>>>>> amdgpu_device *adev, struct amdgpu_vm *vm,
>>>>>>           r = amdgpu_bo_sync_wait(vm->root.bo,
>>>>>>                       AMDGPU_FENCE_OWNER_UNDEFINED, true);
>>>>>>           if (r)
>>>>>> -            goto free_idr;
>>>>>> +            goto free_pasid_entry;
>>>>>>             vm->update_funcs = &amdgpu_vm_cpu_funcs;
>>>>>>       } else {
>>>>>> @@ -3088,31 +3105,14 @@ int amdgpu_vm_make_compute(struct 
>>>>>> amdgpu_device *adev, struct amdgpu_vm *vm,
>>>>>>       vm->last_update = NULL;
>>>>>>       vm->is_compute_context = true;
>>>>>>   -    if (vm->pasid) {
>>>>>> -        unsigned long flags;
>>>>>> -
>>>>>> - spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
>>>>>> -        idr_remove(&adev->vm_manager.pasid_idr, vm->pasid);
>>>>>> - spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
>>>>>> -        vm->pasid = 0;
>>>>>> -    }
>>>>>> -
>>>>>>       /* Free the shadow bo for compute VM */
>>>>>> amdgpu_bo_unref(&to_amdgpu_bo_vm(vm->root.bo)->shadow);
>>>>>>   -    if (pasid)
>>>>>> -        vm->pasid = pasid;
>>>>>> -
>>>>>>       goto unreserve_bo;
>>>>>>   -free_idr:
>>>>>> -    if (pasid) {
>>>>>> -        unsigned long flags;
>>>>>> +free_pasid_entry:
>>>>>> +    amdgpu_vm_set_pasid(adev, vm, 0);
>>>>>>   - spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
>>>>>> -        idr_remove(&adev->vm_manager.pasid_idr, pasid);
>>>>>> - spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
>>>>>> -    }
>>>>>>   unreserve_bo:
>>>>>>       amdgpu_bo_unreserve(vm->root.bo);
>>>>>>       return r;
>>>>>> @@ -3128,14 +3128,7 @@ int amdgpu_vm_make_compute(struct 
>>>>>> amdgpu_device *adev, struct amdgpu_vm *vm,
>>>>>>    */
>>>>>>   void amdgpu_vm_release_compute(struct amdgpu_device *adev, 
>>>>>> struct amdgpu_vm *vm)
>>>>>>   {
>>>>>> -    if (vm->pasid) {
>>>>>> -        unsigned long flags;
>>>>>> -
>>>>>> - spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
>>>>>> -        idr_remove(&adev->vm_manager.pasid_idr, vm->pasid);
>>>>>> - spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
>>>>>> -    }
>>>>>> -    vm->pasid = 0;
>>>>>> +    amdgpu_vm_set_pasid(adev, vm, 0);
>>>>>>       vm->is_compute_context = false;
>>>>>>   }
>>>>>>   @@ -3159,15 +3152,7 @@ void amdgpu_vm_fini(struct amdgpu_device 
>>>>>> *adev, struct amdgpu_vm *vm)
>>>>>>         root = amdgpu_bo_ref(vm->root.bo);
>>>>>>       amdgpu_bo_reserve(root, true);
>>>>>> -    if (vm->pasid) {
>>>>>> -        unsigned long flags;
>>>>>> -
>>>>>> - spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
>>>>>> -        idr_remove(&adev->vm_manager.pasid_idr, vm->pasid);
>>>>>> - spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
>>>>>> -        vm->pasid = 0;
>>>>>> -    }
>>>>>> -
>>>>>> +    amdgpu_vm_set_pasid(adev, vm, 0);
>>>>>>       dma_fence_wait(vm->last_unlocked, false);
>>>>>>       dma_fence_put(vm->last_unlocked);
>>>>>>   @@ -3249,8 +3234,7 @@ void amdgpu_vm_manager_init(struct 
>>>>>> amdgpu_device *adev)
>>>>>>       adev->vm_manager.vm_update_mode = 0;
>>>>>>   #endif
>>>>>>   -    idr_init(&adev->vm_manager.pasid_idr);
>>>>>> - spin_lock_init(&adev->vm_manager.pasid_lock);
>>>>>> +    xa_init_flags(&adev->vm_manager.pasids, XA_FLAGS_LOCK_IRQ);
>>>>>>   }
>>>>>>     /**
>>>>>> @@ -3262,8 +3246,8 @@ void amdgpu_vm_manager_init(struct 
>>>>>> amdgpu_device *adev)
>>>>>>    */
>>>>>>   void amdgpu_vm_manager_fini(struct amdgpu_device *adev)
>>>>>>   {
>>>>>> - WARN_ON(!idr_is_empty(&adev->vm_manager.pasid_idr));
>>>>>> -    idr_destroy(&adev->vm_manager.pasid_idr);
>>>>>> + WARN_ON(!xa_empty(&adev->vm_manager.pasids));
>>>>>> +    xa_destroy(&adev->vm_manager.pasids);
>>>>>>         amdgpu_vmid_mgr_fini(adev);
>>>>>>   }
>>>>>> @@ -3332,13 +3316,13 @@ void amdgpu_vm_get_task_info(struct 
>>>>>> amdgpu_device *adev, u32 pasid,
>>>>>>       struct amdgpu_vm *vm;
>>>>>>       unsigned long flags;
>>>>>>   - spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
>>>>>> +    xa_lock_irqsave(&adev->vm_manager.pasids, flags);
>>>>>>   -    vm = idr_find(&adev->vm_manager.pasid_idr, pasid);
>>>>>> +    vm = xa_load(&adev->vm_manager.pasids, pasid);
>>>>>>       if (vm)
>>>>>>           *task_info = vm->task_info;
>>>>>>   - spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
>>>>>> + xa_unlock_irqrestore(&adev->vm_manager.pasids, flags);
>>>>>>   }
>>>>>>     /**
>>>>>> @@ -3380,15 +3364,15 @@ bool amdgpu_vm_handle_fault(struct 
>>>>>> amdgpu_device *adev, u32 pasid,
>>>>>>       struct amdgpu_vm *vm;
>>>>>>       int r;
>>>>>>   - spin_lock_irqsave(&adev->vm_manager.pasid_lock, irqflags);
>>>>>> -    vm = idr_find(&adev->vm_manager.pasid_idr, pasid);
>>>>>> +    xa_lock_irqsave(&adev->vm_manager.pasids, irqflags);
>>>>>> +    vm = xa_load(&adev->vm_manager.pasids, pasid);
>>>>>>       if (vm) {
>>>>>>           root = amdgpu_bo_ref(vm->root.bo);
>>>>>>           is_compute_context = vm->is_compute_context;
>>>>>>       } else {
>>>>>>           root = NULL;
>>>>>>       }
>>>>>> - spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, irqflags);
>>>>>> + xa_unlock_irqrestore(&adev->vm_manager.pasids, irqflags);
>>>>>>         if (!root)
>>>>>>           return false;
>>>>>> @@ -3406,11 +3390,11 @@ bool amdgpu_vm_handle_fault(struct 
>>>>>> amdgpu_device *adev, u32 pasid,
>>>>>>           goto error_unref;
>>>>>>         /* Double check that the VM still exists */
>>>>>> - spin_lock_irqsave(&adev->vm_manager.pasid_lock, irqflags);
>>>>>> -    vm = idr_find(&adev->vm_manager.pasid_idr, pasid);
>>>>>> +    xa_lock_irqsave(&adev->vm_manager.pasids, irqflags);
>>>>>> +    vm = xa_load(&adev->vm_manager.pasids, pasid);
>>>>>>       if (vm && vm->root.bo != root)
>>>>>>           vm = NULL;
>>>>>> - spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, irqflags);
>>>>>> + xa_unlock_irqrestore(&adev->vm_manager.pasids, irqflags);
>>>>>>       if (!vm)
>>>>>>           goto error_unlock;
>>>>>>   diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h 
>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>>>>> index ddb85a85cbba..31c467764162 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>>>>> @@ -359,8 +359,7 @@ struct amdgpu_vm_manager {
>>>>>>       /* PASID to VM mapping, will be used in interrupt context to
>>>>>>        * look up VM of a page fault
>>>>>>        */
>>>>>> -    struct idr                pasid_idr;
>>>>>> -    spinlock_t                pasid_lock;
>>>>>> +    struct xarray                pasids;
>>>>>>   };
>>>>>>     struct amdgpu_bo_va_mapping;
>>>>>
>>>
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/2] drm/amdgpu: free pasid early before converting a vm

[Felix Kuehling]

On 2021-06-23 8:25 a.m., Nirmoy Das wrote:
> VM code should not be responsible for freeing pasid as pasid
> gets allocated outside of VM code, before initializing a vm.
>
> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 6 ++++++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c           | 5 -----
>   2 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> index f96598279593..42e22b1fdfee 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> @@ -1287,6 +1287,12 @@ int amdgpu_amdkfd_gpuvm_acquire_process_vm(struct kgd_dev *kgd,
>   	if (avm->process_info)
>   		return -EINVAL;
>   
> +	/* Free the original amdgpu allocated pasid,
> +	 * will be replaced with kfd allocated pasid
> +	 */
> +	if (avm->pasid)
> +		amdgpu_pasid_free(avm->pasid);
> +

If amdgpu_vm_make_compute fails, amdgpu_driver_postclose_kms will try to 
free the PASID a second time because vm->pasid still points to it. You 
could fix that by moving the amdgpu_pasid_free after a successful 
amdgpu_vm_make_compute call. But that still means you're making an 
assumption about the side effect of amdgpu_vm_make_compute on vm->pasid. 
IMHO, splitting the amdgpu_pasid_free and the vm->pasid = 0 into two 
different functions is a mistake.

Regards,
   Felix


>   	/* Convert VM into a compute VM */
>   	ret = amdgpu_vm_make_compute(adev, avm, pasid);
>   	if (ret)
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 63975bda8e76..be841d62a1d4 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -3094,11 +3094,6 @@ int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>   		spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
>   		idr_remove(&adev->vm_manager.pasid_idr, vm->pasid);
>   		spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
> -
> -		/* Free the original amdgpu allocated pasid
> -		 * Will be replaced with kfd allocated pasid
> -		 */
> -		amdgpu_pasid_free(vm->pasid);
>   		vm->pasid = 0;
>   	}
>   
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/2] drm/amdgpu: free pasid early before converting a vm

[Das, Nirmoy]

On 6/23/2021 9:05 PM, Felix Kuehling wrote:
> On 2021-06-23 8:25 a.m., Nirmoy Das wrote:
>> VM code should not be responsible for freeing pasid as pasid
>> gets allocated outside of VM code, before initializing a vm.
>>
>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 6 ++++++
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c           | 5 -----
>>   2 files changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>> index f96598279593..42e22b1fdfee 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>> @@ -1287,6 +1287,12 @@ int 
>> amdgpu_amdkfd_gpuvm_acquire_process_vm(struct kgd_dev *kgd,
>>       if (avm->process_info)
>>           return -EINVAL;
>>   +    /* Free the original amdgpu allocated pasid,
>> +     * will be replaced with kfd allocated pasid
>> +     */
>> +    if (avm->pasid)
>> +        amdgpu_pasid_free(avm->pasid);
>> +
>
> If amdgpu_vm_make_compute fails, amdgpu_driver_postclose_kms will try 
> to free the PASID a second time because vm->pasid still points to it. 
> You could fix that by moving the amdgpu_pasid_free after a successful 
> amdgpu_vm_make_compute call. But that still means you're making an 
> assumption about the side effect of amdgpu_vm_make_compute on 
> vm->pasid. IMHO, splitting the amdgpu_pasid_free and the vm->pasid = 0 
> into two different functions is a mistake.


Makes sense, I think I can export amdgpu_vm_set_pasid() and then keep 
amdgpu_pasid_free and the vm->pasid = 0 together.


Regards,

Nirmoy

>
> Regards,
>   Felix
>
>
>>       /* Convert VM into a compute VM */
>>       ret = amdgpu_vm_make_compute(adev, avm, pasid);
>>       if (ret)
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index 63975bda8e76..be841d62a1d4 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -3094,11 +3094,6 @@ int amdgpu_vm_make_compute(struct 
>> amdgpu_device *adev, struct amdgpu_vm *vm,
>>           spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
>>           idr_remove(&adev->vm_manager.pasid_idr, vm->pasid);
>> spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
>> -
>> -        /* Free the original amdgpu allocated pasid
>> -         * Will be replaced with kfd allocated pasid
>> -         */
>> -        amdgpu_pasid_free(vm->pasid);
>>           vm->pasid = 0;
>>       }
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx