arcvm: Enforce cfs_quota again after Pre-ANR condition is handled
For example, consider the following scenario:
1. ARCVM finishes booting.
2. ArcBootPhaseThrottle throttles the instance
3. Pre-ANR happens. ArcPowerThrottle unthrottles the instance
4. Pre-ANR is handled. ArcPowerThrottle throttles the instance
Currently, at #2, the quota is enforced but at #4 it is not because
of #3 (unthrottle). With this CL, the quota is enforced again at #4
by recognizing that the unthrottling at #3 is not by the user's
action.
BUG=b:205762950
TEST=new unit_tests
Change-Id: Ie29b7d38702575c592a90909e6b14e0aa4419aa7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3582887
Reviewed-by: Yury Khmel <khmel@chromium.org>
Commit-Queue: Yusuke Sato <yusukes@chromium.org>
Cr-Commit-Position: refs/heads/main@{#991862}
diff --git a/chrome/browser/ash/arc/instance_throttle/arc_instance_throttle.cc b/chrome/browser/ash/arc/instance_throttle/arc_instance_throttle.cc
index 050b6b3..5c1ad23 100644
--- a/chrome/browser/ash/arc/instance_throttle/arc_instance_throttle.cc
+++ b/chrome/browser/ash/arc/instance_throttle/arc_instance_throttle.cc
@@ -28,6 +28,47 @@
namespace {
+enum class UnthrottlingReason {
+ // Unthrottling ARC to make its boot faster.
+ kFasterBoot = 0,
+ // Unthrottling ARC to prevent ANRs from happening.
+ kPreAnr = 1,
+ // All others.
+ kOther = 2,
+};
+
+// Checks all the |observers| for active ones to find out the reason why the
+// instance is being unthrottled.
+// This function can only be called when the instance is being unthrottled.
+UnthrottlingReason GetUnthrottlingReason(
+ const std::vector<std::unique_ptr<chromeos::ThrottleObserver>>& observers) {
+ std::vector<chromeos::ThrottleObserver*> active_observers;
+
+ // Check which observer(s) are active.
+ for (const auto& observer : observers) {
+ if (observer->active())
+ active_observers.push_back(observer.get());
+ }
+
+ UnthrottlingReason result = UnthrottlingReason::kOther;
+ DCHECK(!active_observers.empty()) << "All observers are inactive";
+
+ for (const auto* active_observer : active_observers) {
+ if (active_observer->name() == kArcBootPhaseThrottleObserverName) {
+ result = UnthrottlingReason::kFasterBoot;
+ } else if (active_observer->name() == kArcPowerThrottleObserverName) {
+ result = UnthrottlingReason::kPreAnr;
+ } else {
+ // If an unknown observer is found, return kOther immediately.
+ return UnthrottlingReason::kOther;
+ }
+ }
+
+ // Note: If both ArcBootPhaseThrottleObserver and ArcPowerThrottleObserver are
+ // active, either kFasterBoot or kPreAnr is returned as a special case.
+ return result;
+}
+
void OnSetArcVmCpuRestriction(
absl::optional<vm_tools::concierge::SetVmCpuRestrictionResponse> response) {
if (!response) {
@@ -208,6 +249,12 @@
: ThrottleService(context),
delegate_(std::make_unique<DefaultDelegateImpl>()),
bridge_(bridge) {
+ // Note: When you add a new observer, consider modifying GetUnthrottlingReason
+ // so that the CPU quota enforcement feature continues to work as intended. In
+ // general, unthrottling that is done automatically without the user's action
+ // might have to be listed in the UnthrottlingReason enum class. See the
+ // comment in ArcInstanceThrottle::ThrottleInstance too.
+
AddObserver(std::make_unique<ArcActiveWindowThrottleObserver>());
AddObserver(std::make_unique<ArcAppLaunchThrottleObserver>());
AddObserver(std::make_unique<ArcBootPhaseThrottleObserver>());
@@ -219,6 +266,7 @@
// This one is controlled by chromeos::ArcPowerControlHandler.
AddObserver(std::make_unique<ash::ThrottleObserver>(
kChromeArcPowerControlPageObserver));
+
StartObservers();
DCHECK(bridge_);
bridge_->power()->AddObserver(this);
@@ -245,16 +293,50 @@
// Check if enforcing quota is possible
bool use_quota = false;
+ if (!should_throttle && !never_enforce_quota_) {
+ // We're unthrottling the instance. Figure out why.
+ switch (GetUnthrottlingReason(observers())) {
+ case UnthrottlingReason::kFasterBoot:
+ DVLOG(2) << "Unthrottling for faster boot. Quota is still applicable.";
+ break;
+ case UnthrottlingReason::kPreAnr:
+ DVLOG(2)
+ << "Unthrottling for preventing ANRs. Quota is still applicable.";
+ break;
+ case UnthrottlingReason::kOther:
+ // ARC is unthrottled by a user action. After such an event, the quota
+ // should never be applied.
+ DVLOG(2) << "Unthrottling because of a user action. Quota is no longer "
+ "applicable.";
+ never_enforce_quota_ = true;
+ break;
+ }
+ // Note on why other throttle observers that don't monitor the user's action
+ // can be classified as |kOther| and can disable quota:
+ //
+ // * ArcSwitchThrottleObserver:
+ // This is for disabling throttling for testing. If the observer gets
+ // activated, the quota shouldn't be applied either.
+ //
+ // * ArcKioskModeThrottleObserver:
+ // If this gets activated, ARC will be used for Kiosk. There's no point in
+ // applying quota since ARC will always be foreground.
+ //
+ // * ArcProvisioningThrottleObserver:
+ // If this gets activated, the provisioning is ongoing. The quota
+ // shouldn't be applied to make provisioning failures less likely to
+ // happen.
+ }
+
const absl::optional<bool>& arc_is_booting =
GetBootObserver()->arc_is_booting();
const bool arc_has_booted = (arc_is_booting && !*arc_is_booting);
const bool is_throttling = (cpu_restriction_state ==
CpuRestrictionState::CPU_RESTRICTION_BACKGROUND);
- if (arc_has_booted && !quota_ever_enforced_ && is_throttling) {
+ if (arc_has_booted && !never_enforce_quota_ && is_throttling) {
// TODO(yusukes): Do not use quota when Android VPN is in use.
use_quota = true;
- quota_ever_enforced_ = true;
DVLOG(2) << "Enforcing cfs_quota";
}
delegate_->SetCpuRestriction(cpu_restriction_state, use_quota);
diff --git a/chrome/browser/ash/arc/instance_throttle/arc_instance_throttle.h b/chrome/browser/ash/arc/instance_throttle/arc_instance_throttle.h
index 40b89a0..b5e3650 100644
--- a/chrome/browser/ash/arc/instance_throttle/arc_instance_throttle.h
+++ b/chrome/browser/ash/arc/instance_throttle/arc_instance_throttle.h
@@ -92,9 +92,9 @@
ArcBootPhaseThrottleObserver* GetBootObserver();
std::unique_ptr<Delegate> delegate_;
- // True if CPU_RESTRICTION_BACKGROUND_WITH_CFS_QUOTA_ENFORCED has ever been
+ // True if CPU_RESTRICTION_BACKGROUND_WITH_CFS_QUOTA_ENFORCED should never be
// used.
- bool quota_ever_enforced_ = false;
+ bool never_enforce_quota_ = false;
// Owned by ArcServiceManager.
ArcBridgeService* const bridge_;
diff --git a/chrome/browser/ash/arc/instance_throttle/arc_instance_throttle_unittest.cc b/chrome/browser/ash/arc/instance_throttle/arc_instance_throttle_unittest.cc
index e1e0dfbad..3c71c66c 100644
--- a/chrome/browser/ash/arc/instance_throttle/arc_instance_throttle_unittest.cc
+++ b/chrome/browser/ash/arc/instance_throttle/arc_instance_throttle_unittest.cc
@@ -24,6 +24,7 @@
#include "base/command_line.h"
#include "chrome/browser/ash/arc/boot_phase_monitor/arc_boot_phase_monitor_bridge.h"
#include "chrome/browser/ash/arc/instance_throttle/arc_boot_phase_throttle_observer.h"
+#include "chrome/browser/ash/arc/instance_throttle/arc_power_throttle_observer.h"
#include "chrome/browser/ash/arc/session/arc_session_manager.h"
#include "chrome/browser/ash/arc/test/test_arc_session_manager.h"
#include "chrome/browser/ash/throttle_observer.h"
@@ -145,10 +146,44 @@
return arc_instance_throttle_;
}
+ // Returns an observer that will be classified as |kOther| in
+ // GetUnthrottlingReason().
ash::ThrottleObserver* GetThrottleObserver() {
const auto& observers = arc_instance_throttle()->observers_for_testing();
DCHECK(!observers.empty());
- return observers[0].get();
+ for (const auto& observer : observers) {
+ // This must be in sync with GetUnthrottlingReason().
+ if (observer->name() == kArcBootPhaseThrottleObserverName)
+ continue;
+ else if (observer->name() == kArcPowerThrottleObserverName)
+ continue;
+ else
+ return observer.get();
+ }
+ NOTREACHED();
+ return nullptr;
+ }
+
+ ash::ThrottleObserver* GetArcBootPhaseThrottleObserver() {
+ const auto& observers = arc_instance_throttle()->observers_for_testing();
+ DCHECK(!observers.empty());
+ for (const auto& observer : observers) {
+ if (observer->name() == kArcBootPhaseThrottleObserverName)
+ return observer.get();
+ }
+ NOTREACHED();
+ return nullptr;
+ }
+
+ ash::ThrottleObserver* GetArcPowerThrottleObserver() {
+ const auto& observers = arc_instance_throttle()->observers_for_testing();
+ DCHECK(!observers.empty());
+ for (const auto& observer : observers) {
+ if (observer->name() == kArcPowerThrottleObserverName)
+ return observer.get();
+ }
+ NOTREACHED();
+ return nullptr;
}
FakePowerInstance* power_instance() { return power_instance_.get(); }
@@ -235,16 +270,18 @@
EXPECT_EQ(1U, disable_cpu_restriction_counter());
}
-// Tests that ArcInstanceThrottle enforces quota only once after ARC
-// boot.
+// Tests that ArcInstanceThrottle enforces quota only until unthrottled via a
+// user action.
TEST_F(ArcInstanceThrottleTest, TestThrottleInstanceQuotaEnforcement) {
// While ARC is booting, quota shouldn't be enforced.
- GetThrottleObserver()->SetActive(false);
+ GetArcBootPhaseThrottleObserver()->SetActive(false);
EXPECT_EQ(1U, enable_cpu_restriction_counter());
EXPECT_EQ(0U, use_quota_counter());
EXPECT_EQ(0U, disable_cpu_restriction_counter());
- GetThrottleObserver()->SetActive(true);
+ // Note: Use ArcBootPhaseThrottleObserver so that quota will still be
+ // applicable.
+ GetArcBootPhaseThrottleObserver()->SetActive(true);
EXPECT_EQ(1U, enable_cpu_restriction_counter());
EXPECT_EQ(0U, use_quota_counter());
EXPECT_EQ(1U, disable_cpu_restriction_counter());
@@ -254,12 +291,12 @@
// Since 10 seconds haven't passed since the mojom connection, quota is
// still not enforced.
- GetThrottleObserver()->SetActive(false);
+ GetArcBootPhaseThrottleObserver()->SetActive(false);
EXPECT_EQ(2U, enable_cpu_restriction_counter());
EXPECT_EQ(0U, use_quota_counter());
EXPECT_EQ(1U, disable_cpu_restriction_counter());
- GetThrottleObserver()->SetActive(true);
+ GetArcBootPhaseThrottleObserver()->SetActive(true);
EXPECT_EQ(2U, enable_cpu_restriction_counter());
EXPECT_EQ(0U, use_quota_counter());
EXPECT_EQ(2U, disable_cpu_restriction_counter());
@@ -269,21 +306,33 @@
ArcBootPhaseThrottleObserver::GetThrottleDelayForTesting());
// Now quota is enforced,
- GetThrottleObserver()->SetActive(false);
+ GetArcBootPhaseThrottleObserver()->SetActive(false);
EXPECT_EQ(3U, enable_cpu_restriction_counter());
EXPECT_EQ(1U, use_quota_counter());
EXPECT_EQ(2U, disable_cpu_restriction_counter());
- // Quota is enforced only once.
- GetThrottleObserver()->SetActive(true);
+ // Unthrottle the instance because of pre-ANR.
+ GetArcPowerThrottleObserver()->SetActive(true);
EXPECT_EQ(3U, enable_cpu_restriction_counter());
EXPECT_EQ(1U, use_quota_counter());
EXPECT_EQ(3U, disable_cpu_restriction_counter());
- GetThrottleObserver()->SetActive(false);
+ // Quota is enforced again once pre-ANR is handled.
+ GetArcPowerThrottleObserver()->SetActive(false);
EXPECT_EQ(4U, enable_cpu_restriction_counter());
- EXPECT_EQ(1U, use_quota_counter());
+ EXPECT_EQ(2U, use_quota_counter());
EXPECT_EQ(3U, disable_cpu_restriction_counter());
+
+ // Quota is not enforced once unthrottled via a user action.
+ GetThrottleObserver()->SetActive(true);
+ EXPECT_EQ(4U, enable_cpu_restriction_counter());
+ EXPECT_EQ(2U, use_quota_counter());
+ EXPECT_EQ(4U, disable_cpu_restriction_counter());
+
+ GetThrottleObserver()->SetActive(false);
+ EXPECT_EQ(5U, enable_cpu_restriction_counter());
+ EXPECT_EQ(2U, use_quota_counter());
+ EXPECT_EQ(4U, disable_cpu_restriction_counter());
}
// Tests that power instance is correctly notified.
diff --git a/chrome/browser/ash/arc/instance_throttle/arc_power_throttle_observer.cc b/chrome/browser/ash/arc/instance_throttle/arc_power_throttle_observer.cc
index 903fb2d..fefc7c9 100644
--- a/chrome/browser/ash/arc/instance_throttle/arc_power_throttle_observer.cc
+++ b/chrome/browser/ash/arc/instance_throttle/arc_power_throttle_observer.cc
@@ -17,7 +17,7 @@
} // namespace
ArcPowerThrottleObserver::ArcPowerThrottleObserver()
- : ThrottleObserver("ArcPower") {}
+ : ThrottleObserver(kArcPowerThrottleObserverName) {}
ArcPowerThrottleObserver::~ArcPowerThrottleObserver() = default;
diff --git a/chrome/browser/ash/arc/instance_throttle/arc_power_throttle_observer.h b/chrome/browser/ash/arc/instance_throttle/arc_power_throttle_observer.h
index 0765559..89d0a09a 100644
--- a/chrome/browser/ash/arc/instance_throttle/arc_power_throttle_observer.h
+++ b/chrome/browser/ash/arc/instance_throttle/arc_power_throttle_observer.h
@@ -11,6 +11,8 @@
namespace arc {
+constexpr char kArcPowerThrottleObserverName[] = "ArcPower";
+
// Listens ARC power events and lifts CPU throttling when needed.
class ArcPowerThrottleObserver : public chromeos::ThrottleObserver,
public ArcPowerBridge::Observer {
