From mboxrd@z Thu Jan 1 00:00:00 1970 From: Joe Barnett Subject: Re: [PATCH] drm/amdgpu: user pages array memory leak fix Date: Thu, 10 Oct 2019 08:37:05 -0700 Message-ID: References: <20191003194423.14468-1-Philip.Yang@amd.com> <6640c636-79d1-6159-a1db-5f39f70096a1@amd.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0104754874==" Return-path: In-Reply-To: <6640c636-79d1-6159-a1db-5f39f70096a1-5C7GfCeVMHo@public.gmane.org> 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: "Koenig, Christian" Cc: "Yang, Philip" , "amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org" --===============0104754874== Content-Type: multipart/alternative; boundary="00000000000026f0490594902d67" --00000000000026f0490594902d67 Content-Type: text/plain; charset="UTF-8" Thanks for looking at this Christian. Let me know if there's anything I can do to help. In the meantime, is there a workaround to avoid the memory leak other than using a kernel from before the HMM change? Thanks, -Joe On Fri, Oct 4, 2019 at 8:02 AM Koenig, Christian wrote: > Hi Philip, > > Am 04.10.19 um 15:40 schrieb Yang, Philip: > > Thanks Joe for the test, I will add your Tested-by. > > > > Hi Christian, > > > > May you help review? The change removes the get user pages from > > gem_userptr_ioctl, this was done if flags AMDGPU_GEM_USERPTR_VALIDATE is > > set, and delay the get user pages to amdgpu_cs_parser_bos, and check if > > user pages are invalidated when amdgpu_cs_submit. I don't find issue for > > overnight test, but not sure if there is potential side effect. > > Yeah, seen that. > > The AMDGPU_GEM_USERPTR_VALIDATE was explicitly added to cause a > validation during BO creation because of some very stupid applications. > > I didn't wanted to reject that without offering an alternative, but we > seriously can't do this or it would break Vulkan/OpenGL. > > I need more time to take a closer look, > Christian. > > > > > Thanks, > > Philip > > > > On 2019-10-03 3:44 p.m., Yang, Philip wrote: > >> user_pages array should always be freed after validation regardless if > >> user pages are changed after bo is created because with HMM change parse > >> bo always allocate user pages array to get user pages for userptr bo. > >> > >> Don't need to get user pages while creating uerptr bo because user pages > >> will only be used while validating after parsing userptr bo. > >> > >> Bugzilla: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1844962 > >> > >> v2: remove unused local variable and amend commit > >> > >> Signed-off-by: Philip Yang > >> --- > >> drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 4 +--- > >> drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 23 +---------------------- > >> 2 files changed, 2 insertions(+), 25 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c > >> index 49b767b7238f..961186e7113e 100644 > >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c > >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c > >> @@ -474,7 +474,6 @@ static int amdgpu_cs_list_validate(struct > amdgpu_cs_parser *p, > >> > >> list_for_each_entry(lobj, validated, tv.head) { > >> struct amdgpu_bo *bo = ttm_to_amdgpu_bo(lobj->tv.bo); > >> - bool binding_userptr = false; > >> struct mm_struct *usermm; > >> > >> usermm = amdgpu_ttm_tt_get_usermm(bo->tbo.ttm); > >> @@ -491,14 +490,13 @@ static int amdgpu_cs_list_validate(struct > amdgpu_cs_parser *p, > >> > >> amdgpu_ttm_tt_set_user_pages(bo->tbo.ttm, > >> lobj->user_pages); > >> - binding_userptr = true; > >> } > >> > >> r = amdgpu_cs_validate(p, bo); > >> if (r) > >> return r; > >> > >> - if (binding_userptr) { > >> + if (lobj->user_pages) { > >> kvfree(lobj->user_pages); > >> lobj->user_pages = NULL; > >> } > >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c > >> index a828e3d0bfbd..3ccd61d69964 100644 > >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c > >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c > >> @@ -283,7 +283,6 @@ int amdgpu_gem_create_ioctl(struct drm_device *dev, > void *data, > >> int amdgpu_gem_userptr_ioctl(struct drm_device *dev, void *data, > >> struct drm_file *filp) > >> { > >> - struct ttm_operation_ctx ctx = { true, false }; > >> struct amdgpu_device *adev = dev->dev_private; > >> struct drm_amdgpu_gem_userptr *args = data; > >> struct drm_gem_object *gobj; > >> @@ -326,32 +325,12 @@ int amdgpu_gem_userptr_ioctl(struct drm_device > *dev, void *data, > >> goto release_object; > >> } > >> > >> - if (args->flags & AMDGPU_GEM_USERPTR_VALIDATE) { > >> - r = amdgpu_ttm_tt_get_user_pages(bo, bo->tbo.ttm->pages); > >> - if (r) > >> - goto release_object; > >> - > >> - r = amdgpu_bo_reserve(bo, true); > >> - if (r) > >> - goto user_pages_done; > >> - > >> - amdgpu_bo_placement_from_domain(bo, AMDGPU_GEM_DOMAIN_GTT); > >> - r = ttm_bo_validate(&bo->tbo, &bo->placement, &ctx); > >> - amdgpu_bo_unreserve(bo); > >> - if (r) > >> - goto user_pages_done; > >> - } > >> - > >> r = drm_gem_handle_create(filp, gobj, &handle); > >> if (r) > >> - goto user_pages_done; > >> + goto release_object; > >> > >> args->handle = handle; > >> > >> -user_pages_done: > >> - if (args->flags & AMDGPU_GEM_USERPTR_VALIDATE) > >> - amdgpu_ttm_tt_get_user_pages_done(bo->tbo.ttm); > >> - > >> release_object: > >> drm_gem_object_put_unlocked(gobj); > >> > >> > > --00000000000026f0490594902d67 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Thanks for looking at this Christian.=C2=A0 Let me kn= ow if there's anything I can do to help.

