Re: [PATCH] drm/amdgpu: fix old fence check in amdgpu_fence_emit

[Chunming Zhou <zhoucm1-5C7GfCeVMHo@public.gmane.org>]

在 2019/4/1 22:07, Koenig, Christian 写道:
> Am 01.04.19 um 16:04 schrieb Zhou, David(ChunMing):
>> 在 2019/4/1 21:05, Christian König 写道:
>>> Am 01.04.19 um 04:54 schrieb Zhou, David(ChunMing):
>>>>> -----Original Message-----
>>>>> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of
>>>>> Christian K?nig
>>>>> Sent: Saturday, March 30, 2019 2:33 AM
>>>>> To: amd-gfx@lists.freedesktop.org
>>>>> Subject: [PATCH] drm/amdgpu: fix old fence check in amdgpu_fence_emit
>>>>>
>>>>> We don't hold a reference to the old fence, so it can go away any
>>>>> time we are
>>>>> waiting for it to signal.
>>>>>
>>>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>>>> ---
>>>>>     drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c | 24 ++++++++++++++++-
>>>>> ------
>>>>>     1 file changed, 17 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>>>>> index ee47c11e92ce..4dee2326b29c 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>>>>> @@ -136,8 +136,9 @@ int amdgpu_fence_emit(struct amdgpu_ring *ring,
>>>>> struct dma_fence **f,  {
>>>>>         struct amdgpu_device *adev = ring->adev;
>>>>>         struct amdgpu_fence *fence;
>>>>> -    struct dma_fence *old, **ptr;
>>>>> +    struct dma_fence __rcu **ptr;
>>>>>         uint32_t seq;
>>>>> +    int r;
>>>>>
>>>>>         fence = kmem_cache_alloc(amdgpu_fence_slab, GFP_KERNEL);
>>>>>         if (fence == NULL)
>>>>> @@ -153,15 +154,24 @@ int amdgpu_fence_emit(struct amdgpu_ring *ring,
>>>>> struct dma_fence **f,
>>>>>                        seq, flags | AMDGPU_FENCE_FLAG_INT);
>>>>>
>>>>>         ptr = &ring->fence_drv.fences[seq & ring-
>>>>>> fence_drv.num_fences_mask];
>>>>> +    if (unlikely(rcu_dereference_protected(*ptr, 1))) {
>>>> Isn't this line redundant with dma_fence_get_rcu_safe? I think it's
>>>> unnecessary.
>>>> Otherwise looks ok to me.
>>> The key point is lock()+dma_fence_get_rcu_safe(ptr)+unlock() is rather
>>> expensive for something which is really unlikely.
>>>
>>> So we check here if we already see the variable as NULL and if that is
>>> true, then we can just skip the whole expensive dance.
>> but that is most unlikely case, isn't it?  That ptr is NULL seems only
>> when before first fence emitted.
> No, the pointer is set to NULL when the fence is processed. See
> amdgpu_fence_process.

Yeah, I see that RCU__INIT again for every singal fence.

Sorry for noise, pathc is Reviewed-by: Chunming Zhou <david1.zhou@amd.com>


-David

>
> Christian.
>
>>
>> -David
>>
>>> Christian.
>>>
>>>> -David
>>>>> +        struct dma_fence *old;
>>>>> +
>>>>> +        rcu_read_lock();
>>>>> +        old = dma_fence_get_rcu_safe(ptr);
>>>>> +        rcu_read_unlock();
>>>>> +
>>>>> +        if (old) {
>>>>> +            r = dma_fence_wait(old, false);
>>>>> +            dma_fence_put(old);
>>>>> +            if (r)
>>>>> +                return r;
>>>>> +        }
>>>>> +    }
>>>>> +
>>>>>         /* This function can't be called concurrently anyway, otherwise
>>>>>          * emitting the fence would mess up the hardware ring buffer.
>>>>>          */
>>>>> -    old = rcu_dereference_protected(*ptr, 1);
>>>>> -    if (old && !dma_fence_is_signaled(old)) {
>>>>> -        DRM_INFO("rcu slot is busy\n");
>>>>> -        dma_fence_wait(old, false);
>>>>> -    }
>>>>> -
>>>>>         rcu_assign_pointer(*ptr, dma_fence_get(&fence->base));
>>>>>
>>>>>         *f = &fence->base;
>>>>> -- 
>>>>> 2.17.1
>>>>>
>>>>> _______________________________________________
>>>>> amd-gfx mailing list
>>>>> amd-gfx@lists.freedesktop.org
>>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx