* [PATCH 1/2] drm/amdgpu: free pasid early before converting a vm
@ 2021-06-23 12:25 Nirmoy Das
2021-06-23 12:25 ` [PATCH 2/2] drm/amdgpu: use xarray for storing pasid in vm Nirmoy Das
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Nirmoy Das @ 2021-06-23 12:25 UTC (permalink / raw)
To: amd-gfx; +Cc: Felix.Kuehling, Nirmoy Das, Christian.Koenig
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/2] drm/amdgpu: use xarray for storing pasid in vm
2021-06-23 12:25 [PATCH 1/2] drm/amdgpu: free pasid early before converting a vm Nirmoy Das
@ 2021-06-23 12:25 ` Nirmoy Das
2021-06-23 12:50 ` Christian König
2021-06-23 12:46 ` [PATCH 1/2] drm/amdgpu: free pasid early before converting a vm Christian König
2021-06-23 19:05 ` Felix Kuehling
2 siblings, 1 reply; 11+ messages in thread
From: Nirmoy Das @ 2021-06-23 12:25 UTC (permalink / raw)
To: amd-gfx; +Cc: Felix.Kuehling, Nirmoy Das, Christian.Koenig
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] drm/amdgpu: free pasid early before converting a vm
2021-06-23 12:25 [PATCH 1/2] drm/amdgpu: free pasid early before converting a vm Nirmoy Das
2021-06-23 12:25 ` [PATCH 2/2] drm/amdgpu: use xarray for storing pasid in vm Nirmoy Das
@ 2021-06-23 12:46 ` Christian König
2021-06-23 19:05 ` Felix Kuehling
2 siblings, 0 replies; 11+ messages in thread
From: Christian König @ 2021-06-23 12:46 UTC (permalink / raw)
To: Nirmoy Das, amd-gfx; +Cc: Felix.Kuehling, Christian.Koenig
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] drm/amdgpu: use xarray for storing pasid in vm
2021-06-23 12:25 ` [PATCH 2/2] drm/amdgpu: use xarray for storing pasid in vm Nirmoy Das
@ 2021-06-23 12:50 ` Christian König
2021-06-23 13:11 ` Das, Nirmoy
0 siblings, 1 reply; 11+ messages in thread
From: Christian König @ 2021-06-23 12:50 UTC (permalink / raw)
To: Nirmoy Das, amd-gfx; +Cc: Felix.Kuehling, Christian.Koenig
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] drm/amdgpu: use xarray for storing pasid in vm
2021-06-23 12:50 ` Christian König
@ 2021-06-23 13:11 ` Das, Nirmoy
2021-06-23 13:40 ` Christian König
0 siblings, 1 reply; 11+ messages in thread
From: Das, Nirmoy @ 2021-06-23 13:11 UTC (permalink / raw)
To: Christian König, amd-gfx; +Cc: Felix.Kuehling, Christian.Koenig
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] drm/amdgpu: use xarray for storing pasid in vm
2021-06-23 13:11 ` Das, Nirmoy
@ 2021-06-23 13:40 ` Christian König
2021-06-23 14:54 ` Das, Nirmoy
0 siblings, 1 reply; 11+ messages in thread
From: Christian König @ 2021-06-23 13:40 UTC (permalink / raw)
To: Das, Nirmoy, Christian König, amd-gfx; +Cc: Felix.Kuehling
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] drm/amdgpu: use xarray for storing pasid in vm
2021-06-23 13:40 ` Christian König
@ 2021-06-23 14:54 ` Das, Nirmoy
2021-06-23 15:02 ` Christian König
0 siblings, 1 reply; 11+ messages in thread
From: Das, Nirmoy @ 2021-06-23 14:54 UTC (permalink / raw)
To: Christian König, Christian König, amd-gfx; +Cc: Felix.Kuehling
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] drm/amdgpu: use xarray for storing pasid in vm
2021-06-23 14:54 ` Das, Nirmoy
@ 2021-06-23 15:02 ` Christian König
2021-06-23 15:24 ` Das, Nirmoy
0 siblings, 1 reply; 11+ messages in thread
From: Christian König @ 2021-06-23 15:02 UTC (permalink / raw)
To: Das, Nirmoy, Christian König, amd-gfx; +Cc: Felix.Kuehling
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] drm/amdgpu: use xarray for storing pasid in vm
2021-06-23 15:02 ` Christian König
@ 2021-06-23 15:24 ` Das, Nirmoy
0 siblings, 0 replies; 11+ messages in thread
From: Das, Nirmoy @ 2021-06-23 15:24 UTC (permalink / raw)
To: Christian König, Christian König, amd-gfx; +Cc: Felix.Kuehling
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] drm/amdgpu: free pasid early before converting a vm
2021-06-23 12:25 [PATCH 1/2] drm/amdgpu: free pasid early before converting a vm Nirmoy Das
2021-06-23 12:25 ` [PATCH 2/2] drm/amdgpu: use xarray for storing pasid in vm Nirmoy Das
2021-06-23 12:46 ` [PATCH 1/2] drm/amdgpu: free pasid early before converting a vm Christian König
@ 2021-06-23 19:05 ` Felix Kuehling
2021-06-29 7:48 ` Das, Nirmoy
2 siblings, 1 reply; 11+ messages in thread
From: Felix Kuehling @ 2021-06-23 19:05 UTC (permalink / raw)
To: Nirmoy Das, amd-gfx; +Cc: Christian.Koenig
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] drm/amdgpu: free pasid early before converting a vm
2021-06-23 19:05 ` Felix Kuehling
@ 2021-06-29 7:48 ` Das, Nirmoy
0 siblings, 0 replies; 11+ messages in thread
From: Das, Nirmoy @ 2021-06-29 7:48 UTC (permalink / raw)
To: Felix Kuehling, amd-gfx; +Cc: Christian.Koenig
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
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-06-29 7:48 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-23 12:25 [PATCH 1/2] drm/amdgpu: free pasid early before converting a vm Nirmoy Das
2021-06-23 12:25 ` [PATCH 2/2] drm/amdgpu: use xarray for storing pasid in vm Nirmoy Das
2021-06-23 12:50 ` Christian König
2021-06-23 13:11 ` Das, Nirmoy
2021-06-23 13:40 ` Christian König
2021-06-23 14:54 ` Das, Nirmoy
2021-06-23 15:02 ` Christian König
2021-06-23 15:24 ` Das, Nirmoy
2021-06-23 12:46 ` [PATCH 1/2] drm/amdgpu: free pasid early before converting a vm Christian König
2021-06-23 19:05 ` Felix Kuehling
2021-06-29 7:48 ` Das, Nirmoy
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.