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

[John Harrison <john.c.harrison@intel.com>]

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.

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