[PATCH] drm/amdgpu: user pages array memory leak fix

[Joe Barnett]

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 at amd.com>
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 at 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);
> >>
> >>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20191010/283a5a71/attachment.html>