Thank you Felix for the catch. Let's do it the cleanest way. Another reason to separate allocate_mqd from init_mqd is, the current init_mqd is actually allocate_and_init_mqd. If we separate it into two functions, the function name makes more sense.
Regards,
Oak
-----Original Message-----
From: Kuehling, Felix <Felix.Kuehling@xxxxxxx>
Sent: Monday, June 3, 2019 6:27 PM
To: Zeng, Oak <Oak.Zeng@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx
Subject: Re: [PATCH 4/5] drm/amdkfd: Fix a circular lock dependency
allocate_vmid, allocate_hqd and allocate_sdma_queue all work on data in the DQM structure. So it seems these need to be protected by the DQM lock. allocate_doorbell doesn't need the DQM lock because its data structures are in the process_device structure, which is protected by the process lock.
What's different in the cpsch case is, that we don't need allocate_vmid and allocate_hqd. The only thing that was broken there accidentally by moving the DQM lock to later, is allocate_sdma_queue, which you're addressing in the next patch.
If you move the DQM lock in create_queue_nocpsch, you'll need to fix up DQM locking in allocate_vmid, allocate_hqd and allocate_sdma_queue in the next change. I think the easier solution is to do create_queue_nocpsch with two critical sections protected by the DQM lock and do the init_mqd between the two critical sections.
Alternatively you could separate allocate_mqd from init_mqd, so that you can do the MQD allocation before you take the DQM lock and the MQD initialization safely under the DQM lock. That way the create queue functions would each have only one critical section. I think that would be the cleanest solution.
Regards,
Felix
On 2019-06-03 1:51 p.m., Zeng, Oak wrote:
> The idea to break the circular lock dependency is to move init_mqd out
> of lock protection of dqm lock in callstack #1 below. There is no need
> to.
>
> [ 59.510149] [drm] Initialized amdgpu 3.30.0 20150101 for 0000:04:00.0 on minor 0
>
> [ 513.604034] ======================================================
> [ 513.604205] WARNING: possible circular locking dependency detected
> [ 513.604375] 4.18.0-kfd-root #2 Tainted: G W
> [ 513.604530] ------------------------------------------------------
> [ 513.604699] kswapd0/611 is trying to acquire lock:
> [ 513.604840] 00000000d254022e (&dqm->lock_hidden){+.+.}, at:
> evict_process_queues_nocpsch+0x26/0x140 [amdgpu] [ 513.605150]
> but task is already holding lock:
> [ 513.605307] 00000000961547fc (&anon_vma->rwsem){++++}, at:
> page_lock_anon_vma_read+0xe4/0x250
> [ 513.605540]
> which lock already depends on the new lock.
>
> [ 513.605747]
> the existing dependency chain (in reverse order) is:
> [ 513.605944]
> -> #4 (&anon_vma->rwsem){++++}:
> [ 513.606106] __vma_adjust+0x147/0x7f0
> [ 513.606231] __split_vma+0x179/0x190
> [ 513.606353] mprotect_fixup+0x217/0x260
> [ 513.606553] do_mprotect_pkey+0x211/0x380
> [ 513.606752] __x64_sys_mprotect+0x1b/0x20
> [ 513.606954] do_syscall_64+0x50/0x1a0
> [ 513.607149] entry_SYSCALL_64_after_hwframe+0x49/0xbe
> [ 513.607380]
> -> #3 (&mapping->i_mmap_rwsem){++++}:
> [ 513.607678] rmap_walk_file+0x1f0/0x280
> [ 513.607887] page_referenced+0xdd/0x180
> [ 513.608081] shrink_page_list+0x853/0xcb0
> [ 513.608279] shrink_inactive_list+0x33b/0x700
> [ 513.608483] shrink_node_memcg+0x37a/0x7f0
> [ 513.608682] shrink_node+0xd8/0x490
> [ 513.608869] balance_pgdat+0x18b/0x3b0
> [ 513.609062] kswapd+0x203/0x5c0
> [ 513.609241] kthread+0x100/0x140
> [ 513.609420] ret_from_fork+0x24/0x30
> [ 513.609607]
> -> #2 (fs_reclaim){+.+.}:
> [ 513.609883] kmem_cache_alloc_trace+0x34/0x2e0
> [ 513.610093] reservation_object_reserve_shared+0x139/0x300
> [ 513.610326] ttm_bo_init_reserved+0x291/0x480 [ttm]
> [ 513.610567] amdgpu_bo_do_create+0x1d2/0x650 [amdgpu]
> [ 513.610811] amdgpu_bo_create+0x40/0x1f0 [amdgpu]
> [ 513.611041] amdgpu_bo_create_reserved+0x249/0x2d0 [amdgpu]
> [ 513.611290] amdgpu_bo_create_kernel+0x12/0x70 [amdgpu]
> [ 513.611584] amdgpu_ttm_init+0x2cb/0x560 [amdgpu]
> [ 513.611823] gmc_v9_0_sw_init+0x400/0x750 [amdgpu]
> [ 513.612491] amdgpu_device_init+0x14eb/0x1990 [amdgpu]
> [ 513.612730] amdgpu_driver_load_kms+0x78/0x290 [amdgpu]
> [ 513.612958] drm_dev_register+0x111/0x1a0
> [ 513.613171] amdgpu_pci_probe+0x11c/0x1e0 [amdgpu]
> [ 513.613389] local_pci_probe+0x3f/0x90
> [ 513.613581] pci_device_probe+0x102/0x1c0
> [ 513.613779] driver_probe_device+0x2a7/0x480
> [ 513.613984] __driver_attach+0x10a/0x110
> [ 513.614179] bus_for_each_dev+0x67/0xc0
> [ 513.614372] bus_add_driver+0x1eb/0x260
> [ 513.614565] driver_register+0x5b/0xe0
> [ 513.614756] do_one_initcall+0xac/0x357
> [ 513.614952] do_init_module+0x5b/0x213
> [ 513.615145] load_module+0x2542/0x2d30
> [ 513.615337] __do_sys_finit_module+0xd2/0x100
> [ 513.615541] do_syscall_64+0x50/0x1a0
> [ 513.615731] entry_SYSCALL_64_after_hwframe+0x49/0xbe
> [ 513.615963]
> -> #1 (reservation_ww_class_mutex){+.+.}:
> [ 513.616293] amdgpu_amdkfd_alloc_gtt_mem+0xcf/0x2c0 [amdgpu]
> [ 513.616554] init_mqd+0x223/0x260 [amdgpu]
> [ 513.616779] create_queue_nocpsch+0x4d9/0x600 [amdgpu]
> [ 513.617031] pqm_create_queue+0x37c/0x520 [amdgpu]
> [ 513.617270] kfd_ioctl_create_queue+0x2f9/0x650 [amdgpu]
> [ 513.617522] kfd_ioctl+0x202/0x350 [amdgpu]
> [ 513.617724] do_vfs_ioctl+0x9f/0x6c0
> [ 513.617914] ksys_ioctl+0x66/0x70
> [ 513.618095] __x64_sys_ioctl+0x16/0x20
> [ 513.618286] do_syscall_64+0x50/0x1a0
> [ 513.618476] entry_SYSCALL_64_after_hwframe+0x49/0xbe
> [ 513.618695]
> -> #0 (&dqm->lock_hidden){+.+.}:
> [ 513.618984] __mutex_lock+0x98/0x970
> [ 513.619197] evict_process_queues_nocpsch+0x26/0x140 [amdgpu]
> [ 513.619459] kfd_process_evict_queues+0x3b/0xb0 [amdgpu]
> [ 513.619710] kgd2kfd_quiesce_mm+0x1c/0x40 [amdgpu]
> [ 513.620103] amdgpu_amdkfd_evict_userptr+0x38/0x70 [amdgpu]
> [ 513.620363] amdgpu_mn_invalidate_range_start_hsa+0xa6/0xc0 [amdgpu]
> [ 513.620614] __mmu_notifier_invalidate_range_start+0x70/0xb0
> [ 513.620851] try_to_unmap_one+0x7fc/0x8f0
> [ 513.621049] rmap_walk_anon+0x121/0x290
> [ 513.621242] try_to_unmap+0x93/0xf0
> [ 513.621428] shrink_page_list+0x606/0xcb0
> [ 513.621625] shrink_inactive_list+0x33b/0x700
> [ 513.621835] shrink_node_memcg+0x37a/0x7f0
> [ 513.622034] shrink_node+0xd8/0x490
> [ 513.622219] balance_pgdat+0x18b/0x3b0
> [ 513.622410] kswapd+0x203/0x5c0
> [ 513.622589] kthread+0x100/0x140
> [ 513.622769] ret_from_fork+0x24/0x30
> [ 513.622957]
> other info that might help us debug this:
>
> [ 513.623354] Chain exists of:
> &dqm->lock_hidden --> &mapping->i_mmap_rwsem -->
> &anon_vma->rwsem
>
> [ 513.623900] Possible unsafe locking scenario:
>
> [ 513.624189] CPU0 CPU1
> [ 513.624397] ---- ----
> [ 513.624594] lock(&anon_vma->rwsem);
> [ 513.624771] lock(&mapping->i_mmap_rwsem);
> [ 513.625020] lock(&anon_vma->rwsem);
> [ 513.625253] lock(&dqm->lock_hidden);
> [ 513.625433]
> *** DEADLOCK ***
>
> [ 513.625783] 3 locks held by kswapd0/611:
> [ 513.625967] #0: 00000000f14edf84 (fs_reclaim){+.+.}, at:
> __fs_reclaim_acquire+0x5/0x30 [ 513.626309] #1: 00000000961547fc
> (&anon_vma->rwsem){++++}, at: page_lock_anon_vma_read+0xe4/0x250
> [ 513.626671] #2: 0000000067b5cd12 (srcu){....}, at:
> __mmu_notifier_invalidate_range_start+0x5/0xb0
> [ 513.627037]
> stack backtrace:
> [ 513.627292] CPU: 0 PID: 611 Comm: kswapd0 Tainted: G W 4.18.0-kfd-root #2
> [ 513.627632] Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS
> VirtualBox 12/01/2006 [ 513.627990] Call Trace:
> [ 513.628143] dump_stack+0x7c/0xbb
> [ 513.628315] print_circular_bug.isra.37+0x21b/0x228
> [ 513.628581] __lock_acquire+0xf7d/0x1470 [ 513.628782] ?
> unwind_next_frame+0x6c/0x4f0 [ 513.628974] ? lock_acquire+0xec/0x1e0
> [ 513.629154] lock_acquire+0xec/0x1e0 [ 513.629357] ?
> evict_process_queues_nocpsch+0x26/0x140 [amdgpu] [ 513.629587]
> __mutex_lock+0x98/0x970 [ 513.629790] ?
> evict_process_queues_nocpsch+0x26/0x140 [amdgpu] [ 513.630047] ?
> evict_process_queues_nocpsch+0x26/0x140 [amdgpu] [ 513.630309] ?
> evict_process_queues_nocpsch+0x26/0x140 [amdgpu] [ 513.630562]
> evict_process_queues_nocpsch+0x26/0x140 [amdgpu] [ 513.630816]
> kfd_process_evict_queues+0x3b/0xb0 [amdgpu] [ 513.631057]
> kgd2kfd_quiesce_mm+0x1c/0x40 [amdgpu] [ 513.631288]
> amdgpu_amdkfd_evict_userptr+0x38/0x70 [amdgpu] [ 513.631536]
> amdgpu_mn_invalidate_range_start_hsa+0xa6/0xc0 [amdgpu] [ 513.632076]
> __mmu_notifier_invalidate_range_start+0x70/0xb0
> [ 513.632299] try_to_unmap_one+0x7fc/0x8f0 [ 513.632487] ?
> page_lock_anon_vma_read+0x68/0x250
> [ 513.632690] rmap_walk_anon+0x121/0x290 [ 513.632875]
> try_to_unmap+0x93/0xf0 [ 513.633050] ? page_remove_rmap+0x330/0x330
> [ 513.633239] ? rcu_read_unlock+0x60/0x60 [ 513.633422] ?
> page_get_anon_vma+0x160/0x160 [ 513.633613]
> shrink_page_list+0x606/0xcb0 [ 513.633800]
> shrink_inactive_list+0x33b/0x700 [ 513.633997]
> shrink_node_memcg+0x37a/0x7f0 [ 513.634186] ? shrink_node+0xd8/0x490
> [ 513.634363] shrink_node+0xd8/0x490 [ 513.634537]
> balance_pgdat+0x18b/0x3b0 [ 513.634718] kswapd+0x203/0x5c0 [
> 513.634887] ? wait_woken+0xb0/0xb0 [ 513.635062]
> kthread+0x100/0x140 [ 513.635231] ? balance_pgdat+0x3b0/0x3b0 [
> 513.635414] ? kthread_delayed_work_timer_fn+0x80/0x80
> [ 513.635626] ret_from_fork+0x24/0x30 [ 513.636042] Evicting PASID
> 32768 queues [ 513.936236] Restoring PASID 32768 queues [
> 524.708912] Evicting PASID 32768 queues [ 524.999875] Restoring PASID
> 32768 queues
>
> Change-Id: Ib40b5fb8d7c6572888a0c879a315b382eb948647
> Signed-off-by: Oak Zeng <Oak.Zeng@xxxxxxx>
> ---
> drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c | 13 ++++++-------
> 1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> index 06654de..39e2d6d 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> @@ -271,19 +271,17 @@ static int create_queue_nocpsch(struct
> device_queue_manager *dqm,
>
> print_queue(q);
>
> - dqm_lock(dqm);
> -
> if (dqm->total_queue_count >= max_num_of_queues_per_device) {
> pr_warn("Can't create new usermode queue because %d queues were already created\n",
> dqm->total_queue_count);
> retval = -EPERM;
> - goto out_unlock;
> + goto out;
> }
>
> if (list_empty(&qpd->queues_list)) {
> retval = allocate_vmid(dqm, qpd, q);
> if (retval)
> - goto out_unlock;
> + goto out;
> }
> q->properties.vmid = qpd->vmid;
> /*
> @@ -340,6 +338,7 @@ static int create_queue_nocpsch(struct device_queue_manager *dqm,
> goto out_uninit_mqd;
> }
>
> + dqm_lock(dqm);
> list_add(&q->list, &qpd->queues_list);
> qpd->queue_count++;
> if (q->properties.is_active)
> @@ -357,7 +356,8 @@ static int create_queue_nocpsch(struct device_queue_manager *dqm,
> dqm->total_queue_count++;
> pr_debug("Total of %d queues are accountable so far\n",
> dqm->total_queue_count);
> - goto out_unlock;
> + dqm_unlock(dqm);
> + return retval;
>
> out_uninit_mqd:
> mqd_mgr->uninit_mqd(mqd_mgr, q->mqd, q->mqd_mem_obj); @@ -372,8
> +372,7 @@ static int create_queue_nocpsch(struct device_queue_manager *dqm,
> deallocate_vmid:
> if (list_empty(&qpd->queues_list))
> deallocate_vmid(dqm, qpd, q);
> -out_unlock:
> - dqm_unlock(dqm);
> +out:
> return retval;
> }
>
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
