* [PATCH 2/2] drm/amdkfd: Free queue after unmap queue success
2022-06-15 21:56 [PATCH 1/2] drm/amdkfd: Add queue to MES if it becomes active Philip Yang
@ 2022-06-15 21:56 ` Philip Yang
2022-06-16 2:57 ` [PATCH 1/2] drm/amdkfd: Add queue to MES if it becomes active Sider, Graham
2022-06-16 14:49 ` Eric Huang
2 siblings, 0 replies; 5+ messages in thread
From: Philip Yang @ 2022-06-15 21:56 UTC (permalink / raw)
To: amd-gfx; +Cc: Philip Yang, Graham.Sider
After queue unmap or remove from MES successfully, free queue sysfs
entries, doorbell and remove from queue list. Otherwise, application may
destroy queue again, cause below kernel warning or crash backtrace.
For outstanding queues, either application forget to destroy or failed
to destroy, kfd_process_notifier_release will remove queue sysfs
entries, kfd_process_wq_release will free queue doorbell.
refcount_t: underflow; use-after-free.
WARNING: CPU: 7 PID: 3053 at lib/refcount.c:28
Call Trace:
kobject_put+0xd6/0x1a0
kfd_procfs_del_queue+0x27/0x30 [amdgpu]
pqm_destroy_queue+0xeb/0x240 [amdgpu]
kfd_ioctl_destroy_queue+0x32/0x70 [amdgpu]
kfd_ioctl+0x27d/0x500 [amdgpu]
do_syscall_64+0x35/0x80
WARNING: CPU: 2 PID: 3053 at drivers/gpu/drm/amd/amdgpu/../amdkfd/kfd_device_queue_manager.c:400
Call Trace:
deallocate_doorbell.isra.0+0x39/0x40 [amdgpu]
destroy_queue_cpsch+0xb3/0x270 [amdgpu]
pqm_destroy_queue+0x108/0x240 [amdgpu]
kfd_ioctl_destroy_queue+0x32/0x70 [amdgpu]
kfd_ioctl+0x27d/0x500 [amdgpu]
general protection fault, probably for non-canonical address
0xdead000000000108:
Call Trace:
pqm_destroy_queue+0xf0/0x200 [amdgpu]
kfd_ioctl_destroy_queue+0x2f/0x60 [amdgpu]
kfd_ioctl+0x19b/0x600 [amdgpu]
Signed-off-by: Philip Yang <Philip.Yang@amd.com>
---
.../drm/amd/amdkfd/kfd_device_queue_manager.c | 27 +++++++++++--------
.../amd/amdkfd/kfd_process_queue_manager.c | 2 +-
2 files changed, 17 insertions(+), 12 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 67ae5b6385a2..d6914a78348d 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
@@ -1875,6 +1875,21 @@ static int destroy_queue_cpsch(struct device_queue_manager *dqm,
}
+ if (q->properties.is_active) {
+ if (!dqm->dev->shared_resources.enable_mes) {
+ decrement_queue_count(dqm, qpd, q);
+ retval = execute_queues_cpsch(dqm,
+ KFD_UNMAP_QUEUES_FILTER_DYNAMIC_QUEUES, 0);
+ if (retval == -ETIME)
+ qpd->reset_wavefronts = true;
+ } else {
+ retval = remove_queue_mes(dqm, q, qpd);
+ }
+
+ if (retval)
+ goto failed_unmap_queue;
+ }
+
mqd_mgr = dqm->mqd_mgrs[get_mqd_type_from_queue_type(
q->properties.type)];
@@ -1888,17 +1903,6 @@ static int destroy_queue_cpsch(struct device_queue_manager *dqm,
list_del(&q->list);
qpd->queue_count--;
- if (q->properties.is_active) {
- if (!dqm->dev->shared_resources.enable_mes) {
- decrement_queue_count(dqm, qpd, q);
- retval = execute_queues_cpsch(dqm,
- KFD_UNMAP_QUEUES_FILTER_DYNAMIC_QUEUES, 0);
- if (retval == -ETIME)
- qpd->reset_wavefronts = true;
- } else {
- retval = remove_queue_mes(dqm, q, qpd);
- }
- }
/*
* Unconditionally decrement this counter, regardless of the queue's
@@ -1915,6 +1919,7 @@ static int destroy_queue_cpsch(struct device_queue_manager *dqm,
return retval;
+failed_unmap_queue:
failed_try_destroy_debugged_queue:
dqm_unlock(dqm);
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c b/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
index dc00484ff484..99f2a6412201 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
@@ -419,7 +419,6 @@ int pqm_destroy_queue(struct process_queue_manager *pqm, unsigned int qid)
}
if (pqn->q) {
- kfd_procfs_del_queue(pqn->q);
dqm = pqn->q->device->dqm;
retval = dqm->ops.destroy_queue(dqm, &pdd->qpd, pqn->q);
if (retval) {
@@ -439,6 +438,7 @@ int pqm_destroy_queue(struct process_queue_manager *pqm, unsigned int qid)
if (dev->shared_resources.enable_mes)
amdgpu_amdkfd_free_gtt_mem(dev->adev,
pqn->q->gang_ctx_bo);
+ kfd_procfs_del_queue(pqn->q);
uninit_queue(pqn->q);
}
--
2.35.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* RE: [PATCH 1/2] drm/amdkfd: Add queue to MES if it becomes active
2022-06-15 21:56 [PATCH 1/2] drm/amdkfd: Add queue to MES if it becomes active Philip Yang
2022-06-15 21:56 ` [PATCH 2/2] drm/amdkfd: Free queue after unmap queue success Philip Yang
@ 2022-06-16 2:57 ` Sider, Graham
2022-06-16 14:49 ` Eric Huang
2 siblings, 0 replies; 5+ messages in thread
From: Sider, Graham @ 2022-06-16 2:57 UTC (permalink / raw)
To: Yang, Philip, amd-gfx
[Public]
Reviewed-by: Graham Sider <Graham.Sider@amd.com>
> -----Original Message-----
> From: Yang, Philip <Philip.Yang@amd.com>
> Sent: Wednesday, June 15, 2022 5:57 PM
> To: amd-gfx@lists.freedesktop.org
> Cc: Sider, Graham <Graham.Sider@amd.com>; Yang, Philip
> <Philip.Yang@amd.com>
> Subject: [PATCH 1/2] drm/amdkfd: Add queue to MES if it becomes active
>
> We remove the user queue from MES scheduler to update queue
> properties.
> If the queue becomes active after updating, add the user queue to MES
> scheduler, to be able to handle command packet submission.
>
> Signed-off-by: Philip Yang <Philip.Yang@amd.com>
> ---
> drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 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 e1797657b04c..67ae5b6385a2 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> @@ -866,8 +866,10 @@ static int update_queue(struct
> device_queue_manager *dqm, struct queue *q,
> * dqm->active_queue_count to determine whether a new runlist
> must be
> * uploaded.
> */
> - if (q->properties.is_active && !prev_active) {
> - increment_queue_count(dqm, &pdd->qpd, q);
> + if (q->properties.is_active) {
> + add_queue = true;
> + if (!prev_active)
> + increment_queue_count(dqm, &pdd->qpd, q);
> } else if (!q->properties.is_active && prev_active) {
> decrement_queue_count(dqm, &pdd->qpd, q);
> } else if (q->gws && !q->properties.is_gws) {
> --
> 2.35.1
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH 1/2] drm/amdkfd: Add queue to MES if it becomes active
2022-06-15 21:56 [PATCH 1/2] drm/amdkfd: Add queue to MES if it becomes active Philip Yang
2022-06-15 21:56 ` [PATCH 2/2] drm/amdkfd: Free queue after unmap queue success Philip Yang
2022-06-16 2:57 ` [PATCH 1/2] drm/amdkfd: Add queue to MES if it becomes active Sider, Graham
@ 2022-06-16 14:49 ` Eric Huang
2022-06-17 19:13 ` philip yang
2 siblings, 1 reply; 5+ messages in thread
From: Eric Huang @ 2022-06-16 14:49 UTC (permalink / raw)
To: amd-gfx, Yang, Philip
Does it break the case of q->gws with q->properties.is_active == true?
Regards,
Eric
On 2022-06-15 17:56, Philip Yang wrote:
> We remove the user queue from MES scheduler to update queue properties.
> If the queue becomes active after updating, add the user queue to MES
> scheduler, to be able to handle command packet submission.
>
> Signed-off-by: Philip Yang <Philip.Yang@amd.com>
> ---
> drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 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 e1797657b04c..67ae5b6385a2 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> @@ -866,8 +866,10 @@ static int update_queue(struct device_queue_manager *dqm, struct queue *q,
> * dqm->active_queue_count to determine whether a new runlist must be
> * uploaded.
> */
> - if (q->properties.is_active && !prev_active) {
> - increment_queue_count(dqm, &pdd->qpd, q);
> + if (q->properties.is_active) {
> + add_queue = true;
> + if (!prev_active)
> + increment_queue_count(dqm, &pdd->qpd, q);
> } else if (!q->properties.is_active && prev_active) {
> decrement_queue_count(dqm, &pdd->qpd, q);
> } else if (q->gws && !q->properties.is_gws) {
^ permalink raw reply [flat|nested] 5+ messages in thread