Re:Re: [PATCH] amdgpu: fixes memleak issue when init failed
发件人："Christian König"
发送日期：2020-04-21 22:53:47
收件人："赵军奎"
抄送人：Alex Deucher ,"David (ChunMing) Zhou"
,David Airlie ,Daniel Vetter
,Tom St Denis ,Ori Messinger
,Sam Ravnborg
,amd-gfx@lists.freedesktop.org,dri-de...@lists.freedesktop.org,linux-ker...@vger.kernel.org,opensource.ker...@vivo.com
主题：Re: [PATCH] amdgpu: fixes memleak issue when init failed>Am 21.04.20 um
15:39 schrieb 赵军奎:
>> 发件人："Christian König"
>> 发送日期：2020-04-21 21:02:27
>> 收件人："赵军奎"
>> 抄送人：Alex Deucher ,"David (ChunMing) Zhou"
>> ,David Airlie ,Daniel Vetter
>> ,Tom St Denis ,Ori Messinger
>> ,Sam Ravnborg
>> ,amd-gfx@lists.freedesktop.org,dri-de...@lists.freedesktop.org,linux-ker...@vger.kernel.org,opensource.ker...@vivo.com
>> 主题：Re: [PATCH] amdgpu: fixes memleak issue when init failed>Am 21.04.20 um
>> 14:09 schrieb 赵军奎:
From: "Christian König"
Date: 2020-04-21 19:22:49
To: Bernard Zhao ,Alex Deucher
,"David (ChunMing) Zhou"
,David Airlie ,Daniel Vetter
,Tom St Denis ,Ori Messinger
,Sam Ravnborg
,amd-gfx@lists.freedesktop.org,dri-de...@lists.freedesktop.org,linux-ker...@vger.kernel.org
Cc: opensource.ker...@vivo.com
Subject: Re: [PATCH] amdgpu: fixes memleak issue when init failed>Am
21.04.20 um 13:17 schrieb Bernard Zhao:
>> VRAM manager and DRM MM when init failed, there is no operaction
>> to free kzalloc memory & remove device file.
>> This will lead to memleak & cause stability issue.
> NAK, failure to create sysfs nodes are not critical.
>
> Christian.
>
OK, get it.
By the way, should i modify this patch to just handle in
error branch, or that it is also unnecessary?
>>> What you can do is to drop the "return ret" if anything with the sysfs
>>> nodes goes wrong and instead print the error code.
>> Emmm, for this part, i am not sure, my modify first print the error, secone
>> release not free memory,
>> and last return error, make everything clear to the system.
>> I think it`s the same with what you mentioned, is there something that I
>> misunderstood?
>
>Yes, maybe an example makes it more clear what to do here. Currently we
>print and error and return when something with the sysfs files goes wrong:
>
>if (ret) {
> DRM_ERROR("Failed to create device file mem_info_vram_total\n");
> return ret;
>}
>
>But what we should do instead is just to print an error and continue and
>in the end return success status:
>
>if (ret)
> DRM_ERROR("Failed to create device file mem_info_vram_total
>(%d)\n", r);
>
>...
>return 0;
>
>Regards,
>Christian.
>
Emmm, i am still confused about two points:
1 Does that mean there is no failed case in this function?
2 There is no need to free the kzmalloc space(no possibility of memory leak )?
Regards,
Bernard
>>
>>> It's really annoying that loading, unloading and loading the driver
>>> again sometimes fails because we have a bug in the sysfs files cleanup.
>>>
>>> We certainly should fix those bugs as well, but they are just not
>>> critical for correct driver functionality.
>>>
>>> Regards,
>>> Christian.
>>
Regards,
Bernard
>> Signed-off-by: Bernard Zhao
>> ---
>> drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c | 24
>>
>> 1 file changed, 19 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
>> index 82a3299e53c0..4c5fb153e6b4 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
>> @@ -175,30 +175,44 @@ static int amdgpu_vram_mgr_init(struct
>> ttm_mem_type_manager *man,
>> ret = device_create_file(adev->dev,
>> &dev_attr_mem_info_vram_total);
>> if (ret) {
>> DRM_ERROR("Failed to create device file
>> mem_info_vram_total\n");
>> -return ret;
>> +goto VRAM_TOTAL_FAIL;
>> }
>> ret = device_create_file(adev->dev,
>> &dev_attr_mem_info_vis_vram_total);
>> if (ret) {
>> DRM_ERROR("Failed to create device file
>> mem_info_vis_vram_total\n");
>> -return ret;
>> +goto VIS_VRAM_TOTA_FAIL;
>> }
>> ret = device_create_file(adev->dev,
>> &dev_attr_mem_info_vram_used);
>> if (ret) {
>> DRM_ERROR("Failed to create device file
>> mem_info_vram_used\n");
>> -return ret;
>> +goto VRAM_USED_FAIL;
>> }
>> ret = device_create_file(adev->dev,
>> &dev_attr_mem_info_vis_vram_used);
>> if (ret) {
>> DRM_ERROR("Failed to create device file
>> mem_info_vis_vram_used\n");
>> -return ret;
>> +goto VIS_VRAM_USED_FAIL;
>>
Re:Re: [PATCH] amdgpu: fixes memleak issue when init failed
发件人："Christian König"
发送日期：2020-04-21 21:02:27
收件人："赵军奎"
抄送人：Alex Deucher ,"David (ChunMing) Zhou"
,David Airlie ,Daniel Vetter
,Tom St Denis ,Ori Messinger
,Sam Ravnborg
,amd-gfx@lists.freedesktop.org,dri-de...@lists.freedesktop.org,linux-ker...@vger.kernel.org,opensource.ker...@vivo.com
主题：Re: [PATCH] amdgpu: fixes memleak issue when init failed>Am 21.04.20 um
14:09 schrieb 赵军奎:
>> From: "Christian König"
>> Date: 2020-04-21 19:22:49
>> To: Bernard Zhao ,Alex Deucher
>> ,"David (ChunMing) Zhou"
>> ,David Airlie ,Daniel Vetter
>> ,Tom St Denis ,Ori Messinger
>> ,Sam Ravnborg
>> ,amd-gfx@lists.freedesktop.org,dri-de...@lists.freedesktop.org,linux-ker...@vger.kernel.org
>> Cc: opensource.ker...@vivo.com
>> Subject: Re: [PATCH] amdgpu: fixes memleak issue when init failed>Am
>> 21.04.20 um 13:17 schrieb Bernard Zhao:
VRAM manager and DRM MM when init failed, there is no operaction
to free kzalloc memory & remove device file.
This will lead to memleak & cause stability issue.
>>> NAK, failure to create sysfs nodes are not critical.
>>>
>>> Christian.
>>>
>> OK, get it.
>> By the way, should i modify this patch to just handle in error
>> branch, or that it is also unnecessary?
>
>What you can do is to drop the "return ret" if anything with the sysfs
>nodes goes wrong and instead print the error code.
Emmm, for this part, i am not sure, my modify first print the error, secone
release not free memory,
and last return error, make everything clear to the system.
I think it`s the same with what you mentioned, is there something that I
misunderstood?
>
>It's really annoying that loading, unloading and loading the driver
>again sometimes fails because we have a bug in the sysfs files cleanup.
>
>We certainly should fix those bugs as well, but they are just not
>critical for correct driver functionality.
>
>Regards,
>Christian.
>>
>> Regards,
>> Bernard
>>
Signed-off-by: Bernard Zhao
---
drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c | 24
1 file changed, 19 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
index 82a3299e53c0..4c5fb153e6b4 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
@@ -175,30 +175,44 @@ static int amdgpu_vram_mgr_init(struct
ttm_mem_type_manager *man,
ret = device_create_file(adev->dev,
&dev_attr_mem_info_vram_total);
if (ret) {
DRM_ERROR("Failed to create device file
mem_info_vram_total\n");
- return ret;
+ goto VRAM_TOTAL_FAIL;
}
ret = device_create_file(adev->dev,
&dev_attr_mem_info_vis_vram_total);
if (ret) {
DRM_ERROR("Failed to create device file
mem_info_vis_vram_total\n");
- return ret;
+ goto VIS_VRAM_TOTA_FAIL;
}
ret = device_create_file(adev->dev,
&dev_attr_mem_info_vram_used);
if (ret) {
DRM_ERROR("Failed to create device file
mem_info_vram_used\n");
- return ret;
+ goto VRAM_USED_FAIL;
}
ret = device_create_file(adev->dev,
&dev_attr_mem_info_vis_vram_used);
if (ret) {
DRM_ERROR("Failed to create device file
mem_info_vis_vram_used\n");
- return ret;
+ goto VIS_VRAM_USED_FAIL;
}
ret = device_create_file(adev->dev,
&dev_attr_mem_info_vram_vendor);
if (ret) {
DRM_ERROR("Failed to create device file
mem_info_vram_vendor\n");
- return ret;
+ goto VRAM_VERDOR_FAIL;
}
return 0;
+
+VRAM_VERDOR_FAIL:
+ device_remove_file(adev->dev, &dev_attr_mem_info_vis_vram_used);
+VIS_VRAM_USED_FAIL:
+ device_remove_file(adev->dev, &dev_attr_mem_info_vram_used);
+RVAM_USED_FAIL:
+ device_remove_file(adev->dev, &dev_attr_mem_info_vis_vram_total);
+VIS_VRAM_TOTA_FAIL:
+ device_remove_file(adev->dev, &dev_attr_mem_info_vram_total);
+VRAM_TOTAL_FAIL:
+ kfree(mgr);
+ man->priv = NULL;
+
+ return ret;
}
/**
>>
>
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re:Re: [PATCH] amdgpu: fixes memleak issue when init failed
From: "Christian König"
Date: 2020-04-21 19:22:49
To: Bernard Zhao ,Alex Deucher
,"David (ChunMing) Zhou" ,David
Airlie ,Daniel Vetter ,Tom St Denis
,Ori Messinger ,Sam Ravnborg
,amd-gfx@lists.freedesktop.org,dri-de...@lists.freedesktop.org,linux-ker...@vger.kernel.org
Cc: opensource.ker...@vivo.com
Subject: Re: [PATCH] amdgpu: fixes memleak issue when init failed>Am 21.04.20
um 13:17 schrieb Bernard Zhao:
>> VRAM manager and DRM MM when init failed, there is no operaction
>> to free kzalloc memory & remove device file.
>> This will lead to memleak & cause stability issue.
>
>NAK, failure to create sysfs nodes are not critical.
>
>Christian.
>
OK, get it.
By the way, should i modify this patch to just handle in error
branch, or that it is also unnecessary?
Regards,
Bernard
>>
>> Signed-off-by: Bernard Zhao
>> ---
>> drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c | 24
>> 1 file changed, 19 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
>> index 82a3299e53c0..4c5fb153e6b4 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
>> @@ -175,30 +175,44 @@ static int amdgpu_vram_mgr_init(struct
>> ttm_mem_type_manager *man,
>> ret = device_create_file(adev->dev, &dev_attr_mem_info_vram_total);
>> if (ret) {
>> DRM_ERROR("Failed to create device file mem_info_vram_total\n");
>> -return ret;
>> +goto VRAM_TOTAL_FAIL;
>> }
>> ret = device_create_file(adev->dev, &dev_attr_mem_info_vis_vram_total);
>> if (ret) {
>> DRM_ERROR("Failed to create device file
>> mem_info_vis_vram_total\n");
>> -return ret;
>> +goto VIS_VRAM_TOTA_FAIL;
>> }
>> ret = device_create_file(adev->dev, &dev_attr_mem_info_vram_used);
>> if (ret) {
>> DRM_ERROR("Failed to create device file mem_info_vram_used\n");
>> -return ret;
>> +goto VRAM_USED_FAIL;
>> }
>> ret = device_create_file(adev->dev, &dev_attr_mem_info_vis_vram_used);
>> if (ret) {
>> DRM_ERROR("Failed to create device file
>> mem_info_vis_vram_used\n");
>> -return ret;
>> +goto VIS_VRAM_USED_FAIL;
>> }
>> ret = device_create_file(adev->dev, &dev_attr_mem_info_vram_vendor);
>> if (ret) {
>> DRM_ERROR("Failed to create device file
>> mem_info_vram_vendor\n");
>> -return ret;
>> +goto VRAM_VERDOR_FAIL;
>> }
>>
>> return 0;
>> +
>> +VRAM_VERDOR_FAIL:
>> +device_remove_file(adev->dev, &dev_attr_mem_info_vis_vram_used);
>> +VIS_VRAM_USED_FAIL:
>> +device_remove_file(adev->dev, &dev_attr_mem_info_vram_used);
>> +RVAM_USED_FAIL:
>> +device_remove_file(adev->dev, &dev_attr_mem_info_vis_vram_total);
>> +VIS_VRAM_TOTA_FAIL:
>> +device_remove_file(adev->dev, &dev_attr_mem_info_vram_total);
>> +VRAM_TOTAL_FAIL:
>> +kfree(mgr);
>> +man->priv = NULL;
>> +
>> +return ret;
>> }
>>
>> /**
>
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
