Re: [PATCH v2] drm/amdgpu: preempt KFD queues before taking the reset domain lock

Thu, 08 Oct 2026 08:58:35 -0700 

[+Christian for comments on reset architecture]

On 2026-10-08 11:46, Kuehling, Felix wrote:


On 2026-10-08 10:00, William Palacek wrote:

A GPU reset issued while processes are using the device can Oops the
host in munmap() (free_swap_cache) after SDMA write faults from a user
queue whose process has already exited.

amdgpu_device_gpu_recover() takes reset_domain->sem for write before
amdgpu_amdkfd_pre_reset(), so the down_read_trylock() in
unmap_queues_cpsch() and execute_queues_cpsch() fails with -EIO and no
unmap is sent. stop_cpsch() still clears sched_running, so user queues
keep running while their memory is freed.



There is a general assumption that the GPU is stuck when we're in a 
GPU reset. It seems in your scenario, the GPU is not entirely stuck. 
So you can still get interrupts from the GPU. At the same time, you 
can't rely on MES or HWS to unmap queues when the GPU is in a 
half-broken state. So while your patch may help in this special case, 
there could be other cases where you can't reliably stop the GPU from 
sending spurious interrupts while a reset is in progress.



Maybe we need something in the interrupt handler to ignore interrupts 
while a GPU reset is in progress.



It's also possible that there are still interrupts pending in the IH 
ring when a reset starts that won't get processed until later. Do we 
drain or discard pending IRQs before starting a reset?


Regards,
   Felix







Call amdgpu_amdkfd_pre_reset() before the lock, for gpu_recover and PCI
error handling. Skip the dequeue when the HWS is hung or the PCI link is
down, so recovery does not wait out the preemption timeout.

Signed-off-by: William Palacek <[email protected]>
---

Notes:
     v2:
     - Drop the SRC_USER gate and the helper; run for every reset source
     - Cover amdgpu_pci_error_detected() as well as gpu_recover()
     - Skip the dequeue when the HWS is hung or the PCI link is down

  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c       | 16 ++++++++++++++--
  .../drm/amd/amdkfd/kfd_device_queue_manager.c    | 14 ++++++++++++++
  2 files changed, 28 insertions(+), 2 deletions(-)


diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c

index e0014611f039..8ae6228dbf31 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c

@@ -5479,6 +5479,16 @@ static void 
amdgpu_device_recovery_prepare(struct amdgpu_device *adev,

      }
  }

  +/* Dequeueing needs reset_domain->sem for read, so run before it 
is write-locked */
+static void amdgpu_device_kfd_pre_reset(struct amdgpu_reset_context 
*reset_context,

+                    struct list_head *device_list)



Instead of adding a new function with a confusingly similar name, 
should we change the signature of amdgpu_amdkfd_pre_reset to accept a 
device-list instead?




+{
+    struct amdgpu_device *tmp_adev;
+
+    list_for_each_entry(tmp_adev, device_list, reset_list)
+        amdgpu_amdkfd_pre_reset(tmp_adev, reset_context);
+}
+

  static void amdgpu_device_recovery_get_reset_lock(struct 
amdgpu_device *adev,

                            struct list_head *device_list)
  {

@@ -5534,8 +5544,6 @@ static void 
amdgpu_device_halt_activities(struct amdgpu_device *adev,

cancel_delayed_work_sync(&tmp_adev->delayed_init_work);
  -        amdgpu_amdkfd_pre_reset(tmp_adev, reset_context);
-
          /*
           * Mark these ASICs to be reset as untracked first
           * And add them back after reset completed

@@ -5784,6 +5792,9 @@ int amdgpu_device_gpu_recover(struct 
amdgpu_device *adev,

      /* Cannot be called after locking reset domain */
      amdgpu_ras_pre_reset(adev, &device_list);
  +    /* Cannot be called after locking reset domain either */
+    amdgpu_device_kfd_pre_reset(reset_context, &device_list);
+

      /* We need to lock reset domain only once both for XGMI and 
single device */

      amdgpu_device_recovery_get_reset_lock(adev, &device_list);

  @@ -6232,6 +6243,7 @@ pci_ers_result_t 
amdgpu_pci_error_detected(struct pci_dev *pdev, pci_channel_sta

          INIT_LIST_HEAD(&device_list);
            amdgpu_device_recovery_prepare(adev, &device_list, hive);
+        amdgpu_device_kfd_pre_reset(&reset_context, &device_list);
          amdgpu_device_recovery_get_reset_lock(adev, &device_list);

          amdgpu_device_halt_activities(adev, NULL, &reset_context, 
&device_list,

                            hive, false);

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c 
b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c

index 5e57576fba21..988fce11a4fb 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c

@@ -81,6 +81,16 @@ static void set_queue_as_reset(struct 
device_queue_manager *dqm, struct queue *q

                     struct qcm_process_device *qpd, bool verbose);

  static int reset_queues_mes(struct device_queue_manager *dqm, 
struct queue *q);

  +/*

+ * A dead HWS or an offline PCI link cannot service a dequeue 
request. Skip it

+ * rather than wait out queue_preemption_timeout_ms per node.
+ */

+static inline bool kfd_dqm_can_reach_hw(struct device_queue_manager 
*dqm)

+{
+    return !dqm->is_hws_hang &&
+           dqm->dev->adev->pci_channel_state == pci_channel_io_normal;



Is pci_channel_state always valid? I notice that pci_channel_io_normal 
is 1. pci_channel_state is only set in amdgpu_pci_error_detected. If 
that never gets called, pci_channel_state is probably 0-initialized 
and this condition will never be true.



I'm a bit cautious about spreading different versions of "is the GPU 
usable" throughout the code. There are lots of things you could look 
at. I've seen different conditions related to GPU hot-unplug in 
another patch I'm reviewing. These kinds of conditions should be 
managed carefully, otherwise we end up with subtly different or broken 
conditions all over the place and it will be a mess to fix later. Or 
the the code becomes unmaintainable if the conditions need to change, 
because now you have to change it in 15 different places.




+}
+
  static inline

  enum KFD_MQD_TYPE get_mqd_type_from_queue_type(enum kfd_queue_type 
type)

  {

@@ -314,6 +324,8 @@ static int 
remove_queue_mes_on_reset_option(struct device_queue_manager *dqm, st

        if (!dqm->sched_running || dqm->sched_halt)
          return 0;
+    if (!kfd_dqm_can_reach_hw(dqm))
+        return 0;



Why does this return 0 and not -EIO? Isn't this logically the same as 
the next condition? We don't talk to MES because the GPU is hanging 
and a reset in progress.




      if (!down_read_trylock(&adev->reset_domain->sem))
          return -EIO;

  @@ -2832,6 +2844,8 @@ static int unmap_queues_cpsch(struct 
device_queue_manager *dqm,

        if (!dqm->sched_running)
          return 0;
+    if (!kfd_dqm_can_reach_hw(dqm))
+        return 0;


Same question as above.

Regards,
  Felix



      if (!dqm->active_runlist)
          return 0;
      if (!down_read_trylock(&dqm->dev->adev->reset_domain->sem))






















					Reply via email to