Re: [PATCH v2] cpuidle: menu: Handle stopped tick more aggressively

[Rafael J. Wysocki]

On Sun, Aug 12, 2018 at 3:44 PM <leo.yan@xxxxxxxxxx> wrote:
>
> On Sun, Aug 12, 2018 at 12:07:45PM +0200, Rafael J. Wysocki wrote:
>
> [...]
>
> > > > > > --- linux-pm.orig/drivers/cpuidle/governors/menu.c
> > > > > > +++ linux-pm/drivers/cpuidle/governors/menu.c
> > > > > > @@ -285,9 +285,8 @@ static int menu_select(struct cpuidle_dr
> > > > > >  {
> > > > > >       struct menu_device *data = this_cpu_ptr(&menu_devices);
> > > > > >       int latency_req = cpuidle_governor_latency_req(dev->cpu);
> > > > > > -     int i;
> > > > > > -     int first_idx;
> > > > > > -     int idx;
> > > > > > +     int first_idx = 0;
> > > > > > +     int idx, i;
> > > > > >       unsigned int interactivity_req;
> > > > > >       unsigned int expected_interval;
> > > > > >       unsigned long nr_iowaiters, cpu_load;
> > > > > > @@ -307,6 +306,18 @@ static int menu_select(struct cpuidle_dr
> > > > > >       /* determine the expected residency time, round up */
> > > > > >       data->next_timer_us = ktime_to_us(tick_nohz_get_sleep_length(&delta_next));
> > > > > >
> > > > > > +     /*
> > > > > > +      * If the tick is already stopped, the cost of possible short idle
> > > > > > +      * duration misprediction is much higher, because the CPU may be stuck
> > > > > > +      * in a shallow idle state for a long time as a result of it.  In that
> > > > > > +      * case say we might mispredict and use the known time till the closest
> > > > > > +      * timer event for the idle state selection.
> > > > > > +      */
> > > > > > +     if (tick_nohz_tick_stopped()) {
> > > > > > +             data->predicted_us = ktime_to_us(delta_next);
> > > > > > +             goto select;
> > > > > > +     }
> > > > > > +
> > > > >
> > > > > This introduce two potential issues:
> > > > >
> > > > > - This will totally ignore the typical pattern in idle loop; I
> > > > >   observed on the mmc driver can trigger multiple times (> 10 times)
> > > > >   with consistent interval;
> > > >
> > > > I'm not sure what you mean by "ignore".
> > >
> > > You could see after move code from blow to this position, the typical
> > > pattern interval will not be accounted; so if in the middle of idles
> > > there have a bunch of interrupts with fix pattern, the upper code
> > > cannot detect this pattern anymore.
> >
> > I'm not really following you here.
> >
> > The part of the code skipped for tick_nohz_tick_stopped() doesn't
> > update the data at all AFAICS.  It only computes some values that
> > would be discarded later anyway, so I'm not sure what the point of
> > running that computation is.
>
> Sorry I don't explain clearly, so try to rephrase:
>
> With your patch for the tick stopped case, it directly uses tick delta
> value as prediction and goto 'select' tag.  So it skips below code
> pieces, these codes have minor improvement for typical pattern which
> can be applied in the middle of idles, for example, the mmc driver
> triggers 16 interrupts with ~1500us interval, these interrupts are all
> handled within the idle loop, so the typical pattern can detect the mmc
> interrupts pattern and it will help idle governor to select a shallower
> idle state so can avoid to break the residency.
>
> You mentioned these computed values would be discarded later, this is
> true for most cases, but it isn't always true actually.  Without your
> patch, the governor will discard the computed values only when
> 'data->predicted_us < TICK_USEC', otherwise the interval pattern is
> still be applied in the prediction.

OK, right.

I'll fix that up in v4, thanks!