[PATCH] drm/amdgpu: Ignore KFD eviction fences invalidating preemptible DMABuf imports

Eric Huang jinhuieric.huang at amd.com
Fri Apr 28 19:51:28 UTC 2023 


On 2023-04-28 15:42, Felix Kuehling wrote:
> On 2023-04-28 14:09, Eric Huang wrote:
>>
>> On 2023-04-28 12:41, Felix Kuehling wrote:
>>> On 2023-04-28 10:17, Eric Huang wrote:
>>>>
>>>> On 2023-04-27 23:46, Kuehling, Felix wrote:
>>>>> [AMD Official Use Only - General]
>>>>>
>>>>> Re-mapping typically happens after evictions, before a new 
>>>>> eviction fence gets attached. At that time the old eviction fence 
>>>>> should be in the signaled state already, so it can't be signaled 
>>>>> again. Therefore I would expect my patch to help with unmapping 
>>>>> the DMABuf import, without breaking the eviction case.
>>>>>
>>>>> Are you talking about remapping with a map-to-gpu call from user 
>>>>> mode? I think that would only be a problem if the KFD BO was 
>>>>> unmapped and remapped multiple times. The first time it's mapped, 
>>>>> the fresh dmabuf import should be in the SYSTEM domain, so the 
>>>>> validation in the SYSTEM domain before GTT would be a no-op.
>>>> Yes. The case scenario I am talking about is from user mode, 
>>>> mapping->unmapping->re-mapping to the KFD GTT BO will trigger the 
>>>> eviction.
>>>>>
>>>>> I sort of agree that we don't really rely on the eviction fence on 
>>>>> the DMABuf import. The reservation object is shared with the 
>>>>> original BO. Moving the original BO triggers the eviction fence, 
>>>>> so we don't need to trigger it again on the dmabuf import. Other 
>>>>> than moving the original BO, I don't think we can do anything to 
>>>>> the DMABuf import that would require an eviction for KFD use case. 
>>>>> It is a special use case because we control both the import and 
>>>>> the export in the same context.
>>>> I am thinking about no adding KFD eviction fence in first place of 
>>>> mapping original GTT BO, because I don't see it can be evicted in 
>>>> any cases.
>>>
>>> That's not an option. We're not adding an eviction fence. The 
>>> reservation object with the eviction fence is shared between the 
>>> exported BO and the imported one. That's just how DMABuf works. If 
>>> you wait for the fences on the imported BO, you are effectively 
>>> waiting for the fences on the exported BOs. And you can't remove the 
>>> eviction fence from the exported BO.
>>
>> What if the exported BO will be never evicted in reality? I 
>> understand how DMABuf works, and imported BO doesn't have eviction 
>> fence, it shares with exported BO's one if eviction happens, but I 
>> don't see the exported BO can be evicted.
>
> The exported BO can be evicted like any other BO. For example 
> KFDEvictTest is there to cause and test evictions of KFD VRAM BOs. 
> Exporting the BO does not pin it (if DMABUF_MOVE_NOTIFIER is enabled, 
> which it in the upstream kernel), so the exported BO can still be 
> evicted.

Yes. KFD VRAM BO can be evicted, but DMABuf 's original exported BO is 
non-paged/GTT BO. Can GTT BO be evicted? It should be like paged/userptr 
that doesn't have KFD eviction fence.

Regards,
Eric

>
> Regards,
>   Felix
>
>
>>
>> Regards,
>> Eric
>>
>>>
>>> Regards,
>>>   Felix
>>>
>>>
>>>> In theory GTT BO is mapped by user calling mmap() in system memory 
>>>> like userptr, unlike VRAM it will be not evicted by amdgpu vram 
>>>> manager. The only thing is CPU invalidation, but GTT BO doesn't 
>>>> register mmu notifier, that will be a potential problem when 
>>>> switching paged/userptr to non-paged/GTT for mes scheduler.
>>>>
>>>> Regards,
>>>> Eric
>>>>>
>>>>> In the general case dmabuf imports need their eviction fences. For 
>>>>> example when we're importing a DMABuf from somewhere else, so the 
>>>>> eviction fence is not shared with a BO that we already control. 
>>>>> Even then, unmapping a dmabuf from our KFD VM does not need to 
>>>>> wait for any fences on the DMABuf.
>>>>>
>>>>> Regards,
>>>>>    Felix
>>>>>
>>>>> -----Original Message-----
>>>>> From: Huang, JinHuiEric <JinHuiEric.Huang at amd.com>
>>>>> Sent: Thursday, April 27, 2023 14:58
>>>>> To: Kuehling, Felix <Felix.Kuehling at amd.com>; Koenig, Christian 
>>>>> <Christian.Koenig at amd.com>; Christian König 
>>>>> <ckoenig.leichtzumerken at gmail.com>; amd-gfx at lists.freedesktop.org
>>>>> Subject: Re: [PATCH] drm/amdgpu: Ignore KFD eviction fences 
>>>>> invalidating preemptible DMABuf imports
>>>>>
>>>>> Hi Felix,
>>>>>
>>>>> I tested your patch on mGPU systems. It doesn't break any KFD 
>>>>> eviction tests, because tests don't allocate DMABuf import, that 
>>>>> doesn't trigger it's eviction fence. The only thing the patch 
>>>>> affects is in re-mapping DMABuf imports that the eviction will 
>>>>> still be triggered.
>>>>>
>>>>> I have an idea that we probably can remove eviction fence for GTT 
>>>>> bo, because currently the only way to trigger the eviction fence 
>>>>> is by calling ttm_bo_validate for CPU domain in 
>>>>> kfd_mem_dmaunmap_dmabuf. Do you know there is other case to 
>>>>> trigger GTT bo's eviction?
>>>>>
>>>>> Regards,
>>>>> Eric
>>>>>
>>>>> On 2023-04-26 22:21, Felix Kuehling wrote:
>>>>>> 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;
>>>>
>>

More information about the amd-gfx mailing list