Skip to content

Commit

Permalink
CHROMIUM: tick-sched: Set last_tick correctly so that timer interrupt…
Browse files Browse the repository at this point in the history
…s happen less

TODO: make this only if the tick_nohz_handler() arrived late, not early.
Some broken hardware may make it arrive earlier.

If there are delays in timer interrupts, or other reasons, we can have
tick_nohz_handler() called in quick succession. This seems useless for
low res, and can momentarily make things appear to be high res.

When the tick is active, a delay in a timer interrupt does not mean the
next tick will also be similarly delayed. This seems counter-intuitive
when we are low res. We want the ticks to be spaced out by at least
TICK_NSEC, not less. So fix that.

Also for stop code,  consider the following scenario in low res mode:

1. Assume ts->last_tick is 8.5ms.

2. The CPU exits from idle for some reason, and the tick is restarted.
   During this restart, it is determined in tick_nohz_restart() that the
   next tick should happen at 9.5ms (HZ=1000). This is programmed to the
   clock event (and also recorded into the hrtimer).

3. Just after step 2, the CPU tries to stop the tick while entering idle.
   During this, there is a call to tick_nohz_next_event() which sets
   ts->timer_expires to 10ms due to rounding to TICK_NSEC.

4. Just after this, tick_nohz_stop_tick() is called which sets
   ts->last_tick to 9.5ms (the value recorded in #2 into the hrtimer)
   and the clock event device is set to the 10ms due to ts->timer_expires.

5. Now the timer interrupt goes off at 10ms, tick_nohz_restart() is
   called again, and it programs the clock event to go off at the
   ts->last_tick + TICKNSEC which is 10.5ms.

6. Now the timer interrupt goes off at 10.5ms.

The end result is, we have 2 timer interrupts that went off at a
granularity of less than 1ms which causes timer wheel and hrtimers to
have higher res than they otherwise would.

Fix by setting ts->last_tick in projectceladon#5 to now, which is really when the last
tick happened. This correct makes tick_nohz_restart() consider the most
recent time that the tick timer fired.

I see a similar issue where tick_nohz_handler() could also program the
next timer event too quickly. For this reason, also set the tick-sched
hrtimer in tick_nohz_handler() to now.

With this, I don't see tick_nohz_handler() firing in quick succession.

( CHROMIUM note:
  I am marking it as CHROMIUM for experimentation in Finch and will push
  upstream after seeing results. It is possible we will just disable
  highres timers if the results are looking good in which case there would
  not be a need to push it upstream and we can just disable
  CONFIG_HIGH_RES_TIMERS in the builds. However, we may want to exclude
  <1K HZ devices such as ARM from that, or still keep the dynamic toggle
  option for those -- that's TBD. )

BUG=b:263289152
TEST=cyclictest --smp -p95 -m with timer_highres=0

Change-Id: I3221e458ec6265d9e4a5f68a3e0b7d1c3558be1b
Signed-off-by: Joel Fernandes (Google) <[email protected]>
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/4552437
Reviewed-by: Hsin-Yi Wang <[email protected]>
Commit-Queue: Joel Fernandes <[email protected]>
Tested-by: Joel Fernandes <[email protected]>
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/4572075
Reviewed-by: Joel Fernandes <[email protected]>
Tested-by: Hsin-Yi Wang <[email protected]>
Auto-Submit: Hsin-Yi Wang <[email protected]>
Commit-Queue: Hsin-Yi Wang <[email protected]>
  • Loading branch information
joelagnel authored and Chromeos LUCI committed May 30, 2023
1 parent eec5573 commit aedb9eb
Showing 1 changed file with 3 additions and 0 deletions.
3 changes: 3 additions & 0 deletions kernel/time/tick-sched.c
Original file line number Diff line number Diff line change
Expand Up @@ -1328,10 +1328,13 @@ static void tick_nohz_handler(struct clock_event_device *dev)
tick_sched_do_timer(ts, now);
tick_sched_handle(ts, regs);

ts->last_tick = now;

/* No need to reprogram if we are running tickless */
if (unlikely(ts->tick_stopped))
return;

hrtimer_set_expires(&ts->sched_timer, ts->last_tick);
hrtimer_forward(&ts->sched_timer, now, TICK_NSEC);
tick_program_event(hrtimer_get_expires(&ts->sched_timer), 1);
}
Expand Down

0 comments on commit aedb9eb

Please sign in to comment.