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

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

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.

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