Android/frameworks_base
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Fix concurrency of BatteryStats cleanup after user removal

Browse Source 
Bug: 193742542
Bug: 180015146
Test: atest --rerun-until-failure 20 FrameworksCoreTests:BatteryStatsUserLifecycleTests#testNoCpuDataForRemovedUser

Change-Id: If45cbf0be6c29fc99771053c6a0ec34c60e293a8
This commit is contained in:
Dmitri Plotnikov
2021-07-15 13:46:22 -07:00
parent 59c65885d2
commit ece4ef7393
5 changed files with 97 additions and 45 deletions
Download Patch File Download Diff File Expand all files Collapse all files

107
core/java/com/android/internal/os/BatteryStatsImpl.java
View File

@@ -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) {

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

@@ -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);

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

@@ -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

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

@@ -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;

16
services/core/java/com/android/server/am/BatteryExternalStatsWorker.java
View File

@@ -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);