Re: [RFT][PATCH v4 2/7] sched: idle: Do not stop the tick upfront in the idle loop

[Rafael J. Wysocki]

On Thu, Mar 15, 2018 at 5:10 PM, Frederic Weisbecker
<frederic@xxxxxxxxxx> wrote:
> On Mon, Mar 12, 2018 at 10:51:11AM +0100, Rafael J. Wysocki wrote:
>> From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
>>
>> 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 <rafael.j.wysocki@xxxxxxxxx>
>> ---
>>  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!