Re: [PATCH] amdkfd: fix deadlock in set_perfcount

[+Benjamin]

On 2026-07-31 05:51, Zhu Lingshan wrote:
> set_perfcount holds the DQM lock (dqm_lock(dqm)
> while walking queues, but pqm_update_mqd
> calls update_queue, which acquires the
> same lock by dqm_lock(dqm), causing deadlocks.
>
> This commit introduces update_queue_locked which assumes
> the dqm lock is already held, and update_queue calls it
> with proper lockings.
>
> set_perfcount calls update_queue_locked instead of pqm_update_mqd,
> therefore the deadlock is fixed.

I think the patch looks fine to me. I'm just wondering how this bug was 
not noticed in developer testing of this feature. I see a very similar 
patch already exists on the DKMS branch but somehow it's missing in 
amd-staging-drm-next:

commit 5cd0710732a45527b6398f8b522289f9adca708b
Author:     Benjamin Welton<[email protected]>
AuthorDate: Mon Jan 6 10:30:01 2025 -0800
Commit:     Chengjun Yao<[email protected]>
CommitDate: Mon Jun 22 12:51:06 2026 +0800

     amd/amdkfd: fix double lock aquisition in set_perfcount
     
     Seperates out locking from update_queue to allow updating
     of queues by code already holding the mqd lock. Fixes a
     hang in set_perfcount. This change was in the original
     mailing list commit for set_perfcount but was not included
     in gerrit.
     
     Fixes: b58289f0abf7 ("Add kfd_ioctl_profiler to contain profiler kernel driver changes")
     
     Signed-off-by: Benjamin Welton<[email protected]>
     Acked-by: Kent Russell<[email protected]>


Benjamin, do you know what's going on here? I think the above patch just 
needs to be ported to amd-staging-drm-next. But then I'm wondering what 
else is missing. Or maybe the whole feature was submitted to 
amd-staging-drm-next by mistake.

Regards,
   Felix


>
> Signed-off-by: Zhu Lingshan<[email protected]>
> ---
>   .../drm/amd/amdkfd/kfd_device_queue_manager.c | 44 ++++++++++++-------
>   1 file changed, 27 insertions(+), 17 deletions(-)
>
> 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 51ee9c39104b..c34c5f6a5541 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> @@ -77,6 +77,9 @@ static struct queue *find_queue_by_doorbell_offset(struct device_queue_manager *
>   static void set_queue_as_reset(struct device_queue_manager *dqm, struct queue *q,
>   			       struct qcm_process_device *qpd);
>   static int reset_queues_mes(struct device_queue_manager *dqm, struct queue *q);
> +static int update_queue_locked(struct device_queue_manager *dqm,
> +			       struct queue *q,
> +			       struct mqd_update_info *minfo);
>   
>   static inline
>   enum KFD_MQD_TYPE get_mqd_type_from_queue_type(enum kfd_queue_type type)
> @@ -341,8 +344,7 @@ static void set_perfcount(struct device_queue_manager *dqm, int enable)
>   	list_for_each_entry(cur, &dqm->queues, list) {
>   		qpd = cur->qpd;
>   		list_for_each_entry(q, &qpd->queues_list, list) {
> -			pqm_update_mqd(qpd->pqm, q->properties.queue_id,
> -						&minfo);
> +			update_queue_locked(dqm, q, &minfo);
>   		}
>   	}
>   	dqm_unlock(dqm);
> @@ -1059,8 +1061,9 @@ static int destroy_queue_nocpsch(struct device_queue_manager *dqm,
>   	return retval;
>   }
>   
> -static int update_queue(struct device_queue_manager *dqm, struct queue *q,
> -			struct mqd_update_info *minfo)
> +static int update_queue_locked(struct device_queue_manager *dqm,
> +			       struct queue *q,
> +			       struct mqd_update_info *minfo)
>   {
>   	int retval = 0;
>   	struct device *dev = dqm->dev->adev->dev;
> @@ -1068,12 +1071,11 @@ static int update_queue(struct device_queue_manager *dqm, struct queue *q,
>   	struct kfd_process_device *pdd;
>   	bool prev_active = false;
>   
> -	dqm_lock(dqm);
> +	lockdep_assert_held(&dqm->lock_hidden);
> +
>   	pdd = kfd_get_process_device_data(q->device, q->process);
> -	if (!pdd) {
> -		retval = -ENODEV;
> -		goto out_unlock;
> -	}
> +	if (!pdd)
> +		return -ENODEV;
>   	mqd_mgr = dqm->mqd_mgrs[get_mqd_type_from_queue_type(
>   			q->properties.type)];
>   
> @@ -1089,14 +1091,12 @@ static int update_queue(struct device_queue_manager *dqm, struct queue *q,
>   			retval = remove_queue_mes(dqm, q, &pdd->qpd);
>   
>   		/* queue is reset so inaccessable  */
> -		if (pdd->has_reset_queue) {
> -			retval = -EACCES;
> -			goto out_unlock;
> -		}
> +		if (pdd->has_reset_queue)
> +			return -EACCES;
>   
>   		if (retval) {
>   			dev_err(dev, "unmap queue failed\n");
> -			goto out_unlock;
> +			return retval;
>   		}
>   	} else if (prev_active &&
>   		   (q->properties.type == KFD_QUEUE_TYPE_COMPUTE ||
> @@ -1105,7 +1105,7 @@ static int update_queue(struct device_queue_manager *dqm, struct queue *q,
>   
>   		if (!dqm->sched_running) {
>   			WARN_ONCE(1, "Update non-HWS queue while stopped\n");
> -			goto out_unlock;
> +			return -EIO;
>   		}
>   
>   		retval = mqd_mgr->destroy_mqd(mqd_mgr, q->mqd,
> @@ -1115,7 +1115,7 @@ static int update_queue(struct device_queue_manager *dqm, struct queue *q,
>   				KFD_UNMAP_LATENCY_MS, q->pipe, q->queue);
>   		if (retval) {
>   			dev_err(dev, "destroy mqd failed\n");
> -			goto out_unlock;
> +			return retval;
>   		}
>   	}
>   
> @@ -1163,8 +1163,18 @@ static int update_queue(struct device_queue_manager *dqm, struct queue *q,
>   						   &q->properties, current->mm);
>   	}
>   
> -out_unlock:
> +	return retval;
> +}
> +
> +static int update_queue(struct device_queue_manager *dqm, struct queue *q,
> +			struct mqd_update_info *minfo)
> +{
> +	int retval;
> +
> +	dqm_lock(dqm);
> +	retval = update_queue_locked(dqm, q, minfo);
>   	dqm_unlock(dqm);
> +
>   	return retval;
>   }
>