From a0326c86ad318483ce05c30290fdba72c7b6e33d Mon Sep 17 00:00:00 2001 From: Misha Wagner Date: Tue, 10 Aug 2021 14:12:48 +0100 Subject: [PATCH 1/2] Fix race condition when using mPreserveTopNApps. Bug: 191357172 Test: atest CacheOomRankerTest Change-Id: I8cdf96ed1ddf56261e6ee953fb1078e961a0bc41 --- .../java/com/android/server/am/CacheOomRanker.java | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/services/core/java/com/android/server/am/CacheOomRanker.java b/services/core/java/com/android/server/am/CacheOomRanker.java index 1ead7e3c589fe..bd7ee019e1eec 100644 --- a/services/core/java/com/android/server/am/CacheOomRanker.java +++ b/services/core/java/com/android/server/am/CacheOomRanker.java @@ -243,6 +243,7 @@ public class CacheOomRanker { float lruWeight; float usesWeight; float rssWeight; + int preserveTopNApps; int[] lruPositions; RankedProcessRecord[] scoredProcessRecords; @@ -250,6 +251,7 @@ public class CacheOomRanker { lruWeight = mLruWeight; usesWeight = mUsesWeight; rssWeight = mRssWeight; + preserveTopNApps = mPreserveTopNApps; lruPositions = mLruPositions; scoredProcessRecords = mScoredProcessRecords; } @@ -276,19 +278,19 @@ public class CacheOomRanker { ++numProcessesEvaluated; } - // Count how many apps we're not re-ranking (up to mPreserveTopNApps). + // Count how many apps we're not re-ranking (up to preserveTopNApps). int numProcessesNotReRanked = 0; while (numProcessesEvaluated < lruProcessServiceStart - && numProcessesNotReRanked < mPreserveTopNApps) { + && numProcessesNotReRanked < preserveTopNApps) { ProcessRecord process = lruList.get(numProcessesEvaluated); if (appCanBeReRanked(process)) { numProcessesNotReRanked++; } numProcessesEvaluated++; } - // Exclude the top `mPreserveTopNApps` apps from re-ranking. - if (numProcessesNotReRanked < mPreserveTopNApps) { - numProcessesReRanked -= mPreserveTopNApps - numProcessesNotReRanked; + // Exclude the top `preserveTopNApps` apps from re-ranking. + if (numProcessesNotReRanked < preserveTopNApps) { + numProcessesReRanked -= preserveTopNApps - numProcessesNotReRanked; if (numProcessesReRanked < 0) { numProcessesReRanked = 0; } From 480cc3123c5ee6ae4fff56599124794a0ff25aa4 Mon Sep 17 00:00:00 2001 From: Misha Wagner Date: Tue, 10 Aug 2021 13:57:57 +0100 Subject: [PATCH 2/2] Change CacheOomRanker's "uses" feature. The previous implementation was incorrect, as an app goes in and out of the cache several times during oom_adj calculation. If all apps started at the same time, then this would be OK - as they all get incremented the same amount, plus get incremented when they actually go in and out of the cache. This would result in the relative counts being correct. However, as all apps aren't started at the same time, the old implementation is instead a measure of how long the app has been started. So instead we use mSetProcState, which isn't changed on each oom_adj calculation. This measure should accurately reflect the number of times the process is used - whether that's by a user app open, or a content receiver call, or an intent being broadcast. See go/sim-v-impl for more details. Test: atest CacheOomRankerTest Bug: 196031723 Change-Id: I49d02362b8277f8472355e78412f79256c2289a1 --- core/java/android/app/ActivityManager.java | 5 +++++ .../com/android/server/am/ProcessStateRecord.java | 11 +++++------ .../src/com/android/server/am/CacheOomRankerTest.java | 9 +++++---- 3 files changed, 15 insertions(+), 10 deletions(-) diff --git a/core/java/android/app/ActivityManager.java b/core/java/android/app/ActivityManager.java index b29349ee47de6..365493ded1838 100644 --- a/core/java/android/app/ActivityManager.java +++ b/core/java/android/app/ActivityManager.java @@ -771,6 +771,11 @@ public class ActivityManager { return procState >= PROCESS_STATE_TRANSIENT_BACKGROUND; } + /** @hide Should this process state be considered in the cache? */ + public static final boolean isProcStateCached(int procState) { + return procState >= PROCESS_STATE_CACHED_ACTIVITY; + } + /** @hide Is this a foreground service type? */ public static boolean isForegroundService(int procState) { return procState == PROCESS_STATE_FOREGROUND_SERVICE; diff --git a/services/core/java/com/android/server/am/ProcessStateRecord.java b/services/core/java/com/android/server/am/ProcessStateRecord.java index 46144f586dbd1..e8e61f2c5734c 100644 --- a/services/core/java/com/android/server/am/ProcessStateRecord.java +++ b/services/core/java/com/android/server/am/ProcessStateRecord.java @@ -611,6 +611,10 @@ final class ProcessStateRecord { @GuardedBy({"mService", "mProcLock"}) void setSetProcState(int setProcState) { + if (ActivityManager.isProcStateCached(mSetProcState) + && !ActivityManager.isProcStateCached(setProcState)) { + mCacheOomRankerUseCount++; + } mSetProcState = setProcState; } @@ -874,12 +878,7 @@ final class ProcessStateRecord { @GuardedBy("mService") void setCached(boolean cached) { - if (mCached != cached) { - mCached = cached; - if (cached) { - ++mCacheOomRankerUseCount; - } - } + mCached = cached; } @GuardedBy("mService") diff --git a/services/tests/mockingservicestests/src/com/android/server/am/CacheOomRankerTest.java b/services/tests/mockingservicestests/src/com/android/server/am/CacheOomRankerTest.java index 15dfd266853ee..d0c9242969454 100644 --- a/services/tests/mockingservicestests/src/com/android/server/am/CacheOomRankerTest.java +++ b/services/tests/mockingservicestests/src/com/android/server/am/CacheOomRankerTest.java @@ -24,6 +24,7 @@ import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.spy; +import android.app.ActivityManager; import android.app.IApplicationThread; import android.content.ComponentName; import android.content.Context; @@ -581,7 +582,7 @@ public class CacheOomRankerTest { } private ProcessRecord nextProcessRecord(int setAdj, long lastActivityTime, long lastRss, - int returnedToCacheCount) { + int wentToForegroundCount) { ApplicationInfo ai = new ApplicationInfo(); ai.packageName = "a.package.name" + mNextPackageName++; ProcessRecord app = new ProcessRecord(mAms, ai, ai.packageName + ":process", mNextUid++); @@ -593,9 +594,9 @@ public class CacheOomRankerTest { app.setLastActivityTime(lastActivityTime); app.mProfile.setLastRss(lastRss); app.mState.setCached(false); - for (int i = 0; i < returnedToCacheCount; ++i) { - app.mState.setCached(false); - app.mState.setCached(true); + for (int i = 0; i < wentToForegroundCount; ++i) { + app.mState.setSetProcState(ActivityManager.PROCESS_STATE_FOREGROUND_SERVICE); + app.mState.setSetProcState(ActivityManager.PROCESS_STATE_CACHED_RECENT); } // Sets the thread returned by ProcessRecord#getThread, which we use to check whether the // app is currently launching.