Re: [PATCH v10a] timers: Move marking timer bases idle into tick_nohz_stop_tick()

Anna-Maria Behnsen
2024-02-20T10:48:22Z

Frederic Weisbecker <frederic@kernel.org> writes:

> Le Mon, Feb 19, 2024 at 09:52:36AM +0100, Anna-Maria Behnsen a écrit :
>> The timer base is marked idle when get_next_timer_interrupt() is
>> executed. But the decision whether the tick will be stopped and whether the
>> system is able to go idle is done later. When the timer bases is marked
>> idle and a new first timer is enqueued remote an IPI is raised. Even if it
>> is not required because the tick is not stopped and the timer base is
>> evaluated again at the next tick.
>>
>> To prevent this, the timer base is marked idle in tick_nohz_stop_tick() and
>> get_next_timer_interrupt() is streamlined by only looking for the next timer
>> interrupt. All other work is postponed to timer_base_try_to_set_idle() which is
>> called by tick_nohz_stop_tick(). timer_base_try_to_set_idle() never resets
>> timer_base::is_idle state. This is done when the tick is restarted via
>> tick_nohz_restart_sched_tick().
>>
>> With this, tick_sched::tick_stopped and timer_base::is_idle are always in
>> sync. So there is no longer the need to execute timer_clear_idle() in
>> tick_nohz_idle_retain_tick(). This was required before, as
>> tick_nohz_next_event() set timer_base::is_idle even if the tick would not be
>> stopped. So timer_clear_idle() is only executed, when timer base is idle. So the
>> check whether timer base is idle, is now no longer required as well.
>>
>> While at it fix some nearby whitespace damage as well.
>>
>> Signed-off-by: Anna-Maria Behnsen <anna-maria@linutronix.de>
>
> Reviewed-by: Frederic Weisbecker <frederic@kernel.org>
>
> Just a small detail below that can be fixed in a further patch:
>
>> @@ -930,6 +947,10 @@ static void tick_nohz_stop_tick(struct tick_sched *ts, int cpu)
>> * scheduler tick in tick_nohz_restart_sched_tick().
>> */
>> if (!ts->tick_stopped) {
>> + /* If the timer base is not idle, retain the tick. */
>> + if (!timer_idle)
>> + return;
>
> This happens after tick_do_timer_cpu has been set to TICK_DO_TIMER_NONE. Ideally
> it would be better to do it before. Not that it hurts in practice: another CPU
> or this one will take the duty. But it looks weird to stop halfway.
>

Yes, you are right. I would prefere, to clean it up directly and add
another patch before this patch which simply moves the
TICK_DO_TIMER_NONE related block after the !ts->tick_stopped
block. Because a changed order shouldn't be a problem at the moment as
well, or am I wrong?

Thanks,

Anna-Maria

---8<----
diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
index 01fb50c1b17e..b93f0e6f273f 100644
--- a/kernel/time/tick-sched.c
+++ b/kernel/time/tick-sched.c
@@ -895,21 +895,6 @@ static void tick_nohz_stop_tick(struct tick_sched *ts, int cpu)
/* Make sure we won't be trying to stop it twice in a row. */
ts->timer_expires_base = 0;

- /*
- * If this CPU is the one which updates jiffies, then give up
- * the assignment and let it be taken by the CPU which runs
- * the tick timer next, which might be this CPU as well. If we
- * don't drop this here, the jiffies might be stale and
- * do_timer() never gets invoked. Keep track of the fact that it
- * was the one which had the do_timer() duty last.
- */
- if (cpu == tick_do_timer_cpu) {
- tick_do_timer_cpu = TICK_DO_TIMER_NONE;
- ts->do_timer_last = 1;
- } else if (tick_do_timer_cpu != TICK_DO_TIMER_NONE) {
- ts->do_timer_last = 0;
- }
-
/* Skip reprogram of event if it's not changed */
if (ts->tick_stopped && (expires == ts->next_tick)) {
/* Sanity check: make sure clockevent is actually programmed */
@@ -938,6 +923,21 @@ static void tick_nohz_stop_tick(struct tick_sched *ts, int cpu)
trace_tick_stop(1, TICK_DEP_MASK_NONE);
}

+ /*
+ * If this CPU is the one which updates jiffies, then give up
+ * the assignment and let it be taken by the CPU which runs
+ * the tick timer next, which might be this CPU as well. If we
+ * don't drop this here, the jiffies might be stale and
+ * do_timer() never gets invoked. Keep track of the fact that it
+ * was the one which had the do_timer() duty last.
+ */
+ if (cpu == tick_do_timer_cpu) {
+ tick_do_timer_cpu = TICK_DO_TIMER_NONE;
+ ts->do_timer_last = 1;
+ } else if (tick_do_timer_cpu != TICK_DO_TIMER_NONE) {
+ ts->do_timer_last = 0;
+ }
+
ts->next_tick = expires;

/*