Re: [PATCH] drm/amdgpu: fix old fence check in amdgpu_fence_emit
在 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 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
>>>>> ---
>>>>> 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 = >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
-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(>base));
>>>>>
>>>>> *f = >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
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 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
>>>> ---
>>>> 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 = >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(>base));
>>>>
>>>> *f = >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
在 2019/4/1 21:05, Christian König 写道:
> Am 01.04.19 um 04:54 schrieb Zhou, David(ChunMing):
>>
>>> -Original Message-
>>> From: amd-gfx 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
>>> ---
>>> 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 = >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(>base));
>>>
>>> *f = >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
Am 01.04.19 um 04:54 schrieb Zhou, David(ChunMing):
-Original Message-
From: amd-gfx 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
---
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 = >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(>base));
*f = >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
> -Original Message-
> From: amd-gfx 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
> ---
> 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 = >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(>base));
>
> *f = >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
[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
---
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 = >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(>base));
*f = >base;
--
2.17.1
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
