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

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

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

Oh and might_sleep() shouldn't be there with the trylock version - I 
mean any flavour of the real trylock.

Regards,

Tvrtko