[PATCH] drm/amdgpu: Ignore KFD eviction fences invalidating preemptible DMABuf imports
Felix Kuehling
felix.kuehling at amd.com
Thu Apr 27 02:21:12 UTC 2023
Hi Eric,
Can you try if the attached patch fixes the problem without breaking the
eviction tests on a multi-GPU PCIe P2P system?
Thanks,
Felix
On 2023-04-26 13:02, Christian König wrote:
> Am 26.04.23 um 18:58 schrieb Felix Kuehling:
>>
>> On 2023-04-26 9:03, Christian König wrote:
>>> Am 25.04.23 um 16:11 schrieb Eric Huang:
>>>> Hi Christian,
>>>>
>>>> What do you think about Felix's explanation?
>>>
>>> That's unfortunately not something we can do here.
>>>
>>>>
>>>> Regards,
>>>> Eric
>>>>
>>>> On 2023-04-13 09:28, Felix Kuehling wrote:
>>>>> Am 2023-04-13 um 07:35 schrieb Christian König:
>>>>>> Am 13.04.23 um 03:01 schrieb Felix Kuehling:
>>>>>>> Am 2023-04-12 um 18:25 schrieb Eric Huang:
>>>>>>>> It is to avoid redundant eviction for KFD's DMAbuf import
>>>>>>>> bo when dmaunmapping DMAbuf. The DMAbuf import bo has
>>>>>>>> been set as AMDGPU_PL_PREEMPT in KFD when mapping.
>>>>>>>>
>>>>>>>> Signed-off-by: Eric Huang <jinhuieric.huang at amd.com>
>>>>>>>
>>>>>>> Reviewed-by: Felix Kuehling <Felix.Kuehling at amd.com>
>>>>>>>
>>>>>>> I'd like to get an Acked-by from Christian as well before
>>>>>>> submitting this.
>>>>>>
>>>>>> I have to admit that I only partially followed the internal
>>>>>> discussion, but in general you need a *really* good explanation
>>>>>> for this.
>>>>>>
>>>>>> E.g. add code comment and explain in the commit message
>>>>>> extensively why this is needed and why there are no alternatives.
>>>>>
>>>>> OK. I'll give it a shot:
>>>>>
>>>>> This code path is used among other things when invalidating DMABuf
>>>>> imports. These imports share a reservation object with the
>>>>> exported
>>>>> BO. Waiting on all the fences in this reservation will trigger KFD
>>>>> eviction fences unnecessarily, for example when a DMABuf import
>>>>> for
>>>>> a DMA mapping on a secondary GPU is being unmapped explicitly.
>>>>> Only
>>>>> moving the original exported BO requires stopping KFD user mode
>>>>> queues. If the invalidation is triggered through a move notifier
>>>>> from the exported BO, then moving the original BO already
>>>>> triggered
>>>>> the eviction fence and we don't need to wait for it again on
>>>>> the import.
>>>>>
>>>>> We can identify DMABuf imports in KFD for secondary GPU DMA
>>>>> mappings
>>>>> by the mem_type AMDGPU_PL_PREEMPT. In this case, use a wait
>>>>> operation that ignores KFD eviction fences.
>>>>>
>>>>> How does this sound?
>>>
>>> To be honest like quite a bad idea. Why in the world are imported
>>> BOs moved from GTT to SYSTEM in the first place?
>>
>> As I understand it, the way to update SG tables in SG BOs (e.g.
>> userptr and dmabuf imports) is to move them back and forth between
>> system and GTT domains. If we left the import in the GTT domain all
>> the time, we would have no way to update it, e.g. after an eviction.
>> Currently the move to the system domain is done in the unmap code path.
>>
>> Before memory is freed, we also need to unmap it from GPUVM,
>> including the DMABuf imports on remote GPUs. For the above reason
>> that currently includes moving the import to the system domain. If we
>> removed that from the unmap code path, we'd need to do the move to
>> system somewhere else, maybe in the mapping/validation path.
>>
>>
>>>
>>> The only reason for this I can think of is that the DMA mappings
>>> become invalid for some reasons and in this case waiting for the KFD
>>> fence is actually the absolutely right thing to do.
>>
>> In this case the reason the only reason for unmapping the memory is
>> that we're about to free the memory and its DMABuf imports on other
>> GPUs. This is coming from the application with a promise "I'm no
>> longer accessing the memory". We don't need to wait for fences here.
>> We only need to invalidate the PTEs to make sure that any further
>> buggy access by the application will fault.
>
> Well in this case just free the BO and it's bo_va structure. The core
> handling should take care of clearing all the freed up regions.
>
> As for updating the SG of a BO you indeed need to move it from GTT to
> SYSTEM and back, but in this case we should either indeed wait for the
> KFD fence since page tables in between the operation still have the
> old entries or we should destroy the BO and create a new one. The
> later would overwrite the PTEs with invalid entries first and then
> fill in new valid ones.
>
> Regards,
> Christian.
>
>>
>> Regards,
>> Felix
>>
>>
>>>
>>> Regards,
>>> Christian.
>>>
>>>>>
>>>>> Regards,
>>>>> Felix
>>>>>
>>>>>
>>>>>>
>>>>>> Regards,
>>>>>> Christian.
>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Felix
>>>>>>>
>>>>>>>
>>>>>>>> ---
>>>>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 7 ++++++-
>>>>>>>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>>>>>> index 2430f3e9f3a7..64795fe9eecb 100644
>>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>>>>>> @@ -526,7 +526,12 @@ static int amdgpu_bo_move(struct
>>>>>>>> ttm_buffer_object *bo, bool evict,
>>>>>>>> if ((old_mem->mem_type == TTM_PL_TT ||
>>>>>>>> old_mem->mem_type == AMDGPU_PL_PREEMPT) &&
>>>>>>>> new_mem->mem_type == TTM_PL_SYSTEM) {
>>>>>>>> - r = ttm_bo_wait_ctx(bo, ctx);
>>>>>>>> + if (old_mem->mem_type == AMDGPU_PL_PREEMPT)
>>>>>>>> + r = amdgpu_bo_sync_wait(abo,
>>>>>>>> + AMDGPU_FENCE_OWNER_KFD,
>>>>>>>> + ctx->interruptible);
>>>>>>>> + else
>>>>>>>> + r = ttm_bo_wait_ctx(bo, ctx);
>>>>>>>> if (r)
>>>>>>>> return r;
>>>>>>
>>>>
>>>
>
-------------- next part --------------
From b4a0cdc2002796dd7c29469154670b6590b1155d Mon Sep 17 00:00:00 2001
From: Felix Kuehling <Felix.Kuehling at amd.com>
Date: Wed, 26 Apr 2023 22:07:43 -0400
Subject: [PATCH] drm/amdkfd: Don't trigger evictions unmapping dmabuf
attachments
Don't move DMABuf attachments for PCIe P2P mappings to the SYSTEM domain
when unmapping. This avoids triggering eviction fences unnecessarily.
Instead do the move to SYSTEM and back to GTT when mapping these
attachments to ensure the SG table gets updated.
Signed-off-by: Felix Kuehling <Felix.Kuehling at amd.com>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 15 ++++++++++-----
1 file changed, 10 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 862e94fbf53c..638610296024 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -526,6 +526,12 @@ kfd_mem_dmamap_dmabuf(struct kfd_mem_attachment *attachment)
{
struct ttm_operation_ctx ctx = {.interruptible = true};
struct amdgpu_bo *bo = attachment->bo_va->base.bo;
+ int ret;
+
+ amdgpu_bo_placement_from_domain(bo, AMDGPU_GEM_DOMAIN_CPU);
+ ret = ttm_bo_validate(&bo->tbo, &bo->placement, &ctx);
+ if (ret)
+ return ret;
amdgpu_bo_placement_from_domain(bo, AMDGPU_GEM_DOMAIN_GTT);
return ttm_bo_validate(&bo->tbo, &bo->placement, &ctx);
@@ -658,11 +664,10 @@ kfd_mem_dmaunmap_userptr(struct kgd_mem *mem,
static void
kfd_mem_dmaunmap_dmabuf(struct kfd_mem_attachment *attachment)
{
- struct ttm_operation_ctx ctx = {.interruptible = true};
- struct amdgpu_bo *bo = attachment->bo_va->base.bo;
-
- amdgpu_bo_placement_from_domain(bo, AMDGPU_GEM_DOMAIN_CPU);
- ttm_bo_validate(&bo->tbo, &bo->placement, &ctx);
+ /* This is a no-op. We don't want to trigger eviction fences when
+ * unmapping DMABufs. Therefore the invalidation (moving to system
+ * domain) is done in kfd_mem_dmamap_dmabuf.
+ */
}
/**
--
2.34.1
More information about the amd-gfx
mailing list