From 023c0e0a957fa6e3a4818e85de1d2b6762ccefd0 Mon Sep 17 00:00:00 2001 From: Denis Nikitin Date: Wed, 27 Nov 2019 15:34:23 -0800 Subject: crosperf: Make StopUI/StartUI more robust There was a bug when crosperf failed due to StopUI failure. This happened when in a preceding run crosperf exits with an exception in DeviceSetup (for example Keyboard interrupt) and fails to call StartUI. Current change fixes this problem and includes a unittest case testing the exception case. BUG=None TEST=unittest and HW test on DUT with stopped ui pass Change-Id: Id6c69aebefe21b12ec7ee3a7c7f9dff92d143908 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/toolchain-utils/+/1941036 Commit-Queue: Denis Nikitin Tested-by: Denis Nikitin Reviewed-by: Zhizhou Yang --- crosperf/suite_runner.py | 111 +++++++++++++++++++++----------------- crosperf/suite_runner_unittest.py | 107 +++++++++++++++++++----------------- 2 files changed, 120 insertions(+), 98 deletions(-) (limited to 'crosperf') diff --git a/crosperf/suite_runner.py b/crosperf/suite_runner.py index ddca65df..b5649e86 100644 --- a/crosperf/suite_runner.py +++ b/crosperf/suite_runner.py @@ -13,6 +13,8 @@ import os import shlex import time +from contextlib import contextmanager + from cros_utils import command_executer from cros_utils.device_setup_utils import DutWrapper @@ -112,59 +114,68 @@ class SuiteRunner(object): break return ret_tup + @contextmanager + def PauseUI(self, run_on_dut): + """Stop UI before and Start UI after the context block. + + Context manager will make sure UI is always resumed at the end. + """ + run_on_dut.StopUI() + try: + yield + + finally: + run_on_dut.StartUI() + def SetupDevice(self, run_on_dut, cros_machine): - # Stop UI before configuring the DUT. + # Pause UI while configuring the DUT. # This will accelerate setup (waiting for cooldown has x10 drop) # and help to reset a Chrome state left after the previous test. - run_on_dut.StopUI() - - # Unless the user turns on ASLR in the flag, we first disable ASLR - # before running the benchmarks - if not self.enable_aslr: - run_on_dut.DisableASLR() - - # CPU usage setup comes first where we enable/disable cores. - run_on_dut.SetupCpuUsage() - cpu_online_status = run_on_dut.GetCpuOnline() - # List of online cores of type int (core number). - online_cores = [ - core for core, status in cpu_online_status.items() if status - ] - if self.dut_config['cooldown_time']: - # Setup power conservative mode for effective cool down. - # Set ignore status since powersave may no be available - # on all platforms and we are going to handle it. - ret = run_on_dut.SetCpuGovernor('powersave', ignore_status=True) - if ret: - # "powersave" is not available, use "ondemand". - # Still not a fatal error if it fails. - ret = run_on_dut.SetCpuGovernor('ondemand', ignore_status=True) - # TODO(denik): Run comparison test for 'powersave' and 'ondemand' - # on scarlet and kevin64. - # We might have to consider reducing freq manually to the min - # if it helps to reduce waiting time. - wait_time = run_on_dut.WaitCooldown() - cros_machine.AddCooldownWaitTime(wait_time) - - # Setup CPU governor for the benchmark run. - # It overwrites the previous governor settings. - governor = self.dut_config['governor'] - # FIXME(denik): Pass online cores to governor setup. - run_on_dut.SetCpuGovernor(governor, ignore_status=False) - - # Disable Turbo and Setup CPU freq should ALWAYS proceed governor setup - # since governor may change: - # - frequency; - # - turbo/boost. - run_on_dut.DisableTurbo() - run_on_dut.SetupCpuFreq(online_cores) - # FIXME(denik): Currently we are not recovering the previous cpufreq - # settings since we do reboot/setup every time anyway. - # But it may change in the future and then we have to recover the - # settings. - - # DUT setup is done. Start a fresh new shiny UI. - run_on_dut.StartUI() + with self.PauseUI(run_on_dut): + # Unless the user turns on ASLR in the flag, we first disable ASLR + # before running the benchmarks + if not self.enable_aslr: + run_on_dut.DisableASLR() + + # CPU usage setup comes first where we enable/disable cores. + run_on_dut.SetupCpuUsage() + cpu_online_status = run_on_dut.GetCpuOnline() + # List of online cores of type int (core number). + online_cores = [ + core for core, status in cpu_online_status.items() if status + ] + if self.dut_config['cooldown_time']: + # Setup power conservative mode for effective cool down. + # Set ignore status since powersave may no be available + # on all platforms and we are going to handle it. + ret = run_on_dut.SetCpuGovernor('powersave', ignore_status=True) + if ret: + # "powersave" is not available, use "ondemand". + # Still not a fatal error if it fails. + ret = run_on_dut.SetCpuGovernor('ondemand', ignore_status=True) + # TODO(denik): Run comparison test for 'powersave' and 'ondemand' + # on scarlet and kevin64. + # We might have to consider reducing freq manually to the min + # if it helps to reduce waiting time. + wait_time = run_on_dut.WaitCooldown() + cros_machine.AddCooldownWaitTime(wait_time) + + # Setup CPU governor for the benchmark run. + # It overwrites the previous governor settings. + governor = self.dut_config['governor'] + # FIXME(denik): Pass online cores to governor setup. + run_on_dut.SetCpuGovernor(governor, ignore_status=False) + + # Disable Turbo and Setup CPU freq should ALWAYS proceed governor setup + # since governor may change: + # - frequency; + # - turbo/boost. + run_on_dut.DisableTurbo() + run_on_dut.SetupCpuFreq(online_cores) + # FIXME(denik): Currently we are not recovering the previous cpufreq + # settings since we do reboot/setup every time anyway. + # But it may change in the future and then we have to recover the + # settings. def Test_That_Run(self, machine, label, benchmark, test_args, profiler_args): """Run the test_that test..""" diff --git a/crosperf/suite_runner_unittest.py b/crosperf/suite_runner_unittest.py index 42b2392d..5f377d7c 100755 --- a/crosperf/suite_runner_unittest.py +++ b/crosperf/suite_runner_unittest.py @@ -178,36 +178,29 @@ class SuiteRunnerTest(unittest.TestCase): del command, ignore_status return 0, '', '' - DutWrapper.RunCommandOnDut = mock.Mock(return_value=FakeRunner) - DutWrapper.DisableASLR = mock.Mock() - DutWrapper.SetupCpuUsage = mock.Mock() - DutWrapper.SetupCpuFreq = mock.Mock() - DutWrapper.DisableTurbo = mock.Mock() - DutWrapper.SetCpuGovernor = mock.Mock() - DutWrapper.WaitCooldown = mock.Mock(return_value=0) - DutWrapper.GetCpuOnline = mock.Mock(return_value={0: 1, 1: 1, 2: 0}) - self.runner.dut_config['cooldown_time'] = 0 - self.runner.dut_config['governor'] = 'fake_governor' - self.runner.dut_config['cpu_freq_pct'] = 65 - machine = 'fake_machine' + mock_run_on_dut = mock.Mock(spec=DutWrapper) cros_machine = MockCrosMachine(machine, self.mock_label.chromeos_root, self.mock_logger) - mock_run_on_dut = DutWrapper( - self.mock_label.chromeos_root, - machine, - logger=self.mock_logger, - dut_config=self.runner.dut_config) + + mock_run_on_dut.RunCommandOnDut = mock.Mock(return_value=FakeRunner) + mock_run_on_dut.WaitCooldown = mock.Mock(return_value=0) + mock_run_on_dut.GetCpuOnline = mock.Mock(return_value={0: 1, 1: 1, 2: 0}) + self.runner.dut_config['cooldown_time'] = 0 + self.runner.dut_config['governor'] = 'fake_governor' + self.runner.dut_config['cpu_freq_pct'] = 65 self.runner.SetupDevice(mock_run_on_dut, cros_machine) - DutWrapper.SetupCpuUsage.assert_called_once_with() - DutWrapper.SetupCpuFreq.assert_called_once_with([0, 1]) - DutWrapper.GetCpuOnline.assert_called_once_with() - DutWrapper.SetCpuGovernor.assert_called_once_with( + mock_run_on_dut.SetupCpuUsage.assert_called_once_with() + mock_run_on_dut.SetupCpuFreq.assert_called_once_with([0, 1]) + mock_run_on_dut.GetCpuOnline.assert_called_once_with() + mock_run_on_dut.SetCpuGovernor.assert_called_once_with( 'fake_governor', ignore_status=False) - DutWrapper.DisableTurbo.assert_called_once_with() - DutWrapper.WaitCooldown.assert_not_called() + mock_run_on_dut.DisableTurbo.assert_called_once_with() + mock_run_on_dut.StopUI.assert_called_once_with() + mock_run_on_dut.StartUI.assert_called_once_with() + mock_run_on_dut.WaitCooldown.assert_not_called() def test_setup_device_with_cooldown(self): @@ -216,40 +209,58 @@ class SuiteRunnerTest(unittest.TestCase): del command, ignore_status return 0, '', '' - DutWrapper.RunCommandOnDut = mock.Mock(return_value=FakeRunner) - DutWrapper.DisableASLR = mock.Mock() - DutWrapper.DisableTurbo = mock.Mock() - DutWrapper.SetCpuGovernor = mock.Mock() - DutWrapper.SetupCpuUsage = mock.Mock() - DutWrapper.SetupCpuFreq = mock.Mock() - DutWrapper.WaitCooldown = mock.Mock(return_value=0) - DutWrapper.GetCpuOnline = mock.Mock(return_value={0: 0, 1: 1}) + machine = 'fake_machine' + mock_run_on_dut = mock.Mock(spec=DutWrapper) + cros_machine = MockCrosMachine(machine, self.mock_label.chromeos_root, + self.mock_logger) + + mock_run_on_dut.RunCommandOnDut = mock.Mock(return_value=FakeRunner) + mock_run_on_dut.WaitCooldown = mock.Mock(return_value=0) + mock_run_on_dut.GetCpuOnline = mock.Mock(return_value={0: 0, 1: 1}) self.runner.dut_config['cooldown_time'] = 10 self.runner.dut_config['governor'] = 'fake_governor' self.runner.dut_config['cpu_freq_pct'] = 75 + self.runner.SetupDevice(mock_run_on_dut, cros_machine) + + mock_run_on_dut.WaitCooldown.assert_called_once_with() + mock_run_on_dut.DisableASLR.assert_called_once() + mock_run_on_dut.DisableTurbo.assert_called_once_with() + mock_run_on_dut.SetupCpuUsage.assert_called_once_with() + mock_run_on_dut.SetupCpuFreq.assert_called_once_with([1]) + mock_run_on_dut.SetCpuGovernor.assert_called() + mock_run_on_dut.GetCpuOnline.assert_called_once_with() + mock_run_on_dut.StopUI.assert_called_once_with() + mock_run_on_dut.StartUI.assert_called_once_with() + self.assertGreater(mock_run_on_dut.SetCpuGovernor.call_count, 1) + self.assertEqual(mock_run_on_dut.SetCpuGovernor.call_args, + mock.call('fake_governor', ignore_status=False)) + + def test_setup_device_with_exception(self): + """Test SetupDevice with an exception.""" + machine = 'fake_machine' + mock_run_on_dut = mock.Mock(spec=DutWrapper) cros_machine = MockCrosMachine(machine, self.mock_label.chromeos_root, self.mock_logger) - mock_run_on_dut = DutWrapper( - self.mock_label.chromeos_root, - machine, - logger=self.mock_logger, - dut_config=self.runner.dut_config) - self.runner.SetupDevice(mock_run_on_dut, cros_machine) - - DutWrapper.WaitCooldown.assert_called_once_with() - DutWrapper.DisableASLR.assert_called_once() - DutWrapper.DisableTurbo.assert_called_once_with() - DutWrapper.SetupCpuUsage.assert_called_once_with() - DutWrapper.SetupCpuFreq.assert_called_once_with([1]) - DutWrapper.SetCpuGovernor.assert_called() - DutWrapper.GetCpuOnline.assert_called_once_with() - self.assertGreater(DutWrapper.SetCpuGovernor.call_count, 1) - self.assertEqual(DutWrapper.SetCpuGovernor.call_args, - mock.call('fake_governor', ignore_status=False)) + mock_run_on_dut.SetupCpuUsage = mock.Mock(side_effect=RuntimeError()) + + with self.assertRaises(RuntimeError): + self.runner.SetupDevice(mock_run_on_dut, cros_machine) + + # This called injected an exception. + mock_run_on_dut.SetupCpuUsage.assert_called_once_with() + # Calls following the expeption are skipped. + mock_run_on_dut.WaitCooldown.assert_not_called() + mock_run_on_dut.DisableTurbo.assert_not_called() + mock_run_on_dut.SetupCpuFreq.assert_not_called() + mock_run_on_dut.SetCpuGovernor.assert_not_called() + mock_run_on_dut.GetCpuOnline.assert_not_called() + # Check that Stop/Start UI are always called. + mock_run_on_dut.StopUI.assert_called_once_with() + mock_run_on_dut.StartUI.assert_called_once_with() @mock.patch.object(command_executer.CommandExecuter, 'CrosRunCommand') @mock.patch.object(command_executer.CommandExecuter, -- cgit v1.2.3