On Sun, Aug 12, 2018 at 3:44 PM [off-list ref] wrote:

On Sun, Aug 12, 2018 at 12:07:45PM +0200, Rafael J. Wysocki wrote:

[...]

quoted

quoted

quoted

quoted

quoted

--- 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!