SF: avoid updating queue if schedule() is called.

Avoids a rarish race condition where a callback is scheduled in the
interim time between the TimerDispatch starting to run and the callback
for that scheduled callback is invoked. In this condition, the code will
now have the next callback pend until the timer queue processes the when
to wakeup next.

Bug: 154303580
Test: 3 new unit tests
Test: boot to home, check some animations
Test: overnight dogfood with patch.

Change-Id: I0e7e2e3698ed6d1765082db20d9cf25f6e6c2db2

services/surfaceflinger/Scheduler/VSyncDispatchTimerQueue.cpp

@@ -87,10 +87,26 @@ ScheduleResult VSyncDispatchTimerQueueEntry::schedule(nsecs_t workDuration, nsec
     return ScheduleResult::Scheduled;
 }
 
+void VSyncDispatchTimerQueueEntry::addPendingWorkloadUpdate(nsecs_t workDuration,
+                                                            nsecs_t earliestVsync) {
+    mWorkloadUpdateInfo = {.earliestVsync = earliestVsync, .duration = workDuration};
+}
+
+bool VSyncDispatchTimerQueueEntry::hasPendingWorkloadUpdate() const {
+    return mWorkloadUpdateInfo.has_value();
+}
+
 void VSyncDispatchTimerQueueEntry::update(VSyncTracker& tracker, nsecs_t now) {
-    if (!mArmedInfo) {
+    if (!mArmedInfo && !mWorkloadUpdateInfo) {
         return;
     }
+
+    if (mWorkloadUpdateInfo) {
+        mEarliestVsync = mWorkloadUpdateInfo->earliestVsync;
+        mWorkDuration = mWorkloadUpdateInfo->duration;
+        mWorkloadUpdateInfo.reset();
+    }
+
     auto const nextVsyncTime =
             tracker.nextAnticipatedVSyncTimeFrom(std::max(mEarliestVsync, now + mWorkDuration));
     mArmedInfo = {nextVsyncTime - mWorkDuration, nextVsyncTime};
@@ -192,7 +208,7 @@ void VSyncDispatchTimerQueue::rearmTimerSkippingUpdateFor(
     std::optional<std::string_view> nextWakeupName;
     for (auto it = mCallbacks.begin(); it != mCallbacks.end(); it++) {
         auto& callback = it->second;
-        if (!callback->wakeupTime()) {
+        if (!callback->wakeupTime() && !callback->hasPendingWorkloadUpdate()) {
             continue;
         }
 
@@ -291,6 +307,15 @@ ScheduleResult VSyncDispatchTimerQueue::schedule(CallbackToken token, nsecs_t wo
         }
         auto& callback = it->second;
         auto const now = mTimeKeeper->now();
+
+        /* If the timer thread will run soon, we'll apply this work update via the callback
+         * timer recalculation to avoid cancelling a callback that is about to fire. */
+        auto const rearmImminent = now > mIntendedWakeupTime;
+        if (CC_UNLIKELY(rearmImminent)) {
+            callback->addPendingWorkloadUpdate(workDuration, earliestVsync);
+            return ScheduleResult::Scheduled;
+        }
+
         result = callback->schedule(workDuration, earliestVsync, mTracker, now);
         if (result == ScheduleResult::CannotSchedule) {
             return result;

services/surfaceflinger/Scheduler/VSyncDispatchTimerQueue.h

@@ -64,6 +64,13 @@ public:
     // This moves the state from armed->running.
     // Store the timestamp that this was intended for as the last called timestamp.
     nsecs_t executing();
+
+    // Adds a pending upload of the earliestVSync and workDuration that will be applied on the next
+    // call to update()
+    void addPendingWorkloadUpdate(nsecs_t workDuration, nsecs_t earliestVsync);
+
+    // Checks if there is a pending update to the workload, returning true if so.
+    bool hasPendingWorkloadUpdate() const;
     // End: functions that are not threadsafe.
 
     // Invoke the callback with the two given timestamps, moving the state from running->disarmed.
@@ -88,6 +95,12 @@ private:
     std::optional<ArmingInfo> mArmedInfo;
     std::optional<nsecs_t> mLastDispatchTime;
 
+    struct WorkloadUpdateInfo {
+        nsecs_t duration;
+        nsecs_t earliestVsync;
+    };
+    std::optional<WorkloadUpdateInfo> mWorkloadUpdateInfo;
+
     mutable std::mutex mRunningMutex;
     std::condition_variable mCv;
     bool mRunning GUARDED_BY(mRunningMutex) = false;

services/surfaceflinger/tests/unittests/VSyncDispatchTimerQueueTest.cpp

@@ -89,15 +89,18 @@ public:
 
     void advanceBy(nsecs_t advancement) {
         mCurrentTime += advancement;
-        if (mCurrentTime >= mNextCallbackTime && mCallback) {
+        if (mCurrentTime >= (mNextCallbackTime + mLag) && mCallback) {
             mCallback();
         }
     };
 
+    void setLag(nsecs_t lag) { mLag = lag; }
+
 private:
     std::function<void()> mCallback;
     nsecs_t mNextCallbackTime = 0;
     nsecs_t mCurrentTime = 0;
+    nsecs_t mLag = 0;
 };
 
 class CountingCallback {
@@ -658,6 +661,46 @@ TEST_F(VSyncDispatchTimerQueueTest, helperMoveAssign) {
     cb1.cancel();
 }
 
+// b/154303580
+TEST_F(VSyncDispatchTimerQueueTest, skipsSchedulingIfTimerReschedulingIsImminent) {
+    Sequence seq;
+    EXPECT_CALL(mMockClock, alarmIn(_, 600)).InSequence(seq);
+    EXPECT_CALL(mMockClock, alarmIn(_, 1200)).InSequence(seq);
+    CountingCallback cb1(mDispatch);
+    CountingCallback cb2(mDispatch);
+
+    EXPECT_EQ(mDispatch.schedule(cb1, 400, 1000), ScheduleResult::Scheduled);
+
+    mMockClock.setLag(100);
+    mMockClock.advanceBy(620);
+
+    EXPECT_EQ(mDispatch.schedule(cb2, 100, 2000), ScheduleResult::Scheduled);
+    mMockClock.advanceBy(80);
+
+    EXPECT_THAT(cb1.mCalls.size(), Eq(1));
+    EXPECT_THAT(cb2.mCalls.size(), Eq(0));
+}
+
+// b/154303580.
+// If the same callback tries to reschedule itself after it's too late, timer opts to apply the
+// update later, as opposed to blocking the calling thread.
+TEST_F(VSyncDispatchTimerQueueTest, skipsSchedulingIfTimerReschedulingIsImminentSameCallback) {
+    Sequence seq;
+    EXPECT_CALL(mMockClock, alarmIn(_, 600)).InSequence(seq);
+    EXPECT_CALL(mMockClock, alarmIn(_, 930)).InSequence(seq);
+    CountingCallback cb(mDispatch);
+
+    EXPECT_EQ(mDispatch.schedule(cb, 400, 1000), ScheduleResult::Scheduled);
+
+    mMockClock.setLag(100);
+    mMockClock.advanceBy(620);
+
+    EXPECT_EQ(mDispatch.schedule(cb, 370, 2000), ScheduleResult::Scheduled);
+    mMockClock.advanceBy(80);
+
+    EXPECT_THAT(cb.mCalls.size(), Eq(1));
+}
+
 class VSyncDispatchTimerQueueEntryTest : public testing::Test {
 protected:
     nsecs_t const mPeriod = 1000;
@@ -817,6 +860,19 @@ TEST_F(VSyncDispatchTimerQueueEntryTest, reportsScheduledIfStillTime) {
     EXPECT_THAT(entry.schedule(1200, 500, mStubTracker, 0), Eq(ScheduleResult::Scheduled));
 }
 
+TEST_F(VSyncDispatchTimerQueueEntryTest, storesPendingUpdatesUntilUpdate) {
+    static constexpr auto effectualOffset = 200;
+    VSyncDispatchTimerQueueEntry entry(
+            "test", [](auto, auto) {}, mVsyncMoveThreshold);
+    EXPECT_FALSE(entry.hasPendingWorkloadUpdate());
+    entry.addPendingWorkloadUpdate(100, 400);
+    entry.addPendingWorkloadUpdate(effectualOffset, 700);
+    EXPECT_TRUE(entry.hasPendingWorkloadUpdate());
+    entry.update(mStubTracker, 0);
+    EXPECT_FALSE(entry.hasPendingWorkloadUpdate());
+    EXPECT_THAT(*entry.wakeupTime(), Eq(mPeriod - effectualOffset));
+}
+
 } // namespace android::scheduler
 
 // TODO(b/129481165): remove the #pragma below and fix conversion issues