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
+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):
     # 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(
+        '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):
     """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 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',
                            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"')
+    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.
 
     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"')