[PATCH 2/2] drm/amdgpu: use xarray for storing pasid in vm
Das, Nirmoy
nirmoy.das at amd.com
Wed Jun 23 14:54:26 UTC 2021
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 at 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;
>>>
>
More information about the amd-gfx
mailing list