| From a3675a4de4388d22bd5fdc87157ef88d4c56c66e Mon Sep 17 00:00:00 2001 |
| From: "Joel Fernandes (Google)" <joel@joelfernandes.org> |
| Date: Thu, 4 May 2023 14:41:55 -0400 |
| Subject: [PATCH] CHROMIUM: tick-sched: Set last_tick correctly so that timer |
| interrupts 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 #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 |
| UPSTREAM-TASK=b:289837863 |
| TEST=cyclictest --smp -p95 -m -i 100 -d 100 with timer_highres=0 |
| |
| Change-Id: I3221e458ec6265d9e4a5f68a3e0b7d1c3558be1b |
| Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> |
| Tested-by: Joel Fernandes <joelaf@google.com> |
| Tested-by: Vineeth Pillai <vineethrp@google.com> |
| (cherry picked from commit aedb9eb98068331f76f875ac5bfa30d28dfac232) |
| Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/4855709 |
| Reviewed-by: Joel Fernandes <joelaf@google.com> |
| Commit-Queue: Joel Fernandes <joelaf@google.com> |
| --- |
| kernel/time/tick-sched.c | 3 +++ |
| 1 file changed, 3 insertions(+) |
| |
| diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c |
| index be77b021e5d63b957465c647bac26714ff7d3825..21f189e8d772375e2572bc21ea81d436f0c3f238 100644 |
| --- a/kernel/time/tick-sched.c |
| +++ b/kernel/time/tick-sched.c |
| @@ -1403,6 +1403,8 @@ static void tick_nohz_lowres_handler(struct clock_event_device *dev) |
| tick_sched_do_timer(ts, now); |
| tick_sched_handle(ts, regs); |
| |
| + ts->last_tick = now; |
| + |
| /* |
| * In dynticks mode, tick reprogram is deferred: |
| * - to the idle task if in dynticks-idle |
| @@ -1413,6 +1415,7 @@ static void tick_nohz_lowres_handler(struct clock_event_device *dev) |
| tick_program_event(hrtimer_get_expires(&ts->sched_timer), 1); |
| } |
| |
| + hrtimer_set_expires(&ts->sched_timer, ts->last_tick); |
| } |
| |
| static inline void tick_nohz_activate(struct tick_sched *ts, int mode) |
| -- |
| 2.43.0.rc2.451.g8631bc7472-goog |
| |