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

From: Daniel Vetter <daniel@ffwll.ch>
To: Jesse Barnes <jbarnes@virtuousgeek.org>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH 6/8] drm/i915: add struct_mutex WARNs to i915_gem_stolen.c
Date: Wed, 1 Jul 2015 17:43:35 +0200	[thread overview]
Message-ID: <20150701154335.GA23343@phenom.ffwll.local> (raw)
In-Reply-To: <55940491.4010308@virtuousgeek.org>

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