A more robust way to set/query CPU properties on devices

build/android/pylib/perf/perf_control.py

 # Copyright 2013 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 import atexit
 import logging
 
 from pylib import android_commands
 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):
     # TODO(jbudorick) Remove once telemetry gets switched over.
     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):
     """Sets the highest stable performance mode for the device."""
     if not self._device.old_interface.IsRootEnabled():
       message = 'Need root for performance mode. Results may be NOISY!!'
       logging.warning(message)
       # Add an additional warning at exit, such that it's clear that any results
       # may be different/noisy (due to the lack of intended performance mode).
       atexit.register(logging.warning, message)
@@ skipping 39 matching lines @@
         'GT-I9300': 'pegasusq',
         'Galaxy Nexus': 'interactive',
         'Nexus 4': 'ondemand',
         'Nexus 5': 'ondemand',
         'Nexus 7': 'interactive',
         'Nexus 10': 'interactive'
     }.get(product_model, 'ondemand')
     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',
                            str(value),
                            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.
 
     Some vendors (or only Qualcomm?) hot-plug their CPUs, which can add noise
     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".
 
     """
     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"')