Hi Horace,
Andrey needs to take a look here since I haven't worked on that code in
years.
In general I still suggest to unify the reset code for hive and non-hive
code paths which would solve this quite gracefully.
Regards,
Christian.
Am 08.01.21 um 12:34 schrieb Chen, Horace:
[AMD Public Use]
Hi Christian,
Can you help review this change?
This issue happens when 2 jobs on 2 schedulers time out at the same time. Which will lead 2 threads to enter amdgpu_device_gpu_recover() at the same time. The problem is that if device is not an XGMI node, the adev->gmc.xgmi.head will be added to device_list which is a stack variable.
So the first thread will get the device in to its device list and start to iterate, meanwhile the second thread may rob the device away from the first thread and add to its own device list. This will cause the first thread get in to a bad state in its iteration.
The solution is to lock the device earily, before we add device to the local device list.
Thanks & Regards,
Horace.
-----Original Message-----
From: Horace Chen <horace.chen@xxxxxxx>
Sent: Wednesday, January 6, 2021 8:43 PM
To: amd-gfx@xxxxxxxxxxxxxxxxxxxxx
Cc: Chen, Horace <Horace.Chen@xxxxxxx>; Tuikov, Luben <Luben.Tuikov@xxxxxxx>; Koenig, Christian <Christian.Koenig@xxxxxxx>; Deucher, Alexander <Alexander.Deucher@xxxxxxx>; Xiao, Jack <Jack.Xiao@xxxxxxx>; Zhang, Hawking <Hawking.Zhang@xxxxxxx>; Liu, Monk <Monk.Liu@xxxxxxx>; Xu, Feifei <Feifei.Xu@xxxxxxx>; Wang, Kevin(Yang) <Kevin1.Wang@xxxxxxx>; Xiaojie Yuan <xiaojie.yuan@xxxxxxx>
Subject: [PATCH] drm/amdgpu: fix issue when 2 ring job timeout
Fix a racing issue when 2 rings job timeout simultaneously.
If 2 rings timed out at the same time, the
amdgpu_device_gpu_recover will be reentered. Then the
adev->gmc.xgmi.head will be grabbed by 2 local linked list,
which may cause wild pointer issue in iterating.
lock the device earily to prevent the node be added to 2
different lists.
Signed-off-by: Horace Chen <horace.chen@xxxxxxx>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 25 ++++++++++++++++------
1 file changed, 18 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 9a3cb98d03be..233dae27c8eb 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -4620,23 +4620,34 @@ int amdgpu_device_gpu_recover(struct amdgpu_device *adev,
if (adev->gmc.xgmi.num_physical_nodes > 1) {
if (!hive)
return -ENODEV;
+
+ list_for_each_entry(tmp_adev, &hive->device_list, gmc.xgmi.head) {
+ if (!amdgpu_device_lock_adev(tmp_adev, hive)) {
+ dev_info(tmp_adev->dev, "Bailing on TDR for s_job:%llx, as another already in progress",
+ job ? job->base.id : -1);
+ r = 0;
+ goto skip_recovery;
+ }
+ }
+
if (!list_is_first(&adev->gmc.xgmi.head, &hive->device_list))
list_rotate_to_front(&adev->gmc.xgmi.head, &hive->device_list);
device_list_handle = &hive->device_list;
} else {
+ /* if current dev is already in reset, skip adding list to prevent race issue */
+ if (!amdgpu_device_lock_adev(adev, hive)) {
+ dev_info(adev->dev, "Bailing on TDR for s_job:%llx, as another already in progress",
+ job ? job->base.id : -1);
+ r = 0;
+ goto skip_recovery;
+ }
+
list_add_tail(&adev->gmc.xgmi.head, &device_list);
device_list_handle = &device_list;
}
/* 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, hive)) {
- dev_info(tmp_adev->dev, "Bailing on TDR for s_job:%llx, as another already in progress",
- job ? job->base.id : -1);
- r = 0;
- goto skip_recovery;
- }
-
/*
* Try to put the audio codec into suspend state
* before gpu reset started.
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
