Re: [PATCH 1/2] drm/amdgpu: fix deadlock caused by overflow

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Am 15.09.22 um 22:37 schrieb Andrey Grodzovsky:

On 2022-09-15 15:26, Christian König wrote:
Am 15.09.22 um 20:29 schrieb Andrey Grodzovsky:

On 2022-09-15 06:09, Zhao, Victor wrote:
[AMD Official Use Only - General]

Hi Christian,

The test sequence is executing a compute engine hang while running a lot of containers submitting gfx jobs. We have advanced tdr mode and mode2 reset enabled on driver. When a compute hang job timeout happens, the 2 jobs on the gfx pending list maybe signaled after drm_sched_stop. So they will not be removed from pending list but have the DMA_FENCE_FLAG_SIGNALED_BIT set. At the amdgpu_device_recheck_guilty_jobs step, the first job will be rerun and removed from pending list. At the resubmit setp, the second job (with signaled bit) will be resubmitted. Since it still has signaled bit, drm_sched_job_done will be called directly. This decrease the hw_rq_count which allows more jobs emitted but did not clean fence_drv rcu ptr. This results in an overflow in the fence_drv. Since we will use num_fences_mask in amdgpu_fence_process, when overflow happens, the signal of some job will be skipped which result in an infinite wait for the fence_drv rcu ptr.

So close irq before sched_stop could avoid signal jobs after drm_sched_stop. And signal job one by one in fence_process instead of using a mask will handle the overflow situation.

Another fix could be skip submitting jobs which already signaled during resubmit stage, which may look cleaner.

Please help give some advice.


How about the code bellow  instead ? The real problem is that we reuse a dma fence twice which is not according to fma fence design, so maybe this can help ?


diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
index 8adeb7469f1e..033f0ae16784 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
@@ -164,6 +164,10 @@ int amdgpu_fence_emit(struct amdgpu_ring *ring, struct dma_fence **f, struct amd
        if (job && job->job_run_counter) {
                /* reinit seq for resubmitted jobs */
                fence->seqno = seq;
+
+               /* For resubmitted job clear the singled bit */
+               celar_bit(DMA_FENCE_FLAG_TIMESTAMP_BIT, &fence->flags);
+

Upstream will pretty much kill you for that.

Re-setting a fence from a signaled to an unsignaled state is a massive no-go.

Christian.


Is it worse then doing fence->seqno = seq; ? This is already a huge hack , no ?

No, it's as equally bad. I don't think we can do either.

Christian.


Andrey




                /* TO be inline with external fence creation and other drivers */
                dma_fence_get(fence);
        } else {


Andrey




Thanks,
Victor



-----Original Message-----
From: Koenig, Christian <Christian.Koenig@xxxxxxx>
Sent: Thursday, September 15, 2022 2:32 PM
To: Zhao, Victor <Victor.Zhao@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx; Grodzovsky, Andrey <Andrey.Grodzovsky@xxxxxxx>
Cc: Deng, Emily <Emily.Deng@xxxxxxx>
Subject: Re: [PATCH 1/2] drm/amdgpu: fix deadlock caused by overflow



Am 15.09.22 um 06:02 schrieb Zhao, Victor:
[AMD Official Use Only - General]

Ping.

Hi @Koenig, Christian and @Grodzovsky, Andrey,

We found some reset related issues during stress test on the sequence. Please help give some comments.


Thanks,
Victor



-----Original Message-----
From: Victor Zhao <Victor.Zhao@xxxxxxx>
Sent: Wednesday, September 14, 2022 6:10 PM
To: amd-gfx@xxxxxxxxxxxxxxxxxxxxx
Cc: Deng, Emily <Emily.Deng@xxxxxxx>; Grodzovsky, Andrey
<Andrey.Grodzovsky@xxxxxxx>; Zhao, Victor <Victor.Zhao@xxxxxxx>
Subject: [PATCH 1/2] drm/amdgpu: fix deadlock caused by overflow

[background]
For a gpu recovery caused by a hang on one ring (e.g. compute), jobs from another ring (e.g. gfx) may continue signaling during drm_sched_stop stage. The signal bit will not be cleared.

At the resubmit stage after recovery, the job with hw fence signaled bit set will call job done directly instead go through fence process. This makes the hw_rq_count decrease but rcu fence pointer not cleared yet.

Then overflow happens in the fence driver slots and some jobs may be skipped and leave the rcu pointer not cleared which makes an infinite wait for the slot on the next fence emitted.

This infinite wait cause a job timeout on the emitting job. And driver will stuck at the its sched stop step because kthread_park cannot be done.

[how]
1. move amdgpu_fence_driver_isr_toggle earlier to close interrupt
before drm sched stop 2. handle all fences in fence process to aviod
skip when overflow happens

Signed-off-by: Victor Zhao <Victor.Zhao@xxxxxxx>
---
   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 16 +++++++++++++--- drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c |  6 +-----
   2 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 943c9e750575..c0cfae52f12b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -4610,8 +4610,6 @@ int amdgpu_device_pre_asic_reset(struct amdgpu_device *adev,
           amdgpu_virt_fini_data_exchange(adev);
       }
   -    amdgpu_fence_driver_isr_toggle(adev, true);
-
       /* block all schedulers and reset given job's ring */
       for (i = 0; i < AMDGPU_MAX_RINGS; ++i) {
           struct amdgpu_ring *ring = adev->rings[i]; @@ -5214,6 +5212,8 @@ int amdgpu_device_gpu_recover(struct amdgpu_device *adev,
amdgpu_device_ip_need_full_reset(tmp_adev))
               amdgpu_ras_suspend(tmp_adev);
   +        amdgpu_fence_driver_isr_toggle(tmp_adev, true);
+
           for (i = 0; i < AMDGPU_MAX_RINGS; ++i) {
               struct amdgpu_ring *ring = tmp_adev->rings[i];
   @@ -5228,8 +5228,12 @@ int amdgpu_device_gpu_recover(struct amdgpu_device *adev,
atomic_inc(&tmp_adev->gpu_reset_counter);
       }
   -    if (need_emergency_restart)
+    if (need_emergency_restart) {
+        list_for_each_entry (tmp_adev, device_list_handle, reset_list) {
+            amdgpu_fence_driver_isr_toggle(tmp_adev, false);
+        }
           goto skip_sched_resume;
+    }
          /*
        * Must check guilty signal here since after this point all old @@ -5240,6 +5244,9 @@ int amdgpu_device_gpu_recover(struct amdgpu_device *adev,
       if (job && dma_fence_is_signaled(&job->hw_fence)) {
           job_signaled = true;
           dev_info(adev->dev, "Guilty job already signaled, skipping HW
reset");
+        list_for_each_entry (tmp_adev, device_list_handle, reset_list) {
+            amdgpu_fence_driver_isr_toggle(tmp_adev, false);
+        }
           goto skip_hw_reset;
       }
   @@ -5276,6 +5283,7 @@ int amdgpu_device_gpu_recover(struct amdgpu_device *adev,
           if (r && r == -EAGAIN) {
               set_bit(AMDGPU_SKIP_MODE2_RESET, &reset_context->flags);
               adev->asic_reset_res = 0;
+            amdgpu_fence_driver_isr_toggle(adev, true);
               goto retry;
           }
       }
@@ -5711,6 +5719,8 @@ pci_ers_result_t amdgpu_pci_slot_reset(struct pci_dev *pdev)
       set_bit(AMDGPU_SKIP_HW_RESET, &reset_context.flags);
       set_bit(AMDGPU_SKIP_MODE2_RESET, &reset_context.flags);
   +    amdgpu_fence_driver_isr_toggle(adev, true);
+
       adev->no_hw_access = true;
       r = amdgpu_device_pre_asic_reset(adev, &reset_context);
       adev->no_hw_access = false;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
index 8adeb7469f1e..65a877e1a7fc 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
@@ -287,15 +287,11 @@ bool amdgpu_fence_process(struct amdgpu_ring *ring)
       if (unlikely(seq == last_seq))
           return false;
   -    last_seq &= drv->num_fences_mask;
-    seq &= drv->num_fences_mask;
-
       do {
           struct dma_fence *fence, **ptr;
              ++last_seq;
-        last_seq &= drv->num_fences_mask;
-        ptr = &drv->fences[last_seq];
+        ptr = &drv->fences[last_seq & drv->num_fences_mask];
              /* There is always exactly one thread signaling this fence slot */
           fence = rcu_dereference_protected(*ptr, 1);
Those changes here doesn't seem to make sense. Please explain further why that is necessary.

Christian.

--
2.25.1





[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux