On 01/11/2022 16:56, John Harrison wrote:
On 11/1/2022 02:58, Tvrtko Ursulin wrote:
On 31/10/2022 18:30, John Harrison wrote:
On 10/31/2022 05:51, Tvrtko Ursulin wrote:
On 31/10/2022 10:09, Tvrtko Ursulin wrote:
On 28/10/2022 20:46, John.C.Harrison@xxxxxxxxx wrote:
From: John Harrison <John.C.Harrison@xxxxxxxxx>
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@xxxxxxxxx>
---
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(>->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, >->reset.flags)) {
rcu_read_unlock();
+ if (!retry)
+ return -EBUSY;
+
if (wait_event_interruptible(gt->reset.queue,
!test_bit(I915_RESET_BACKOFF,
>->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...
To me, it would seem totally more obvious to have a function called
'trylock' not wait forever until it can manage to acquire the lock.
However, according to '2caffbf1176256 drm/i915: Revoke mmaps and
prevent access to fence registers across reset', the current
behaviour is exactly how the code was originally written and
intended. It hasn't just mutated into some confused evolution a
thousand patches later. So I figure there is some subtle but
important reason why it was named how it is named and yet does what
it does. Therefore it seemed safest to not change it unnecessarily.
Yeah I looked at that but honestly I don't see the trylock semantics
anywhere. The only failure to lock path comes from
wait_event_interruptible. It could have easily been just a naming mishap.
And I find adding a retry parameter to something called trylock makes
this even more non-intuitive and would personally rather rename it
all. Proof in the pudding is that the trylock naming did bite during
development and review of the code this patch is now fixing.
I do however understand your point about a degree of uncertainty but
my feeling is to rather err on the side of obvious naming. Shall we
ask for a third opinion?
Umesh had commented (internally) that the naming seems wrong and would
be good to change it. So we already have a third :).
To be clear, you are thinking to keep the wrappers but rename to
intel_gt_reset_trylock() [retry = false] and
intel_gt_reset_interruptible() [retry = true]? Which will obviously
involve updating all but one existing user to use the interruptible name
as the existing name will change behaviour in a backwards breaking manner.
Yes, intel_gt_reset_lock_interruptible and intel_gt_reset_trylock.
I don't get the behaviour breaking part? Only the name will change.
And amount of churn does not seem a problem:
$ grep intel_gt_reset_trylock -r .
./gem/i915_gem_mman.c: ret = intel_gt_reset_trylock(ggtt->vm.gt, &srcu);
./gt/uc/intel_guc_submission.c: ret = intel_gt_reset_trylock(gt, &srcu);
./gt/intel_reset.c:int intel_gt_reset_trylock(struct intel_gt *gt, int *srcu)
./gt/intel_reset.h:int __must_check intel_gt_reset_trylock(struct intel_gt *gt, int *srcu)
Regards,
Tvrtko
John.
Oh and might_sleep() shouldn't be there with the trylock version - I
mean any flavour of the real trylock.
You mean if the code is split into two completely separate functions?
Or do you just mean to wrap the might_sleep() call with 'if(!retry)'?
And just to be totally clear, the unconditional call to
rcu_read_lock() is not something that can sleep? One doesn't need a
might_sleep() before doing that lock?
Corrrect, rcu_read_lock() can not sleep - it just disables preemption.
So leaving the unconditional might_sleep() would have opportunity for
false positives.
Regards,
Tvrtko
