CPULimiter: Refactor class to manage the CPU limitation.
This new class replaces the functionality embedded in UpdateAttempter
that limits the max CPU usage allowed by update_engine. This refactor
helps reusing this class outside of the brillo UpdateAttempter.
Bug: None
TEST=FEATURES=test emerge-link update_engine
Change-Id: Ib5487d314846b497a44bb78a3b94609571e0fe38
diff --git a/Android.mk b/Android.mk
index 04ecfe5..fb5af1b 100644
--- a/Android.mk
+++ b/Android.mk
@@ -152,6 +152,7 @@
common/certificate_checker.cc \
common/clock.cc \
common/constants.cc \
+ common/cpu_limiter.cc \
common/hash_calculator.cc \
common/http_common.cc \
common/http_fetcher.cc \
diff --git a/common/cpu_limiter.cc b/common/cpu_limiter.cc
new file mode 100644
index 0000000..67c50b6
--- /dev/null
+++ b/common/cpu_limiter.cc
@@ -0,0 +1,88 @@
+//
+// Copyright (C) 2016 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+//
+
+#include "update_engine/common/cpu_limiter.h"
+
+#include <string>
+
+#include <base/bind.h>
+#include <base/logging.h>
+#include <base/strings/string_number_conversions.h>
+#include <base/time/time.h>
+
+#include "update_engine/common/utils.h"
+
+namespace {
+
+// Cgroup container is created in update-engine's upstart script located at
+// /etc/init/update-engine.conf.
+const char kCGroupSharesPath[] = "/sys/fs/cgroup/cpu/update-engine/cpu.shares";
+
+} // namespace
+
+namespace chromeos_update_engine {
+
+CPULimiter::~CPULimiter() {
+ // Set everything back to normal on destruction.
+ CPULimiter::SetCpuShares(CpuShares::kNormal);
+}
+
+void CPULimiter::StartLimiter() {
+ if (manage_shares_id_ != brillo::MessageLoop::kTaskIdNull) {
+ LOG(ERROR) << "Cpu shares timeout source hasn't been destroyed.";
+ StopLimiter();
+ }
+ manage_shares_id_ = brillo::MessageLoop::current()->PostDelayedTask(
+ FROM_HERE,
+ base::Bind(&CPULimiter::StopLimiterCallback, base::Unretained(this)),
+ base::TimeDelta::FromHours(2));
+ SetCpuShares(CpuShares::kLow);
+}
+
+void CPULimiter::StopLimiter() {
+ if (manage_shares_id_ != brillo::MessageLoop::kTaskIdNull) {
+ // If the shares were never set and there isn't a message loop instance,
+ // we avoid calling CancelTask(), which otherwise would have been a no-op.
+ brillo::MessageLoop::current()->CancelTask(manage_shares_id_);
+ manage_shares_id_ = brillo::MessageLoop::kTaskIdNull;
+ }
+ SetCpuShares(CpuShares::kNormal);
+}
+
+bool CPULimiter::SetCpuShares(CpuShares shares) {
+ // Short-circuit to avoid re-setting the shares.
+ if (shares_ == shares)
+ return true;
+
+ std::string string_shares = base::IntToString(static_cast<int>(shares));
+ LOG(INFO) << "Setting cgroup cpu shares to " << string_shares;
+ if (!utils::WriteFile(
+ kCGroupSharesPath, string_shares.c_str(), string_shares.size())) {
+ LOG(ERROR) << "Failed to change cgroup cpu shares to " << string_shares
+ << " using " << kCGroupSharesPath;
+ return false;
+ }
+ shares_ = shares;
+ LOG(INFO) << "CPU shares = " << shares_;
+ return true;
+}
+
+void CPULimiter::StopLimiterCallback() {
+ SetCpuShares(CpuShares::kNormal);
+ manage_shares_id_ = brillo::MessageLoop::kTaskIdNull;
+}
+
+} // namespace chromeos_update_engine
diff --git a/common/cpu_limiter.h b/common/cpu_limiter.h
new file mode 100644
index 0000000..c7add89
--- /dev/null
+++ b/common/cpu_limiter.h
@@ -0,0 +1,67 @@
+//
+// Copyright (C) 2016 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+//
+
+#ifndef UPDATE_ENGINE_COMMON_CPU_LIMITER_H_
+#define UPDATE_ENGINE_COMMON_CPU_LIMITER_H_
+
+#include <brillo/message_loops/message_loop.h>
+
+namespace chromeos_update_engine {
+
+// Cgroups cpu shares constants. 1024 is the default shares a standard process
+// gets and 2 is the minimum value. We set High as a value that gives the
+// update-engine 2x the cpu share of a standard process.
+enum class CpuShares : int {
+ kHigh = 2048,
+ kNormal = 1024,
+ kLow = 2,
+};
+
+// Sets the current process shares to |shares|. Returns true on
+// success, false otherwise.
+bool SetCpuShares(CpuShares shares);
+
+class CPULimiter {
+ public:
+ CPULimiter() = default;
+ ~CPULimiter();
+
+ // Sets the cpu shares to low and sets up timeout events to stop the limiter.
+ void StartLimiter();
+
+ // Resets the cpu shares to normal and destroys any scheduled timeout sources.
+ void StopLimiter();
+
+ // Sets the cpu shares to |shares|. This method can be user at any time, but
+ // if the limiter is not running, the shares won't be reset to normal.
+ bool SetCpuShares(CpuShares shares);
+
+ private:
+ // The cpu shares timeout source callback sets the current cpu shares to
+ // normal.
+ void StopLimiterCallback();
+
+ // Current cpu shares.
+ CpuShares shares_ = CpuShares::kNormal;
+
+ // The cpu shares management timeout task id.
+ brillo::MessageLoop::TaskId manage_shares_id_{
+ brillo::MessageLoop::kTaskIdNull};
+};
+
+} // namespace chromeos_update_engine
+
+#endif // UPDATE_ENGINE_COMMON_CPU_LIMITER_H_
diff --git a/common/cpu_limiter_unittest.cc b/common/cpu_limiter_unittest.cc
new file mode 100644
index 0000000..d549b4c
--- /dev/null
+++ b/common/cpu_limiter_unittest.cc
@@ -0,0 +1,42 @@
+//
+// Copyright (C) 2012 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+//
+
+#include "update_engine/common/cpu_limiter.h"
+
+#include <gtest/gtest.h>
+
+namespace chromeos_update_engine {
+
+class CPULimiterTest : public ::testing::Test {};
+
+namespace {
+// Compares cpu shares and returns an integer that is less
+// than, equal to or greater than 0 if |shares_lhs| is,
+// respectively, lower than, same as or higher than |shares_rhs|.
+int CompareCpuShares(CpuShares shares_lhs, CpuShares shares_rhs) {
+ return static_cast<int>(shares_lhs) - static_cast<int>(shares_rhs);
+}
+} // namespace
+
+// Tests the CPU shares enum is in the order we expect it.
+TEST(CPULimiterTest, CompareCpuSharesTest) {
+ EXPECT_LT(CompareCpuShares(CpuShares::kLow, CpuShares::kNormal), 0);
+ EXPECT_GT(CompareCpuShares(CpuShares::kNormal, CpuShares::kLow), 0);
+ EXPECT_EQ(CompareCpuShares(CpuShares::kNormal, CpuShares::kNormal), 0);
+ EXPECT_GT(CompareCpuShares(CpuShares::kHigh, CpuShares::kNormal), 0);
+}
+
+} // namespace chromeos_update_engine
diff --git a/common/utils.cc b/common/utils.cc
index d3b5baa..41a75f8 100644
--- a/common/utils.cc
+++ b/common/utils.cc
@@ -157,10 +157,6 @@
namespace utils {
-// Cgroup container is created in update-engine's upstart script located at
-// /etc/init/update-engine.conf.
-static const char kCGroupDir[] = "/sys/fs/cgroup/cpu/update-engine";
-
string ParseECVersion(string input_line) {
base::TrimWhitespaceASCII(input_line, base::TRIM_ALL, &input_line);
@@ -885,20 +881,6 @@
base::Bind(&TriggerCrashReporterUpload));
}
-bool SetCpuShares(CpuShares shares) {
- string string_shares = base::IntToString(static_cast<int>(shares));
- string cpu_shares_file = string(utils::kCGroupDir) + "/cpu.shares";
- LOG(INFO) << "Setting cgroup cpu shares to " << string_shares;
- if (utils::WriteFile(cpu_shares_file.c_str(), string_shares.c_str(),
- string_shares.size())) {
- return true;
- } else {
- LOG(ERROR) << "Failed to change cgroup cpu shares to "<< string_shares
- << " using " << cpu_shares_file;
- return false;
- }
-}
-
int FuzzInt(int value, unsigned int range) {
int min = value - range / 2;
int max = value + range - range / 2;
diff --git a/common/utils.h b/common/utils.h
index 0440dd8..ecb7fb9 100644
--- a/common/utils.h
+++ b/common/utils.h
@@ -277,19 +277,6 @@
}
}
-// Cgroups cpu shares constants. 1024 is the default shares a standard process
-// gets and 2 is the minimum value. We set High as a value that gives the
-// update-engine 2x the cpu share of a standard process.
-enum CpuShares {
- kCpuSharesHigh = 2048,
- kCpuSharesNormal = 1024,
- kCpuSharesLow = 2,
-};
-
-// Sets the current process shares to |shares|. Returns true on
-// success, false otherwise.
-bool SetCpuShares(CpuShares shares);
-
// Converts seconds into human readable notation including days, hours, minutes
// and seconds. For example, 185 will yield 3m5s, 4300 will yield 1h11m40s, and
// 360000 will yield 4d4h0m0s. Zero padding not applied. Seconds are always
diff --git a/common/utils_unittest.cc b/common/utils_unittest.cc
index 02f919e..fde89e8 100644
--- a/common/utils_unittest.cc
+++ b/common/utils_unittest.cc
@@ -180,28 +180,6 @@
utils::MakePartitionNameForMount("/dev/ubiblock1"));
}
-namespace {
-// Compares cpu shares and returns an integer that is less
-// than, equal to or greater than 0 if |shares_lhs| is,
-// respectively, lower than, same as or higher than |shares_rhs|.
-int CompareCpuShares(utils::CpuShares shares_lhs,
- utils::CpuShares shares_rhs) {
- return static_cast<int>(shares_lhs) - static_cast<int>(shares_rhs);
-}
-} // namespace
-
-// Tests the CPU shares enum is in the order we expect it.
-TEST(UtilsTest, CompareCpuSharesTest) {
- EXPECT_LT(CompareCpuShares(utils::kCpuSharesLow,
- utils::kCpuSharesNormal), 0);
- EXPECT_GT(CompareCpuShares(utils::kCpuSharesNormal,
- utils::kCpuSharesLow), 0);
- EXPECT_EQ(CompareCpuShares(utils::kCpuSharesNormal,
- utils::kCpuSharesNormal), 0);
- EXPECT_GT(CompareCpuShares(utils::kCpuSharesHigh,
- utils::kCpuSharesNormal), 0);
-}
-
TEST(UtilsTest, FuzzIntTest) {
static const unsigned int kRanges[] = { 0, 1, 2, 20 };
for (unsigned int range : kRanges) {
diff --git a/update_attempter.cc b/update_attempter.cc
index 4e0b997..b826121 100644
--- a/update_attempter.cc
+++ b/update_attempter.cc
@@ -133,7 +133,6 @@
// CertificateChecker might not be initialized in unittests.
if (cert_checker_)
cert_checker_->SetObserver(nullptr);
- CleanupCpuSharesManagement();
// Release ourselves as the ActionProcessor's delegate to prevent
// re-scheduling the updates due to the processing stopped.
processor_->set_delegate(nullptr);
@@ -910,7 +909,7 @@
actions_.clear();
// Reset cpu shares back to normal.
- CleanupCpuSharesManagement();
+ cpu_limiter_.StopLimiter();
if (status_ == UpdateStatus::REPORTING_ERROR_EVENT) {
LOG(INFO) << "Error event sent.";
@@ -987,7 +986,7 @@
void UpdateAttempter::ProcessingStopped(const ActionProcessor* processor) {
// Reset cpu shares back to normal.
- CleanupCpuSharesManagement();
+ cpu_limiter_.StopLimiter();
download_progress_ = 0.0;
SetStatusAndNotify(UpdateStatus::IDLE);
ScheduleUpdates();
@@ -1053,7 +1052,7 @@
new_version_ = plan.version;
new_payload_size_ = plan.payload_size;
SetupDownload();
- SetupCpuSharesManagement();
+ cpu_limiter_.StartLimiter();
SetStatusAndNotify(UpdateStatus::UPDATE_AVAILABLE);
} else if (type == DownloadAction::StaticType()) {
SetStatusAndNotify(UpdateStatus::FINALIZING);
@@ -1340,41 +1339,6 @@
return true;
}
-void UpdateAttempter::SetCpuShares(utils::CpuShares shares) {
- if (shares_ == shares) {
- return;
- }
- if (utils::SetCpuShares(shares)) {
- shares_ = shares;
- LOG(INFO) << "CPU shares = " << shares_;
- }
-}
-
-void UpdateAttempter::SetupCpuSharesManagement() {
- if (manage_shares_id_ != MessageLoop::kTaskIdNull) {
- LOG(ERROR) << "Cpu shares timeout source hasn't been destroyed.";
- CleanupCpuSharesManagement();
- }
- const int kCpuSharesTimeout = 2 * 60 * 60; // 2 hours
- manage_shares_id_ = MessageLoop::current()->PostDelayedTask(
- FROM_HERE,
- Bind(&UpdateAttempter::ManageCpuSharesCallback, base::Unretained(this)),
- TimeDelta::FromSeconds(kCpuSharesTimeout));
- SetCpuShares(utils::kCpuSharesLow);
-}
-
-void UpdateAttempter::CleanupCpuSharesManagement() {
- if (manage_shares_id_ != MessageLoop::kTaskIdNull) {
- // The UpdateAttempter is instantiated by default by the FakeSystemState,
- // even when it is not used. We check the manage_shares_id_ before calling
- // the MessageLoop::current() since the unit test using a FakeSystemState
- // may have not define a MessageLoop for the current thread.
- MessageLoop::current()->CancelTask(manage_shares_id_);
- manage_shares_id_ = MessageLoop::kTaskIdNull;
- }
- SetCpuShares(utils::kCpuSharesNormal);
-}
-
void UpdateAttempter::ScheduleProcessingStart() {
LOG(INFO) << "Scheduling an action processor start.";
start_action_processor_ = false;
@@ -1383,11 +1347,6 @@
Bind([this] { this->processor_->StartProcessing(); }));
}
-void UpdateAttempter::ManageCpuSharesCallback() {
- SetCpuShares(utils::kCpuSharesNormal);
- manage_shares_id_ = MessageLoop::kTaskIdNull;
-}
-
void UpdateAttempter::DisableDeltaUpdateIfNeeded() {
int64_t delta_failures;
if (omaha_request_params_->delta_okay() &&
diff --git a/update_attempter.h b/update_attempter.h
index 2f53869..8f6fd18 100644
--- a/update_attempter.h
+++ b/update_attempter.h
@@ -34,6 +34,7 @@
#include "update_engine/client_library/include/update_engine/update_status.h"
#include "update_engine/common/action_processor.h"
#include "update_engine/common/certificate_checker.h"
+#include "update_engine/common/cpu_limiter.h"
#include "update_engine/libcros_proxy.h"
#include "update_engine/omaha_request_params.h"
#include "update_engine/omaha_response_handler_action.h"
@@ -292,22 +293,6 @@
// otherwise.
bool ScheduleErrorEventAction();
- // Sets the cpu shares to |shares| and updates |shares_| if the new
- // |shares| is different than the current |shares_|, otherwise simply
- // returns.
- void SetCpuShares(utils::CpuShares shares);
-
- // Sets the cpu shares to low and sets up timeout events to increase it.
- void SetupCpuSharesManagement();
-
- // Resets the cpu shares to normal and destroys any scheduled timeout
- // sources.
- void CleanupCpuSharesManagement();
-
- // The cpu shares timeout source callback sets the current cpu shares to
- // normal.
- void ManageCpuSharesCallback();
-
// Schedules an event loop callback to start the action processor. This is
// scheduled asynchronously to unblock the event loop.
void ScheduleProcessingStart();
@@ -446,12 +431,8 @@
// HTTP server response code from the last HTTP request action.
int http_response_code_ = 0;
- // Current cpu shares.
- utils::CpuShares shares_ = utils::kCpuSharesNormal;
-
- // The cpu shares management timeout task id.
- brillo::MessageLoop::TaskId manage_shares_id_{
- brillo::MessageLoop::kTaskIdNull};
+ // CPU limiter during the update.
+ CPULimiter cpu_limiter_;
// For status:
UpdateStatus status_{UpdateStatus::IDLE};
diff --git a/update_attempter_unittest.cc b/update_attempter_unittest.cc
index 10fb352..6703a96 100644
--- a/update_attempter_unittest.cc
+++ b/update_attempter_unittest.cc
@@ -134,11 +134,8 @@
void SetUp() override {
CHECK(utils::MakeTempDirectory("UpdateAttempterTest-XXXXXX", &test_dir_));
- EXPECT_EQ(nullptr, attempter_.dbus_adaptor_);
EXPECT_NE(nullptr, attempter_.system_state_);
EXPECT_EQ(0, attempter_.http_response_code_);
- EXPECT_EQ(utils::kCpuSharesNormal, attempter_.shares_);
- EXPECT_EQ(MessageLoop::kTaskIdNull, attempter_.manage_shares_id_);
EXPECT_EQ(UpdateStatus::IDLE, attempter_.status_);
EXPECT_EQ(0.0, attempter_.download_progress_);
EXPECT_EQ(0, attempter_.last_checked_time_);
diff --git a/update_engine.gyp b/update_engine.gyp
index 0a40a51..7b0597a 100644
--- a/update_engine.gyp
+++ b/update_engine.gyp
@@ -160,6 +160,7 @@
'common/certificate_checker.cc',
'common/clock.cc',
'common/constants.cc',
+ 'common/cpu_limiter.cc',
'common/hash_calculator.cc',
'common/http_common.cc',
'common/http_fetcher.cc',
@@ -470,6 +471,7 @@
'common/action_processor_unittest.cc',
'common/action_unittest.cc',
'common/certificate_checker_unittest.cc',
+ 'common/cpu_limiter_unittest.cc',
'common/fake_prefs.cc',
'common/hash_calculator_unittest.cc',
'common/http_fetcher_unittest.cc',