Re: [PATCH] amdkfd: allow profiler process reenter

On 2026-08-13 02:21, Zhu, Lingshan wrote:
> 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.

That's exactly the problem. The API is apparently designed for user mode 
to "reserve" profiler access once, presumably to get exclusive access to 
some profiler resources. If user space reserves multiple times, you 
probably have multiple uncoordinated users of the profiler features in 
the same process. If they are not aware of each other, they will run 
into problems because they're expecting exclusive access to resources 
that are actually shared with someone else.

For example they will both independently call "unreserve" as well. When 
the first one calls "unreserve", the other one breaks. This may happen 
intermittently, depending on the order of operations. Or it may be 
noticed as a regression after seemingly harmless changes that affect the 
order of operations. This is much harder to debug than a clear error 
code returned when a double reservation first happens.

By returning 0 here, you paper over the user mode bug. But chances are, 
you're going to run into other bugs later that have the same cause and 
are much more difficult to debug. Unless someone explains to me that I'm 
misunderstanding the intent of the reservation API, I will not approve 
this change.


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

I agree with this part.

Regards,
   Felix


>
> 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
>>>