Re: [PATCH v9 30/32] timers: Implement the hierarchical pull model

Anna-Maria Behnsen
2023-12-08T09:02:06Z

Sebastian Siewior <bigeasy@linutronix.de> writes:

> On 2023-12-01 10:26:52 [+0100], Anna-Maria Behnsen wrote:

[...]

>> diff --git a/kernel/time/timer.c b/kernel/time/timer.c
>> index b6c9ac0c3712..ac3e888d053f 100644
>> --- a/kernel/time/timer.c
>> +++ b/kernel/time/timer.c
>> @@ -2103,6 +2104,64 @@ void timer_lock_remote_bases(unsigned int cpu)
> …
>> +static void timer_use_tmigr(unsigned long basej, u64 basem,
>> + unsigned long *nextevt, bool *tick_stop_path,
>> + bool timer_base_idle, struct timer_events *tevt)
>> +{
>> + u64 next_tmigr;
>> +
>> + if (timer_base_idle)
>> + next_tmigr = tmigr_cpu_new_timer(tevt->global);
>> + else if (tick_stop_path)
>> + next_tmigr = tmigr_cpu_deactivate(tevt->global);
>> + else
>> + next_tmigr = tmigr_quick_check();
>> +
>> + /*
>> + * If the CPU is the last going idle in timer migration hierarchy, make
>> + * sure the CPU will wake up in time to handle remote timers.
>> + * next_tmigr == KTIME_MAX if other CPUs are still active.
>> + */
>> + if (next_tmigr < tevt->local) {
>> + u64 tmp;
>> +
>> + /* If we missed a tick already, force 0 delta */
>> + if (next_tmigr < basem)
>> + next_tmigr = basem;
>> +
>> + tmp = div_u64(next_tmigr - basem, TICK_NSEC);
>
> Is this considered a hot path? Asking because u64 divs are nice if can
> be avoided ;)

It's the 'try to go idle path' - so no hot path. Please correct me if
I'm wrong.

> I guess the original value is from fetch_next_timer_interrupt(). But
> then you only need it if the caller (__get_next_timer_interrupt()) has
> the `idle' value set. Otherwise the operation is pointless.
> Would it somehow work to replace
> base_local->is_idle = time_after(nextevt, basej + 1);
>
> with maybe something like
> base_local->is_idle = tevt.local > basem + TICK_NSEC
>
> If so you could avoid the `nextevt' maneuver.
>

This change could be done indepentant as an improvement on top of the
queue as well. I will not improve it right now, if it's ok.

>> + *nextevt = basej + (unsigned long)tmp;
>> + tevt->local = next_tmigr;
>> + }
>> +}
>> +# else

Thanks for the other input - I already changed it for v10 of the queue.

Thanks,

Anna-Maria