Telemetry: Set scaling governor on all cpus (on all devices)

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 cde12ec9612b7933d88544124965364239b821df..274c4329a0f9d33be022dc0b6cbe1db9f9bd5604 100644
--- a/build/android/pylib/perf/perf_control.py
+++ b/build/android/pylib/perf/perf_control.py
@@ -32,6 +32,7 @@ class PerfControl(object):
 
   def SetHighPerfMode(self):
     """Sets the highest possible performance mode for the device."""
+    self._ForceAllCpusOnline(True)
     self._SetScalingGovernorInternal('performance')
 
   def SetDefaultPerfMode(self):
@@ -45,10 +46,12 @@ class PerfControl(object):
         'Nexus 10': 'interactive'
     }.get(product_model, 'ondemand')
     self._SetScalingGovernorInternal(governor_mode)
+    self._ForceAllCpusOnline(False)
 
   def RestoreOriginalPerfMode(self):
     """Resets the original performance mode of the device."""
     self._SetScalingGovernorInternal(self._original_scaling_governor)
+    self._ForceAllCpusOnline(False)

[pasko, 2014/06/17 11:48:30]

All this function does fithe False is 'start mpdecision'. Maybe factor this
functionality out to make it easier to read?

For non-qualcomm SoCs this does not bring down the CPUs that were not up
originally. I am not sure how important that is, and how robust it would be to
try doing the proper thing here (given the script gets interrupted by the user
sometimes), but it seems we may change the device behavior to something far from
the initial. This may cause unpleasant surprises (like half of our fleet
performs differently from another half, go figure).

[Sami, 2014/06/17 13:04:07]

We could always reboot the devices after running the tests if this turns out to
be a problem.

[pasko, 2014/06/17 15:12:37]

Maybe we should just remove the RestoreOriginalPerfMode()? Set the mode before
running tests, don't care about the behavior after running the tests => simpler.

[Sami, 2014/06/17 15:46:35]

Hmm, this is called from various front-ends -- not just the bot test runner
script -- and that wouldn't be appropriate in all cases.

[pasko, 2014/06/17 17:02:22]

OK, I just don't understand the contract we are maintaining. Why leaving
leftover cores online is acceptable, but the leftover governor is not? What are
these various frontends?

[epenner, 2014/06/17 22:10:55]

My thinking was that this only only different if mpdecision exists (other
devices were already like this, right?). And if mpdecision exists, it will
restore the cores to whatever it decides is 'correct' after we start it again. 

If any cores are left enabled though, then we need to restore their governors
(especially for CPU0).

Since all tests currently do this, I don't think there's currently an issue with
half the fleet having different behavior, but also if this doesn't work then I
don't see any fix short of rebooting the device. Ideally though mpdecision isn't
stateful and the device will behave the same any time it is enabled.

[pasko, 2014/06/18 10:59:49]

The issue is that we cannot really divide our current fleet by half :)
But I attempt to be optimistic: we will have more devices.
Spinning off the discussion here:
https://code.google.com/p/chromium/issues/detail?id=386082

 
   def _SetScalingGovernorInternal(self, value):
     for cpu in range(self._kernel_max + 1):
@@ -59,38 +62,35 @@ class PerfControl(object):
         self._device.old_interface.SetProtectedFileContents(
             scaling_governor_file, value)
 
-  def ForceAllCpusOnline(self, force_online):
-    """Force all CPUs on a device to be online.
+  def _ForceAllCpusOnline(self, force_online):
+    """Enable all CPUs on a device.
 
-    Force every CPU core on an Android device to remain online, or return the
-    cores under system power management control. This is needed to work around
-    a bug in perf which makes it unable to record samples from CPUs that become
-    online when recording is already underway.
+    Some vendors (or only Qualcomm?) hot-plug their CPUs, which can add noise

[pasko, 2014/06/17 11:48:30]

It seems Tegra and OMAP4 support hotplug, they just have a special scaling
governor value for that. Qualcomm is special in that regard: hotplugging via
userspace mpdecision even with governor set to 'performance', and not the sysfs
setting.

Since we have a notion of ProductModel, we can maintain a list of all the
Qualcomm product models here and avoid unnecessary adb shell commands for
others. WDYT about this (premature?) optimization? Caveat: 'Nexus 7v1' is a
tegra and 'Nexus 7v2' is a QC, having the same product model :/

[Sami, 2014/06/17 13:04:07]

I think such a list would quickly become unmaintanable -- we'd probably forget
to update it when adding new devices. "start" and "stop" should already be
no-ops on non-QC-devices, but we could also check whether
/system/sbin/mpdecision (IIRC) exists before running them.

[pasko, 2014/06/17 15:12:37]

Yes, I would prefer a real no-op :) If we check for mpdecision existence once
and reuse this knowledge for unaffected devices, that's better. Currently we are
running these same shell commands between test runs, that slows things down for
no reason.

