From bcd46f1ed43762a2584ab36f35250fd33b492839 Mon Sep 17 00:00:00 2001 From: Dmitri Plotnikov Date: Mon, 28 Jun 2021 07:53:10 -0700 Subject: [PATCH] Fix boot time BatteryStats reset failure We need some services to create a battery stats snapshot captured at the time of battery reset. The issue is that if the battery stats reset occurs during boot, before the system is ready, those services may not be prepared. The fix is to postpone battery stats reset until after the system server is ready. Also, we will now reset the stats if battery got partially charged and discharged repeatedly without ever reaching the full charge. This will prevent battery sessions from going on forever and overflowing internal data structures. This CL partially reverts https://cs.android.com/android/_/android/platform/frameworks/base/+/0ed84f12639c4cb7ba6623dc232ae31602827bae Bug: 192252233 Test: run PowerOnOffTest#testPowerOnOff repeatedly Change-Id: Ib486e4caeeac7c933288c3a1055259c05f45c6c8 --- .../android/internal/os/BatteryStatsImpl.java | 42 +++++++++++++++---- .../internal/os/BatteryUsageStatsStore.java | 14 ++++++- .../os/BatteryUsageStatsProviderTest.java | 1 + .../internal/os/BatteryUsageStatsRule.java | 1 + .../os/BatteryUsageStatsStoreTest.java | 2 + .../server/am/ActivityManagerService.java | 1 + .../server/am/BatteryStatsService.java | 12 +++++- 7 files changed, 62 insertions(+), 11 deletions(-) diff --git a/core/java/com/android/internal/os/BatteryStatsImpl.java b/core/java/com/android/internal/os/BatteryStatsImpl.java index dab3e9fa15fa8..be059a1e195ba 100644 --- a/core/java/com/android/internal/os/BatteryStatsImpl.java +++ b/core/java/com/android/internal/os/BatteryStatsImpl.java @@ -191,6 +191,11 @@ public class BatteryStatsImpl extends BatteryStats { @VisibleForTesting public static final int WAKE_LOCK_WEIGHT = 50; + public static final int RESET_REASON_CORRUPT_FILE = 1; + public static final int RESET_REASON_ADB_COMMAND = 2; + public static final int RESET_REASON_FULL_CHARGE = 3; + public static final int RESET_REASON_MEASURED_ENERGY_BUCKETS_CHANGE = 4; + protected Clocks mClocks; private final AtomicFile mStatsFile; @@ -348,8 +353,9 @@ public class BatteryStatsImpl extends BatteryStats { /** * Callback invoked immediately prior to resetting battery stats. + * @param resetReason One of the RESET_REASON_* constants. */ - void prepareForBatteryStatsReset(); + void prepareForBatteryStatsReset(int resetReason); } private BatteryResetListener mBatteryResetListener; @@ -747,6 +753,7 @@ public class BatteryStatsImpl extends BatteryStats { // CPU update, even if we aren't currently running wake locks. boolean mDistributeWakelockCpu; + private boolean mSystemReady; boolean mShuttingDown; final HistoryEventTracker mActiveEvents = new HistoryEventTracker(); @@ -11210,7 +11217,7 @@ public class BatteryStatsImpl extends BatteryStats { long uptimeUs = mSecUptime * 1000; long mSecRealtime = mClocks.elapsedRealtime(); long realtimeUs = mSecRealtime * 1000; - resetAllStatsLocked(mSecUptime, mSecRealtime); + resetAllStatsLocked(mSecUptime, mSecRealtime, RESET_REASON_ADB_COMMAND); mDischargeStartLevel = mHistoryCur.batteryLevel; pullPendingStateUpdatesLocked(); addHistoryRecordLocked(mSecRealtime, mSecUptime); @@ -11239,9 +11246,10 @@ public class BatteryStatsImpl extends BatteryStats { initActiveHistoryEventsLocked(mSecRealtime, mSecUptime); } - private void resetAllStatsLocked(long uptimeMillis, long elapsedRealtimeMillis) { + private void resetAllStatsLocked(long uptimeMillis, long elapsedRealtimeMillis, + int resetReason) { if (mBatteryResetListener != null) { - mBatteryResetListener.prepareForBatteryStatsReset(); + mBatteryResetListener.prepareForBatteryStatsReset(resetReason); } final long uptimeUs = uptimeMillis * 1000; @@ -13477,6 +13485,13 @@ public class BatteryStatsImpl extends BatteryStats { return false; } + /** + * Notifies BatteryStatsImpl that the system server is ready. + */ + public void onSystemReady() { + mSystemReady = true; + } + @GuardedBy("this") protected void setOnBatteryLocked(final long mSecRealtime, final long mSecUptime, final boolean onBattery, final int oldStatus, final int level, final int chargeUah) { @@ -13493,10 +13508,17 @@ public class BatteryStatsImpl extends BatteryStats { // battery was last full, or the level is at 100, or // we have gone through a significant charge (from a very low // level to a now very high level). + // Also, we will reset the stats if battery got partially charged + // and discharged repeatedly without ever reaching the full charge. + // This reset is done in order to prevent stats sessions from going on forever. + // Exceedingly long battery sessions would lead to an overflow of + // data structures such as mWakeupReasonStats. boolean reset = false; - if (!mNoAutoReset && (oldStatus == BatteryManager.BATTERY_STATUS_FULL + if (!mNoAutoReset && mSystemReady + && (oldStatus == BatteryManager.BATTERY_STATUS_FULL || level >= 90 - || (mDischargeCurrentLevel < 20 && level >= 80))) { + || (mDischargeCurrentLevel < 20 && level >= 80) + || getHighDischargeAmountSinceCharge() >= 200)) { Slog.i(TAG, "Resetting battery stats: level=" + level + " status=" + oldStatus + " dischargeLevel=" + mDischargeCurrentLevel + " lowAmount=" + getLowDischargeAmountSinceCharge() @@ -13534,7 +13556,7 @@ public class BatteryStatsImpl extends BatteryStats { }); } doWrite = true; - resetAllStatsLocked(mSecUptime, mSecRealtime); + resetAllStatsLocked(mSecUptime, mSecRealtime, RESET_REASON_FULL_CHARGE); if (chargeUah > 0 && level > 0) { // Only use the reported coulomb charge value if it is supported and reported. mEstimatedBatteryCapacityMah = (int) ((chargeUah / 1000) / (level / 100.0)); @@ -14502,7 +14524,8 @@ public class BatteryStatsImpl extends BatteryStats { ? null : new MeasuredEnergyStats(supportedStandardBuckets, customBucketNames); // Supported power buckets changed since last boot. // Existing data is no longer reliable. - resetAllStatsLocked(SystemClock.uptimeMillis(), SystemClock.elapsedRealtime()); + resetAllStatsLocked(SystemClock.uptimeMillis(), SystemClock.elapsedRealtime(), + RESET_REASON_MEASURED_ENERGY_BUCKETS_CHANGE); } } @@ -14949,7 +14972,8 @@ public class BatteryStatsImpl extends BatteryStats { } } catch (Exception e) { Slog.e(TAG, "Error reading battery statistics", e); - resetAllStatsLocked(SystemClock.uptimeMillis(), SystemClock.elapsedRealtime()); + resetAllStatsLocked(SystemClock.uptimeMillis(), SystemClock.elapsedRealtime(), + RESET_REASON_CORRUPT_FILE); } finally { stats.recycle(); } diff --git a/core/java/com/android/internal/os/BatteryUsageStatsStore.java b/core/java/com/android/internal/os/BatteryUsageStatsStore.java index 5c976025d39dd..fd54b32bd12ff 100644 --- a/core/java/com/android/internal/os/BatteryUsageStatsStore.java +++ b/core/java/com/android/internal/os/BatteryUsageStatsStore.java @@ -69,6 +69,7 @@ public class BatteryUsageStatsStore { private final Context mContext; private final BatteryStatsImpl mBatteryStats; + private boolean mSystemReady; private final File mStoreDir; private final File mLockFile; private final AtomicFile mConfigFile; @@ -95,7 +96,18 @@ public class BatteryUsageStatsStore { mBatteryUsageStatsProvider = new BatteryUsageStatsProvider(mContext, mBatteryStats); } - private void prepareForBatteryStatsReset() { + /** + * Notifies BatteryUsageStatsStore that the system server is ready. + */ + public void onSystemReady() { + mSystemReady = true; + } + + private void prepareForBatteryStatsReset(int resetReason) { + if (resetReason == BatteryStatsImpl.RESET_REASON_CORRUPT_FILE || !mSystemReady) { + return; + } + final List stats = mBatteryUsageStatsProvider.getBatteryUsageStats(BATTERY_USAGE_STATS_QUERY); if (stats.isEmpty()) { diff --git a/core/tests/coretests/src/com/android/internal/os/BatteryUsageStatsProviderTest.java b/core/tests/coretests/src/com/android/internal/os/BatteryUsageStatsProviderTest.java index cbd67c8324f4c..0147cdb186f3c 100644 --- a/core/tests/coretests/src/com/android/internal/os/BatteryUsageStatsProviderTest.java +++ b/core/tests/coretests/src/com/android/internal/os/BatteryUsageStatsProviderTest.java @@ -204,6 +204,7 @@ public class BatteryUsageStatsProviderTest { BatteryUsageStatsStore batteryUsageStatsStore = new BatteryUsageStatsStore(context, batteryStats, new File(context.getCacheDir(), "BatteryUsageStatsProviderTest"), new TestHandler(), Integer.MAX_VALUE); + batteryUsageStatsStore.onSystemReady(); BatteryUsageStatsProvider provider = new BatteryUsageStatsProvider(context, batteryStats, batteryUsageStatsStore); diff --git a/core/tests/coretests/src/com/android/internal/os/BatteryUsageStatsRule.java b/core/tests/coretests/src/com/android/internal/os/BatteryUsageStatsRule.java index 961e85950b293..083090c54619d 100644 --- a/core/tests/coretests/src/com/android/internal/os/BatteryUsageStatsRule.java +++ b/core/tests/coretests/src/com/android/internal/os/BatteryUsageStatsRule.java @@ -66,6 +66,7 @@ public class BatteryUsageStatsRule implements TestRule { mMockClocks.currentTime = currentTime; mBatteryStats = new MockBatteryStatsImpl(mMockClocks); mBatteryStats.setPowerProfile(mPowerProfile); + mBatteryStats.onSystemReady(); } public BatteryUsageStatsRule setAveragePower(String key, double value) { diff --git a/core/tests/coretests/src/com/android/internal/os/BatteryUsageStatsStoreTest.java b/core/tests/coretests/src/com/android/internal/os/BatteryUsageStatsStoreTest.java index 141a9fa30c850..e478cd7765585 100644 --- a/core/tests/coretests/src/com/android/internal/os/BatteryUsageStatsStoreTest.java +++ b/core/tests/coretests/src/com/android/internal/os/BatteryUsageStatsStoreTest.java @@ -59,6 +59,7 @@ public class BatteryUsageStatsStoreTest { mBatteryStats = new MockBatteryStatsImpl(mMockClocks); mBatteryStats.setNoAutoReset(true); mBatteryStats.setPowerProfile(mock(PowerProfile.class)); + mBatteryStats.onSystemReady(); Context context = InstrumentationRegistry.getContext(); @@ -67,6 +68,7 @@ public class BatteryUsageStatsStoreTest { mBatteryUsageStatsStore = new BatteryUsageStatsStore(context, mBatteryStats, mStoreDirectory, new TestHandler(), MAX_BATTERY_STATS_SNAPSHOT_STORAGE_BYTES); + mBatteryUsageStatsStore.onSystemReady(); mBatteryUsageStatsProvider = new BatteryUsageStatsProvider(context, mBatteryStats); } diff --git a/services/core/java/com/android/server/am/ActivityManagerService.java b/services/core/java/com/android/server/am/ActivityManagerService.java index a04edc760dfb0..aa103f45ad756 100644 --- a/services/core/java/com/android/server/am/ActivityManagerService.java +++ b/services/core/java/com/android/server/am/ActivityManagerService.java @@ -7629,6 +7629,7 @@ public class ActivityManagerService extends IActivityManager.Stub t.traceEnd(); t.traceBegin("ActivityManagerStartApps"); + mBatteryStatsService.onSystemReady(); mBatteryStatsService.noteEvent(BatteryStats.HistoryItem.EVENT_USER_RUNNING_START, Integer.toString(currentUserId), currentUserId); mBatteryStatsService.noteEvent(BatteryStats.HistoryItem.EVENT_USER_FOREGROUND_START, diff --git a/services/core/java/com/android/server/am/BatteryStatsService.java b/services/core/java/com/android/server/am/BatteryStatsService.java index 7d9d78969ca97..9f41c8ba96266 100644 --- a/services/core/java/com/android/server/am/BatteryStatsService.java +++ b/services/core/java/com/android/server/am/BatteryStatsService.java @@ -126,7 +126,7 @@ public final class BatteryStatsService extends IBatteryStats.Stub Watchdog.Monitor { static final String TAG = "BatteryStatsService"; static final boolean DBG = false; - private static final boolean BATTERY_USAGE_STORE_ENABLED = false; + private static final boolean BATTERY_USAGE_STORE_ENABLED = true; private static IBatteryStats sService; @@ -398,6 +398,16 @@ public final class BatteryStatsService extends IBatteryStats.Stub registerStatsCallbacks(); } + /** + * Notifies BatteryStatsService that the system server is ready. + */ + public void onSystemReady() { + mStats.onSystemReady(); + if (BATTERY_USAGE_STORE_ENABLED) { + mBatteryUsageStatsStore.onSystemReady(); + } + } + private final class LocalService extends BatteryStatsInternal { @Override public String[] getWifiIfaces() {