Merge "Fix concurrency of BatteryStats cleanup after user removal" (95ccc85b) · Commits · e / os / android_frameworks_base

core/java/com/android/internal/os/BatteryStatsImpl.java

+72 −39

Original line number	Diff line number	Diff line
		@@ -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) {

core/tests/coretests/src/com/android/internal/os/BatteryStatsCpuTimesTest.java

+5 −5

Original line number	Diff line number	Diff line
		@@ -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);

core/tests/coretests/src/com/android/internal/os/BatteryStatsUserLifecycleTests.java

+2 −2

Original line number	Diff line number	Diff line
		@@ -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

core/tests/coretests/src/com/android/internal/os/MockBatteryStatsImpl.java

+5 −0

Original line number	Diff line number	Diff line
		@@ -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;

services/core/java/com/android/server/am/BatteryExternalStatsWorker.java

+15 −1

Original line number	Diff line number	Diff line
		@@ -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);