<div dir="ltr"><div>Thanks for looking at this Christian. Let me know if there's anything I can do to help. </div><div> </div><div>In the meantime, is there a workaround to avoid the memory leak other than using a kernel from before the HMM change?</div><div> </div><div>Thanks,</div><div>-Joe </div></div> <div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Oct 4, 2019 at 8:02 AM Koenig, Christian <<a href="mailto:Christian.Koenig@amd.com">Christian.Koenig@amd.com</a>> wrote: </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">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: <a href="https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1844962" rel="noreferrer" target="_blank">https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1844962</a> >> >> v2: remove unused local variable and amend commit >> >> Signed-off-by: Philip Yang <<a href="mailto:Philip.Yang@amd.com" target="_blank">Philip.Yang@amd.com</a>> >> --- >> 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-><a href="http://tv.bo" rel="noreferrer" target="_blank">tv.bo</a>); >> - 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); >> >> </blockquote></div>