I think it is a limitation of init_rwsem.
And exactly that's wrong, this is intentional and perfectly correct.
As far as I know it is perfectly possible that the locks in the hive are
not always grabbed in the same order. And that's why lockdep is
complaining about this.
What we should do instead is to make sure we have only a single lock for
the complete hive instead.
Regards,
Christian.
Am 06.08.20 um 11:15 schrieb Li, Dennis:
[AMD Official Use Only - Internal Distribution Only]
Hi, Christian,
For this case, it is safe to use separated lock key. Please see the definition of init_rwsem as the below. Every init_rwsem calling will use a new static key, but devices of the hive share the same code to do initialization, so their lock_class_key are the same. I think it is a limitation of init_rwsem. In our case, it should be correct that reset_sem of each adev has different lock_class_key. BTW, this change doesn't effect dead-lock detection and just correct it.
#define init_rwsem(sem) \
do { \
static struct lock_class_key __key; \
\
__init_rwsem((sem), #sem, &__key); \
} while (0)
Best Regards
Dennis Li
-----Original Message-----
From: Koenig, Christian <Christian.Koenig@xxxxxxx>
Sent: Thursday, August 6, 2020 4:13 PM
To: Li, Dennis <Dennis.Li@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx; Deucher, Alexander <Alexander.Deucher@xxxxxxx>; Kuehling, Felix <Felix.Kuehling@xxxxxxx>; Zhang, Hawking <Hawking.Zhang@xxxxxxx>
Subject: Re: [PATCH] drm/amdgpu: annotate a false positive recursive locking
Preventing locking problems during implementation is obviously a good approach, but lockdep has proven to be massively useful for finding and fixing problems.
Disabling lockdep splat by annotating lock with separate classes is usually a no-go and only allowed if there is no other potential approach.
In this case here we should really clean things up instead.
Christian.
Am 06.08.20 um 09:44 schrieb Li, Dennis:
[AMD Official Use Only - Internal Distribution Only]
Hi, Christian,
I agree with your concern. However we shouldn't rely on system to detect dead-lock, and should consider this when doing code implementation. In fact, dead-lock detection isn't enabled by default.
For your proposal to remove reset_sem into the hive structure, we can open a new topic to discuss it. Currently we couldn't make sure which is the best solution. For example, with your proposal, we must wait for all devices resuming successfully before resubmit an old task in one device, which will effect performance.
Best Regards
Dennis Li
-----Original Message-----
From: amd-gfx <amd-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of
Christian König
Sent: Thursday, August 6, 2020 3:08 PM
To: Li, Dennis <Dennis.Li@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx;
Deucher, Alexander <Alexander.Deucher@xxxxxxx>; Kuehling, Felix
<Felix.Kuehling@xxxxxxx>; Zhang, Hawking <Hawking.Zhang@xxxxxxx>
Subject: Re: [PATCH] drm/amdgpu: annotate a false positive recursive
locking
Am 06.08.20 um 09:02 schrieb Dennis Li:
[ 584.110304] ============================================
[ 584.110590] WARNING: possible recursive locking detected
[ 584.110876] 5.6.0-deli-v5.6-2848-g3f3109b0e75f #1 Tainted: G OE
[ 584.111164] --------------------------------------------
[ 584.111456] kworker/38:1/553 is trying to acquire lock:
[ 584.111721] ffff9b15ff0a47a0 (&adev->reset_sem){++++}, at:
amdgpu_device_gpu_recover+0x262/0x1030 [amdgpu] [ 584.112112]
but task is already holding lock:
[ 584.112673] ffff9b1603d247a0 (&adev->reset_sem){++++}, at:
amdgpu_device_gpu_recover+0x262/0x1030 [amdgpu] [ 584.113068]
other info that might help us debug this:
[ 584.113689] Possible unsafe locking scenario:
[ 584.114350] CPU0
[ 584.114685] ----
[ 584.115014] lock(&adev->reset_sem);
[ 584.115349] lock(&adev->reset_sem);
[ 584.115678]
*** DEADLOCK ***
[ 584.116624] May be due to missing lock nesting notation
[ 584.117284] 4 locks held by kworker/38:1/553:
[ 584.117616] #0: ffff9ad635c1d348 ((wq_completion)events){+.+.},
at: process_one_work+0x21f/0x630 [ 584.117967] #1: ffffac708e1c3e58
((work_completion)(&con->recovery_work)){+.+.}, at:
process_one_work+0x21f/0x630 [ 584.118358] #2: ffffffffc1c2a5d0
(&tmp->hive_lock){+.+.}, at: amdgpu_device_gpu_recover+0xae/0x1030 [amdgpu] [ 584.118786] #3: ffff9b1603d247a0 (&adev->reset_sem){++++}, at: amdgpu_device_gpu_recover+0x262/0x1030 [amdgpu] [ 584.119222]
stack backtrace:
[ 584.119990] CPU: 38 PID: 553 Comm: kworker/38:1 Kdump: loaded Tainted: G OE 5.6.0-deli-v5.6-2848-g3f3109b0e75f #1
[ 584.120782] Hardware name: Supermicro SYS-7049GP-TRT/X11DPG-QT,
BIOS 3.1 05/23/2019 [ 584.121223] Workqueue: events
amdgpu_ras_do_recovery [amdgpu] [ 584.121638] Call Trace:
[ 584.122050] dump_stack+0x98/0xd5
[ 584.122499] __lock_acquire+0x1139/0x16e0 [ 584.122931] ?
trace_hardirqs_on+0x3b/0xf0 [ 584.123358] ?
cancel_delayed_work+0xa6/0xc0 [ 584.123771] lock_acquire+0xb8/0x1c0
[ 584.124197] ? amdgpu_device_gpu_recover+0x262/0x1030 [amdgpu] [
584.124599] down_write+0x49/0x120 [ 584.125032] ?
amdgpu_device_gpu_recover+0x262/0x1030 [amdgpu] [ 584.125472]
amdgpu_device_gpu_recover+0x262/0x1030 [amdgpu] [ 584.125910] ?
amdgpu_ras_error_query+0x1b8/0x2a0 [amdgpu] [ 584.126367]
amdgpu_ras_do_recovery+0x159/0x190 [amdgpu] [ 584.126789]
process_one_work+0x29e/0x630 [ 584.127208] worker_thread+0x3c/0x3f0
[ 584.127621] ? __kthread_parkme+0x61/0x90 [ 584.128014]
kthread+0x12f/0x150 [ 584.128402] ? process_one_work+0x630/0x630 [
584.128790] ? kthread_park+0x90/0x90 [ 584.129174]
ret_from_fork+0x3a/0x50
Each adev has owned lock_class_key to avoid false positive recursive
locking.
NAK, that is not a false positive but a real problem.
The issue here is that we have multiple reset semaphores, one for each device in the hive. If those are not acquired in the correct order we deadlock.
The real solution would be to move the reset_sem into the hive structure and make sure that we lock it only once.
Christian.
Signed-off-by: Dennis Li <Dennis.Li@xxxxxxx>
Change-Id: I7571efeccbf15483982031d00504a353031a854a
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index e97c088d03b3..766dc8f8c8a0 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -967,6 +967,7 @@ struct amdgpu_device {
atomic_t in_gpu_reset;
enum pp_mp1_state mp1_state;
struct rw_semaphore reset_sem;
+ struct lock_class_key lock_key;
struct amdgpu_doorbell_index doorbell_index;
struct mutex notifier_lock;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 6c572db42d92..d78df9312d34 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -3037,6 +3037,7 @@ int amdgpu_device_init(struct amdgpu_device *adev,
mutex_init(&adev->virt.vf_errors.lock);
hash_init(adev->mn_hash);
init_rwsem(&adev->reset_sem);
+ lockdep_set_class(&adev->reset_sem, &adev->lock_key);
atomic_set(&adev->in_gpu_reset, 0);
mutex_init(&adev->psp.mutex);
mutex_init(&adev->notifier_lock);
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flist
s.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7CDe
nnis.Li%40amd.com%7C56c95f939ddd441bd10408d839d77c9e%7C3dd8961fe4884e6
08e11a82d994e183d%7C0%7C0%7C637322944985436656&sdata=%2FBoRyEW3iK5
9Y52ctLWd4y1lOmi2CInb6lpIgAF88i4%3D&reserved=0
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
