From b52f000feceb6a73072e94a73c1883d469f24a29 Mon Sep 17 00:00:00 2001 From: Tim Murray Date: Wed, 2 May 2018 15:14:35 -0700 Subject: [PATCH] ActivityManagerService: explicitly break connection cycles Currently, it is possible for two services to bind to each other and receive each other's oom_adj, even if there are no other bindings that would cause either process to be promoted. This causes a pair of services to remain at a higher oom_adj level than they intended if one service was promoted by a binding that was later destroyed. To fix this, any process that encounters a cycle will be marked for a later adjustment, and those bindings that encounter a cycle will not be used to promote processes. After the initial oom_adj adjustments, all processes that encountered a cycle will be retried. These retries will continue until no process that encountered a cycle changes its process state, at which point the process states should be stable. Test: CTS; boots; runs normally bug 78894563 Change-Id: I7cb2aa2fc461c08bc8e7f687a673249aef78351b --- .../server/am/ActivityManagerService.java | 97 +++++++++++++++++-- .../com/android/server/am/ProcessRecord.java | 2 + 2 files changed, 89 insertions(+), 10 deletions(-) diff --git a/services/core/java/com/android/server/am/ActivityManagerService.java b/services/core/java/com/android/server/am/ActivityManagerService.java index a8e63f6695d46..680b90c6c90c0 100644 --- a/services/core/java/com/android/server/am/ActivityManagerService.java +++ b/services/core/java/com/android/server/am/ActivityManagerService.java @@ -22967,18 +22967,27 @@ public class ActivityManagerService extends IActivityManager.Stub } } - private final int computeOomAdjLocked(ProcessRecord app, int cachedAdj, ProcessRecord TOP_APP, + private final boolean computeOomAdjLocked(ProcessRecord app, int cachedAdj, ProcessRecord TOP_APP, boolean doingAll, long now) { if (mAdjSeq == app.adjSeq) { - // This adjustment has already been computed. - return app.curRawAdj; + if (app.adjSeq == app.completedAdjSeq) { + // This adjustment has already been computed successfully. + return false; + } else { + // The process is being computed, so there is a cycle. We cannot + // rely on this process's state. + app.containsCycle = true; + return false; + } } if (app.thread == null) { app.adjSeq = mAdjSeq; app.curSchedGroup = ProcessList.SCHED_GROUP_BACKGROUND; app.curProcState = ActivityManager.PROCESS_STATE_CACHED_EMPTY; - return (app.curAdj=app.curRawAdj=ProcessList.CACHED_APP_MAX_ADJ); + app.curAdj=app.curRawAdj=ProcessList.CACHED_APP_MAX_ADJ; + app.completedAdjSeq = app.adjSeq; + return false; } app.adjTypeCode = ActivityManager.RunningAppProcessInfo.REASON_UNKNOWN; @@ -22991,6 +23000,8 @@ public class ActivityManagerService extends IActivityManager.Stub final int appUid = app.info.uid; final int logUid = mCurOomAdjUid; + int prevAppAdj = app.curAdj; + if (app.maxAdj <= ProcessList.FOREGROUND_APP_ADJ) { // The max adjustment doesn't allow this app to be anything // below foreground, so it is not worth doing work for it. @@ -23035,7 +23046,10 @@ public class ActivityManagerService extends IActivityManager.Stub app.curSchedGroup = ProcessList.SCHED_GROUP_RESTRICTED; } } - return (app.curAdj=app.maxAdj); + app.curAdj = app.maxAdj; + app.completedAdjSeq = app.adjSeq; + // if curAdj is less than prevAppAdj, then this process was promoted + return app.curAdj < prevAppAdj; } app.systemNoUi = false; @@ -23047,6 +23061,8 @@ public class ActivityManagerService extends IActivityManager.Stub int adj; int schedGroup; int procState; + int cachedAdjSeq; + boolean foregroundActivities = false; mTmpBroadcastQueue.clear(); if (PROCESS_STATE_CUR_TOP == ActivityManager.PROCESS_STATE_TOP && app == TOP_APP) { @@ -23360,9 +23376,9 @@ public class ActivityManagerService extends IActivityManager.Stub // there are applications dependent on our services or providers, but // this gives us a baseline and makes sure we don't get into an // infinite recursion. - app.adjSeq = mAdjSeq; app.curRawAdj = adj; app.hasStartedServices = false; + app.adjSeq = mAdjSeq; if (mBackupTarget != null && app == mBackupTarget.app) { // If possible we want to avoid killing apps while they're being backed up @@ -23461,8 +23477,15 @@ public class ActivityManagerService extends IActivityManager.Stub if ((cr.flags&Context.BIND_WAIVE_PRIORITY) == 0) { ProcessRecord client = cr.binding.client; - int clientAdj = computeOomAdjLocked(client, cachedAdj, - TOP_APP, doingAll, now); + computeOomAdjLocked(client, cachedAdj, TOP_APP, doingAll, now); + if (client.containsCycle) { + // We've detected a cycle. We should ignore this connection and allow + // this process to retry computeOomAdjLocked later in case a later-checked + // connection from a client would raise its priority legitimately. + app.containsCycle = true; + continue; + } + int clientAdj = client.curRawAdj; int clientProcState = client.curProcState; if (clientProcState >= ActivityManager.PROCESS_STATE_CACHED_ACTIVITY) { // If the other app is cached for any reason, for purposes here @@ -23681,7 +23704,15 @@ public class ActivityManagerService extends IActivityManager.Stub // Being our own client is not interesting. continue; } - int clientAdj = computeOomAdjLocked(client, cachedAdj, TOP_APP, doingAll, now); + computeOomAdjLocked(client, cachedAdj, TOP_APP, doingAll, now); + if (client.containsCycle) { + // We've detected a cycle. We should ignore this connection and allow + // this process to retry computeOomAdjLocked later in case a later-checked + // connection from a client would raise its priority legitimately. + app.containsCycle = true; + continue; + } + int clientAdj = client.curRawAdj; int clientProcState = client.curProcState; if (clientProcState >= ActivityManager.PROCESS_STATE_CACHED_ACTIVITY) { // If the other app is cached for any reason, for purposes here @@ -23909,8 +23940,10 @@ public class ActivityManagerService extends IActivityManager.Stub app.curSchedGroup = schedGroup; app.curProcState = procState; app.foregroundActivities = foregroundActivities; + app.completedAdjSeq = mAdjSeq; - return app.curRawAdj; + // if curAdj is less than prevAppAdj, then this process was promoted + return app.curAdj < prevAppAdj; } /** @@ -24884,12 +24917,23 @@ public class ActivityManagerService extends IActivityManager.Stub int nextCachedAdj = curCachedAdj+1; int curEmptyAdj = ProcessList.CACHED_APP_MIN_ADJ; int nextEmptyAdj = curEmptyAdj+2; + + boolean retryCycles = false; + + // need to reset cycle state before calling computeOomAdjLocked because of service connections + for (int i=N-1; i>=0; i--) { + ProcessRecord app = mLruProcesses.get(i); + app.containsCycle = false; + } for (int i=N-1; i>=0; i--) { ProcessRecord app = mLruProcesses.get(i); if (!app.killedByAm && app.thread != null) { app.procStateChanged = false; computeOomAdjLocked(app, ProcessList.UNKNOWN_ADJ, TOP_APP, true, now); + // if any app encountered a cycle, we need to perform an additional loop later + retryCycles |= app.containsCycle; + // If we haven't yet assigned the final cached adj // to the process, do that now. if (app.curAdj >= ProcessList.UNKNOWN_ADJ) { @@ -24943,6 +24987,39 @@ public class ActivityManagerService extends IActivityManager.Stub } } + + } + } + + // Cycle strategy: + // - Retry computing any process that has encountered a cycle. + // - Continue retrying until no process was promoted. + // - Iterate from least important to most important. + int cycleCount = 0; + while (retryCycles) { + cycleCount++; + retryCycles = false; + + for (int i=0; i=0; i--) { + ProcessRecord app = mLruProcesses.get(i); + if (!app.killedByAm && app.thread != null) { applyOomAdjLocked(app, true, now, nowElapsed); // Count the number of process types. diff --git a/services/core/java/com/android/server/am/ProcessRecord.java b/services/core/java/com/android/server/am/ProcessRecord.java index b7fde1da1eda0..caf52e3595484 100644 --- a/services/core/java/com/android/server/am/ProcessRecord.java +++ b/services/core/java/com/android/server/am/ProcessRecord.java @@ -149,6 +149,8 @@ final class ProcessRecord { String waitingToKill; // Process is waiting to be killed when in the bg, and reason Object forcingToImportant; // Token that is forcing this process to be important int adjSeq; // Sequence id for identifying oom_adj assignment cycles + int completedAdjSeq; // Sequence id for identifying oom_adj assignment cycles + boolean containsCycle; // Whether this app has encountered a cycle in the most recent update int lruSeq; // Sequence id for identifying LRU update cycles CompatibilityInfo compat; // last used compatibility mode IBinder.DeathRecipient deathRecipient; // Who is watching for the death.