From dd4afb0faf6f8fb3bf5571895e3dd6810852d80f Mon Sep 17 00:00:00 2001 From: rereview Date: Fri, 11 Sep 2026 13:05:05 -0400 Subject: [PATCH] [Docs, Test] (MG_Remote, Wire): state the five sequence watermarks, the late-never-early batching rule and the pad-does-not-count rule on RingControl, and pin all three with five cases --- MobileGL/MG_Remote/Transport/Ring.h | 43 ++++++++ MobileGL/MG_Test/Wire/RingTest.cpp | 152 ++++++++++++++++++++++++++++ 2 files changed, 195 insertions(+) diff --git a/MobileGL/MG_Remote/Transport/Ring.h b/MobileGL/MG_Remote/Transport/Ring.h index fcb216ab..d6be4af0 100644 --- a/MobileGL/MG_Remote/Transport/Ring.h +++ b/MobileGL/MG_Remote/Transport/Ring.h @@ -41,6 +41,49 @@ // (Records.def / PipeCalls.def) is a separate deliverable; the ring itself // only needs kind/flags/size, so it can carry the real records the day they // land without changing shape. +// +// --------------------------------------------------------------------------- +// THE FIVE WATERMARKS (P5 R-9). One sentence each, and they are a contract: +// every one of the five was declared here at P0 and written by nobody but +// InitRingControl, so until P5 there was nothing to disagree with. +// +// submittedSeq Advanced by the PRODUCER after it publishes. NOBODY +// WAITS ON IT - it is diagnostic, the answer to "how far +// ahead of the server is the client right now". +// appliedSeq Advanced by the CONSUMER for EVERY SINGLE RECORD it +// applies. The client's verb barrier and every reply wait +// read it, so it is the one watermark P5 FORBIDS BATCHING: +// the sixty-four-record batching this ring was designed +// for makes a waiter block on work that already ran, or - +// far worse - resume on work that has not. +// retiredSeq Advanced by the CONSUMER once it has finished with the +// SEG_STAGE bytes a record referenced. The staging +// allocator reclaims behind it, and nothing else may. +// completedFrameSerial Advanced by the SERVER when a present completes. What +// recycling and ageing wait on; it trails appliedSeq by +// the GPU's own depth and must never be conflated with it. +// presentAckSerial Advanced by the SERVER when it returns a present credit. +// The client's present throttle waits on it; it is the +// only back-pressure that bounds latency rather than bytes. +// +// Every wait on all five is `>=`, never `==`: a waiter that tests equality +// misses the wakeup the moment a producer or consumer moves by more than one. +// +// BATCHING MAY ONLY MAKE A WATERMARK LATE. All five except appliedSeq may be +// published lazily, because a waiter that sees an old value waits longer than +// it had to and is still correct. NONE of them may ever be published EARLY: a +// watermark that reports more than was actually done turns every waiter into a +// silent use of work that has not happened, and there is no checksum anywhere +// on this ring that would catch it. +// +// kRecPad DOES NOT ADVANCE SEQ. A wrap filler is framing, not a record: it has +// no opcode, no payload meaning and no reply slot. Both sides must skip it +// BEFORE counting. If one side counts it and the other does not, the two seq +// spaces drift by one at every wrap - and because seq IS the reply-slot id +// (P5 R-3), a drifted seq silently reads another call's answer rather than +// failing. Nothing on this ring would detect that, which is why the rule is +// stated here rather than left to each side's loop. +// --------------------------------------------------------------------------- #pragma once diff --git a/MobileGL/MG_Test/Wire/RingTest.cpp b/MobileGL/MG_Test/Wire/RingTest.cpp index f38ea98a..49b7b85d 100644 --- a/MobileGL/MG_Test/Wire/RingTest.cpp +++ b/MobileGL/MG_Test/Wire/RingTest.cpp @@ -517,3 +517,155 @@ TEST(RingTest, DoorbellHandoffWakesBothSidesOnEveryPublish) { EXPECT_TRUE(ok.load()); EXPECT_TRUE(ring.Invariants()); } + +// --------------------------------------------------------------------------- +// P5 R-9: the five watermarks and the pad rule, from Ring.h's header comment. +// +// Nothing in the tree advanced any of the five before P5 - InitRingControl zeroed +// them and that was all - so these five cases pin the RULES against the sessions +// that are about to start writing them, rather than testing today's (absent) +// writers. Each one is the negative control for one sentence of that comment. +// --------------------------------------------------------------------------- + +// R-9, sentence 0: all five start at zero, so "has not moved" and "moved to zero" +// are the same state and a waiter that starts before its peer cannot be fooled by +// a stale non-zero value left over from a previous session. +TEST(RingTest, WatermarksAreAllZeroUntilSomeoneAdvancesThem) { + alignas(4096) RingControl control{}; + InitRingControl(control); + EXPECT_EQ(control.submittedSeq.load(), 0u); + EXPECT_EQ(control.appliedSeq.load(), 0u); + EXPECT_EQ(control.retiredSeq.load(), 0u); + EXPECT_EQ(control.completedFrameSerial.load(), 0u); + EXPECT_EQ(control.presentAckSerial.load(), 0u); + // ... while the two GENERATIONS start at one, because for them zero means + // "uninitialized" and must never be a legal value. The two conventions are + // opposite on purpose and are next to each other in the same struct. + EXPECT_EQ(control.serverEpoch.load(), 1u); + EXPECT_EQ(control.ringGeneration.load(), 1u); +} + +// R-9, "every wait is >=, never ==". Both sides advance in jumps - a consumer that +// applies two records before republishing, a server that completes two frames in one +// poll - so an equality test misses its wakeup and the waiter hangs until the next +// coincidence. This case is that hang, made deterministic. +TEST(RingTest, AWatermarkWaiterMustTestGreaterOrEqualRatherThanEqual) { + alignas(4096) RingControl control{}; + InitRingControl(control); + + const std::uint64_t mySeq = 7; + // The peer jumps straight past the value this waiter cares about. + control.appliedSeq.store(mySeq + 1, std::memory_order_release); + + const std::uint64_t seen = control.appliedSeq.load(std::memory_order_acquire); + EXPECT_FALSE(seen == mySeq) << "an equality waiter is still asleep at this point"; + EXPECT_TRUE(seen >= mySeq) << "the >= waiter this contract mandates has been released"; +} + +// R-9, appliedSeq's row: advanced by the consumer for EVERY SINGLE RECORD, and P5 +// forbids the 64-record batching the ring was designed for, because the verb barrier +// and every reply wait read it. The invariant that must hold after each Pop is +// `appliedSeq == records applied so far` - not "eventually", every time. +TEST(RingTest, AppliedSeqAdvancesOncePerRecordAndIsNeverBatchedInP5) { + RingFixture ring(1024); + constexpr int kRecords = 12; + for (int i = 0; i < kRecords; ++i) { + ASSERT_TRUE(ring.WriteRecord(static_cast(i + 1), 16, + static_cast(i))); + } + + std::uint64_t applied = 0; + RingRecordView view{}; + while (ring.Consumer().Pop(view)) { + ++applied; + ring.Control().appliedSeq.store(applied, std::memory_order_release); + // The reader's guarantee, checked at EVERY record rather than at the end: + // a batched watermark would sit at 0 here for 63 of every 64 iterations, + // and a client barrier reading it would block on work that already ran. + EXPECT_EQ(ring.Control().appliedSeq.load(std::memory_order_acquire), applied); + } + EXPECT_EQ(applied, static_cast(kRecords)); + EXPECT_TRUE(ring.Invariants()); +} + +// R-9, "batching may only make a watermark LATE, never early". retiredSeq is the one +// the staging allocator reclaims behind, so a value published ahead of the actual +// drain hands live bytes back to the producer. Late is merely slow; early is a +// use-after-free that nothing on this ring checksums. +TEST(RingTest, ALazyWatermarkMayTrailTheWorkButMustNeverLeadIt) { + RingFixture ring(1024); + constexpr int kRecords = 8; + for (int i = 0; i < kRecords; ++i) { + ASSERT_TRUE(ring.WriteRecord(static_cast(i + 1), 16, + static_cast(i))); + } + + std::uint64_t drained = 0; + RingRecordView view{}; + while (ring.Consumer().Pop(view)) { + ++drained; + // A deliberately lazy publisher: only every third record. This is legal. + if (drained % 3 == 0) { + ring.Control().retiredSeq.store(drained, std::memory_order_release); + } + EXPECT_LE(ring.Control().retiredSeq.load(std::memory_order_acquire), drained) + << "retiredSeq ran ahead of the drain; those staged bytes are still live"; + } + // Trailing at the end is fine and is what "late" means. + EXPECT_LE(ring.Control().retiredSeq.load(), static_cast(kRecords)); + EXPECT_TRUE(ring.Invariants()); +} + +// R-9's last sentence, and the one with no other detector: kRecPad DOES NOT ADVANCE +// SEQ. A wrap filler is framing - no opcode, no payload, no reply slot - so a side +// that counts it drifts from the side that does not, by one per wrap, for ever. And +// because seq IS the reply-slot id (R-3), a drifted seq reads ANOTHER CALL'S ANSWER +// instead of failing. Here the ring is sized so the last record cannot fit before the +// wrap boundary, which forces the producer to emit a filler; the consumer must count +// the records and not the filler. +TEST(RingTest, AWrapFillerDoesNotAdvanceTheRecordSequence) { + RingFixture ring(256); + constexpr std::uint64_t kPayload = 56; // 8-byte header + 56 = 64 per record + + // Three records fill 192 of 256 bytes; the fourth needs 64 and only 64 remain, so + // it lands exactly at the boundary. The fifth is what forces the filler. + std::uint64_t written = 0; + for (int i = 0; i < 3; ++i) { + ASSERT_TRUE(ring.WriteRecord(static_cast(i + 1), kPayload, + static_cast(i))); + ++written; + } + const std::uint64_t headAfterThree = ring.Producer().LocalHead(); + + std::uint64_t popped = 0; + RingRecordView view{}; + while (ring.Consumer().Pop(view)) { + // Pop skips fillers by contract, so a pad must never reach a caller that is + // about to number it. If one ever does, that is the drift itself. + EXPECT_EQ(view.flags & kRecPad, 0u) << "a wrap filler reached the record counter"; + EXPECT_NE(view.kind, kRingPadRecordKind); + ++popped; + } + EXPECT_EQ(popped, written) << "the consumer numbered something the producer did not send"; + ring.Consumer().PublishApplied(); + ring.Consumer().PublishRetired(); + + // Now drive the producer across the wrap and prove a filler really was emitted: + // the head advances by MORE than the records' own bytes, and that surplus is the + // pad. The record count still has to match. + for (int i = 0; i < 3; ++i) { + ASSERT_TRUE(ring.WriteRecord(static_cast(i + 10), kPayload, + static_cast(i + 10))); + ++written; + } + const std::uint64_t headAfterSix = ring.Producer().LocalHead(); + EXPECT_GE(headAfterSix - headAfterThree, 3u * (kPayload + sizeof(RingRecordHeader))); + + while (ring.Consumer().Pop(view)) { + EXPECT_EQ(view.flags & kRecPad, 0u) << "a wrap filler reached the record counter"; + ++popped; + } + EXPECT_EQ(popped, written) + << "the two sides' sequence spaces have drifted by the fillers between them"; + EXPECT_TRUE(ring.Invariants()); +}