From 75cb48988fba32b0d60c69d7eebb6e3b8b324dd4 Mon Sep 17 00:00:00 2001 From: Sudheer Shanka Date: Mon, 5 Dec 2022 20:12:40 +0000 Subject: [PATCH 1/2] Don't wait for message queues to become idle in wait-for-barrier. It would take a long time for some of the message queues in system_server process to become idle and we only need to make sure any messages currently waiting have been handled as part of "wait-for-barrier". Bug: 260158381 Test: TH Change-Id: Ibcb36ebf767806c23f087ed34ad8792672fbe0d6 --- .../server/am/ActivityManagerService.java | 2 +- .../android/server/am/BroadcastLoopers.java | 37 ++++++++++++++++--- 2 files changed, 33 insertions(+), 6 deletions(-) diff --git a/services/core/java/com/android/server/am/ActivityManagerService.java b/services/core/java/com/android/server/am/ActivityManagerService.java index 7566bab93cc15..efe14f4dec1c6 100644 --- a/services/core/java/com/android/server/am/ActivityManagerService.java +++ b/services/core/java/com/android/server/am/ActivityManagerService.java @@ -18324,7 +18324,7 @@ public class ActivityManagerService extends IActivityManager.Stub public void waitForBroadcastBarrier(@Nullable PrintWriter pw) { enforceCallingPermission(permission.DUMP, "waitForBroadcastBarrier()"); - BroadcastLoopers.waitForIdle(pw); + BroadcastLoopers.waitForBarrier(pw); for (BroadcastQueue queue : mBroadcastQueues) { queue.waitForBarrier(pw); } diff --git a/services/core/java/com/android/server/am/BroadcastLoopers.java b/services/core/java/com/android/server/am/BroadcastLoopers.java index b828720c9162e..a5535cb13165c 100644 --- a/services/core/java/com/android/server/am/BroadcastLoopers.java +++ b/services/core/java/com/android/server/am/BroadcastLoopers.java @@ -18,6 +18,7 @@ package com.android.server.am; import android.annotation.NonNull; import android.annotation.Nullable; +import android.os.Handler; import android.os.Looper; import android.os.Message; import android.os.MessageQueue; @@ -30,6 +31,7 @@ import com.android.internal.annotations.GuardedBy; import java.io.PrintWriter; import java.util.Objects; import java.util.concurrent.CountDownLatch; +import java.util.function.BiConsumer; /** * Collection of {@link Looper} that are known to be used for broadcast dispatch @@ -73,19 +75,44 @@ public class BroadcastLoopers { * still in the future are ignored for the purposes of the idle test. */ public static void waitForIdle(@Nullable PrintWriter pw) { + waitForCondition(pw, (looper, latch) -> { + final MessageQueue queue = looper.getQueue(); + queue.addIdleHandler(() -> { + latch.countDown(); + return false; + }); + }); + } + + /** + * Wait for all registered {@link Looper} instances to handle currently waiting messages. + * Note that {@link Message#when} still in the future are ignored for the purposes + * of the idle test. + */ + public static void waitForBarrier(@Nullable PrintWriter pw) { + waitForCondition(pw, (looper, latch) -> { + (new Handler(looper)).post(() -> { + latch.countDown(); + }); + }); + } + + /** + * Wait for all registered {@link Looper} instances to meet a certain condition. + */ + private static void waitForCondition(@Nullable PrintWriter pw, + @NonNull BiConsumer condition) { final CountDownLatch latch; synchronized (sLoopers) { final int N = sLoopers.size(); latch = new CountDownLatch(N); for (int i = 0; i < N; i++) { - final MessageQueue queue = sLoopers.valueAt(i).getQueue(); + final Looper looper = sLoopers.valueAt(i); + final MessageQueue queue = looper.getQueue(); if (queue.isIdle()) { latch.countDown(); } else { - queue.addIdleHandler(() -> { - latch.countDown(); - return false; - }); + condition.accept(looper, latch); } } } From c37dea91b1f2dfbaedc5d5c99a54adfd96df38bb Mon Sep 17 00:00:00 2001 From: Sudheer Shanka Date: Mon, 5 Dec 2022 23:31:50 +0000 Subject: [PATCH 2/2] Don't let offload broadcasts to be starved. Similar to how we are making sure urgent broadcasts don't starve normal broadcasts, we need to make sure offload broadcasts don't get starved by urgent and normal broadcasts. Bug: 260158381 Test: atest services/tests/mockingservicestests/src/com/android/server/am/BroadcastQueueTest.java Test: atest services/tests/mockingservicestests/src/com/android/server/am/BroadcastQueueModernImplTest.java Change-Id: Ibd70ef40598178673d6731efa06b1f57fa90970c --- .../android/server/am/BroadcastConstants.java | 22 +++++ .../server/am/BroadcastProcessQueue.java | 83 ++++++++++++------- .../am/BroadcastQueueModernImplTest.java | 67 +++++++++++++++ 3 files changed, 141 insertions(+), 31 deletions(-) diff --git a/services/core/java/com/android/server/am/BroadcastConstants.java b/services/core/java/com/android/server/am/BroadcastConstants.java index 1eebd0127818e..f5d1c106705dc 100644 --- a/services/core/java/com/android/server/am/BroadcastConstants.java +++ b/services/core/java/com/android/server/am/BroadcastConstants.java @@ -153,11 +153,26 @@ public class BroadcastConstants { "bcast_extra_running_urgent_process_queues"; private static final int DEFAULT_EXTRA_RUNNING_URGENT_PROCESS_QUEUES = 1; + /** + * For {@link BroadcastQueueModernImpl}: Maximum number of consecutive urgent + * broadcast dispatches allowed before letting broadcasts in lower priority queue + * to be scheduled in order to avoid starvation. + */ public int MAX_CONSECUTIVE_URGENT_DISPATCHES = DEFAULT_MAX_CONSECUTIVE_URGENT_DISPATCHES; private static final String KEY_MAX_CONSECUTIVE_URGENT_DISPATCHES = "bcast_max_consecutive_urgent_dispatches"; private static final int DEFAULT_MAX_CONSECUTIVE_URGENT_DISPATCHES = 3; + /** + * For {@link BroadcastQueueModernImpl}: Maximum number of consecutive normal + * broadcast dispatches allowed before letting broadcasts in lower priority queue + * to be scheduled in order to avoid starvation. + */ + public int MAX_CONSECUTIVE_NORMAL_DISPATCHES = DEFAULT_MAX_CONSECUTIVE_NORMAL_DISPATCHES; + private static final String KEY_MAX_CONSECUTIVE_NORMAL_DISPATCHES = + "bcast_max_consecutive_normal_dispatches"; + private static final int DEFAULT_MAX_CONSECUTIVE_NORMAL_DISPATCHES = 10; + /** * For {@link BroadcastQueueModernImpl}: Maximum number of active broadcasts * to dispatch to a "running" process queue before we retire them back to @@ -341,6 +356,9 @@ public class BroadcastConstants { MAX_CONSECUTIVE_URGENT_DISPATCHES = getDeviceConfigInt( KEY_MAX_CONSECUTIVE_URGENT_DISPATCHES, DEFAULT_MAX_CONSECUTIVE_URGENT_DISPATCHES); + MAX_CONSECUTIVE_NORMAL_DISPATCHES = getDeviceConfigInt( + KEY_MAX_CONSECUTIVE_NORMAL_DISPATCHES, + DEFAULT_MAX_CONSECUTIVE_NORMAL_DISPATCHES); MAX_RUNNING_ACTIVE_BROADCASTS = getDeviceConfigInt(KEY_MAX_RUNNING_ACTIVE_BROADCASTS, DEFAULT_MAX_RUNNING_ACTIVE_BROADCASTS); MAX_PENDING_BROADCASTS = getDeviceConfigInt(KEY_MAX_PENDING_BROADCASTS, @@ -396,6 +414,10 @@ public class BroadcastConstants { TimeUtils.formatDuration(DELAY_URGENT_MILLIS)).println(); pw.print(KEY_MAX_HISTORY_COMPLETE_SIZE, MAX_HISTORY_COMPLETE_SIZE).println(); pw.print(KEY_MAX_HISTORY_SUMMARY_SIZE, MAX_HISTORY_SUMMARY_SIZE).println(); + pw.print(KEY_MAX_CONSECUTIVE_URGENT_DISPATCHES, + MAX_CONSECUTIVE_URGENT_DISPATCHES).println(); + pw.print(KEY_MAX_CONSECUTIVE_NORMAL_DISPATCHES, + MAX_CONSECUTIVE_NORMAL_DISPATCHES).println(); pw.decreaseIndent(); pw.println(); } diff --git a/services/core/java/com/android/server/am/BroadcastProcessQueue.java b/services/core/java/com/android/server/am/BroadcastProcessQueue.java index 66d7fc92340a1..672392d46a356 100644 --- a/services/core/java/com/android/server/am/BroadcastProcessQueue.java +++ b/services/core/java/com/android/server/am/BroadcastProcessQueue.java @@ -152,6 +152,12 @@ class BroadcastProcessQueue { */ private int mActiveCountConsecutiveUrgent; + /** + * Number of consecutive normal broadcasts that have been dispatched + * since the last offload dispatch. + */ + private int mActiveCountConsecutiveNormal; + /** * Count of pending broadcasts of these various flavors. */ @@ -551,48 +557,63 @@ class BroadcastProcessQueue { * Will thrown an exception if there are no pending broadcasts; relies on * {@link #isEmpty()} being false. */ - SomeArgs removeNextBroadcast() { + private @Nullable SomeArgs removeNextBroadcast() { final ArrayDeque queue = queueForNextBroadcast(); if (queue == mPendingUrgent) { mActiveCountConsecutiveUrgent++; - } else { + } else if (queue == mPending) { mActiveCountConsecutiveUrgent = 0; + mActiveCountConsecutiveNormal++; + } else if (queue == mPendingOffload) { + mActiveCountConsecutiveUrgent = 0; + mActiveCountConsecutiveNormal = 0; } - return queue.removeFirst(); + return !isQueueEmpty(queue) ? queue.removeFirst() : null; } @Nullable ArrayDeque queueForNextBroadcast() { - ArrayDeque nextUrgent = mPendingUrgent.isEmpty() ? null : mPendingUrgent; - ArrayDeque nextNormal = null; - if (!mPending.isEmpty()) { - nextNormal = mPending; - } else if (!mPendingOffload.isEmpty()) { - nextNormal = mPendingOffload; + final ArrayDeque nextNormal = queueForNextBroadcast( + mPending, mPendingOffload, + mActiveCountConsecutiveNormal, constants.MAX_CONSECUTIVE_NORMAL_DISPATCHES); + final ArrayDeque nextBroadcastQueue = queueForNextBroadcast( + mPendingUrgent, nextNormal, + mActiveCountConsecutiveUrgent, constants.MAX_CONSECUTIVE_URGENT_DISPATCHES); + return nextBroadcastQueue; + } + + private @Nullable ArrayDeque queueForNextBroadcast( + @Nullable ArrayDeque highPriorityQueue, + @Nullable ArrayDeque lowPriorityQueue, + int consecutiveHighPriorityCount, + int maxHighPriorityDispatchLimit) { + // nothing high priority pending, no further decisionmaking + if (isQueueEmpty(highPriorityQueue)) { + return lowPriorityQueue; } - // nothing urgent pending, no further decisionmaking - if (nextUrgent == null) { - return nextNormal; - } - // nothing but urgent pending, also no further decisionmaking - if (nextNormal == null) { - return nextUrgent; + // nothing but high priority pending, also no further decisionmaking + if (isQueueEmpty(lowPriorityQueue)) { + return highPriorityQueue; } - // Starvation mitigation: although we prioritize urgent broadcasts by default, - // we allow non-urgent deliveries to make steady progress even if urgent - // broadcasts are arriving faster than they can be dispatched. + // Starvation mitigation: although we prioritize high priority queues by default, + // we allow low priority queues to make steady progress even if broadcasts in + // high priority queue are arriving faster than they can be dispatched. // - // We do not try to defer to the next non-urgent broadcast if that broadcast + // We do not try to defer to the next broadcast in low priority queues if that broadcast // is ordered and still blocked on delivery to other recipients. - final SomeArgs nextNormalArgs = nextNormal.peekFirst(); - final BroadcastRecord rNormal = (BroadcastRecord) nextNormalArgs.arg1; - final int nextNormalIndex = nextNormalArgs.argi1; - final BroadcastRecord rUrgent = (BroadcastRecord) nextUrgent.peekFirst().arg1; - final boolean canTakeNormal = - mActiveCountConsecutiveUrgent >= constants.MAX_CONSECUTIVE_URGENT_DISPATCHES - && rNormal.enqueueTime <= rUrgent.enqueueTime - && !blockedOnOrderedDispatch(rNormal, nextNormalIndex); - return canTakeNormal ? nextNormal : nextUrgent; + final SomeArgs nextLPArgs = lowPriorityQueue.peekFirst(); + final BroadcastRecord nextLPRecord = (BroadcastRecord) nextLPArgs.arg1; + final int nextLPRecordIndex = nextLPArgs.argi1; + final BroadcastRecord nextHPRecord = (BroadcastRecord) highPriorityQueue.peekFirst().arg1; + final boolean isLPQueueEligible = + consecutiveHighPriorityCount >= maxHighPriorityDispatchLimit + && nextLPRecord.enqueueTime <= nextHPRecord.enqueueTime + && !blockedOnOrderedDispatch(nextLPRecord, nextLPRecordIndex); + return isLPQueueEligible ? lowPriorityQueue : highPriorityQueue; + } + + private static boolean isQueueEmpty(@Nullable ArrayDeque queue) { + return (queue == null || queue.isEmpty()); } /** @@ -600,13 +621,13 @@ class BroadcastProcessQueue { */ @Nullable SomeArgs peekNextBroadcast() { ArrayDeque queue = queueForNextBroadcast(); - return (queue != null) ? queue.peekFirst() : null; + return !isQueueEmpty(queue) ? queue.peekFirst() : null; } @VisibleForTesting @Nullable BroadcastRecord peekNextBroadcastRecord() { ArrayDeque queue = queueForNextBroadcast(); - return (queue != null) ? (BroadcastRecord) queue.peekFirst().arg1 : null; + return !isQueueEmpty(queue) ? (BroadcastRecord) queue.peekFirst().arg1 : null; } /** diff --git a/services/tests/mockingservicestests/src/com/android/server/am/BroadcastQueueModernImplTest.java b/services/tests/mockingservicestests/src/com/android/server/am/BroadcastQueueModernImplTest.java index c87fd26fbe822..de09b19438396 100644 --- a/services/tests/mockingservicestests/src/com/android/server/am/BroadcastQueueModernImplTest.java +++ b/services/tests/mockingservicestests/src/com/android/server/am/BroadcastQueueModernImplTest.java @@ -591,6 +591,73 @@ public class BroadcastQueueModernImplTest { assertEquals(Intent.ACTION_ALARM_CHANGED, queue.getActive().intent.getAction()); } + /** + * Verify that offload broadcasts are not starved because of broadcasts in higher priority + * queues. + */ + @Test + public void testOffloadStarvation() { + final BroadcastOptions optInteractive = BroadcastOptions.makeBasic(); + optInteractive.setInteractive(true); + + mConstants.MAX_CONSECUTIVE_URGENT_DISPATCHES = 1; + mConstants.MAX_CONSECUTIVE_NORMAL_DISPATCHES = 2; + final BroadcastProcessQueue queue = new BroadcastProcessQueue(mConstants, + PACKAGE_GREEN, getUidForPackage(PACKAGE_GREEN)); + + // mix of broadcasts, with more than 2 normal + queue.enqueueOrReplaceBroadcast( + makeBroadcastRecord(new Intent(Intent.ACTION_BOOT_COMPLETED) + .addFlags(Intent.FLAG_RECEIVER_OFFLOAD)), 0); + queue.enqueueOrReplaceBroadcast( + makeBroadcastRecord(new Intent(Intent.ACTION_TIMEZONE_CHANGED)), 0); + queue.enqueueOrReplaceBroadcast( + makeBroadcastRecord(new Intent(Intent.ACTION_PACKAGE_CHANGED) + .addFlags(Intent.FLAG_RECEIVER_OFFLOAD)), 0); + queue.enqueueOrReplaceBroadcast( + makeBroadcastRecord(new Intent(Intent.ACTION_ALARM_CHANGED)), 0); + queue.enqueueOrReplaceBroadcast( + makeBroadcastRecord(new Intent(Intent.ACTION_TIME_TICK)), 0); + queue.enqueueOrReplaceBroadcast( + makeBroadcastRecord(new Intent(Intent.ACTION_LOCALE_CHANGED) + .addFlags(Intent.FLAG_RECEIVER_FOREGROUND)), 0); + queue.enqueueOrReplaceBroadcast( + makeBroadcastRecord(new Intent(Intent.ACTION_APPLICATION_PREFERENCES), + optInteractive), 0); + queue.enqueueOrReplaceBroadcast( + makeBroadcastRecord(new Intent(AppWidgetManager.ACTION_APPWIDGET_UPDATE), + optInteractive), 0); + queue.enqueueOrReplaceBroadcast( + makeBroadcastRecord(new Intent(Intent.ACTION_INPUT_METHOD_CHANGED) + .addFlags(Intent.FLAG_RECEIVER_FOREGROUND)), 0); + queue.enqueueOrReplaceBroadcast( + makeBroadcastRecord(new Intent(Intent.ACTION_NEW_OUTGOING_CALL), + optInteractive), 0); + + queue.makeActiveNextPending(); + assertEquals(Intent.ACTION_LOCALE_CHANGED, queue.getActive().intent.getAction()); + // after MAX_CONSECUTIVE_URGENT_DISPATCHES expect an ordinary one next + queue.makeActiveNextPending(); + assertEquals(Intent.ACTION_TIMEZONE_CHANGED, queue.getActive().intent.getAction()); + // and then back to prioritizing urgent ones + queue.makeActiveNextPending(); + assertEquals(Intent.ACTION_APPLICATION_PREFERENCES, queue.getActive().intent.getAction()); + // after MAX_CONSECUTIVE_URGENT_DISPATCHES, again an ordinary one next + queue.makeActiveNextPending(); + assertEquals(Intent.ACTION_ALARM_CHANGED, queue.getActive().intent.getAction()); + // and then back to prioritizing urgent ones + queue.makeActiveNextPending(); + assertEquals(AppWidgetManager.ACTION_APPWIDGET_UPDATE, + queue.getActive().intent.getAction()); + // after MAX_CONSECUTIVE_URGENT_DISPATCHES and MAX_CONSECUTIVE_NORMAL_DISPATCHES, + // expect an offload one + queue.makeActiveNextPending(); + assertEquals(Intent.ACTION_BOOT_COMPLETED, queue.getActive().intent.getAction()); + // and then back to prioritizing urgent ones + queue.makeActiveNextPending(); + assertEquals(Intent.ACTION_INPUT_METHOD_CHANGED, queue.getActive().intent.getAction()); + } + /** * Verify that sending a broadcast that removes any matching pending * broadcasts is applied as expected.