AMD General Hi Felix,
Thanks for the review. v3 addresses these: - Interrupts during reset: with v3, KFD pre-reset runs before the reset domain is locked, so the unmap is sent and waited for while the GPU can still respond. Filtering interrupts for a half-broken GPU is a separate change; KFD also needs the fence interrupt for process-close draining, so I'd rather do that as a follow-up than fold it in here. - amdgpu_amdkfd_pre_reset() now takes the device list; the extra wrapper is gone. - pci_channel_state: you're right, it is only set in amdgpu_pci_error_detected(). kfd_dqm_can_reach_hw() is dropped, so no new "is the GPU usable" condition is added. - unmap_queues_cpsch() now returns -EIO when the HWS is known to be hung. The MES check is removed. Regards, William ---------------------------------------------------------------------------------------------------------------------------------- 1 Commerce Valley Drive, Markham, ON L3T 7X6 LinkedIn | Instagram | X | amd.com -----Original Message----- From: Kuehling, Felix <[email protected]> Sent: Thursday, October 8, 2026 11:58 AM To: Palacek, William <[email protected]>; [email protected]; Koenig, Christian <[email protected]> Cc: Russell, Kent <[email protected]>; Kasiviswanathan, Harish <[email protected]> Subject: Re: [PATCH v2] drm/amdgpu: preempt KFD queues before taking the reset domain lock [+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))
