<html> <head> <meta content="text/html; charset=utf-8" http-equiv="Content-Type"> </head> <body text="#000000" bgcolor="#FFFFFF"> <div class="moz-cite-prefix">Hi AlexBin, No, David is right. This is a very common coding pattern in the kernel module. Freeing up a BO while there still exists a kernel mapping is perfectly ok, the mapping will just be released a bit later on. So this code is actually perfectly ok and just an optimization, but your patch breaks it and creates a memory leak. Regards, Christian. Am 18.04.2017 um 17:17 schrieb Xie, AlexBin: </div> <blockquote cite="mid:MWHPR1201MB00452889FF2C7FDC55630B4BF2190@MWHPR1201MB0045.namprd12.prod.outlook.com" type="cite"> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> <style type="text/css" style="display:none;"></style> <div id="divtagdefaultwrapper" style="font-size:12pt;color:#000000;font-family:Calibri,Arial,Helvetica,sans-serif;" dir="ltr"> Hi David, When amdgpu_bo_reserve return errors, we cannot release the BO. This is not a memory leak. General speaking, memory leak is unnoticed and unintentional. The caller of function amdgpu_vram_scratch_fini ignores the return error value... The "memory leak" is not caused by my patch. It is caused because reserving BO fails. This patch only aim to make function amdgpu_vram_scratch_fini behave correctly. To follow up, we can add a warning message when amdgpu_bo_reserve error happens in a different patch. If function call amdgpu_bo_reserve is changed to uninterruptible, this changes driver behaviour. Without a substantial issue, I would be cautious for such a change. Thanks, Alex Bin Xie <div style="color: rgb(0, 0, 0);"> <hr tabindex="-1" style="display:inline-block; width:98%"> <div id="divRplyFwdMsg" dir="ltr">From: Zhou, David(ChunMing) Sent: Monday, April 17, 2017 10:38 PM To: Xie, AlexBin; <a class="moz-txt-link-abbreviated" href="mailto:amd-gfx@lists.freedesktop.org">amd-gfx@lists.freedesktop.org</a> Subject: Re: [PATCH] dmr/amdgpu: Fix wrongly unref of BO <div> </div> </div> <div> <div class="moz-cite-prefix">On 2017年04月17日 22:54, Xie, AlexBin wrote: </div> <blockquote type="cite"> <div id="divtagdefaultwrapper" dir="ltr" style="font-size:12pt; color:#000000; font-family:Calibri,Arial,Helvetica,sans-serif"> Hi David, Thanks for the comments. However, please have look at amdgpu_bo_reserve definition. static inline int amdgpu_bo_reserve(struct amdgpu_bo *bo, bool no_intr) </div> </blockquote> Ah, this is a wired wrapper for ttm_bo_reserve. <blockquote type="cite"> <div id="divtagdefaultwrapper" dir="ltr" style="font-size:12pt; color:#000000; font-family:Calibri,Arial,Helvetica,sans-serif"> When we call this function like the following: r = amdgpu_bo_reserve(adev->vram_scratch.robj, false); The false means interruptible. On the other hand, when amdgpu_bo_reserve function return error, why do we unref BO without kunmap and unpin the BO? This is wrong implementation when amdgpu_bo_reserve return any error. </div> </blockquote> Yeah, I see your mean, it's in driver un-loading, How about changing to no interruptible? Your patch will make a memleak if bo_reserve fails, but it seems not matter. I have no strong preference. Regards, David Zhou <blockquote type="cite"> <div id="divtagdefaultwrapper" dir="ltr" style="font-size:12pt; color:#000000; font-family:Calibri,Arial,Helvetica,sans-serif"> Thanks, <div>Alex Bin Xie <div style="color:rgb(0,0,0)"> <div> <hr tabindex="-1" style="display:inline-block; width:98%"> <div id="x_divRplyFwdMsg" dir="ltr">From: Zhou, David(ChunMing) Sent: Friday, April 14, 2017 12:00 AM To: Xie, AlexBin; <a moz-do-not-send="true" class="moz-txt-link-abbreviated" href="mailto:amd-gfx@lists.freedesktop.org"> amd-gfx@lists.freedesktop.org</a> Subject: Re: [PATCH] dmr/amdgpu: Fix wrongly unref of BO <div> </div> </div> </div> <div class="PlainText"> On 2017年04月14日 05:34, Alex Xie wrote: > According to comment of amdgpu_bo_reserve, amdgpu_bo_reserve > can return with -ERESTARTSYS. When this function was interrupted > by a signal, BO should not be unref. Otherwise the BO might be > released while is kmapped and pinned, or BO MIGHT be deref > multiple times, etc. r = amdgpu_bo_reserve(adev->vram_scratch.robj, false); we have specified interruptible to false, so -ERESTARTSYS isn't possible here. Thanks, David Zhou > > Change-Id: If76071a768950a0d3ad9d5da7fcae04881807621 > Signed-off-by: Alex Xie <a moz-do-not-send="true" class="moz-txt-link-rfc2396E" href="mailto:AlexBin.Xie@amd.com"> <AlexBin.Xie@amd.com></a> > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > index 53996e3..1dcc2d1 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > @@ -355,8 +355,8 @@ static void amdgpu_vram_scratch_fini(struct amdgpu_device *adev) > amdgpu_bo_kunmap(adev->vram_scratch.robj); > amdgpu_bo_unpin(adev->vram_scratch.robj); > amdgpu_bo_unreserve(adev->vram_scratch.robj); > + amdgpu_bo_unref(&adev->vram_scratch.robj); > } > - amdgpu_bo_unref(&adev->vram_scratch.robj); > } > > /** </div> </div> </div> </div> </blockquote> </div> </div> </div> <fieldset class="mimeAttachmentHeader"></fieldset> <pre wrap="">_______________________________________________ amd-gfx mailing list <a class="moz-txt-link-abbreviated" href="mailto:amd-gfx@lists.freedesktop.org">amd-gfx@lists.freedesktop.org</a> <a class="moz-txt-link-freetext" href="https://lists.freedesktop.org/mailman/listinfo/amd-gfx">https://lists.freedesktop.org/mailman/listinfo/amd-gfx</a> </pre> </blockquote> </body> </html>