Re: [Intel-gfx] [PATCH v5] drm/i915: Avoid circular locking dependency when flush delayed work on gt reset

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 9/5/2023 23:50, Daniel Vetter wrote:
On Mon, Aug 28, 2023 at 04:01:38PM -0700, John Harrison wrote:
On 8/23/2023 10:37, John Harrison wrote:
On 8/23/2023 09:00, Daniel Vetter wrote:
On Tue, Aug 22, 2023 at 11:53:24AM -0700, John Harrison wrote:
On 8/11/2023 11:20, Zhanjun Dong wrote:
This attempts to avoid circular locking dependency between
flush delayed
work and intel_gt_reset.
When intel_gt_reset was called, task will hold a lock.
To cacel delayed work here, the _sync version will also
acquire a lock,
which might trigger the possible cirular locking dependency warning.
When intel_gt_reset called, reset_in_progress flag will be
set, add code
to check the flag, call async verion if reset is in progress.

Signed-off-by: Zhanjun Dong<zhanjun.dong@xxxxxxxxx>
Cc: John Harrison<John.C.Harrison@xxxxxxxxx>
Cc: Andi Shyti<andi.shyti@xxxxxxxxxxxxxxx>
Cc: Daniel Vetter<daniel@xxxxxxxx>
---
    drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 11 ++++++++++-
    1 file changed, 10 insertions(+), 1 deletion(-)

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 a0e3ef1c65d2..600388c849f7 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
@@ -1359,7 +1359,16 @@ 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);
+    /*
+     * When intel_gt_reset was called, task will hold a lock.
+     * To cacel delayed work here, the _sync version will
also acquire a lock, which might
+     * trigger the possible cirular locking dependency warning.
+     * Check the reset_in_progress flag, call async verion
if reset is in progress.
+     */
This needs to explain in much more detail what is going on and
why it is not
a problem. E.g.:

     The busyness worker needs to be cancelled. In general that means
     using the synchronous cancel version to ensure that an in-progress
     worker will not keep executing beyond whatever is happening that
     needs the cancel. E.g. suspend, driver unload, etc. However, in the
     case of a reset, the synchronous version is not required and can
     trigger a false deadlock detection warning.

     The business worker takes the reset mutex to protect against resets
     interfering with it. However, it does a trylock and bails
out if the
     reset lock is already acquired. Thus there is no actual deadlock or
     other concern with the worker running concurrently with a reset. So
     an asynchronous cancel is safe in the case of a reset rather than a
     driver unload or suspend type operation. On the other hand, if the
     cancel_sync version is used when a reset is in progress then the
     mutex deadlock detection sees the mutex being acquired through
     multiple paths and complains.

     So just don't bother. That keeps the detection code happy and is
     safe because of the trylock code described above.
So why do we even need to cancel anything if it doesn't do anything
while
the reset is in progress?
It still needs to be cancelled. The worker only aborts if it is actively
executing concurrently with the reset. It might not start to execute
until after the reset has completed. And there is presumably a reason
why the cancel is being called, a reason not necessarily related to
resets at all. Leaving the worker to run arbitrarily after the driver is
expecting it to be stopped will lead to much worse things than a fake
lockdep splat, e.g. a use after free pointer deref.

John.
@Daniel Vetter - ping? Is this explanation sufficient? Are you okay with
this change now?
Sorry for the late reply, I'm constantly behind on mails :-/ Ping me on
irc next time around if I don't reply, that's quicker.

"presumably" isn't good enough for locking design. Either you know, and
can prove it all, or you shouldn't touch the code and its locking design
before you've figured this out.

Again, either this is a deadlock, race condition, or the cancel isn't
necessary. And this argument works in full generality. All this patch does
it replace the dealock with one of the other two, and that's not good
enough if you don't even know which one it is.

- if you need the cancel, you have a race condition

- if you don't have a race condition, you don't need the cancel
In the case of a reset in progress, we do not strictly need the cancel. The worker thread will take care of avoiding a deadlock by itself. But it is more efficient to do the cancel and avoid unnecessary code execution if possible. It is also more logically correct - the worker is being stopped, therefore we should cancel any pending execution of the worker.

In the case of a reset not being in progress, we absolutely do need the cancel as there are multiple race conditions.


- currently you have the deadlock
No, we do not. There is no deadlock.

The worker thread explicitly does a trylock and reschedules itself for later if it could not get the lock. Lockdep does not understand the back off semantics of the trylock and reports a false failure.

As explained in the above code comment, if a reset is in progress then the synchronous cancel is not required because the trylock will take care of it. An asynchronous cancel is still better than no cancel because it at least tries to stop the worker from running. It's not a problem if the worker does run, but there is no advantage to running it so why not attempt to cancel it and prevent unnecessary code execution? If a reset is not in progress then we do want the synchronous cancel because the disable is potentially part of a driver unload or similar operation that requires the worker to be stopped first. Otherwise, the worker will attempt to reference pointers that might no longer exist at the time it runs. Clearly, that is not the situation if a reset is in progress. The driver cannot be unloading if it is in the middle of a reset operation.


"presumably" and "maybe" aint enoug for locking design.
Bad choice of words. I simply meant that the low level helper does not know the exact call stack it was called with. It can trivially determine if a reset is in progress or not, and that is all it really needs to know about. There are multiple other paths to this helper, none of which involve resets and some of which require a synchronous cancel.

A previous version of this patch attempted to have two separate helpers - one synchronous and one asynchronous. The intention being that any call stack involving reset would call the async version and any stack requiring the sync version would call that. However, the i915 reset design is hideously complex and those changes were much more intrusive and fragile. Simply testing the reset flag at the lowest level is significantly simpler and safer.

John.


Cheers, Daniel

John.

Just remove the cancel from the reset path as uneeded instead, and
explain
why that's ok? Because that's defacto what the cancel_work with a
potential deadlock scenario for cancel_work_sync does, you either don't
need it at all, or the replacement creates a bug.
-Daniel

John.


+    if (guc_to_gt(guc)->uc.reset_in_progress)
+        cancel_delayed_work(&guc->timestamp.work);
+    else
+ cancel_delayed_work_sync(&guc->timestamp.work);
    }
    static void __reset_guc_busyness_stats(struct intel_guc *guc)




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux