AMD General
On 2026-09-30, Felix Kuehling wrote:
> I suspect this may have been introduced by this patch by Alex:
> commit f34034ce5d2116f397008c63901c9c1946e5d665
> ("drm/amdkfd: disable debug before retiring MES process context on teardown")
> If you can confirm that, please add an appropriate Fixes: tag.
I looked into it and I think the root cause predates f34034ce. That commit
only reorders dequeue vs. kfd_dbg_trap_disable() in the teardown path;
kfd_dbg_trap_disable() was already called from the mmu_notifier release
path, and the process is removed from kfd_processes_table before we get
there in either case. So reverting f34034ce would not avoid the hang.
The lost wakeup is inherent to the drain design: the wakeup
(kfd_process_close_interrupt_drain()) looks the process up by PASID in
kfd_processes_table, which no longer holds an exiting process. That was
introduced in:
Fixes: 12fb1ad70d65 ("drm/amdkfd: update process interrupt handling for debug
events")
I added that tag in v2. Please let me know if you'd still prefer the
f34034ce reference.
> The way you're setting and clearing p->irq_drain_deferred seems pretty
> fragile. I'd prefer to just pass this as a parameter to
> kfd_dbg_trap_disable and kfd_dbg_clean_exception_status. Then you also
> don't need two different drain functions.
Done. kfd_dbg_trap_disable() and kfd_dbg_clean_exception_status() now take
a drain_irqs argument, and kfd_process_drain_deferred_interrupts() and the
irq_drain_deferred field are gone.
While doing this I decided to skip the drain entirely for a process exiting
on its own, instead of deferring it to kfd_process_wq_release(): the
self-exit path now passes drain_irqs=false. Once the process is out of
kfd_processes_table its interrupts are no longer delivered to it, and its
PASID owner is cleared before the PASID is freed and later reallocated, so
there is nothing left to drain. The debugger disabling a target still
passes true. Please shout if you think the exiting process still needs a
real drain here.
> If you use an XArray here, you have a more efficient lookup and you also
> get the spin-lock for free. In fact, that xarray already exists in
> amdgpu_ids.c: amdgpu_pasid_xa. [...] you could just add the completion to
> struct amdgpu_fpriv. Maybe the whole IRQ draining logic should move to
> amdgpu_ids.c in that case, and KFD would just invoke it with a PASID.
Done. I added a struct completion irq_drain_done to struct amdgpu_fpriv and
moved the drain/wait and the wakeup into amdgpu_ids.c as
amdgpu_pasid_drain_irq() / amdgpu_pasid_drain_irq_done(), both looking the
fpriv up via amdgpu_pasid_get_fpriv_locked() under the PASID lock. KFD now
just passes the PASID. The kfd_irq_drain_list/lock and the per-process
wait_irq_drain/irq_drain_is_open fields are removed.
KFD holds a reference to the DRM file that owns the PASID while it drains,
so the fpriv (and its completion) stays valid after the PASID lock is
dropped.
Thanks for the review,
Heng
发件人: Kuehling, Felix <[email protected]>
发送时间: Thursday, October 1, 2026 5:04 AM
收件人: Zhou, Heng <[email protected]>; [email protected]; Sierra
Guiza, Alejandro (Alex) <[email protected]>
抄送: Lazar, Lijo <[email protected]>; Koenig, Christian
<[email protected]>; Deng, Emily <[email protected]>; Zhao, Victor
<[email protected]>; [email protected]; Ma, Qing (Mark) <[email protected]>;
Chang, HaiJun <[email protected]>; Kim, Jonathan <[email protected]>
主题: Re: [PATCH] drm/amdkfd: fix lost wakeup in interrupt drain on debugged
process exit
On 2026-09-30 09:42, Heng Zhou wrote:
When a debugged process exits, kfd_process_notifier_release_internal()
first removes it from kfd_processes_table and then calls
kfd_dbg_trap_disable(), which drains the process interrupts. The drain
sends a fence through the IH and waits, without a timeout, for
kfd_process_close_interrupt_drain() to wake it up. That wakeup looks
the process up by PASID in kfd_processes_table, which no longer holds
it, so the wakeup is lost and the exiting task sleeps forever. The
fatal signal has already been consumed in do_exit(), so the
interruptible wait cannot be broken either.
The wait happens inside the mmu_notifier release callback, i.e. within
the global mmu_notifier SRCU read-side critical section, so every
synchronize_srcu() on it stalls as well. Any other process releasing
its mm then hangs in D state and the system needs a reboot. This is
seen with rocgdb on MI300X.
I suspect this may have been introduced by this patch by Alex:
commit f34034ce5d2116f397008c63901c9c1946e5d665
Author: Alex Sierra <[email protected]><mailto:[email protected]>
Date: Fri Jul 24 15:53:48 2026 -0500
drm/amdkfd: disable debug before retiring MES process context on teardown
...
If you can confirm that, please add an appropriate Fixes: tag.
Fix this by:
- Tracking pending drains in a list keyed by PASID instead of looking
the process up in kfd_processes_table, so the drain fence always
finds its waiter.
- Bounding the wait with a timeout, since the fence can be lost when
the IH ring or the KFD interrupt FIFO overflows, and warning when the
drain does not complete.
- Deferring the drain of an exiting debugged process from the
mmu_notifier release path to kfd_process_wq_release(), before the
PDDs and their PASIDs are released, so that stale interrupts cannot
hit a process that reuses the PASID.
Signed-off-by: Heng Zhou <[email protected]><mailto:[email protected]>
---
drivers/gpu/drm/amd/amdkfd/kfd_debug.c | 3 +-
drivers/gpu/drm/amd/amdkfd/kfd_priv.h | 7 +-
drivers/gpu/drm/amd/amdkfd/kfd_process.c | 89 +++++++++++++++++-------
3 files changed, 68 insertions(+), 31 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_debug.c
b/drivers/gpu/drm/amd/amdkfd/kfd_debug.c
index 0dd1fd448059..2b227d9de492 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_debug.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_debug.c
@@ -664,7 +664,8 @@ static void kfd_dbg_clean_exception_status(struct
kfd_process *target)
for (i = 0; i < target->n_pdds; i++) {
struct kfd_process_device *pdd = target->pdds[i];
- kfd_process_drain_interrupts(pdd);
+ if (!target->irq_drain_deferred)
+ kfd_process_drain_interrupts(pdd);
The way you're setting and clearing p->irq_drain_deferred seems pretty fragile.
I'd prefer to just pass this as a parameter to kfd_dbg_trap_disable and
kfd_dbg_clean_exception_status. Then you also don't need two different drain
functions.
pdd->exception_status = 0;
}
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
index 2847a5ec5ede..c04c3a03f559 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
@@ -1030,9 +1030,7 @@ struct kfd_process {
uint64_t exception_enable_mask;
uint64_t exception_status;
- /* Used to drain stale interrupts */
- wait_queue_head_t wait_irq_drain;
- bool irq_drain_is_open;
+ bool irq_drain_deferred;
/* shared virtual memory registered by this process */
struct svm_range_list svms;
@@ -1245,7 +1243,8 @@ bool enqueue_ih_ring_entry(struct kfd_node *kfd, const
void *ih_ring_entry);
bool interrupt_is_wanted(struct kfd_node *dev,
const uint32_t *ih_ring_entry,
uint32_t *patched_ihre, bool *flag);
-int kfd_process_drain_interrupts(struct kfd_process_device *pdd);
+void kfd_process_drain_interrupts(struct kfd_process_device *pdd);
+void kfd_process_drain_deferred_interrupts(struct kfd_process *p);
void kfd_process_close_interrupt_drain(unsigned int pasid);
/* amdkfd Apertures */
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
index 226f52626e84..b2365c21db9e 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
@@ -55,6 +55,22 @@ DEFINE_MUTEX(kfd_processes_mutex);
DEFINE_SRCU(kfd_processes_srcu);
+/*
+ * Pending interrupt drains, keyed by PASID. Deliberately not keyed off
+ * kfd_processes_table: a draining process may already have been removed from
+ * it, and the drain fence must still be able to find its waiter.
+ */
+struct kfd_irq_drain_waiter {
+ struct list_head list;
+ u32 pasid;
+ struct completion done;
+};
+
+static DEFINE_SPINLOCK(kfd_irq_drain_lock);
+static LIST_HEAD(kfd_irq_drain_list);
If you use an XArray here, you have a more efficient lookup and you also get
the spin-lock for free. In fact, that xarray already exists in amdgpu_ids.c:
amdgpu_pasid_xa. It stores a pointer to the fpriv. You can get it with
amdgpu_pasid_get_fpriv_locked. So you could just add the completion to struct
amdgpu_fpriv. Maybe the whole IRQ draining logic should move to amdgpu_ids.c in
that case, and KFD would just invoke it with a PASID.
Regards,
Felix
+
+#define KFD_IRQ_DRAIN_TIMEOUT msecs_to_jiffies(1000)
+
/* For process termination handling */
static struct workqueue_struct *kfd_process_wq;
@@ -1019,8 +1035,6 @@ struct kfd_process *kfd_create_process(struct task_struct
*thread)
if (ret)
pr_warn("Failed to create debugfs entry for the kfd_process,
ret = %d\n",
ret);
-
- init_waitqueue_head(&process->wait_irq_drain);
}
out:
mutex_unlock(&kfd_processes_mutex);
@@ -1344,6 +1358,8 @@ static void kfd_process_wq_release(struct work_struct
*work)
kfd_process_free_outstanding_kfd_bos(p);
svm_range_list_fini(p);
+ kfd_process_drain_deferred_interrupts(p);
+
kfd_process_destroy_pdds(p);
dma_fence_put(ef);
@@ -1420,6 +1436,8 @@ void kfd_process_notifier_release_internal(struct
kfd_process *p)
* back on the MES list and, once proc_ctx_bo is freed, MES would fault
* on the freed process context. Retiring the context last avoids this.
*/
+ if (p->debug_trap_enabled)
+ p->irq_drain_deferred = true;
kfd_dbg_trap_disable(p);
if (atomic_read(&p->debugged_process_count) > 0) {
@@ -2313,17 +2331,16 @@ int kfd_resume_all_processes(void)
return ret;
}
-/* assumes caller holds process lock. */
-int kfd_process_drain_interrupts(struct kfd_process_device *pdd)
+void kfd_process_drain_interrupts(struct kfd_process_device *pdd)
{
+ struct kfd_irq_drain_waiter waiter;
uint32_t irq_drain_fence[8];
+ unsigned long flags;
uint8_t node_id = 0;
- int r = 0;
+ long r;
if (!KFD_IS_SOC15(pdd->dev))
- return 0;
-
- pdd->process->irq_drain_is_open = true;
+ return;
memset(irq_drain_fence, 0, sizeof(irq_drain_fence));
irq_drain_fence[0] = (KFD_IRQ_FENCE_SOURCEID << 8) |
@@ -2341,33 +2358,53 @@ int kfd_process_drain_interrupts(struct
kfd_process_device *pdd)
irq_drain_fence[3] |= node_id << 16;
}
+ waiter.pasid = pdd->pasid;
+ init_completion(&waiter.done);
+
+ spin_lock_irqsave(&kfd_irq_drain_lock, flags);
+ list_add(&waiter.list, &kfd_irq_drain_list);
+ spin_unlock_irqrestore(&kfd_irq_drain_lock, flags);
+
/* ensure stale irqs scheduled KFD interrupts and send drain fence. */
- if (amdgpu_amdkfd_send_close_event_drain_irq(pdd->dev->adev,
- irq_drain_fence)) {
- pdd->process->irq_drain_is_open = false;
- return 0;
+ if (!amdgpu_amdkfd_send_close_event_drain_irq(pdd->dev->adev,
+ irq_drain_fence)) {
+ r = wait_for_completion_interruptible_timeout(&waiter.done,
+ KFD_IRQ_DRAIN_TIMEOUT);
+ if (r <= 0)
+ dev_warn_ratelimited(pdd->dev->adev->dev,
+ "irq drain on pasid 0x%x did not complete
(%ld)\n",
+ waiter.pasid, r);
}
- r = wait_event_interruptible(pdd->process->wait_irq_drain,
- !READ_ONCE(pdd->process->irq_drain_is_open));
- if (r)
- pdd->process->irq_drain_is_open = false;
-
- return r;
+ spin_lock_irqsave(&kfd_irq_drain_lock, flags);
+ list_del(&waiter.list);
+ spin_unlock_irqrestore(&kfd_irq_drain_lock, flags);
}
-void kfd_process_close_interrupt_drain(unsigned int pasid)
+void kfd_process_drain_deferred_interrupts(struct kfd_process *p)
{
- struct kfd_process *p;
-
- p = kfd_lookup_process_by_pasid(pasid, NULL);
+ int i;
- if (!p)
+ if (!p->irq_drain_deferred)
return;
- WRITE_ONCE(p->irq_drain_is_open, false);
- wake_up_all(&p->wait_irq_drain);
- kfd_unref_process(p);
+ p->irq_drain_deferred = false;
+
+ for (i = 0; i < p->n_pdds; i++)
+ kfd_process_drain_interrupts(p->pdds[i]);
+}
+
+void kfd_process_close_interrupt_drain(unsigned int pasid)
+{
+ struct kfd_irq_drain_waiter *waiter;
+ unsigned long flags;
+
+ spin_lock_irqsave(&kfd_irq_drain_lock, flags);
+ list_for_each_entry(waiter, &kfd_irq_drain_list, list) {
+ if (waiter->pasid == pasid)
+ complete(&waiter->done);
+ }
+ spin_unlock_irqrestore(&kfd_irq_drain_lock, flags);
}
struct send_exception_work_handler_workarea {