[epenner, 2014/06/17 22:10:56]

Okay I buy it's good to check for mpdecision first, but are we sure it's worth
caching it?  There is several shell commands here and all the shell commands are
pretty tiny compared to the tests themselves.

[pasko, 2014/06/18 10:59:49]

You are right, we are deciding whether to trade smallish perf benefit for
smallish complexity.

A few more arguments in favor of more complexity:
* these shell commands take a few seconds total between test runs
* more adb commands pollute the verbose output on the bots, have to look through
more irrelevancy in the logs
* Seeing failures in the logs can be quite misleading, these quickly become red
herringz when something else happens on bots => harder for inexperienced
sheriffs to investigate


On the other hand, the core idea of this CL is much more important than this
minor comment, so I'm stopping my side of the bikeshed now :)

+    to measurements:
+    - In perf, samples are only taken for the CPUs that are online when the
+      measurement is started.
+    - The scaling governor can't be set for an offline CPU and frequency scaling
+      on newly enabled CPUs adds noise to both perf and tracing measurements.
+
+    It appears Qualcomm is the only vendor that hot-plugs CPUs, and on Qualcomm
+    this is done by "mpdecision".
 
-    Args:
-      force_online: True to set all CPUs online, False to return them under
-          system power management control.
     """
-    def ForceCpuOnline(online_path):
-      script = 'chmod 644 {0}; echo 1 > {0}; chmod 444 {0}'.format(online_path)
-      self._device.old_interface.RunShellCommandWithSU(script)
-      return self._device.old_interface.GetFileContents(online_path)[0] == '1'
-
-    def ResetCpu(online_path):
-      self._device.old_interface.RunShellCommandWithSU(
-          'chmod 644 %s' % online_path)
-
-    def WaitFor(condition):
-      for _ in range(100):
-        if condition():
-          return
-        time.sleep(0.1)
-      raise RuntimeError('Timed out')
+
+    # This is a no-op if the SOC doesn't use mpdecision.
+    script = 'stop mpdecision' if force_online else 'start mpdecision'
+    self._device.old_interface.RunShellCommandWithSU(script)

[pasko, 2014/06/17 11:48:30]

SU is not available on some devices. Here we can assume running as root, so
using RunShellCommand() would work on those (userdebug?) builds.

RunShellCommand/RunShellCommandWithSU is broken, instead it should be
RunShellCommand/RunPrivilegedShellCommand, and the RunPrivilegedShellCommand
should discover the best way to run (via SU or make sure it runs as root, or
fails). Let's fix it in another change.

[Sami, 2014/06/17 13:04:07]

Can we really assume that for all callers? At least telemetry doesn't require
root. Agreed that the interfaces for running-commands-as-root are somewhat
messed up but let's fix that separately.

[pasko, 2014/06/17 15:12:37]

Yes, let's do it separately.

I am absolutely sure that my test device will not work with this code the next
day the CL is committed, and I am about 99% sure nothing on the perf bots will
break if we assume root here today.

[Sami, 2014/06/17 15:46:35]

Right, again, this affects more than just the perf bots :(

[epenner, 2014/06/17 22:10:56]

Can I take away this okay for now? Will these commands can just fail silently
for non root devices? That should be fine?

[pasko, 2014/06/18 10:59:49]

Yeah, I think it will fail silently if adb is not run as root.

 
     cpu_online_files = self._device.old_interface.RunShellCommand(
         'ls -d /sys/devices/system/cpu/cpu[0-9]*/online')

[pasko, 2014/06/17 11:48:30]

Please use a constant like _CPU_DIR_FMT? there is also self._kernel_max, which
may soon exceed 10, no need to run extra shell commands.

-    for online_path in cpu_online_files:
-      if force_online:
-        WaitFor(lambda: ForceCpuOnline(online_path))
-      else:
-        ResetCpu(online_path)
+    if force_online:

[Sami, 2014/06/17 04:16:43]

nit: This could be moved to line 83 to early out before getting the list of
cores.

[epenner, 2014/06/17 22:10:55]

Nice. Done.

+      for online_path in cpu_online_files:
+        script = 'echo 1 > {0};'.format(online_path)

[pasko, 2014/06/17 11:48:30]

There is android_commands.SetProtectedFileContents()

[Sami, 2014/06/17 13:04:07]

Right, we should use that. FWIW it wasn't used here before because it adds quite
a bit of latency. Now that we stop mpdecision that's no longer an issue.

[epenner, 2014/06/17 22:10:56]

Ahh good point, yeah it needed the && before to win the race mpdecision. I'll
change it.

+        self._device.old_interface.RunShellCommandWithSU(script)
+
+      # Double check they all stayed online.
+      time.sleep(0.25)
+      for online_path in cpu_online_files:
+        if self._device.old_interface.GetFileContents(online_path)[0] == '0':
+          raise RuntimeError('Failed to force CPUs online')
+