* [PATCH v3] drm/amdgpu: user pages array memory leak fix
@ 2019-10-11 14:36 Yang, Philip
[not found] ` <20191011143620.8785-1-Philip.Yang-5C7GfCeVMHo@public.gmane.org>
0 siblings, 1 reply; 4+ messages in thread
From: Yang, Philip @ 2019-10-11 14:36 UTC (permalink / raw)
To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Koenig, Christian,
thejoe-Re5JQEeQqe8AvxtiuMwx3w
Cc: Yang, Philip
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.
v2: remove unused local variable and amend commit
v3: add back get user pages in gem_userptr_ioctl, to detect application
bug where an userptr VMA is not ananymous memory and reject it.
Bugzilla: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1844962
Signed-off-by: Philip Yang <Philip.Yang@amd.com>
Tested-by: Joe Barnett <thejoe@gmail.com>
Reviewed-by: Christian König <christian.koenig@amd.com>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index c18a153b3d2a..e7b39daa22f6 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -476,7 +476,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);
@@ -493,14 +492,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;
}
--
2.17.1
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v3] drm/amdgpu: user pages array memory leak fix
[not found] ` <20191011143620.8785-1-Philip.Yang-5C7GfCeVMHo@public.gmane.org>
@ 2019-10-11 17:33 ` Kuehling, Felix
[not found] ` <9abf4c92-a30c-ae3d-5544-6428c3aec2ce-5C7GfCeVMHo@public.gmane.org>
2019-10-12 16:12 ` Joe Barnett
1 sibling, 1 reply; 4+ messages in thread
From: Kuehling, Felix @ 2019-10-11 17:33 UTC (permalink / raw)
To: Yang, Philip, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Koenig,
Christian, thejoe-Re5JQEeQqe8AvxtiuMwx3w
On 2019-10-11 10:36 a.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.
>
> v2: remove unused local variable and amend commit
>
> v3: add back get user pages in gem_userptr_ioctl, to detect application
> bug where an userptr VMA is not ananymous memory and reject it.
>
> Bugzilla: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1844962
>
> Signed-off-by: Philip Yang <Philip.Yang@amd.com>
> Tested-by: Joe Barnett <thejoe@gmail.com>
> Reviewed-by: Christian König <christian.koenig@amd.com>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index c18a153b3d2a..e7b39daa22f6 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -476,7 +476,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);
> @@ -493,14 +492,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) {
This if is not needed. kvfree should be able to handle NULL pointers,
and unconditionally setting the pointer to NULL afterwards is not a
problem either. With that fixed, this commit is
Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>
However, I don't think this should be the final solution. My concern
with this solution is, that you end up freeing and regenerating the
user_pages arrays more frequently than necessary: On every command
submission, even if there was no MMU notifier since the last command
submission. I was hoping we could get back to a solution where we can
maintain the same user_pages array across command submissions, since MMU
notifiers are rare. That should reduce overhead from doing all thos page
table walks in HMM on every command submissions when using userptrs.
Regards,
Felix
> kvfree(lobj->user_pages);
> lobj->user_pages = NULL;
> }
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v3] drm/amdgpu: user pages array memory leak fix
[not found] ` <9abf4c92-a30c-ae3d-5544-6428c3aec2ce-5C7GfCeVMHo@public.gmane.org>
@ 2019-10-11 18:47 ` Yang, Philip
0 siblings, 0 replies; 4+ messages in thread
From: Yang, Philip @ 2019-10-11 18:47 UTC (permalink / raw)
To: Kuehling, Felix, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
Koenig, Christian, thejoe-Re5JQEeQqe8AvxtiuMwx3w
On 2019-10-11 1:33 p.m., Kuehling, Felix wrote:
> On 2019-10-11 10:36 a.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.
>>
>> v2: remove unused local variable and amend commit
>>
>> v3: add back get user pages in gem_userptr_ioctl, to detect application
>> bug where an userptr VMA is not ananymous memory and reject it.
>>
>> Bugzilla: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1844962
>>
>> Signed-off-by: Philip Yang <Philip.Yang@amd.com>
>> Tested-by: Joe Barnett <thejoe@gmail.com>
>> Reviewed-by: Christian König <christian.koenig@amd.com>
>> ---
>> drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 4 +---
>> 1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> index c18a153b3d2a..e7b39daa22f6 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> @@ -476,7 +476,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);
>> @@ -493,14 +492,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) {
>
> This if is not needed. kvfree should be able to handle NULL pointers,
> and unconditionally setting the pointer to NULL afterwards is not a
> problem either. With that fixed, this commit is
>
> Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>
>
> However, I don't think this should be the final solution. My concern
> with this solution is, that you end up freeing and regenerating the
> user_pages arrays more frequently than necessary: On every command
> submission, even if there was no MMU notifier since the last command
> submission. I was hoping we could get back to a solution where we can
> maintain the same user_pages array across command submissions, since MMU
> notifiers are rare. That should reduce overhead from doing all thos page
> table walks in HMM on every command submissions when using userptrs.
>
Yes, I will have another patch to address this using hmm_range_valid,
the idea is to allow hmm range tracking cross gem_userptr_ioctl and
cs_ioctl.
Thanks,
Philip
> Regards,
> Felix
>
>
>> kvfree(lobj->user_pages);
>> lobj->user_pages = NULL;
>> }
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v3] drm/amdgpu: user pages array memory leak fix
[not found] ` <20191011143620.8785-1-Philip.Yang-5C7GfCeVMHo@public.gmane.org>
2019-10-11 17:33 ` Kuehling, Felix
@ 2019-10-12 16:12 ` Joe Barnett
1 sibling, 0 replies; 4+ messages in thread
From: Joe Barnett @ 2019-10-12 16:12 UTC (permalink / raw)
To: Yang, Philip; +Cc: Koenig, Christian, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
[-- Attachment #1.1: Type: text/plain, Size: 2298 bytes --]
Confirming that v3 patch still fixes the bug.
Thanks,
-Joe
On Fri, Oct 11, 2019 at 7:36 AM Yang, Philip <Philip.Yang-5C7GfCeVMHo@public.gmane.org> 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.
>
> v2: remove unused local variable and amend commit
>
> v3: add back get user pages in gem_userptr_ioctl, to detect application
> bug where an userptr VMA is not ananymous memory and reject it.
>
> Bugzilla: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1844962
>
> Signed-off-by: Philip Yang <Philip.Yang-5C7GfCeVMHo@public.gmane.org>
> Tested-by: Joe Barnett <thejoe-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Reviewed-by: Christian König <christian.koenig-5C7GfCeVMHo@public.gmane.org>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index c18a153b3d2a..e7b39daa22f6 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -476,7 +476,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);
> @@ -493,14 +492,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;
> }
> --
> 2.17.1
>
>
[-- Attachment #1.2: Type: text/html, Size: 3321 bytes --]
[-- Attachment #2: Type: text/plain, Size: 153 bytes --]
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-10-12 16:12 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-11 14:36 [PATCH v3] drm/amdgpu: user pages array memory leak fix Yang, Philip
[not found] ` <20191011143620.8785-1-Philip.Yang-5C7GfCeVMHo@public.gmane.org>
2019-10-11 17:33 ` Kuehling, Felix
[not found] ` <9abf4c92-a30c-ae3d-5544-6428c3aec2ce-5C7GfCeVMHo@public.gmane.org>
2019-10-11 18:47 ` Yang, Philip
2019-10-12 16:12 ` Joe Barnett
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.