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

Anna-Maria Behnsen
2024-01-22T11:45:06Z

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