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

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

在 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.


-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