[RFC PATCH v5] drm/amdgpu: Remove kfd eviction fence before release bo
Christian König
christian.koenig at amd.com
Wed Feb 19 08:24:12 UTC 2020
Am 19.02.20 um 02:54 schrieb Pan, Xinhui:
>
>> 2020年2月19日 07:10,Kuehling, Felix <Felix.Kuehling at amd.com> 写道:
>>
>> Hi Xinhui,
>>
>> Two suggestions inline. Looks good to me otherwise.
>>
>> On 2020-02-17 10:36 p.m., xinhui pan wrote:
>>> No need to trigger eviction as the memory mapping will not be used
>>> anymore.
>>>
>>> All pt/pd bos share same resv, hence the same shared eviction fence.
>>> Everytime page table is freed, the fence will be signled and that cuases
>>> kfd unexcepted evictions.
>>>
>>> Signed-off-by: xinhui pan <xinhui.pan at amd.com>
>>> CC: Christian König <christian.koenig at amd.com>
>>> CC: Felix Kuehling <felix.kuehling at amd.com>
>>> CC: Alex Deucher <alexander.deucher at amd.com>
>>> ---
>>> change from v4:
>>> based on new ttm code.
>>>
>>> change from v3:
>>> fix a coding error
>>>
>>> change from v2:
>>> based on Chris' drm/ttm: rework BO delayed delete patchset.
>>>
>>> ---
>>> ---
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h | 1 +
>>> .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 37 +++++++++++++++++++
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 4 ++
>>> 3 files changed, 42 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
>>> index 9e8db702d878..0ee8aae6c519 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
>>> @@ -96,6 +96,7 @@ struct amdgpu_amdkfd_fence *amdgpu_amdkfd_fence_create(u64 context,
>>> struct mm_struct *mm);
>>> bool amdkfd_fence_check_mm(struct dma_fence *f, struct mm_struct *mm);
>>> struct amdgpu_amdkfd_fence *to_amdgpu_amdkfd_fence(struct dma_fence *f);
>>> +int amdgpu_amdkfd_remove_fence_on_pt_pd_bos(struct amdgpu_bo *bo);
>>> struct amdkfd_process_info {
>>> /* List head of all VMs that belong to a KFD process */
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>> index ef721cb65868..6aa20aa82bd3 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>> @@ -276,6 +276,41 @@ static int amdgpu_amdkfd_remove_eviction_fence(struct amdgpu_bo *bo,
>>> return 0;
>>> }
>>> +int amdgpu_amdkfd_remove_fence_on_pt_pd_bos(struct amdgpu_bo *bo)
>>> +{
>>> + struct amdgpu_bo *root = bo;
>>> + struct amdgpu_vm_bo_base *vm_bo;
>>> + struct amdgpu_vm *vm;
>>> + struct amdkfd_process_info *info;
>>> + struct amdgpu_amdkfd_fence *ef;
>>> + int ret;
>>> +
>>> + while (root->parent)
>>> + root = root->parent;
>> This should not be necessary. Every page table BO has a pointer to a vm_bo that has a pointer to the vm. So you don't need to find the root.
>>
>> This should do the trick:
>>
>> if (!bo->vm_bo || !bo->vm_bo->vm)
>> return 0;
>> vm = bo->vm_bo->vm;
>>
>>
> well,when free page tables, it clears bo->vm_bo first then release pt/pd bo.
> Also we can change the sequence like I do in V2, looks like hit some weird issues.
>
>>> +
>>> + vm_bo = root->vm_bo;
>>> + if (!vm_bo)
>>> + return 0;
>>> +
>>> + vm = vm_bo->vm;
>>> + if (!vm)
>>> + return 0;
>>> +
>>> + info = vm->process_info;
>>> + if (!info || !info->eviction_fence)
>>> + return 0;
>>> +
>>> + ef = container_of(dma_fence_get(&info->eviction_fence->base),
>>> + struct amdgpu_amdkfd_fence, base);
>>> +
>>> + dma_resv_lock(bo->tbo.base.resv, NULL);
Now that Felix mentioned it this should be a dma_resv_trylock().
dma_resv_lock() can intentionally fail randomly for testing purposes,
but trylock() will always succeed because we are the only one
referencing the BO at the moment.
Regards,
Christian.
>>> + ret = amdgpu_amdkfd_remove_eviction_fence(bo, ef);
>>> + dma_resv_unlock(bo->tbo.base.resv);
>>> +
>>> + dma_fence_put(&ef->base);
>>> + return ret;
>>> +}
>>> +
>>> static int amdgpu_amdkfd_bo_validate(struct amdgpu_bo *bo, uint32_t domain,
>>> bool wait)
>>> {
>>> @@ -1045,6 +1080,8 @@ void amdgpu_amdkfd_gpuvm_destroy_cb(struct amdgpu_device *adev,
>>> list_del(&vm->vm_list_node);
>>> mutex_unlock(&process_info->lock);
>>> + vm->process_info = NULL;
>>> +
>>> /* Release per-process resources when last compute VM is destroyed */
>>> if (!process_info->n_vms) {
>>> WARN_ON(!list_empty(&process_info->kfd_bo_list));
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>> index 6f60a581e3ba..16586651020f 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>> @@ -1307,6 +1307,10 @@ void amdgpu_bo_release_notify(struct ttm_buffer_object *bo)
>>> if (abo->kfd_bo)
>>> amdgpu_amdkfd_unreserve_memory_limit(abo);
>>> + /* We only remove the fence if the resv has individualized. */
>>> + if (bo->base.resv == &bo->base._resv)
>> Should this be a WARN_ON? We expect this condition to be always true. If it's not, there should be a noisy warning that something is wrong.
> good point.
>
> thanks
> xinhui
>
>> Regards,
>> Felix
>>
>>
>>> + amdgpu_amdkfd_remove_fence_on_pt_pd_bos(abo);
>>> +
>>> if (bo->mem.mem_type != TTM_PL_VRAM || !bo->mem.mm_node ||
>>> !(abo->flags & AMDGPU_GEM_CREATE_VRAM_WIPE_ON_RELEASE))
>>> return;
More information about the amd-gfx
mailing list