Re: [PATCH 2/2] drm/amdgpu: use xarray for storing pasid in vm
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
---
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(>vm_manager.pasids, flags);
+ r = xa_err(__xa_store(>vm_manager.pasids, pasid, vm,
+ GFP_ATOMIC));
+ xa_unlock_irqrestore(>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(>vm_manager.pasids, flags);
+ xa_for_each(>vm_manager.pasids, index, res) {
+ if (res == vm) {
+ __xa_erase(>vm_manager.pasids, index);
+ break;
+ }
+ }
+ xa_unlock_irqrestore(>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(>vm_manager.pasid_lock, flags);
- r = idr_alloc(>vm_manager.pasid_idr, vm, pasid,
pasid + 1,
- GFP_ATOMIC);
- spin_unlock_irqrestore(>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)
Re: [PATCH 2/2] drm/amdgpu: use xarray for storing pasid in vm
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
---
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(>vm_manager.pasids, flags);
+ r = xa_err(__xa_store(>vm_manager.pasids, pasid, vm,
+ GFP_ATOMIC));
+ xa_unlock_irqrestore(>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(>vm_manager.pasids, flags);
+ xa_for_each(>vm_manager.pasids, index, res) {
+ if (res == vm) {
+ __xa_erase(>vm_manager.pasids, index);
+ break;
+ }
+ }
+ xa_unlock_irqrestore(>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(>vm_manager.pasid_lock, flags);
- r = idr_alloc(>vm_manager.pasid_idr, vm, pasid,
pasid + 1,
- GFP_ATOMIC);
- spin_unlock_irqrestore(>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(>vm_manager.pasid_lock, flags);
- r =
Re: [PATCH 2/2] drm/amdgpu: use xarray for storing pasid in vm
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
---
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(>vm_manager.pasids, flags);
+ r = xa_err(__xa_store(>vm_manager.pasids, pasid, vm,
+ GFP_ATOMIC));
+ xa_unlock_irqrestore(>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(>vm_manager.pasids, flags);
+ xa_for_each(>vm_manager.pasids, index, res) {
+ if (res == vm) {
+ __xa_erase(>vm_manager.pasids, index);
+ break;
+ }
+ }
+ xa_unlock_irqrestore(>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(>vm_manager.pasid_lock, flags);
- r = idr_alloc(>vm_manager.pasid_idr, vm, pasid,
pasid + 1,
- GFP_ATOMIC);
- spin_unlock_irqrestore(>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(>vm_manager.pasid_lock, flags);
- r = idr_alloc(>vm_manager.pasid_idr, vm, pasid,
pasid + 1,
- GFP_ATOMIC);
- spin_unlock_irqrestore(>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);
Re: [PATCH 2/2] drm/amdgpu: use xarray for storing pasid in vm
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
---
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(>vm_manager.pasids, flags);
+ r = xa_err(__xa_store(>vm_manager.pasids, pasid, vm,
+ GFP_ATOMIC));
+ xa_unlock_irqrestore(>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(>vm_manager.pasids, flags);
+ xa_for_each(>vm_manager.pasids, index, res) {
+ if (res == vm) {
+ __xa_erase(>vm_manager.pasids, index);
+ break;
+ }
+ }
+ xa_unlock_irqrestore(>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(>vm_manager.pasid_lock, flags);
- r = idr_alloc(>vm_manager.pasid_idr, vm, pasid, pasid
+ 1,
- GFP_ATOMIC);
- spin_unlock_irqrestore(>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(>vm_manager.pasid_lock, flags);
- r = idr_alloc(>vm_manager.pasid_idr, vm, pasid, pasid
+ 1,
- GFP_ATOMIC);
- spin_unlock_irqrestore(>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 = _vm_cpu_funcs;
} else {
@@ -3088,31 +3105,14 @@ int
Re: [PATCH 2/2] drm/amdgpu: use xarray for storing pasid in vm
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
---
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(>vm_manager.pasids, flags);
+ r = xa_err(__xa_store(>vm_manager.pasids, pasid, vm,
+ GFP_ATOMIC));
+ xa_unlock_irqrestore(>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(>vm_manager.pasids, flags);
+ xa_for_each(>vm_manager.pasids, index, res) {
+ if (res == vm) {
+ __xa_erase(>vm_manager.pasids, index);
+ break;
+ }
+ }
+ xa_unlock_irqrestore(>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(>vm_manager.pasid_lock, flags);
- r = idr_alloc(>vm_manager.pasid_idr, vm, pasid, pasid
+ 1,
- GFP_ATOMIC);
- spin_unlock_irqrestore(>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(>vm_manager.pasid_lock, flags);
- r = idr_alloc(>vm_manager.pasid_idr, vm, pasid, pasid
+ 1,
- GFP_ATOMIC);
- spin_unlock_irqrestore(>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 = _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(>vm_manager.pasid_lock, flags);
- idr_remove(>vm_manager.pasid_idr, vm->pasid);
- spin_unlock_irqrestore(>vm_manager.pasid_lock, flags);
-
[PATCH 2/2] drm/amdgpu: use xarray for storing pasid in vm
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
---
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(>vm_manager.pasids, flags);
+ r = xa_err(__xa_store(>vm_manager.pasids, pasid, vm,
+ GFP_ATOMIC));
+ xa_unlock_irqrestore(>vm_manager.pasids, flags);
+ if (r < 0)
+ return r;
+ } else {
+ unsigned long index;
+ struct amdgpu_vm *res;
+
+ xa_lock_irqsave(>vm_manager.pasids, flags);
+ xa_for_each(>vm_manager.pasids, index, res) {
+ if (res == vm) {
+ __xa_erase(>vm_manager.pasids, index);
+ break;
+ }
+ }
+ xa_unlock_irqrestore(>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(>vm_manager.pasid_lock, flags);
- r = idr_alloc(>vm_manager.pasid_idr, vm, pasid, pasid + 1,
- GFP_ATOMIC);
- spin_unlock_irqrestore(>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(>vm_manager.pasid_lock, flags);
- r = idr_alloc(>vm_manager.pasid_idr, vm, pasid, pasid + 1,
- GFP_ATOMIC);
- spin_unlock_irqrestore(>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 = _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(>vm_manager.pasid_lock, flags);
- idr_remove(>vm_manager.pasid_idr, vm->pasid);
- spin_unlock_irqrestore(>vm_manager.pasid_lock, flags);
- vm->pasid = 0;
- }
-
/* Free the shadow bo for compute VM */
amdgpu_bo_unref(_amdgpu_bo_vm(vm->root.bo)->shadow);
- if (pasid)
- vm->pasid = pasid;
-
goto unreserve_bo;
-free_idr:
- if (pasid) {
-
Re: [PATCH 2/2] drm/amdgpu: use xarray for storing pasid in vm
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
---
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(>vm_manager.pasids, flags);
+ r = xa_err(__xa_store(>vm_manager.pasids, pasid, vm,
+ GFP_ATOMIC));
+ xa_unlock_irqrestore(>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(>vm_manager.pasids, flags);
+ xa_for_each(>vm_manager.pasids, index, res) {
+ if (res == vm) {
+ __xa_erase(>vm_manager.pasids, index);
+ break;
+ }
+ }
+ xa_unlock_irqrestore(>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(>vm_manager.pasid_lock, flags);
- r = idr_alloc(>vm_manager.pasid_idr, vm, pasid, pasid + 1,
- GFP_ATOMIC);
- spin_unlock_irqrestore(>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(>vm_manager.pasid_lock, flags);
- r = idr_alloc(>vm_manager.pasid_idr, vm, pasid, pasid + 1,
- GFP_ATOMIC);
- spin_unlock_irqrestore(>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 = _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(>vm_manager.pasid_lock, flags);
- idr_remove(>vm_manager.pasid_idr,
