[PATCH] drm/i915: Only free the unpin_work if cancelled before being run

[PATCH] drm/i915: Only free the unpin_work if cancelled before being run

[Chris Wilson]

The unpin worker frees it work struct and so during intel_crtc_disable
we should only also free the work struct if cancel_work_sync() reports
that it successfully cancelled the work prior to it being executed and
thus avoid the double free.

The impact is only for people unloading modules during a fullscreen game
or movie playback, so extremely small.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_display.c |    4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8298b72..78390e8 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7602,10 +7602,8 @@ static void intel_crtc_destroy(struct drm_crtc *crtc)
 	intel_crtc->unpin_work = NULL;
 	spin_unlock_irqrestore(&dev->event_lock, flags);
 
-	if (work) {
-		cancel_work_sync(&work->work);
+	if (work && cancel_work_sync(&work->work))
 		kfree(work);
-	}
 
 	drm_crtc_cleanup(crtc);
 
-- 
1.7.10

Re: [PATCH] drm/i915: Only free the unpin_work if cancelled before being run

[Daniel Vetter]

On Tue, Apr 17, 2012 at 10:29:38AM +0100, Chris Wilson wrote:
> The unpin worker frees it work struct and so during intel_crtc_disable
s/disable/destroy/

> we should only also free the work struct if cancel_work_sync() reports
> that it successfully cancelled the work prior to it being executed and
> thus avoid the double free.
> 
> The impact is only for people unloading modules during a fullscreen game
> or movie playback, so extremely small.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/i915/intel_display.c |    4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 8298b72..78390e8 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -7602,10 +7602,8 @@ static void intel_crtc_destroy(struct drm_crtc *crtc)
>  	intel_crtc->unpin_work = NULL;
>  	spin_unlock_irqrestore(&dev->event_lock, flags);
>  
> -	if (work) {
> -		cancel_work_sync(&work->work);
> +	if (work && cancel_work_sync(&work->work))
>  		kfree(work);
> -	}
>  
>  	drm_crtc_cleanup(crtc);
>  
> -- 
> 1.7.10
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH] drm/i915: Only free the unpin_work if cancelled before being run

[Chris Wilson]

On Tue, 17 Apr 2012 10:29:38 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> The unpin worker frees it work struct and so during intel_crtc_disable
> we should only also free the work struct if cancel_work_sync() reports
> that it successfully cancelled the work prior to it being executed and
> thus avoid the double free.
> 
> The impact is only for people unloading modules during a fullscreen game
> or movie playback, so extremely small.

Futher review (hunting for some sign of workqueue corruption, cf
https://bugs.freedesktop.org/show_bug.cgi?id=48798) says that if work is
non-NULL here it will not have been scheduled so cancel_work_sync() will
always return true.

Which also means that we have no way of waiting upon the scheduled
unpin_work. :|
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Only free the unpin_work if cancelled before being run

[Daniel Vetter]

On Tue, Apr 17, 2012 at 10:53:24AM +0100, Chris Wilson wrote:
> On Tue, 17 Apr 2012 10:29:38 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > The unpin worker frees it work struct and so during intel_crtc_disable
> > we should only also free the work struct if cancel_work_sync() reports
> > that it successfully cancelled the work prior to it being executed and
> > thus avoid the double free.
> > 
> > The impact is only for people unloading modules during a fullscreen game
> > or movie playback, so extremely small.
> 
> Futher review (hunting for some sign of workqueue corruption, cf
> https://bugs.freedesktop.org/show_bug.cgi?id=48798) says that if work is
> non-NULL here it will not have been scheduled so cancel_work_sync() will
> always return true.

Well, I've failed to notice this while reviewing the unpin_work life-cycle
...

> Which also means that we have no way of waiting upon the scheduled
> unpin_work. :|

... but have noticed that we lose any reference from intel_crtc to the
unpin work once it's scheduled, and checked that we properly flush the
work queue: See the ordering of irq disable, flush workqueue, then crtc
destroy (in mode_config_cleanup).

We even have a flush_workqueu in i915_dma.c before tearing down gem, but
that won't work too well now that unpin also frobs around with the fbc
state (which is gone by now). But there's still the misleading comment
that this syncs up with unpin work.

Care to clean this up a bit by
- ditching the unnecessary flush_workqueu in i915_dma.c
- move the comment about syncing up with unpin_work to where we actually
  sync up
- and kill the superflous cancel_work?

Cheers, Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH] drm/i915: Only free the unpin_work if cancelled before being run

[Chris Wilson]

On Tue, 17 Apr 2012 12:33:46 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, Apr 17, 2012 at 10:53:24AM +0100, Chris Wilson wrote:
> > On Tue, 17 Apr 2012 10:29:38 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > > The unpin worker frees it work struct and so during intel_crtc_disable
> > > we should only also free the work struct if cancel_work_sync() reports
> > > that it successfully cancelled the work prior to it being executed and
> > > thus avoid the double free.
> > > 
> > > The impact is only for people unloading modules during a fullscreen game
> > > or movie playback, so extremely small.
> > 
> > Futher review (hunting for some sign of workqueue corruption, cf
> > https://bugs.freedesktop.org/show_bug.cgi?id=48798) says that if work is
> > non-NULL here it will not have been scheduled so cancel_work_sync() will
> > always return true.
> 
> Well, I've failed to notice this while reviewing the unpin_work life-cycle
> ...
> 
> > Which also means that we have no way of waiting upon the scheduled
> > unpin_work. :|
> 
> ... but have noticed that we lose any reference from intel_crtc to the
> unpin work once it's scheduled, and checked that we properly flush the
> work queue: See the ordering of irq disable, flush workqueue, then crtc
> destroy (in mode_config_cleanup).
> 
> We even have a flush_workqueu in i915_dma.c before tearing down gem, but
> that won't work too well now that unpin also frobs around with the fbc
> state (which is gone by now). But there's still the misleading comment
> that this syncs up with unpin work.
> 
> Care to clean this up a bit by
> - ditching the unnecessary flush_workqueu in i915_dma.c
Indeed looks superfluous. If we consider the unlikelihood the cleanup
code being well-tested, I'd prefer that we did do something like
drain_workqueue() as the first step in unload().

> - move the comment about syncing up with unpin_work to where we actually
>   sync up

I'm not finding another point where we explicitly sync with outstanding
unpin work. Probably due to the comment being in the wrong place...

> - and kill the superflous cancel_work?
Which also reminds me that the unpin_work holds onto a few references
that need to be released as well as the kfree. See above about these code
paths being relatively untrod.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Only free the unpin_work if cancelled before being run

[Daniel Vetter]

On Tue, Apr 17, 2012 at 02:30:35PM +0100, Chris Wilson wrote:
> On Tue, 17 Apr 2012 12:33:46 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Tue, Apr 17, 2012 at 10:53:24AM +0100, Chris Wilson wrote:
> > > On Tue, 17 Apr 2012 10:29:38 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > > > The unpin worker frees it work struct and so during intel_crtc_disable
> > > > we should only also free the work struct if cancel_work_sync() reports
> > > > that it successfully cancelled the work prior to it being executed and
> > > > thus avoid the double free.
> > > > 
> > > > The impact is only for people unloading modules during a fullscreen game
> > > > or movie playback, so extremely small.
> > > 
> > > Futher review (hunting for some sign of workqueue corruption, cf
> > > https://bugs.freedesktop.org/show_bug.cgi?id=48798) says that if work is
> > > non-NULL here it will not have been scheduled so cancel_work_sync() will
> > > always return true.
> > 
> > Well, I've failed to notice this while reviewing the unpin_work life-cycle
> > ...
> > 
> > > Which also means that we have no way of waiting upon the scheduled
> > > unpin_work. :|
> > 
> > ... but have noticed that we lose any reference from intel_crtc to the
> > unpin work once it's scheduled, and checked that we properly flush the
> > work queue: See the ordering of irq disable, flush workqueue, then crtc
> > destroy (in mode_config_cleanup).
> > 
> > We even have a flush_workqueu in i915_dma.c before tearing down gem, but
> > that won't work too well now that unpin also frobs around with the fbc
> > state (which is gone by now). But there's still the misleading comment
> > that this syncs up with unpin work.
> > 
> > Care to clean this up a bit by
> > - ditching the unnecessary flush_workqueu in i915_dma.c
> Indeed looks superfluous. If we consider the unlikelihood the cleanup
> code being well-tested, I'd prefer that we did do something like
> drain_workqueue() as the first step in unload().

Sounds good.

> > - move the comment about syncing up with unpin_work to where we actually
> >   sync up
> 
> I'm not finding another point where we explicitly sync with outstanding
> unpin work. Probably due to the comment being in the wrong place...

Well, I've just noticed that we call flush_scheduled_work instead of
flush_workqueue (i.e. the global one instead of our own), but we also put
the unpin work onto the global queue with schedule_work instead of our own
with queue_work

> > - and kill the superflous cancel_work?
> Which also reminds me that the unpin_work holds onto a few references
> that need to be released as well as the kfree. See above about these code
> paths being relatively untrod.

Oops. The problem is that unpin_work also calls the fbc update, so we need
to ensure that this is done before the fbc cleanup happened. Fun.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH] drm/i915: Only free the unpin_work if cancelled before being run

[Daniel Vetter]

On Tue, Apr 17, 2012 at 03:44:01PM +0200, Daniel Vetter wrote:
> On Tue, Apr 17, 2012 at 02:30:35PM +0100, Chris Wilson wrote:
> > On Tue, 17 Apr 2012 12:33:46 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> > > On Tue, Apr 17, 2012 at 10:53:24AM +0100, Chris Wilson wrote:
> > > > On Tue, 17 Apr 2012 10:29:38 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > > > > The unpin worker frees it work struct and so during intel_crtc_disable
> > > > > we should only also free the work struct if cancel_work_sync() reports
> > > > > that it successfully cancelled the work prior to it being executed and
> > > > > thus avoid the double free.
> > > > > 
> > > > > The impact is only for people unloading modules during a fullscreen game
> > > > > or movie playback, so extremely small.
> > > > 
> > > > Futher review (hunting for some sign of workqueue corruption, cf
> > > > https://bugs.freedesktop.org/show_bug.cgi?id=48798) says that if work is
> > > > non-NULL here it will not have been scheduled so cancel_work_sync() will
> > > > always return true.
> > > 
> > > Well, I've failed to notice this while reviewing the unpin_work life-cycle
> > > ...
> > > 
> > > > Which also means that we have no way of waiting upon the scheduled
> > > > unpin_work. :|
> > > 
> > > ... but have noticed that we lose any reference from intel_crtc to the
> > > unpin work once it's scheduled, and checked that we properly flush the
> > > work queue: See the ordering of irq disable, flush workqueue, then crtc
> > > destroy (in mode_config_cleanup).
> > > 
> > > We even have a flush_workqueu in i915_dma.c before tearing down gem, but
> > > that won't work too well now that unpin also frobs around with the fbc
> > > state (which is gone by now). But there's still the misleading comment
> > > that this syncs up with unpin work.
> > > 
> > > Care to clean this up a bit by
> > > - ditching the unnecessary flush_workqueu in i915_dma.c
> > Indeed looks superfluous. If we consider the unlikelihood the cleanup
> > code being well-tested, I'd prefer that we did do something like
> > drain_workqueue() as the first step in unload().
> 
> Sounds good.
> 
> > > - move the comment about syncing up with unpin_work to where we actually
> > >   sync up
> > 
> > I'm not finding another point where we explicitly sync with outstanding
> > unpin work. Probably due to the comment being in the wrong place...
> 
> Well, I've just noticed that we call flush_scheduled_work instead of
> flush_workqueue (i.e. the global one instead of our own), but we also put
> the unpin work onto the global queue with schedule_work instead of our own
> with queue_work
> 
> > > - and kill the superflous cancel_work?
> > Which also reminds me that the unpin_work holds onto a few references
> > that need to be released as well as the kfree. See above about these code
> > paths being relatively untrod.
> 
> Oops. The problem is that unpin_work also calls the fbc update, so we need
> to ensure that this is done before the fbc cleanup happened. Fun.

While we bitch around about this code: unpin_work is a bit misleading
given the fbc frobbing. So maybe we should call it finish_pageflip_work or
something like that ...
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48