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.
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))
