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

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

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

Regards,

Tvrtko

> @@ -1429,6 +1432,16 @@ int intel_gt_reset_trylock(struct intel_gt *gt, int *srcu)
>   	return 0;
>   }
>   
> +int intel_gt_reset_trylock_noretry(struct intel_gt *gt, int *srcu)
> +{
> +	return _intel_gt_reset_trylock(gt, srcu, false);
> +}
> +
> +int intel_gt_reset_trylock(struct intel_gt *gt, int *srcu)
> +{
> +	return _intel_gt_reset_trylock(gt, srcu, true);
> +}
> +
>   void intel_gt_reset_unlock(struct intel_gt *gt, int tag)
>   __releases(&gt->reset.backoff_srcu)
>   {
> diff --git a/drivers/gpu/drm/i915/gt/intel_reset.h b/drivers/gpu/drm/i915/gt/intel_reset.h
> index adc734e673870..7f863726eb6a2 100644
> --- a/drivers/gpu/drm/i915/gt/intel_reset.h
> +++ b/drivers/gpu/drm/i915/gt/intel_reset.h
> @@ -38,6 +38,7 @@ int __intel_engine_reset_bh(struct intel_engine_cs *engine,
>   
>   void __i915_request_reset(struct i915_request *rq, bool guilty);
>   
> +int __must_check intel_gt_reset_trylock_noretry(struct intel_gt *gt, int *srcu);
>   int __must_check intel_gt_reset_trylock(struct intel_gt *gt, int *srcu);
>   void intel_gt_reset_unlock(struct intel_gt *gt, int tag);
>   
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> index 941613be3b9dd..1fa1bc7dde3df 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> @@ -1401,9 +1401,11 @@ static void guc_timestamp_ping(struct work_struct *wrk)
>   
>   	/*
>   	 * Synchronize with gt reset to make sure the worker does not
> -	 * corrupt the engine/guc stats.
> +	 * corrupt the engine/guc stats. NB: can't actually block waiting
> +	 * for a reset to complete as the reset requires flushing out
> +	 * any running worker thread. So waiting would deadlock.
>   	 */
> -	ret = intel_gt_reset_trylock(gt, &srcu);
> +	ret = intel_gt_reset_trylock_noretry(gt, &srcu);
>   	if (ret)
>   		return;
>