[PATCH] drm/amdgpu: fix a potential circular locking dependency
Christian König
christian.koenig at amd.com
Wed Aug 12 08:56:04 UTC 2020
Am 12.08.20 um 03:33 schrieb Li, Dennis:
> [AMD Official Use Only - Internal Distribution Only]
>
> Hi, Christian,
>
> Re: I was wondering the same thing for the amdgpu_gem_va_ioctl() as well. We shouldn't have any hardware access here, so taking the reset_sem looks like overkill to me.
>
> [Dennis Li] amdgpu_vm_bo_unmap, amdgpu_vm_bo_clear_mappings, amdgpu_vm_bo_replace_map and amdgpu_gem_va_update_vm all a chance to access hardware.
This is complete nonsense. The functions intentionally work through the
scheduler to avoid accessing the hardware directly for exactly that reason.
The only hardware access we have here is the HDP flush and that can fail
in the case of a GPU reset without causing problems.
Regards,
Christian.
>
> Best Regards
> Dennis Li
> -----Original Message-----
> From: Koenig, Christian <Christian.Koenig at amd.com>
> Sent: Wednesday, August 12, 2020 12:15 AM
> To: Kuehling, Felix <Felix.Kuehling at amd.com>; Li, Dennis <Dennis.Li at amd.com>; amd-gfx at lists.freedesktop.org; Deucher, Alexander <Alexander.Deucher at amd.com>; Zhang, Hawking <Hawking.Zhang at amd.com>
> Subject: Re: [PATCH] drm/amdgpu: fix a potential circular locking dependency
>
> Am 11.08.20 um 15:57 schrieb Felix Kuehling:
>> Am 2020-08-11 um 5:32 a.m. schrieb Dennis Li:
>>> [ 653.902305] ======================================================
>>> [ 653.902928] WARNING: possible circular locking dependency detected
>>> [ 653.903517] 5.6.0-deli-v5.6-2848-g3f3109b0e75f #1 Tainted: G OE
>>> [ 653.904098] ------------------------------------------------------
>>> [ 653.904675] amdgpu_test/3975 is trying to acquire lock:
>>> [ 653.905241] ffff97848f8647a0 (&adev->reset_sem){.+.+}, at:
>>> amdgpu_gem_va_ioctl+0x286/0x4f0 [amdgpu] [ 653.905953]
>>> but task is already holding lock:
>>> [ 653.907087] ffff9744adbee1f8 (reservation_ww_class_mutex){+.+.},
>>> at: ttm_eu_reserve_buffers+0x1ae/0x520 [ttm] [ 653.907694]
>>> which lock already depends on the new lock.
>>>
>>> [ 653.909423]
>>> the existing dependency chain (in reverse order) is:
>>> [ 653.910594]
>>> -> #1 (reservation_ww_class_mutex){+.+.}:
>>> [ 653.911759] __ww_mutex_lock.constprop.15+0xca/0x1120
>>> [ 653.912350] ww_mutex_lock+0x73/0x80
>>> [ 653.913044] amdgpu_amdkfd_alloc_gtt_mem+0xde/0x380 [amdgpu]
>>> [ 653.913724] kgd2kfd_device_init+0x13f/0x5e0 [amdgpu]
>>> [ 653.914388] amdgpu_amdkfd_device_init+0x155/0x190 [amdgpu]
>>> [ 653.915033] amdgpu_device_init+0x1303/0x1e10 [amdgpu]
>>> [ 653.915685] amdgpu_driver_load_kms+0x5c/0x2c0 [amdgpu]
>>> [ 653.916349] amdgpu_pci_probe+0x11d/0x200 [amdgpu]
>>> [ 653.916959] local_pci_probe+0x47/0xa0
>>> [ 653.917570] work_for_cpu_fn+0x1a/0x30
>>> [ 653.918184] process_one_work+0x29e/0x630
>>> [ 653.918803] worker_thread+0x22b/0x3f0
>>> [ 653.919427] kthread+0x12f/0x150
>>> [ 653.920047] ret_from_fork+0x3a/0x50
>>> [ 653.920661]
>>> -> #0 (&adev->reset_sem){.+.+}:
>>> [ 653.921893] __lock_acquire+0x13ec/0x16e0
>>> [ 653.922531] lock_acquire+0xb8/0x1c0
>>> [ 653.923174] down_read+0x48/0x230
>>> [ 653.923886] amdgpu_gem_va_ioctl+0x286/0x4f0 [amdgpu]
>>> [ 653.924588] drm_ioctl_kernel+0xb6/0x100 [drm]
>>> [ 653.925283] drm_ioctl+0x389/0x450 [drm]
>>> [ 653.926013] amdgpu_drm_ioctl+0x4f/0x80 [amdgpu]
>>> [ 653.926686] ksys_ioctl+0x98/0xb0
>>> [ 653.927357] __x64_sys_ioctl+0x1a/0x20
>>> [ 653.928030] do_syscall_64+0x5f/0x250
>>> [ 653.928697] entry_SYSCALL_64_after_hwframe+0x49/0xbe
>>> [ 653.929373]
>>> other info that might help us debug this:
>>>
>>> [ 653.931356] Possible unsafe locking scenario:
>>>
>>> [ 653.932647] CPU0 CPU1
>>> [ 653.933287] ---- ----
>>> [ 653.933911] lock(reservation_ww_class_mutex);
>>> [ 653.934530] lock(&adev->reset_sem);
>>> [ 653.935154] lock(reservation_ww_class_mutex);
>>> [ 653.935766] lock(&adev->reset_sem);
>>> [ 653.936360]
>>> *** DEADLOCK ***
>>>
>>> [ 653.938028] 2 locks held by amdgpu_test/3975:
>>> [ 653.938574] #0: ffffb2a862d6bcd0
>>> (reservation_ww_class_acquire){+.+.}, at:
>>> amdgpu_gem_va_ioctl+0x39b/0x4f0 [amdgpu] [ 653.939233] #1:
>>> ffff9744adbee1f8 (reservation_ww_class_mutex){+.+.}, at:
>>> ttm_eu_reserve_buffers+0x1ae/0x520 [ttm]
>>>
>>> change the order of reservation_ww_class_mutex and adev->reset_sem in
>>> amdgpu_gem_va_ioctl the same as ones in amdgpu_amdkfd_alloc_gtt_mem,
>>> to avoid potential dead lock.
>> It may be better to fix it the other way around in
>> amdgpu_amdkfd_alloc_gtt_mem. Always take the reset_sem inside the
>> reservation. Otherwise you will never be able to take the reset_sem
>> while any BOs are reserved. That's probably going to cause you other
>> problems later.
>>
>> That makes me wonder, why do you need the reset_sem in
>> amdgpu_amdkfd_alloc_gtt_mem in the first place? There is no obvious
>> hardware access in that function. Is it for amdgpu_ttm_alloc_gart
>> updating the GART table through HDP?
> I was wondering the same thing for the amdgpu_gem_va_ioctl() as well.
>
> We shouldn't have any hardware access here, so taking the reset_sem looks like overkill to me.
>
> Christian.
>
>> Regards,
>> Felix
>>
>>
>>> Signed-off-by: Dennis Li <Dennis.Li at amd.com>
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>> index ee1e8fff83b2..fc889c477696 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>> @@ -652,6 +652,8 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data,
>>> abo = NULL;
>>> }
>>>
>>> + down_read(&adev->reset_sem);
>>> +
>>> amdgpu_vm_get_pd_bo(&fpriv->vm, &list, &vm_pd);
>>>
>>> r = ttm_eu_reserve_buffers(&ticket, &list, true, &duplicates); @@
>>> -670,8 +672,6 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data,
>>> bo_va = NULL;
>>> }
>>>
>>> - down_read(&adev->reset_sem);
>>> -
>>> switch (args->operation) {
>>> case AMDGPU_VA_OP_MAP:
>>> va_flags = amdgpu_gem_va_map_flags(adev, args->flags); @@ -701,12
>>> +701,11 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data,
>>> amdgpu_gem_va_update_vm(adev, &fpriv->vm, bo_va,
>>> args->operation);
>>>
>>> - up_read(&adev->reset_sem);
>>> -
>>> error_backoff:
>>> ttm_eu_backoff_reservation(&ticket, &list);
>>>
>>> error_unref:
>>> + up_read(&adev->reset_sem);
>>> drm_gem_object_put_unlocked(gobj);
>>> return r;
>>> }
More information about the amd-gfx
mailing list