Re: [PATCH] amdkfd: fix deadlock in set_perfcount

Tue, 04 Aug 2026 15:41:31 -0700 

[+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;
  }

Reply via email to