On Fri, Jul 10, 2026 at 7:52 AM <[email protected]> wrote:
>
> From: Vitaly Prosyak <[email protected]>
>
> Move fs_reclaim_acquire() to before all lock acquisitions to teach
> lockdep the correct dependency order and eliminate false positive
> circular locking dependency warning.
>
> PROBLEM:
>
> amdgpu_lockdep_init() trains lockdep by acquiring locks in sequence
> while calling fs_reclaim_acquire() in the middle. The fs_reclaim call
> was placed while notifier_lock was held, teaching lockdep:
>
> notifier_lock -> fs_reclaim
>
> However, at runtime, MMU notifier callbacks run from inside memory
> reclaim, establishing the opposite dependency:
>
> fs_reclaim -> mmu_notifier -> notifier_lock
>
> These form a false cycle that lockdep reports when reclaim unmaps a
> page covered by an amdgpu userptr notifier.
>
> LOCKDEP WARNING EXAMPLE (BEFORE FIX):
>
> WARNING: possible circular locking dependency detected
> 6.19.0+ #48 Not tainted
> amd_lockdep/3762 is trying to acquire lock:
> ffff8881756f3c30 (&amdgpu_notifier_lock_key), at:
> amdgpu_hmm_invalidate_gfx
>
> but task is already holding lock:
> ffffffff92e4b5c0 (mmu_notifier_invalidate_range_start), at:
> try_to_unmap_one
>
> Chain exists of:
> &amdgpu_notifier_lock_key --> fs_reclaim -->
> mmu_notifier_invalidate_range_start
>
> Possible unsafe locking scenario:
> CPU0 CPU1
> ---- ----
> lock(mmu_notifier_invalidate_range_start);
> lock(fs_reclaim);
> lock(mmu_notifier_invalidate_range_start);
> lock(&amdgpu_notifier_lock_key);
>
> *** DEADLOCK ***
>
> The dependency chain shows:
> -> #5 (fs_reclaim):
> amdgpu_lockdep_init+0x61e/0x730 [amdgpu] <- FALSE EDGE
> -> #0 (&amdgpu_notifier_lock_key):
> amdgpu_hmm_invalidate_gfx+0x77/0x110 [amdgpu] <- RUNTIME PATH
>
> SOLUTION:
>
> Move fs_reclaim_acquire/release to BEFORE all lock acquisitions.
> This teaches lockdep that fs_reclaim is outermost, matching runtime
> where reclaim calls MMU notifiers which then take notifier_lock.
>
> VERIFICATION (AFTER FIX):
>
> Test reproducer (based on Michael Gavrilov's standalone test case)
> creates a 64MB GPU userptr memory buffer, then repeatedly forces the
> kernel to reclaim it by moving pages to disk (simulating memory
> pressure). Each reclaim cycle triggers the MMU notifier callback path.
>
> The test would be available as an IGT subtest:
> tests/amdgpu/amd_lockdep.c::notifier-reclaim-splat
>
> Results:
> Before fix: Lockdep false positive warning on first reclaim attempt
> After fix: Completed 8 reclaim cycles with no lockdep warnings
>
> dmesg after fix shows only test success messages, no circular dependency
> warnings. The fix is minimal (relocate 2 lines) with no runtime overhead,
> only corrects lockdep's static analysis model.
>
> Reported-by: Michael Gavrilov <[email protected]>
> Analyzed-by: Michael Gavrilov <[email protected]>
> Test-case-by: Michael Gavrilov <[email protected]>
> Suggested-by: Christian König <[email protected]>
> Tested-by: Vitaly Prosyak <[email protected]>
> Cc: Christian König <[email protected]>
> Cc: Alex Deucher <[email protected]>
> Signed-off-by: Vitaly Prosyak <[email protected]>
> Change-Id: I584f833aaeac39d7eb777980f49ee503e70eb058
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_lockdep.c | 47 +++++++++++++++------
> 1 file changed, 34 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_lockdep.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_lockdep.c
> index 61450af539a6..718411f724f3 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_lockdep.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_lockdep.c
> @@ -18,6 +18,7 @@
>
> struct amdgpu_lockdep_dummy_locks {
> struct mutex reset_lock;
> + struct mutex filelist_mutex;
> struct mutex userq_sch_mutex;
> struct mutex userq_mutex;
> struct mutex notifier_lock;
> @@ -34,6 +35,7 @@ static struct lock_class_key amdgpu_notifier_lock_key;
> static struct lock_class_key amdgpu_vram_lock_key;
> static struct lock_class_key amdgpu_reset_sem_key;
> static struct lock_class_key amdgpu_reset_lock_key;
> +static struct lock_class_key amdgpu_filelist_mutex_key;
> static struct lock_class_key amdgpu_srbm_lock_key;
> static struct lock_class_key amdgpu_grbm_lock_key;
> static struct lock_class_key amdgpu_mmio_lock_key;
> @@ -57,6 +59,9 @@ void amdgpu_lockdep_set_class(struct amdgpu_device *adev)
> if (adev->reset_domain)
> lockdep_set_class(&adev->reset_domain->sem,
> &amdgpu_reset_sem_key);
> +
> + lockdep_set_class(&adev_to_drm(adev)->filelist_mutex,
> + &amdgpu_filelist_mutex_key);
> }
>
> /**
> @@ -74,9 +79,10 @@ void amdgpu_lockdep_set_class(struct amdgpu_device *adev)
> * 4. vram_lock - VRAM allocator lock
> * 5. reset_domain->sem - GPU reset synchronization
> * 6. reset_lock - Reset control lock
> - * 7. srbm_mutex - SRBM register access
> - * 8. grbm_idx_mutex - GRBM index access
> - * 9. mmio_idx_lock - MMIO index access (spinlock)
> + * 7. filelist_mutex - DRM file list iteration (ddev->filelist_mutex)
> + * 8. srbm_mutex - SRBM register access
> + * 9. grbm_idx_mutex - GRBM index access
> + * 10. mmio_idx_lock - MMIO index access (spinlock)
> *
> * Evidence:
> * - userq_sch_mutex -> userq_mutex: amdgpu_gfx_kfd_sch_ctrl() calls
> @@ -88,6 +94,9 @@ void amdgpu_lockdep_set_class(struct amdgpu_device *adev)
> * must be outer to reset_domain->sem
> * - vram_lock -> reset_domain->sem: VRAM management paths may need to
> * wait for ongoing reset to complete
> + * - reset_domain->sem -> filelist_mutex: GPU reset path
> + * (amdgpu_device_gpu_recover) holds reset_domain->sem and calls
> + * amdgpu_device_eventfd_signal_gpu_reset() which takes filelist_mutex
> *
> * Note: mmap_lock ordering relative to GPU locks is already taught
> * by dma-resv (drivers/dma-buf/dma-resv.c).
> @@ -117,6 +126,7 @@ int amdgpu_lockdep_init(void)
> mutex_init(&locks->notifier_lock);
> mutex_init(&locks->vram_lock);
> mutex_init(&locks->reset_lock);
> + mutex_init(&locks->filelist_mutex);
> mutex_init(&locks->srbm_mutex);
> mutex_init(&locks->grbm_idx_mutex);
> spin_lock_init(&locks->mmio_idx_lock);
> @@ -132,6 +142,7 @@ int amdgpu_lockdep_init(void)
> lockdep_set_class(&locks->vram_lock, &amdgpu_vram_lock_key);
> lockdep_set_class(&reset_domain->sem, &amdgpu_reset_sem_key);
> lockdep_set_class(&locks->reset_lock, &amdgpu_reset_lock_key);
> + lockdep_set_class(&locks->filelist_mutex, &amdgpu_filelist_mutex_key);
> lockdep_set_class(&locks->srbm_mutex, &amdgpu_srbm_lock_key);
> lockdep_set_class(&locks->grbm_idx_mutex, &amdgpu_grbm_lock_key);
> lockdep_set_class(&locks->mmio_idx_lock, &amdgpu_mmio_lock_key);
> @@ -139,6 +150,18 @@ int amdgpu_lockdep_init(void)
> * Take locks in the correct order to train lockdep.
> * This establishes the dependency chain.
> */
> + /*
> + * Train fs_reclaim FIRST, before taking any locks.
> + * This teaches lockdep that fs_reclaim is the OUTERMOST context.
> + * When memory reclaim later calls MMU notifiers (which take
> notifier_lock),
> + * lockdep will see: fs_reclaim -> notifier_lock (correct runtime
> edge).
> + *
> + * Bug fix: Previously fs_reclaim_acquire() was called while holding
> + * notifier_lock, teaching lockdep the FALSE edge: notifier_lock ->
> fs_reclaim.
> + * This caused false positive circular dependency warnings.
> + */
> + fs_reclaim_acquire(GFP_KERNEL);
> + fs_reclaim_release(GFP_KERNEL);
>
> /* Level 1: Global userq scheduler mutex (outermost) */
> mutex_lock(&locks->userq_sch_mutex);
> @@ -154,18 +177,16 @@ int amdgpu_lockdep_init(void)
>
> /* Level 6: Reset control lock */
> mutex_lock(&locks->reset_lock);
> - /*
> - * Mark potential memory reclaim boundary.
> - * GPU operations might trigger memory allocation/reclaim.
> - */
> - fs_reclaim_acquire(GFP_KERNEL);
>
> - /* Level 7: SRBM register access */
> + /* Level 7: DRM file list mutex */
> + mutex_lock(&locks->filelist_mutex);
> +
> + /* Level 8: SRBM register access */
> mutex_lock(&locks->srbm_mutex);
> - /* Level 8: GRBM index access */
> + /* Level 9: GRBM index access */
> mutex_lock(&locks->grbm_idx_mutex);
>
> - /* Level 9: MMIO index access (innermost lock, spinlock) */
> + /* Level 10: MMIO index access (innermost lock, spinlock) */
> spin_lock_irqsave(&locks->mmio_idx_lock, flags);
> /*
> * All locks acquired in order.
> @@ -176,8 +197,8 @@ int amdgpu_lockdep_init(void)
> spin_unlock_irqrestore(&locks->mmio_idx_lock, flags);
> mutex_unlock(&locks->grbm_idx_mutex);
> mutex_unlock(&locks->srbm_mutex);
> - fs_reclaim_release(GFP_KERNEL);
>
> + mutex_unlock(&locks->filelist_mutex);
> mutex_unlock(&locks->reset_lock);
> up_read(&reset_domain->sem);
>
> @@ -190,7 +211,7 @@ int amdgpu_lockdep_init(void)
> amdgpu_reset_put_reset_domain(reset_domain);
>
> kfree(locks);
> - pr_info("AMDGPU: Lockdep annotations initialized (9 lock levels)\n");
> + pr_info("AMDGPU: Lockdep annotations initialized (10 lock levels)\n");
>
> return 0;
> }
> --
> 2.54.0
>
Thanks Vitaly. Tested on amd-staging-drm-next (RX 7900 XTX, Ryzen 9 7950X)
with CONFIG_PROVE_LOCKING=y + CONFIG_DEBUG_LOCKDEP=y, via the
notifier-reclaim-splat subtest:
- without this patch: the false circular-locking-dependency splat fires
(&amdgpu_notifier_lock_key --> fs_reclaim --> mmu_notifier_invalidate_
range_start), triggered from the subtest's madvise(MADV_PAGEOUT) path;
- with this patch: no splat, and lockdep stays active for the whole boot
(no module-load report from the reworked ordering either).
For the kernel fix:
Tested-by: Mikhail Gavrilov <[email protected]>
The IGT subtest builds and behaves as intended (splat before, clean after),
so it works as a regression test.
A few small things:
- Could you s/Michael/Mikhail/ in all trailers, in both patches? The address
is right but the first name is wrong and it will end up in git history:
Reported-by / Analyzed-by / Test-case-by here, Based-on-patch-by in the IGT
patch. It should be "Mikhail Gavrilov <[email protected]>".
- Please add a Fixes: tag so this is linked to the regression:
Fixes: 1d0f5838b126 ("drm/amdgpu: Add lockdep annotations for
lock ordering validation")
- Minor, not a blocker: the moved fs_reclaim_acquire()/fs_reclaim_release()
pair does not actually create a fs_reclaim -> notifier_lock edge, since no
lock is taken between the acquire and the release; it only registers the
fs_reclaim class. The fs_reclaim -> notifier_lock ordering is established at
runtime when reclaim invokes the MMU notifier that takes notifier_lock,
independent of this annotation. So functionally this is equivalent to just
dropping the pair. The result is correct; only the comment ("teaches lockdep
that fs_reclaim is the OUTERMOST context ... lockdep will see fs_reclaim ->
notifier_lock") overstates what the annotation does. Might be worth rewording.
One process question: 1d0f5838b126 landed in the 7.2 merge window, so this is a
7.2-cycle regression, while amd-staging-drm-next targets 7.3. Would it make
sense to route this fix to the 7.2 fixes stream so 7.2 does not ship with the
false splat? The Fixes: tag would also let stable pick it up as a backstop.
Your call on the routing.
--
Thanks,
Mikhail
