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