I= n the meantime, is there a workaround to avoid the memory leak other than u= sing a kernel from before the HMM change?

Thanks,<= /div>
-Joe

On Fri, Oct 4, 2019 at 8:02 AM Koenig, Christian &l= t;Christian.Koenig-5C7GfCeVMHo@public.gmane.org&= gt; wrote:
Hi Ph= ilip,

Am 04.10.19 um 15:40 schrieb Yang, Philip:
> Thanks Joe for the test, I will add your Tested-by.
>
> Hi Christian,
>
> May you help review? The change removes the get user pages from
> gem_userptr_ioctl, this was done if flags AMDGPU_GEM_USERPTR_VALIDATE = is
> set, and delay the get user pages to amdgpu_cs_parser_bos, and check i= f
> user pages are invalidated when amdgpu_cs_submit. I don't find iss= ue for
> overnight test, but not sure if there is potential side effect.

Yeah, seen that.

The AMDGPU_GEM_USERPTR_VALIDATE was explicitly added to cause a
validation during BO creation because of some very stupid applications.

I didn't wanted to reject that without offering an alternative, but we =
seriously can't do this or it would break Vulkan/OpenGL.

I need more time to take a closer look,
Christian.

>
> Thanks,
> Philip
>
> On 2019-10-03 3:44 p.m., Yang, Philip wrote:
>> user_pages array should always be freed after validation regardles= s if
>> user pages are changed after bo is created because with HMM change= parse
>> bo always allocate user pages array to get user pages for userptr = bo.
>>
>> Don't need to get user pages while creating uerptr bo because = user pages
>> will only be used while validating after parsing userptr bo.
>>
>> Bugzilla: https://bugs.launchpa= d.net/ubuntu/+source/linux/+bug/1844962
>>
>> v2: remove unused local variable and amend commit
>>
>> Signed-off-by: Philip Yang <Philip.Yang-5C7GfCeVMHo@public.gmane.org>
>> ---
>>=C2=A0 =C2=A0 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c=C2=A0 |=C2=A0 = 4 +---
>>=C2=A0 =C2=A0 drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 23 +-------= ---------------
>>=C2=A0 =C2=A0 2 files changed, 2 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/= drm/amd/amdgpu/amdgpu_cs.c
>> index 49b767b7238f..961186e7113e 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> @@ -474,7 +474,6 @@ static int amdgpu_cs_list_validate(struct amdg= pu_cs_parser *p,
>>=C2=A0 =C2=A0
>>=C2=A0 =C2=A0 =C2=A0 list_for_each_entry(lobj, validated, tv.head) = {
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 struct amdgpu_bo *= bo =3D ttm_to_amdgpu_bo(lobj->tv.bo);
>> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 bool binding_userptr = =3D false;
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 struct mm_struct *= usermm;
>>=C2=A0 =C2=A0
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 usermm =3D amdgpu_= ttm_tt_get_usermm(bo->tbo.ttm);
>> @@ -491,14 +490,13 @@ static int amdgpu_cs_list_validate(struct am= dgpu_cs_parser *p,
>>=C2=A0 =C2=A0
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 amdgpu_ttm_tt_set_user_pages(bo->tbo.ttm,
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0lobj->user_pages);
>> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 binding_userptr =3D true;
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
>>=C2=A0 =C2=A0
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 r =3D amdgpu_cs_va= lidate(p, bo);
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (r)
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 return r;
>>=C2=A0 =C2=A0
>> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (binding_userptr) {<= br> >> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (lobj->user_pages= ) {
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 kvfree(lobj->user_pages);
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 lobj->user_pages =3D NULL;
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu= /drm/amd/amdgpu/amdgpu_gem.c
>> index a828e3d0bfbd..3ccd61d69964 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> @@ -283,7 +283,6 @@ int amdgpu_gem_create_ioctl(struct drm_device = *dev, void *data,
>>=C2=A0 =C2=A0 int amdgpu_gem_userptr_ioctl(struct drm_device *dev, = void *data,
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0struct drm_file *filp)
>>=C2=A0 =C2=A0 {
>> -=C2=A0 =C2=A0 struct ttm_operation_ctx ctx =3D { true, false }; >>=C2=A0 =C2=A0 =C2=A0 struct amdgpu_device *adev =3D dev->dev_pri= vate;
>>=C2=A0 =C2=A0 =C2=A0 struct drm_amdgpu_gem_userptr *args =3D data;<= br> >>=C2=A0 =C2=A0 =C2=A0 struct drm_gem_object *gobj;
>> @@ -326,32 +325,12 @@ int amdgpu_gem_userptr_ioctl(struct drm_devi= ce *dev, void *data,
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 goto release_object;
>>=C2=A0 =C2=A0 =C2=A0 }
>>=C2=A0 =C2=A0
>> -=C2=A0 =C2=A0 if (args->flags & AMDGPU_GEM_USERPTR_VALIDAT= E) {
>> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 r =3D amdgpu_ttm_tt_get= _user_pages(bo, bo->tbo.ttm->pages);
>> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (r)
>> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 goto release_object;
>> -
>> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 r =3D amdgpu_bo_reserve= (bo, true);
>> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (r)
>> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 goto user_pages_done;
>> -
>> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 amdgpu_bo_placement_fro= m_domain(bo, AMDGPU_GEM_DOMAIN_GTT);
>> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 r =3D ttm_bo_validate(&= amp;bo->tbo, &bo->placement, &ctx);
>> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 amdgpu_bo_unreserve(bo)= ;
>> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (r)
>> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 goto user_pages_done;
>> -=C2=A0 =C2=A0 }
>> -
>>=C2=A0 =C2=A0 =C2=A0 r =3D drm_gem_handle_create(filp, gobj, &h= andle);
>>=C2=A0 =C2=A0 =C2=A0 if (r)
>> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 goto user_pages_done; >> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 goto release_object; >>=C2=A0 =C2=A0
>>=C2=A0 =C2=A0 =C2=A0 args->handle =3D handle;
>>=C2=A0 =C2=A0
>> -user_pages_done:
>> -=C2=A0 =C2=A0 if (args->flags & AMDGPU_GEM_USERPTR_VALIDAT= E)
>> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 amdgpu_ttm_tt_get_user_= pages_done(bo->tbo.ttm);
>> -
>>=C2=A0 =C2=A0 release_object:
>>=C2=A0 =C2=A0 =C2=A0 drm_gem_object_put_unlocked(gobj);
>>=C2=A0 =C2=A0
>>

--00000000000026f0490594902d67-- --===============0104754874== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KYW1kLWdmeCBt YWlsaW5nIGxpc3QKYW1kLWdmeEBsaXN0cy5mcmVlZGVza3RvcC5vcmcKaHR0cHM6Ly9saXN0cy5m cmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9hbWQtZ2Z4 --===============0104754874==--