[PATCH 2/2] cyclic: parse cyclic list only if next cyclic timestamp is reached

[Patrice CHOTARD]

On 7/3/26 16:38, Marek Vasut wrote:
> On 7/3/26 11:55 AM, Patrice CHOTARD wrote:
> 
> Hello Patrice,
> 
>>>>    +    /* check if the next cyclic function's timestamp is reached */
>>>> +    now = get_timer_us(0);
>>>> +    if (time_after_eq64(gd->next_cyclic_call, now))
>>>> +        return;
>>>> +
>>>>        gd->flags |= GD_FLG_CYCLIC_RUNNING;
>>>>        hlist_for_each_entry_safe(cyclic, tmp, cyclic_get_list(), list) {
>>>>            /*
>>>> @@ -102,7 +108,13 @@ static void cyclic_run(void)
>>>>                    cyclic->already_warned = true;
>>>>                }
>>>>            }
>>>> +        if (next_call > cyclic->next_call)
>>>> +            next_call = cyclic->next_call;
>>>
>>> Can you move this entry to the beginning of the cyclic list, so that the entry with the earliest deadline would be right at the beginning of the list ? That should squeeze some more improvement out of this I think.
>>
>> Hi Marek
>>
>> I did a test, it doesn't bring any noticeable improvements, but nevertheless i will submit a v3 with this update.
> 
> Does the list iteration exit when it finds the first entry that is not going to be executed at this time ? If it does not, then there will be no improvement. But for this to work, the list would have to be surely fully ordered in incrementing timestamp order, is it ?

No, the list iteration parse all entries.

I see your point, on original implementation, entries are not sort by incrementing timestamp order.
I did some implementation test, by sorting cyclic entries in cyclic_list when :
  _ registering a new cyclic callback, put it directly in the right position in cyclic_list.
  _ in cyclic_run(), after running a cyclic, compute its next call and insrt it in the right position in cyclic_list.

There is no noticeable gain.

Thanks
Patrice

> 
>>>
>>>>        }
>>>> +
>>>> +    if (next_call != UINT64_MAX)
>>>
>>> Can the next_call literally hit UINT64_MAX in that one unlikely case ?
>>> You likely do need a separate flag, bool next_call_valid, to avoid that edge case.
>>
>> In case, no cyclic has been registered, we don't parse the cyclic list and next_call stays set to UINT64_MAX
>> We don't want that next_cyclic_call be set to UINT64_MAX, otherwise in that case, we will always exit cyclic_run() before parsing cyclic list.
> Consider this:
> 
> +    u64 next_call = UINT64_MAX;
> ...
>      hlist_for_each_entry_safe(cyclic, tmp, cyclic_get_list(), list) {
> ...
>                 cyclic->next_call = now + cyclic->delay_us; // = ~0ULL
> ...
> +        if (next_call > cyclic->next_call)      // False
> +            next_call = cyclic->next_call;  // Not set
> ...
> +    if (next_call != UINT64_MAX) // False, branch not executed, but it should be executed ?
> +        gd->next_cyclic_call = next_call;
> ...