[Intel-gfx] [PATCH v2 2/2] drm/i915/guc: Don't deadlock busyness stats vs reset
Umesh Nerlige Ramappa
umesh.nerlige.ramappa at intel.com
Thu Nov 3 18:54:21 UTC 2022
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 at Intel.com wrote:
>>>From: John Harrison <John.C.Harrison at 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 at 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(>->reset.backoff_srcu);
>>>- might_sleep();
>>>+ if (retry)
>>>+ might_sleep();
>>> rcu_read_lock();
>>> while (test_bit(I915_RESET_BACKOFF, >->reset.flags)) {
>>> rcu_read_unlock();
>>> + if (!retry)
>>>+ return -EBUSY;
>>>+
>>> if (wait_event_interruptible(gt->reset.queue,
>>> !test_bit(I915_RESET_BACKOFF,
>>> >->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(>->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 at intel.com>
Thanks,
Umesh
>
>John.
>
>>
>>Regards,
>>
>>Tvrtko
>
More information about the Intel-gfx
mailing list