[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <878r3f5s3w.fsf@somnus>
Date: Tue, 20 Feb 2024 11:48:19 +0100
From: Anna-Maria Behnsen <anna-maria@...utronix.de>
To: Frederic Weisbecker <frederic@...nel.org>
Cc: linux-kernel@...r.kernel.org, Peter Zijlstra <peterz@...radead.org>,
John Stultz <jstultz@...gle.com>, Thomas Gleixner <tglx@...utronix.de>,
Eric Dumazet <edumazet@...gle.com>, "Rafael J . Wysocki"
<rafael.j.wysocki@...el.com>, Arjan van de Ven <arjan@...radead.org>,
"Paul E . McKenney" <paulmck@...nel.org>, Rik van Riel <riel@...riel.com>,
Steven Rostedt <rostedt@...dmis.org>, Sebastian Siewior
<bigeasy@...utronix.de>, Giovanni Gherdovich <ggherdovich@...e.cz>, Lukasz
Luba <lukasz.luba@....com>, "Gautham R . Shenoy" <gautham.shenoy@....com>,
Srinivas Pandruvada <srinivas.pandruvada@...el.com>, K Prateek Nayak
<kprateek.nayak@....com>
Subject: Re: [PATCH v10a] timers: Move marking timer bases idle into
tick_nohz_stop_tick()
Frederic Weisbecker <frederic@...nel.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@...utronix.de>
>
> Reviewed-by: Frederic Weisbecker <frederic@...nel.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;
/*
Powered by blists - more mailing lists