diff --git a/MobileGL/MG_Remote/Transport/Ring.cpp b/MobileGL/MG_Remote/Transport/Ring.cpp index 0164392b..3e714a01 100644 --- a/MobileGL/MG_Remote/Transport/Ring.cpp +++ b/MobileGL/MG_Remote/Transport/Ring.cpp @@ -500,6 +500,10 @@ namespace MobileGL::MG_Remote::Transport { // The wake target for the duration of one wait; 0 again afterwards, so a later wait that // needs any progress (or one outside this class on the same flag) is rung as before. namespace { + // A wake target with this bit names a present-ack serial rather than an appliedSeq. + // Neither counter comes near 2^63, so the tag cannot collide with a real sequence. + constexpr std::uint64_t kWakeTargetPresentAck = 1ull << 63; + struct ScopedWakeTarget { std::atomic& target; ScopedWakeTarget(std::atomic& t, std::uint64_t seq) : target(t) { @@ -555,6 +559,10 @@ namespace MobileGL::MG_Remote::Transport { return SessionWait::TimedOut; } RingControl* control = m_control; + // P15: a present-credit wait is rung only by the credit (or a full event ring), not by + // every drain batch the server retires meanwhile - those were futile wake-ups of the + // GL thread and a futex syscall per batch on the apply thread (FCL inproc: 0.056 ms/frame). + const ScopedWakeTarget wake(control->producerWakeSeq, kWakeTargetPresentAck | serial); return Park( [control, serial] { return Watermark::Reached(control->Progress.presentAckSerial, serial) || @@ -690,9 +698,14 @@ namespace MobileGL::MG_Remote::Transport { std::atomic_thread_fence(std::memory_order_seq_cst); if (m_control->producerParked.load(std::memory_order_acquire) == 0) return; const std::uint64_t target = m_control->producerWakeSeq.load(std::memory_order_seq_cst); - if (target != 0 && !Watermark::Reached(m_control->Progress.appliedSeq, target) && - m_control->eventRingFull.load(std::memory_order_acquire) == 0) + const bool eventRingFull = m_control->eventRingFull.load(std::memory_order_acquire) != 0; + if ((target & kWakeTargetPresentAck) != 0) { + if (!Watermark::Reached(m_control->Progress.presentAckSerial, target & ~kWakeTargetPresentAck) && + !eventRingFull) + return; + } else if (target != 0 && !Watermark::Reached(m_control->Progress.appliedSeq, target) && !eventRingFull) { return; + } m_peerBell->Notify(); } } diff --git a/MobileGL/MG_Remote/Transport/Ring.h b/MobileGL/MG_Remote/Transport/Ring.h index 09139317..5b197218 100644 --- a/MobileGL/MG_Remote/Transport/Ring.h +++ b/MobileGL/MG_Remote/Transport/Ring.h @@ -168,7 +168,8 @@ namespace MobileGL::MG_Remote::Transport { // ---- the parked producer's wake target ------------------------------- // // What a parked client is waiting for, so the server rings only when it is there: 0 = - // any progress (present credit, ring space, events), otherwise the appliedSeq it needs. + // any progress (present credit, ring space, events), otherwise the appliedSeq it needs - + // or, with bit 63 set, the present-ack serial a present-credit wait needs (Ring.cpp). // Without it every record the server applied while the client waited for a later one // was a socket write and a futile wake-up. Written by the client before it parks (and // reset to 0 after), read by the server only after it saw producerParked set; on a link diff --git a/MobileGL/MG_Test/Wire/SessionTest.cpp b/MobileGL/MG_Test/Wire/SessionTest.cpp index 029ea4bf..594704ac 100644 --- a/MobileGL/MG_Test/Wire/SessionTest.cpp +++ b/MobileGL/MG_Test/Wire/SessionTest.cpp @@ -683,6 +683,43 @@ TEST(SessionTest, DrainingAFullEventRingClearsTheLatchAndRingsTheServer) { EXPECT_EQ(serverBell.rings.load(), 1u); } +// P15: a client parked for the present credit is rung by the credit (or a full event ring) and +// by nothing else. Every drain batch the server retires while the GL thread waits for the +// previous present used to ring it: a futile wake-up of the GL thread and a futex syscall on the +// apply thread per batch (FCL inproc: 0.056 ms/frame). +// +// Red-once: drop the ScopedWakeTarget from WaitForPresentAckOrEventBacklog and the retire below +// rings (rings == 1 before the credit). +TEST(SessionTest, AClientParkedOnThePresentCreditIsRungOnlyByTheCreditOrABacklog) { + auto session = std::make_shared(); + ASSERT_TRUE(session->Build(TestSizes())); + CountingBell clientBell; + RingControl* serverControl = session->serverSegments.CmdControl(); + session->consumer.Attach(serverControl, &session->cmdConsumer, &clientBell, + &session->serverTransport->SelfDoorbell(), kDefaultSpinUs); + + std::atomic released{false}; + std::thread parked([session, &released] { + (void)session->producer.WaitForPresentAckOrEventBacklog(2, 5000); + released.store(true, std::memory_order_release); + }); + while (session->Control().producerParked.load() == 0 && !released.load()) { + std::this_thread::yield(); + } + + session->consumer.RetireThrough(0); + session->consumer.CompleteFrame(1); + EXPECT_EQ(clientBell.rings.load(), 0u) << "progress the parked client does not wait for rang it"; + + session->consumer.ReturnPresentCredit(2); + EXPECT_EQ(clientBell.rings.load(), 1u); + + // The counting bell wakes nobody; release the waiter through the real one. + session->serverTransport->PeerDoorbell().Notify(); + parked.join(); + EXPECT_TRUE(released.load()); +} + // --------------------------------------------------------------------------- // kRecPad (R-9's last sentence, and the one with no other detector) // ---------------------------------------------------------------------------