On 2023-11-23 15:32, Felix Kuehling wrote:
On 2023-11-23 15:18, James Zhu wrote:
On 2023-11-22 17:15, Felix Kuehling wrote:
On 2023-11-03 09:11, James Zhu wrote:
Check pcs_enrty valid for pc sampling ioctl.
Signed-off-by: James Zhu <James.Zhu@xxxxxxx>
---
drivers/gpu/drm/amd/amdkfd/kfd_pc_sampling.c | 30
++++++++++++++++++--
1 file changed, 27 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_pc_sampling.c
b/drivers/gpu/drm/amd/amdkfd/kfd_pc_sampling.c
index 4c9fc48e1a6a..36366c8847de 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_pc_sampling.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_pc_sampling.c
@@ -179,6 +179,21 @@ static int kfd_pc_sample_destroy(struct
kfd_process_device *pdd, uint32_t trace_
int kfd_pc_sample(struct kfd_process_device *pdd,
struct kfd_ioctl_pc_sample_args __user *args)
{
+ struct pc_sampling_entry *pcs_entry;
+
+ if (args->op != KFD_IOCTL_PCS_OP_QUERY_CAPABILITIES &&
+ args->op != KFD_IOCTL_PCS_OP_CREATE) {
+
+ mutex_lock(&pdd->dev->pcs_data.mutex);
+ pcs_entry =
idr_find(&pdd->dev->pcs_data.hosttrap_entry.base.pc_sampling_idr,
+ args->trace_id);
+ mutex_unlock(&pdd->dev->pcs_data.mutex);
You need to keep holding the lock while the pcs_entry is still used.
That includes any of the kfd_pc_sample_<op> functions below.
Otherwise someone could free it concurrently. It would also simplify
the ..._<op> functions, if they didn't have to worry about the
locking themselves.
[JZ] pcs_entry is only for this pc sampling process, which has
kfd_process->mutex protected here.
OK. That's not obvious. I'm also wary about depending too much on the
big process lock. We will need to make that locking more granular
soon, because it is causing performance issues with multi-threaded
processes.
[Jz] Let me add some comments on pcs_entry.
Regards,
Felix
Regards,
Felix
+
+ if (!pcs_entry ||
+ pcs_entry->pdd != pdd)
+ return -EINVAL;
+ }
+
switch (args->op) {
case KFD_IOCTL_PCS_OP_QUERY_CAPABILITIES:
return kfd_pc_sample_query_cap(pdd, args);
@@ -187,13 +202,22 @@ int kfd_pc_sample(struct kfd_process_device
*pdd,
return kfd_pc_sample_create(pdd, args);
case KFD_IOCTL_PCS_OP_DESTROY:
- return kfd_pc_sample_destroy(pdd, args->trace_id);
+ if (pcs_entry->enabled)
+ return -EBUSY;
+ else
+ return kfd_pc_sample_destroy(pdd, args->trace_id);
case KFD_IOCTL_PCS_OP_START:
- return kfd_pc_sample_start(pdd);
+ if (pcs_entry->enabled)
+ return -EALREADY;
+ else
+ return kfd_pc_sample_start(pdd);
case KFD_IOCTL_PCS_OP_STOP:
- return kfd_pc_sample_stop(pdd);
+ if (!pcs_entry->enabled)
+ return -EALREADY;
+ else
+ return kfd_pc_sample_stop(pdd);
}
return -EINVAL;
