[RFC 19/34] drm/xe: Remove pm_runtime lockdep
Rodrigo Vivi
rodrigo.vivi at intel.com
Thu Feb 15 22:47:48 UTC 2024
On Mon, Feb 05, 2024 at 11:54:45AM +0000, Matthew Auld wrote:
> On 26/01/2024 20:30, Rodrigo Vivi wrote:
> > This lockdep was initially designed for the mem_access,
> > where the mem_access needed to run the resume sync from
> > the innerbounds and count the references.
> >
> > With the runtime moving to the outer bounds of the driver
> > and the mem_access replaced by the pure rpm get/put
> > references, it is no longer needed and it is in a matter
>
> We are also calling it in workers like invalidation_fence_work_func(),
> xe_sched_process_msg_work() etc. It is still quite easy to deadlock with
> that.
>
> > of fact just splatting may false positives as the following:
>
> Yeah, calling invalidation_fence_work_func() directly in the callers context
> is a false positive since ioctl has the rpm ref. But what about calling that
> in the async case where invalidation_fence_work_func() is only called when
> the in-fence signals? We are sure that is safe? It should be simple to fix
> the false positive here, no? If we just delete all the annotations we get
> zero help from lockdep for the more complicated cases. Also IIRC lockdep
> doesn't show you every splat once it triggers once, so who knows what else
> is lurking and whether that is also false positive?
>
> >
> > -> #1 (xe_pm_runtime_lockdep_map){+.+.}-{0:0}:
> > [ 384.778761] xe_pm_runtime_get+0xa3/0x100 [xe]
> > [ 384.783871] invalidation_fence_work_func+0x7f/0x2b0 [xe]
> > [ 384.789942] invalidation_fence_init+0x8c2/0xce0 [xe]
> > [ 384.795671] __xe_pt_unbind_vma+0x4a7/0x1be0 [xe]
> > [ 384.801050] xe_vm_unbind+0x22f/0xc70 [xe]
> > [ 384.805821] __xe_vma_op_execute+0xc67/0x1af0 [xe]
> > [ 384.811286] xe_vm_bind_ioctl+0x3a36/0x66c0 [xe]
> > [ 384.816579] drm_ioctl_kernel+0x14a/0x2c0
> > [ 384.821132] drm_ioctl+0x4c6/0xab0
> > [ 384.825073] xe_drm_ioctl+0xa1/0xe0 [xe]
> > [ 384.829651] __x64_sys_ioctl+0x130/0x1a0
> > [ 384.834115] do_syscall_64+0x5c/0xe0
> > [ 384.838232] entry_SYSCALL_64_after_hwframe+0x6e/0x76
> > [ 384.843829]
> > -> #0 (reservation_ww_class_mutex){+.+.}-{4:4}:
> > [ 384.850911] __lock_acquire+0x3261/0x6330
> > [ 384.855462] lock_acquire+0x19b/0x4d0
> > [ 384.859666] __ww_mutex_lock.constprop.0+0x1d8/0x3500
> > [ 384.865263] ww_mutex_lock+0x38/0x150
> > [ 384.869465] xe_bo_lock+0x41/0x70 [xe]
> > [ 384.873869] xe_bo_evict_all+0x7ad/0xa40 [xe]
> > [ 384.878883] xe_pm_runtime_suspend+0x297/0x340 [xe]
> > [ 384.884431] xe_pci_runtime_suspend+0x3b/0x1e0 [xe]
> > [ 384.889975] pci_pm_runtime_suspend+0x168/0x540
> > [ 384.895052] __rpm_callback+0xa9/0x390
> > [ 384.899343] rpm_callback+0x1aa/0x210
> > [ 384.903543] rpm_suspend+0x2ea/0x14c0
> > [ 384.907746] pm_runtime_work+0x133/0x170
> > [ 384.912213] process_one_work+0x73b/0x1230
> > [ 384.916853] worker_thread+0x726/0x1320
> > [ 384.921237] kthread+0x2ee/0x3d0
> > [ 384.925005] ret_from_fork+0x2d/0x70
> > [ 384.929120] ret_from_fork_asm+0x1b/0x30
> > [ 384.933585]
> > other info that might help us debug this:
> >
> > [ 384.941625] Possible unsafe locking scenario:
> >
> > [ 384.947572] CPU0 CPU1
> > [ 384.952123] ---- ----
> > [ 384.956676] lock(xe_pm_runtime_lockdep_map);
> > [ 384.961140] lock(reservation_ww_class_mutex);
> > [ 384.968220] lock(xe_pm_runtime_lockdep_map);
> > [ 384.975214] lock(reservation_ww_class_mutex);
> > [ 384.979765]
> > *** DEADLOCK ***
> >
> > In a matter of fact, there's actually a third lock that is not
> > in this picture:
> > spin_lock_irq(&dev->power.lock);
> > and INIT_WORK(&dev->power.work, pm_runtime_work);
> >
> > The pm_callback_task will ensure that there's no recursive
> > calls of the resume function and it will increase the
> > reference counter anyway.
> >
> > Then, the pm_runtime workqueue and spin locks will avoid that
> > any resume and suspend operations happens in parallel with
> > other resume and suspend operations.
> >
> > With that, the only thing that we are actually doing here is
> > to wrongly train the lockdep, basically saying that we will
> > acquire some locks on resume and on suspend concurrently,
> > entirely ignoring its serialization and protection.
> >
> > The above scenario is simply not possible because there's
> > a serialization with the spin_lock_irq(&dev->power.lock)
> > before each operation. However we are telling the lockep
> > that the lock(xe_pm_runtime_lockdep_map) occurs before
> > the &dev->power.lock and lockdep is not capable to see
> > that other protection.
>
> Can you share some more info here? AFAIK dev->power.lock is an RPM core lock
> that protects internal state like transitioning between ACTIVE, SUSPENDING,
> RESUMING etc. It is never held when calling any of our rpm callbacks, so it
> should never factor in to xe_pm_runtime_lockdep_map.
>
> The overall thing should look like this:
>
> xe_rpm_get:
> lock_map_acquire(&xe_pm_runtime_lockdep_map);
> lock_map_release(&xe_pm_runtime_lockdep_map);
> .....
> RPM core grabs dev->power.lock
>
> rpm resume callback: (RPM has dropped dev->power.lock)
^ this is exactly the problem.
RPM doesn't drop the dev->power.lock while the callback
is called. it relaxes while waiting for other transaction
to finish, but it is hold by the time it gets to the callback.
> lock_map_acquire(&xe_pm_runtime_lockdep_map);
> ....do resume stuff
> lock_map_release(&xe_pm_runtime_lockdep_map);
>
> rpm suspend callback: (RPM has dropped dev->power.lock)
> lock_map_acquire(&xe_pm_runtime_lockdep_map);
> .....do suspend stuff
> lock_map_release(&xe_pm_runtime_lockdep_map);
>
> >
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > ---
> > drivers/gpu/drm/xe/xe_pm.c | 55 --------------------------------------
> > 1 file changed, 55 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c
> > index 86bf225dba02..f49e449d9fb7 100644
> > --- a/drivers/gpu/drm/xe/xe_pm.c
> > +++ b/drivers/gpu/drm/xe/xe_pm.c
> > @@ -67,12 +67,6 @@
> > */
> > -#ifdef CONFIG_LOCKDEP
> > -struct lockdep_map xe_pm_runtime_lockdep_map = {
> > - .name = "xe_pm_runtime_lockdep_map"
> > -};
> > -#endif
> > -
> > /**
> > * xe_pm_suspend - Helper for System suspend, i.e. S0->S3 / S0->S2idle
> > * @xe: xe device instance
> > @@ -291,29 +285,6 @@ int xe_pm_runtime_suspend(struct xe_device *xe)
> > /* Disable access_ongoing asserts and prevent recursive pm calls */
> > xe_pm_write_callback_task(xe, current);
> > - /*
> > - * The actual xe_pm_runtime_put() is always async underneath, so
> > - * exactly where that is called should makes no difference to us. However
> > - * we still need to be very careful with the locks that this callback
> > - * acquires and the locks that are acquired and held by any callers of
> > - * xe_runtime_pm_get(). We already have the matching annotation
> > - * on that side, but we also need it here. For example lockdep should be
> > - * able to tell us if the following scenario is in theory possible:
> > - *
> > - * CPU0 | CPU1 (kworker)
> > - * lock(A) |
> > - * | xe_pm_runtime_suspend()
> > - * | lock(A)
> > - * xe_pm_runtime_get() |
> > - *
> > - * This will clearly deadlock since rpm core needs to wait for
> > - * xe_pm_runtime_suspend() to complete, but here we are holding lock(A)
> > - * on CPU0 which prevents CPU1 making forward progress. With the
> > - * annotation here and in xe_pm_runtime_get() lockdep will see
> > - * the potential lock inversion and give us a nice splat.
> > - */
> > - lock_map_acquire(&xe_pm_runtime_lockdep_map);
> > -
> > /*
> > * Applying lock for entire list op as xe_ttm_bo_destroy and xe_bo_move_notify
> > * also checks and delets bo entry from user fault list.
> > @@ -341,7 +312,6 @@ int xe_pm_runtime_suspend(struct xe_device *xe)
> > if (xe->d3cold.allowed)
> > xe_display_pm_runtime_suspend(xe);
> > out:
> > - lock_map_release(&xe_pm_runtime_lockdep_map);
> > xe_pm_write_callback_task(xe, NULL);
> > return err;
> > }
> > @@ -361,8 +331,6 @@ int xe_pm_runtime_resume(struct xe_device *xe)
> > /* Disable access_ongoing asserts and prevent recursive pm calls */
> > xe_pm_write_callback_task(xe, current);
> > - lock_map_acquire(&xe_pm_runtime_lockdep_map);
> > -
> > /*
> > * It can be possible that xe has allowed d3cold but other pcie devices
> > * in gfx card soc would have blocked d3cold, therefore card has not
> > @@ -400,31 +368,10 @@ int xe_pm_runtime_resume(struct xe_device *xe)
> > goto out;
> > }
> > out:
> > - lock_map_release(&xe_pm_runtime_lockdep_map);
> > xe_pm_write_callback_task(xe, NULL);
> > return err;
> > }
> > -/*
> > - * For places where resume is synchronous it can be quite easy to deadlock
> > - * if we are not careful. Also in practice it might be quite timing
> > - * sensitive to ever see the 0 -> 1 transition with the callers locks
> > - * held, so deadlocks might exist but are hard for lockdep to ever see.
> > - * With this in mind, help lockdep learn about the potentially scary
> > - * stuff that can happen inside the runtime_resume callback by acquiring
> > - * a dummy lock (it doesn't protect anything and gets compiled out on
> > - * non-debug builds). Lockdep then only needs to see the
> > - * xe_pm_runtime_lockdep_map -> runtime_resume callback once, and then can
> > - * hopefully validate all the (callers_locks) -> xe_pm_runtime_lockdep_map.
> > - * For example if the (callers_locks) are ever grabbed in the
> > - * runtime_resume callback, lockdep should give us a nice splat.
> > - */
> > -static void pm_runtime_lockdep_training(void)
> > -{
> > - lock_map_acquire(&xe_pm_runtime_lockdep_map);
> > - lock_map_release(&xe_pm_runtime_lockdep_map);
> > -}
> > -
> > /**
> > * xe_pm_runtime_get - Get a runtime_pm reference and resume synchronously
> > * @xe: xe device instance
> > @@ -436,7 +383,6 @@ void xe_pm_runtime_get(struct xe_device *xe)
> > if (xe_pm_read_callback_task(xe) == current)
> > return;
> > - pm_runtime_lockdep_training();
> > pm_runtime_resume(xe->drm.dev);
> > }
> > @@ -466,7 +412,6 @@ int xe_pm_runtime_get_sync(struct xe_device *xe)
> > if (WARN_ON(xe_pm_read_callback_task(xe) == current))
> > return -ELOOP;
> > - pm_runtime_lockdep_training();
> > return pm_runtime_get_sync(xe->drm.dev);
> > }
More information about the Intel-xe
mailing list