# P5d round 3, package T - transport: apply loop idle poll, clock-free spin, park counters Worktree `MobileGL-p5d-T`, branch `p5d-T`, base `85362a85`. Not built, not committed. Patch: `scratchpad/p5d/patches/T.patch` (13 files, +489/-25). ## What changed, per file ### `MG_Remote/Server/ServerLoop.{h,cpp}` - item 1, the idle poll's mutex - New `std::atomic m_controlPosted` replaces a `std::condition_variable` of the same name that had **no waiter and no notifier anywhere in the tree** (`m_controlDone` carries the whole handshake). `ControlIsPending()` is now one acquire load, no lock, and moved to the public section (a test reads it - see red-once); `PumpControlRequest()` gained a fast negative on the same load before it touches `m_controlMutex`. - Protocol: the poster stores the shadow **last of the four fields, with release**, inside the existing critical section in `RunOnApplyThread`; the pump clears it **under the lock, when it takes the request**; the C2 exit block clears it under the same lock as the `m_running` clear. The C2 critical section, the NOT_INITIALIZED answer at exit, publish-then-ring and the re-entrant inline arm are unchanged in meaning; `ready` now reads only atomics. - Why: simpleperf at `56a77348` put `mutex::lock` 12.4 + `mutex::unlock` 13.6 + the two `pthread_mutex_*` halves at ~49% of `mgl-srv-apply`, with `ApplyThreadMain` the **direct** caller of 58-60% and 99.8% of `pthread_mutex_lock`-via-`mutex::lock`. Those are these two call sites, twice per idle iteration; most of `@plt` 6.1% goes with them. - Item 2 (`DrainRing` on an empty ring): **audited, no code change**, finding written into the function. `ApplyOne` -> `RingConsumer::Pop`, whose first act is one acquire load of `cmdHead` against `m_localTail` (`Ring.cpp:222-232`); `applied == 0` skips the retire block. No mutex, no clock, no allocation. ### `MG_Remote/Transport/Doorbell.{h,cpp}` - item 3, the clock-free spin - `Detail::g_spinItersPerUs` + `Detail::CalibrateSpinItersPerUs()`, and an inline `SpinItersPerUs()` that is a relaxed load and a predicted branch - deliberately **not** a function-local static, whose guard-variable load on every `Wait` is the cost this removes. `Wait()`'s spin phase is now `SpinItersPerUs() * spinUs` iterations of `ready()` + `CpuRelax()` and reads **no clock at all**; the deadline is computed once, at the top of the park phase, and `kWaitForever` reads no clock even there. - Calibration: two passes of 8192 `load(acquire)` + `CpuRelax()` timed with `steady_clock`, faster pass wins (a preempted pass reads absurdly slow, and under-spinning costs a park), clamped to [4, 2000] iters/us with a 200 fallback for a clock too coarse to see the probe; a race between two calibrators stores two legitimate budgets. - **Overshoot bound** (documented in the file): the old loop overshot by at most one batch of 64 yields; this one by the calibration error - a DVFS drop or little-core migration stretches it by the frequency ratio (~3-4x here, so 50 us can reach ~200 us), a preemption adds one slice as before, and the probe omits the caller's `ready()`, so a real iteration is *dearer* than a probed one and the spin ends early by that factor. Not a correctness bound: `ready()` is re-tested every iteration and the deadline is still real-clock. Stated drift: the spin is no longer charged against `timeoutMs`, so a wait is bounded by `timeoutMs + budget` - 50 us against millisecond-scale timeouts, and erring long is the safe direction for a timeout. - `Doorbell::ParkEntries()` + a private relaxed counter incremented at the one place a waiter stops spinning and blocks - unconditional (one relaxed add on a path about to enter a condvar or `poll()`), so a test can read it without `MOBILEGL_PIPE_STATS`. - Why: `steady_clock::now()` 2.34 self / 5.91 incl + `__kernel_clock_gettime` 2.37 + `clock_gettime` 1.94 = ~6.6% of the GL thread at 900+ waits/frame, for a duration that is a hint. `WaitForApplied`'s common path paid 2+ reads per wait; it now pays zero unless it parks. ### `MG_Remote/Transport/SessionRings.h`, `Transport/Ring.cpp` - item 4, client half - `SessionProducer::Waits()` (relaxed atomic, incremented in `Park` *after* the two refusals that never reach the bell) and `Parks()` (delegates to the self bell's `ParkEntries()` - only `Doorbell::Wait` knows whether its budget ran out). Atomic because `ClientSession::Stop`'s drain waits through this producer from the tearing-down thread; it is never copied (checked). ### `MG_Util/Metrics/PipeStats.{h,cpp}`, `Client/EmitTables.cpp`, `Client/ClientSession.cpp` - Four gauges, **all inside existing `#if MOBILEGL_PIPE_PUSH` blocks**: `ServerWaits`, `ServerParks`, `ClientWaits`, `ClientParks`. New `wait[srv= srvpark= cli= clipark=]` group on the one `MGPipe stats:` line (it closes the `emit[` bracket the ring gauges were riding), same four under long names in the JSON dump, run totals like the gauges above them. Both server numbers, not one: `srv` is `ParkCount()`'s long-standing meaning (an intention, counted on the way toward a park - ServerLoopTest's C10 note), `srvpark` is a real block. - No new include edge into MG_Util: the existing `PublishGauge` pattern already crosses that way (`EmitTables.cpp:1000`). The client pair is published from `EmitPresent`; the **server pair by the apply thread from its own loop**, because CONTRACT-P5C rule E layer 2 forbids a GL-thread read of `ServerLoop`'s memory - PipeStats is the meeting point precisely because it is below both roles. Both publishes sit behind `PipeStats::Enabled()`. - `ClientSession::LogWireLedger` also gained `cliwait=`/`clipark=`: that line is in **every** split private log, and the round-3 profile runs did not set `MOBILEGL_PIPE_STATS`. ## Red-once | change | goes red as | |---|---| | shadow **set** side dropped | `ServerLoopTest.AControlRequestRunsOnTheApplyThreadAndUnparksIt` (existing) - the predicate sees nothing, the poster blocks, the case says so within 5 s | | shadow **clear** side dropped | `ServerLoopTest.TheControlShadowIsClearedSoTheLoopParksAgainAfterARequest` (**new**) - `ready` stays true, the loop spins for ever and never stores `consumerParked`, so the second `WaitUntilTrulyParked()` times out; plus a direct `ControlIsPending()` read | | spin budget zero / calibration broken | `InProcessTransportTest.DoorbellSpinsWithoutParkingWhenReadyFlipsInsideTheBudget` (**new**) - `ParkEntries()` moves although the condition flipped on the 8th probe | | spin never ends / park unreachable | `InProcessTransportTest.DoorbellStillParksWhenTheSpinBudgetRunsOut` (**new**) - `ParkEntries()` does not move | | `wait[]` renamed or dropped | `PipeStatsTest.SummaryLineCarriesEveryClassAndGate` (extended) | Kept passing by inspection: `DoorbellWakesAParkedWaiter`, `DoorbellReturnsImmediatelyWhenAlreadyReady`, `DoorbellTimesOutWhenNothingHappens` (the uncharged spin only makes elapsed larger than its `>= 20 ms`), `ShutdownUnparksAWaiterWithNoDeadline` (fence protocol and `Dead()` unchanged), `RingTest`, `SessionTest`. ## G1 / monolith impact **None.** Every touched file is MG_Remote (built only under `MOBILEGL_BUILD_DISAGGREGATED`, `CMakeLists.txt:547-578`), a test, or - for `PipeStats.{h,cpp}`, which *is* in the pull build - edited exclusively inside pre-existing `#if MOBILEGL_PIPE_PUSH` regions (`PipeStats.h:186-236`, `PipeStats.cpp:484-545`, `:600-622`), so `MOBILEGL_PIPE_PUSH=OFF` compiles no added token and the pull build's text is unmoved. No include-closure probe is touched. ## Risks 1. **The spin's duration bound is weaker** (see above): it burns CPU before a park, cannot change a result, and the [4, 2000] clamp plus probed-cheaper-than-real bounds both directions. The probe reads the clock twice per pass, once per process (2-30 us) - no steady-state path. 2. `m_controlPosted` is a second copy of one mailbox bit. Disagreement is a performance fault in both directions (a spin that never parks; a wakeup the doorbell's own re-test recovers), never a lost request - the pump re-checks `m_controlPending` under the lock and clears a stale shadow. 3. `wait[` closes `emit[` earlier than before, but `PipeStatsWindow::CounterOrAbsent` matches `" name="` / `"[name="`, so `fbe`/`sve`/`csom`/`mpr`/`rsp`/... are unaffected and the four new names collide with nothing (`cli=` cannot match `clipark=`). ## What I did not do - Did not build, run, commit or push. No `Co-Authored-By` anywhere. - Did not hoist `ServerSession::Consumer()` / `Applier()` out of `DrainRing` - two out-of-line calls per idle iteration, a small win, but caching them touches `ApplyThreadMain`'s opening lines, which package D owns. Flagged for round 4 / D; nothing of `OnApplyThread`, `m_applyThreadId` or those lines is touched here. Also left: the per-park `steady_clock::now()` under `kWaitForever` (one read per real park), to keep the park phase structurally identical. - Did not update `docs/Disaggregated/ARCHITECTURE.md:623`, which enumerates the summary line's counters and now omits `wait[]` - left to the round's docs pass so four packages do not collide. Same for stale `Doorbell.h:NNN` references nearby: already drifted at `85362a85`. - The brief's "visible to `ControlIsPending` **without the pump**" is not directly constructible: `m_callerMutex` allows one outstanding request and a live apply thread pumps every iteration, so no window exists in which a posted-but-unpumped request is observable by a third thread. The new test pins the clear side (which nothing else could see), the existing unpark test the set side. --- # Fix round (after review) Two reviews, no blockers, three distinct majors (two of them the same defect seen from two sides) and five minors. Every one is addressed below; nothing was widened. Patch regenerated from `MobileGL-p5d-T`: 16 files, +749/-40 (was 13 / +489/-25). Still not built, not committed. ## Majors ### M1 (review 1) - the clear side's red-once did not fire **Upheld.** `PumpControlRequest` clears the shadow twice - at the take (`ServerLoop.cpp`, under the lock, when the work is handed out) and defensively on the next pump when it finds the shadow set with `m_controlPending` false. The old case asserted only the END STATE ("the shadow is clear after the request was answered, and the loop parks again"), and the defensive clear supplies that end state on the very next iteration, so deleting the take-clear left the case green. The reviewer's trace is exact. **Fix: observe the window, keep both clears.** `ServerLoopTest.TheControlShadowIsClearedSoTheLoop ParksAgainAfterARequest` is replaced by `ServerLoopTest.TheControlShadowIsClearedWhenThePumpTakesTheRequest`. The posted work now blocks on a gate the test thread holds; the test waits until the work is really running and reads `ControlIsPending()` in that window. There `m_controlPending` is still true and no pump is running, so the ONLY thing that can make the accessor answer false is the clear at the take - delete it and the read returns true on every run, deterministically. The end-state assertions (answer returned, `WaitUntilTrulyParked()` again) are kept as the symptom an operator would see, with their comment now saying which clear each one pins. The defensive clear is kept: it is a genuine self-heal for a shadow that outlived its request, and with the window observed directly it no longer masks anything. ### M2 (both reviews) - `Parks()` was not the subset of `Waits()` that everything claimed **Upheld, in all three of its parts**, and the fix is one mechanism for all three. 1. *Counted per `Park()`, not per `Wait()`.* The increment sat inside the park-retry loop, so a wait that parked, woke on a remembered `Notify`, re-tested false and parked again counted twice - a routine path, exactly as review 2 describes. 2. *Counted for `Park(0)`.* When the remaining deadline rounds down to 0 ms the loop spins through the counting site without ever blocking. 3. *Counted off a shared bell.* `SessionProducer::Parks()` returned `m_selfBell->ParkEntries()`, and `ClientSession::Start` installs that same object as the encoder's stage-retirement bell (`PipeWireCodec::SetStageRetirementDoorbell`). Those waits never touch `m_waits`, and with `spinUs == 0` they now park unconditionally, so `clipark` could print larger than `cli` on any lane with stage retirement (the SmallRing / E3(e) lane asserts `stageReclaimWaits >= 1`). **Fix.** - `Doorbell::Wait` gained a fifth, defaulted parameter `std::atomic* parkTally`. It is bumped at the same site as the bell's own counter, and both are now guarded by `chunkMs != 0 && !countedPark`, where `countedPark` is a local hoisted above the park loop. One park per `Wait`, and only when the call really blocks. - `SessionProducer` owns `m_parks` and `SessionProducer::Park` passes `&m_parks`, so the client pair counts the same events on both sides. `Parks()` reads it; `ParkEntries()` is no longer consulted by any ledger. - The server half got the same treatment even though its bell has no second waiter today: `ServerLoop::m_parkBlocks` (+ `ParkBlockCount()`, reset in m-3's per-session block beside `m_parks`) is passed into the loop's one `Wait`, so `srvpark <= srv` cannot be broken later by anything else waiting on the consumer doorbell (`SessionConsumer::WaitForWork` waits on that same bell). - Comments corrected where they asserted the old, false relation: `Doorbell::ParkEntries()`'s block now says it is per BELL and names the two waiters that share the client's, `SessionRings.h`'s `Waits()`/`Parks()` block says why the tally is ours and not the bell's, and `PipeStats.h` says the subset relation holds only because both halves are counted by the same path. **Red-once for the fix** (both new, both deterministic): - `InProcessTransportTest.AWaitThatParksTwiceOnARememberedNotifyIsStillOnePark` - `Notify()` with nobody waiting, then one `Wait` whose condition never flips: the first `Park` returns at once on the remembered wakeup and the loop parks again. `ParkEntries()` must move by exactly 1. Revert `countedPark` and it moves by 2. - `SessionTest.TheProducersParkLedgerCountsItsOwnWaitsAndNotTheBellsOtherWaiters` - a foreign `Wait` on `clientTransport->SelfDoorbell()` (spinUs 0, condition never true - the encoder's exact shape) must move neither `producer.Waits()` nor `producer.Parks()`; the producer's own 20 ms `WaitForApplied` must move both by exactly one. Revert `Parks()` to `ParkEntries()` and the first `EXPECT_EQ` on `Parks()` goes red. ## Minors ### m1 - `spinUs == 0` silently changed meaning (both reviews) **Upheld; stated, and the probe no longer runs on that path.** The old batched loop ran 63 iterations before its first clock read could end it; the new budget is `itersPerUs * spinUs`, so zero means zero. That is now an explicit `spinUs == 0 ? 0ull : ...` short-circuit with a comment naming the delta, both callers that pass 0 (PipeWireCodec's SEG_STAGE retirement wait, and any lane with `MOBILEGL_IPC_SPIN_US=0`), and the reason it is deliberate rather than accidental. The short-circuit also keeps a zero-spin caller from paying the one-time calibration probe - review 1's second half of the finding. `PipeWireCodec.cpp`'s `0` is left as it is: the retirement wait is rare, already slow, and wants the park; changing it is a codec decision, not this package's. ### m2 - `ControlIsPending()` no longer means what its name says **Upheld; documented rather than renamed.** The brief names the accessor, and three call sites' worth of churn buys nothing, so the header block now states the predicate literally - "posted and not yet taken" - says that it answers false for the whole duration of `work(user)` while `m_controlPending` is still true, says it is a PARK PREDICATE and not a liveness query, and notes that the only in-tree reader besides the test is the apply thread's own `ready` lambda, which by construction cannot be inside that window. The new test in M1 is built on exactly that window, so the documented meaning is now the tested one. ### m3 - the calibration clamp's two opposite readings (review 2) **Upheld.** `perUs` truncates to 0 for any pass longer than 8.192 ms, i.e. the heavily preempted probe, and that landed on the same `best == 0` arm as "no usable clock" and installed 200 iters/us - 50x the floor - on the machine least able to afford it. `CalibrateSpinItersPerUs` now carries `sawUsableClock`, set whenever `elapsedNs > 0`: measured-but-slow clamps UP to `kMinSpinItersPerUs`, and `kFallbackSpinItersPerUs` is reserved for the case where no pass was measurable at all. The `Doorbell.h` paragraph already described this rule ("a probe that was preempted cannot produce a budget smaller than kMinSpinItersPerUs"); it is now true. ### m4 - `Doorbell.h:NNN` citations invalidated by this package's insert (review 2) **Upheld - and the first report's "already drifted at 85362a85" was wrong.** Checked at baseline: `139-178` really was the spin loop through the `Dead()` arm, `186-193` really was the `};` plus NotifyIfParked's PRECONDITION, `211-221` really was CondVarDoorbell/`Kill`. The +60 lines moved all three, and `check_doc_citations.py` cannot see it because the file only grew. Every in-tree citation is re-pointed to SYMBOL NAMES rather than to new line numbers, so the next insert cannot break them again: `ServerLoop.cpp` (x3), `ServerLoop.h`, `SessionRings.h` (x3), `EventRing.h`, `ClientSession.cpp`, `ClientSession.h` (x2) and `CONTRACT-P5.md:355`. The checker only resolves `path:line` pairs, so dropping the numbers leaves every gate green. ### m5 (review 2's note, not filed as a finding) - `srv=0` under `spawn` Not a defect of this patch, but the new `PipeStats.h` comment asserted "PipeStats is the meeting point precisely because it is below both", which is true in one transport mode. The comment now says so: under `spawn` the server pair is published into the server process's PipeStats and the client's line prints `srv=0 srvpark=0` for the whole run - the same property `rsp` already has - and the round this pair was added for is `inproc`. ## Red-once table, updated | change | goes red as | |---|---| | shadow **set** side dropped | `ServerLoopTest.AControlRequestRunsOnTheApplyThreadAndUnparksIt` (existing) | | shadow **take-clear** dropped | `ServerLoopTest.TheControlShadowIsClearedWhenThePumpTakesTheRequest` (**new, rewritten**) - reads `ControlIsPending()` while the work is still running, where the defensive clear cannot cover for it | | park counted per `Park` instead of per `Wait` | `InProcessTransportTest.AWaitThatParksTwiceOnARememberedNotifyIsStillOnePark` (**new**) | | client parks read off the shared bell again | `SessionTest.TheProducersParkLedgerCountsItsOwnWaitsAndNotTheBellsOtherWaiters` (**new**) | | spin budget zero / calibration broken | `InProcessTransportTest.DoorbellSpinsWithoutParkingWhenReadyFlipsInsideTheBudget` | | spin never ends / park unreachable | `InProcessTransportTest.DoorbellStillParksWhenTheSpinBudgetRunsOut` | | `wait[]` renamed or dropped | `PipeStatsTest.SummaryLineCarriesEveryClassAndGate` (extended) | ## G1 / monolith impact **Still none.** The fix round touched `Doorbell.{h,cpp}`, `Ring.cpp`, `SessionRings.h`, `EventRing.h`, `ServerLoop.{h,cpp}`, `ClientSession.{h,cpp}`, `CONTRACT-P5.md` (all MG_Remote or a doc), three test files, and `PipeStats.h` - the last one inside the same pre-existing `#if MOBILEGL_PIPE_PUSH` region as before, comment text only. No new include edge; the new `Wait` parameter is defaulted, so the two call sites that do not pass a tally (`PipeWireCodec.cpp`, `SessionConsumer::Park` in `Ring.cpp`) are unchanged. ## Not changed, with reasons - **The defensive clear in `PumpControlRequest` stays.** Review 1 offered "drop it so the take-clear is load-bearing" as an alternative; dropping a self-heal to make a test work is the wrong trade when the test can observe the real window instead. - **`ControlIsPending` is not renamed** (see m2). - **`PipeWireCodec.cpp`'s `spinUs == 0` is not changed** (see m1). - **`ARCHITECTURE.md:623`'s summary-line enumeration** still omits `wait[]` - left to the round's docs pass, as before, so four packages do not collide on one file. The `Doorbell.h` citations were different: those were made wrong BY this patch, so this patch fixes them. - Did not build, run, commit or push. No attribution lines anywhere.