Received-SPF: pass (google.com: domain of linux-kernel+bounces-72973-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
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: <ZdSQBD_ZpWvH5SoZ@localhost.localdomain>
References: <20240115143743.27827-4-anna-maria@linutronix.de>
 <20240219085236.10624-1-anna-maria@linutronix.de>
 <ZdPYEzno3KqIPo4S@localhost.localdomain> <878r3f5s3w.fsf@somnus>
 <ZdSQBD_ZpWvH5SoZ@localhost.localdomain>
Date: Tue, 20 Feb 2024 13:02:18 +0100
Message-ID: <87zfvv4a45.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 Tue, Feb 20, 2024 at 11:48:19AM +0100, Anna-Maria Behnsen a =C3=A9crit=
 :
>> Frederic Weisbecker <frederic@kernel.org> writes:
>> 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=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 */
>
> That should work but then you lose the optimization that resets
> ts->do_timer_last even if the next timer hasn't changed.
>

Beside of this optimization thing, I see onther problem. But I'm not
sure, if I understood it correctly: When the CPU drops the
tick_do_timer_cpu assignment and stops the tick, it is possible, that
this CPU nevertheless executes tick_sched_do_timer() and then reassigns
to tick_do_timer_cpu?

Then it is mandatory that we have this drop the assignment also in the
path when the tick is already stopped. Otherwise the problem described
in the comment could happen with stale jiffies, no?

Thanks

> Thanks.
>
>
>
>> @@ -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=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=20
>>  	/*