On 2024-08-02 11:28, Philip Yang wrote:
Add helper function kfd_queue_unreference_buffers to reduce queue buffer
refcount, separate it from release queue buffers.
Because it is circular locking to hold dqm_lock to take vm lock,
kfd_ioctl_destroy_queue should take vm lock, unreference queue buffers
first, but not release queue buffers, to handle error in case failed to
hold vm lock. Then hold dqm_lock to remove queue from queue list and
then release queue buffers.
Restore process worker restore queue hold dqm_lock, will always find
the queue with valid queue buffers.
Signed-off-by: Philip Yang <Philip.Yang@xxxxxxx>
---
drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 1 +
drivers/gpu/drm/amd/amdkfd/kfd_priv.h | 5 +-
.../amd/amdkfd/kfd_process_queue_manager.c | 8 ++-
drivers/gpu/drm/amd/amdkfd/kfd_queue.c | 62 ++++++++++++-------
4 files changed, 49 insertions(+), 27 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
index 0622ebd7e8ef..10d6e29b23cb 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
@@ -400,6 +400,7 @@ static int kfd_ioctl_create_queue(struct file *filep, struct kfd_process *p,
return 0;
err_create_queue:
+ kfd_queue_unreference_buffers(pdd, &q_properties);
kfd_queue_release_buffers(pdd, &q_properties);
The naming of these functions is a bit unfortunate because
kfd_queue_release_buffers actually unreferences the buffers, and
kfd_queue_unreference_buffers affects the virtual address mappings
(technically amdgpu_bo_vas), not the buffers themselves. I would suggest
the following rename:
kfd_queue_unreference_buffers -> kfd_queue_unref_bo_vas
err_acquire_queue_buf:
err_sdma_engine_id:
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
index 057d20446c31..e38484b40467 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
@@ -1298,9 +1298,12 @@ void print_queue_properties(struct queue_properties *q);
void print_queue(struct queue *q);
int kfd_queue_buffer_get(struct amdgpu_vm *vm, void __user *addr, struct amdgpu_bo **pbo,
u64 expected_size);
-void kfd_queue_buffer_put(struct amdgpu_vm *vm, struct amdgpu_bo **bo);
+void kfd_queue_buffer_put(struct amdgpu_bo **bo);
int kfd_queue_acquire_buffers(struct kfd_process_device *pdd, struct queue_properties *properties);
int kfd_queue_release_buffers(struct kfd_process_device *pdd, struct queue_properties *properties);
+void kfd_queue_unreference_buffer(struct amdgpu_vm *vm, struct amdgpu_bo **bo);
+int kfd_queue_unreference_buffers(struct kfd_process_device *pdd,
+ struct queue_properties *properties);
void kfd_queue_ctx_save_restore_size(struct kfd_topology_device *dev);
struct mqd_manager *mqd_manager_init_cik(enum KFD_MQD_TYPE type,
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 f732ee35b531..ef76a9cbc7e2 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
@@ -217,6 +217,7 @@ void pqm_uninit(struct process_queue_manager *pqm)
list_for_each_entry_safe(pqn, next, &pqm->queues, process_queue_list) {
if (pqn->q) {
pdd = kfd_get_process_device_data(pqn->q->device, pqm->process);
+ kfd_queue_unreference_buffers(pdd, &pqn->q->properties);
kfd_queue_release_buffers(pdd, &pqn->q->properties);
pqm_clean_queue_resource(pqm, pqn);
}
@@ -512,7 +513,7 @@ int pqm_destroy_queue(struct process_queue_manager *pqm, unsigned int qid)
}
if (pqn->q) {
- retval = kfd_queue_release_buffers(pdd, &pqn->q->properties);
+ retval = kfd_queue_unreference_buffers(pdd, &pqn->q->properties);
if (retval)
goto err_destroy_queue;
@@ -526,7 +527,7 @@ int pqm_destroy_queue(struct process_queue_manager *pqm, unsigned int qid)
if (retval != -ETIME)
goto err_destroy_queue;
}
-
+ kfd_queue_release_buffers(pdd, &pqn->q->properties);
pqm_clean_queue_resource(pqm, pqn);
uninit_queue(pqn->q);
}
@@ -579,7 +580,8 @@ int pqm_update_queue_properties(struct process_queue_manager *pqm,
return -EFAULT;
}
- kfd_queue_buffer_put(vm, &pqn->q->properties.ring_bo);
+ kfd_queue_unreference_buffer(vm, &pqn->q->properties.ring_bo);
+ kfd_queue_buffer_put(&pqn->q->properties.ring_bo);
amdgpu_bo_unreserve(vm->root.bo);
pqn->q->properties.ring_bo = p->ring_bo;
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_queue.c b/drivers/gpu/drm/amd/amdkfd/kfd_queue.c
index e0a073ae4a49..9ac15dff527f 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_queue.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_queue.c
@@ -224,16 +224,8 @@ int kfd_queue_buffer_get(struct amdgpu_vm *vm, void __user *addr, struct amdgpu_
return -EINVAL;
}
-void kfd_queue_buffer_put(struct amdgpu_vm *vm, struct amdgpu_bo **bo)
+void kfd_queue_buffer_put(struct amdgpu_bo **bo)
{
- if (*bo) {
- struct amdgpu_bo_va *bo_va;
-
- bo_va = amdgpu_vm_bo_find(vm, *bo);
- if (bo_va)
- bo_va->queue_refcount--;
- }
-
amdgpu_bo_unref(bo);
You can remove this function and just call amdgpu_bo_unref directly.
}
@@ -327,6 +319,7 @@ int kfd_queue_acquire_buffers(struct kfd_process_device *pdd, struct queue_prope
out_err_unreserve:
amdgpu_bo_unreserve(vm->root.bo);
out_err_release:
+ kfd_queue_unreference_buffers(pdd, properties);
While you're cleaning this up, I'd recommend you add a version
kfd_queue_unref_bo_va_locked that assumes the caller is holding the VM
reservation and call it before amdgpu_bo_unreserve. That removes one
potential leak on the error handling code path.
Regards,
Felix
kfd_queue_release_buffers(pdd, properties);
return err;
}
@@ -334,22 +327,13 @@ int kfd_queue_acquire_buffers(struct kfd_process_device *pdd, struct queue_prope
int kfd_queue_release_buffers(struct kfd_process_device *pdd, struct queue_properties *properties)
{
struct kfd_topology_device *topo_dev;
- struct amdgpu_vm *vm;
u32 total_cwsr_size;
- int err;
-
- vm = drm_priv_to_vm(pdd->drm_priv);
- err = amdgpu_bo_reserve(vm->root.bo, false);
- if (err)
- return err;
-
- kfd_queue_buffer_put(vm, &properties->wptr_bo);
- kfd_queue_buffer_put(vm, &properties->rptr_bo);
- kfd_queue_buffer_put(vm, &properties->ring_bo);
- kfd_queue_buffer_put(vm, &properties->eop_buf_bo);
- kfd_queue_buffer_put(vm, &properties->cwsr_bo);
- amdgpu_bo_unreserve(vm->root.bo);
+ kfd_queue_buffer_put(&properties->wptr_bo);
+ kfd_queue_buffer_put(&properties->rptr_bo);
+ kfd_queue_buffer_put(&properties->ring_bo);
+ kfd_queue_buffer_put(&properties->eop_buf_bo);
+ kfd_queue_buffer_put(&properties->cwsr_bo);
topo_dev = kfd_topology_device_by_id(pdd->dev->id);
if (!topo_dev)
@@ -362,6 +346,38 @@ int kfd_queue_release_buffers(struct kfd_process_device *pdd, struct queue_prope
return 0;
}
+void kfd_queue_unreference_buffer(struct amdgpu_vm *vm, struct amdgpu_bo **bo)
+{
+ if (*bo) {
+ struct amdgpu_bo_va *bo_va;
+
+ bo_va = amdgpu_vm_bo_find(vm, *bo);
+ if (bo_va && bo_va->queue_refcount)
+ bo_va->queue_refcount--;
+ }
+}
+
+int kfd_queue_unreference_buffers(struct kfd_process_device *pdd,
+ struct queue_properties *properties)
+{
+ struct amdgpu_vm *vm;
+ int err;
+
+ vm = drm_priv_to_vm(pdd->drm_priv);
+ err = amdgpu_bo_reserve(vm->root.bo, false);
+ if (err)
+ return err;
+
+ kfd_queue_unreference_buffer(vm, &properties->wptr_bo);
+ kfd_queue_unreference_buffer(vm, &properties->rptr_bo);
+ kfd_queue_unreference_buffer(vm, &properties->ring_bo);
+ kfd_queue_unreference_buffer(vm, &properties->eop_buf_bo);
+ kfd_queue_unreference_buffer(vm, &properties->cwsr_bo);
+
+ amdgpu_bo_unreserve(vm->root.bo);
+ return 0;
+}
+
#define SGPR_SIZE_PER_CU 0x4000
#define LDS_SIZE_PER_CU 0x10000
#define HWREG_SIZE_PER_CU 0x1000
