* [PATCH] drm/amdgpu: fix memory leak in wait_all_fence
@ 2017-04-07 3:36 Chunming Zhou
[not found] ` <1491536178-5978-1-git-send-email-David1.Zhou-5C7GfCeVMHo@public.gmane.org>
0 siblings, 1 reply; 6+ messages in thread
From: Chunming Zhou @ 2017-04-07 3:36 UTC (permalink / raw)
To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Ken.Wang-5C7GfCeVMHo
Cc: Chunming Zhou
Change-Id: Ib3e271e00e49f10152c1b3eace981a6bf78820de
Signed-off-by: Chunming Zhou <David1.Zhou@amd.com>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 25 +++++++++++++++++++------
1 file changed, 19 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index de1c4c3..d842452 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -1216,22 +1216,28 @@ static int amdgpu_cs_wait_all_fences(struct amdgpu_device *adev,
struct drm_amdgpu_fence *fences)
{
uint32_t fence_count = wait->in.fence_count;
+ struct fence **array;
unsigned int i;
long r = 1;
+ array = kcalloc(fence_count, sizeof(struct fence *), GFP_KERNEL);
+
+ if (array == NULL)
+ return -ENOMEM;
for (i = 0; i < fence_count; i++) {
struct fence *fence;
unsigned long timeout = amdgpu_gem_timeout(wait->in.timeout_ns);
fence = amdgpu_cs_get_fence(adev, filp, &fences[i]);
- if (IS_ERR(fence))
- return PTR_ERR(fence);
- else if (!fence)
+ if (IS_ERR(fence)) {
+ r = PTR_ERR(fence);
+ goto err;
+ } else if (!fence)
continue;
-
+ array[i] = fence;
r = kcl_fence_wait_timeout(fence, true, timeout);
if (r < 0)
- return r;
+ goto err;
if (r == 0)
break;
@@ -1240,7 +1246,14 @@ static int amdgpu_cs_wait_all_fences(struct amdgpu_device *adev,
memset(wait, 0, sizeof(*wait));
wait->out.status = (r > 0);
- return 0;
+ r = 0;
+
+err:
+ for (i = 0; i < fence_count; i++)
+ fence_put(array[i]);
+ kfree(array);
+
+ return r;
}
/**
--
1.9.1
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply related [flat|nested] 6+ messages in thread
* 答复: [PATCH] drm/amdgpu: fix memory leak in wait_all_fence
[not found] ` <1491536178-5978-1-git-send-email-David1.Zhou-5C7GfCeVMHo@public.gmane.org>
@ 2017-04-07 6:45 ` Wang, Ken
2017-04-07 8:36 ` Christian König
1 sibling, 0 replies; 6+ messages in thread
From: Wang, Ken @ 2017-04-07 6:45 UTC (permalink / raw)
To: Zhou, David(ChunMing), amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
[-- Attachment #1.1: Type: text/plain, Size: 2594 bytes --]
Reviewed-by: Ken Wang <Qingqing.Wang@amd.com>
________________________________
发件人: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> 代表 Chunming Zhou <David1.Zhou@amd.com>
发送时间: 2017年4月7日 11:36:18
收件人: amd-gfx@lists.freedesktop.org; Wang, Ken
抄送: Zhou, David(ChunMing)
主题: [PATCH] drm/amdgpu: fix memory leak in wait_all_fence
Change-Id: Ib3e271e00e49f10152c1b3eace981a6bf78820de
Signed-off-by: Chunming Zhou <David1.Zhou@amd.com>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 25 +++++++++++++++++++------
1 file changed, 19 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index de1c4c3..d842452 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -1216,22 +1216,28 @@ static int amdgpu_cs_wait_all_fences(struct amdgpu_device *adev,
struct drm_amdgpu_fence *fences)
{
uint32_t fence_count = wait->in.fence_count;
+ struct fence **array;
unsigned int i;
long r = 1;
+ array = kcalloc(fence_count, sizeof(struct fence *), GFP_KERNEL);
+
+ if (array == NULL)
+ return -ENOMEM;
for (i = 0; i < fence_count; i++) {
struct fence *fence;
unsigned long timeout = amdgpu_gem_timeout(wait->in.timeout_ns);
fence = amdgpu_cs_get_fence(adev, filp, &fences[i]);
- if (IS_ERR(fence))
- return PTR_ERR(fence);
- else if (!fence)
+ if (IS_ERR(fence)) {
+ r = PTR_ERR(fence);
+ goto err;
+ } else if (!fence)
continue;
-
+ array[i] = fence;
r = kcl_fence_wait_timeout(fence, true, timeout);
if (r < 0)
- return r;
+ goto err;
if (r == 0)
break;
@@ -1240,7 +1246,14 @@ static int amdgpu_cs_wait_all_fences(struct amdgpu_device *adev,
memset(wait, 0, sizeof(*wait));
wait->out.status = (r > 0);
- return 0;
+ r = 0;
+
+err:
+ for (i = 0; i < fence_count; i++)
+ fence_put(array[i]);
+ kfree(array);
+
+ return r;
}
/**
--
1.9.1
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[-- Attachment #1.2: Type: text/html, Size: 6743 bytes --]
[-- Attachment #2: Type: text/plain, Size: 154 bytes --]
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/amdgpu: fix memory leak in wait_all_fence
[not found] ` <1491536178-5978-1-git-send-email-David1.Zhou-5C7GfCeVMHo@public.gmane.org>
2017-04-07 6:45 ` 答复: " Wang, Ken
@ 2017-04-07 8:36 ` Christian König
[not found] ` <058491a9-73a6-eddf-73fb-aaac066f9b3e-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org>
1 sibling, 1 reply; 6+ messages in thread
From: Christian König @ 2017-04-07 8:36 UTC (permalink / raw)
To: Chunming Zhou, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
Ken.Wang-5C7GfCeVMHo
Am 07.04.2017 um 05:36 schrieb Chunming Zhou:
> Change-Id: Ib3e271e00e49f10152c1b3eace981a6bf78820de
> Signed-off-by: Chunming Zhou <David1.Zhou@amd.com>
NAK, that will allocate an array for the fence again, which we wanted to
avoid.
We should just drop the fence reference directly after waiting for it.
Regards,
Christian.
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 25 +++++++++++++++++++------
> 1 file changed, 19 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index de1c4c3..d842452 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -1216,22 +1216,28 @@ static int amdgpu_cs_wait_all_fences(struct amdgpu_device *adev,
> struct drm_amdgpu_fence *fences)
> {
> uint32_t fence_count = wait->in.fence_count;
> + struct fence **array;
> unsigned int i;
> long r = 1;
>
> + array = kcalloc(fence_count, sizeof(struct fence *), GFP_KERNEL);
> +
> + if (array == NULL)
> + return -ENOMEM;
> for (i = 0; i < fence_count; i++) {
> struct fence *fence;
> unsigned long timeout = amdgpu_gem_timeout(wait->in.timeout_ns);
>
> fence = amdgpu_cs_get_fence(adev, filp, &fences[i]);
> - if (IS_ERR(fence))
> - return PTR_ERR(fence);
> - else if (!fence)
> + if (IS_ERR(fence)) {
> + r = PTR_ERR(fence);
> + goto err;
> + } else if (!fence)
> continue;
> -
> + array[i] = fence;
> r = kcl_fence_wait_timeout(fence, true, timeout);
> if (r < 0)
> - return r;
> + goto err;
>
> if (r == 0)
> break;
> @@ -1240,7 +1246,14 @@ static int amdgpu_cs_wait_all_fences(struct amdgpu_device *adev,
> memset(wait, 0, sizeof(*wait));
> wait->out.status = (r > 0);
>
> - return 0;
> + r = 0;
> +
> +err:
> + for (i = 0; i < fence_count; i++)
> + fence_put(array[i]);
> + kfree(array);
> +
> + return r;
> }
>
> /**
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/amdgpu: fix memory leak in wait_all_fence
[not found] ` <058491a9-73a6-eddf-73fb-aaac066f9b3e-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org>
@ 2017-04-07 8:46 ` zhoucm1
[not found] ` <58E751D5.50707-5C7GfCeVMHo@public.gmane.org>
0 siblings, 1 reply; 6+ messages in thread
From: zhoucm1 @ 2017-04-07 8:46 UTC (permalink / raw)
To: Christian König, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
Ken.Wang-5C7GfCeVMHo
On 2017年04月07日 16:36, Christian König wrote:
> Am 07.04.2017 um 05:36 schrieb Chunming Zhou:
>> Change-Id: Ib3e271e00e49f10152c1b3eace981a6bf78820de
>> Signed-off-by: Chunming Zhou <David1.Zhou@amd.com>
>
> NAK, that will allocate an array for the fence again, which we wanted
> to avoid.
I don't got your means, the **array just to store fence temporary, freed
at the end.
>
> We should just drop the fence reference directly after waiting for it.
How to handle err case in the middle.
Regards,
David Zhou
>
> Regards,
> Christian.
>
>> ---
>> drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 25 +++++++++++++++++++------
>> 1 file changed, 19 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> index de1c4c3..d842452 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> @@ -1216,22 +1216,28 @@ static int amdgpu_cs_wait_all_fences(struct
>> amdgpu_device *adev,
>> struct drm_amdgpu_fence *fences)
>> {
>> uint32_t fence_count = wait->in.fence_count;
>> + struct fence **array;
>> unsigned int i;
>> long r = 1;
>> + array = kcalloc(fence_count, sizeof(struct fence *), GFP_KERNEL);
>> +
>> + if (array == NULL)
>> + return -ENOMEM;
>> for (i = 0; i < fence_count; i++) {
>> struct fence *fence;
>> unsigned long timeout =
>> amdgpu_gem_timeout(wait->in.timeout_ns);
>> fence = amdgpu_cs_get_fence(adev, filp, &fences[i]);
>> - if (IS_ERR(fence))
>> - return PTR_ERR(fence);
>> - else if (!fence)
>> + if (IS_ERR(fence)) {
>> + r = PTR_ERR(fence);
>> + goto err;
>> + } else if (!fence)
>> continue;
>> -
>> + array[i] = fence;
>> r = kcl_fence_wait_timeout(fence, true, timeout);
>> if (r < 0)
>> - return r;
>> + goto err;
>> if (r == 0)
>> break;
>> @@ -1240,7 +1246,14 @@ static int amdgpu_cs_wait_all_fences(struct
>> amdgpu_device *adev,
>> memset(wait, 0, sizeof(*wait));
>> wait->out.status = (r > 0);
>> - return 0;
>> + r = 0;
>> +
>> +err:
>> + for (i = 0; i < fence_count; i++)
>> + fence_put(array[i]);
>> + kfree(array);
>> +
>> + return r;
>> }
>> /**
>
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/amdgpu: fix memory leak in wait_all_fence
[not found] ` <58E751D5.50707-5C7GfCeVMHo@public.gmane.org>
@ 2017-04-07 8:55 ` Christian König
[not found] ` <6aa1afcf-b2d3-ea84-b08c-3520fffb0b7c-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org>
0 siblings, 1 reply; 6+ messages in thread
From: Christian König @ 2017-04-07 8:55 UTC (permalink / raw)
To: zhoucm1, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Ken.Wang-5C7GfCeVMHo
Am 07.04.2017 um 10:46 schrieb zhoucm1:
>
>
> On 2017年04月07日 16:36, Christian König wrote:
>> Am 07.04.2017 um 05:36 schrieb Chunming Zhou:
>>> Change-Id: Ib3e271e00e49f10152c1b3eace981a6bf78820de
>>> Signed-off-by: Chunming Zhou <David1.Zhou@amd.com>
>>
>> NAK, that will allocate an array for the fence again, which we wanted
>> to avoid.
> I don't got your means, the **array just to store fence temporary,
> freed at the end.
Yeah, but that is unnecessary. We created this function to just avoid that.
>
>>
>> We should just drop the fence reference directly after waiting for it.
> How to handle err case in the middle.
just put a fence_put() directly after fence_wait_timeout(). We don't
need to keep a reference to the fence till the end.
Regards,
Christian.
>
> Regards,
> David Zhou
>>
>> Regards,
>> Christian.
>>
>>> ---
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 25 +++++++++++++++++++------
>>> 1 file changed, 19 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>> index de1c4c3..d842452 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>> @@ -1216,22 +1216,28 @@ static int amdgpu_cs_wait_all_fences(struct
>>> amdgpu_device *adev,
>>> struct drm_amdgpu_fence *fences)
>>> {
>>> uint32_t fence_count = wait->in.fence_count;
>>> + struct fence **array;
>>> unsigned int i;
>>> long r = 1;
>>> + array = kcalloc(fence_count, sizeof(struct fence *),
>>> GFP_KERNEL);
>>> +
>>> + if (array == NULL)
>>> + return -ENOMEM;
>>> for (i = 0; i < fence_count; i++) {
>>> struct fence *fence;
>>> unsigned long timeout =
>>> amdgpu_gem_timeout(wait->in.timeout_ns);
>>> fence = amdgpu_cs_get_fence(adev, filp, &fences[i]);
>>> - if (IS_ERR(fence))
>>> - return PTR_ERR(fence);
>>> - else if (!fence)
>>> + if (IS_ERR(fence)) {
>>> + r = PTR_ERR(fence);
>>> + goto err;
>>> + } else if (!fence)
>>> continue;
>>> -
>>> + array[i] = fence;
>>> r = kcl_fence_wait_timeout(fence, true, timeout);
>>> if (r < 0)
>>> - return r;
>>> + goto err;
>>> if (r == 0)
>>> break;
>>> @@ -1240,7 +1246,14 @@ static int amdgpu_cs_wait_all_fences(struct
>>> amdgpu_device *adev,
>>> memset(wait, 0, sizeof(*wait));
>>> wait->out.status = (r > 0);
>>> - return 0;
>>> + r = 0;
>>> +
>>> +err:
>>> + for (i = 0; i < fence_count; i++)
>>> + fence_put(array[i]);
>>> + kfree(array);
>>> +
>>> + return r;
>>> }
>>> /**
>>
>>
>
> _______________________________________________
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/amdgpu: fix memory leak in wait_all_fence
[not found] ` <6aa1afcf-b2d3-ea84-b08c-3520fffb0b7c-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org>
@ 2017-04-07 9:03 ` zhoucm1
0 siblings, 0 replies; 6+ messages in thread
From: zhoucm1 @ 2017-04-07 9:03 UTC (permalink / raw)
To: Christian König, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
Ken.Wang-5C7GfCeVMHo
On 2017年04月07日 16:55, Christian König wrote:
> Am 07.04.2017 um 10:46 schrieb zhoucm1:
>>
>>
>> On 2017年04月07日 16:36, Christian König wrote:
>>> Am 07.04.2017 um 05:36 schrieb Chunming Zhou:
>>>> Change-Id: Ib3e271e00e49f10152c1b3eace981a6bf78820de
>>>> Signed-off-by: Chunming Zhou <David1.Zhou@amd.com>
>>>
>>> NAK, that will allocate an array for the fence again, which we
>>> wanted to avoid.
>> I don't got your means, the **array just to store fence temporary,
>> freed at the end.
>
> Yeah, but that is unnecessary. We created this function to just avoid
> that.
>
>>
>>>
>>> We should just drop the fence reference directly after waiting for it.
>> How to handle err case in the middle.
>
> just put a fence_put() directly after fence_wait_timeout(). We don't
> need to keep a reference to the fence till the end.
I see your mean, will send V2 soon.
Regards,
David Zhou
>
> Regards,
> Christian.
>
>>
>> Regards,
>> David Zhou
>>>
>>> Regards,
>>> Christian.
>>>
>>>> ---
>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 25
>>>> +++++++++++++++++++------
>>>> 1 file changed, 19 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>> index de1c4c3..d842452 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>> @@ -1216,22 +1216,28 @@ static int amdgpu_cs_wait_all_fences(struct
>>>> amdgpu_device *adev,
>>>> struct drm_amdgpu_fence *fences)
>>>> {
>>>> uint32_t fence_count = wait->in.fence_count;
>>>> + struct fence **array;
>>>> unsigned int i;
>>>> long r = 1;
>>>> + array = kcalloc(fence_count, sizeof(struct fence *),
>>>> GFP_KERNEL);
>>>> +
>>>> + if (array == NULL)
>>>> + return -ENOMEM;
>>>> for (i = 0; i < fence_count; i++) {
>>>> struct fence *fence;
>>>> unsigned long timeout =
>>>> amdgpu_gem_timeout(wait->in.timeout_ns);
>>>> fence = amdgpu_cs_get_fence(adev, filp, &fences[i]);
>>>> - if (IS_ERR(fence))
>>>> - return PTR_ERR(fence);
>>>> - else if (!fence)
>>>> + if (IS_ERR(fence)) {
>>>> + r = PTR_ERR(fence);
>>>> + goto err;
>>>> + } else if (!fence)
>>>> continue;
>>>> -
>>>> + array[i] = fence;
>>>> r = kcl_fence_wait_timeout(fence, true, timeout);
>>>> if (r < 0)
>>>> - return r;
>>>> + goto err;
>>>> if (r == 0)
>>>> break;
>>>> @@ -1240,7 +1246,14 @@ static int amdgpu_cs_wait_all_fences(struct
>>>> amdgpu_device *adev,
>>>> memset(wait, 0, sizeof(*wait));
>>>> wait->out.status = (r > 0);
>>>> - return 0;
>>>> + r = 0;
>>>> +
>>>> +err:
>>>> + for (i = 0; i < fence_count; i++)
>>>> + fence_put(array[i]);
>>>> + kfree(array);
>>>> +
>>>> + return r;
>>>> }
>>>> /**
>>>
>>>
>>
>> _______________________________________________
>> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-04-07 9:03 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-07 3:36 [PATCH] drm/amdgpu: fix memory leak in wait_all_fence Chunming Zhou
[not found] ` <1491536178-5978-1-git-send-email-David1.Zhou-5C7GfCeVMHo@public.gmane.org>
2017-04-07 6:45 ` 答复: " Wang, Ken
2017-04-07 8:36 ` Christian König
[not found] ` <058491a9-73a6-eddf-73fb-aaac066f9b3e-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org>
2017-04-07 8:46 ` zhoucm1
[not found] ` <58E751D5.50707-5C7GfCeVMHo@public.gmane.org>
2017-04-07 8:55 ` Christian König
[not found] ` <6aa1afcf-b2d3-ea84-b08c-3520fffb0b7c-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org>
2017-04-07 9:03 ` zhoucm1
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.