Re: [RFC 1/3] tick-sched: Remove last_tick and calculate next tick from now

[Thomas Gleixner <tglx@linutronix.de>]

On Tue, Nov 12 2024 at 00:43, Frederic Weisbecker wrote:
> Le Fri, Nov 08, 2024 at 05:48:34PM +0000, Joel Fernandes (Google) a écrit :

>> During tick restart, we use last_tick and forward it past now.
>>
>> Since we are forwarding past now, we can simply use now as a reference
>> instead of last_tick. This patch removes last_tick and does so.
>>
>> This patch potentially does more mul/imul than the existing code,
>> as sometimes forwarding past now need not be done if last_tick > now.
>> However, the patch is a cleanup which reduces LOC and reduces the size
>> of struct tick_sched.

May I politely ask you to read and follow the Documentation
vs. changelogs?

  https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#changelog

Also

git grep 'This patch' Documentation/process

might give you a hint.

>> -	/* Forward the time to expire in the future */
>> -	hrtimer_forward(&ts->sched_timer, now, TICK_NSEC);
>> +	hrtimer_set_expires(&ts->sched_timer, DIV_ROUND_UP_ULL(now, TICK_NSEC) * TICK_NSEC);

How is a division and multiplication in this hotpath helpful? That's
awfully slow on 32-bit machines and pointless on 64-bit too.

Using now is also wrong as it breaks the sched_skew_tick distribution by
aligning the tick on all CPUs again.

IOW, this "cleanup" is making things worse.

> We don't want to rewrite hrtimer_forward() but, after all, the current expiry is
> enough a relevant information.
>
> How about just this? It's worth it as it now forwards after the real last programmed
> tick, which should be close enough from @now with a delta below TICK_NSEC, or even
> better @now is below the expiry. Therefore it should resume as just a no-op
> or at worst an addition within hrtimer_forward():
>
> diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
> index 753a184c7090..ffd0c026a248 100644
> --- a/kernel/time/tick-sched.c
> +++ b/kernel/time/tick-sched.c
> @@ -838,7 +838,6 @@ EXPORT_SYMBOL_GPL(get_cpu_iowait_time_us);
>  static void tick_nohz_restart(struct tick_sched *ts, ktime_t now)
>  {
>  	hrtimer_cancel(&ts->sched_timer);
> -	hrtimer_set_expires(&ts->sched_timer, ts->last_tick);
>  
>  	/* Forward the time to expire in the future */
>  	hrtimer_forward(&ts->sched_timer, now, TICK_NSEC);

That's just wrong. ts->sched_timer.expires contains a tick in the
future. If tick_nohz_stop_tick() set it to 10 ticks in the future and
the CPU goes out of idle due to a device interrupt before the timer
expires, then hrtimer_forward() will do nothing because expires is ahead
of now.

Which means the CPU is not idle and has no tick until the delayed tick
which was set by tick_nohz_stop_tick() expires. Not really correct.

Thanks,

        tglx