diff options
3 files changed, 284 insertions, 43 deletions
diff --git a/services/core/java/com/android/server/power/hint/HintManagerService.java b/services/core/java/com/android/server/power/hint/HintManagerService.java index 88c2e095453d..dd39fb02573e 100644 --- a/services/core/java/com/android/server/power/hint/HintManagerService.java +++ b/services/core/java/com/android/server/power/hint/HintManagerService.java @@ -34,7 +34,7 @@ import android.os.RemoteException; import android.os.SystemProperties; import android.util.ArrayMap; import android.util.ArraySet; -import android.util.SparseArray; +import android.util.SparseIntArray; import android.util.StatsEvent; import com.android.internal.annotations.GuardedBy; @@ -256,10 +256,11 @@ public final class HintManagerService extends SystemService { @VisibleForTesting final class MyUidObserver extends UidObserver { - private final SparseArray<Integer> mProcStatesCache = new SparseArray<>(); - + private final Object mCacheLock = new Object(); + @GuardedBy("mCacheLock") + private final SparseIntArray mProcStatesCache = new SparseIntArray(); public boolean isUidForeground(int uid) { - synchronized (mLock) { + synchronized (mCacheLock) { return mProcStatesCache.get(uid, ActivityManager.PROCESS_STATE_IMPORTANT_FOREGROUND) <= ActivityManager.PROCESS_STATE_IMPORTANT_FOREGROUND; } @@ -268,6 +269,9 @@ public final class HintManagerService extends SystemService { @Override public void onUidGone(int uid, boolean disabled) { FgThread.getHandler().post(() -> { + synchronized (mCacheLock) { + mProcStatesCache.delete(uid); + } synchronized (mLock) { ArrayMap<IBinder, ArraySet<AppHintSession>> tokenMap = mActiveSessions.get(uid); if (tokenMap == null) { @@ -280,7 +284,6 @@ public final class HintManagerService extends SystemService { sessionSet.valueAt(j).close(); } } - mProcStatesCache.delete(uid); } }); } @@ -292,15 +295,18 @@ public final class HintManagerService extends SystemService { @Override public void onUidStateChanged(int uid, int procState, long procStateSeq, int capability) { FgThread.getHandler().post(() -> { - synchronized (mLock) { + synchronized (mCacheLock) { mProcStatesCache.put(uid, procState); + } + boolean shouldAllowUpdate = isUidForeground(uid); + synchronized (mLock) { ArrayMap<IBinder, ArraySet<AppHintSession>> tokenMap = mActiveSessions.get(uid); if (tokenMap == null) { return; } for (ArraySet<AppHintSession> sessionSet : tokenMap.values()) { for (AppHintSession s : sessionSet) { - s.onProcStateChanged(); + s.onProcStateChanged(shouldAllowUpdate); } } } @@ -429,10 +435,10 @@ public final class HintManagerService extends SystemService { if (!DumpUtils.checkDumpPermission(getContext(), TAG, pw)) { return; } + pw.println("HintSessionPreferredRate: " + mHintSessionPreferredRate); + pw.println("HAL Support: " + isHalSupported()); + pw.println("Active Sessions:"); synchronized (mLock) { - pw.println("HintSessionPreferredRate: " + mHintSessionPreferredRate); - pw.println("HAL Support: " + isHalSupported()); - pw.println("Active Sessions:"); for (int i = 0; i < mActiveSessions.size(); i++) { pw.println("Uid " + mActiveSessions.keyAt(i).toString() + ":"); ArrayMap<IBinder, ArraySet<AppHintSession>> tokenMap = @@ -476,7 +482,8 @@ public final class HintManagerService extends SystemService { mHalSessionPtr = halSessionPtr; mTargetDurationNanos = durationNanos; mUpdateAllowed = true; - updateHintAllowed(); + final boolean allowed = mUidObserver.isUidForeground(mUid); + updateHintAllowed(allowed); try { token.linkToDeath(this, 0); } catch (RemoteException e) { @@ -486,9 +493,8 @@ public final class HintManagerService extends SystemService { } @VisibleForTesting - boolean updateHintAllowed() { - synchronized (mLock) { - final boolean allowed = mUidObserver.isUidForeground(mUid); + boolean updateHintAllowed(boolean allowed) { + synchronized (this) { if (allowed && !mUpdateAllowed) resume(); if (!allowed && mUpdateAllowed) pause(); mUpdateAllowed = allowed; @@ -498,8 +504,8 @@ public final class HintManagerService extends SystemService { @Override public void updateTargetWorkDuration(long targetDurationNanos) { - synchronized (mLock) { - if (mHalSessionPtr == 0 || !updateHintAllowed()) { + synchronized (this) { + if (mHalSessionPtr == 0 || !mUpdateAllowed) { return; } Preconditions.checkArgument(targetDurationNanos > 0, "Expected" @@ -511,8 +517,8 @@ public final class HintManagerService extends SystemService { @Override public void reportActualWorkDuration(long[] actualDurationNanos, long[] timeStampNanos) { - synchronized (mLock) { - if (mHalSessionPtr == 0 || !updateHintAllowed()) { + synchronized (this) { + if (mHalSessionPtr == 0 || !mUpdateAllowed) { return; } Preconditions.checkArgument(actualDurationNanos.length != 0, "the count" @@ -534,11 +540,13 @@ public final class HintManagerService extends SystemService { /** TODO: consider monitor session threads and close session if any thread is dead. */ @Override public void close() { - synchronized (mLock) { + synchronized (this) { if (mHalSessionPtr == 0) return; mNativeWrapper.halCloseHintSession(mHalSessionPtr); mHalSessionPtr = 0; mToken.unlinkToDeath(this, 0); + } + synchronized (mLock) { ArrayMap<IBinder, ArraySet<AppHintSession>> tokenMap = mActiveSessions.get(mUid); if (tokenMap == null) { Slogf.w(TAG, "UID %d is not present in active session map", mUid); @@ -557,8 +565,8 @@ public final class HintManagerService extends SystemService { @Override public void sendHint(@PerformanceHintManager.Session.Hint int hint) { - synchronized (mLock) { - if (mHalSessionPtr == 0 || !updateHintAllowed()) { + synchronized (this) { + if (mHalSessionPtr == 0 || !mUpdateAllowed) { return; } Preconditions.checkArgument(hint >= 0, "the hint ID value should be" @@ -568,7 +576,7 @@ public final class HintManagerService extends SystemService { } public void setThreads(@NonNull int[] tids) { - synchronized (mLock) { + synchronized (this) { if (mHalSessionPtr == 0) { return; } @@ -588,7 +596,7 @@ public final class HintManagerService extends SystemService { } finally { Binder.restoreCallingIdentity(identity); } - if (!updateHintAllowed()) { + if (!mUpdateAllowed) { Slogf.v(TAG, "update hint not allowed, storing tids."); mNewThreadIds = tids; return; @@ -599,13 +607,15 @@ public final class HintManagerService extends SystemService { } public int[] getThreadIds() { - return mThreadIds; + synchronized (this) { + return Arrays.copyOf(mThreadIds, mThreadIds.length); + } } @Override public void setMode(int mode, boolean enabled) { - synchronized (mLock) { - if (mHalSessionPtr == 0 || !updateHintAllowed()) { + synchronized (this) { + if (mHalSessionPtr == 0 || !mUpdateAllowed) { return; } Preconditions.checkArgument(mode >= 0, "the mode Id value should be" @@ -614,19 +624,19 @@ public final class HintManagerService extends SystemService { } } - private void onProcStateChanged() { - updateHintAllowed(); + private void onProcStateChanged(boolean updateAllowed) { + updateHintAllowed(updateAllowed); } private void pause() { - synchronized (mLock) { + synchronized (this) { if (mHalSessionPtr == 0) return; mNativeWrapper.halPauseHintSession(mHalSessionPtr); } } private void resume() { - synchronized (mLock) { + synchronized (this) { if (mHalSessionPtr == 0) return; mNativeWrapper.halResumeHintSession(mHalSessionPtr); if (mNewThreadIds != null) { @@ -638,12 +648,12 @@ public final class HintManagerService extends SystemService { } private void dump(PrintWriter pw, String prefix) { - synchronized (mLock) { + synchronized (this) { pw.println(prefix + "SessionPID: " + mPid); pw.println(prefix + "SessionUID: " + mUid); pw.println(prefix + "SessionTIDs: " + Arrays.toString(mThreadIds)); pw.println(prefix + "SessionTargetDurationNanos: " + mTargetDurationNanos); - pw.println(prefix + "SessionAllowed: " + updateHintAllowed()); + pw.println(prefix + "SessionAllowed: " + mUpdateAllowed); } } diff --git a/services/core/java/com/android/server/power/hint/TEST_MAPPING b/services/core/java/com/android/server/power/hint/TEST_MAPPING new file mode 100644 index 000000000000..10c53624b7ee --- /dev/null +++ b/services/core/java/com/android/server/power/hint/TEST_MAPPING @@ -0,0 +1,15 @@ +{ + "postsubmit": [ + { + "name": "FrameworksServicesTests", + "options": [ + { + "include-filter": "com.android.server.power.hint" + }, + { + "exclude-annotation": "androidx.test.filters.FlakyTest" + } + ] + } + ] +}
\ No newline at end of file diff --git a/services/tests/servicestests/src/com/android/server/power/hint/HintManagerServiceTest.java b/services/tests/servicestests/src/com/android/server/power/hint/HintManagerServiceTest.java index 9fca513e50b9..d09aa89179b8 100644 --- a/services/tests/servicestests/src/com/android/server/power/hint/HintManagerServiceTest.java +++ b/services/tests/servicestests/src/com/android/server/power/hint/HintManagerServiceTest.java @@ -25,8 +25,6 @@ import static org.junit.Assert.assertNotEquals; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertTrue; -import static org.junit.Assume.assumeFalse; -import static org.junit.Assume.assumeTrue; import static org.mockito.Mockito.any; import static org.mockito.Mockito.anyBoolean; import static org.mockito.Mockito.anyInt; @@ -46,6 +44,7 @@ import android.os.IBinder; import android.os.IHintSession; import android.os.PerformanceHintManager; import android.os.Process; +import android.util.Log; import com.android.server.FgThread; import com.android.server.LocalServices; @@ -58,7 +57,14 @@ import org.junit.Test; import org.mockito.Mock; import org.mockito.MockitoAnnotations; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicReference; +import java.util.concurrent.locks.LockSupport; /** * Tests for {@link com.android.server.power.hint.HintManagerService}. @@ -67,8 +73,11 @@ import java.util.concurrent.CountDownLatch; * atest FrameworksServicesTests:HintManagerServiceTest */ public class HintManagerServiceTest { + private static final String TAG = "HintManagerServiceTest"; + private static final long DEFAULT_HINT_PREFERRED_RATE = 16666666L; private static final long DEFAULT_TARGET_DURATION = 16666666L; + private static final long CONCURRENCY_TEST_DURATION_SEC = 10; private static final int UID = Process.myUid(); private static final int TID = Process.myPid(); private static final int TGID = Process.getThreadGroupLeader(TID); @@ -103,6 +112,55 @@ public class HintManagerServiceTest { LocalServices.addService(ActivityManagerInternal.class, mAmInternalMock); } + static class NativeWrapperFake extends NativeWrapper { + @Override + public void halInit() { + } + + @Override + public long halGetHintSessionPreferredRate() { + return 1; + } + + @Override + public long halCreateHintSession(int tgid, int uid, int[] tids, long durationNanos) { + return 1; + } + + @Override + public void halPauseHintSession(long halPtr) { + } + + @Override + public void halResumeHintSession(long halPtr) { + } + + @Override + public void halCloseHintSession(long halPtr) { + } + + @Override + public void halUpdateTargetWorkDuration(long halPtr, long targetDurationNanos) { + } + + @Override + public void halReportActualWorkDuration( + long halPtr, long[] actualDurationNanos, long[] timeStampNanos) { + } + + @Override + public void halSendHint(long halPtr, int hint) { + } + + @Override + public void halSetThreads(long halPtr, int[] tids) { + } + + @Override + public void halSetMode(long halPtr, int mode, boolean enabled) { + } + } + private HintManagerService createService() { mService = new HintManagerService(mContext, new Injector() { NativeWrapper createNativeWrapper() { @@ -112,6 +170,15 @@ public class HintManagerServiceTest { return mService; } + private HintManagerService createServiceWithFakeWrapper() { + mService = new HintManagerService(mContext, new Injector() { + NativeWrapper createNativeWrapper() { + return new NativeWrapperFake(); + } + }); + return mService; + } + @Test public void testInitializeService() { HintManagerService service = createService(); @@ -166,8 +233,7 @@ public class HintManagerServiceTest { latch.countDown(); }); latch.await(); - - assumeFalse(a.updateHintAllowed()); + assertFalse(service.mUidObserver.isUidForeground(a.mUid)); verify(mNativeWrapperMock, times(1)).halPauseHintSession(anyLong()); // Set session to foreground and calling updateHintAllowed() would invoke resume(); @@ -181,7 +247,7 @@ public class HintManagerServiceTest { }); latch2.await(); - assumeTrue(a.updateHintAllowed()); + assertTrue(service.mUidObserver.isUidForeground(a.mUid)); verify(mNativeWrapperMock, times(1)).halResumeHintSession(anyLong()); } @@ -254,7 +320,7 @@ public class HintManagerServiceTest { }); latch.await(); - assumeFalse(a.updateHintAllowed()); + assertFalse(service.mUidObserver.isUidForeground(a.mUid)); a.reportActualWorkDuration(DURATIONS_THREE, TIMESTAMPS_THREE); verify(mNativeWrapperMock, never()).halReportActualWorkDuration(anyLong(), any(), any()); } @@ -280,7 +346,7 @@ public class HintManagerServiceTest { service.mUidObserver.onUidStateChanged( a.mUid, ActivityManager.PROCESS_STATE_TRANSIENT_BACKGROUND, 0, 0); FgThread.getHandler().runWithScissors(() -> { }, 500); - assertFalse(a.updateHintAllowed()); + assertFalse(service.mUidObserver.isUidForeground(a.mUid)); a.sendHint(PerformanceHintManager.Session.CPU_LOAD_RESET); verify(mNativeWrapperMock, never()).halSendHint(anyLong(), anyInt()); } @@ -303,7 +369,7 @@ public class HintManagerServiceTest { }); latch.await(); - assertFalse(a.updateHintAllowed()); + assertFalse(service.mUidObserver.isUidForeground(a.mUid)); } @Test @@ -316,7 +382,7 @@ public class HintManagerServiceTest { service.mUidObserver.onUidStateChanged( a.mUid, ActivityManager.PROCESS_STATE_IMPORTANT_FOREGROUND, 0, 0); - assertTrue(a.updateHintAllowed()); + assertTrue(service.mUidObserver.isUidForeground(a.mUid)); } @Test @@ -342,7 +408,7 @@ public class HintManagerServiceTest { service.mUidObserver.onUidStateChanged( a.mUid, ActivityManager.PROCESS_STATE_TRANSIENT_BACKGROUND, 0, 0); FgThread.getHandler().runWithScissors(() -> { }, 500); - assertFalse(a.updateHintAllowed()); + assertFalse(service.mUidObserver.isUidForeground(a.mUid)); a.setThreads(SESSION_TIDS_A); verify(mNativeWrapperMock, never()).halSetThreads(anyLong(), any()); } @@ -372,9 +438,159 @@ public class HintManagerServiceTest { service.mUidObserver.onUidStateChanged( a.mUid, ActivityManager.PROCESS_STATE_TRANSIENT_BACKGROUND, 0, 0); FgThread.getHandler().runWithScissors(() -> { }, 500); - assertFalse(a.updateHintAllowed()); + assertFalse(service.mUidObserver.isUidForeground(a.mUid)); a.setMode(0, true); verify(mNativeWrapperMock, never()).halSetMode(anyLong(), anyInt(), anyBoolean()); } + // This test checks that concurrent operations from different threads on IHintService, + // IHintSession and UidObserver will not cause data race or deadlock. Ideally we should also + // check the output of threads' reportActualDuration performance to detect lock starvation + // but the result is not stable, so it's better checked manually. + @Test + public void testConcurrency() throws Exception { + HintManagerService service = createServiceWithFakeWrapper(); + // initialize session threads to run in parallel + final int sessionCount = 10; + // the signal that the main thread will send to session threads to check for run or exit + AtomicReference<Boolean> shouldRun = new AtomicReference<>(true); + // the signal for main test thread to wait for session threads and notifier thread to + // finish and exit + CountDownLatch latch = new CountDownLatch(sessionCount + 1); + // list of exceptions with one per session thread or notifier thread + List<AtomicReference<Exception>> execs = new ArrayList<>(sessionCount + 1); + List<Thread> threads = new ArrayList<>(sessionCount + 1); + for (int i = 0; i < sessionCount; i++) { + final AtomicReference<Exception> exec = new AtomicReference<>(); + execs.add(exec); + int j = i; + Thread app = new Thread(() -> { + try { + while (shouldRun.get()) { + runAppHintSession(service, j, shouldRun); + } + } catch (Exception e) { + exec.set(e); + } finally { + latch.countDown(); + } + }); + threads.add(app); + } + + // initialize a UID state notifier thread to run in parallel + final AtomicReference<Exception> notifierExec = new AtomicReference<>(); + execs.add(notifierExec); + Thread notifier = new Thread(() -> { + try { + long min = Long.MAX_VALUE; + long max = Long.MIN_VALUE; + long sum = 0; + int count = 0; + while (shouldRun.get()) { + long start = System.nanoTime(); + service.mUidObserver.onUidStateChanged(UID, + ActivityManager.PROCESS_STATE_IMPORTANT_FOREGROUND, 0, 0); + long elapsed = System.nanoTime() - start; + sum += elapsed; + count++; + min = Math.min(min, elapsed); + max = Math.max(max, elapsed); + LockSupport.parkNanos(TimeUnit.MILLISECONDS.toNanos(500)); + service.mUidObserver.onUidStateChanged(UID, + ActivityManager.PROCESS_STATE_TRANSIENT_BACKGROUND, 0, 0); + LockSupport.parkNanos(TimeUnit.MILLISECONDS.toNanos(500)); + } + Log.d(TAG, "notifier thread min " + min + " max " + max + " avg " + sum / count); + service.mUidObserver.onUidGone(UID, true); + } catch (Exception e) { + notifierExec.set(e); + } finally { + latch.countDown(); + } + }); + threads.add(notifier); + + // start all the threads + for (Thread thread : threads) { + thread.start(); + } + // keep the test running for a few seconds + LockSupport.parkNanos(TimeUnit.SECONDS.toNanos(CONCURRENCY_TEST_DURATION_SEC)); + // send signal to stop all threads + shouldRun.set(false); + // wait for all threads to exit + latch.await(); + // check if any thread throws exception + for (AtomicReference<Exception> exec : execs) { + if (exec.get() != null) { + throw exec.get(); + } + } + } + + private void runAppHintSession(HintManagerService service, int logId, + AtomicReference<Boolean> shouldRun) throws Exception { + IBinder token = new Binder(); + AppHintSession a = (AppHintSession) service.getBinderServiceInstance() + .createHintSession(token, SESSION_TIDS_A, DEFAULT_TARGET_DURATION); + // we will start some threads and get their valid TIDs to update + int threadCount = 3; + // the list of TIDs + int[] tids = new int[threadCount]; + // atomic index for each thread to set its TID in the list + AtomicInteger k = new AtomicInteger(0); + // signal for the session main thread to wait for child threads to finish updating TIDs + CountDownLatch latch = new CountDownLatch(threadCount); + // signal for the session main thread to notify child threads to exit + CountDownLatch stopLatch = new CountDownLatch(1); + for (int j = 0; j < threadCount; j++) { + Thread thread = new Thread(() -> { + try { + tids[k.getAndIncrement()] = android.os.Process.myTid(); + latch.countDown(); + stopLatch.await(); + } catch (InterruptedException e) { + throw new RuntimeException(e); + } + }); + thread.start(); + } + latch.await(); + a.setThreads(tids); + // we don't need the threads to exist after update + stopLatch.countDown(); + a.updateTargetWorkDuration(5); + // measure the time it takes in HintManagerService to run reportActualDuration + long min = Long.MAX_VALUE; + long max = Long.MIN_VALUE; + long sum = 0; + int count = 0; + List<Long> values = new ArrayList<>(); + long testStart = System.nanoTime(); + // run report actual for 4-second per cycle + while (shouldRun.get() && System.nanoTime() - testStart < TimeUnit.SECONDS.toNanos( + Math.min(4, CONCURRENCY_TEST_DURATION_SEC))) { + long start = System.nanoTime(); + a.reportActualWorkDuration(new long[]{5}, new long[]{start}); + long elapsed = System.nanoTime() - start; + values.add(elapsed); + LockSupport.parkNanos(TimeUnit.MILLISECONDS.toNanos(5)); + sum += elapsed; + count++; + min = Math.min(min, elapsed); + max = Math.max(max, elapsed); + } + Collections.sort(values); + if (!values.isEmpty()) { + Log.d(TAG, "app thread " + logId + " min " + min + " max " + max + + " avg " + sum / count + " count " + count + + " 80th " + values.get((int) (values.size() * 0.8)) + + " 90th " + values.get((int) (values.size() * 0.9)) + + " 95th " + values.get((int) (values.size() * 0.95))); + } else { + Log.w(TAG, "No reportActualWorkDuration executed"); + } + a.close(); + } } |