Re: [Intel-gfx] [PATCH 2/2] drm/i915/guc: Don't deadlock busyness stats vs reset

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 01/11/2022 16:56, John Harrison wrote:
> On 11/1/2022 02:58, Tvrtko Ursulin wrote:
>> On 31/10/2022 18:30, John Harrison wrote:
>>> On 10/31/2022 05:51, Tvrtko Ursulin wrote:
>>>> On 31/10/2022 10:09, Tvrtko Ursulin wrote:
>>>>> On 28/10/2022 20:46, John.C.Harrison@Intel.com wrote:
>>>>>> From: John Harrison <John.C.Harrison@Intel.com>
>>>>>>
>>>>>> The engine busyness stats has a worker function to do things like
>>>>>> 64bit extend the 32bit hardware counters. The GuC's reset prepare
>>>>>> function flushes out this worker function to ensure no corruption
>>>>>> happens during the reset. Unforunately, the worker function has an
>>>>>> infinite wait for active resets to finish before doing its work. Thus
>>>>>> a deadlock would occur if the worker function had actually started
>>>>>> just as the reset starts.
>>>>>>
>>>>>> Update the worker to abort if a reset is in progress rather than
>>>>>> waiting for it to complete. It will still acquire the reset lock in
>>>>>> the case where a reset was not already in progress. So the processing
>>>>>> is still safe from corruption, but the deadlock can no longer occur.
>>>>>>
>>>>>> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
>>>>>> ---
>>>>>>   drivers/gpu/drm/i915/gt/intel_reset.c             | 15 
>>>>>> ++++++++++++++-
>>>>>>   drivers/gpu/drm/i915/gt/intel_reset.h             |  1 +
>>>>>>   drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c |  6 ++++--
>>>>>>   3 files changed, 19 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c 
>>>>>> b/drivers/gpu/drm/i915/gt/intel_reset.c
>>>>>> index 3159df6cdd492..2f48c6e4420ea 100644
>>>>>> --- a/drivers/gpu/drm/i915/gt/intel_reset.c
>>>>>> +++ b/drivers/gpu/drm/i915/gt/intel_reset.c
>>>>>> @@ -1407,7 +1407,7 @@ void intel_gt_handle_error(struct intel_gt *gt,
>>>>>>       intel_runtime_pm_put(gt->uncore->rpm, wakeref);
>>>>>>   }
>>>>>> -int intel_gt_reset_trylock(struct intel_gt *gt, int *srcu)
>>>>>> +static int _intel_gt_reset_trylock(struct intel_gt *gt, int 
>>>>>> *srcu, bool retry)
>>>>>>   {
>>>>>>       might_lock(&gt->reset.backoff_srcu);
>>>>>>       might_sleep();
>>>>>> @@ -1416,6 +1416,9 @@ int intel_gt_reset_trylock(struct intel_gt 
>>>>>> *gt, int *srcu)
>>>>>>       while (test_bit(I915_RESET_BACKOFF, &gt->reset.flags)) {
>>>>>>           rcu_read_unlock();
>>>>>> +        if (!retry)
>>>>>> +            return -EBUSY;
>>>>>> +
>>>>>>           if (wait_event_interruptible(gt->reset.queue,
>>>>>>                            !test_bit(I915_RESET_BACKOFF,
>>>>>> &gt->reset.flags)))
>>>>>
>>>>> Would it be more obvious to rename the existing semantics to 
>>>>> intel_gt_reset_interruptible(), while the flavour you add in this 
>>>>> patch truly is trylock? I am not sure, since it's all a bit 
>>>>> special, but trylock sure feels confusing if it can sleep forever...
>>> To me, it would seem totally more obvious to have a function called 
>>> 'trylock' not wait forever until it can manage to acquire the lock. 
>>> However, according to '2caffbf1176256 drm/i915: Revoke mmaps and 
>>> prevent access to fence registers across reset', the current 
>>> behaviour is exactly how the code was originally written and 
>>> intended. It hasn't just mutated into some confused evolution a 
>>> thousand patches later. So I figure there is some subtle but 
>>> important reason why it was named how it is named and yet does what 
>>> it does. Therefore it seemed safest to not change it unnecessarily.
>>
>> Yeah I looked at that but honestly I don't see the trylock semantics 
>> anywhere. The only failure to lock path comes from 
>> wait_event_interruptible. It could have easily been just a naming mishap.
>>
>> And I find adding a retry parameter to something called trylock makes 
>> this even more non-intuitive and would personally rather rename it 
>> all. Proof in the pudding is that the trylock naming did bite during 
>> development and review of the code this patch is now fixing.
>>
>> I do however understand your point about a degree of uncertainty but 
>> my feeling is to rather err on the side of obvious naming. Shall we 
>> ask for a third opinion?
> Umesh had commented (internally) that the naming seems wrong and would 
> be good to change it. So we already have a third :).
> 
> To be clear, you are thinking to keep the wrappers but rename to 
> intel_gt_reset_trylock() [retry = false] and 
> intel_gt_reset_interruptible() [retry = true]? Which will obviously 
> involve updating all but one existing user to use the interruptible name 
> as the existing name will change behaviour in a backwards breaking manner.

Yes, intel_gt_reset_lock_interruptible and intel_gt_reset_trylock.

I don't get the behaviour breaking part? Only the name will change.

And amount of churn does not seem a problem:

$ grep intel_gt_reset_trylock -r .
./gem/i915_gem_mman.c:  ret = intel_gt_reset_trylock(ggtt->vm.gt, &srcu);
./gt/uc/intel_guc_submission.c: ret = intel_gt_reset_trylock(gt, &srcu);
./gt/intel_reset.c:int intel_gt_reset_trylock(struct intel_gt *gt, int *srcu)
./gt/intel_reset.h:int __must_check intel_gt_reset_trylock(struct intel_gt *gt, int *srcu)

Regards,

Tvrtko

> 
> John.
> 
>>
>>>> Oh and might_sleep() shouldn't be there with the trylock version - I 
>>>> mean any flavour of the real trylock.
>>> You mean if the code is split into two completely separate functions? 
>>> Or do you just mean to wrap the might_sleep() call with 'if(!retry)'?
>>>
>>> And just to be totally clear, the unconditional call to 
>>> rcu_read_lock() is not something that can sleep? One doesn't need a 
>>> might_sleep() before doing that lock?
>>
>> Corrrect, rcu_read_lock() can not sleep - it just disables preemption. 
>> So leaving the unconditional might_sleep() would have opportunity for 
>> false positives.
>>
>> Regards,
>>
>> Tvrtko
>