* [PATCH] drm/i915: Prevent signals from interrupting close() @ 2014-04-09 7:03 Chris Wilson 2014-04-09 14:49 ` Daniel Vetter 2014-04-09 17:43 ` Ben Widawsky 0 siblings, 2 replies; 8+ messages in thread From: Chris Wilson @ 2014-04-09 7:03 UTC (permalink / raw) To: intel-gfx; +Cc: Ben Widawsky We neither report any unfinished operations during releasing GEM objects associated with the file, and even if we did, it is bad form to report -EINTR from a close(). The root cause of the bug that first showed itself during close is that we do not do proper live tracking of vma and contexts under full-ppgtt, but this is useful piece of defensive programming enforcing our userspace API contract. Cc: Ben Widawsky <benjamin.widawsky@intel.com> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/i915_dma.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index 24dd55a16436..d67ca8051e07 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -1937,9 +1937,18 @@ void i915_driver_lastclose(struct drm_device * dev) void i915_driver_preclose(struct drm_device * dev, struct drm_file *file_priv) { + struct drm_i915_private *dev_priv = to_i915(dev); + bool was_interruptible; + mutex_lock(&dev->struct_mutex); + was_interruptible = dev_priv->mm.interruptible; + WARN_ON(!was_interruptible); + dev_priv->mm.interruptible = false; + i915_gem_context_close(dev, file_priv); i915_gem_release(dev, file_priv); + + dev_priv->mm.interruptible = was_interruptible; mutex_unlock(&dev->struct_mutex); } -- 1.9.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/i915: Prevent signals from interrupting close() 2014-04-09 7:03 [PATCH] drm/i915: Prevent signals from interrupting close() Chris Wilson @ 2014-04-09 14:49 ` Daniel Vetter 2014-04-09 15:03 ` Chris Wilson 2014-04-09 17:43 ` Ben Widawsky 1 sibling, 1 reply; 8+ messages in thread From: Daniel Vetter @ 2014-04-09 14:49 UTC (permalink / raw) To: Chris Wilson; +Cc: intel-gfx, Ben Widawsky On Wed, Apr 09, 2014 at 08:03:39AM +0100, Chris Wilson wrote: > We neither report any unfinished operations during releasing GEM objects > associated with the file, and even if we did, it is bad form to report > -EINTR from a close(). > > The root cause of the bug that first showed itself during close is that > we do not do proper live tracking of vma and contexts under full-ppgtt, > but this is useful piece of defensive programming enforcing our > userspace API contract. > > Cc: Ben Widawsky <benjamin.widawsky@intel.com> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> I'd really prefer something annoying and loud like we've done when nuking the deferred free list in commit 1488fc08c1706288616c602416654fd38c773deb Author: Chris Wilson <chris@chris-wilson.co.uk> Date: Tue Apr 24 15:47:31 2012 +0100 drm/i915: Remove the deferred-free list where we've added a WARN_ON in gem_free_object if any unbind was failing due to interrupts. This patch here disables that imo useful safety check. -Daniel > --- > drivers/gpu/drm/i915/i915_dma.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c > index 24dd55a16436..d67ca8051e07 100644 > --- a/drivers/gpu/drm/i915/i915_dma.c > +++ b/drivers/gpu/drm/i915/i915_dma.c > @@ -1937,9 +1937,18 @@ void i915_driver_lastclose(struct drm_device * dev) > > void i915_driver_preclose(struct drm_device * dev, struct drm_file *file_priv) > { > + struct drm_i915_private *dev_priv = to_i915(dev); > + bool was_interruptible; > + > mutex_lock(&dev->struct_mutex); > + was_interruptible = dev_priv->mm.interruptible; > + WARN_ON(!was_interruptible); > + dev_priv->mm.interruptible = false; > + > i915_gem_context_close(dev, file_priv); > i915_gem_release(dev, file_priv); > + > + dev_priv->mm.interruptible = was_interruptible; > mutex_unlock(&dev->struct_mutex); > } > > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/i915: Prevent signals from interrupting close() 2014-04-09 14:49 ` Daniel Vetter @ 2014-04-09 15:03 ` Chris Wilson 2014-04-09 16:50 ` Daniel Vetter 0 siblings, 1 reply; 8+ messages in thread From: Chris Wilson @ 2014-04-09 15:03 UTC (permalink / raw) To: Daniel Vetter; +Cc: intel-gfx, Ben Widawsky On Wed, Apr 09, 2014 at 04:49:02PM +0200, Daniel Vetter wrote: > On Wed, Apr 09, 2014 at 08:03:39AM +0100, Chris Wilson wrote: > > We neither report any unfinished operations during releasing GEM objects > > associated with the file, and even if we did, it is bad form to report > > -EINTR from a close(). > > > > The root cause of the bug that first showed itself during close is that > > we do not do proper live tracking of vma and contexts under full-ppgtt, > > but this is useful piece of defensive programming enforcing our > > userspace API contract. > > > > Cc: Ben Widawsky <benjamin.widawsky@intel.com> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > I'd really prefer something annoying and loud like we've done when nuking > the deferred free list in > > commit 1488fc08c1706288616c602416654fd38c773deb > Author: Chris Wilson <chris@chris-wilson.co.uk> > Date: Tue Apr 24 15:47:31 2012 +0100 > > drm/i915: Remove the deferred-free list > > where we've added a WARN_ON in gem_free_object if any unbind was failing > due to interrupts. This patch here disables that imo useful safety check. It doesn't classify as a safety check if kernel/userspace explodes. The trick would be to somehow get the error code back. And in this case we cannot accept any error anyway. So yes, I would like a nice big warning to catch bugs, but that is icing on the cake compared to preventing bugs from destroying data. -Chris -- Chris Wilson, Intel Open Source Technology Centre ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/i915: Prevent signals from interrupting close() 2014-04-09 15:03 ` Chris Wilson @ 2014-04-09 16:50 ` Daniel Vetter 2014-04-09 16:57 ` Chris Wilson 0 siblings, 1 reply; 8+ messages in thread From: Daniel Vetter @ 2014-04-09 16:50 UTC (permalink / raw) To: Chris Wilson, Daniel Vetter, intel-gfx, Ben Widawsky On Wed, Apr 09, 2014 at 04:03:23PM +0100, Chris Wilson wrote: > On Wed, Apr 09, 2014 at 04:49:02PM +0200, Daniel Vetter wrote: > > On Wed, Apr 09, 2014 at 08:03:39AM +0100, Chris Wilson wrote: > > > We neither report any unfinished operations during releasing GEM objects > > > associated with the file, and even if we did, it is bad form to report > > > -EINTR from a close(). > > > > > > The root cause of the bug that first showed itself during close is that > > > we do not do proper live tracking of vma and contexts under full-ppgtt, > > > but this is useful piece of defensive programming enforcing our > > > userspace API contract. > > > > > > Cc: Ben Widawsky <benjamin.widawsky@intel.com> > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > > > I'd really prefer something annoying and loud like we've done when nuking > > the deferred free list in > > > > commit 1488fc08c1706288616c602416654fd38c773deb > > Author: Chris Wilson <chris@chris-wilson.co.uk> > > Date: Tue Apr 24 15:47:31 2012 +0100 > > > > drm/i915: Remove the deferred-free list > > > > where we've added a WARN_ON in gem_free_object if any unbind was failing > > due to interrupts. This patch here disables that imo useful safety check. > > It doesn't classify as a safety check if kernel/userspace explodes. The > trick would be to somehow get the error code back. And in this case we > cannot accept any error anyway. So yes, I would like a nice big warning > to catch bugs, but that is icing on the cake compared to preventing bugs > from destroying data. Hm ... double-checking: This is just for full ppgtt, right? I think in that case I prefer if people working on it just carry this around locally. Otherwise I need to freak out ;-) -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/i915: Prevent signals from interrupting close() 2014-04-09 16:50 ` Daniel Vetter @ 2014-04-09 16:57 ` Chris Wilson 0 siblings, 0 replies; 8+ messages in thread From: Chris Wilson @ 2014-04-09 16:57 UTC (permalink / raw) To: Daniel Vetter; +Cc: intel-gfx, Ben Widawsky On Wed, Apr 09, 2014 at 06:50:03PM +0200, Daniel Vetter wrote: > On Wed, Apr 09, 2014 at 04:03:23PM +0100, Chris Wilson wrote: > > On Wed, Apr 09, 2014 at 04:49:02PM +0200, Daniel Vetter wrote: > > > On Wed, Apr 09, 2014 at 08:03:39AM +0100, Chris Wilson wrote: > > > > We neither report any unfinished operations during releasing GEM objects > > > > associated with the file, and even if we did, it is bad form to report > > > > -EINTR from a close(). > > > > > > > > The root cause of the bug that first showed itself during close is that > > > > we do not do proper live tracking of vma and contexts under full-ppgtt, > > > > but this is useful piece of defensive programming enforcing our > > > > userspace API contract. > > > > > > > > Cc: Ben Widawsky <benjamin.widawsky@intel.com> > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > > > > > I'd really prefer something annoying and loud like we've done when nuking > > > the deferred free list in > > > > > > commit 1488fc08c1706288616c602416654fd38c773deb > > > Author: Chris Wilson <chris@chris-wilson.co.uk> > > > Date: Tue Apr 24 15:47:31 2012 +0100 > > > > > > drm/i915: Remove the deferred-free list > > > > > > where we've added a WARN_ON in gem_free_object if any unbind was failing > > > due to interrupts. This patch here disables that imo useful safety check. > > > > It doesn't classify as a safety check if kernel/userspace explodes. The > > trick would be to somehow get the error code back. And in this case we > > cannot accept any error anyway. So yes, I would like a nice big warning > > to catch bugs, but that is icing on the cake compared to preventing bugs > > from destroying data. > > Hm ... double-checking: This is just for full ppgtt, right? I think in > that case I prefer if people working on it just carry this around locally. > Otherwise I need to freak out ;-) The current bug under discussion is full-ppgtt (well, hopefully). I'd prefer it if we could prevent driver bugs leaking out into userspace... Pretty much everywhere we could hit an error but can't report one is simmering trouble. (And to continue playing the fiddle, and when the driver does explode, I expect to be able to continue using all the memory management and modesetting intefaces.) -Chris -- Chris Wilson, Intel Open Source Technology Centre ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/i915: Prevent signals from interrupting close() 2014-04-09 7:03 [PATCH] drm/i915: Prevent signals from interrupting close() Chris Wilson 2014-04-09 14:49 ` Daniel Vetter @ 2014-04-09 17:43 ` Ben Widawsky 2014-04-09 17:58 ` Chris Wilson 1 sibling, 1 reply; 8+ messages in thread From: Ben Widawsky @ 2014-04-09 17:43 UTC (permalink / raw) To: Chris Wilson; +Cc: intel-gfx On Wed, Apr 09, 2014 at 08:03:39AM +0100, Chris Wilson wrote: > We neither report any unfinished operations during releasing GEM objects > associated with the file, and even if we did, it is bad form to report > -EINTR from a close(). > > The root cause of the bug that first showed itself during close is that > we do not do proper live tracking of vma and contexts under full-ppgtt, > but this is useful piece of defensive programming enforcing our > userspace API contract. > > Cc: Ben Widawsky <benjamin.widawsky@intel.com> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/i915_dma.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c > index 24dd55a16436..d67ca8051e07 100644 > --- a/drivers/gpu/drm/i915/i915_dma.c > +++ b/drivers/gpu/drm/i915/i915_dma.c > @@ -1937,9 +1937,18 @@ void i915_driver_lastclose(struct drm_device * dev) > > void i915_driver_preclose(struct drm_device * dev, struct drm_file *file_priv) > { > + struct drm_i915_private *dev_priv = to_i915(dev); > + bool was_interruptible; > + > mutex_lock(&dev->struct_mutex); > + was_interruptible = dev_priv->mm.interruptible; > + WARN_ON(!was_interruptible); > + dev_priv->mm.interruptible = false; > + > i915_gem_context_close(dev, file_priv); > i915_gem_release(dev, file_priv); > + > + dev_priv->mm.interruptible = was_interruptible; > mutex_unlock(&dev->struct_mutex); > } > I guess you missed: 1396905423-19453-1-git-send-email-benjamin.widawsky@intel.com True in my case, I should have put the read of 'dev_priv->mm.interruptible' within the lock. I don't think we need to protect gem_release. -- Ben Widawsky, Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/i915: Prevent signals from interrupting close() 2014-04-09 17:43 ` Ben Widawsky @ 2014-04-09 17:58 ` Chris Wilson 2014-04-11 6:44 ` Ben Widawsky 0 siblings, 1 reply; 8+ messages in thread From: Chris Wilson @ 2014-04-09 17:58 UTC (permalink / raw) To: Ben Widawsky; +Cc: intel-gfx On Wed, Apr 09, 2014 at 10:43:47AM -0700, Ben Widawsky wrote: > On Wed, Apr 09, 2014 at 08:03:39AM +0100, Chris Wilson wrote: > > We neither report any unfinished operations during releasing GEM objects > > associated with the file, and even if we did, it is bad form to report > > -EINTR from a close(). > > > > The root cause of the bug that first showed itself during close is that > > we do not do proper live tracking of vma and contexts under full-ppgtt, > > but this is useful piece of defensive programming enforcing our > > userspace API contract. > > > > Cc: Ben Widawsky <benjamin.widawsky@intel.com> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > --- > > drivers/gpu/drm/i915/i915_dma.c | 9 +++++++++ > > 1 file changed, 9 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c > > index 24dd55a16436..d67ca8051e07 100644 > > --- a/drivers/gpu/drm/i915/i915_dma.c > > +++ b/drivers/gpu/drm/i915/i915_dma.c > > @@ -1937,9 +1937,18 @@ void i915_driver_lastclose(struct drm_device * dev) > > > > void i915_driver_preclose(struct drm_device * dev, struct drm_file *file_priv) > > { > > + struct drm_i915_private *dev_priv = to_i915(dev); > > + bool was_interruptible; > > + > > mutex_lock(&dev->struct_mutex); > > + was_interruptible = dev_priv->mm.interruptible; > > + WARN_ON(!was_interruptible); > > + dev_priv->mm.interruptible = false; > > + > > i915_gem_context_close(dev, file_priv); > > i915_gem_release(dev, file_priv); > > + > > + dev_priv->mm.interruptible = was_interruptible; > > mutex_unlock(&dev->struct_mutex); > > } > > > > I guess you missed: > 1396905423-19453-1-git-send-email-benjamin.widawsky@intel.com Oops, I did. > True in my case, I should have put the read of > 'dev_priv->mm.interruptible' within the lock. > > I don't think we need to protect gem_release. My argument is that I want to protect the entire preclose() as it cannot be allowed to fail, i.e. all future bugs. -Chris -- Chris Wilson, Intel Open Source Technology Centre ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/i915: Prevent signals from interrupting close() 2014-04-09 17:58 ` Chris Wilson @ 2014-04-11 6:44 ` Ben Widawsky 0 siblings, 0 replies; 8+ messages in thread From: Ben Widawsky @ 2014-04-11 6:44 UTC (permalink / raw) To: Chris Wilson, Ben Widawsky, intel-gfx On Wed, Apr 09, 2014 at 06:58:41PM +0100, Chris Wilson wrote: > On Wed, Apr 09, 2014 at 10:43:47AM -0700, Ben Widawsky wrote: > > On Wed, Apr 09, 2014 at 08:03:39AM +0100, Chris Wilson wrote: > > > We neither report any unfinished operations during releasing GEM objects > > > associated with the file, and even if we did, it is bad form to report > > > -EINTR from a close(). > > > > > > The root cause of the bug that first showed itself during close is that > > > we do not do proper live tracking of vma and contexts under full-ppgtt, > > > but this is useful piece of defensive programming enforcing our > > > userspace API contract. > > > > > > Cc: Ben Widawsky <benjamin.widawsky@intel.com> > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > > --- > > > drivers/gpu/drm/i915/i915_dma.c | 9 +++++++++ > > > 1 file changed, 9 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c > > > index 24dd55a16436..d67ca8051e07 100644 > > > --- a/drivers/gpu/drm/i915/i915_dma.c > > > +++ b/drivers/gpu/drm/i915/i915_dma.c > > > @@ -1937,9 +1937,18 @@ void i915_driver_lastclose(struct drm_device * dev) > > > > > > void i915_driver_preclose(struct drm_device * dev, struct drm_file *file_priv) > > > { > > > + struct drm_i915_private *dev_priv = to_i915(dev); > > > + bool was_interruptible; > > > + > > > mutex_lock(&dev->struct_mutex); > > > + was_interruptible = dev_priv->mm.interruptible; > > > + WARN_ON(!was_interruptible); > > > + dev_priv->mm.interruptible = false; > > > + > > > i915_gem_context_close(dev, file_priv); > > > i915_gem_release(dev, file_priv); > > > + > > > + dev_priv->mm.interruptible = was_interruptible; > > > mutex_unlock(&dev->struct_mutex); > > > } > > > > > > > I guess you missed: > > 1396905423-19453-1-git-send-email-benjamin.widawsky@intel.com > > Oops, I did. > > > True in my case, I should have put the read of > > 'dev_priv->mm.interruptible' within the lock. > > > > I don't think we need to protect gem_release. > > My argument is that I want to protect the entire preclose() as it cannot > be allowed to fail, i.e. all future bugs. > -Chris > Reviewed-by: Ben Widawsky <ben@bwidawsk.net> (I haven't actually tested this patch, but it's similar enough to my patch that I think it could probably get a Tested-by too) -- Ben Widawsky, Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2014-04-11 6:44 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-04-09 7:03 [PATCH] drm/i915: Prevent signals from interrupting close() Chris Wilson 2014-04-09 14:49 ` Daniel Vetter 2014-04-09 15:03 ` Chris Wilson 2014-04-09 16:50 ` Daniel Vetter 2014-04-09 16:57 ` Chris Wilson 2014-04-09 17:43 ` Ben Widawsky 2014-04-09 17:58 ` Chris Wilson 2014-04-11 6:44 ` Ben Widawsky
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox