Re: [PATCH] drm/atomic: Fix memleak on ERESTARTSYS during non-blocking commits

From: Daniel Vetter <daniel@ffwll.ch>
To: Alex Deucher <alexdeucher@gmail.com>
Cc: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>,
	Maling list - DRI developers <dri-devel@lists.freedesktop.org>
Subject: Re: [PATCH] drm/atomic: Fix memleak on ERESTARTSYS during non-blocking commits
Date: Wed, 10 Jan 2018 10:27:36 +0100	[thread overview]
Message-ID: <20180110092736.GH13066@phenom.ffwll.local> (raw)
In-Reply-To: <CADnq5_OATUZPsDZXdS5YtTj8Q6BzVrmbAV9GwqQzrYEURUjoJw@mail.gmail.com>

On Tue, Jan 09, 2018 at 01:13:33PM -0500, Alex Deucher wrote:
> On Mon, Jan 8, 2018 at 4:30 PM, Harry Wentland <harry.wentland@amd.com> wrote:
> > On 2018-01-04 02:47 PM, sunpeng.li@amd.com wrote:
> >> From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>
> >>
> >> During a non-blocking commit, it is possible to return before the
> >> commit_tail work is queued (-ERESTARTSYS, for example).
> >>
> >> Since a reference on the crtc commit object is obtained for the pending
> >> vblank event when preparing the commit, the above situation will leave
> >> us with an extra reference.
> >>
> >> Therefore, if the commit_tail worker has not consumed the event at the
> >> end of a commit, release it's reference.
> >>
> >> Signed-off-by: Leo (Sunpeng) Li <sunpeng.li@amd.com>
> >
> > No expert on this but looks sane to me.
> >
> > Acked-by: Harry Wentland <harry.wentland@amd.com>
> 
> 
> Pushed to drm-misc-next.

This blows up all over the place in our CI:

https://bugs.freedesktop.org/show_bug.cgi?id=104566

I think as a leak fix it should also be pushed to -fixes. And we didn't
really figure out what's wrong, so we're going with the quick revert until
you folks are all awake.

Cheers, Daniel

> 
> Thanks,
> 
> Alex
> 
> >
> > Harry
> >
> >> ---
> >>  drivers/gpu/drm/drm_atomic_helper.c | 9 +++++++++
> >>  1 file changed, 9 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> >> index ab40321..4253f57 100644
> >> --- a/drivers/gpu/drm/drm_atomic_helper.c
> >> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> >> @@ -3421,6 +3421,15 @@ EXPORT_SYMBOL(drm_atomic_helper_crtc_duplicate_state);
> >>  void __drm_atomic_helper_crtc_destroy_state(struct drm_crtc_state *state)
> >>  {
> >>       if (state->commit) {
> >> +             /*
> >> +              * In the event that a non-blocking commit returns
> >> +              * -ERESTARTSYS before the commit_tail work is queued, we will
> >> +              * have an extra reference to the commit object. Release it, if
> >> +              * the event has not been consumed by the worker.
> >> +              */
> >> +             if (state->event)
> >> +                     drm_crtc_commit_put(state->commit);
> >> +
> >>               kfree(state->commit->event);
> >>               state->commit->event = NULL;
> >>               drm_crtc_commit_put(state->commit);
> >>
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel