On Tue, Dec 19, 2023 at 11:59:57AM -0800, John.C.Harrison@xxxxxxxxx wrote:
> From: John Harrison <John.C.Harrison@xxxxxxxxx>
>
> Avoid the following lockdep complaint:
> <4> [298.856498] ======================================================
> <4> [298.856500] WARNING: possible circular locking dependency detected
> <4> [298.856503] 6.7.0-rc5-CI_DRM_14017-g58ac4ffc75b6+ #1 Tainted: G
> N
> <4> [298.856505] ------------------------------------------------------
> <4> [298.856507] kworker/4:1H/190 is trying to acquire lock:
> <4> [298.856509] ffff8881103e9978 (>->reset.backoff_srcu){++++}-{0:0}, at:
> _intel_gt_reset_lock+0x35/0x380 [i915]
> <4> [298.856661]
> but task is already holding lock:
> <4> [298.856663] ffffc900013f7e58
> ((work_completion)(&(&guc->timestamp.work)->work)){+.+.}-{0:0}, at:
> process_scheduled_works+0x264/0x530
> <4> [298.856671]
> which lock already depends on the new lock.
>
> The complaint is not actually valid. The busyness worker thread does
> indeed hold the worker lock and then attempt to acquire the reset lock
> (which may have happened in reverse order elsewhere). However, it does
> so with a trylock that exits if the reset lock is not available
> (specifically to prevent this and other similar deadlocks).
> Unfortunately, lockdep does not understand the trylock semantics (the
> lock is an i915 specific custom implementation for resets).
>
> Not doing a synchronous flush of the worker thread when a reset is in
> progress resolves the lockdep splat by never even attempting to grab
> the lock in this particular scenario.
>
> There are situatons where a synchronous cancel is required, however.
> So, always do the synchronous cancel if not in reset. And add an extra
> synchronous cancel to the end of the reset flow to account for when a
> reset is occurring at driver shutdown and the cancel is no longer
> synchronous but could lead to unallocated memory accesses if the
> worker is not stopped.
>
> Signed-off-by: Zhanjun Dong <zhanjun.dong@xxxxxxxxx>
> Signed-off-by: John Harrison <John.C.Harrison@xxxxxxxxx>
> Cc: Andi Shyti <andi.shyti@xxxxxxxxxxxxxxx>
> Cc: Daniel Vetter <daniel@xxxxxxxx>
> ---
> .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 48 ++++++++++++++++++-
> drivers/gpu/drm/i915/gt/uc/intel_uc.c | 2 +-
> 2 files changed, 48 insertions(+), 2 deletions(-)
>
> 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 a259f1118c5ab..0228a77d456ed 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> @@ -1362,7 +1362,45 @@ static void guc_enable_busyness_worker(struct intel_guc *guc)
>
> static void guc_cancel_busyness_worker(struct intel_guc *guc)
> {
> - cancel_delayed_work_sync(&guc->timestamp.work);
> + /*
> + * There are many different call stacks that can get here. Some of them
> + * hold the reset mutex. The busyness worker also attempts to acquire the
> + * reset mutex. Synchronously flushing a worker thread requires acquiring
> + * the worker mutex. Lockdep sees this as a conflict. It thinks that the
> + * flush can deadlock because it holds the worker mutex while waiting for
> + * the reset mutex, but another thread is holding the reset mutex and might
> + * attempt to use other worker functions.
> + *
> + * In practice, this scenario does not exist because the busyness worker
> + * does not block waiting for the reset mutex. It does a try-lock on it and
> + * immediately exits if the lock is already held. Unfortunately, the mutex
> + * in question (I915_RESET_BACKOFF) is an i915 implementation which has lockdep
> + * annotation but not to the extent of explaining the 'might lock' is also a
> + * 'does not need to lock'. So one option would be to add more complex lockdep
> + * annotations to ignore the issue (if at all possible). A simpler option is to
> + * just not flush synchronously when a rest in progress. Given that the worker
> + * will just early exit and re-schedule itself anyway, there is no advantage
> + * to running it immediately.
> + *
> + * If a reset is not in progress, then the synchronous flush may be required.
> + * As noted many call stacks lead here, some during suspend and driver unload
> + * which do require a synchronous flush to make sure the worker is stopped
> + * before memory is freed.
> + *
> + * Trying to pass a 'need_sync' or 'in_reset' flag all the way down through
> + * every possible call stack is unfeasible. It would be too intrusive to many
> + * areas that really don't care about the GuC backend. However, there is the
> + * 'reset_in_progress' flag available, so just use that.
> + *
> + * And note that in the case of a reset occurring during driver unload
> + * (wedge_on_fini), skipping the cancel in _prepare (when the reset flag is set
> + * is fine because there is another cancel in _finish (when the reset flag is
> + * not).
> + */
> + if (guc_to_gt(guc)->uc.reset_in_progress)
> + cancel_delayed_work(&guc->timestamp.work);
> + else
> + cancel_delayed_work_sync(&guc->timestamp.work);
I think it's all fairly horrible (but that's not news), but this comment
here I think explains in adequate detail what's all going on, what all
matters, why it's all safe and why it's rather hard to fix in a clean
fashion. So realistically about as good as it will ever get.
More importantly, it doesn't gloss over any of the details which do matter
(of which there is a lot, as the length of this comment shows).
Acked-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> }
>
> static void __reset_guc_busyness_stats(struct intel_guc *guc)
> @@ -1948,8 +1986,16 @@ void intel_guc_submission_cancel_requests(struct intel_guc *guc)
>
> void intel_guc_submission_reset_finish(struct intel_guc *guc)
> {
> + /*
> + * Ensure the busyness worker gets cancelled even on a fatal wedge.
> + * Note that reset_prepare is not allowed to because it confuses lockdep.
> + */
> + if (guc_submission_initialized(guc))
> + guc_cancel_busyness_worker(guc);
> +
> /* Reset called during driver load or during wedge? */
> if (unlikely(!guc_submission_initialized(guc) ||
> + !intel_guc_is_fw_running(guc) ||
> intel_gt_is_wedged(guc_to_gt(guc)))) {
> return;
> }
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> index 3872d309ed31f..5a49305fb13be 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> @@ -640,7 +640,7 @@ void intel_uc_reset_finish(struct intel_uc *uc)
> uc->reset_in_progress = false;
>
> /* Firmware expected to be running when this function is called */
> - if (intel_guc_is_fw_running(guc) && intel_uc_uses_guc_submission(uc))
> + if (intel_uc_uses_guc_submission(uc))
> intel_guc_submission_reset_finish(guc);
> }
>
> --
> 2.41.0
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
