Received-SPF: pass (google.com: domain of linux-kernel+bounces-72851-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
From: Anna-Maria Behnsen <anna-maria@linutronix.de>
To: Frederic Weisbecker <frederic@kernel.org>
Cc: linux-kernel@vger.kernel.org, Peter Zijlstra <peterz@infradead.org>,
 John Stultz <jstultz@google.com>, Thomas Gleixner <tglx@linutronix.de>,
 Eric Dumazet <edumazet@google.com>, "Rafael J . Wysocki"
 <rafael.j.wysocki@intel.com>, Arjan van de Ven <arjan@infradead.org>,
 "Paul E . McKenney" <paulmck@kernel.org>, Rik van Riel <riel@surriel.com>,
 Steven Rostedt <rostedt@goodmis.org>, Sebastian Siewior
 <bigeasy@linutronix.de>, Giovanni Gherdovich <ggherdovich@suse.cz>, Lukasz
 Luba <lukasz.luba@arm.com>, "Gautham R . Shenoy" <gautham.shenoy@amd.com>,
 Srinivas Pandruvada <srinivas.pandruvada@intel.com>, K Prateek Nayak
 <kprateek.nayak@amd.com>
Subject: Re: [PATCH v10a] timers: Move marking timer bases idle into
 tick_nohz_stop_tick()
In-Reply-To: <ZdPYEzno3KqIPo4S@localhost.localdomain>
References: <20240115143743.27827-4-anna-maria@linutronix.de>
 <20240219085236.10624-1-anna-maria@linutronix.de>
 <ZdPYEzno3KqIPo4S@localhost.localdomain>
Date: Tue, 20 Feb 2024 11:48:19 +0100
Message-ID: <878r3f5s3w.fsf@somnus>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable

Frederic Weisbecker <frederic@kernel.org> writes:

> Le Mon, Feb 19, 2024 at 09:52:36AM +0100, Anna-Maria Behnsen a =C3=A9crit=
 :
>> 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.
>>=20
>> 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 t=
imer
>> interrupt. All other work is postponed to timer_base_try_to_set_idle() w=
hich is
>> called by tick_nohz_stop_tick(). timer_base_try_to_set_idle() never rese=
ts
>> timer_base::is_idle state. This is done when the tick is restarted via
>> tick_nohz_restart_sched_tick().
>>=20
>> 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 no=
t 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.
>>=20
>> While at it fix some nearby whitespace damage as well.
>>=20
>> 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: anothe=
r 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 =3D 0;
=20
-	/*
-	 * 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 =3D=3D tick_do_timer_cpu) {
-		tick_do_timer_cpu =3D TICK_DO_TIMER_NONE;
-		ts->do_timer_last =3D 1;
-	} else if (tick_do_timer_cpu !=3D TICK_DO_TIMER_NONE) {
-		ts->do_timer_last =3D 0;
-	}
-
 	/* Skip reprogram of event if it's not changed */
 	if (ts->tick_stopped && (expires =3D=3D 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);
 	}
=20
+	/*
+	 * 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 =3D=3D tick_do_timer_cpu) {
+		tick_do_timer_cpu =3D TICK_DO_TIMER_NONE;
+		ts->do_timer_last =3D 1;
+	} else if (tick_do_timer_cpu !=3D TICK_DO_TIMER_NONE) {
+		ts->do_timer_last =3D 0;
+	}
+
 	ts->next_tick =3D expires;
=20
 	/*