A more robust way to set/query CPU properties on 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 ede131789d70faca081d3810e5ccafb659adfb07..81bc7be983b8d8d826d9781c54c34124f1af0e57 100644
--- a/build/android/pylib/perf/perf_control.py
+++ b/build/android/pylib/perf/perf_control.py
@@ -10,9 +10,7 @@ from pylib.device import device_utils
 
 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')
-  _CPU_ONLINE_FMT = '/sys/devices/system/cpu/cpu%d/online'
+  _CPU_PATH = '/sys/devices/system/cpu'
   _KERNEL_MAX = '/sys/devices/system/cpu/kernel_max'
 
   def __init__(self, device):
@@ -20,11 +18,10 @@ class PerfControl(object):
     if isinstance(device, android_commands.AndroidCommands):
       device = device_utils.DeviceUtils(device)
     self._device = device
-    cpu_files = self._device.RunShellCommand(
-      'ls -d /sys/devices/system/cpu/cpu[0-9]*')
-    self._num_cpu_cores = len(cpu_files)
-    assert self._num_cpu_cores > 0, 'Failed to detect CPUs.'
-    logging.info('Number of CPUs: %d', self._num_cpu_cores)
+    self._cpu_files = self._device.RunShellCommand('ls -d cpu[0-9]*',

[jbudorick, 2014/11/18 18:55:47]

Same as below re param formatting.

+        cwd=self._CPU_PATH, check_return=True, as_root=True)
+    self._cpu_file_list = ' '.join(self._cpu_files)
+    logging.info('CPUs found: %s', self._cpu_file_list)
     self._have_mpdecision = self._device.FileExists('/system/bin/mpdecision')
 
   def SetHighPerfMode(self):
@@ -84,22 +81,40 @@ class PerfControl(object):
     self._SetScalingGovernorInternal(governor_mode)
     self._ForceAllCpusOnline(False)
 
+  def GetCpuInfo(self):
+    online = (out.rstrip() == '1' and status == 0
+              for (_, out, status) in self._ForEachCpu('cat "$CPU/online"'))
+    governor = (out.rstrip() if status == 0 else None
+                for (_, out, status) in

[jbudorick, 2014/11/18 18:55:47]

nit: move "in" down a line.

+                self._ForEachCpu('cat "$CPU/cpufreq/scaling_governor"'))
+    return zip(self._cpu_files, online, governor)
+
+  def _ForEachCpu(self, cmd):
+    script = '; '.join([
+      'for CPU in %s' % self._cpu_file_list,
+      'do %s' % cmd,
+      'echo -n "%~%$?%~%"',
+      'done'
+    ])
+    output = self._device.RunShellCommand(script, cwd=self._CPU_PATH,

[jbudorick, 2014/11/18 18:55:47]

Either:
  1) put parameters on the same line as the function call and indent subsequent
lines to the depth of the first parameter, or
  2) put all parameters on subsequent lines indented four spaces.

Don't mix the two.

+        check_return=True, as_root=True)
+    output = '\n'.join(output).split('%~%')

[perezju, 2014/11/18 17:42:42]

I do get the argument of not littering RunShellCommand with more options, but I
do feel it's a bit "wrong" that there is no way at all to get back the raw
output from the command (this join is not guaranteed to produce the exact same
original output, since splitlines is effectively not reversible). Anyway, here
it doesn't matter much, and perhaps in most other cases it also wont matter at
all. I guess I'll just have to live with that small dissonance in my head. :P

[jbudorick, 2014/11/18 18:55:47]

I'm having a hard time coming up with a case where a caller would care about
which lines end with CRLF vs which lines end with LF. If that happens, we can
reconsider this option.

+    return zip(self._cpu_files, output[0::2], (int(c) for c in output[1::2]))
+
+  def _WriteEachCpuFile(self, path, value):
+    output = self._ForEachCpu('test -e %(file)s && echo %(value)s > %(file)s'

[jbudorick, 2014/11/18 18:55:47]

Same as above re param formatting.

+        % {'file': '"$CPU/%s"' % path, 'value': value})
+    cpus = ' '.join(cpu for (cpu, _, status) in output if status == 0)
+    if cpus:
+      logging.info('Succesfully set %s to %r on: %s', path, value, cpus)
+    else:
+      logging.warning('Failed to set %s to %r on any cpus')
+
   def _SetScalingGovernorInternal(self, value):
-    cpu_cores = ' '.join([str(x) for x in range(self._num_cpu_cores)])
-    script = ('for CPU in %s; do\n'
-        '  FILE="/sys/devices/system/cpu/cpu$CPU/cpufreq/scaling_governor"\n'
-        '  test -e $FILE && echo %s > $FILE\n'
-        'done\n') % (cpu_cores, value)
-    logging.info('Setting scaling governor mode: %s', value)
-    self._device.RunShellCommand(script, as_root=True)
+    self._WriteEachCpuFile('cpufreq/scaling_governor', value)
 
   def _SetScalingMaxFreq(self, value):
-    cpu_cores = ' '.join([str(x) for x in range(self._num_cpu_cores)])
-    script = ('for CPU in %s; do\n'
-        '  FILE="/sys/devices/system/cpu/cpu$CPU/cpufreq/scaling_max_freq"\n'
-        '  test -e $FILE && echo %d > $FILE\n'
-        'done\n') % (cpu_cores, value)
-    self._device.RunShellCommand(script, as_root=True)
+    self._WriteEachCpuFile('cpufreq/scaling_max_freq', '%d' % value)
 
   def _SetMaxGpuClock(self, value):
     self._device.WriteFile('/sys/class/kgsl/kgsl-3d0/max_gpuclk',
@@ -107,14 +122,12 @@ class PerfControl(object):
                            as_root=True)
 
   def _AllCpusAreOnline(self):
-    for cpu in range(1, self._num_cpu_cores):
-      online_path = PerfControl._CPU_ONLINE_FMT % cpu
-      # TODO(epenner): Investigate why file may be missing
-      # (http://crbug.com/397118)
-      if not self._device.FileExists(online_path) or \
-            self._device.ReadFile(online_path)[0] == '0':
-        return False
-    return True
+    output = self._ForEachCpu('cat "$CPU/online"')
+    # TODO(epenner): Investigate why file may be missing
+    # (http://crbug.com/397118)
+    return all(out.rstrip() == '1' and status == 0
+               for (cpu, out, status) in output
+               if cpu != 'cpu0')
 
   def _ForceAllCpusOnline(self, force_online):
     """Enable all CPUs on a device.
@@ -132,15 +145,10 @@ class PerfControl(object):
     """
     if self._have_mpdecision:
       script = 'stop mpdecision' if force_online else 'start mpdecision'
-      self._device.RunShellCommand(script, as_root=True)
+      self._device.RunShellCommand(script, check_return=True, as_root=True)
 
     if not self._have_mpdecision and not self._AllCpusAreOnline():
       logging.warning('Unexpected cpu hot plugging detected.')
 
-    if not force_online:
-      return
-
-    for cpu in range(self._num_cpu_cores):
-      online_path = PerfControl._CPU_ONLINE_FMT % cpu
-      self._device.WriteFile(online_path, '1', as_root=True)
-
+    if force_online:
+      self._ForEachCpu('echo 1 > "$CPU/online"')