Received-SPF: pass (google.com: domain of linux-kernel+bounces-32964-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 v10 04/20] timers: Optimization for
 timer_base_try_to_set_idle()
In-Reply-To: <ZagEK9l06c_7F3pH@localhost.localdomain>
References: <20240115143743.27827-1-anna-maria@linutronix.de>
 <20240115143743.27827-5-anna-maria@linutronix.de>
 <ZagEK9l06c_7F3pH@localhost.localdomain>
Date: Mon, 22 Jan 2024 12:48:21 +0100
Message-ID: <87r0i9y4fu.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:27PM +0100, Anna-Maria Behnsen a =C3=A9crit=
 :
>> When tick is stopped also the timer base is_idle flag is set. When
>> reentering the timer_base_try_to_set_idle() with the tick stopped, there=
 is
>> no need to check whether the timer base needs to be set idle again. When=
 a
>> timer was enqueued in the meantime, this is already handled by the
>> tick_nohz_next_event() call which was executed before
>> tick_nohz_stop_tick().
>>=20
>> Signed-off-by: Anna-Maria Behnsen <anna-maria@linutronix.de>
>> ---
>>  kernel/time/tick-sched.c |  2 +-
>>  kernel/time/timer.c      | 11 ++++++++---
>>  2 files changed, 9 insertions(+), 4 deletions(-)
>>=20
>> diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
>> index c6223afc801f..27f1a2ae7f39 100644
>> --- a/kernel/time/tick-sched.c
>> +++ b/kernel/time/tick-sched.c
>> @@ -886,7 +886,7 @@ static void tick_nohz_stop_tick(struct tick_sched *t=
s, 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;
>> -	bool timer_idle;
>> +	bool timer_idle =3D ts->tick_stopped;
>>  	u64 expires;
>>=20=20
>>  	/* Make sure we won't be trying to stop it twice in a row. */
>> diff --git a/kernel/time/timer.c b/kernel/time/timer.c
>> index 3a668060692e..2f69a485a070 100644
>> --- a/kernel/time/timer.c
>> +++ b/kernel/time/timer.c
>> @@ -1999,13 +1999,18 @@ u64 get_next_timer_interrupt(unsigned long basej=
, u64 basem)
>>   * timer_base_try_to_set_idle() - Try to set the idle state of the time=
r bases
>>   * @basej:	base time jiffies
>>   * @basem:	base time clock monotonic
>> - * @idle:	pointer to store the value of timer_base->is_idle
>> + * @idle:	pointer to store the value of timer_base->is_idle on return;
>> + *		*idle contains the information whether tick was already stopped
>>   *
>> - * Returns the tick aligned clock monotonic time of the next pending
>> - * timer or KTIME_MAX if no timer is pending.
>> + * Returns the tick aligned clock monotonic time of the next pending ti=
mer or
>> + * KTIME_MAX if no timer is pending. When tick was already stopped KTIM=
E_MAX is
>> + * returned as well.
>>   */
>>  u64 timer_base_try_to_set_idle(unsigned long basej, u64 basem, bool *id=
le)
>>  {
>> +	if (*idle)
>> +		return KTIME_MAX;
>
> Ok now I see the reason behind the behavioural change.
>
> So either:
>
> * We remove the old behaviour consisting in clearing base->is_idle if the=
 new
>   next timer is within a jiffy while the tick is stopped. But then the ch=
angelog
>   from the previous patch should state that and comments must be clarifie=
d.
>

I would like to take 'either'. I thought the changelog already mentioned
it. But maybe I have to make it more explicit. I'll go and rework the
comments once more.

Thanks,

	Anna-Maria