From 0419d437c75de94d4da3dd1eecbc38a1ed8d10ea Mon Sep 17 00:00:00 2001 From: Connor O'Brien Date: Thu, 6 May 2021 18:44:31 -0700 Subject: [PATCH] Don't remove UID data in readKernelCpuUid*TimesLocked In both of these cases (removed isolated UID and nonexistent user) the affected UID has already previously been enqueued in mPendingRemovedUids and so all underlying cpu time data will be cleared together after 5 minutes. Clearing the different sets of times separately is buggy due to creating inconsistency among the KernelCpuUid*TimeReaders. Removing data in these functions also defeats the 5-minute delay that was intended to give statsd a chance to read the data for removed UIDs. Also update BatteryStatsCpuTimesTest to reflect the new behavior Bug: 182272121 Test: atest com.android.internal.os.BatteryStatsTests shows no new failures Change-Id: I5719aecd5603cd9cecfbf75cede5eceeda8d0f5b Signed-off-by: Connor O'Brien --- .../android/internal/os/BatteryStatsImpl.java | 20 ------------------- .../internal/os/BatteryStatsCpuTimesTest.java | 6 ------ 2 files changed, 26 deletions(-) diff --git a/core/java/com/android/internal/os/BatteryStatsImpl.java b/core/java/com/android/internal/os/BatteryStatsImpl.java index db67bab25e786..012d4a0c6215a 100644 --- a/core/java/com/android/internal/os/BatteryStatsImpl.java +++ b/core/java/com/android/internal/os/BatteryStatsImpl.java @@ -13091,13 +13091,11 @@ public class BatteryStatsImpl extends BatteryStats { if (Process.isIsolated(uid)) { // This could happen if the isolated uid mapping was removed before that process // was actually killed. - mCpuUidUserSysTimeReader.removeUid(uid); if (DEBUG) Slog.d(TAG, "Got readings for an isolated uid: " + uid); return; } if (!mUserInfoProvider.exists(UserHandle.getUserId(uid))) { if (DEBUG) Slog.d(TAG, "Got readings for an invalid user's uid " + uid); - mCpuUidUserSysTimeReader.removeUid(uid); return; } final Uid u = getUidStatsLocked(uid, elapsedRealtimeMs, startTimeMs); @@ -13202,19 +13200,16 @@ public class BatteryStatsImpl extends BatteryStats { mWakeLockAllocationsUs = null; final long startTimeMs = mClocks.uptimeMillis(); final long elapsedRealtimeMs = mClocks.elapsedRealtime(); - final List uidsToRemove = new ArrayList<>(); // If power is being accumulated for attribution, data needs to be read immediately. final boolean forceRead = powerAccumulator != null; mCpuUidFreqTimeReader.readDelta(forceRead, (uid, cpuFreqTimeMs) -> { uid = mapUid(uid); if (Process.isIsolated(uid)) { - uidsToRemove.add(uid); if (DEBUG) Slog.d(TAG, "Got freq readings for an isolated uid: " + uid); return; } if (!mUserInfoProvider.exists(UserHandle.getUserId(uid))) { if (DEBUG) Slog.d(TAG, "Got freq readings for an invalid user's uid " + uid); - uidsToRemove.add(uid); return; } final Uid u = getUidStatsLocked(uid, elapsedRealtimeMs, startTimeMs); @@ -13278,9 +13273,6 @@ public class BatteryStatsImpl extends BatteryStats { } } }); - for (int uid : uidsToRemove) { - mCpuUidFreqTimeReader.removeUid(uid); - } final long elapsedTimeMs = mClocks.uptimeMillis() - startTimeMs; if (DEBUG_ENERGY_CPU || elapsedTimeMs >= 100) { @@ -13332,25 +13324,19 @@ public class BatteryStatsImpl extends BatteryStats { public void readKernelUidCpuActiveTimesLocked(boolean onBattery) { final long startTimeMs = mClocks.uptimeMillis(); final long elapsedRealtimeMs = mClocks.elapsedRealtime(); - final List uidsToRemove = new ArrayList<>(); mCpuUidActiveTimeReader.readDelta(false, (uid, cpuActiveTimesMs) -> { uid = mapUid(uid); if (Process.isIsolated(uid)) { - uidsToRemove.add(uid); if (DEBUG) Slog.w(TAG, "Got active times for an isolated uid: " + uid); return; } if (!mUserInfoProvider.exists(UserHandle.getUserId(uid))) { if (DEBUG) Slog.w(TAG, "Got active times for an invalid user's uid " + uid); - uidsToRemove.add(uid); return; } final Uid u = getUidStatsLocked(uid, elapsedRealtimeMs, startTimeMs); u.mCpuActiveTimeMs.addCountLocked(cpuActiveTimesMs, onBattery); }); - for (int uid : uidsToRemove) { - mCpuUidActiveTimeReader.removeUid(uid); - } final long elapsedTimeMs = mClocks.uptimeMillis() - startTimeMs; if (DEBUG_ENERGY_CPU || elapsedTimeMs >= 100) { @@ -13371,19 +13357,16 @@ public class BatteryStatsImpl extends BatteryStats { @Nullable CpuDeltaPowerAccumulator powerAccumulator) { final long startTimeMs = mClocks.uptimeMillis(); final long elapsedRealtimeMs = mClocks.elapsedRealtime(); - final List uidsToRemove = new ArrayList<>(); // If power is being accumulated for attribution, data needs to be read immediately. final boolean forceRead = powerAccumulator != null; mCpuUidClusterTimeReader.readDelta(forceRead, (uid, cpuClusterTimesMs) -> { uid = mapUid(uid); if (Process.isIsolated(uid)) { - uidsToRemove.add(uid); if (DEBUG) Slog.w(TAG, "Got cluster times for an isolated uid: " + uid); return; } if (!mUserInfoProvider.exists(UserHandle.getUserId(uid))) { if (DEBUG) Slog.w(TAG, "Got cluster times for an invalid user's uid " + uid); - uidsToRemove.add(uid); return; } final Uid u = getUidStatsLocked(uid, elapsedRealtimeMs, startTimeMs); @@ -13393,9 +13376,6 @@ public class BatteryStatsImpl extends BatteryStats { powerAccumulator.addCpuClusterDurationsMs(u, cpuClusterTimesMs); } }); - for (int uid : uidsToRemove) { - mCpuUidClusterTimeReader.removeUid(uid); - } final long elapsedTimeMs = mClocks.uptimeMillis() - startTimeMs; if (DEBUG_ENERGY_CPU || elapsedTimeMs >= 100) { diff --git a/core/tests/coretests/src/com/android/internal/os/BatteryStatsCpuTimesTest.java b/core/tests/coretests/src/com/android/internal/os/BatteryStatsCpuTimesTest.java index 623e77e19ac83..54d8701c5a6e3 100644 --- a/core/tests/coretests/src/com/android/internal/os/BatteryStatsCpuTimesTest.java +++ b/core/tests/coretests/src/com/android/internal/os/BatteryStatsCpuTimesTest.java @@ -363,7 +363,6 @@ public class BatteryStatsCpuTimesTest { assertEquals("Unexpected system cpu time for uid=" + testUids[i], uidTimesUs[i][1], u.getSystemCpuTimeUs(STATS_SINCE_CHARGED)); } - verify(mCpuUidUserSysTimeReader).removeUid(isolatedUid); // Add an isolated uid mapping and repeat the test. @@ -453,7 +452,6 @@ public class BatteryStatsCpuTimesTest { } assertNull("There shouldn't be an entry for invalid uid=" + invalidUid, mBatteryStatsImpl.getUidStats().get(invalidUid)); - verify(mCpuUidUserSysTimeReader).removeUid(invalidUid); } @Test @@ -943,7 +941,6 @@ public class BatteryStatsCpuTimesTest { assertNull("Unexpected screen-off cpu times for uid=" + testUids[i], u.getScreenOffCpuFreqTimes(STATS_SINCE_CHARGED)); } - verify(mCpuUidFreqTimeReader).removeUid(isolatedUid); // Add an isolated uid mapping and repeat the test. @@ -1038,7 +1035,6 @@ public class BatteryStatsCpuTimesTest { } assertNull("There shouldn't be an entry for invalid uid=" + invalidUid, mBatteryStatsImpl.getUidStats().get(invalidUid)); - verify(mCpuUidFreqTimeReader).removeUid(invalidUid); } @Test @@ -1142,7 +1138,6 @@ public class BatteryStatsCpuTimesTest { } assertNull("There shouldn't be an entry for invalid uid=" + invalidUid, mBatteryStatsImpl.getUidStats().get(invalidUid)); - verify(mCpuUidActiveTimeReader).removeUid(invalidUid); } @Test @@ -1259,7 +1254,6 @@ public class BatteryStatsCpuTimesTest { } assertNull("There shouldn't be an entry for invalid uid=" + invalidUid, mBatteryStatsImpl.getUidStats().get(invalidUid)); - verify(mCpuUidClusterTimeReader).removeUid(invalidUid); } @Test