Received-SPF: pass (google.com: domain of linux-kernel+bounces-32958-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
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 v10 03/20] timers: Move marking timer bases idle into
 tick_nohz_stop_tick()
In-Reply-To: <Zaf6E61X_l1Bx6Mu@localhost.localdomain>
References: <20240115143743.27827-1-anna-maria@linutronix.de>
 <20240115143743.27827-4-anna-maria@linutronix.de>
 <Zaf6E61X_l1Bx6Mu@localhost.localdomain>
Date: Mon, 22 Jan 2024 12:45:03 +0100
Message-ID: <87ttn5y4lc.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, Jan 15, 2024 at 03:37:26PM +0100, Anna-Maria Behnsen a =C3=A9crit=
 :
>> @@ -889,12 +884,41 @@ static ktime_t tick_nohz_next_event(struct tick_sc=
hed *ts, int cpu)
>>  static void tick_nohz_stop_tick(struct tick_sched *ts, int cpu)
>>  {
>>  	struct clock_event_device *dev =3D __this_cpu_read(tick_cpu_device.evt=
dev);
>> +	unsigned long basejiff =3D ts->last_jiffies;
>>  	u64 basemono =3D ts->timer_expires_base;
>> -	u64 expires =3D ts->timer_expires;
>> +	bool timer_idle;
>> +	u64 expires;
>>=20=20
>>  	/* Make sure we won't be trying to stop it twice in a row. */
>>  	ts->timer_expires_base =3D 0;
>>=20=20
>> +	/*
>> +	 * Now the tick should be stopped definitely - so the timer base needs
>> +	 * to be marked idle as well to not miss a newly queued timer.
>> +	 */
>> +	expires =3D timer_base_try_to_set_idle(basejiff, basemono, &timer_idle=
);
>> +	if (!timer_idle) {
>> +		/*
>> +		 * Do not clear tick_stopped here when it was already set - it
>
> Can that really happen? Looking at __get_next_timer_interrupt(), you're m=
aking a
> behavioural change: if base->is_idle was previously set and the next time=
r is
> now below/equal a jiffy, base->is_idle is not going to be cleared by
> __get_next_timer_interrupt().
>
> Therefore you shouldn't observe ts->tick_stopped && !timer_idle
>
> But I'm assuming that behavioural change wasn't intended?

It was intended to keep tick_stopped and base->is_idle in sync. So when
tick_stopped is set also base->is_idle needs to be set and dropping it
before tick_stopped is dropped will break the plan to keep it in sync.

>> +		 * will be retained on the next idle iteration when the tick
>> +		 * expired earlier than expected.
>
> I'm a bit confused by this sentence.

Me too :) It is there because of a previous version and I didn't cleaned
it up properly.

>> +		 */
>> +		expires =3D basemono + TICK_NSEC;
>
> Do you need this line?

No. After revisiting it once more, it is not required, as it should be
set properly by the return value of timer_base_try_to_set_idle(). So I
should be able to completely drop this first part of the if statement.

>
>> @@ -1147,11 +1175,6 @@ void tick_nohz_idle_stop_tick(void)
>>  void tick_nohz_idle_retain_tick(void)
>>  {
>>  	tick_nohz_retain_tick(this_cpu_ptr(&tick_cpu_sched));
>
> Looks like the content of tick_nohz_retain_tick() can move here now.

I can do this.

>> -	/*
>> -	 * Undo the effect of get_next_timer_interrupt() called from
>> -	 * tick_nohz_next_event().
>> -	 */
>> -	timer_clear_idle();
>>  }
>
> Thanks.

Thanks,

	Anna-Maria