[v8,10/25] timers: Move marking timer bases idle into tick_nohz_stop_tick()

  The timer base is marked idle when get_next_timer_interrupt() is
executed. But the decision whether the tick will be stopped and whether the
system is able to go idle is done later. When the timer bases is marked
idle and a new first timer is enqueued remote an IPI is raised. Even if it
is not required because the tick is not stopped and the timer base is
evaluated again at the next tick.

To prevent this, the timer base is marked idle in tick_nohz_stop_tick() and
get_next_timer_interrupt() is streamlined by only looking for the next
timer interrupt. All other work is postponed to timer_set_idle() which is
called by tick_nohz_stop_tick().

While at it a whitespace damage is fixed as well.

Signed-off-by: Anna-Maria Behnsen <anna-maria@linutronix.de>
---
 kernel/time/tick-internal.h |  1 +
 kernel/time/tick-sched.c    | 38 ++++++++++++++++++++++++-----------
 kernel/time/timer.c         | 40 +++++++++++++++++++++++++++++++++----
 3 files changed, 63 insertions(+), 16 deletions(-)
  

Le Wed, Oct 04, 2023 at 02:34:39PM +0200, Anna-Maria Behnsen a écrit :
>  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 = ts->tick_stopped;
> +	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 timer base needs to be
> +	 * marked idle as well to not miss a newly queued timer.
> +	 */
> +	expires = timer_set_idle(basejiff, basemono, &timer_idle);
> +	if (!timer_idle) {
> +		/*
> +		 * Do not clear tick_stopped here when it was already set - it will
> +		 * be retained on next idle iteration when tick expired earlier
> +		 * than expected.
> +		 */
> +		expires = basemono + TICK_NSEC;
> +
> +		/* Undo the effect of timer_set_idle() */
> +		timer_clear_idle();

Looks like you don't even need to clear ->is_idle on failure. timer_set_idle()
does it for you.

> +	} else if (expires < ts->timer_expires) {
> +		ts->timer_expires = expires;
> +	} else {
> +		expires = ts->timer_expires;

Is it because timer_set_idle() doesn't recalculate the next hrtimer (as opposed
to get_next_timer_interrupt())? And since tick_nohz_next_event() did, the fact
that ts->timer_expires has a lower value may mean there is an hrtimer to take
into account and so you rather use the old calculation?

If so please add a comment explaining that because it's not that obvious. It's
worth noting also the side effect that the nearest timer may have been cancelled
in-between and we might reprogram too-early but the event should be rare enough
that we don't care.

Another reason also is that cpuidle may have programmed a shallow C-state
because it saw an early next expiration estimation. And if the related timer is
cancelled in-between and we didn't keep the old expiration estimation, we would
otherwise stop the tick for a long time with a shallow C-state.

> @@ -926,7 +944,7 @@ static void tick_nohz_stop_tick(struct tick_sched *ts, int cpu)
>  	 * first call we save the current tick time, so we can restart
>  	 * the scheduler tick in nohz_restart_sched_tick.
>  	 */
> -	if (!ts->tick_stopped) {
> +	if (!ts->tick_stopped && timer_idle) {

In fact, if (!ts->tick_stopped && !timer_idle) then you
should return now and avoid the reprogramming.

> @@ -1950,6 +1950,40 @@ u64 get_next_timer_interrupt(unsigned long basej, u64 basem)
>  	if (cpu_is_offline(smp_processor_id()))
>  		return expires;
>  
> +	raw_spin_lock(&base->lock);
> +	nextevt = __get_next_timer_interrupt(basej, base);
> +	raw_spin_unlock(&base->lock);

It's unfortunate we have to lock here, which means we lock twice
on the idle path. But I can't think of a better way and I guess
the follow-up patches rely on that.

Thanks.
  

Frederic Weisbecker <frederic@kernel.org> writes:

> Le Wed, Oct 04, 2023 at 02:34:39PM +0200, Anna-Maria Behnsen a écrit :
>>  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 = ts->tick_stopped;
>> +	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 timer base needs to be
>> +	 * marked idle as well to not miss a newly queued timer.
>> +	 */
>> +	expires = timer_set_idle(basejiff, basemono, &timer_idle);
>> +	if (!timer_idle) {
>> +		/*
>> +		 * Do not clear tick_stopped here when it was already set - it will
>> +		 * be retained on next idle iteration when tick expired earlier
>> +		 * than expected.
>> +		 */
>> +		expires = basemono + TICK_NSEC;
>> +
>> +		/* Undo the effect of timer_set_idle() */
>> +		timer_clear_idle();
>
> Looks like you don't even need to clear ->is_idle on failure. timer_set_idle()
> does it for you.

You are right. I tried several approaches and then forgot to remove it
here.

>> +	} else if (expires < ts->timer_expires) {
>> +		ts->timer_expires = expires;
>> +	} else {
>> +		expires = ts->timer_expires;
>
> Is it because timer_set_idle() doesn't recalculate the next hrtimer (as opposed
> to get_next_timer_interrupt())? And since tick_nohz_next_event() did, the fact
> that ts->timer_expires has a lower value may mean there is an hrtimer to take
> into account and so you rather use the old calculation?

Yes and because power things rely on it.

> If so please add a comment explaining that because it's not that obvious. It's
> worth noting also the side effect that the nearest timer may have been cancelled
> in-between and we might reprogram too-early but the event should be rare enough
> that we don't care.
>
> Another reason also is that cpuidle may have programmed a shallow C-state
> because it saw an early next expiration estimation. And if the related timer is
> cancelled in-between and we didn't keep the old expiration estimation, we would
> otherwise stop the tick for a long time with a shallow C-state.

I'll add a comment covering all your input! Thanks!
The probability that there happens a lot of enqueue and dequeue of
timers between get_next_timer_interrupt() and setting timer base idle is
not very high. But we have to make sure that we do not miss a new first
timer there.

>> @@ -926,7 +944,7 @@ static void tick_nohz_stop_tick(struct tick_sched *ts, int cpu)
>>  	 * first call we save the current tick time, so we can restart
>>  	 * the scheduler tick in nohz_restart_sched_tick.
>>  	 */
>> -	if (!ts->tick_stopped) {
>> +	if (!ts->tick_stopped && timer_idle) {
>
> In fact, if (!ts->tick_stopped && !timer_idle) then you
> should return now and avoid the reprogramming.

You are right. I'll add it and test it.

>> @@ -1950,6 +1950,40 @@ u64 get_next_timer_interrupt(unsigned long basej, u64 basem)
>>  	if (cpu_is_offline(smp_processor_id()))
>>  		return expires;
>>  
>> +	raw_spin_lock(&base->lock);
>> +	nextevt = __get_next_timer_interrupt(basej, base);
>> +	raw_spin_unlock(&base->lock);
>
> It's unfortunate we have to lock here, which means we lock twice
> on the idle path. But I can't think of a better way and I guess
> the follow-up patches rely on that.

We have to do it like this, because power people need the sleep length
information to able to decide whether to stop the tick or not. If we do
not want to have the timer base locked two times in idle path, we will
not be able to move timer base idle marking into
tick_nohz_stop_tick(). But the good thing is, that we do not mark timer
bases idle, when tick is not stopped with this approach.

btw, I try to rewrite this patch completely as tglx was not happy about
some parts of code duplication. I'll make sure that your remarks are
also covered.

Thanks,

	Anna-Maria