RE: [RFC/RFT][PATCH v8] cpuidle: New timer events oriented governor for tickless systems

["Doug Smythies" <dsmythies@telus.net>]

On 2019.10.06 08:34 Rafael J. Wysocki wrote:
> On Sun, Oct 6, 2019 at 4:46 PM Doug Smythies <dsmythies@telus.net> wrote:
>> On 2019.10.01 02:32 Rafael J. Wysocki wrote:
>>> On Sun, Sep 29, 2019 at 6:05 PM Doug Smythies <dsmythies@telus.net> wrote:
>>>> On 2019.09.26 09:32 Doug Smythies wrote:
>>>>
>>>>> If the deepest idle state is disabled, the system
>>>>> can become somewhat unstable, with anywhere between no problem
>>>>> at all, to the occasional temporary jump using a lot more
>>>>> power for a few seconds, to a permanent jump using a lot more
>>>>> power continuously. I have been unable to isolate the exact
>>>>> test load conditions under which this will occur. However,
>>>>> temporarily disabling and then enabling other idle states
>>>>> seems to make for a somewhat repeatable test. It is important
>>>>> to note that the issue occurs with only ever disabling the deepest
>>>>> idle state, just not reliably.
>>>>>
>>>>> I want to know how you want to proceed before I do a bunch of
>>>>> regression testing.
>>>>
>> I do not think I stated it clearly before: The problem here is that some CPUs
>> seem to get stuck in idle state 0, and when they do power consumption spikes,
>> often by several hundred % and often indefinitely.
>
> That indeed has not been clear to me, thanks for the clarification!

>
>> I made a hack job automated test:
>> Kernel  tests  	         fail rate
>> 5.4-rc1		  6616           13.45%
>> 5.3              2376            4.50%
>> 5.3-teov7       12136            0.00%  <<< teo.c reverted and teov7 put in its place.
>> 5.4-rc1-ds      11168        0.00%  <<< [old] proposed patch (> 7 hours test time)


   5.4-rc1-ds12	  4224		0.005 <<< new proposed patch

>>
>> [old] Proposed patch (on top of kernel 5.4-rc1): [deleted]

> This change may cause the deepest state to be selected even if its
> "hits" metric is less than the "misses" one AFAICS, in which case the
> max_early_index state should be selected instead.
> 
> It looks like the max_early_index computation is broken when the
> deepest state is disabled.

O.K. Thanks for your quick reply, and insight.

I think long durations always need to be counted, but currently if
the deepest idle state is disabled, they are not.
How about this?:
(test results added above, more tests pending if this might be a path forward.)

diff --git a/drivers/cpuidle/governors/teo.c b/drivers/cpuidle/governors/teo.c
index b5a0e49..a970d2c 100644
--- a/drivers/cpuidle/governors/teo.c
+++ b/drivers/cpuidle/governors/teo.c
@@ -155,10 +155,12 @@ static void teo_update(struct cpuidle_driver *drv, struct cpuidle_device *dev)

                cpu_data->states[i].early_hits -= early_hits >> DECAY_SHIFT;

-               if (drv->states[i].target_residency <= sleep_length_us) {
-                       idx_timer = i;
-                       if (drv->states[i].target_residency <= measured_us)
-                               idx_hit = i;
+               if (!(drv->states[i].disabled || dev->states_usage[i].disable)){
+                       if (drv->states[i].target_residency <= sleep_length_us) {
+                               idx_timer = i;
+                               if (drv->states[i].target_residency <= measured_us)
+                                       idx_hit = i;
+                       }
                }
        }

@@ -256,39 +258,25 @@ static int teo_select(struct cpuidle_driver *drv, struct cpuidle_device *dev,
                struct cpuidle_state *s = &drv->states[i];
                struct cpuidle_state_usage *su = &dev->states_usage[i];

-               if (s->disabled || su->disable) {
-                       /*
-                        * If the "early hits" metric of a disabled state is
-                        * greater than the current maximum, it should be taken
-                        * into account, because it would be a mistake to select
-                        * a deeper state with lower "early hits" metric.  The
-                        * index cannot be changed to point to it, however, so
-                        * just increase the max count alone and let the index
-                        * still point to a shallower idle state.
-                        */
-                       if (max_early_idx >= 0 &&
-                           count < cpu_data->states[i].early_hits)
-                               count = cpu_data->states[i].early_hits;
-
-                       continue;
-               }

-               if (idx < 0)
-                       idx = i; /* first enabled state */
+               if (!(s->disabled || su->disable)) {
+                       if (idx < 0)
+                               idx = i; /* first enabled state */

-               if (s->target_residency > duration_us)
-                       break;
+                       if (s->target_residency > duration_us)
+                               break;

-               if (s->exit_latency > latency_req && constraint_idx > i)
-                       constraint_idx = i;
+                       if (s->exit_latency > latency_req && constraint_idx > i)
+                               constraint_idx = i;

-               idx = i;
+                       idx = i;

-               if (count < cpu_data->states[i].early_hits &&
-                   !(tick_nohz_tick_stopped() &&
-                     drv->states[i].target_residency < TICK_USEC)) {
-                       count = cpu_data->states[i].early_hits;
-                       max_early_idx = i;
+                       if (count < cpu_data->states[i].early_hits &&
+                           !(tick_nohz_tick_stopped() &&
+                             drv->states[i].target_residency < TICK_USEC)) {
+                               count = cpu_data->states[i].early_hits;
+                               max_early_idx = i;
+                       }
                }
        }