On 8/14/2026 12:05 AM, Kuehling, Felix wrote: > 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. do you mean some pthreads in a process that own the same kfd_process? > > 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.
I will send out a patch to fix this part. Thanks Lingshan > > 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 >>>>
