Re: [PATCH] sched: idle: Reenable sched tick for cpuidle request

[Rafael J. Wysocki]

On Thu, Aug 9, 2018 at 6:29 PM,  <leo.yan@xxxxxxxxxx> wrote:
> On Thu, Aug 09, 2018 at 05:42:30PM +0200, Rafael J. Wysocki wrote:
>
> [...]
>
>> >> This issue can be easily reproduce with the case on Arm Hikey board: use
>> >> CPU0 to send IPI to CPU7, CPU7 receives the IPI and in the callback
>> >> function it start a hrtimer with 4ms, so the 4ms timer delta value can
>> >> let 'menu' governor to choose deepest state in the next entering idle
>> >> time.  From then on, CPU7 restarts hrtimer with 1ms interval for total
>> >> 10 times, so this can utilize the typical pattern in 'menu' governor to
>> >> have prediction for 1ms duration, finally idle governor is easily to
>> >> select a shallow state, on Hikey board it usually is to select CPU off
>> >> state.  From then on, CPU7 stays in this shallow state for long time
>> >> until there have other interrupts on it.
>> >
>> > And which means that the above-mentioned code misses this case.
>>
>> And I don't really understand how this happens. :-/
>>
>> If menu sees that the tick has been stopped, it sets
>> data->predicted_us to the minimum of TICK_USEC and
>> ktime_to_us(delta_next) and the latency requirements comes from PM QoS
>> (no interactivity boost).  Thus the only case when it will say "do not
>> stop the tick" is when delta_next is below the tick period length, but
>> that's OK, because it means that there is a timer pending that much
>> time away, so it doesn't make sense to select a deeper idle state
>> then.
>>
>> If there is a short-interval timer pending every time we go idle, it
>> doesn't matter that the tick is stopped really, because the other
>> timer will wake the CPU up anyway.
>>
>> Have I missed anything?
>
> Yeah, you miss one case is if there haven't anymore timer event, for this
> case the ktime_to_us(delta_next) is a quite large value and
> data->predicted_us will be to set TICK_USEC; if HZ=1000 then TICK_USEC is
> 1000us, on Hikey board if data->predicted_us is 1000us then it's easily
> to set shallow state (C1) rather than C2.  Unfortunately, this is the
> last time the CPU can predict idle state before it will stay in idle
> for long period.

Fair enough, but in that case the governor will want the tick to be
stopped, because expected_interval is TICK_USEC then, so I'm not sure
how the patch helps?

> [...]
>
>> >> diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c
>> >> index 1a3e9bd..802286e 100644
>> >> --- a/kernel/sched/idle.c
>> >> +++ b/kernel/sched/idle.c
>> >> @@ -190,10 +190,18 @@ static void cpuidle_idle_call(void)
>> >>                */
>> >>               next_state = cpuidle_select(drv, dev, &stop_tick);
>> >>
>> >> -             if (stop_tick)
>> >> +             if (stop_tick) {
>> >>                       tick_nohz_idle_stop_tick();
>> >> -             else
>> >> +             } else {
>> >> +                     /*
>> >> +                      * The cpuidle framework says to not stop tick but
>> >> +                      * the tick has been stopped yet, so restart it.
>> >> +                      */
>> >> +                     if (tick_nohz_tick_stopped())
>> >> +                             tick_nohz_idle_restart_tick();
>> >
>> > You need an "else" here IMO as Peter said.
>
> Yeah, I have replied to Peter, after we restart the tick, I found must to
> call tick_retain() to clear 'ts->timer_expires_base' to 0, otherwise
> tick_nohz_idle_exit() reports warning when exit from idle loop.

I see now, thanks.

>> And I really would prefer to avoid restarting the tick here, because
>> it is overhead and quite likely unnecessary.
>
> I understand the logic when read the code, actually I did some experiments
> on the function menu_select(), in menu_select() function it discards the
> consideration for typical pattern interval and it also tries to avoid to
> enable tick and select more shallow state at the bottom of function.  So I
> agree that in the middle of idles it's redundant to reenable tick and the
> code is careful thought.
>
> But this patch tries to rescue the case at the last time the CPU enter one
> shallow idle state but without wake up event.

It is better to avoid entering a shallow state IMO.  Let me think
about that a bit.