drm/atomic: Fix memleak on ERESTARTSYS during non-blocking commits
diff mbox

Message ID 1515095253-29817-1-git-send-email-sunpeng.li@amd.com
State New
Headers show

Commit Message

Leo Jan. 4, 2018, 7:47 p.m. UTC
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>
---
 drivers/gpu/drm/drm_atomic_helper.c | 9 +++++++++
 1 file changed, 9 insertions(+)

Comments

Harry Wentland Jan. 8, 2018, 9:30 p.m. UTC | #1
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>

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);
>
Alex Deucher Jan. 9, 2018, 6:13 p.m. UTC | #2
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.

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
Daniel Vetter Jan. 10, 2018, 9:27 a.m. UTC | #3
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
Maarten Lankhorst Jan. 10, 2018, 9:41 a.m. UTC | #4
Op 08-01-18 om 22:30 schreef Harry Wentland:
> 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>
>
> Harry
Hey,

I've reverted the patch for now. I planned to apply it to the drm-misc-fixes branch with the
appropriate commits referenced, but it is causing crashes on the kms_flip testcase.

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

I'll investigate it some more then send out a fixed patch. :)

~Maarten

Patch
diff mbox

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);