From 5706277f2d43d5be20a5510d1fb87a53c901ed52 Mon Sep 17 00:00:00 2001 From: Jing Ji Date: Tue, 6 Oct 2020 01:28:17 -0700 Subject: [PATCH] Track the app forked processes by using the accounting info Previously we always scan all of the processes we found and check their parent processes, this is pretty expensive. Now we extract the app forked processes from accounting info of app process list. Bug: 170154111 Test: atest AppChildProcessTest Test: atest CtsAppTestCases:ActivityManagerTest Change-Id: Ia2210dcdada9f221d632bcfe8d4dc248eeeb8c9f --- .../server/am/ActivityManagerService.java | 1 + .../android/server/am/PhantomProcessList.java | 257 +++++++++++++++--- .../server/am/AppChildProcessTest.java | 114 ++++++-- 3 files changed, 314 insertions(+), 58 deletions(-) diff --git a/services/core/java/com/android/server/am/ActivityManagerService.java b/services/core/java/com/android/server/am/ActivityManagerService.java index d3f4667fe4487..91bea42edcd95 100644 --- a/services/core/java/com/android/server/am/ActivityManagerService.java +++ b/services/core/java/com/android/server/am/ActivityManagerService.java @@ -12134,6 +12134,7 @@ public class ActivityManagerService extends IActivityManager.Stub app.setHasClientActivities(false); mServices.killServicesLocked(app, allowRestart); + mPhantomProcessList.onAppDied(app.pid); boolean restart = false; diff --git a/services/core/java/com/android/server/am/PhantomProcessList.java b/services/core/java/com/android/server/am/PhantomProcessList.java index e2fcf08ce82b7..5167c57199551 100644 --- a/services/core/java/com/android/server/am/PhantomProcessList.java +++ b/services/core/java/com/android/server/am/PhantomProcessList.java @@ -27,15 +27,22 @@ import android.app.ApplicationExitInfo.Reason; import android.app.ApplicationExitInfo.SubReason; import android.os.Handler; import android.os.Process; +import android.os.StrictMode; import android.util.Slog; import android.util.SparseArray; import com.android.internal.annotations.GuardedBy; +import com.android.internal.annotations.VisibleForTesting; +import com.android.internal.os.ProcStatsUtil; import com.android.internal.os.ProcessCpuTracker; import libcore.io.IoUtils; import java.io.FileDescriptor; +import java.io.FileInputStream; +import java.io.FileNotFoundException; +import java.io.IOException; +import java.io.InputStream; import java.io.PrintWriter; import java.util.ArrayList; import java.util.Collections; @@ -78,18 +85,178 @@ public final class PhantomProcessList { @GuardedBy("mLock") private final ArrayList mTempPhantomProcesses = new ArrayList<>(); + /** + * The mapping between a phantom process ID to its parent process (an app process) + */ + @GuardedBy("mLock") + private final SparseArray mPhantomToAppProcessMap = new SparseArray<>(); + + @GuardedBy("mLock") + private final SparseArray mCgroupProcsFds = new SparseArray<>(); + + @GuardedBy("mLock") + private final byte[] mDataBuffer = new byte[4096]; + @GuardedBy("mLock") private boolean mTrimPhantomProcessScheduled = false; @GuardedBy("mLock") int mUpdateSeq; + @VisibleForTesting + Injector mInjector; + private final ActivityManagerService mService; private final Handler mKillHandler; PhantomProcessList(final ActivityManagerService service) { mService = service; mKillHandler = service.mProcessList.sKillHandler; + mInjector = new Injector(); + } + + @VisibleForTesting + @GuardedBy("mLock") + void lookForPhantomProcessesLocked() { + mPhantomToAppProcessMap.clear(); + StrictMode.ThreadPolicy oldPolicy = StrictMode.allowThreadDiskReads(); + try { + synchronized (mService.mPidsSelfLocked) { + for (int i = mService.mPidsSelfLocked.size() - 1; i >= 0; i--) { + final ProcessRecord app = mService.mPidsSelfLocked.valueAt(i); + lookForPhantomProcessesLocked(app); + } + } + } finally { + StrictMode.setThreadPolicy(oldPolicy); + } + } + + @GuardedBy({"mLock", "mService.mPidsSelfLocked"}) + private void lookForPhantomProcessesLocked(ProcessRecord app) { + if (app.appZygote || app.killed || app.killedByAm) { + // process forked from app zygote doesn't have its own acct entry + return; + } + InputStream input = mCgroupProcsFds.get(app.pid); + if (input == null) { + final String path = getCgroupFilePath(app.info.uid, app.pid); + try { + input = mInjector.openCgroupProcs(path); + } catch (FileNotFoundException | SecurityException e) { + if (DEBUG_PROCESSES) { + Slog.w(TAG, "Unable to open " + path, e); + } + return; + } + // Keep the FD open for better performance + mCgroupProcsFds.put(app.pid, input); + } + final byte[] buf = mDataBuffer; + try { + int read = 0; + int pid = 0; + long totalRead = 0; + do { + read = mInjector.readCgroupProcs(input, buf, 0, buf.length); + if (read == -1) { + break; + } + totalRead += read; + for (int i = 0; i < read; i++) { + final byte b = buf[i]; + if (b == '\n') { + addChildPidLocked(app, pid); + pid = 0; + } else { + pid = pid * 10 + (b - '0'); + } + } + if (read < buf.length) { + // we may break from here safely as sysfs reading should return the whole page + // if the remaining data is larger than a page + break; + } + } while (true); + if (pid != 0) { + addChildPidLocked(app, pid); + } + // rewind the fd for the next read + input.skip(-totalRead); + } catch (IOException e) { + Slog.e(TAG, "Error in reading cgroup procs from " + app, e); + IoUtils.closeQuietly(input); + mCgroupProcsFds.delete(app.pid); + } + } + + @VisibleForTesting + static String getCgroupFilePath(int uid, int pid) { + return "/acct/uid_" + uid + "/pid_" + pid + "/cgroup.procs"; + } + + static String getProcessName(int pid) { + String procName = ProcStatsUtil.readTerminatedProcFile( + "/proc/" + pid + "/cmdline", (byte) '\0'); + if (procName == null) { + return null; + } + int l = procName.lastIndexOf('/'); + if (l > 0 && l < procName.length() - 1) { + procName = procName.substring(l + 1); + } + return procName; + } + + @GuardedBy({"mLock", "mService.mPidsSelfLocked"}) + private void addChildPidLocked(final ProcessRecord app, final int pid) { + if (app.pid != pid) { + // That's something else... + final ProcessRecord r = mService.mPidsSelfLocked.get(pid); + if (r != null) { + // Is this a process forked via app zygote? + if (!r.appZygote) { + // Unexpected... + if (DEBUG_PROCESSES) { + Slog.w(TAG, "Unexpected: " + r + " appears in the cgroup.procs of " + app); + } + } else { + // Just a child process of app zygote, no worries + } + } else { + final int index = mPhantomToAppProcessMap.indexOfKey(pid); + if (index >= 0) { // unlikely since we cleared the map at the beginning + final ProcessRecord current = mPhantomToAppProcessMap.valueAt(index); + if (app == current) { + // Okay it's unchanged + return; + } + mPhantomToAppProcessMap.setValueAt(index, app); + } else { + mPhantomToAppProcessMap.put(pid, app); + } + // Its UID isn't necessarily to be the same as the app.info.uid, since it could be + // forked from child processes of app zygote + final int uid = Process.getUidForPid(pid); + String procName = mInjector.getProcessName(pid); + if (procName == null || uid < 0) { + mPhantomToAppProcessMap.delete(pid); + return; + } + getOrCreatePhantomProcessIfNeededLocked(procName, uid, pid, true); + } + } + } + + void onAppDied(final int pid) { + synchronized (mLock) { + final int index = mCgroupProcsFds.indexOfKey(pid); + if (index >= 0) { + final InputStream inputStream = mCgroupProcsFds.valueAt(index); + mCgroupProcsFds.removeAt(index); + IoUtils.closeQuietly(inputStream); + } + } } /** @@ -99,7 +266,7 @@ public final class PhantomProcessList { */ @GuardedBy("mLock") PhantomProcessRecord getOrCreatePhantomProcessIfNeededLocked(final String processName, - final int uid, final int pid) { + final int uid, final int pid, boolean createIfNeeded) { // First check if it's actually an app process we know if (isAppProcess(pid)) { return null; @@ -123,56 +290,47 @@ public final class PhantomProcessList { if (proc.equals(processName, uid, pid)) { return proc; } - // Our zombie process information is outdated, let's remove this one, it shoud + // Our zombie process information is outdated, let's remove this one, it should // have been gone. mZombiePhantomProcesses.removeAt(idx); } } - int ppid = getParentPid(pid); + if (!createIfNeeded) { + return null; + } - // Walk through its parents and see if it could be traced back to an app process. - while (ppid > 1) { - if (isAppProcess(ppid)) { - // It's a phantom process, bookkeep it - try { - final PhantomProcessRecord proc = new PhantomProcessRecord( - processName, uid, pid, ppid, mService, - this::onPhantomProcessKilledLocked); - proc.mUpdateSeq = mUpdateSeq; - mPhantomProcesses.put(pid, proc); - SparseArray array = mAppPhantomProcessMap.get(ppid); - if (array == null) { - array = new SparseArray<>(); - mAppPhantomProcessMap.put(ppid, array); - } - array.put(pid, proc); - if (proc.mPidFd != null) { - mKillHandler.getLooper().getQueue().addOnFileDescriptorEventListener( - proc.mPidFd, EVENT_INPUT | EVENT_ERROR, - this::onPhantomProcessFdEvent); - mPhantomProcessesPidFds.put(proc.mPidFd.getInt$(), proc); - } - scheduleTrimPhantomProcessesLocked(); - return proc; - } catch (IllegalStateException e) { - return null; + final ProcessRecord r = mPhantomToAppProcessMap.get(pid); + + if (r != null) { + // It's a phantom process, bookkeep it + try { + final PhantomProcessRecord proc = new PhantomProcessRecord( + processName, uid, pid, r.pid, mService, + this::onPhantomProcessKilledLocked); + proc.mUpdateSeq = mUpdateSeq; + mPhantomProcesses.put(pid, proc); + SparseArray array = mAppPhantomProcessMap.get(r.pid); + if (array == null) { + array = new SparseArray<>(); + mAppPhantomProcessMap.put(r.pid, array); } + array.put(pid, proc); + if (proc.mPidFd != null) { + mKillHandler.getLooper().getQueue().addOnFileDescriptorEventListener( + proc.mPidFd, EVENT_INPUT | EVENT_ERROR, + this::onPhantomProcessFdEvent); + mPhantomProcessesPidFds.put(proc.mPidFd.getInt$(), proc); + } + scheduleTrimPhantomProcessesLocked(); + return proc; + } catch (IllegalStateException e) { + return null; } - - ppid = getParentPid(ppid); } return null; } - private static int getParentPid(int pid) { - try { - return Process.getParentPid(pid); - } catch (Exception e) { - } - return -1; - } - private boolean isAppProcess(int pid) { synchronized (mService.mPidsSelfLocked) { return mService.mPidsSelfLocked.get(pid) != null; @@ -346,10 +504,14 @@ public final class PhantomProcessList { synchronized (mLock) { // refresh the phantom process list with the latest cpu stats results. mUpdateSeq++; + + // Scan app process's accounting procs + lookForPhantomProcessesLocked(); + for (int i = tracker.countStats() - 1; i >= 0; i--) { final ProcessCpuTracker.Stats st = tracker.getStats(i); final PhantomProcessRecord r = - getOrCreatePhantomProcessIfNeededLocked(st.name, st.uid, st.pid); + getOrCreatePhantomProcessIfNeededLocked(st.name, st.uid, st.pid, false); if (r != null) { r.mUpdateSeq = mUpdateSeq; r.mCurrentCputime += st.rel_utime + st.rel_stime; @@ -392,4 +554,19 @@ public final class PhantomProcessList { proc.dump(pw, prefix + " "); } } + + @VisibleForTesting + static class Injector { + InputStream openCgroupProcs(String path) throws FileNotFoundException, SecurityException { + return new FileInputStream(path); + } + + int readCgroupProcs(InputStream input, byte[] buf, int offset, int len) throws IOException { + return input.read(buf, offset, len); + } + + String getProcessName(final int pid) { + return PhantomProcessList.getProcessName(pid); + } + } } diff --git a/services/tests/mockingservicestests/src/com/android/server/am/AppChildProcessTest.java b/services/tests/mockingservicestests/src/com/android/server/am/AppChildProcessTest.java index 04e8b634e72e0..b85da94604760 100644 --- a/services/tests/mockingservicestests/src/com/android/server/am/AppChildProcessTest.java +++ b/services/tests/mockingservicestests/src/com/android/server/am/AppChildProcessTest.java @@ -34,12 +34,12 @@ import android.os.Handler; import android.os.HandlerThread; import android.os.Process; import android.platform.test.annotations.Presubmit; +import android.util.ArrayMap; import android.util.ArraySet; import com.android.dx.mockito.inline.extended.StaticMockitoSession; import com.android.server.LocalServices; import com.android.server.ServiceThread; -import com.android.server.am.ActivityManagerService.Injector; import com.android.server.appop.AppOpsService; import com.android.server.wm.ActivityTaskManagerService; @@ -55,7 +55,11 @@ import org.mockito.Mock; import org.mockito.MockitoAnnotations; import org.mockito.quality.Strictness; +import java.io.ByteArrayInputStream; import java.io.File; +import java.io.FileNotFoundException; +import java.io.IOException; +import java.io.InputStream; @Presubmit public class AppChildProcessTest { @@ -68,6 +72,7 @@ public class AppChildProcessTest { private Context mContext = getInstrumentation().getTargetContext(); private TestInjector mInjector; + private PhantomTestInjector mPhantomInjector; private ActivityManagerService mAms; private ProcessList mProcessList; private PhantomProcessList mPhantomProcessList; @@ -94,6 +99,7 @@ public class AppChildProcessTest { mProcessList = spy(pList); mInjector = new TestInjector(mContext); + mPhantomInjector = new PhantomTestInjector(); mAms = new ActivityManagerService(mInjector, mServiceThreadRule.getThread()); mAms.mActivityTaskManager = new ActivityTaskManagerService(mContext); mAms.mActivityTaskManager.initialize(null, null, mContext.getMainLooper()); @@ -101,6 +107,7 @@ public class AppChildProcessTest { mAms.mPackageManagerInt = mPackageManagerInt; pList.mService = mAms; mPhantomProcessList = mAms.mPhantomProcessList; + mPhantomProcessList.mInjector = mPhantomInjector; doReturn(new ComponentName("", "")).when(mPackageManagerInt).getSystemUiServiceComponent(); doReturn(false).when(() -> Process.supportsPidFd()); // Remove stale instance of PackageManagerInternal if there is any @@ -136,47 +143,60 @@ public class AppChildProcessTest { final String child2ProcessName = "test1_child1_child2"; final String nativeProcessName = "test_native"; - makeParent(zygote64Pid, initPid); - makeParent(zygote32Pid, initPid); + makeProcess(rootUid, zygote64Pid, zygote64ProcessName); + makeParent(rootUid, zygote64Pid, initPid); + makeProcess(rootUid, zygote32Pid, zygote32ProcessName); + makeParent(rootUid, zygote32Pid, initPid); makeAppProcess(app1Pid, app1Uid, app1ProcessName, app1ProcessName); - makeParent(app1Pid, zygote64Pid); makeAppProcess(app2Pid, app2Uid, app2ProcessName, app2ProcessName); - makeParent(app2Pid, zygote64Pid); + + mPhantomProcessList.lookForPhantomProcessesLocked(); assertEquals(0, mPhantomProcessList.mPhantomProcesses.size()); // Verify zygote itself isn't a phantom process assertEquals(null, mPhantomProcessList.getOrCreatePhantomProcessIfNeededLocked( - zygote64ProcessName, rootUid, zygote64Pid)); + zygote64ProcessName, rootUid, zygote64Pid, false)); assertEquals(null, mPhantomProcessList.getOrCreatePhantomProcessIfNeededLocked( - zygote32ProcessName, rootUid, zygote32Pid)); + zygote32ProcessName, rootUid, zygote32Pid, false)); // Verify none of the app isn't a phantom process assertEquals(null, mPhantomProcessList.getOrCreatePhantomProcessIfNeededLocked( - app1ProcessName, app1Uid, app1Pid)); + app1ProcessName, app1Uid, app1Pid, false)); assertEquals(null, mPhantomProcessList.getOrCreatePhantomProcessIfNeededLocked( - app2ProcessName, app2Uid, app2Pid)); + app2ProcessName, app2Uid, app2Pid, false)); // "Fork" an app child process - makeParent(child1Pid, app1Pid); + makeProcess(app1Uid, child1Pid, child1ProcessName); + makeParent(app1Uid, child1Pid, app1Pid); + mPhantomProcessList.lookForPhantomProcessesLocked(); + PhantomProcessRecord pr = mPhantomProcessList - .getOrCreatePhantomProcessIfNeededLocked(child1ProcessName, app1Uid, child1Pid); + .getOrCreatePhantomProcessIfNeededLocked( + child1ProcessName, app1Uid, child1Pid, true); assertTrue(pr != null); assertEquals(1, mPhantomProcessList.mPhantomProcesses.size()); assertEquals(pr, mPhantomProcessList.mPhantomProcesses.valueAt(0)); verifyPhantomProcessRecord(pr, child1ProcessName, app1Uid, child1Pid); // Create another native process from init - makeParent(nativePid, initPid); + makeProcess(rootUid, nativePid, nativeProcessName); + makeParent(rootUid, nativePid, initPid); + mPhantomProcessList.lookForPhantomProcessesLocked(); + assertEquals(null, mPhantomProcessList.getOrCreatePhantomProcessIfNeededLocked( - nativeProcessName, rootUid, nativePid)); + nativeProcessName, rootUid, nativePid, false)); assertEquals(1, mPhantomProcessList.mPhantomProcesses.size()); assertEquals(pr, mPhantomProcessList.mPhantomProcesses.valueAt(0)); // "Fork" another app child process - makeParent(child2Pid, child1Pid); + makeProcess(app1Uid, child2Pid, child2ProcessName); + makeParent(app1Uid, child2Pid, app1Pid); + mPhantomProcessList.lookForPhantomProcessesLocked(); + PhantomProcessRecord pr2 = mPhantomProcessList - .getOrCreatePhantomProcessIfNeededLocked(child2ProcessName, app1Uid, child2Pid); + .getOrCreatePhantomProcessIfNeededLocked( + child2ProcessName, app1Uid, child2Pid, false); assertTrue(pr2 != null); assertEquals(2, mPhantomProcessList.mPhantomProcesses.size()); verifyPhantomProcessRecord(pr2, child2ProcessName, app1Uid, child2Pid); @@ -197,19 +217,27 @@ public class AppChildProcessTest { assertEquals(pid, pr.mPid); } + private void makeProcess(int uid, int pid, String processName) { + doReturn(uid).when(() -> Process.getUidForPid(eq(pid))); + mPhantomInjector.mPidToName.put(pid, processName); + } + private void makeAppProcess(int pid, int uid, String packageName, String processName) { + makeProcess(uid, pid, processName); ApplicationInfo ai = new ApplicationInfo(); ai.packageName = packageName; + ai.uid = uid; ProcessRecord app = new ProcessRecord(mAms, ai, processName, uid); app.pid = pid; mAms.mPidsSelfLocked.doAddInternal(app); + mPhantomInjector.addToProcess(uid, pid, pid); } - private void makeParent(int pid, int ppid) { - doReturn(ppid).when(() -> Process.getParentPid(eq(pid))); + private void makeParent(int uid, int pid, int ppid) { + mPhantomInjector.addToProcess(uid, ppid, pid); } - private class TestInjector extends Injector { + private class TestInjector extends ActivityManagerService.Injector { TestInjector(Context context) { super(context); } @@ -230,6 +258,56 @@ public class AppChildProcessTest { } } + private class PhantomTestInjector extends PhantomProcessList.Injector { + ArrayMap mPathToInput = new ArrayMap<>(); + ArrayMap mPathToData = new ArrayMap<>(); + ArrayMap mInputToData = new ArrayMap<>(); + ArrayMap mPidToName = new ArrayMap<>(); + + @Override + InputStream openCgroupProcs(String path) throws FileNotFoundException, SecurityException { + InputStream input = mPathToInput.get(path); + if (input != null) { + return input; + } + input = new ByteArrayInputStream(new byte[8]); // buf size doesn't matter here + mPathToInput.put(path, input); + StringBuffer sb = mPathToData.get(path); + if (sb == null) { + sb = new StringBuffer(); + mPathToData.put(path, sb); + } + mInputToData.put(input, sb); + return input; + } + + @Override + int readCgroupProcs(InputStream input, byte[] buf, int offset, int len) throws IOException { + StringBuffer sb = mInputToData.get(input); + if (sb == null) { + return -1; + } + byte[] avail = sb.toString().getBytes(); + System.arraycopy(avail, 0, buf, offset, Math.min(len, avail.length)); + return Math.min(len, avail.length); + } + + @Override + String getProcessName(final int pid) { + return mPidToName.get(pid); + } + + void addToProcess(int uid, int pid, int newPid) { + final String path = PhantomProcessList.getCgroupFilePath(uid, pid); + StringBuffer sb = mPathToData.get(path); + if (sb == null) { + sb = new StringBuffer(); + mPathToData.put(path, sb); + } + sb.append(newPid).append('\n'); + } + } + static class ServiceThreadRule implements TestRule { private ServiceThread mThread;