| Message ID | 1435672392-7329-7-git-send-email-przanoni@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jun 30, 2015 at 10:53:10AM -0300, Paulo Zanoni wrote: > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > Let's make sure the future Paulos don't forget that we need > struct_mutex when touching dev_priv->mm.stolen. > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > --- > drivers/gpu/drm/i915/i915_gem_stolen.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c > index 793bcba..cac1bce 100644 > --- a/drivers/gpu/drm/i915/i915_gem_stolen.c > +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c > @@ -160,6 +160,8 @@ static int find_compression_threshold(struct drm_device *dev, > int compression_threshold = 1; > int ret; > > + WARN_ON(!mutex_is_locked(&dev->struct_mutex)); I'm not a huge fan of vague mutex warnings that don't even check the owner. I'm espcially not a fan of adding a WARN and not handling the error. -Chris
2015-06-30 11:15 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>: > On Tue, Jun 30, 2015 at 10:53:10AM -0300, Paulo Zanoni wrote: >> From: Paulo Zanoni <paulo.r.zanoni@intel.com> >> >> Let's make sure the future Paulos don't forget that we need >> struct_mutex when touching dev_priv->mm.stolen. >> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> >> --- >> drivers/gpu/drm/i915/i915_gem_stolen.c | 13 +++++++++++++ >> 1 file changed, 13 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c >> index 793bcba..cac1bce 100644 >> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c >> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c >> @@ -160,6 +160,8 @@ static int find_compression_threshold(struct drm_device *dev, >> int compression_threshold = 1; >> int ret; >> >> + WARN_ON(!mutex_is_locked(&dev->struct_mutex)); > > I'm not a huge fan of vague mutex warnings that don't even check the owner. > I'm espcially not a fan of adding a WARN and not handling the error. But then, what exactly is your proposal? What would you like to see here? We can discard this patch if you want. But I hope you're not advocating for lockdep_assert_held(), because if I switch to lockdep, then Daniel is going to deny it again. Also, this type of WARN_ON is a common pattern on our codebase... > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre
On Tue, Jun 30, 2015 at 10:53:10AM -0300, Paulo Zanoni wrote: > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > Let's make sure the future Paulos don't forget that we need > struct_mutex when touching dev_priv->mm.stolen. As I elluded to in patch 5, I think the stolen warns are a misstep. -Chris
On Tue, Jun 30, 2015 at 11:26:11AM -0300, Paulo Zanoni wrote: > 2015-06-30 11:15 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>: > > On Tue, Jun 30, 2015 at 10:53:10AM -0300, Paulo Zanoni wrote: > >> From: Paulo Zanoni <paulo.r.zanoni@intel.com> > >> > >> Let's make sure the future Paulos don't forget that we need > >> struct_mutex when touching dev_priv->mm.stolen. > >> > >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > >> --- > >> drivers/gpu/drm/i915/i915_gem_stolen.c | 13 +++++++++++++ > >> 1 file changed, 13 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c > >> index 793bcba..cac1bce 100644 > >> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c > >> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c > >> @@ -160,6 +160,8 @@ static int find_compression_threshold(struct drm_device *dev, > >> int compression_threshold = 1; > >> int ret; > >> > >> + WARN_ON(!mutex_is_locked(&dev->struct_mutex)); > > > > I'm not a huge fan of vague mutex warnings that don't even check the owner. > > I'm espcially not a fan of adding a WARN and not handling the error. > > But then, what exactly is your proposal? What would you like to see here? > > We can discard this patch if you want. But I hope you're not > advocating for lockdep_assert_held(), because if I switch to lockdep, > then Daniel is going to deny it again. Also, this type of WARN_ON is a > common pattern on our codebase... I'm just trying to convince Daniel that blindly using this pattern is the wrong approach and encouraging a proliferation of unhandled WARN_ON doesn't improve driver robustness. -Chris
On 06/30/2015 07:36 AM, Chris Wilson wrote: > On Tue, Jun 30, 2015 at 11:26:11AM -0300, Paulo Zanoni wrote: >> 2015-06-30 11:15 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>: >>> On Tue, Jun 30, 2015 at 10:53:10AM -0300, Paulo Zanoni wrote: >>>> From: Paulo Zanoni <paulo.r.zanoni@intel.com> >>>> >>>> Let's make sure the future Paulos don't forget that we need >>>> struct_mutex when touching dev_priv->mm.stolen. >>>> >>>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> >>>> --- >>>> drivers/gpu/drm/i915/i915_gem_stolen.c | 13 +++++++++++++ >>>> 1 file changed, 13 insertions(+) >>>> >>>> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c >>>> index 793bcba..cac1bce 100644 >>>> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c >>>> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c >>>> @@ -160,6 +160,8 @@ static int find_compression_threshold(struct drm_device *dev, >>>> int compression_threshold = 1; >>>> int ret; >>>> >>>> + WARN_ON(!mutex_is_locked(&dev->struct_mutex)); >>> >>> I'm not a huge fan of vague mutex warnings that don't even check the owner. >>> I'm espcially not a fan of adding a WARN and not handling the error. >> >> But then, what exactly is your proposal? What would you like to see here? >> >> We can discard this patch if you want. But I hope you're not >> advocating for lockdep_assert_held(), because if I switch to lockdep, >> then Daniel is going to deny it again. Also, this type of WARN_ON is a >> common pattern on our codebase... > > I'm just trying to convince Daniel that blindly using this pattern is > the wrong approach and encouraging a proliferation of unhandled WARN_ON > doesn't improve driver robustness. I think they serve as useful documentation at the very least, whether in lockdep form, WARN form, or BUG form. It's not really something we can recover from either (maybe returning early before touching data?), so...
On Tue, Jun 30, 2015 at 01:30:27PM -0700, Jesse Barnes wrote: > I think they serve as useful documentation at the very least, whether in > lockdep form, WARN form, or BUG form. It's not really something we can > recover from either (maybe returning early before touching data?), so... As a debug aide they are less useful than lockdep and no more useful than lockdep for documentation purposes. But it feels like more and more of the WARN_ONs are instead of error handling. See the olr fiasco. -Chris
On Tue, Jun 30, 2015 at 01:30:27PM -0700, Jesse Barnes wrote: > On 06/30/2015 07:36 AM, Chris Wilson wrote: > > On Tue, Jun 30, 2015 at 11:26:11AM -0300, Paulo Zanoni wrote: > >> 2015-06-30 11:15 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>: > >>> On Tue, Jun 30, 2015 at 10:53:10AM -0300, Paulo Zanoni wrote: > >>>> From: Paulo Zanoni <paulo.r.zanoni@intel.com> > >>>> > >>>> Let's make sure the future Paulos don't forget that we need > >>>> struct_mutex when touching dev_priv->mm.stolen. > >>>> > >>>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > >>>> --- > >>>> drivers/gpu/drm/i915/i915_gem_stolen.c | 13 +++++++++++++ > >>>> 1 file changed, 13 insertions(+) > >>>> > >>>> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c > >>>> index 793bcba..cac1bce 100644 > >>>> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c > >>>> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c > >>>> @@ -160,6 +160,8 @@ static int find_compression_threshold(struct drm_device *dev, > >>>> int compression_threshold = 1; > >>>> int ret; > >>>> > >>>> + WARN_ON(!mutex_is_locked(&dev->struct_mutex)); > >>> > >>> I'm not a huge fan of vague mutex warnings that don't even check the owner. > >>> I'm espcially not a fan of adding a WARN and not handling the error. > >> > >> But then, what exactly is your proposal? What would you like to see here? > >> > >> We can discard this patch if you want. But I hope you're not > >> advocating for lockdep_assert_held(), because if I switch to lockdep, > >> then Daniel is going to deny it again. Also, this type of WARN_ON is a > >> common pattern on our codebase... > > > > I'm just trying to convince Daniel that blindly using this pattern is > > the wrong approach and encouraging a proliferation of unhandled WARN_ON > > doesn't improve driver robustness. > > I think they serve as useful documentation at the very least, whether in > lockdep form, WARN form, or BUG form. It's not really something we can > recover from either (maybe returning early before touching data?), so... Not grabbing a lock is generally a harmless error since real races out there are rare with X being single-threaded and all that. Especially in stuff called from modeset code. Hence I think just WARN_ON plus continuing on with blissful ignorance is the best approach. I don't the lockdep versions personally since they don't work when lockdep is disabled, which is pretty much always the case. Might be useful to do an assert_mutex_held which always does the most paranoid check (i.e. WARN_ON without lockdep, lockdep_assert_held with lockdep). -Daniel
On Tue, Jun 30, 2015 at 03:34:55PM +0100, Chris Wilson wrote: > On Tue, Jun 30, 2015 at 10:53:10AM -0300, Paulo Zanoni wrote: > > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > Let's make sure the future Paulos don't forget that we need > > struct_mutex when touching dev_priv->mm.stolen. > > As I elluded to in patch 5, I think the stolen warns are a misstep. Imo switching to a separate stolen_mutex should be a separate patch, this just documents the current rules. Which seems fine to me. -Daniel
On Wed, Jul 01, 2015 at 04:00:23PM +0200, Daniel Vetter wrote: > On Tue, Jun 30, 2015 at 03:34:55PM +0100, Chris Wilson wrote: > > On Tue, Jun 30, 2015 at 10:53:10AM -0300, Paulo Zanoni wrote: > > > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > > > Let's make sure the future Paulos don't forget that we need > > > struct_mutex when touching dev_priv->mm.stolen. > > > > As I elluded to in patch 5, I think the stolen warns are a misstep. > > Imo switching to a separate stolen_mutex should be a separate patch, this > just documents the current rules. Which seems fine to me. Introducing a stolen mutex won't be a very much larger patch, and the current locking rules are an impediment for use elsewhere. -Chris
2015-07-01 11:02 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>: > On Wed, Jul 01, 2015 at 04:00:23PM +0200, Daniel Vetter wrote: >> On Tue, Jun 30, 2015 at 03:34:55PM +0100, Chris Wilson wrote: >> > On Tue, Jun 30, 2015 at 10:53:10AM -0300, Paulo Zanoni wrote: >> > > From: Paulo Zanoni <paulo.r.zanoni@intel.com> >> > > >> > > Let's make sure the future Paulos don't forget that we need >> > > struct_mutex when touching dev_priv->mm.stolen. >> > >> > As I elluded to in patch 5, I think the stolen warns are a misstep. >> >> Imo switching to a separate stolen_mutex should be a separate patch, this >> just documents the current rules. Which seems fine to me. > > Introducing a stolen mutex won't be a very much larger patch, and the > current locking rules are an impediment for use elsewhere. I wrote the stolen_mutex patches yesterday, I'll send them soon. > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre
On 07/01/2015 06:56 AM, Daniel Vetter wrote: > On Tue, Jun 30, 2015 at 01:30:27PM -0700, Jesse Barnes wrote: >> On 06/30/2015 07:36 AM, Chris Wilson wrote: >>> On Tue, Jun 30, 2015 at 11:26:11AM -0300, Paulo Zanoni wrote: >>>> 2015-06-30 11:15 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>: >>>>> On Tue, Jun 30, 2015 at 10:53:10AM -0300, Paulo Zanoni wrote: >>>>>> From: Paulo Zanoni <paulo.r.zanoni@intel.com> >>>>>> >>>>>> Let's make sure the future Paulos don't forget that we need >>>>>> struct_mutex when touching dev_priv->mm.stolen. >>>>>> >>>>>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> >>>>>> --- >>>>>> drivers/gpu/drm/i915/i915_gem_stolen.c | 13 +++++++++++++ >>>>>> 1 file changed, 13 insertions(+) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c >>>>>> index 793bcba..cac1bce 100644 >>>>>> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c >>>>>> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c >>>>>> @@ -160,6 +160,8 @@ static int find_compression_threshold(struct drm_device *dev, >>>>>> int compression_threshold = 1; >>>>>> int ret; >>>>>> >>>>>> + WARN_ON(!mutex_is_locked(&dev->struct_mutex)); >>>>> >>>>> I'm not a huge fan of vague mutex warnings that don't even check the owner. >>>>> I'm espcially not a fan of adding a WARN and not handling the error. >>>> >>>> But then, what exactly is your proposal? What would you like to see here? >>>> >>>> We can discard this patch if you want. But I hope you're not >>>> advocating for lockdep_assert_held(), because if I switch to lockdep, >>>> then Daniel is going to deny it again. Also, this type of WARN_ON is a >>>> common pattern on our codebase... >>> >>> I'm just trying to convince Daniel that blindly using this pattern is >>> the wrong approach and encouraging a proliferation of unhandled WARN_ON >>> doesn't improve driver robustness. >> >> I think they serve as useful documentation at the very least, whether in >> lockdep form, WARN form, or BUG form. It's not really something we can >> recover from either (maybe returning early before touching data?), so... > > Not grabbing a lock is generally a harmless error since real races out > there are rare with X being single-threaded and all that. Especially in > stuff called from modeset code. Hence I think just WARN_ON plus continuing > on with blissful ignorance is the best approach. > > I don't the lockdep versions personally since they don't work when lockdep > is disabled, which is pretty much always the case. Might be useful to do > an assert_mutex_held which always does the most paranoid check (i.e. > WARN_ON without lockdep, lockdep_assert_held with lockdep). Maybe we should add WARN_ONs to the lockdep_assert macros in the !CONFIG_LOCKDEP case. That would give us documentation, checking in both cases, and everyone would be happy, right? Jesse
On Wed, Jul 01, 2015 at 08:17:37AM -0700, Jesse Barnes wrote: > On 07/01/2015 06:56 AM, Daniel Vetter wrote: > > On Tue, Jun 30, 2015 at 01:30:27PM -0700, Jesse Barnes wrote: > >> On 06/30/2015 07:36 AM, Chris Wilson wrote: > >>> On Tue, Jun 30, 2015 at 11:26:11AM -0300, Paulo Zanoni wrote: > >>>> 2015-06-30 11:15 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>: > >>>>> On Tue, Jun 30, 2015 at 10:53:10AM -0300, Paulo Zanoni wrote: > >>>>>> From: Paulo Zanoni <paulo.r.zanoni@intel.com> > >>>>>> > >>>>>> Let's make sure the future Paulos don't forget that we need > >>>>>> struct_mutex when touching dev_priv->mm.stolen. > >>>>>> > >>>>>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > >>>>>> --- > >>>>>> drivers/gpu/drm/i915/i915_gem_stolen.c | 13 +++++++++++++ > >>>>>> 1 file changed, 13 insertions(+) > >>>>>> > >>>>>> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c > >>>>>> index 793bcba..cac1bce 100644 > >>>>>> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c > >>>>>> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c > >>>>>> @@ -160,6 +160,8 @@ static int find_compression_threshold(struct drm_device *dev, > >>>>>> int compression_threshold = 1; > >>>>>> int ret; > >>>>>> > >>>>>> + WARN_ON(!mutex_is_locked(&dev->struct_mutex)); > >>>>> > >>>>> I'm not a huge fan of vague mutex warnings that don't even check the owner. > >>>>> I'm espcially not a fan of adding a WARN and not handling the error. > >>>> > >>>> But then, what exactly is your proposal? What would you like to see here? > >>>> > >>>> We can discard this patch if you want. But I hope you're not > >>>> advocating for lockdep_assert_held(), because if I switch to lockdep, > >>>> then Daniel is going to deny it again. Also, this type of WARN_ON is a > >>>> common pattern on our codebase... > >>> > >>> I'm just trying to convince Daniel that blindly using this pattern is > >>> the wrong approach and encouraging a proliferation of unhandled WARN_ON > >>> doesn't improve driver robustness. > >> > >> I think they serve as useful documentation at the very least, whether in > >> lockdep form, WARN form, or BUG form. It's not really something we can > >> recover from either (maybe returning early before touching data?), so... > > > > Not grabbing a lock is generally a harmless error since real races out > > there are rare with X being single-threaded and all that. Especially in > > stuff called from modeset code. Hence I think just WARN_ON plus continuing > > on with blissful ignorance is the best approach. > > > > I don't the lockdep versions personally since they don't work when lockdep > > is disabled, which is pretty much always the case. Might be useful to do > > an assert_mutex_held which always does the most paranoid check (i.e. > > WARN_ON without lockdep, lockdep_assert_held with lockdep). > > Maybe we should add WARN_ONs to the lockdep_assert macros in the > !CONFIG_LOCKDEP case. That would give us documentation, checking in > both cases, and everyone would be happy, right? tbh never tried that ;-) -Daniel
diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c index 793bcba..cac1bce 100644 --- a/drivers/gpu/drm/i915/i915_gem_stolen.c +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c @@ -160,6 +160,8 @@ static int find_compression_threshold(struct drm_device *dev, int compression_threshold = 1; int ret; + WARN_ON(!mutex_is_locked(&dev->struct_mutex)); + /* HACK: This code depends on what we will do in *_enable_fbc. If that * code changes, this code needs to change as well. * @@ -198,6 +200,8 @@ static int i915_setup_compression(struct drm_device *dev, int size, int fb_cpp) struct drm_mm_node *uninitialized_var(compressed_llb); int ret; + WARN_ON(!mutex_is_locked(&dev->struct_mutex)); + ret = find_compression_threshold(dev, &dev_priv->fbc.compressed_fb, size, fb_cpp); if (!ret) @@ -250,6 +254,7 @@ int i915_gem_stolen_setup_compression(struct drm_device *dev, int size, int fb_c { struct drm_i915_private *dev_priv = dev->dev_private; + WARN_ON(!mutex_is_locked(&dev->struct_mutex)); WARN_ON(!mutex_is_locked(&dev_priv->fbc.lock)); if (!drm_mm_initialized(&dev_priv->mm.stolen)) @@ -287,6 +292,8 @@ void i915_gem_cleanup_stolen(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; + WARN_ON(!mutex_is_locked(&dev->struct_mutex)); + if (!drm_mm_initialized(&dev_priv->mm.stolen)) return; @@ -349,6 +356,8 @@ i915_pages_create_for_stolen(struct drm_device *dev, struct sg_table *st; struct scatterlist *sg; + WARN_ON(!mutex_is_locked(&dev->struct_mutex)); + DRM_DEBUG_DRIVER("offset=0x%x, size=%d\n", offset, size); BUG_ON(offset > dev_priv->gtt.stolen_size - size); @@ -445,6 +454,8 @@ i915_gem_object_create_stolen(struct drm_device *dev, u32 size) struct drm_mm_node *stolen; int ret; + WARN_ON(!mutex_is_locked(&dev->struct_mutex)); + if (!drm_mm_initialized(&dev_priv->mm.stolen)) return NULL; @@ -485,6 +496,8 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev, struct i915_vma *vma; int ret; + WARN_ON(!mutex_is_locked(&dev->struct_mutex)); + if (!drm_mm_initialized(&dev_priv->mm.stolen)) return NULL;