[PATCH v2 3/3] drm/amdkfd: don't sleep when event age unmatch

[James Zhu]

On 2023-06-07 13:27, Felix Kuehling wrote:
> On 2023-06-06 12:24, James Zhu wrote:
>> Don't sleep when event age unmatch, and update last_event_age.
>> It is only for KFD_EVENT_TYPE_SIGNAL which is checked by user space.
>>
>> Signed-off-by: James Zhu <James.Zhu at amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdkfd/kfd_events.c | 15 +++++++++++++++
>>   1 file changed, 15 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c 
>> b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
>> index c7689181cc22..f4ceb5be78ed 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
>> @@ -952,6 +952,21 @@ int kfd_wait_on_events(struct kfd_process *p,
>>                       event_data.event_id);
>>           if (ret)
>>               goto out_unlock;
>> +
>> +        /* last_event_age = 0 reserved for backward compatible */
>> +        if (event_data.signal_event_data.last_event_age &&
>> +            event_waiters[i].event->event_age !=
>> +                event_data.signal_event_data.last_event_age) {
>> +            event_data.signal_event_data.last_event_age =
>> +                event_waiters[i].event->event_age;
>
> The event_age is updated in set_event under the event->spin_lock. You 
> need to take that lock for this check here as well.
>
> I think the easiest way to do this would be to move the check into 
> init_event_waiter. That way you can initialize the waiter as activated 
> if the event age is not up to date.
[JZ] Sure
>
>
>> + WRITE_ONCE(event_waiters[i].activated, true);
>> +
>> +            if (copy_to_user(&events[i], &event_data,
>> +                sizeof(struct kfd_event_data))) {
>> +                ret = -EFAULT;
>> +                goto out_unlock;
>> +            }
>> +        }
>
> I think we also need to update the event age in event data after an 
> event has signaled. You should probably move updating and copying of 
> the event age to user mode into copy_signaled_event_data. That way it 
> would handle all the cases.
[JZ] Sure
>
> Regards,
>   Felix
>
>
>>       }
>>         /* Check condition once. */