Re: [PATCH 6/8] drm/i915: add struct_mutex WARNs to i915_gem_stolen.c

[Daniel Vetter <daniel@ffwll.ch>]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx