* [PATCH v3] drm/amdgpu: annotate a false positive recursive locking
@ 2020-08-11 2:12 Dennis Li
2020-08-11 6:53 ` Christian König
2020-08-11 6:55 ` Daniel Vetter
0 siblings, 2 replies; 10+ messages in thread
From: Dennis Li @ 2020-08-11 2:12 UTC (permalink / raw)
To: amd-gfx, Alexander.Deucher, felix.kuehling, Hawking.Zhang,
christian.koenig, daniel
Cc: 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.
v2:
1. register adev->lock_key into lockdep, otherwise lockdep will
report the below warning
[ 1216.705820] BUG: key ffff890183b647d0 has not been registered!
[ 1216.705924] ------------[ cut here ]------------
[ 1216.705972] DEBUG_LOCKS_WARN_ON(1)
[ 1216.705997] WARNING: CPU: 20 PID: 541 at kernel/locking/lockdep.c:3743 lockdep_init_map+0x150/0x210
v3:
change to use down_write_nest_lock to annotate the false dead-lock
warning.
Signed-off-by: Dennis Li <Dennis.Li@amd.com>
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 62ecac97fbd2..8a55b0bc044a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -4145,12 +4145,15 @@ static int amdgpu_do_asic_reset(struct amdgpu_hive_info *hive,
return r;
}
-static bool amdgpu_device_lock_adev(struct amdgpu_device *adev)
+static bool amdgpu_device_lock_adev(struct amdgpu_device *adev, struct amdgpu_hive_info *hive)
{
if (atomic_cmpxchg(&adev->in_gpu_reset, 0, 1) != 0)
return false;
- down_write(&adev->reset_sem);
+ if (hive) {
+ down_write_nest_lock(&adev->reset_sem, &hive->hive_lock);
+ } else
+ down_write(&adev->reset_sem);
atomic_inc(&adev->gpu_reset_counter);
switch (amdgpu_asic_reset_method(adev)) {
@@ -4312,7 +4315,7 @@ int amdgpu_device_gpu_recover(struct amdgpu_device *adev,
/* block all schedulers and reset given job's ring */
list_for_each_entry(tmp_adev, device_list_handle, gmc.xgmi.head) {
- if (!amdgpu_device_lock_adev(tmp_adev)) {
+ if (!amdgpu_device_lock_adev(tmp_adev, hive)) {
DRM_INFO("Bailing on TDR for s_job:%llx, as another already in progress",
job ? job->base.id : -1);
r = 0;
--
2.17.1
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v3] drm/amdgpu: annotate a false positive recursive locking
2020-08-11 2:12 [PATCH v3] drm/amdgpu: annotate a false positive recursive locking Dennis Li
@ 2020-08-11 6:53 ` Christian König
2020-08-11 7:36 ` Chen, Guchun
2020-08-11 6:55 ` Daniel Vetter
1 sibling, 1 reply; 10+ messages in thread
From: Christian König @ 2020-08-11 6:53 UTC (permalink / raw)
To: Dennis Li, amd-gfx, Alexander.Deucher, felix.kuehling,
Hawking.Zhang, daniel
Am 11.08.20 um 04:12 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.
>
> v2:
> 1. register adev->lock_key into lockdep, otherwise lockdep will
> report the below warning
>
> [ 1216.705820] BUG: key ffff890183b647d0 has not been registered!
> [ 1216.705924] ------------[ cut here ]------------
> [ 1216.705972] DEBUG_LOCKS_WARN_ON(1)
> [ 1216.705997] WARNING: CPU: 20 PID: 541 at kernel/locking/lockdep.c:3743 lockdep_init_map+0x150/0x210
>
> v3:
> change to use down_write_nest_lock to annotate the false dead-lock
> warning.
>
> Signed-off-by: Dennis Li <Dennis.Li@amd.com>
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 62ecac97fbd2..8a55b0bc044a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -4145,12 +4145,15 @@ static int amdgpu_do_asic_reset(struct amdgpu_hive_info *hive,
> return r;
> }
>
> -static bool amdgpu_device_lock_adev(struct amdgpu_device *adev)
> +static bool amdgpu_device_lock_adev(struct amdgpu_device *adev, struct amdgpu_hive_info *hive)
> {
> if (atomic_cmpxchg(&adev->in_gpu_reset, 0, 1) != 0)
> return false;
>
> - down_write(&adev->reset_sem);
> + if (hive) {
> + down_write_nest_lock(&adev->reset_sem, &hive->hive_lock);
> + } else
> + down_write(&adev->reset_sem);
Coding style nit pick: You should drop the {} here.
Apart from that the patch is Reviewed-by: Christian König
<christian.koenig@amd.com>
>
> atomic_inc(&adev->gpu_reset_counter);
> switch (amdgpu_asic_reset_method(adev)) {
> @@ -4312,7 +4315,7 @@ int amdgpu_device_gpu_recover(struct amdgpu_device *adev,
>
> /* block all schedulers and reset given job's ring */
> list_for_each_entry(tmp_adev, device_list_handle, gmc.xgmi.head) {
> - if (!amdgpu_device_lock_adev(tmp_adev)) {
> + if (!amdgpu_device_lock_adev(tmp_adev, hive)) {
> DRM_INFO("Bailing on TDR for s_job:%llx, as another already in progress",
> job ? job->base.id : -1);
> r = 0;
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3] drm/amdgpu: annotate a false positive recursive locking
2020-08-11 2:12 [PATCH v3] drm/amdgpu: annotate a false positive recursive locking Dennis Li
2020-08-11 6:53 ` Christian König
@ 2020-08-11 6:55 ` Daniel Vetter
1 sibling, 0 replies; 10+ messages in thread
From: Daniel Vetter @ 2020-08-11 6:55 UTC (permalink / raw)
To: Dennis Li
Cc: Alex Deucher, Kuehling, Felix, Christian König, amd-gfx list,
Hawking Zhang
On Tue, Aug 11, 2020 at 4:12 AM Dennis Li <Dennis.Li@amd.com> wrote:
>
> [ 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.
>
> v2:
> 1. register adev->lock_key into lockdep, otherwise lockdep will
> report the below warning
>
> [ 1216.705820] BUG: key ffff890183b647d0 has not been registered!
> [ 1216.705924] ------------[ cut here ]------------
> [ 1216.705972] DEBUG_LOCKS_WARN_ON(1)
> [ 1216.705997] WARNING: CPU: 20 PID: 541 at kernel/locking/lockdep.c:3743 lockdep_init_map+0x150/0x210
>
> v3:
> change to use down_write_nest_lock to annotate the false dead-lock
> warning.
>
> Signed-off-by: Dennis Li <Dennis.Li@amd.com>
I'd explain a bit more in writing what the actual deadlock possibility
is, but up to you folks.
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Cheers, Daniel
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 62ecac97fbd2..8a55b0bc044a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -4145,12 +4145,15 @@ static int amdgpu_do_asic_reset(struct amdgpu_hive_info *hive,
> return r;
> }
>
> -static bool amdgpu_device_lock_adev(struct amdgpu_device *adev)
> +static bool amdgpu_device_lock_adev(struct amdgpu_device *adev, struct amdgpu_hive_info *hive)
> {
> if (atomic_cmpxchg(&adev->in_gpu_reset, 0, 1) != 0)
> return false;
>
> - down_write(&adev->reset_sem);
> + if (hive) {
> + down_write_nest_lock(&adev->reset_sem, &hive->hive_lock);
> + } else
> + down_write(&adev->reset_sem);
>
> atomic_inc(&adev->gpu_reset_counter);
> switch (amdgpu_asic_reset_method(adev)) {
> @@ -4312,7 +4315,7 @@ int amdgpu_device_gpu_recover(struct amdgpu_device *adev,
>
> /* block all schedulers and reset given job's ring */
> list_for_each_entry(tmp_adev, device_list_handle, gmc.xgmi.head) {
> - if (!amdgpu_device_lock_adev(tmp_adev)) {
> + if (!amdgpu_device_lock_adev(tmp_adev, hive)) {
> DRM_INFO("Bailing on TDR for s_job:%llx, as another already in progress",
> job ? job->base.id : -1);
> r = 0;
> --
> 2.17.1
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH v3] drm/amdgpu: annotate a false positive recursive locking
2020-08-11 6:53 ` Christian König
@ 2020-08-11 7:36 ` Chen, Guchun
2020-08-11 7:40 ` Christian König
0 siblings, 1 reply; 10+ messages in thread
From: Chen, Guchun @ 2020-08-11 7:36 UTC (permalink / raw)
To: Koenig, Christian, Li, Dennis, amd-gfx@lists.freedesktop.org,
Deucher, Alexander, Kuehling, Felix, Zhang, Hawking,
daniel@ffwll.ch
[AMD Public Use]
> - down_write(&adev->reset_sem);
> + if (hive) {
> + down_write_nest_lock(&adev->reset_sem, &hive->hive_lock);
> + } else
> + down_write(&adev->reset_sem);
Coding style nit pick: You should drop the {} here.
{} could not be dropped here, as down_write_nest_lock Is one macro with multiple lines, otherwise, build error would say 'else' missed one previous 'if'.
Instead of dropping, another {} should be added to else to include down_write(&adev->reset_sem), which makes the braces been balanced.
Regards,
Guchun
-----Original Message-----
From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Christian König
Sent: Tuesday, August 11, 2020 2:53 PM
To: Li, Dennis <Dennis.Li@amd.com>; amd-gfx@lists.freedesktop.org; Deucher, Alexander <Alexander.Deucher@amd.com>; Kuehling, Felix <Felix.Kuehling@amd.com>; Zhang, Hawking <Hawking.Zhang@amd.com>; daniel@ffwll.ch
Subject: Re: [PATCH v3] drm/amdgpu: annotate a false positive recursive locking
Am 11.08.20 um 04:12 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.
>
> v2:
> 1. register adev->lock_key into lockdep, otherwise lockdep will report
> the below warning
>
> [ 1216.705820] BUG: key ffff890183b647d0 has not been registered!
> [ 1216.705924] ------------[ cut here ]------------ [ 1216.705972]
> DEBUG_LOCKS_WARN_ON(1) [ 1216.705997] WARNING: CPU: 20 PID: 541 at
> kernel/locking/lockdep.c:3743 lockdep_init_map+0x150/0x210
>
> v3:
> change to use down_write_nest_lock to annotate the false dead-lock
> warning.
>
> Signed-off-by: Dennis Li <Dennis.Li@amd.com>
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 62ecac97fbd2..8a55b0bc044a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -4145,12 +4145,15 @@ static int amdgpu_do_asic_reset(struct amdgpu_hive_info *hive,
> return r;
> }
>
> -static bool amdgpu_device_lock_adev(struct amdgpu_device *adev)
> +static bool amdgpu_device_lock_adev(struct amdgpu_device *adev,
> +struct amdgpu_hive_info *hive)
> {
> if (atomic_cmpxchg(&adev->in_gpu_reset, 0, 1) != 0)
> return false;
>
> - down_write(&adev->reset_sem);
> + if (hive) {
> + down_write_nest_lock(&adev->reset_sem, &hive->hive_lock);
> + } else
> + down_write(&adev->reset_sem);
Coding style nit pick: You should drop the {} here.
Apart from that the patch is Reviewed-by: Christian König <christian.koenig@amd.com>
>
> atomic_inc(&adev->gpu_reset_counter);
> switch (amdgpu_asic_reset_method(adev)) { @@ -4312,7 +4315,7 @@ int
> amdgpu_device_gpu_recover(struct amdgpu_device *adev,
>
> /* block all schedulers and reset given job's ring */
> list_for_each_entry(tmp_adev, device_list_handle, gmc.xgmi.head) {
> - if (!amdgpu_device_lock_adev(tmp_adev)) {
> + if (!amdgpu_device_lock_adev(tmp_adev, hive)) {
> DRM_INFO("Bailing on TDR for s_job:%llx, as another already in progress",
> job ? job->base.id : -1);
> r = 0;
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7Cguchun.chen%40amd.com%7C0a3fd06c0e774002a0e908d83dc337a0%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637327255952065510&sdata=rJ%2BbWsWJ6NfPUinwUHxnRfVxYi%2Ft4JQCTtamMATrLws%3D&reserved=0
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3] drm/amdgpu: annotate a false positive recursive locking
2020-08-11 7:36 ` Chen, Guchun
@ 2020-08-11 7:40 ` Christian König
2020-08-11 7:44 ` Daniel Vetter
2020-08-11 7:44 ` Chen, Guchun
0 siblings, 2 replies; 10+ messages in thread
From: Christian König @ 2020-08-11 7:40 UTC (permalink / raw)
To: Chen, Guchun, Li, Dennis, amd-gfx@lists.freedesktop.org,
Deucher, Alexander, Kuehling, Felix, Zhang, Hawking,
daniel@ffwll.ch
Am 11.08.20 um 09:36 schrieb Chen, Guchun:
> [AMD Public Use]
>
>> - down_write(&adev->reset_sem);
>> + if (hive) {
>> + down_write_nest_lock(&adev->reset_sem, &hive->hive_lock);
>> + } else
>> + down_write(&adev->reset_sem);
> Coding style nit pick: You should drop the {} here.
>
> {} could not be dropped here, as down_write_nest_lock Is one macro with multiple lines, otherwise, build error would say 'else' missed one previous 'if'.
> Instead of dropping, another {} should be added to else to include down_write(&adev->reset_sem), which makes the braces been balanced.
Interesting bug, that is something which should not happen with macros
in the Linux kernel. They protection by "do { ... } while(0)" is
mandatory here.
Let me take a look,
Christian.
>
> Regards,
> Guchun
>
> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Christian König
> Sent: Tuesday, August 11, 2020 2:53 PM
> To: Li, Dennis <Dennis.Li@amd.com>; amd-gfx@lists.freedesktop.org; Deucher, Alexander <Alexander.Deucher@amd.com>; Kuehling, Felix <Felix.Kuehling@amd.com>; Zhang, Hawking <Hawking.Zhang@amd.com>; daniel@ffwll.ch
> Subject: Re: [PATCH v3] drm/amdgpu: annotate a false positive recursive locking
>
> Am 11.08.20 um 04:12 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.
>>
>> v2:
>> 1. register adev->lock_key into lockdep, otherwise lockdep will report
>> the below warning
>>
>> [ 1216.705820] BUG: key ffff890183b647d0 has not been registered!
>> [ 1216.705924] ------------[ cut here ]------------ [ 1216.705972]
>> DEBUG_LOCKS_WARN_ON(1) [ 1216.705997] WARNING: CPU: 20 PID: 541 at
>> kernel/locking/lockdep.c:3743 lockdep_init_map+0x150/0x210
>>
>> v3:
>> change to use down_write_nest_lock to annotate the false dead-lock
>> warning.
>>
>> Signed-off-by: Dennis Li <Dennis.Li@amd.com>
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> index 62ecac97fbd2..8a55b0bc044a 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> @@ -4145,12 +4145,15 @@ static int amdgpu_do_asic_reset(struct amdgpu_hive_info *hive,
>> return r;
>> }
>>
>> -static bool amdgpu_device_lock_adev(struct amdgpu_device *adev)
>> +static bool amdgpu_device_lock_adev(struct amdgpu_device *adev,
>> +struct amdgpu_hive_info *hive)
>> {
>> if (atomic_cmpxchg(&adev->in_gpu_reset, 0, 1) != 0)
>> return false;
>>
>> - down_write(&adev->reset_sem);
>> + if (hive) {
>> + down_write_nest_lock(&adev->reset_sem, &hive->hive_lock);
>> + } else
>> + down_write(&adev->reset_sem);
> Coding style nit pick: You should drop the {} here.
>
> Apart from that the patch is Reviewed-by: Christian König <christian.koenig@amd.com>
>
>>
>> atomic_inc(&adev->gpu_reset_counter);
>> switch (amdgpu_asic_reset_method(adev)) { @@ -4312,7 +4315,7 @@ int
>> amdgpu_device_gpu_recover(struct amdgpu_device *adev,
>>
>> /* block all schedulers and reset given job's ring */
>> list_for_each_entry(tmp_adev, device_list_handle, gmc.xgmi.head) {
>> - if (!amdgpu_device_lock_adev(tmp_adev)) {
>> + if (!amdgpu_device_lock_adev(tmp_adev, hive)) {
>> DRM_INFO("Bailing on TDR for s_job:%llx, as another already in progress",
>> job ? job->base.id : -1);
>> r = 0;
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7Cguchun.chen%40amd.com%7C0a3fd06c0e774002a0e908d83dc337a0%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637327255952065510&sdata=rJ%2BbWsWJ6NfPUinwUHxnRfVxYi%2Ft4JQCTtamMATrLws%3D&reserved=0
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3] drm/amdgpu: annotate a false positive recursive locking
2020-08-11 7:40 ` Christian König
@ 2020-08-11 7:44 ` Daniel Vetter
2020-08-11 10:34 ` Li, Dennis
2020-08-11 7:44 ` Chen, Guchun
1 sibling, 1 reply; 10+ messages in thread
From: Daniel Vetter @ 2020-08-11 7:44 UTC (permalink / raw)
To: Christian König
Cc: Chen, Guchun, Kuehling, Felix, amd-gfx@lists.freedesktop.org,
Deucher, Alexander, Li, Dennis, Zhang, Hawking
On Tue, Aug 11, 2020 at 9:40 AM Christian König
<christian.koenig@amd.com> wrote:
>
> Am 11.08.20 um 09:36 schrieb Chen, Guchun:
> > [AMD Public Use]
> >
> >> - down_write(&adev->reset_sem);
> >> + if (hive) {
> >> + down_write_nest_lock(&adev->reset_sem, &hive->hive_lock);
> >> + } else
> >> + down_write(&adev->reset_sem);
> > Coding style nit pick: You should drop the {} here.
> >
> > {} could not be dropped here, as down_write_nest_lock Is one macro with multiple lines, otherwise, build error would say 'else' missed one previous 'if'.
> > Instead of dropping, another {} should be added to else to include down_write(&adev->reset_sem), which makes the braces been balanced.
>
> Interesting bug, that is something which should not happen with macros
> in the Linux kernel. They protection by "do { ... } while(0)" is
> mandatory here.
Quick grep says it is protected like that ... can you pls paste the
full build error without {}?
-Daniel
>
> Let me take a look,
> Christian.
>
> >
> > Regards,
> > Guchun
> >
> > -----Original Message-----
> > From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Christian König
> > Sent: Tuesday, August 11, 2020 2:53 PM
> > To: Li, Dennis <Dennis.Li@amd.com>; amd-gfx@lists.freedesktop.org; Deucher, Alexander <Alexander.Deucher@amd.com>; Kuehling, Felix <Felix.Kuehling@amd.com>; Zhang, Hawking <Hawking.Zhang@amd.com>; daniel@ffwll.ch
> > Subject: Re: [PATCH v3] drm/amdgpu: annotate a false positive recursive locking
> >
> > Am 11.08.20 um 04:12 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.
> >>
> >> v2:
> >> 1. register adev->lock_key into lockdep, otherwise lockdep will report
> >> the below warning
> >>
> >> [ 1216.705820] BUG: key ffff890183b647d0 has not been registered!
> >> [ 1216.705924] ------------[ cut here ]------------ [ 1216.705972]
> >> DEBUG_LOCKS_WARN_ON(1) [ 1216.705997] WARNING: CPU: 20 PID: 541 at
> >> kernel/locking/lockdep.c:3743 lockdep_init_map+0x150/0x210
> >>
> >> v3:
> >> change to use down_write_nest_lock to annotate the false dead-lock
> >> warning.
> >>
> >> Signed-off-by: Dennis Li <Dennis.Li@amd.com>
> >>
> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> >> index 62ecac97fbd2..8a55b0bc044a 100644
> >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> >> @@ -4145,12 +4145,15 @@ static int amdgpu_do_asic_reset(struct amdgpu_hive_info *hive,
> >> return r;
> >> }
> >>
> >> -static bool amdgpu_device_lock_adev(struct amdgpu_device *adev)
> >> +static bool amdgpu_device_lock_adev(struct amdgpu_device *adev,
> >> +struct amdgpu_hive_info *hive)
> >> {
> >> if (atomic_cmpxchg(&adev->in_gpu_reset, 0, 1) != 0)
> >> return false;
> >>
> >> - down_write(&adev->reset_sem);
> >> + if (hive) {
> >> + down_write_nest_lock(&adev->reset_sem, &hive->hive_lock);
> >> + } else
> >> + down_write(&adev->reset_sem);
> > Coding style nit pick: You should drop the {} here.
> >
> > Apart from that the patch is Reviewed-by: Christian König <christian.koenig@amd.com>
> >
> >>
> >> atomic_inc(&adev->gpu_reset_counter);
> >> switch (amdgpu_asic_reset_method(adev)) { @@ -4312,7 +4315,7 @@ int
> >> amdgpu_device_gpu_recover(struct amdgpu_device *adev,
> >>
> >> /* block all schedulers and reset given job's ring */
> >> list_for_each_entry(tmp_adev, device_list_handle, gmc.xgmi.head) {
> >> - if (!amdgpu_device_lock_adev(tmp_adev)) {
> >> + if (!amdgpu_device_lock_adev(tmp_adev, hive)) {
> >> DRM_INFO("Bailing on TDR for s_job:%llx, as another already in progress",
> >> job ? job->base.id : -1);
> >> r = 0;
> > _______________________________________________
> > amd-gfx mailing list
> > amd-gfx@lists.freedesktop.org
> > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7Cguchun.chen%40amd.com%7C0a3fd06c0e774002a0e908d83dc337a0%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637327255952065510&sdata=rJ%2BbWsWJ6NfPUinwUHxnRfVxYi%2Ft4JQCTtamMATrLws%3D&reserved=0
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH v3] drm/amdgpu: annotate a false positive recursive locking
2020-08-11 7:40 ` Christian König
2020-08-11 7:44 ` Daniel Vetter
@ 2020-08-11 7:44 ` Chen, Guchun
2020-08-11 7:51 ` Christian König
1 sibling, 1 reply; 10+ messages in thread
From: Chen, Guchun @ 2020-08-11 7:44 UTC (permalink / raw)
To: Koenig, Christian, Li, Dennis, amd-gfx@lists.freedesktop.org,
Deucher, Alexander, Kuehling, Felix, Zhang, Hawking,
daniel@ffwll.ch
[AMD Public Use]
# define down_write_nest_lock(sem, nest_lock) \
do { \
typecheck(struct lockdep_map *, &(nest_lock)->dep_map); \
_down_write_nest_lock(sem, &(nest_lock)->dep_map); \
} while (0);
Looks the ';' after while (0) is the error point. It should be dropped.
Regards,
Guchun
-----Original Message-----
From: Koenig, Christian <Christian.Koenig@amd.com>
Sent: Tuesday, August 11, 2020 3:41 PM
To: Chen, Guchun <Guchun.Chen@amd.com>; Li, Dennis <Dennis.Li@amd.com>; amd-gfx@lists.freedesktop.org; Deucher, Alexander <Alexander.Deucher@amd.com>; Kuehling, Felix <Felix.Kuehling@amd.com>; Zhang, Hawking <Hawking.Zhang@amd.com>; daniel@ffwll.ch
Subject: Re: [PATCH v3] drm/amdgpu: annotate a false positive recursive locking
Am 11.08.20 um 09:36 schrieb Chen, Guchun:
> [AMD Public Use]
>
>> - down_write(&adev->reset_sem);
>> + if (hive) {
>> + down_write_nest_lock(&adev->reset_sem, &hive->hive_lock);
>> + } else
>> + down_write(&adev->reset_sem);
> Coding style nit pick: You should drop the {} here.
>
> {} could not be dropped here, as down_write_nest_lock Is one macro with multiple lines, otherwise, build error would say 'else' missed one previous 'if'.
> Instead of dropping, another {} should be added to else to include down_write(&adev->reset_sem), which makes the braces been balanced.
Interesting bug, that is something which should not happen with macros in the Linux kernel. They protection by "do { ... } while(0)" is mandatory here.
Let me take a look,
Christian.
>
> Regards,
> Guchun
>
> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of
> Christian König
> Sent: Tuesday, August 11, 2020 2:53 PM
> To: Li, Dennis <Dennis.Li@amd.com>; amd-gfx@lists.freedesktop.org;
> Deucher, Alexander <Alexander.Deucher@amd.com>; Kuehling, Felix
> <Felix.Kuehling@amd.com>; Zhang, Hawking <Hawking.Zhang@amd.com>;
> daniel@ffwll.ch
> Subject: Re: [PATCH v3] drm/amdgpu: annotate a false positive
> recursive locking
>
> Am 11.08.20 um 04:12 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.
>>
>> v2:
>> 1. register adev->lock_key into lockdep, otherwise lockdep will
>> report the below warning
>>
>> [ 1216.705820] BUG: key ffff890183b647d0 has not been registered!
>> [ 1216.705924] ------------[ cut here ]------------ [ 1216.705972]
>> DEBUG_LOCKS_WARN_ON(1) [ 1216.705997] WARNING: CPU: 20 PID: 541 at
>> kernel/locking/lockdep.c:3743 lockdep_init_map+0x150/0x210
>>
>> v3:
>> change to use down_write_nest_lock to annotate the false dead-lock
>> warning.
>>
>> Signed-off-by: Dennis Li <Dennis.Li@amd.com>
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> index 62ecac97fbd2..8a55b0bc044a 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> @@ -4145,12 +4145,15 @@ static int amdgpu_do_asic_reset(struct amdgpu_hive_info *hive,
>> return r;
>> }
>>
>> -static bool amdgpu_device_lock_adev(struct amdgpu_device *adev)
>> +static bool amdgpu_device_lock_adev(struct amdgpu_device *adev,
>> +struct amdgpu_hive_info *hive)
>> {
>> if (atomic_cmpxchg(&adev->in_gpu_reset, 0, 1) != 0)
>> return false;
>>
>> - down_write(&adev->reset_sem);
>> + if (hive) {
>> + down_write_nest_lock(&adev->reset_sem, &hive->hive_lock);
>> + } else
>> + down_write(&adev->reset_sem);
> Coding style nit pick: You should drop the {} here.
>
> Apart from that the patch is Reviewed-by: Christian König
> <christian.koenig@amd.com>
>
>>
>> atomic_inc(&adev->gpu_reset_counter);
>> switch (amdgpu_asic_reset_method(adev)) { @@ -4312,7 +4315,7 @@
>> int amdgpu_device_gpu_recover(struct amdgpu_device *adev,
>>
>> /* block all schedulers and reset given job's ring */
>> list_for_each_entry(tmp_adev, device_list_handle, gmc.xgmi.head) {
>> - if (!amdgpu_device_lock_adev(tmp_adev)) {
>> + if (!amdgpu_device_lock_adev(tmp_adev, hive)) {
>> DRM_INFO("Bailing on TDR for s_job:%llx, as another already in progress",
>> job ? job->base.id : -1);
>> r = 0;
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flist
> s.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7Cgu
> chun.chen%40amd.com%7C0a3fd06c0e774002a0e908d83dc337a0%7C3dd8961fe4884
> e608e11a82d994e183d%7C0%7C0%7C637327255952065510&sdata=rJ%2BbWsWJ6
> NfPUinwUHxnRfVxYi%2Ft4JQCTtamMATrLws%3D&reserved=0
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3] drm/amdgpu: annotate a false positive recursive locking
2020-08-11 7:44 ` Chen, Guchun
@ 2020-08-11 7:51 ` Christian König
2020-08-11 8:01 ` Chen, Guchun
0 siblings, 1 reply; 10+ messages in thread
From: Christian König @ 2020-08-11 7:51 UTC (permalink / raw)
To: Chen, Guchun, Li, Dennis, amd-gfx@lists.freedesktop.org,
Deucher, Alexander, Kuehling, Felix, Zhang, Hawking,
daniel@ffwll.ch
Nice catch, it's not often that we stumble over something like this :)
Can you provide a patch to fix this? Problem is that we probably won't
be able to push it to the AMD servers, but I can probably merge it
through drm-misc-next.
Until this is fixed feel free to commit the patch with the {} in place.
Thanks,
Christian.
Am 11.08.20 um 09:44 schrieb Chen, Guchun:
> [AMD Public Use]
>
> # define down_write_nest_lock(sem, nest_lock) \
> do { \
> typecheck(struct lockdep_map *, &(nest_lock)->dep_map); \
> _down_write_nest_lock(sem, &(nest_lock)->dep_map); \
> } while (0);
>
> Looks the ';' after while (0) is the error point. It should be dropped.
>
> Regards,
> Guchun
>
> -----Original Message-----
> From: Koenig, Christian <Christian.Koenig@amd.com>
> Sent: Tuesday, August 11, 2020 3:41 PM
> To: Chen, Guchun <Guchun.Chen@amd.com>; Li, Dennis <Dennis.Li@amd.com>; amd-gfx@lists.freedesktop.org; Deucher, Alexander <Alexander.Deucher@amd.com>; Kuehling, Felix <Felix.Kuehling@amd.com>; Zhang, Hawking <Hawking.Zhang@amd.com>; daniel@ffwll.ch
> Subject: Re: [PATCH v3] drm/amdgpu: annotate a false positive recursive locking
>
> Am 11.08.20 um 09:36 schrieb Chen, Guchun:
>> [AMD Public Use]
>>
>>> - down_write(&adev->reset_sem);
>>> + if (hive) {
>>> + down_write_nest_lock(&adev->reset_sem, &hive->hive_lock);
>>> + } else
>>> + down_write(&adev->reset_sem);
>> Coding style nit pick: You should drop the {} here.
>>
>> {} could not be dropped here, as down_write_nest_lock Is one macro with multiple lines, otherwise, build error would say 'else' missed one previous 'if'.
>> Instead of dropping, another {} should be added to else to include down_write(&adev->reset_sem), which makes the braces been balanced.
> Interesting bug, that is something which should not happen with macros in the Linux kernel. They protection by "do { ... } while(0)" is mandatory here.
>
> Let me take a look,
> Christian.
>
>> Regards,
>> Guchun
>>
>> -----Original Message-----
>> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of
>> Christian König
>> Sent: Tuesday, August 11, 2020 2:53 PM
>> To: Li, Dennis <Dennis.Li@amd.com>; amd-gfx@lists.freedesktop.org;
>> Deucher, Alexander <Alexander.Deucher@amd.com>; Kuehling, Felix
>> <Felix.Kuehling@amd.com>; Zhang, Hawking <Hawking.Zhang@amd.com>;
>> daniel@ffwll.ch
>> Subject: Re: [PATCH v3] drm/amdgpu: annotate a false positive
>> recursive locking
>>
>> Am 11.08.20 um 04:12 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.
>>>
>>> v2:
>>> 1. register adev->lock_key into lockdep, otherwise lockdep will
>>> report the below warning
>>>
>>> [ 1216.705820] BUG: key ffff890183b647d0 has not been registered!
>>> [ 1216.705924] ------------[ cut here ]------------ [ 1216.705972]
>>> DEBUG_LOCKS_WARN_ON(1) [ 1216.705997] WARNING: CPU: 20 PID: 541 at
>>> kernel/locking/lockdep.c:3743 lockdep_init_map+0x150/0x210
>>>
>>> v3:
>>> change to use down_write_nest_lock to annotate the false dead-lock
>>> warning.
>>>
>>> Signed-off-by: Dennis Li <Dennis.Li@amd.com>
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> index 62ecac97fbd2..8a55b0bc044a 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> @@ -4145,12 +4145,15 @@ static int amdgpu_do_asic_reset(struct amdgpu_hive_info *hive,
>>> return r;
>>> }
>>>
>>> -static bool amdgpu_device_lock_adev(struct amdgpu_device *adev)
>>> +static bool amdgpu_device_lock_adev(struct amdgpu_device *adev,
>>> +struct amdgpu_hive_info *hive)
>>> {
>>> if (atomic_cmpxchg(&adev->in_gpu_reset, 0, 1) != 0)
>>> return false;
>>>
>>> - down_write(&adev->reset_sem);
>>> + if (hive) {
>>> + down_write_nest_lock(&adev->reset_sem, &hive->hive_lock);
>>> + } else
>>> + down_write(&adev->reset_sem);
>> Coding style nit pick: You should drop the {} here.
>>
>> Apart from that the patch is Reviewed-by: Christian König
>> <christian.koenig@amd.com>
>>
>>>
>>> atomic_inc(&adev->gpu_reset_counter);
>>> switch (amdgpu_asic_reset_method(adev)) { @@ -4312,7 +4315,7 @@
>>> int amdgpu_device_gpu_recover(struct amdgpu_device *adev,
>>>
>>> /* block all schedulers and reset given job's ring */
>>> list_for_each_entry(tmp_adev, device_list_handle, gmc.xgmi.head) {
>>> - if (!amdgpu_device_lock_adev(tmp_adev)) {
>>> + if (!amdgpu_device_lock_adev(tmp_adev, hive)) {
>>> DRM_INFO("Bailing on TDR for s_job:%llx, as another already in progress",
>>> job ? job->base.id : -1);
>>> r = 0;
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flist
>> s.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7Cgu
>> chun.chen%40amd.com%7C0a3fd06c0e774002a0e908d83dc337a0%7C3dd8961fe4884
>> e608e11a82d994e183d%7C0%7C0%7C637327255952065510&sdata=rJ%2BbWsWJ6
>> NfPUinwUHxnRfVxYi%2Ft4JQCTtamMATrLws%3D&reserved=0
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH v3] drm/amdgpu: annotate a false positive recursive locking
2020-08-11 7:51 ` Christian König
@ 2020-08-11 8:01 ` Chen, Guchun
0 siblings, 0 replies; 10+ messages in thread
From: Chen, Guchun @ 2020-08-11 8:01 UTC (permalink / raw)
To: Koenig, Christian, Li, Dennis, amd-gfx@lists.freedesktop.org,
Deucher, Alexander, Kuehling, Felix, Zhang, Hawking,
daniel@ffwll.ch
[-- Attachment #1: Type: text/plain, Size: 8905 bytes --]
[AMD Public Use]
Hi Christian,
Since it's out of amdgpu driver scope, so I just attached my patch in this email thread instead of sending it by git sent-email.
Regards,
Guchun
-----Original Message-----
From: Koenig, Christian <Christian.Koenig@amd.com>
Sent: Tuesday, August 11, 2020 3:52 PM
To: Chen, Guchun <Guchun.Chen@amd.com>; Li, Dennis <Dennis.Li@amd.com>; amd-gfx@lists.freedesktop.org; Deucher, Alexander <Alexander.Deucher@amd.com>; Kuehling, Felix <Felix.Kuehling@amd.com>; Zhang, Hawking <Hawking.Zhang@amd.com>; daniel@ffwll.ch
Subject: Re: [PATCH v3] drm/amdgpu: annotate a false positive recursive locking
Nice catch, it's not often that we stumble over something like this :)
Can you provide a patch to fix this? Problem is that we probably won't be able to push it to the AMD servers, but I can probably merge it through drm-misc-next.
Until this is fixed feel free to commit the patch with the {} in place.
Thanks,
Christian.
Am 11.08.20 um 09:44 schrieb Chen, Guchun:
> [AMD Public Use]
>
> # define down_write_nest_lock(sem, nest_lock) \
> do { \
> typecheck(struct lockdep_map *, &(nest_lock)->dep_map); \
> _down_write_nest_lock(sem, &(nest_lock)->dep_map); \
> } while (0);
>
> Looks the ';' after while (0) is the error point. It should be dropped.
>
> Regards,
> Guchun
>
> -----Original Message-----
> From: Koenig, Christian <Christian.Koenig@amd.com>
> Sent: Tuesday, August 11, 2020 3:41 PM
> To: Chen, Guchun <Guchun.Chen@amd.com>; Li, Dennis
> <Dennis.Li@amd.com>; amd-gfx@lists.freedesktop.org; Deucher, Alexander
> <Alexander.Deucher@amd.com>; Kuehling, Felix <Felix.Kuehling@amd.com>;
> Zhang, Hawking <Hawking.Zhang@amd.com>; daniel@ffwll.ch
> Subject: Re: [PATCH v3] drm/amdgpu: annotate a false positive
> recursive locking
>
> Am 11.08.20 um 09:36 schrieb Chen, Guchun:
>> [AMD Public Use]
>>
>>> - down_write(&adev->reset_sem);
>>> + if (hive) {
>>> + down_write_nest_lock(&adev->reset_sem, &hive->hive_lock);
>>> + } else
>>> + down_write(&adev->reset_sem);
>> Coding style nit pick: You should drop the {} here.
>>
>> {} could not be dropped here, as down_write_nest_lock Is one macro with multiple lines, otherwise, build error would say 'else' missed one previous 'if'.
>> Instead of dropping, another {} should be added to else to include down_write(&adev->reset_sem), which makes the braces been balanced.
> Interesting bug, that is something which should not happen with macros in the Linux kernel. They protection by "do { ... } while(0)" is mandatory here.
>
> Let me take a look,
> Christian.
>
>> Regards,
>> Guchun
>>
>> -----Original Message-----
>> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of
>> Christian König
>> Sent: Tuesday, August 11, 2020 2:53 PM
>> To: Li, Dennis <Dennis.Li@amd.com>; amd-gfx@lists.freedesktop.org;
>> Deucher, Alexander <Alexander.Deucher@amd.com>; Kuehling, Felix
>> <Felix.Kuehling@amd.com>; Zhang, Hawking <Hawking.Zhang@amd.com>;
>> daniel@ffwll.ch
>> Subject: Re: [PATCH v3] drm/amdgpu: annotate a false positive
>> recursive locking
>>
>> Am 11.08.20 um 04:12 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.
>>>
>>> v2:
>>> 1. register adev->lock_key into lockdep, otherwise lockdep will
>>> report the below warning
>>>
>>> [ 1216.705820] BUG: key ffff890183b647d0 has not been registered!
>>> [ 1216.705924] ------------[ cut here ]------------ [ 1216.705972]
>>> DEBUG_LOCKS_WARN_ON(1) [ 1216.705997] WARNING: CPU: 20 PID: 541 at
>>> kernel/locking/lockdep.c:3743 lockdep_init_map+0x150/0x210
>>>
>>> v3:
>>> change to use down_write_nest_lock to annotate the false dead-lock
>>> warning.
>>>
>>> Signed-off-by: Dennis Li <Dennis.Li@amd.com>
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> index 62ecac97fbd2..8a55b0bc044a 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> @@ -4145,12 +4145,15 @@ static int amdgpu_do_asic_reset(struct amdgpu_hive_info *hive,
>>> return r;
>>> }
>>>
>>> -static bool amdgpu_device_lock_adev(struct amdgpu_device *adev)
>>> +static bool amdgpu_device_lock_adev(struct amdgpu_device *adev,
>>> +struct amdgpu_hive_info *hive)
>>> {
>>> if (atomic_cmpxchg(&adev->in_gpu_reset, 0, 1) != 0)
>>> return false;
>>>
>>> - down_write(&adev->reset_sem);
>>> + if (hive) {
>>> + down_write_nest_lock(&adev->reset_sem, &hive->hive_lock);
>>> + } else
>>> + down_write(&adev->reset_sem);
>> Coding style nit pick: You should drop the {} here.
>>
>> Apart from that the patch is Reviewed-by: Christian König
>> <christian.koenig@amd.com>
>>
>>>
>>> atomic_inc(&adev->gpu_reset_counter);
>>> switch (amdgpu_asic_reset_method(adev)) { @@ -4312,7 +4315,7 @@
>>> int amdgpu_device_gpu_recover(struct amdgpu_device *adev,
>>>
>>> /* block all schedulers and reset given job's ring */
>>> list_for_each_entry(tmp_adev, device_list_handle, gmc.xgmi.head) {
>>> - if (!amdgpu_device_lock_adev(tmp_adev)) {
>>> + if (!amdgpu_device_lock_adev(tmp_adev, hive)) {
>>> DRM_INFO("Bailing on TDR for s_job:%llx, as another already in progress",
>>> job ? job->base.id : -1);
>>> r = 0;
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flis
>> t
>> s.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7Cg
>> u
>> chun.chen%40amd.com%7C0a3fd06c0e774002a0e908d83dc337a0%7C3dd8961fe488
>> 4
>> e608e11a82d994e183d%7C0%7C0%7C637327255952065510&sdata=rJ%2BbWsWJ
>> 6
>> NfPUinwUHxnRfVxYi%2Ft4JQCTtamMATrLws%3D&reserved=0
[-- Attachment #2: 0001-locking-rwsem-remove-redundant-semicolon-of-down_wri.patch --]
[-- Type: application/octet-stream, Size: 901 bytes --]
From 68d142e006e434ed1133014bfcf06a30a9ca980f Mon Sep 17 00:00:00 2001
From: Guchun Chen <guchun.chen@amd.com>
Date: Tue, 11 Aug 2020 15:56:10 +0800
Subject: [PATCH] locking/rwsem: remove redundant semicolon of
down_write_nest_lock
Otherwise, braces are needed when using it.
Signed-off-by: Guchun Chen <guchun.chen@amd.com>
---
include/linux/rwsem.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/rwsem.h b/include/linux/rwsem.h
index 00d6054687dd..4c4c569055fb 100644
--- a/include/linux/rwsem.h
+++ b/include/linux/rwsem.h
@@ -181,7 +181,7 @@ extern void _down_write_nest_lock(struct rw_semaphore *sem, struct lockdep_map *
do { \
typecheck(struct lockdep_map *, &(nest_lock)->dep_map); \
_down_write_nest_lock(sem, &(nest_lock)->dep_map); \
-} while (0);
+} while (0)
/*
* Take/release a lock when not the owner will release it.
--
2.17.1
[-- Attachment #3: Type: text/plain, Size: 154 bytes --]
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply related [flat|nested] 10+ messages in thread
* RE: [PATCH v3] drm/amdgpu: annotate a false positive recursive locking
2020-08-11 7:44 ` Daniel Vetter
@ 2020-08-11 10:34 ` Li, Dennis
0 siblings, 0 replies; 10+ messages in thread
From: Li, Dennis @ 2020-08-11 10:34 UTC (permalink / raw)
To: Daniel Vetter, Koenig, Christian
Cc: Deucher, Alexander, Kuehling, Felix,
amd-gfx@lists.freedesktop.org, Chen, Guchun, Zhang, Hawking
[AMD Official Use Only - Internal Distribution Only]
Quick grep says it is protected like that ... can you pls paste the full build error without {}?
-Daniel
[Dennis Li] hi, Daniel, the full build error as the following:
make: Entering directory '/home/yajunl/workspace/amd/brahma-staging/BUILD/x86_64/linux'
CC [M] drivers/gpu/drm/amd/amdgpu/amdgpu_device.o
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c: In function ‘amdgpu_device_lock_adev’:
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c:4155:2: error: ‘else’ without a previous ‘if’
else
^~~~
scripts/Makefile.build:267: recipe for target 'drivers/gpu/drm/amd/amdgpu/amdgpu_device.o' failed
make[1]: *** [drivers/gpu/drm/amd/amdgpu/amdgpu_device.o] Error 1
Makefile:1683: recipe for target 'drivers/gpu/drm/amd/amdgpu' failed
make: *** [drivers/gpu/drm/amd/amdgpu] Error 2
>
> Let me take a look,
> Christian.
>
> >
> > Regards,
> > Guchun
> >
> > -----Original Message-----
> > From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of
> > Christian König
> > Sent: Tuesday, August 11, 2020 2:53 PM
> > To: Li, Dennis <Dennis.Li@amd.com>; amd-gfx@lists.freedesktop.org;
> > Deucher, Alexander <Alexander.Deucher@amd.com>; Kuehling, Felix
> > <Felix.Kuehling@amd.com>; Zhang, Hawking <Hawking.Zhang@amd.com>;
> > daniel@ffwll.ch
> > Subject: Re: [PATCH v3] drm/amdgpu: annotate a false positive
> > recursive locking
> >
> > Am 11.08.20 um 04:12 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
> >> kthread+[
> >> 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.
> >>
> >> v2:
> >> 1. register adev->lock_key into lockdep, otherwise lockdep will
> >> report the below warning
> >>
> >> [ 1216.705820] BUG: key ffff890183b647d0 has not been registered!
> >> [ 1216.705924] ------------[ cut here ]------------ [ 1216.705972]
> >> DEBUG_LOCKS_WARN_ON(1) [ 1216.705997] WARNING: CPU: 20 PID: 541 at
> >> kernel/locking/lockdep.c:3743 lockdep_init_map+0x150/0x210
> >>
> >> v3:
> >> change to use down_write_nest_lock to annotate the false dead-lock
> >> warning.
> >>
> >> Signed-off-by: Dennis Li <Dennis.Li@amd.com>
> >>
> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> >> index 62ecac97fbd2..8a55b0bc044a 100644
> >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> >> @@ -4145,12 +4145,15 @@ static int amdgpu_do_asic_reset(struct amdgpu_hive_info *hive,
> >> return r;
> >> }
> >>
> >> -static bool amdgpu_device_lock_adev(struct amdgpu_device *adev)
> >> +static bool amdgpu_device_lock_adev(struct amdgpu_device *adev,
> >> +struct amdgpu_hive_info *hive)
> >> {
> >> if (atomic_cmpxchg(&adev->in_gpu_reset, 0, 1) != 0)
> >> return false;
> >>
> >> - down_write(&adev->reset_sem);
> >> + if (hive) {
> >> + down_write_nest_lock(&adev->reset_sem, &hive->hive_lock);
> >> + } else
> >> + down_write(&adev->reset_sem);
> > Coding style nit pick: You should drop the {} here.
> >
> > Apart from that the patch is Reviewed-by: Christian König
> > <christian.koenig@amd.com>
> >
> >>
> >> atomic_inc(&adev->gpu_reset_counter);
> >> switch (amdgpu_asic_reset_method(adev)) { @@ -4312,7 +4315,7
> >> @@ int amdgpu_device_gpu_recover(struct amdgpu_device *adev,
> >>
> >> /* block all schedulers and reset given job's ring */
> >> list_for_each_entry(tmp_adev, device_list_handle, gmc.xgmi.head) {
> >> - if (!amdgpu_device_lock_adev(tmp_adev)) {
> >> + if (!amdgpu_device_lock_adev(tmp_adev, hive)) {
> >> DRM_INFO("Bailing on TDR for s_job:%llx, as another already in progress",
> >> job ? job->base.id : -1);
> >> r = 0;
> > _______________________________________________
> > amd-gfx mailing list
> > amd-gfx@lists.freedesktop.org
> > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fli
> > sts.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%
> > 7CDennis.Li%40amd.com%7Cee5574efd1f2475112a908d83dca5daa%7C3dd8961fe
> > 4884e608e11a82d994e183d%7C0%7C0%7C637327286674286678&sdata=K5Sx4
> > cO3DIQaxNRSo6q%2B8u%2BscGkCpy0ueQ0cOWyipFA%3D&reserved=0
>
--
Daniel Vetter
Software Engineer, Intel Corporation
https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fblog.ffwll.ch%2F&data=02%7C01%7CDennis.Li%40amd.com%7Cee5574efd1f2475112a908d83dca5daa%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637327286674286678&sdata=7tC0Gakzg7uFgxH6N9pvn5%2B5TCYL3aKK%2FhHTkTyCmtM%3D&reserved=0
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-08-11 10:34 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-08-11 2:12 [PATCH v3] drm/amdgpu: annotate a false positive recursive locking Dennis Li
2020-08-11 6:53 ` Christian König
2020-08-11 7:36 ` Chen, Guchun
2020-08-11 7:40 ` Christian König
2020-08-11 7:44 ` Daniel Vetter
2020-08-11 10:34 ` Li, Dennis
2020-08-11 7:44 ` Chen, Guchun
2020-08-11 7:51 ` Christian König
2020-08-11 8:01 ` Chen, Guchun
2020-08-11 6:55 ` Daniel Vetter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.