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

[Anna-Maria Behnsen]

Sebastian Siewior <bigeasy@xxxxxxxxxxxxx> 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