linux-kernel - Re: [PATCH v10 03/20] timers: Move marking timer bases idle into tick_nohz_stop

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Mon, 22 Jan 2024 12:45:03 +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 v10 03/20] timers: Move marking timer bases idle into
 tick_nohz_stop_tick()

Frederic Weisbecker <frederic@...nel.org> writes:

> Le Mon, Jan 15, 2024 at 03:37:26PM +0100, Anna-Maria Behnsen a écrit :
>> @@ -889,12 +884,41 @@ static ktime_t tick_nohz_next_event(struct tick_sched *ts, int cpu)
>>  static void tick_nohz_stop_tick(struct tick_sched *ts, int cpu)
>>  {
>>  	struct clock_event_device *dev = __this_cpu_read(tick_cpu_device.evtdev);
>> +	unsigned long basejiff = ts->last_jiffies;
>>  	u64 basemono = ts->timer_expires_base;
>> -	u64 expires = ts->timer_expires;
>> +	bool timer_idle;
>> +	u64 expires;
>>  
>>  	/* Make sure we won't be trying to stop it twice in a row. */
>>  	ts->timer_expires_base = 0;
>>  
>> +	/*
>> +	 * 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 = 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 making a
> behavioural change: if base->is_idle was previously set and the next timer 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 = 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