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

[Jesse Barnes <jbarnes@virtuousgeek.org>]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx