Telemetry: manage CPU core online state for some device types

build/android/pylib/perf/perf_control.py

Index: build/android/pylib/perf/perf_control.py
diff --git a/build/android/pylib/perf/perf_control.py b/build/android/pylib/perf/perf_control.py
index e289991f84abd918780ce520db4467e3ceb5ec64..0f617380276a8cbf8aa237e1ca1d5e10e6549e7f 100644
--- a/build/android/pylib/perf/perf_control.py
+++ b/build/android/pylib/perf/perf_control.py
@@ -4,13 +4,21 @@
 
 
 import logging
+import time
 
 
 class PerfControl(object):
   """Provides methods for setting the performance mode of a device."""
-  _SCALING_GOVERNOR_FMT = (
-      '/sys/devices/system/cpu/cpu%d/cpufreq/scaling_governor')
+
   _KERNEL_MAX = '/sys/devices/system/cpu/kernel_max'
+  _CPU_DIR_FMT = '/sys/devices/system/cpu/cpu%d/'
+  _SCALING_GOVERNOR_FMT = _CPU_DIR_FMT + 'cpufreq/scaling_governor'
+  _CPU_ONLINE_FMT = _CPU_DIR_FMT + 'online'
+  _SCALING_FREQUENCY_STAT_NAMES = [
+      'cpufreq/scaling_min_freq',
+      'cpufreq/scaling_max_freq',
+      'cpufreq/scaling_cur_freq',
+      ]

[bulach, 2013/11/15 17:54:43]

nit: align with _ from 17

[pasko, 2013/11/18 15:11:18]

Done.

 
   def __init__(self, adb):
     self._adb = adb
@@ -22,13 +30,87 @@ class PerfControl(object):
     self._original_scaling_governor = self._adb.GetFileContents(
         PerfControl._SCALING_GOVERNOR_FMT % 0,
         log_result=False)[0]
+    self._LogCurrentCpuFrequencyInfo()
+
+    # Manage CPUs on some device types manually to reduce variance in results.

[bulach, 2013/11/15 17:54:43]

"manually" is a bit misleading, and they're all "managed" one way or another....

so perhaps:
self._forced_cpus as the list (i.e, in place of "cpus_to_manager" ?
no need for the bool, you can infer that from self._forced_cpus not being
empty... wdyt?

[pasko, 2013/11/18 15:11:18]

I tried that, and it looks less readable to me to check for emptiness of a list
in a few places compared to checking for one boolean mode-like variable that is
set up once and remains constant throughout the lifetime of the object. Though
arguably the list is setup only once too. _forced_cpus sounds also a bit
general, how about _must_be_online_cpus?

[bulach, 2013/11/19 15:13:57]

how about this:

extract cpu_indexes_to_manage_manually into a constant:

_MODELS_TO_FULLY_MANAGED_CPU = {
  'Nexus 4': [0, 1],
  'Nexus 5': [0, 1],
}

then rather than a bool + list, it can use:
  self._fully_managed_cpus = _MODELS_TO_FULLY_MANAGED_CPU.get(
    self._adb.GetProductModel(), [])

and from then on, you can just test "if self._fully_managed_cpus" as a simple
bool-like test..

I still think "manually" is a bit misleading as nothing here is manual, this
script can't even be called from the shell directly :)

...or perhaps even more directly like governor_mode, and just have:

self._must_be_online_cpus = {
  'Nexus 4' : [0, 1],
  'Nexut 5' : [0, 1],
}.get(self._adb.GetProductModel(), [])

[pasko, 2013/11/19 18:00:08]

I like "fully managed CPUs" as a term. Thanks!!

self._fully_managed_cpus is okay, I'll go with that, it is less readable to me
when one variable gets interpreted as different types in different contexts (I
just had one of those rare Perl flashbacks), but you are right, seems like more
pythonistic readers will find it more convenient.

+    # Other device types are either hard to manage properly or we did not decide
+    # what settings look 'typical' for them. 'Galaxy Nexus' and 'Nexus 10' seem
+    # to use both cores all time without hotplugging them manually, but we still
+    # may need to manage their frequency later. TODO(pasko): manage Nexus 7
+    # manually, v1 and v2 differ significantly.
+    #
+    # *Not managing* CPUs manually means that only a subset of CPU properties
+    # are updated (scaling governor only now) and only for those CPUs
+    # represented by sysfs at the moment (/sys/devices/system/cpu/cpuX/...)
+    self._manage_cpus_manually = False
+    cpu_indexes_to_manage_manually = {
+        'Nexus 4': [0, 1],
+        'Nexus 5': [0, 1],

[aberent, 2013/11/15 19:00:12]

What is your intention in only managing two of the CPUs? I have certainly seen,
in traces, Chrome using 4 CPUs, and the (relatively small) difference in Chrome
startup time between a normal Nexus4 and Svelte Nexus 4 appeared to be due to
svelte disabling two of the CPUs. 

[pasko, 2013/11/18 15:11:18]

I looked at a few pagecycler runs on the bot and found that 2-3 CPUs are used at
the end of the tests. With 2 I am also trying to be on the safer side to avoid
overheating, but we can certainly tweak these numbers later. The main purpose is
making the number of CPUs online be a constant throughout the test run to make
the results more predictable.

Frankly, I did not measure whether anything in fact became more stable, we would
need to check that on the dashboard since variability impact can be quite
different for different tests.

+    }
+    device_model = self._adb.GetProductModel()
+    if device_model in cpu_indexes_to_manage_manually:
+      self._manage_cpus_manually = True
+      self._cpus_to_manage = cpu_indexes_to_manage_manually[device_model]
+      self._cpus_on_originally = []
+      for cpu in xrange(self._kernel_max +1):

[bulach, 2013/11/15 17:54:43]

nit: space after +

[pasko, 2013/11/18 15:11:18]

Done.

+        if '1' in self._ReadProtectedFile(PerfControl._CPU_ONLINE_FMT % cpu):
+          self._cpus_on_originally.append(cpu)
+
+  def _BringCpuOffline(self, cpu_index):
+    logging.info('Bringing CPU #%d offline.' %cpu_index)

[bulach, 2013/11/15 17:54:43]

nit: logging takes format and vararg, so replace % with ,

[pasko, 2013/11/18 15:11:18]

Done.

+    self._adb.SetProtectedFileContents(PerfControl._CPU_ONLINE_FMT, '0')

[aberent, 2013/11/15 19:00:12]

This doesn't use cpu_index, how does it know which cpu to take offline? Do you
mean PerfControl._CPU_ONLINE_FMT % cpu_index?

[pasko, 2013/11/18 15:11:18]

That's a great catch! Thank you! Do you think it makes sense to mock self._adb
and verify the logic in unittests? The logic is not particularly tricky, but ..
with Python I am never sure even the syntax of the code is correct without
covering every line of code at runtime, which can be even more tedious than
writing unittests. Yes, I am not a big advocate for Python :)

+
+  def _BringCpuOnline(self, cpu_index):
+    "Brings a CPU core online, waits for it to show up in sysfs."
+
+    logging.info('Bringing CPU #%d online.' %cpu_index)
+    times_to_wait_left = 5
+    cpu_online_file = PerfControl._CPU_ONLINE_FMT % cpu_index
+    while (True):
+      self._adb.SetProtectedFileContents(cpu_online_file, '1')
+      if '1' in self._ReadProtectedFile(cpu_online_file):
+        break
+      else:

[bulach, 2013/11/15 17:54:43]

nit: no need for else, unindent the following block.

[pasko, 2013/11/18 15:11:18]

Done.

+        if times_to_wait_left == 0:
+          logging.warning('Gave up bringing CPU %d online')

[bulach, 2013/11/15 17:54:43]

nit: missing ", cpu_index"

[pasko, 2013/11/18 15:11:18]

Thank you! You know, I really got used to GCC warnings about unsatisfied format
strings, but luckily good reviewers do the job of the compiler. Adds a little
bit feeling of guilt, but not outside manageable :)

Done.

+          break
+        times_to_wait_left -= 1
+        logging.warning('Could not bring CPU %d online, retrying..' %

[bulach, 2013/11/15 17:54:43]

nit: s/%/,/

[pasko, 2013/11/18 15:11:18]

Done.

+            cpu_index)
+        time.sleep(0.1)
+
+  def _ReadProtectedFile(self, file_name):
+    return '\n'.join(self._adb.GetProtectedFileContents(file_name))

[bulach, 2013/11/15 17:54:43]

it this function needed? :)
line 71 would be fine:
'1' in ['1', 'foo', 'bar']

and then just do this directly at line 90...

[pasko, 2013/11/18 15:11:18]

This function made 3 lines (line 56, 72 and 90) shorter and not wrap, on the
other hand it added 3 lines to the file. The final argument of it being a
descriptive name won.

[bulach, 2013/11/19 15:13:57]

perhaps then name it as _GetProtectedFileContents as the underlying method it's
calling?

[pasko, 2013/11/19 18:00:08]

Whichever you like. Done.

+
+  def _LogCurrentCpuFrequencyInfo(self):
+    for cpu in xrange(self._kernel_max + 1):
+      for stat_name in PerfControl._SCALING_FREQUENCY_STAT_NAMES:
+        stat_file_name = (PerfControl._CPU_DIR_FMT + stat_name) % cpu;
+        if self._adb.FileExistsOnDevice(stat_file_name):
+          info = self._ReadProtectedFile(stat_file_name)
+          logging.info('CPU #%d frequency info: %s: %s', cpu, stat_name, info)
 
   def SetHighPerfMode(self):
     """Sets the highest possible performance mode for the device."""
-    self._SetScalingGovernorInternal('performance')
+
+    if not self._manage_cpus_manually:
+      for cpu in xrange(self._kernel_max + 1):
+        self._SetScalingGovernorInternal(cpu, 'performance')
+    else:
+      # Stop the proprietary 'mpdecision' hotplug manager to get a constant

[bulach, 2013/11/15 17:54:43]

is "proprietary" relevant? How about just "Stop 'mdpdecision' hotplug ...

[pasko, 2013/11/18 15:11:18]

no, not relevant, I was just angry :) Removed. I would not take the suggestion
of your s with a typo because it may sound even more rude than "proprietary".
But if you want, we can make it "m#$%decision". 

+      # number of CPUs online when running tests. TODO(pasko): stop 'thermald'.

[aberent, 2013/11/15 19:00:12]

Isn't stopping thermald going to burn out the device?

[pasko, 2013/11/18 15:11:18]

Yes, it certainly may. So I was thinking of having it as option for devices with
enough heatsinks installed. It is not typical to user devices, but I value low
variance more than being maximum representative. Let's separate this: in-lab
performance tests should be with low variance, in-field metrics/histograms
should be representative. WDYT?

[aberent, 2013/11/18 16:32:05]

Makes sense, if we can find devices that will run reliably at full performance
without overheating. 

+      self._adb.RunShellCommandWithSU('stop mpdecision')
+      for cpu in xrange(self._kernel_max + 1):
+        if cpu in self._cpus_to_manage:
+          self._BringCpuOnline(cpu)
+          self._SetScalingGovernorInternal(cpu, 'performance')
+        else:
+          self._BringCpuOffline(cpu)
 
   def SetDefaultPerfMode(self):
     """Sets the performance mode for the device to its default mode."""
+
+    self._LogCurrentCpuFrequencyInfo()
     product_model = self._adb.GetProductModel()
     governor_mode = {
         'GT-I9300': 'pegasusq',
@@ -37,16 +119,36 @@ class PerfControl(object):
         'Nexus 7': 'interactive',
         'Nexus 10': 'interactive'
     }.get(product_model, 'ondemand')
-    self._SetScalingGovernorInternal(governor_mode)
+    if not self._manage_cpus_manually:
+      for cpu in xrange(self._kernel_max + 1):
+        self._SetScalingGovernorInternal(cpu, governor_mode)
+    else:
+      # What is the 'Default' state for CPU online/offline state if we manage it
+      # manually? It is the choice of CPU presence that was made earlier.
+      pass

[bulach, 2013/11/15 17:54:43]

sorry, this block is a bit confusing :)
how about just inside 122:

if not self._forced_cpus:
  # Set the default perf mode only if the CPU was not forced online.
  for cpu in xrange(...):
    ...

[pasko, 2013/11/18 15:11:18]

Sorry, it was. Good suggestion. Done.

+    # If 'mpdecision' is present on the system, it will manage CPU
+    # online/offline state, otherwise it is a no-op.
+    self._adb.RunShellCommandWithSU('start mpdecision')
 
   def RestoreOriginalPerfMode(self):
     """Resets the original performance mode of the device."""
-    self._SetScalingGovernorInternal(self._original_scaling_governor)
-
-  def _SetScalingGovernorInternal(self, value):
-    for cpu in range(self._kernel_max + 1):
-      scaling_governor_file = PerfControl._SCALING_GOVERNOR_FMT % cpu
-      if self._adb.FileExistsOnDevice(scaling_governor_file):
-        logging.info('Writing scaling governor mode \'%s\' -> %s',
-                     value, scaling_governor_file)
-        self._adb.SetProtectedFileContents(scaling_governor_file, value)
+
+    self._LogCurrentCpuFrequencyInfo()
+    if not self._manage_cpus_manually:
+      for cpu in xrange(self._kernel_max + 1):
+        self._SetScalingGovernorInternal(cpu, self._original_scaling_governor)
+    else:
+      for cpu in xrange(self._kernel_max + 1):
+        if cpu in self._cpus_on_originally:

[aberent, 2013/11/15 19:00:12]

I think if the CPU was both online originally and was in the manually managed
set you need to reset the performance mode.

[pasko, 2013/11/18 15:11:18]

That's right. I forgot to restore the performance governor mode, mainly because
it would be manipulated by mpdecision anyway. This would be cleaner if restores
were performed with scaling governor involved explicitly. Done.

By the way, always wanted to ask. Do we use SetDefaultPerfMode() for something
that is running on perf bots or is it just for local experimentation?

+          self._BringCpuOnline(cpu)
+        else:
+          self._BringCpuOffline(cpu)

[aberent, 2013/11/15 19:00:12]

Does this clear the performance mode back to the default, or does it simply hide
it? If it simply hides it then you need to reset the performance mode before
taking it offline.

[pasko, 2013/11/18 15:11:18]

Done.

+      self._adb.RunShellCommandWithSU('start mpdecision')
+
+  def _SetScalingGovernorInternal(self, cpu, value):
+    scaling_governor_file = PerfControl._SCALING_GOVERNOR_FMT % cpu
+    if (self._manage_cpus_manually or
+        self._adb.FileExistsOnDevice(scaling_governor_file)):
+      logging.info('Writing scaling governor mode \'%s\' -> %s',

[bulach, 2013/11/15 17:54:43]

nit: replace \' with "

[pasko, 2013/11/18 15:11:18]

Done.

+                   value, scaling_governor_file)
+      self._adb.SetProtectedFileContents(scaling_governor_file, value)