On Thu, Mar 15, 2018 at 5:10 PM, Frederic Weisbecker <[email protected]> wrote: > On Mon, Mar 12, 2018 at 10:51:11AM +0100, Rafael J. Wysocki wrote: >> From: Rafael J. Wysocki <[email protected]> >> >> Push the decision whether or not to stop the tick somewhat deeper >> into the idle loop. >> >> Stopping the tick upfront leads to unpleasant outcomes in case the >> idle governor doesn't agree with the timekeeping code on the duration >> of the upcoming idle period. > > Looks like you meant "nohz" instead of "timekeeping"?
Yes, I did. >> Specifically, if the tick has been >> stopped and the idle governor predicts short idle, the situation is >> bad regardless of whether or not the prediction is accurate. If it >> is accurate, the tick has been stopped unnecessarily which means >> excessive overhead. If it is not accurate, the CPU is likely to >> spend too much time in the (shallow, because short idle has been >> predicted) idle state selected by the governor [1]. >> >> As the first step towards addressing this problem, change the code >> to make the tick stopping decision inside of the loop in do_idle(). >> In particular, do not stop the tick in the cpu_idle_poll() code path. >> Also don't do that in tick_nohz_irq_exit() which doesn't really have >> enough information on whether or not to stop the tick. >> >> Link: https://marc.info/?l=linux-pm&m=150116085925208&w=2 # [1] >> Link: >> https://tu-dresden.de/zih/forschung/ressourcen/dateien/projekte/haec/powernightmares.pdf >> Signed-off-by: Rafael J. Wysocki <[email protected]> >> --- >> kernel/sched/idle.c | 8 +++++--- >> kernel/time/tick-sched.c | 6 ++---- >> 2 files changed, 7 insertions(+), 7 deletions(-) >> >> Index: linux-pm/kernel/sched/idle.c >> =================================================================== >> --- linux-pm.orig/kernel/sched/idle.c >> +++ linux-pm/kernel/sched/idle.c >> @@ -241,10 +241,12 @@ static void do_idle(void) >> * broadcast device expired for us, we don't want to go deep >> * idle as we know that the IPI is going to arrive right away. >> */ >> - if (cpu_idle_force_poll || tick_check_broadcast_expired()) >> + if (cpu_idle_force_poll || tick_check_broadcast_expired()) { >> cpu_idle_poll(); >> - else >> + } else { >> + tick_nohz_idle_stop_tick(); >> cpuidle_idle_call(); >> + } > > I'm worried about one thing here. Say we enter cpuidle_idle_call() and the > tick is stopped. > Later on, we get a tick, so we exit cpuidle_idle_call(), then we find > cpu_idle_force_poll > or tick_check_broadcast_expired() to be true. So we poll but the tick hasn't > been updated > to fire again. > > I don't know if it can happen but cpu_idle_poll_ctrl() seem to be callable > anytime. > It looks like it's only used on __init code or on power suspend/resume, not > sure about > the implications on the latter, still there could be further misuse in the > future. > > Concerning tick_check_broadcast_expired(), it's hard to tell if it can be > enabled > concurrently from another CPU or from interrupts. > > Anyway perhaps we should have, out of paranoia: > > + if (cpu_idle_force_poll || tick_check_broadcast_expired()) { > + tick_nohz_idle_restart_tick(); > cpu_idle_poll(); > - else > Agreed, I'll update the patch accordingly. Thanks!
