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..6e40ef8429afc89589a6a8f64c3408709a7e3e2a 100644
--- a/build/android/pylib/perf/perf_control.py
+++ b/build/android/pylib/perf/perf_control.py
@@ -4,15 +4,15 @@
 
 import atexit
 import logging
+import posixpath
 
 from pylib import android_commands
+from pylib import cmd_helper
 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 +20,11 @@ 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(
+        'cd %s && ls -d cpu[0-9]*' % self._CPU_PATH,

[Sami, 2014/11/18 13:24:04]

nit: You could replace "ls -d" with "echo" here.

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

"ls -d" is more robust. If for whatever reason the CPU files are not found, ls
will return a non-zero exit code and cause an exception to be raised. Echo, on
the other hand, will make us think that a single cpu file named "cpu[0-9]*" was
found.

+        check_return=True, as_root=True)

[perezju, 2014/11/17 21:20:56]

This will already throw an exception if either the path does not exist, or no
cpu files are found on the path.

+    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 +84,40 @@ class PerfControl(object):
     self._SetScalingGovernorInternal(governor_mode)
     self._ForceAllCpusOnline(False)
 
+  def GetCpuInfo(self):
+    online = (out.rstrip() == '1' and code == 0

[Sami, 2014/11/18 13:24:04]

nit: I think something like |exit_status| may be a better name for |code|.

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

Acknowledged. I went for |status| just to keep it short in these list
comprehensions.

+              for (_, out, code) in self._ForEachCpu('cat "$CPU/online"'))
+    governor = (out.rstrip() if code == 0 else None
+                for (_, out, code) in
+                self._ForEachCpu('cat "$CPU/cpufreq/scaling_governor"'))
+    return zip(self._cpu_files, online, governor)
+
+  def _ForEachCpu(self, cmd):
+    script = ('cd %s\n'
+              'for CPU in %s; do\n'
+              '  %s; echo -n "%%~%%$?%%~%%"\n'

[jbudorick, 2014/11/18 02:29:38]

What's the reasoning behind "%~%$?%~%"?

Do we just think it's sufficiently unlikely to be output by anything else?

[Sami, 2014/11/18 13:24:04]

Alternatively we could append the exit codes to a variable and print that at the
end for parsing.

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

The reasoning is basically just that.

Also I think it's best to keep the markers here, since they also serve as a
separator between the outputs of different iterations of the loop.

+              'done' % (self._CPU_PATH, self._cpu_file_list, cmd))

[jbudorick, 2014/11/18 02:29:38]

I think this would be more readable if you handled each line individually and
then joined them together once you've built them, e.g.

script_commands = [
  'cd %s' % self._CPU_PATH,
  'for CPU in %s' % self._cpu_file_list,
  'do %s' % cmd,
  'echo -n "%%~%%?%%~%%"',
  'done'
]
script = ';'.join(script_commands)

nit: I'd also rather you use semicolons than newlines while handrolling shell
scripts.

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

Acknowledged.

+    output = self._device.RunShellCommand(script, check_return=True,
+                                          as_root=True, raw_output=True)

[jbudorick, 2014/11/18 02:29:38]

I don't think this justifies adding raw_output to RunShellCommand.

+    output = output.split('%~%')
+    return zip(self._cpu_files, output[0::2], (int(c) for c in output[1::2]))

[perezju, 2014/11/17 21:20:56]

This is where the magic happens now, for each cpu we get a triple: (cpu, output,
exit_code), with the result of running each command on the device.

+
+  def _WriteEachCpuFile(self, path, value):
+    cmd = ('test -e %(path)s && echo %(value)s > %(path)s' % {
+        'path': cmd_helper.DoubleQuote(posixpath.join('$CPU', path)),

[jbudorick, 2014/11/18 02:29:37]

posixpath.join?

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

ok, ok, _maybe_ I don't have to be this much defensive on a private method that
is only called with sensible values from *other* private methods within the same
object.

+        'value': value})
+    output = self._ForEachCpu(cmd)
+    cpus = ' '.join(cpu for (cpu, _, code) in output if code == 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 +125,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"')
+    return all(out.rstrip() == '1' and code == 0

[perezju, 2014/11/17 21:20:56]

code will be non-zero if the file does not exist, and the output is not '1'
(usually '0'), if the cpu is offline.

+               for (cpu, out, code) in output
+               # TODO(epenner): Investigate why cpu0/online may be missing
+               # (http://crbug.com/397118)
+               if cpu != 'cpu0')
 
   def _ForceAllCpusOnline(self, force_online):
     """Enable all CPUs on a device.
@@ -132,15 +148,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"')