Re: [PATCH v10a] timers: Move marking timer bases idle into tick_nohz_stop_tick()

[Anna-Maria Behnsen]

Frederic Weisbecker <frederic@xxxxxxxxxx> writes:

> Le Tue, Feb 20, 2024 at 01:02:18PM +0100, Anna-Maria Behnsen a écrit :
>> Frederic Weisbecker <frederic@xxxxxxxxxx> writes:
>> 
>> > Le Tue, Feb 20, 2024 at 11:48:19AM +0100, Anna-Maria Behnsen a écrit :
>> >> Frederic Weisbecker <frederic@xxxxxxxxxx> writes:
>> >> diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
>> >> index 01fb50c1b17e..b93f0e6f273f 100644
>> >> --- a/kernel/time/tick-sched.c
>> >> +++ b/kernel/time/tick-sched.c
>> >> @@ -895,21 +895,6 @@ static void tick_nohz_stop_tick(struct tick_sched *ts, int cpu)
>> >>  	/* Make sure we won't be trying to stop it twice in a row. */
>> >>  	ts->timer_expires_base = 0;
>> >>  
>> >> -	/*
>> >> -	 * If this CPU is the one which updates jiffies, then give up
>> >> -	 * the assignment and let it be taken by the CPU which runs
>> >> -	 * the tick timer next, which might be this CPU as well. If we
>> >> -	 * don't drop this here, the jiffies might be stale and
>> >> -	 * do_timer() never gets invoked. Keep track of the fact that it
>> >> -	 * was the one which had the do_timer() duty last.
>> >> -	 */
>> >> -	if (cpu == tick_do_timer_cpu) {
>> >> -		tick_do_timer_cpu = TICK_DO_TIMER_NONE;
>> >> -		ts->do_timer_last = 1;
>> >> -	} else if (tick_do_timer_cpu != TICK_DO_TIMER_NONE) {
>> >> -		ts->do_timer_last = 0;
>> >> -	}
>> >> -
>> >>  	/* Skip reprogram of event if it's not changed */
>> >>  	if (ts->tick_stopped && (expires == ts->next_tick)) {
>> >>  		/* Sanity check: make sure clockevent is actually programmed */
>> >
>> > That should work but then you lose the optimization that resets
>> > ts->do_timer_last even if the next timer hasn't changed.
>> >
>> 
>> Beside of this optimization thing, I see onther problem. But I'm not
>> sure, if I understood it correctly: When the CPU drops the
>> tick_do_timer_cpu assignment and stops the tick, it is possible, that
>> this CPU nevertheless executes tick_sched_do_timer() and then reassigns
>> to tick_do_timer_cpu?
>
> Yes but in this case a timer interrupt has executed and ts->next_tick
> is cleared, so the above skip reprogramm branch is not taken.
>

Yes... So I need to change it without dropping the
optimization. Otherwise someone might complain about it.

Two possible solutions:

a) split out this if/else thing for dropping the tick_do_timer_cpu
   assignment into a separate function and call it:
   - before the return in the skip reprogramm branch
   - and after the if clause which contains stopping the tick (where it
     is executed in the current proposal)

b) Take my current proposal and add before the return in the skip
   reprogramm branch the following lines:

   if (tick_do_timer_cpu != TICK_DO_TIMER_NONE)
   	ts->do_timer_last = 0;

   as the first part of the tick_do_timer_cpu/last logic shouldn't be
   required (because then also ts->next_tick is already cleared).

What do you prefere? Or do you prefere something else?

Thanks

> Thanks.
>
>> 
>> Then it is mandatory that we have this drop the assignment also in the
>> path when the tick is already stopped. Otherwise the problem described
>> in the comment could happen with stale jiffies, no?
>> 
>> Thanks
>> 
>> > Thanks.
>> >
>> >
>> >
>> >> @@ -938,6 +923,21 @@ static void tick_nohz_stop_tick(struct tick_sched *ts, int cpu)
>> >>  		trace_tick_stop(1, TICK_DEP_MASK_NONE);
>> >>  	}
>> >>  
>> >> +	/*
>> >> +	 * If this CPU is the one which updates jiffies, then give up
>> >> +	 * the assignment and let it be taken by the CPU which runs
>> >> +	 * the tick timer next, which might be this CPU as well. If we
>> >> +	 * don't drop this here, the jiffies might be stale and
>> >> +	 * do_timer() never gets invoked. Keep track of the fact that it
>> >> +	 * was the one which had the do_timer() duty last.
>> >> +	 */
>> >> +	if (cpu == tick_do_timer_cpu) {
>> >> +		tick_do_timer_cpu = TICK_DO_TIMER_NONE;
>> >> +		ts->do_timer_last = 1;
>> >> +	} else if (tick_do_timer_cpu != TICK_DO_TIMER_NONE) {
>> >> +		ts->do_timer_last = 0;
>> >> +	}
>> >> +
>> >>  	ts->next_tick = expires;
>> >>  
>> >>  	/*