Re: [PATCH v10 03/20] timers: Move marking timer bases idle into tick_nohz_stop_tick()

[Anna-Maria Behnsen]

Frederic Weisbecker <frederic@xxxxxxxxxx> 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