Re: [PATCH] drm/amdkfd: fix lost wakeup in interrupt drain on debugged process exit

["Kuehling, Felix" <felix.kuehling@amd.com>]

[-- Attachment #1: Type: text/plain, Size: 9529 bytes --]

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<alex.sierra@amd.com>
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<Heng.Zhou@amd.com>
> ---
>   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 {

[-- Attachment #2: Type: text/html, Size: 10334 bytes --]