RE: [PATCH] amdkfd: fix deadlock in set_perfcount

AMD General

Interesting, wonder how this was dropped. This has been present and been used by customers for a while (and this is an issue they would absolutely hit).

There was some weirdness in the migration if I remember correctly from DKMS -> Public when  a migration for the PTL work took place and I suspect that maybe the change just wasn’t picked up or was dropped as part of that migration. What two branches do you need to check here?

Ben

From: Kuehling, Felix <[email protected]>
Sent: Tuesday, August 4, 2026 3:41 PM
To: Zhu, Lingshan <[email protected]>; Deucher, Alexander <[email protected]>; Koenig, Christian <[email protected]>; Welton, Benjamin <[email protected]>
Cc: Huang, Ray <[email protected]>; [email protected]
Subject: 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]><mailto:[email protected]>

AuthorDate: Mon Jan 6 10:30:01 2025 -0800

Commit:     Chengjun Yao <[email protected]><mailto:[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]><mailto:[email protected]>

    Acked-by: Kent Russell <[email protected]><mailto:[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]><mailto:[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;

 }