Re: [PATCH] amdkfd: allow profiler process reenter

Hello Felix,

Yes I agree the user space may be buggy, and the kernel should be robust.

Even though it is named locking, it is actually a reservation, not recursive locking.
The kfd_process that successfully invokes the ioctl reserves the device as a profiler. 
So when a user space locks the device multiple times, it does not increase any reference counters, and doesn't need to unlock multiple times.

Multiple times lockings from the same kfd_process just keeps the reservation, it still reserves the device successfully,
so IMHO kfd should return 0 other than -EALREADY, which is a negative error code.

By the way, the function is declared to return uint32_t, which is definitely wrong, because the function may return errors, for example -EBUSY.

Thanks
Lingshan

On 8/13/2026 12:27 AM, Kuehling, Felix wrote:

> I suspect this won't work correctly. If the profiler locks the device
> multiple times, it probably also unlocks it multiple times. But I see
> no reference counting being done, so this may result in a situation
> where user mode locks twice, and unlocks once. Now it's lost the lock
> but it was still expecting to have it from the first lock operation.
>
> I suspect that this is a user mode bug, and KFD is working as expected.
>
> Regards,
>   Felix
>
> On 2026-08-12 03:14, Zhu, Lingshan wrote:
>> AMD General
>>
>> Gentle Ping.
>>
>> Thanks
>> Lingshan
>>
>> -----Original Message-----
>> From: Zhu, Lingshan <[email protected]>
>> Sent: Thursday, August 6, 2026 4:43 PM
>> To: Deucher, Alexander <[email protected]>; Koenig, Christian
>> <[email protected]>; Kuehling, Felix <[email protected]>
>> Cc: Huang, Ray <[email protected]>; [email protected];
>> Zhu, Lingshan <[email protected]>
>> Subject: [PATCH] amdkfd: allow profiler process reenter
>>
>> profile_lock_device() currently returns -EALREADY when the current
>> profiler process requests to lock the device again. However any
>> negtive values here are treated as failure, so this causes rejecting
>> a profiler operation even though the caller still owns the device
>> locking.
>>
>> This commit fixes this problem by returning 0 when the current
>> profiler process locking the device again.
>>
>> This commit also changes profile_lock_device() to return an int value
>> other than uint32_t, because it returns negative number when fail.
>>
>> Signed-off-by: Zhu Lingshan <[email protected]>
>> ---
>>   drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
>> b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
>> index 309510e23315..9acd5c91e51c 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
>> @@ -3329,8 +3329,8 @@ static int kfd_ioctl_create_process(struct file
>> *filep, struct kfd_process *p, v
>>          return 0;
>>   }
>>
>> -static inline uint32_t profile_lock_device(struct kfd_process *p,
>> -                                          uint32_t gpu_id, uint32_t op)
>> +static inline int profile_lock_device(struct kfd_process *p,
>> +                                     uint32_t gpu_id, uint32_t op)
>>   {
>>          struct kfd_process_device *pdd;
>>          struct kfd_dev *kfd;
>> @@ -3365,7 +3365,7 @@ static inline uint32_t
>> profile_lock_device(struct kfd_process *p,
>>                          }
>>                          return status;
>>                  } else if (kfd->profiler_process == p) {
>> -                       status = -EALREADY;
>> +                       status = 0;
>>                  } else {
>>                          status = -EBUSY;
>>                  }
>> -- 
>> 2.53.0
>>