* [PATCH] drm/amdgpu: user pages array memory leak fix
@ 2019-10-03 19:07 Yang, Philip
[not found] ` <20191003190653.15455-1-Philip.Yang-5C7GfCeVMHo@public.gmane.org>
0 siblings, 1 reply; 8+ messages in thread
From: Yang, Philip @ 2019-10-03 19:07 UTC (permalink / raw)
To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, thejoe-Re5JQEeQqe8AvxtiuMwx3w
Cc: Yang, Philip
user_pages array should be freed regardless if user pages are
invalidated after bo is created because HMM change to always allocate
user pages array to get user pages while parsing user page bo.
Don't need to to get user pages while creating bo because user pages
will only be used after parsing user page bo.
Bugzilla: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1844962
Signed-off-by: Philip Yang <Philip.Yang@amd.com>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 2 +-
drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 23 +----------------------
2 files changed, 2 insertions(+), 23 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index 49b767b7238f..e861de259def 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -498,7 +498,7 @@ static int amdgpu_cs_list_validate(struct amdgpu_cs_parser *p,
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);
--
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] 8+ messages in thread[parent not found: <20191003190653.15455-1-Philip.Yang-5C7GfCeVMHo@public.gmane.org>]
* Re: [PATCH] drm/amdgpu: user pages array memory leak fix [not found] ` <20191003190653.15455-1-Philip.Yang-5C7GfCeVMHo@public.gmane.org> @ 2019-10-03 22:45 ` Joe Barnett 0 siblings, 0 replies; 8+ messages in thread From: Joe Barnett @ 2019-10-03 22:45 UTC (permalink / raw) To: Yang, Philip; +Cc: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW [-- Attachment #1.1: Type: text/plain, Size: 3320 bytes --] I've tested applying v2 of this patch against a v5.3 tagged kernel and it appears to fix the issue I reported. Thanks, -Joe On Thu, Oct 3, 2019 at 12:07 PM Yang, Philip <Philip.Yang-5C7GfCeVMHo@public.gmane.org> wrote: > user_pages array should be freed regardless if user pages are > invalidated after bo is created because HMM change to always allocate > user pages array to get user pages while parsing user page bo. > > Don't need to to get user pages while creating bo because user pages > will only be used after parsing user page bo. > > Bugzilla: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1844962 > > Signed-off-by: Philip Yang <Philip.Yang-5C7GfCeVMHo@public.gmane.org> > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 2 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 23 +---------------------- > 2 files changed, 2 insertions(+), 23 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c > index 49b767b7238f..e861de259def 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c > @@ -498,7 +498,7 @@ static int amdgpu_cs_list_validate(struct > amdgpu_cs_parser *p, > 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); > > -- > 2.17.1 > > [-- Attachment #1.2: Type: text/html, Size: 4435 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] 8+ messages in thread
* [PATCH] drm/amdgpu: user pages array memory leak fix
@ 2019-10-03 19:44 Yang, Philip
[not found] ` <20191003194423.14468-1-Philip.Yang-5C7GfCeVMHo@public.gmane.org>
0 siblings, 1 reply; 8+ messages in thread
From: Yang, Philip @ 2019-10-03 19:44 UTC (permalink / raw)
To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, thej^C; +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.
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 <Philip.Yang@amd.com>
---
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);
--
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] 8+ messages in thread[parent not found: <20191003194423.14468-1-Philip.Yang-5C7GfCeVMHo@public.gmane.org>]
* Re: [PATCH] drm/amdgpu: user pages array memory leak fix [not found] ` <20191003194423.14468-1-Philip.Yang-5C7GfCeVMHo@public.gmane.org> @ 2019-10-04 13:40 ` Yang, Philip [not found] ` <e86d92cd-7bae-6704-00db-1a79ccbb1011-5C7GfCeVMHo@public.gmane.org> 2019-10-11 8:40 ` Christian König 1 sibling, 1 reply; 8+ messages in thread From: Yang, Philip @ 2019-10-04 13:40 UTC (permalink / raw) To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, thejoe-Re5JQEeQqe8AvxtiuMwx3w, Koenig, Christian 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. 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 <Philip.Yang@amd.com> > --- > 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); > > _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 8+ messages in thread
[parent not found: <e86d92cd-7bae-6704-00db-1a79ccbb1011-5C7GfCeVMHo@public.gmane.org>]
* Re: [PATCH] drm/amdgpu: user pages array memory leak fix [not found] ` <e86d92cd-7bae-6704-00db-1a79ccbb1011-5C7GfCeVMHo@public.gmane.org> @ 2019-10-04 15:02 ` Koenig, Christian [not found] ` <6640c636-79d1-6159-a1db-5f39f70096a1-5C7GfCeVMHo@public.gmane.org> 0 siblings, 1 reply; 8+ messages in thread From: Koenig, Christian @ 2019-10-04 15:02 UTC (permalink / raw) To: Yang, Philip, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, thejoe-Re5JQEeQqe8AvxtiuMwx3w 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 <Philip.Yang@amd.com> >> --- >> 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); >> >> _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 8+ messages in thread
[parent not found: <6640c636-79d1-6159-a1db-5f39f70096a1-5C7GfCeVMHo@public.gmane.org>]
* Re: [PATCH] drm/amdgpu: user pages array memory leak fix [not found] ` <6640c636-79d1-6159-a1db-5f39f70096a1-5C7GfCeVMHo@public.gmane.org> @ 2019-10-10 15:37 ` Joe Barnett 0 siblings, 0 replies; 8+ messages in thread From: Joe Barnett @ 2019-10-10 15:37 UTC (permalink / raw) To: Koenig, Christian; +Cc: Yang, Philip, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW [-- Attachment #1.1: Type: text/plain, Size: 5251 bytes --] 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 <Christian.Koenig-5C7GfCeVMHo@public.gmane.org> 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 <Philip.Yang-5C7GfCeVMHo@public.gmane.org> > >> --- > >> 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); > >> > >> > > [-- Attachment #1.2: Type: text/html, Size: 7283 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] 8+ messages in thread
* Re: [PATCH] drm/amdgpu: user pages array memory leak fix [not found] ` <20191003194423.14468-1-Philip.Yang-5C7GfCeVMHo@public.gmane.org> 2019-10-04 13:40 ` Yang, Philip @ 2019-10-11 8:40 ` Christian König [not found] ` <af664c43-a81b-bae4-8f6f-c70bc4ab9de5-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 1 sibling, 1 reply; 8+ messages in thread From: Christian König @ 2019-10-11 8:40 UTC (permalink / raw) To: Yang, Philip, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, thej^C Am 03.10.19 um 21:44 schrieb 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. > > 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 <Philip.Yang@amd.com> > --- > 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) { We can't drop that handling here, it is mandatory to detect an application bug where an application tries to user an userptr with a VMA which is not anonymous memory. This must be detected and rejected as invalid here. I suggest that we allocate a local pages array similar to how we do it during CS and release that after the function is done. Regards, Christian. > - 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); > _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 8+ messages in thread
[parent not found: <af664c43-a81b-bae4-8f6f-c70bc4ab9de5-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]
* Re: [PATCH] drm/amdgpu: user pages array memory leak fix [not found] ` <af664c43-a81b-bae4-8f6f-c70bc4ab9de5-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> @ 2019-10-11 14:33 ` Yang, Philip 0 siblings, 0 replies; 8+ messages in thread From: Yang, Philip @ 2019-10-11 14:33 UTC (permalink / raw) To: Koenig, Christian, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, thej^C On 2019-10-11 4:40 a.m., Christian König wrote: > Am 03.10.19 um 21:44 schrieb 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. >> >> 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 <Philip.Yang@amd.com> >> --- >> 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) { > > We can't drop that handling here, it is mandatory to detect an > application bug where an application tries to user an userptr with a VMA > which is not anonymous memory. > > This must be detected and rejected as invalid here. > > I suggest that we allocate a local pages array similar to how we do it > during CS and release that after the function is done. > Thanks for this, we can use bo->tbo.ttm->pages array here to avoid extra alloc/free of pages array because CS uses local pages array and update to bo->tbo.ttm->pages if user pages are moved. I will submit patch v3 for review. > Regards, > Christian. > >> - 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); > _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-10-11 14:33 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-03 19:07 [PATCH] drm/amdgpu: user pages array memory leak fix Yang, Philip
[not found] ` <20191003190653.15455-1-Philip.Yang-5C7GfCeVMHo@public.gmane.org>
2019-10-03 22:45 ` Joe Barnett
2019-10-03 19:44 Yang, Philip
[not found] ` <20191003194423.14468-1-Philip.Yang-5C7GfCeVMHo@public.gmane.org>
2019-10-04 13:40 ` Yang, Philip
[not found] ` <e86d92cd-7bae-6704-00db-1a79ccbb1011-5C7GfCeVMHo@public.gmane.org>
2019-10-04 15:02 ` Koenig, Christian
[not found] ` <6640c636-79d1-6159-a1db-5f39f70096a1-5C7GfCeVMHo@public.gmane.org>
2019-10-10 15:37 ` Joe Barnett
2019-10-11 8:40 ` Christian König
[not found] ` <af664c43-a81b-bae4-8f6f-c70bc4ab9de5-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2019-10-11 14:33 ` Yang, Philip
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.