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

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

[Christian König]

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))) {
+		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

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

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

-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

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

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

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

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

[Chunming Zhou]

在 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

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

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

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

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

[Chunming Zhou]

在 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