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

[Patrice CHOTARD]

On 7/6/26 11:04, Rasmus Villemoes wrote:
> On Fri, Jul 03 2026, "Patrice Chotard" <patrice.chotard at foss.st.com> wrote:
> 
>> On STM32MP157C-DK2, when using the "ums" command, in sleep_thread(),
>> ctrlc() is called every ~640ms which doesn't allows high reactivity when
>> user press CTRL+C in U-Boot console.
>>
>> In sleep_thread() loop, ctrlc() is called every 200000 iterations.
>> But schedule is called on each loop iteration.
>>
>> Optimize cyclic_run() in order to parse the cyclic list only is the
>> next cyclic to run is due.
>> During cyclic list parsing, save the nearest cyclic timestamp, which
>> allows to exit from next cyclic_run() call if no cyclic will be executed.
>>
>> This allow to save computation time :
>>   _ before : ctrlc() is called every ~640ms
>>   _ after  : ctrlc() is called every ~230ms
>>
>> Signed-off-by: Patrice Chotard <patrice.chotard at foss.st.com>
>> Cc: Marek Vasut <marek.vasut at mailbox.org>
>> ---
>>  common/cyclic.c                   | 16 ++++++++++++++++
>>  include/asm-generic/global_data.h |  4 ++++
>>  2 files changed, 20 insertions(+)
>>
>> diff --git a/common/cyclic.c b/common/cyclic.c
>> index b37cd6d8ff0..10ea0711ba1 100644
>> --- a/common/cyclic.c
>> +++ b/common/cyclic.c
>> @@ -68,11 +68,17 @@ static void cyclic_run(void)
>>  	struct cyclic_info *cyclic;
>>  	struct hlist_node *tmp;
>>  	u64 now, cpu_time;
>> +	u64 next_call = UINT64_MAX;
>>
>>  	/* Prevent recursion */
>>  	if (gd->flags & GD_FLG_CYCLIC_RUNNING)
>>  		return;
>>
>> +	/* 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,17 @@ static void cyclic_run(void)
>>  				cyclic->already_warned = true;
>>  			}
>>  		}
>> +		if (next_call > cyclic->next_call) {
>> +			next_call = cyclic->next_call;
>> +			/* move this cyclic to list's head */
>> +			hlist_del(&cyclic->list);
>> +			hlist_add_head(&cyclic->list, cyclic_get_list());
>> +		}
>>  	}
>> +
>> +	if (next_call != UINT64_MAX)
>> +		gd->next_cyclic_call = next_call;
> 
> Are you sure this is correct? There are at least two things I can think
> of that might be wrong:
> 
> (1) Doing > or < comparison (as done in the next_call > cyclic_next_call
> line), that should never be done, because the logic around timestamps is
> that we do allow wrapping, assuming the two timestamps being compared
> never represent absolute times that differ by more than what can be
> represented in the signed version of the timestamp type - that's why the
> time_after*/time_before* family do a subtraction and a signed compare
> to 0.

Ah yes, agree

> 
> (2) What happens if we have a single cyclic consumer registered, with a
> rather long period (say 1s), hence a long time till its next_call, then another
> cyclic consumer is registered which should be called every 50ms. Since I
> don't see anything added to cyclic_register(), it seems that the new
> consumer would be ignored for almost a second?

You are right, i forgot this case

> 
> I'm also not really sure what the "move the first expiring timer to the
> head" logic is supposed to achieve. Either way, if any timer has
> expired, we'll run through the whole list, so while the first expiring
> timer would then be handled first on the next cyclic_run(), that doesn't
> mean we'll save any runtime. And this exercise seems to be about doing as
> little as possible in cyclic_run(), so can you explain what the pointer
> juggling does?

As suggested by Marek in v1, by putting the cyclic's callback with the nearest timestamp
in head of list, we don't need to parse all the cyclic_list entries to find the next cyclic to run.

> 
> I assume that what makes cyclic_run() somewhat expensive isn't actually
> running through the cyclic_list(), but the get_timer_us() calls done for
> each entry. Could you try to see what something like this (entirely
> untested, but you get the idea) would do:
> 
> diff --git a/common/cyclic.c b/common/cyclic.c
> index ec952a01ee1..2616c4d5bc6 100644
> --- a/common/cyclic.c
> +++ b/common/cyclic.c
> @@ -67,26 +67,28 @@ static void cyclic_run(void)
>  {
>         struct cyclic_info *cyclic;
>         struct hlist_node *tmp;
> -       uint64_t now, cpu_time;
> +       uint64_t now, after, cpu_time;
>  
>         /* Prevent recursion */
>         if (gd->flags & GD_FLG_CYCLIC_RUNNING)
>                 return;
>  
>         gd->flags |= GD_FLG_CYCLIC_RUNNING;
> +       now = get_timer_us(0);
>         hlist_for_each_entry_safe(cyclic, tmp, cyclic_get_list(), list) {
>                 /*
>                  * Check if this cyclic function needs to get called, e.g.
>                  * do not call the cyclic func too often
>                  */
> -               now = get_timer_us(0);
>                 if (time_after_eq64(now, cyclic->next_call)) {
>                         /* Call cyclic function and account it's cpu-time */
>                         cyclic->next_call = now + cyclic->delay_us;
>                         cyclic->func(cyclic);
> +                       after = get_timer_us(0);
>                         cyclic->run_cnt++;
> -                       cpu_time = get_timer_us(0) - now;
> +                       cpu_time = after - now;
>                         cyclic->cpu_time_us += cpu_time;
> +                       now = after;
>  
>                         /* Check if cpu-time exceeds max allowed time */
>                         if ((cpu_time > CONFIG_CYCLIC_MAX_CPU_TIME_US) &&
> 
> Because the time_after_eq64() test really doesn't take anywhere near a
> micro-second, nor does fetching the next element from the linked list,
> so using the same value for "now" until we actually do hit an expired
> timer should be ok.

I tested your patch proposal, and yes, what cost the most is get_timer_us() calls.
I obtain the same gain using your patch proposal than with my buggy implementation ;-)
which doesn't, moreover, cover all cases.

I will submit a v3 with you proposal.

Thanks
Patrice> 
> [I also want to move the computation of the new cyclic->next_call to
> after the callback, so that the callback can update its ->delay_us value
> if it needs to be called at another frequency, but that's a separate
> topic.]
> 
> Rasmus