[PATCH] drm/amdkfd: Fix Circular Locking Dependency in 'svm_range_cpu_invalidate_pagetables'
SRINIVASAN SHANMUGAM
srinivasan.shanmugam at amd.com
Thu Feb 20 05:28:39 UTC 2025
On 2/20/2025 10:25 AM, Felix Kuehling wrote:
> On 2025-02-18 15:51, Philip Yang wrote:
>> On 2025-02-18 11:01, Srinivasan Shanmugam wrote:
>>> This commit addresses a circular locking dependency in the
>>> svm_range_cpu_invalidate_pagetables function. The function previously
>>> held a lock while determining whether to perform an unmap or eviction
>>> operation, which could lead to deadlocks.
>>>
>>> To resolve this issue, a flag named `needs_unmap_or_evict` has been
>>> introduced to indicate if an unmap or eviction operation is required.
>>> The function now unlocks the `prange` lock before performing the
>>> necessary operations, ensuring that the critical section is minimized
>>> and preventing circular dependencies.
>>>
>>> Fixes the below:
>>>
>>> [ 223.418794] ======================================================
>>> [ 223.418820] WARNING: possible circular locking dependency detected
>>> [ 223.418845] 6.12.0-amdstaging-drm-next-lol-050225 #14 Tainted: G U OE
>>> [ 223.418869] ------------------------------------------------------
>>> [ 223.418889] kfdtest/3939 is trying to acquire lock:
>>> [ 223.418906] ffff8957552eae38 (&dqm->lock_hidden){+.+.}-{3:3}, at: evict_process_queues_cpsch+0x43/0x210 [amdgpu]
>>> [ 223.419302]
>>> but task is already holding lock:
>>> [ 223.419303] ffff8957556b83b0 (&prange->lock){+.+.}-{3:3}, at: svm_range_cpu_invalidate_pagetables+0x9d/0x850 [amdgpu]
>>> [ 223.419447] Console: switching to colour dummy device 80x25
>>> [ 223.419477] [IGT] amd_basic: executing
>>> [ 223.419599]
>>> which lock already depends on the new lock.
>>>
>>> [ 223.419611]
>>> the existing dependency chain (in reverse order) is:
>>> [ 223.419621]
>>> -> #2 (&prange->lock){+.+.}-{3:3}:
>>> [ 223.419636] __mutex_lock+0x85/0xe20
>>> [ 223.419647] mutex_lock_nested+0x1b/0x30
>>> [ 223.419656] svm_range_validate_and_map+0x2f1/0x15b0 [amdgpu]
>>> [ 223.419954] svm_range_set_attr+0xe8c/0x1710 [amdgpu]
>>> [ 223.420236] svm_ioctl+0x46/0x50 [amdgpu]
>>> [ 223.420503] kfd_ioctl_svm+0x50/0x90 [amdgpu]
>>> [ 223.420763] kfd_ioctl+0x409/0x6d0 [amdgpu]
>>> [ 223.421024] __x64_sys_ioctl+0x95/0xd0
>>> [ 223.421036] x64_sys_call+0x1205/0x20d0
>>> [ 223.421047] do_syscall_64+0x87/0x140
>>> [ 223.421056] entry_SYSCALL_64_after_hwframe+0x76/0x7e
>>> [ 223.421068]
>>> -> #1 (reservation_ww_class_mutex){+.+.}-{3:3}:
>>> [ 223.421084] __ww_mutex_lock.constprop.0+0xab/0x1560
>>> [ 223.421095] ww_mutex_lock+0x2b/0x90
>>> [ 223.421103] amdgpu_amdkfd_alloc_gtt_mem+0xcc/0x2b0 [amdgpu]
>>> [ 223.421361] add_queue_mes+0x3bc/0x440 [amdgpu]
>>> [ 223.421623] unhalt_cpsch+0x1ae/0x240 [amdgpu]
>>> [ 223.421888] kgd2kfd_start_sched+0x5e/0xd0 [amdgpu]
>>> [ 223.422148] amdgpu_amdkfd_start_sched+0x3d/0x50 [amdgpu]
>>> [ 223.422414] amdgpu_gfx_enforce_isolation_handler+0x132/0x270 [amdgpu]
>>> [ 223.422662] process_one_work+0x21e/0x680
>>> [ 223.422673] worker_thread+0x190/0x330
>>> [ 223.422682] kthread+0xe7/0x120
>>> [ 223.422690] ret_from_fork+0x3c/0x60
>>> [ 223.422699] ret_from_fork_asm+0x1a/0x30
>>> [ 223.422708]
>> This hold dqm_lock, then allocate pdd->proc_ctx_bo and map to GPU, it is illegal usage.
> Good catch. The problem was introduced by this patch after a suggestion I made in a previous code review, to allocate this buffer in a lazy fashion when the first queue is created.
>
> commit 3e5199134e47745256c3be448ca466d06acaebde
> Author: Jesse.zhang at amd.com <Jesse.zhang at amd.com>
> Date: Thu Dec 5 17:41:26 2024 +0800
>
> drm/amdkfd: pause autosuspend when creating pdd
>
> When using MES creating a pdd will require talking to the GPU to
> setup the relevant context. The code here forgot to wake up the GPU
> in case it was in suspend, this causes KVM to EFAULT for passthrough
> GPU for example. This issue can be masked if the GPU was woken up by
> other things (e.g. opening the KMS node) first and have not yet gone to sleep.
>
> v4: do the allocation of proc_ctx_bo in a lazy fashion
> when the first queue is created in a process (Felix)
>
> Signed-off-by: Jesse Zhang <jesse.zhang at amd.com>
> Reviewed-by: Yunxiang Li <Yunxiang.Li at amd.com>
>
> I guess a better approach is to allocate this buffer somewhere in pqm_create_queue when the first queue of the process is created, rather than when it is added to the MES scheduler. That should be outside the DQM lock. Do it with a condition that applies only when MES is enabled.
>
> Regards,
> Felix
>
thanks for all your feedback's, will go through all your feedback's,
analyze, and propose v2 again.
Regards,
Srini
>> Not sure but seems pdd->proc_ctx_bo/proc_ctx_gpu_addr is only needed by debug mode path, as kfd_dbg_set_mes_debug_mode also allocate proc_ctx_bo, the proc_ctx_bo allocation should remove from add_queue_mes, and move the allocation into kfd_create_process_device_data if this is also needed for non debug_mode path.
>>
>>> -> #0 (&dqm->lock_hidden){+.+.}-{3:3}:
>>> [ 223.422723] __lock_acquire+0x16f4/0x2810
>>> [ 223.422734] lock_acquire+0xd1/0x300
>>> [ 223.422742] __mutex_lock+0x85/0xe20
>>> [ 223.422751] mutex_lock_nested+0x1b/0x30
>>> [ 223.422760] evict_process_queues_cpsch+0x43/0x210 [amdgpu]
>>> [ 223.423025] kfd_process_evict_queues+0x8a/0x1d0 [amdgpu]
>>> [ 223.423285] kgd2kfd_quiesce_mm+0x43/0x90 [amdgpu]
>>> [ 223.423540] svm_range_cpu_invalidate_pagetables+0x4a7/0x850 [amdgpu]
>>> [ 223.423807] __mmu_notifier_invalidate_range_start+0x1f5/0x250
>>> [ 223.423819] copy_page_range+0x1e94/0x1ea0
>>> [ 223.423829] copy_process+0x172f/0x2ad0
>>> [ 223.423839] kernel_clone+0x9c/0x3f0
>>> [ 223.423847] __do_sys_clone+0x66/0x90
>>> [ 223.423856] __x64_sys_clone+0x25/0x30
>>> [ 223.423864] x64_sys_call+0x1d7c/0x20d0
>>> [ 223.423872] do_syscall_64+0x87/0x140
>>> [ 223.423880] entry_SYSCALL_64_after_hwframe+0x76/0x7e
>>> [ 223.423891]
>>> other info that might help us debug this:
>>>
>>> [ 223.423903] Chain exists of:
>>> &dqm->lock_hidden --> reservation_ww_class_mutex --> &prange->lock
>>>
>>> [ 223.423926] Possible unsafe locking scenario:
>>>
>>> [ 223.423935] CPU0 CPU1
>>> [ 223.423942] ---- ----
>>> [ 223.423949] lock(&prange->lock);
>>> [ 223.423958] lock(reservation_ww_class_mutex);
>>> [ 223.423970] lock(&prange->lock);
>>> [ 223.423981] lock(&dqm->lock_hidden);
>>> [ 223.423990]
>>> *** DEADLOCK ***
>>>
>>> [ 223.423999] 5 locks held by kfdtest/3939:
>>> [ 223.424006] #0: ffffffffb82b4fc0 (dup_mmap_sem){.+.+}-{0:0}, at: copy_process+0x1387/0x2ad0
>>> [ 223.424026] #1: ffff89575eda81b0 (&mm->mmap_lock){++++}-{3:3}, at: copy_process+0x13a8/0x2ad0
>>> [ 223.424046] #2: ffff89575edaf3b0 (&mm->mmap_lock/1){+.+.}-{3:3}, at: copy_process+0x13e4/0x2ad0
>>> [ 223.424066] #3: ffffffffb82e76e0 (mmu_notifier_invalidate_range_start){+.+.}-{0:0}, at: copy_page_range+0x1cea/0x1ea0
>>> [ 223.424088] #4: ffff8957556b83b0 (&prange->lock){+.+.}-{3:3}, at: svm_range_cpu_invalidate_pagetables+0x9d/0x850 [amdgpu]
>>> [ 223.424365]
>>> stack backtrace:
>>> [ 223.424374] CPU: 0 UID: 0 PID: 3939 Comm: kfdtest Tainted: G U OE 6.12.0-amdstaging-drm-next-lol-050225 #14
>>> [ 223.424392] Tainted: [U]=USER, [O]=OOT_MODULE, [E]=UNSIGNED_MODULE
>>> [ 223.424401] Hardware name: Gigabyte Technology Co., Ltd. X570 AORUS PRO WIFI/X570 AORUS PRO WIFI, BIOS F36a 02/16/2022
>>> [ 223.424416] Call Trace:
>>> [ 223.424423] <TASK>
>>> [ 223.424430] dump_stack_lvl+0x9b/0xf0
>>> [ 223.424441] dump_stack+0x10/0x20
>>> [ 223.424449] print_circular_bug+0x275/0x350
>>> [ 223.424460] check_noncircular+0x157/0x170
>>> [ 223.424469] ? __bfs+0xfd/0x2c0
>>> [ 223.424481] __lock_acquire+0x16f4/0x2810
>>> [ 223.424490] ? srso_return_thunk+0x5/0x5f
>>> [ 223.424505] lock_acquire+0xd1/0x300
>>> [ 223.424514] ? evict_process_queues_cpsch+0x43/0x210 [amdgpu]
>>> [ 223.424783] __mutex_lock+0x85/0xe20
>>> [ 223.424792] ? evict_process_queues_cpsch+0x43/0x210 [amdgpu]
>>> [ 223.425058] ? srso_return_thunk+0x5/0x5f
>>> [ 223.425067] ? mark_held_locks+0x54/0x90
>>> [ 223.425076] ? evict_process_queues_cpsch+0x43/0x210 [amdgpu]
>>> [ 223.425339] ? srso_return_thunk+0x5/0x5f
>>> [ 223.425350] mutex_lock_nested+0x1b/0x30
>>> [ 223.425358] ? mutex_lock_nested+0x1b/0x30
>>> [ 223.425367] evict_process_queues_cpsch+0x43/0x210 [amdgpu]
>>> [ 223.425631] kfd_process_evict_queues+0x8a/0x1d0 [amdgpu]
>>> [ 223.425893] kgd2kfd_quiesce_mm+0x43/0x90 [amdgpu]
>>> [ 223.426156] svm_range_cpu_invalidate_pagetables+0x4a7/0x850 [amdgpu]
>>> [ 223.426423] ? srso_return_thunk+0x5/0x5f
>>> [ 223.426436] __mmu_notifier_invalidate_range_start+0x1f5/0x250
>>> [ 223.426450] copy_page_range+0x1e94/0x1ea0
>>> [ 223.426461] ? srso_return_thunk+0x5/0x5f
>>> [ 223.426474] ? srso_return_thunk+0x5/0x5f
>>> [ 223.426484] ? lock_acquire+0xd1/0x300
>>> [ 223.426494] ? copy_process+0x1718/0x2ad0
>>> [ 223.426502] ? srso_return_thunk+0x5/0x5f
>>> [ 223.426510] ? sched_clock_noinstr+0x9/0x10
>>> [ 223.426519] ? local_clock_noinstr+0xe/0xc0
>>> [ 223.426528] ? copy_process+0x1718/0x2ad0
>>> [ 223.426537] ? srso_return_thunk+0x5/0x5f
>>> [ 223.426550] copy_process+0x172f/0x2ad0
>>> [ 223.426569] kernel_clone+0x9c/0x3f0
>>> [ 223.426577] ? __schedule+0x4c9/0x1b00
>>> [ 223.426586] ? srso_return_thunk+0x5/0x5f
>>> [ 223.426594] ? sched_clock_noinstr+0x9/0x10
>>> [ 223.426602] ? srso_return_thunk+0x5/0x5f
>>> [ 223.426610] ? local_clock_noinstr+0xe/0xc0
>>> [ 223.426619] ? schedule+0x107/0x1a0
>>> [ 223.426629] __do_sys_clone+0x66/0x90
>>> [ 223.426643] __x64_sys_clone+0x25/0x30
>>> [ 223.426652] x64_sys_call+0x1d7c/0x20d0
>>> [ 223.426661] do_syscall_64+0x87/0x140
>>> [ 223.426671] ? srso_return_thunk+0x5/0x5f
>>> [ 223.426679] ? common_nsleep+0x44/0x50
>>> [ 223.426690] ? srso_return_thunk+0x5/0x5f
>>> [ 223.426698] ? trace_hardirqs_off+0x52/0xd0
>>> [ 223.426709] ? srso_return_thunk+0x5/0x5f
>>> [ 223.426717] ? syscall_exit_to_user_mode+0xcc/0x200
>>> [ 223.426727] ? srso_return_thunk+0x5/0x5f
>>> [ 223.426736] ? do_syscall_64+0x93/0x140
>>> [ 223.426748] ? srso_return_thunk+0x5/0x5f
>>> [ 223.426756] ? up_write+0x1c/0x1e0
>>> [ 223.426765] ? srso_return_thunk+0x5/0x5f
>>> [ 223.426775] ? srso_return_thunk+0x5/0x5f
>>> [ 223.426783] ? trace_hardirqs_off+0x52/0xd0
>>> [ 223.426792] ? srso_return_thunk+0x5/0x5f
>>> [ 223.426800] ? syscall_exit_to_user_mode+0xcc/0x200
>>> [ 223.426810] ? srso_return_thunk+0x5/0x5f
>>> [ 223.426818] ? do_syscall_64+0x93/0x140
>>> [ 223.426826] ? syscall_exit_to_user_mode+0xcc/0x200
>>> [ 223.426836] ? srso_return_thunk+0x5/0x5f
>>> [ 223.426844] ? do_syscall_64+0x93/0x140
>>> [ 223.426853] ? srso_return_thunk+0x5/0x5f
>>> [ 223.426861] ? irqentry_exit+0x6b/0x90
>>> [ 223.426869] ? srso_return_thunk+0x5/0x5f
>>> [ 223.426877] ? exc_page_fault+0xa7/0x2c0
>>> [ 223.426888] entry_SYSCALL_64_after_hwframe+0x76/0x7e
>>> [ 223.426898] RIP: 0033:0x7f46758eab57
>>> [ 223.426906] Code: ba 04 00 f3 0f 1e fa 64 48 8b 04 25 10 00 00 00 45 31 c0 31 d2 31 f6 bf 11 00 20 01 4c 8d 90 d0 02 00 00 b8 38 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 41 41 89 c0 85 c0 75 2c 64 48 8b 04 25 10 00
>>> [ 223.426930] RSP: 002b:00007fff5c3e5188 EFLAGS: 00000246 ORIG_RAX: 0000000000000038
>>> [ 223.426943] RAX: ffffffffffffffda RBX: 00007f4675f8c040 RCX: 00007f46758eab57
>>> [ 223.426954] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000001200011
>>> [ 223.426965] RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000000
>>> [ 223.426975] R10: 00007f4675e81a50 R11: 0000000000000246 R12: 0000000000000001
>>> [ 223.426986] R13: 00007fff5c3e5470 R14: 00007fff5c3e53e0 R15: 00007fff5c3e5410
>>> [ 223.427004] </TASK>
>>>
>>> Fixes: 4683cfecadeb ("drm/amdkfd: deregister svm range")
>>> Cc: Philip Yang <Philip.Yang at amd.com>
>>> Cc: Alex Sierra <alex.sierra at amd.com>
>>> Cc: Felix Kuehling <Felix.Kuehling at amd.com>
>>> Cc: Christian König <christian.koenig at amd.com>
>>> Cc: Alex Deucher <alexander.deucher at amd.com>
>>> Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam at amd.com>
>>> ---
>>> drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 14 +++++++++++---
>>> 1 file changed, 11 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>>> index db3034b00dac..a076269cce7f 100644
>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>>> @@ -2574,6 +2574,7 @@ svm_range_cpu_invalidate_pagetables(struct mmu_interval_notifier *mni,
>>> struct svm_range *prange;
>>> unsigned long start;
>>> unsigned long last;
>>> + bool needs_unmap_or_evict = false;
>>> if (range->event == MMU_NOTIFY_RELEASE)
>>> return true;
>>> @@ -2597,14 +2598,21 @@ svm_range_cpu_invalidate_pagetables(struct mmu_interval_notifier *mni,
>>> switch (range->event) {
>>> case MMU_NOTIFY_UNMAP:
>>> - svm_range_unmap_from_cpu(mni->mm, prange, start, last);
>>> + needs_unmap_or_evict = true;
>>> break;
>>> default:
>>> - svm_range_evict(prange, mni->mm, start, last, range->event);
>>> + needs_unmap_or_evict = true;
>>> break;
>>> }
>>> -
>>> svm_range_unlock(prange);
>>> +
>>> + if (needs_unmap_or_evict) {
>>> + if (range->event == MMU_NOTIFY_UNMAP)
>>> + svm_range_unmap_from_cpu(mni->mm, prange, start, last);
>> This is incorrect, we should hold prange lock to split prange.
>>
>> Regards,
>>
>> Philip
>>
>>> + else
>>> + svm_range_evict(prange, mni->mm, start, last, range->event);
>>> + }
>>> +
>>> mmput(mni->mm);
>>> return true;
More information about the amd-gfx
mailing list