Make isAppBad() lock-free

We trade off increased cost of (rare) mutations of the set of "bad" apps
in order to make the core lookup "is this specific app bad?" lock-free.
Locking discipline turned out to be a dominant cost in a very hot code
path within the running system, even uncontended.

Bug: 175339368
Bug: 163297662
Test: atest ActivityManagerServiceTest
Change-Id: I8803a177d46b01b86897f1e8d0966eb2452849fd
Merged-In: I8803a177d46b01b86897f1e8d0966eb2452849fd

4 files changed, 48 insertions, 21 deletions

diff --git a/core/java/com/android/internal/app/ProcessMap.java b/core/java/com/android/internal/app/ProcessMap.java
index 81036f7ecba8..4917a47eb000 100644
--- a/core/java/com/android/internal/app/ProcessMap.java
+++ b/core/java/com/android/internal/app/ProcessMap.java
@@ -22,7 +22,7 @@ import android.util.SparseArray;
 public class ProcessMap<E> {
     final ArrayMap<String, SparseArray<E>> mMap
             = new ArrayMap<String, SparseArray<E>>();
-    
+
     public E get(String name, int uid) {
         SparseArray<E> uids = mMap.get(name);
         if (uids == null) return null;
@@ -58,4 +58,6 @@ public class ProcessMap<E> {
     public int size() {
         return mMap.size();
     }
+
+    public void putAll(ProcessMap<E> other) { mMap.putAll(other.mMap); }
 }
diff --git a/services/core/java/com/android/server/am/ActivityManagerService.java b/services/core/java/com/android/server/am/ActivityManagerService.java
index 0f6c8fe55d14..6d58c1e11b67 100644
--- a/services/core/java/com/android/server/am/ActivityManagerService.java
+++ b/services/core/java/com/android/server/am/ActivityManagerService.java
@@ -5999,9 +5999,7 @@ public class ActivityManagerService extends IActivityManager.Stub
     }
 
     private boolean isAppBad(ApplicationInfo info) {
-        synchronized (this) {
-            return mAppErrors.isBadProcessLocked(info);
-        }
+        return mAppErrors.isBadProcess(info.processName, info.uid);
     }
 
     // NOTE: this is an internal method used by the OnShellCommand implementation only and should
diff --git a/services/core/java/com/android/server/am/AppErrors.java b/services/core/java/com/android/server/am/AppErrors.java
index 50d2cab0af81..9838f01510a3 100644
--- a/services/core/java/com/android/server/am/AppErrors.java
+++ b/services/core/java/com/android/server/am/AppErrors.java
@@ -97,9 +97,19 @@ class AppErrors {
      * a minimum amount of time; they are removed from it when they are
      * later restarted (hopefully due to some user action).  The value is the
      * time it was added to the list.
+     *
+     * Read access is UNLOCKED, and must either be based on a single lookup
+     * call on the current mBadProcesses instance, or a local copy of that
+     * reference must be made and the local copy treated as the source of
+     * truth.  Mutations are performed by synchronizing on mBadProcessLock,
+     * cloning the existing mBadProcesses instance, performing the mutation,
+     * then changing the volatile "live" mBadProcesses reference to point to the
+     * mutated version.  These operations are very rare compared to lookups:
+     * we intentionally trade additional cost for mutations for eliminating
+     * lock operations from the simple lookup cases.
      */
-    private final ProcessMap<BadProcessInfo> mBadProcesses = new ProcessMap<>();
-
+    private volatile ProcessMap<BadProcessInfo> mBadProcesses = new ProcessMap<>();
+    private final Object mBadProcessLock = new Object();
 
     AppErrors(Context context, ActivityManagerService service, PackageWatchdog watchdog) {
         context.assertRuntimeOverlayThemable();
@@ -109,7 +119,8 @@ class AppErrors {
     }
 
     void dumpDebug(ProtoOutputStream proto, long fieldId, String dumpPackage) {
-        if (mProcessCrashTimes.getMap().isEmpty() && mBadProcesses.getMap().isEmpty()) {
+        final ProcessMap<BadProcessInfo> badProcesses = mBadProcesses;
+        if (mProcessCrashTimes.getMap().isEmpty() && badProcesses.getMap().isEmpty()) {
             return;
         }
 
@@ -144,8 +155,8 @@ class AppErrors {
 
         }
 
-        if (!mBadProcesses.getMap().isEmpty()) {
-            final ArrayMap<String, SparseArray<BadProcessInfo>> pmap = mBadProcesses.getMap();
+        if (!badProcesses.getMap().isEmpty()) {
+            final ArrayMap<String, SparseArray<BadProcessInfo>> pmap = badProcesses.getMap();
             final int processCount = pmap.size();
             for (int ip = 0; ip < processCount; ip++) {
                 final long btoken = proto.start(AppErrorsProto.BAD_PROCESSES);
@@ -209,9 +220,10 @@ class AppErrors {
             }
         }
 
-        if (!mBadProcesses.getMap().isEmpty()) {
+        final ProcessMap<BadProcessInfo> badProcesses = mBadProcesses;
+        if (!badProcesses.getMap().isEmpty()) {
             boolean printed = false;
-            final ArrayMap<String, SparseArray<BadProcessInfo>> pmap = mBadProcesses.getMap();
+            final ArrayMap<String, SparseArray<BadProcessInfo>> pmap = badProcesses.getMap();
             final int processCount = pmap.size();
             for (int ip = 0; ip < processCount; ip++) {
                 final String pname = pmap.keyAt(ip);
@@ -263,12 +275,27 @@ class AppErrors {
         return needSep;
     }
 
-    boolean isBadProcessLocked(ApplicationInfo info) {
-        return mBadProcesses.get(info.processName, info.uid) != null;
+    boolean isBadProcess(final String processName, final int uid) {
+        // NO LOCKING for the simple lookup
+        return mBadProcesses.get(processName, uid) != null;
+    }
+
+    void clearBadProcess(final String processName, final int uid) {
+        synchronized (mBadProcessLock) {
+            final ProcessMap<BadProcessInfo> badProcesses = new ProcessMap<>();
+            badProcesses.putAll(mBadProcesses);
+            badProcesses.remove(processName, uid);
+            mBadProcesses = badProcesses;
+        }
     }
 
-    void clearBadProcessLocked(ApplicationInfo info) {
-        mBadProcesses.remove(info.processName, info.uid);
+    void markBadProcess(final String processName, final int uid, BadProcessInfo info) {
+        synchronized (mBadProcessLock) {
+            final ProcessMap<BadProcessInfo> badProcesses = new ProcessMap<>();
+            badProcesses.putAll(mBadProcesses);
+            badProcesses.put(processName, uid, info);
+            mBadProcesses = badProcesses;
+        }
     }
 
     void resetProcessCrashTimeLocked(ApplicationInfo info) {
@@ -737,10 +764,10 @@ class AppErrors {
                         app.info.processName);
                 if (!app.isolated) {
                     // XXX We don't have a way to mark isolated processes
-                    // as bad, since they don't have a peristent identity.
-                    mBadProcesses.put(app.info.processName, app.uid,
+                    // as bad, since they don't have a persistent identity.
+                    markBadProcess(app.info.processName, app.uid,
                             new BadProcessInfo(now, shortMsg, longMsg, stackTrace));
-                    mProcessCrashTimes.remove(app.info.processName, app.uid);
+                    mProcessCrashTimes.remove(app.processName, app.uid);
                 }
                 app.bad = true;
                 app.removed = true;
diff --git a/services/core/java/com/android/server/am/ProcessList.java b/services/core/java/com/android/server/am/ProcessList.java
index c851a88b6e68..f1e4ae5ceecd 100644
--- a/services/core/java/com/android/server/am/ProcessList.java
+++ b/services/core/java/com/android/server/am/ProcessList.java
@@ -2336,7 +2336,7 @@ public final class ProcessList {
             if ((intentFlags & Intent.FLAG_FROM_BACKGROUND) != 0) {
                 // If we are in the background, then check to see if this process
                 // is bad.  If so, we will just silently fail.
-                if (mService.mAppErrors.isBadProcessLocked(info)) {
+                if (mService.mAppErrors.isBadProcess(info.processName, info.uid)) {
                     if (DEBUG_PROCESSES) Slog.v(TAG, "Bad process: " + info.uid
                             + "/" + info.processName);
                     return null;
@@ -2349,11 +2349,11 @@ public final class ProcessList {
                 if (DEBUG_PROCESSES) Slog.v(TAG, "Clearing bad process: " + info.uid
                         + "/" + info.processName);
                 mService.mAppErrors.resetProcessCrashTimeLocked(info);
-                if (mService.mAppErrors.isBadProcessLocked(info)) {
+                if (mService.mAppErrors.isBadProcess(info.processName, info.uid)) {
                     EventLog.writeEvent(EventLogTags.AM_PROC_GOOD,
                             UserHandle.getUserId(info.uid), info.uid,
                             info.processName);
-                    mService.mAppErrors.clearBadProcessLocked(info);
+                    mService.mAppErrors.clearBadProcess(info.processName, info.uid);
                     if (app != null) {
                         app.bad = false;
                     }