Re: [RFT][PATCH v5 0/7] sched/cpuidle: Idle loop rework

[Rafael J. Wysocki]

On Mon, Mar 19, 2018 at 12:36 PM, Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote:
> On Mon, Mar 19, 2018 at 11:49 AM, Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
>> On Sun, Mar 18, 2018 at 05:15:22PM +0100, Rafael J. Wysocki wrote:
>>> On Sun, Mar 18, 2018 at 12:00 PM, Rafael J. Wysocki <rjw@xxxxxxxxxxxxx> wrote:
>>> > @@ -354,6 +360,7 @@ static int menu_select(struct cpuidle_dr
>>> >         if (latency_req > interactivity_req)
>>> >                 latency_req = interactivity_req;
>>> >
>>> > +       expected_interval = TICK_USEC_HZ;
>>> >         /*
>>> >          * Find the idle state with the lowest power while satisfying
>>> >          * our constraints.
>>> > @@ -367,17 +374,44 @@ static int menu_select(struct cpuidle_dr
>>> >                         continue;
>>> >                 if (idx == -1)
>>> >                         idx = i; /* first enabled state */
>>> > -               if (s->target_residency > data->predicted_us)
>>> > +               if (s->target_residency > data->predicted_us) {
>>> > +                       /*
>>> > +                        * Retain the tick if the selected state is shallower
>>> > +                        * than the deepest available one with target residency
>>> > +                        * within the tick period range.
>>> > +                        *
>>> > +                        * This allows the tick to be stopped even if the
>>> > +                        * predicted idle duration is within the tick period
>>> > +                        * range to counter the effect by which the prediction
>>> > +                        * may be skewed towards lower values due to the tick
>>> > +                        * bias.
>>> > +                        */
>>> > +                       expected_interval = s->target_residency;
>>> >                         break;
>>>
>>> BTW, I guess I need to explain the motivation here more thoroughly, so
>>> here it goes.
>>>
>>> The governor predicts idle duration under the assumption that the
>>> tick will be stopped, so if the result of the prediction is within the tick
>>> period range and it is not accurate, that needs to be taken into
>>> account in the governor's statistics.  However, if the tick is allowed
>>> to run every time the governor predicts idle duration within the tick
>>> period range, the governor will always see that it was "almost
>>> right" and the correction factor applied by it to improve the
>>> prediction next time will not be sufficient.  For this reason, it
>>> is better to stop the tick at least sometimes when the governor
>>> predicts idle duration within the tick period range and the idea
>>> here is to do that when the selected state is the deepest available
>>> one with the target residency within the tick period range.  This
>>> allows the opportunity to save more energy to be seized which
>>> balances the extra overhead of stopping the tick.
>>
>> My brain is just not willing to understand how that work this morning.
>> Also it sounds really dodgy.
>
> Well, I guess I can't really explain it better. :-)
>
> The reason why this works better than the original v5 is because of
> how menu_update() works AFAICS.

Actually, it looks like menu_update() doesn't do the right thing when
the tick isn't stopped at all, because data->next_timer_us is useless
then.

Let me try to fix that in a new respin of the series.