From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Xie, AlexBin" Subject: Re: [PATCH] dmr/amdgpu: Fix wrongly unref of BO Date: Tue, 18 Apr 2017 15:17:17 +0000 Message-ID: References: <1492119298-8526-1-git-send-email-AlexBin.Xie@amd.com>, <58F0495D.60607@amd.com> , <58F57C14.20403@amd.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0543324516==" Return-path: In-Reply-To: <58F57C14.20403-5C7GfCeVMHo@public.gmane.org> Content-Language: en-US List-Id: Discussion list for AMD gfx List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: amd-gfx-bounces-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org Sender: "amd-gfx" To: "Zhou, David(ChunMing)" , "amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org" --===============0543324516== Content-Language: en-US Content-Type: multipart/alternative; boundary="_000_MWHPR1201MB00452889FF2C7FDC55630B4BF2190MWHPR1201MB0045_" --_000_MWHPR1201MB00452889FF2C7FDC55630B4BF2190MWHPR1201MB0045_ Content-Type: text/plain; charset="iso-2022-jp" Content-Transfer-Encoding: quoted-printable 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 unintentiona= l. The caller of function amdgpu_vram_scratch_fini ignores the return error va= lue... 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 correc= tly. To follow up, we can add a warning message when amdgpu_bo_reserve error hap= pens in a different patch. If function call amdgpu_bo_reserve is changed to uninterruptible, this chan= ges driver behaviour. Without a substantial issue, I would be cautious for = such a change. Thanks, Alex Bin Xie ________________________________ From: Zhou, David(ChunMing) Sent: Monday, April 17, 2017 10:38 PM To: Xie, AlexBin; amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org Subject: Re: [PATCH] dmr/amdgpu: Fix wrongly unref of BO On 2017=1B$BG/=1B(B04=1B$B7n=1B(B17=1B$BF|=1B(B 22:54, Xie, AlexBin wrote: Hi David, Thanks for the comments. However, please have look at amdgpu_bo_reserve def= inition. static inline int amdgpu_bo_reserve(struct amdgpu_bo *bo, bool no_intr) Ah, this is a wired wrapper for ttm_bo_reserve. When we call this function like the following: r =3D 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 whe= n amdgpu_bo_reserve return any error. 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 s= eems not matter. I have no strong preference. Regards, David Zhou Thanks, Alex Bin Xie ________________________________ From: Zhou, David(ChunMing) Sent: Friday, April 14, 2017 12:00 AM To: Xie, AlexBin; amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org Subject: Re: [PATCH] dmr/amdgpu: Fix wrongly unref of BO On 2017=1B$BG/=1B(B04=1B$B7n=1B(B14=1B$BF|=1B(B 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 =3D 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 > --- > 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_de= vice *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); > } > > /** --_000_MWHPR1201MB00452889FF2C7FDC55630B4BF2190MWHPR1201MB0045_ Content-Type: text/html; charset="iso-2022-jp" Content-Transfer-Encoding: quoted-printable

Hi David,


When amdgpu_bo_reserve return errors, we cannot release t= he BO. This is not a memory leak. General speaking, memory leak is unn= oticed and unintentional.


The caller of function amdgpu_vram_scratch_fini ign= ores the return error value...


The "memory leak" is not caused by my patch. I= t 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.<= /p>


If function call amdgpu_bo_reserve is ch= anged to uninterruptible, this changes driver behaviour. Without a substantial issue, I would be cautious for such a change.=


Thanks,

Alex Bin Xie


From: Zhou, David(ChunMing)=
Sent: Monday, April 17, 2017 10:38 PM
To: Xie, AlexBin; amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
Subject: Re: [PATCH] dmr/amdgpu: Fix wrongly unref of BO
 


On 2017=1B$BG/=1B(B04=1B$B7n=1B(B17=1B$BF|= =1B(B 22:54, Xie, AlexBin wrote:

Hi David,


Thanks for the comments. However, please have look at a= mdgpu_bo_reserve definition.

static inline int amdgpu_bo_reserve(struct amdgpu_bo *bo, bool no_= intr)

Ah, this is a wired wrapper for ttm_bo_reserve.


When we call this function like the following:

         r =3D 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 with= out kunmap and unpin the BO? This is wrong implementation when amdgpu_bo_reserve return any error.

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 s= eems not matter. I have no strong preference.

Regards,
David Zhou


Thanks,
Alex Bin Xie

From: Zhou, David(ChunMin= g)
Sent: Friday, April 14, 2017 12:00 AM
To: Xie, AlexBin; amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
Subject: Re: [PATCH] dmr/amdgpu: Fix wrongly unref of BO
 


On 2017=1B$BG/=1B(B04=1B$B7n=1B(B14=1B$BF|=1B(B 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 =3D amdgpu_bo_reserve(ad= ev->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 <AlexBin.Xie-5C7GfCeVMHo@public.gmane.org>
> ---
>   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 am= dgpu_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.r= obj);
>   }
>  
>   /**


--_000_MWHPR1201MB00452889FF2C7FDC55630B4BF2190MWHPR1201MB0045_-- --===============0543324516== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KYW1kLWdmeCBt YWlsaW5nIGxpc3QKYW1kLWdmeEBsaXN0cy5mcmVlZGVza3RvcC5vcmcKaHR0cHM6Ly9saXN0cy5m cmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9hbWQtZ2Z4Cg== --===============0543324516==--