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

[Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>]

On Thu, Nov 03, 2022 at 11:45:57AM -0700, John Harrison wrote:
>On 11/3/2022 04:31, Tvrtko Ursulin wrote:
>>On 02/11/2022 19:21, 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.
>>>
>>>The function being used to lock the reset-in-progress mutex is called
>>>intel_gt_reset_trylock(). However, as noted it does not follow
>>>standard 'trylock' conventions and exit if already locked. So rename
>>>the current _trylock function to intel_gt_reset_lock_interruptible(),
>>>which is the behaviour it actually provides. In addition, add a new
>>>implementation of _trylock and call that from the busyness stats
>>>worker instead.
>>>
>>>v2: Rename existing trylock to interruptible rather than trying to
>>>preserve the existing (confusing) naming scheme (review comments from
>>>Tvrtko).
>>>
>>>Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
>>>---
>>>  drivers/gpu/drm/i915/gem/i915_gem_mman.c       |  2 +-
>>>  drivers/gpu/drm/i915/gt/intel_reset.c          | 18 ++++++++++++++++--
>>>  drivers/gpu/drm/i915/gt/intel_reset.h          |  1 +
>>>  .../gpu/drm/i915/gt/uc/intel_guc_submission.c  |  4 +++-
>>>  4 files changed, 21 insertions(+), 4 deletions(-)
>>>
>>>diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c 
>>>b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
>>>index e63329bc80659..c29efdef8313a 100644
>>>--- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
>>>+++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
>>>@@ -330,7 +330,7 @@ static vm_fault_t vm_fault_gtt(struct vm_fault *vmf)
>>>      if (ret)
>>>          goto err_rpm;
>>>  -    ret = intel_gt_reset_trylock(ggtt->vm.gt, &srcu);
>>>+    ret = intel_gt_reset_lock_interruptible(ggtt->vm.gt, &srcu);
>>>      if (ret)
>>>          goto err_pages;
>>>  diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c 
>>>b/drivers/gpu/drm/i915/gt/intel_reset.c
>>>index 3159df6cdd492..24736ebee17c2 100644
>>>--- a/drivers/gpu/drm/i915/gt/intel_reset.c
>>>+++ b/drivers/gpu/drm/i915/gt/intel_reset.c
>>>@@ -1407,15 +1407,19 @@ 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_lock(struct intel_gt *gt, int *srcu, 
>>>bool retry)
>>>  {
>>>      might_lock(&gt->reset.backoff_srcu);
>>>-    might_sleep();
>>>+    if (retry)
>>>+        might_sleep();
>>>        rcu_read_lock();
>>>      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)))
>>>@@ -1429,6 +1433,16 @@ int intel_gt_reset_trylock(struct intel_gt 
>>>*gt, int *srcu)
>>>      return 0;
>>>  }
>>>  +int intel_gt_reset_trylock(struct intel_gt *gt, int *srcu)
>>>+{
>>>+    return _intel_gt_reset_lock(gt, srcu, false);
>>>+}
>>>+
>>>+int intel_gt_reset_lock_interruptible(struct intel_gt *gt, int *srcu)
>>>+{
>>>+    return _intel_gt_reset_lock(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..25c975b6e8fc0 100644
>>>--- a/drivers/gpu/drm/i915/gt/intel_reset.h
>>>+++ b/drivers/gpu/drm/i915/gt/intel_reset.h
>>>@@ -39,6 +39,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(struct intel_gt *gt, 
>>>int *srcu);
>>>+int __must_check intel_gt_reset_lock_interruptible(struct 
>>>intel_gt *gt, int *srcu);
>>>  void intel_gt_reset_unlock(struct intel_gt *gt, int tag);
>>>    void intel_gt_set_wedged(struct intel_gt *gt);
>>>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..92e514061d20b 100644
>>>--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
>>>+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
>>>@@ -1401,7 +1401,9 @@ 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
>>>+     * this worker thread if started. So waiting would deadlock.
>>>       */
>>>      ret = intel_gt_reset_trylock(gt, &srcu);
>>>      if (ret)
>>
>>LGTM but I don't remember fully how ping worker and reset interact 
>>so I'll let Umesh r-b. Like is it okay to skip the ping or we'd need 
>>to re-schedule it ASAP due wrap issues? Maybe reset makes that 
>>pointless, I don't remember.
>The reset is cancelling the worker anyway. And it will then be 
>rescheduled once the reset is done. And the ping time is defined as 
>1/8th the wrap time (being approx 223 seconds on current platforms). 
>So as long as the reset doesn't take longer than about 200s, there is 
>no issue. And if the reset did take longer than that then we have 
>bigger issues than the busyness stats (which can't actually be 
>counting anyway because nothing is running if the GT is in reset) 
>being slightly off.

In addition to canceling the ping worker, __reset_guc_busyness_stats is 
performing the same activities that the ping-worker would do if it were 
to run, so we should be safe to skip the worker when a reset is in 
progress, so lgtm,

Reviewed-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>

Thanks,
Umesh

>
>John.
>
>>
>>Regards,
>>
>>Tvrtko
>