From ece4ef73939dc975d7be309eead2a6b736518e47 Mon Sep 17 00:00:00 2001 From: Dmitri Plotnikov Date: Thu, 15 Jul 2021 13:46:22 -0700 Subject: [PATCH] Fix concurrency of BatteryStats cleanup after user removal Bug: 193742542 Bug: 180015146 Test: atest --rerun-until-failure 20 FrameworksCoreTests:BatteryStatsUserLifecycleTests#testNoCpuDataForRemovedUser Change-Id: If45cbf0be6c29fc99771053c6a0ec34c60e293a8 --- .../android/internal/os/BatteryStatsImpl.java | 107 ++++++++++++------ .../internal/os/BatteryStatsCpuTimesTest.java | 10 +- .../os/BatteryStatsUserLifecycleTests.java | 4 +- .../internal/os/MockBatteryStatsImpl.java | 5 + .../server/am/BatteryExternalStatsWorker.java | 16 ++- 5 files changed, 97 insertions(+), 45 deletions(-) diff --git a/core/java/com/android/internal/os/BatteryStatsImpl.java b/core/java/com/android/internal/os/BatteryStatsImpl.java index 84ef5a19772e8..434f617d4619f 100644 --- a/core/java/com/android/internal/os/BatteryStatsImpl.java +++ b/core/java/com/android/internal/os/BatteryStatsImpl.java @@ -300,9 +300,9 @@ public class BatteryStatsImpl extends BatteryStats { @VisibleForTesting public final class UidToRemove { - int startUid; - int endUid; - long mTimeAddedInQueueMs; + private final int mStartUid; + private final int mEndUid; + private final long mUidRemovalTimestamp; /** Remove just one UID */ public UidToRemove(int uid, long timestamp) { @@ -311,38 +311,18 @@ public class BatteryStatsImpl extends BatteryStats { /** Remove a range of UIDs, startUid must be smaller than endUid. */ public UidToRemove(int startUid, int endUid, long timestamp) { - this.startUid = startUid; - this.endUid = endUid; - mTimeAddedInQueueMs = timestamp; + mStartUid = startUid; + mEndUid = endUid; + mUidRemovalTimestamp = timestamp; } - void remove() { - if (startUid == endUid) { - mCpuUidUserSysTimeReader.removeUid(startUid); - mCpuUidFreqTimeReader.removeUid(startUid); - if (mConstants.TRACK_CPU_ACTIVE_CLUSTER_TIME) { - mCpuUidActiveTimeReader.removeUid(startUid); - mCpuUidClusterTimeReader.removeUid(startUid); - } - if (mKernelSingleUidTimeReader != null) { - mKernelSingleUidTimeReader.removeUid(startUid); - } - mNumUidsRemoved++; - } else if (startUid < endUid) { - mCpuUidFreqTimeReader.removeUidsInRange(startUid, endUid); - mCpuUidUserSysTimeReader.removeUidsInRange(startUid, endUid); - if (mConstants.TRACK_CPU_ACTIVE_CLUSTER_TIME) { - mCpuUidActiveTimeReader.removeUidsInRange(startUid, endUid); - mCpuUidClusterTimeReader.removeUidsInRange(startUid, endUid); - } - if (mKernelSingleUidTimeReader != null) { - mKernelSingleUidTimeReader.removeUidsInRange(startUid, endUid); - } - // Treat as one. We don't know how many uids there are in between. - mNumUidsRemoved++; - } else { - Slog.w(TAG, "End UID " + endUid + " is smaller than start UID " + startUid); - } + public long getUidRemovalTimestamp() { + return mUidRemovalTimestamp; + } + + @GuardedBy("BatteryStatsImpl.this") + void removeLocked() { + removeCpuStatsForUidRangeLocked(mStartUid, mEndUid); } } @@ -526,11 +506,16 @@ public class BatteryStatsImpl extends BatteryStats { } } - public void clearPendingRemovedUids() { + /** + * Removes kernel CPU stats for removed UIDs, in the order they were added to the + * mPendingRemovedUids queue. + */ + @GuardedBy("this") + public void clearPendingRemovedUidsLocked() { long cutOffTimeMs = mClocks.elapsedRealtime() - mConstants.UID_REMOVE_DELAY_MS; while (!mPendingRemovedUids.isEmpty() - && mPendingRemovedUids.peek().mTimeAddedInQueueMs < cutOffTimeMs) { - mPendingRemovedUids.poll().remove(); + && mPendingRemovedUids.peek().getUidRemovalTimestamp() < cutOffTimeMs) { + mPendingRemovedUids.poll().removeLocked(); } } @@ -694,6 +679,8 @@ public class BatteryStatsImpl extends BatteryStats { Future scheduleCpuSyncDueToWakelockChange(long delayMillis); void cancelCpuSyncDueToWakelockChange(); Future scheduleSyncDueToBatteryLevelChange(long delayMillis); + /** Schedule removal of UIDs corresponding to a removed user */ + Future scheduleCleanupDueToRemovedUser(int userId); } public Handler mHandler; @@ -14390,6 +14377,18 @@ public class BatteryStatsImpl extends BatteryStats { } public void onUserRemovedLocked(int userId) { + if (mExternalSync != null) { + // Clear out the removed user's UIDs after a short delay. The delay is needed + // because at the point that this method is called, some activities are still + // being wrapped up by those UIDs + mExternalSync.scheduleCleanupDueToRemovedUser(userId); + } + } + + /** + * Removes battery stats for UIDs corresponding to a removed user. + */ + public void clearRemovedUserUidsLocked(int userId) { final int firstUidForUser = UserHandle.getUid(userId, 0); final int lastUidForUser = UserHandle.getUid(userId, UserHandle.PER_USER_RANGE - 1); mUidStats.put(firstUidForUser, null); @@ -14403,6 +14402,7 @@ public class BatteryStatsImpl extends BatteryStats { } } mUidStats.removeAtRange(firstIndex, lastIndex - firstIndex + 1); + removeCpuStatsForUidRangeLocked(firstUidForUser, lastUidForUser); } /** @@ -14425,6 +14425,39 @@ public class BatteryStatsImpl extends BatteryStats { mPendingRemovedUids.add(new UidToRemove(uid, elapsedRealtimeMs)); } + /** + * Removes the data for the deleted UIDs from the underlying kernel eBPF tables. + */ + @GuardedBy("this") + private void removeCpuStatsForUidRangeLocked(int startUid, int endUid) { + if (startUid == endUid) { + mCpuUidUserSysTimeReader.removeUid(startUid); + mCpuUidFreqTimeReader.removeUid(startUid); + if (mConstants.TRACK_CPU_ACTIVE_CLUSTER_TIME) { + mCpuUidActiveTimeReader.removeUid(startUid); + mCpuUidClusterTimeReader.removeUid(startUid); + } + if (mKernelSingleUidTimeReader != null) { + mKernelSingleUidTimeReader.removeUid(startUid); + } + mNumUidsRemoved++; + } else if (startUid < endUid) { + mCpuUidFreqTimeReader.removeUidsInRange(startUid, endUid); + mCpuUidUserSysTimeReader.removeUidsInRange(startUid, endUid); + if (mConstants.TRACK_CPU_ACTIVE_CLUSTER_TIME) { + mCpuUidActiveTimeReader.removeUidsInRange(startUid, endUid); + mCpuUidClusterTimeReader.removeUidsInRange(startUid, endUid); + } + if (mKernelSingleUidTimeReader != null) { + mKernelSingleUidTimeReader.removeUidsInRange(startUid, endUid); + } + // Treat as one. We don't know how many uids there are in between. + mNumUidsRemoved++; + } else { + Slog.w(TAG, "End UID " + endUid + " is smaller than start UID " + startUid); + } + } + /** * Retrieve the statistics object for a particular process, creating * if needed. @@ -14729,7 +14762,7 @@ public class BatteryStatsImpl extends BatteryStats { private void updateUidRemoveDelay(long newTimeMs) { UID_REMOVE_DELAY_MS = newTimeMs; - clearPendingRemovedUids(); + clearPendingRemovedUidsLocked(); } public void dumpLocked(PrintWriter pw) { 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 54d8701c5a6e3..65df390e91338 100644 --- a/core/tests/coretests/src/com/android/internal/os/BatteryStatsCpuTimesTest.java +++ b/core/tests/coretests/src/com/android/internal/os/BatteryStatsCpuTimesTest.java @@ -1263,21 +1263,21 @@ public class BatteryStatsCpuTimesTest { mBatteryStatsImpl.new UidToRemove(1, mClocks.elapsedRealtime())); mBatteryStatsImpl.getPendingRemovedUids().add( mBatteryStatsImpl.new UidToRemove(5, 10, mClocks.elapsedRealtime())); - mBatteryStatsImpl.clearPendingRemovedUids(); + mBatteryStatsImpl.clearPendingRemovedUidsLocked(); assertEquals(2, mBatteryStatsImpl.getPendingRemovedUids().size()); mClocks.realtime = mClocks.uptime = 100_000; - mBatteryStatsImpl.clearPendingRemovedUids(); + mBatteryStatsImpl.clearPendingRemovedUidsLocked(); assertEquals(2, mBatteryStatsImpl.getPendingRemovedUids().size()); mClocks.realtime = mClocks.uptime = 200_000; mBatteryStatsImpl.getPendingRemovedUids().add( mBatteryStatsImpl.new UidToRemove(100, mClocks.elapsedRealtime())); - mBatteryStatsImpl.clearPendingRemovedUids(); + mBatteryStatsImpl.clearPendingRemovedUidsLocked(); assertEquals(3, mBatteryStatsImpl.getPendingRemovedUids().size()); mClocks.realtime = mClocks.uptime = 400_000; - mBatteryStatsImpl.clearPendingRemovedUids(); + mBatteryStatsImpl.clearPendingRemovedUidsLocked(); assertEquals(1, mBatteryStatsImpl.getPendingRemovedUids().size()); verify(mCpuUidActiveTimeReader).removeUid(1); verify(mCpuUidActiveTimeReader).removeUidsInRange(5, 10); @@ -1289,7 +1289,7 @@ public class BatteryStatsCpuTimesTest { verify(mCpuUidUserSysTimeReader).removeUidsInRange(5, 10); mClocks.realtime = mClocks.uptime = 800_000; - mBatteryStatsImpl.clearPendingRemovedUids(); + mBatteryStatsImpl.clearPendingRemovedUidsLocked(); assertEquals(0, mBatteryStatsImpl.getPendingRemovedUids().size()); verify(mCpuUidActiveTimeReader).removeUid(100); verify(mCpuUidClusterTimeReader).removeUid(100); diff --git a/core/tests/coretests/src/com/android/internal/os/BatteryStatsUserLifecycleTests.java b/core/tests/coretests/src/com/android/internal/os/BatteryStatsUserLifecycleTests.java index e90bcb76e457f..de50dcbf6c003 100644 --- a/core/tests/coretests/src/com/android/internal/os/BatteryStatsUserLifecycleTests.java +++ b/core/tests/coretests/src/com/android/internal/os/BatteryStatsUserLifecycleTests.java @@ -51,6 +51,7 @@ public class BatteryStatsUserLifecycleTests { private static final long POLL_INTERVAL_MS = 500; private static final long USER_REMOVE_TIMEOUT_MS = 5_000; private static final long STOP_USER_TIMEOUT_MS = 10_000; + private static final long USER_UIDS_REMOVE_TIMEOUT_MS = 15_000; private static final long BATTERYSTATS_POLLING_TIMEOUT_MS = 5_000; private static final String CPU_DATA_TAG = "cpu"; @@ -78,7 +79,6 @@ public class BatteryStatsUserLifecycleTests { } @Test - @SkipPresubmit("b/180015146") public void testNoCpuDataForRemovedUser() throws Exception { mIam.startUserInBackground(mTestUserId); waitUntilTrue("No uids for started user " + mTestUserId, @@ -108,7 +108,7 @@ public class BatteryStatsUserLifecycleTests { return true; }, USER_REMOVE_TIMEOUT_MS); waitUntilTrue("Uids still found for removed user " + mTestUserId, - () -> getNumberOfUidsInBatteryStats() == 0, BATTERYSTATS_POLLING_TIMEOUT_MS); + () -> getNumberOfUidsInBatteryStats() == 0, USER_UIDS_REMOVE_TIMEOUT_MS); } @After diff --git a/core/tests/coretests/src/com/android/internal/os/MockBatteryStatsImpl.java b/core/tests/coretests/src/com/android/internal/os/MockBatteryStatsImpl.java index 99d576d259ecc..d9b69b1493554 100644 --- a/core/tests/coretests/src/com/android/internal/os/MockBatteryStatsImpl.java +++ b/core/tests/coretests/src/com/android/internal/os/MockBatteryStatsImpl.java @@ -189,6 +189,11 @@ public class MockBatteryStatsImpl extends BatteryStatsImpl { return null; } + @Override + public Future scheduleCleanupDueToRemovedUser(int userId) { + return null; + } + @Override public Future scheduleCpuSyncDueToRemovedUid(int uid) { return null; diff --git a/services/core/java/com/android/server/am/BatteryExternalStatsWorker.java b/services/core/java/com/android/server/am/BatteryExternalStatsWorker.java index 0c633cacda924..449f02ea52249 100644 --- a/services/core/java/com/android/server/am/BatteryExternalStatsWorker.java +++ b/services/core/java/com/android/server/am/BatteryExternalStatsWorker.java @@ -79,6 +79,9 @@ class BatteryExternalStatsWorker implements BatteryStatsImpl.ExternalStatsSync { // There is some accuracy error in wifi reports so allow some slop in the results. private static final long MAX_WIFI_STATS_SAMPLE_ERROR_MILLIS = 750; + // Delay for clearing out battery stats for UIDs corresponding to a removed user + public static final int UID_REMOVAL_AFTER_USER_REMOVAL_DELAY_MILLIS = 10_000; + private final ScheduledExecutorService mExecutorService = Executors.newSingleThreadScheduledExecutor( (ThreadFactory) r -> { @@ -346,6 +349,17 @@ class BatteryExternalStatsWorker implements BatteryStatsImpl.ExternalStatsSync { } } + @Override + public Future scheduleCleanupDueToRemovedUser(int userId) { + synchronized (BatteryExternalStatsWorker.this) { + return mExecutorService.schedule(() -> { + synchronized (mStats) { + mStats.clearRemovedUserUidsLocked(userId); + } + }, UID_REMOVAL_AFTER_USER_REMOVAL_DELAY_MILLIS, TimeUnit.MILLISECONDS); + } + } + /** * Schedule a sync {@param syncRunnable} with a delay. If there's already a scheduled sync, a * new sync won't be scheduled unless it is being scheduled to run immediately (delayMillis=0). @@ -481,7 +495,7 @@ class BatteryExternalStatsWorker implements BatteryStatsImpl.ExternalStatsSync { mStats.removeIsolatedUidLocked(uid, SystemClock.elapsedRealtime(), SystemClock.uptimeMillis()); } - mStats.clearPendingRemovedUids(); + mStats.clearPendingRemovedUidsLocked(); } } catch (Exception e) { Slog.wtf(TAG, "Error updating external stats: ", e);