diff --git a/MobileGL/MG_Impl/Pipe/PipeFill.cpp b/MobileGL/MG_Impl/Pipe/PipeFill.cpp index 8e05a588..0c851f34 100644 --- a/MobileGL/MG_Impl/Pipe/PipeFill.cpp +++ b/MobileGL/MG_Impl/Pipe/PipeFill.cpp @@ -394,7 +394,39 @@ namespace MobileGL::MG_Pipe { // transform-feedback span, so clause 2 is false for it - and the verb is a no-op below // either way. Bool ClientVerbIsBarriered(MGPipeVerb verb, GLContext* ctx) { - const MGPWireOp op = WireOpForVerb(verb); + MGPWireOp op = WireOpForVerb(verb); + // ---- P5e (ra2): THE JOIN IS MANY-TO-ONE ON THE DRAW FAMILY, SO THE INVERSE IS NOT + // A FUNCTION, AND THE DEFAULT BELOW WAS ANSWERING FOR NINETEEN VERBS ----------------- + // + // MGP_VERB_OP_LIST carries ONE row for the whole draw family - `DrawVbo -> + // DrawArrays` - because that is the direction the SERVER needs: a draw_vbo record + // stamps a verb boundary and DrawArrays is the name it prints. Inverting it + // verb-first therefore answers kOpCount for DrawElements, DrawElementsBaseVertex, + // DrawElementsIndirect, MultiDrawElementsBaseVertex and every other indexed / + // instanced / multi / indirect verb - all of which emit exactly that same draw_vbo. + // + // The conservative default below reads "a verb the join does not know answers + // BARRIERED: fill it, wait for it". THE FILL HAPPENS AND THE WAIT DOES NOT. The wait + // is not decided here - it is decided per RECORD, by MGPipeBarriered(op, payload, + // st) at the publish (§2.1) - and the record is a draw_vbo, whose wait class is + // kWaitNone. So every indexed draw filled gPipeInputs while telling + // RefusePipeInputsTouchWhileApplierOwnsIt, through isBarrieredFill, that this thread + // was about to park behind it, and then ran on without parking. The apply thread was + // measured inside a record at that instant, and the abort it produced named the + // CLIENT's verb - a verb the applier cannot stamp, which is what identifies the + // writer (report §2). + // + // So the family's one row is applied to the family. Every other unknown verb keeps + // the conservative answer, which is now honest rather than hoped for: the fill site + // establishes quiescence before it writes (MGPipeValidateForVerb below), so a + // barriered fill no longer rests on a park that may never come. + static_assert(MGPipeVerbForWireOp(MGPWireOp::DrawVbo) == MGPipeVerb::DrawArrays, + "the draw family's representative row moved; the fallback below names " + "draw_vbo because that is the record every kDraw verb emits"); + if (op == MGPWireOp::kOpCount && + kMGPipeVerbClass[static_cast(verb)] == MGPipeVerbClass::kDraw) { + op = MGPWireOp::DrawVbo; + } if (op == MGPWireOp::kOpCount) return true; if (MGPipeWaitClassFor(op) != kWaitNone) return true; if (MGPipeCallClassFor(op) == kCtxVerb && ctx != nullptr && @@ -404,6 +436,40 @@ namespace MobileGL::MG_Pipe { return false; } + // ---- P5e (ra2), CONTRACT-P5E §3.5 AMENDED: A BARRIERED FILL MAKES ITSELF QUIESCENT ---- + // + // §3.5 exempted the residual fill of a BARRIERED record from the single-writer rule on + // the ground that "this thread is about to park behind it". That is a claim about the + // FUTURE, and the write is in the PRESENT: the order at the validate point is fill, + // then emit, then park, and between the fill and the park the apply thread is still + // draining the UNBARRIERED records the client ran ahead of. The claim was therefore + // never an argument about this instant, and the guard that took it - the + // `if (isBarrieredFill) return;` arm of RefusePipeInputsTouchWhileApplierOwnsIt - could + // not fire however wrong the fill was. + // + // This makes the claim TRUE instead of asserting it. §2.5's forced wait is exactly + // "publish nothing, wait for the applier to reach LastPublishedSeq, drain the reverse + // channel", which is the definition of the window the fill needs, and it already exists + // for glFinish and for BackendObject_Remote's forwarders. + // + // IT IS CALLED AT EVERY GL-THREAD WRITE INTO THE BLOCK, not once per verb, because the + // validate point writes the block in TWO phases that straddle record publication: the + // serial bump / stamp withdrawal / verb rename run BEFORE the emitters, and the 63-field + // walk of step 4 runs AFTER them - and the records the emitters published are records + // whose apply READS the block. One wait cannot cover both halves. + // + // THE ARM IS STATED, NOT INFERRED (ID-81): WaitForApplyToCatchUp's own first test is + // `m_runAheadArmed && m_started`, and m_runAheadArmed is the latch of + // `Transport != Monolith && Ipc.RunAhead && kMGPipeP5eRunAheadReady && the caps bit`. So + // on the monolith arm, on the pull build and under MOBILEGL_IPC_RUN_AHEAD=0 this is a + // call that returns, and the lockstep client's behaviour is unchanged byte for byte + // (G1) - under lockstep appliedSeq is already at LastPublishedSeq by construction. + void QuiesceApplierBeforeFill(const char* surface) { + MG_Remote::Client::ClientSession* session = MG_Remote::Client::ClientSession::Active(); + if (session == nullptr) return; // no session: this process has no applier to outrun + session->WaitForApplyToCatchUp(surface); + } + // Whether the LAST fill actually happened, i.e. whether the rows in the block describe // the verb in flight. Read by MGPipeNoteFrontendMutation, which refreshes one field of // that fill: with no fill behind it there is nothing to refresh and the write would be @@ -637,6 +703,14 @@ namespace MobileGL::MG_Pipe { // P5c (gt, layer 2): the single-field refresh is a client write into gPipeInputs too - // same gate as the fill, and it makes the same claim the fill made: the rows it is // touching belong to a record this thread is parked behind (or will park behind). + // + // P5e (ra2): SO IT TAKES THE SAME WAIT. This one runs at a frontend mutation point, not + // at a verb, so "will park behind" is even weaker here than it is at the fill - the + // mutation can land anywhere between two verbs, with the whole run-ahead backlog in + // flight. The wait is a no-op unless run-ahead is armed, and at a mutation point after a + // barriered verb the applier is usually already caught up, so this is a watermark test + // rather than a park in the common case. + QuiesceApplierBeforeFill("MGPipeNoteFrontendMutation"); MG_Remote::Client::ClientSession::RefusePipeInputsTouchWhileApplierOwnsIt( "MGPipeNoteFrontendMutation", /*isBarrieredFill=*/true); #endif @@ -3315,6 +3389,17 @@ namespace MobileGL::MG_Pipe { // thread is about to park behind it. The second argument is which of the two // claims the caller is making, and a fill that made the second one falsely is the // named abort. + // + // P5e (ra2): AND IT IS MADE TRUE FIRST. Phase 1 of the block write is three lines + // below - the serial bump, MGPipeServerClearVerbBoundary() and SetVerb - and every + // one of them is a scalar the apply thread reads at each field access inside the + // record it is currently applying. Withdrawing the server's own stamp from under it + // is what produced `Fatal{UnmigratedPipeInput, "@"}` on the apply thread (report §2). THE RED-ONCE IS THIS LINE: + // delete it and the guard below aborts with Fatal{RoleViolation, "gPipeInputs"} + // naming MGPipeValidateForVerb, because the guard now tests the fact rather than + // taking the claim. + QuiesceApplierBeforeFill("MGPipeValidateForVerb"); MG_Remote::Client::ClientSession::RefusePipeInputsTouchWhileApplierOwnsIt( "MGPipeValidateForVerb", /*isBarrieredFill=*/barriered); } @@ -3630,6 +3715,20 @@ namespace MobileGL::MG_Pipe { // own boundary and §3.3's detector turns any field it still needs from here into // Fatal{UnmigratedPipeInput, "@"} by name, which is what makes the strict // lane a gate rather than a count. +#if MOBILEGL_BUILD_DISAGGREGATED + // P5e (ra2): PHASE 2 OF THE BLOCK WRITE, AND IT NEEDS ITS OWN WAIT. Everything between + // the phase-1 wait and here PUBLISHED RECORDS - the fourteen emitters above - and the + // apply thread reads gPipeInputs while it applies them. So the window the fill opened at + // the top of this function was closed again by this function's own emissions, and the + // 63-field walk below would run straight into it. Same call, same arm, same no-op + // everywhere run-ahead is not armed; the guard beside it is what turns a missing one + // into a named abort rather than a torn read. + if (fillOwed) { + QuiesceApplierBeforeFill("MGPipeValidateForVerb/residual"); + MG_Remote::Client::ClientSession::RefusePipeInputsTouchWhileApplierOwnsIt( + "MGPipeValidateForVerb/residual", /*isBarrieredFill=*/true); + } +#endif const Bool applierDerives = ApplierDerivesRenderStateFields(); // BY VALUE, NOT BY REFERENCE: the memo's storage is rebuilt in place when the key moves, // and the walk below holds this across 63 iterations. diff --git a/MobileGL/MG_Remote/Client/ClientSession.cpp b/MobileGL/MG_Remote/Client/ClientSession.cpp index d9fcc6dc..eebc405d 100644 --- a/MobileGL/MG_Remote/Client/ClientSession.cpp +++ b/MobileGL/MG_Remote/Client/ClientSession.cpp @@ -1316,14 +1316,40 @@ namespace MobileGL::MG_Remote::Client { // // This is where a fill left standing for an unbarriered record lands, by name. if (ClientSessionInstance().RunAheadArmed()) { - if (isBarrieredFill) return; + // ---- P5e (ra2): THE EXEMPTION IS A FACT NOW, NOT A CLAIM ------------------------ + // + // This arm used to read `if (isBarrieredFill) return;`, i.e. the caller's own + // sentence "this touch is the residual fill of a record this thread is about to park + // behind" was accepted as the whole argument. It is an argument about the FUTURE. + // The order at the validate point is fill, then emit, then park, so at the instant + // of the write the apply thread is still draining the unbarriered records the client + // ran ahead of - and the measured consequence was the GL thread bumping + // CurrentVerbSerial, withdrawing MGPipeServerClearVerbBoundary's flag and renaming + // m_currentVerb underneath a record the applier was inside, which surfaced as + // Fatal{UnmigratedPipeInput, "@"} on the apply thread. + // Because the exemption was unconditional, this guard could not fire on any of it: + // that is the answer to "does RefusePipeInputsTouchWhileApplierOwnsIt fire, and if + // not, why not". + // + // So the sentence still exempts the fill, but only once it is TRUE of this instant. + // The fill sites establish it by taking §2.5's forced wait first + // (QuiesceApplierBeforeFill, PipeFill.cpp); removing that wait is the red-once and + // lands here, by name, on the first barriered verb behind a run-ahead backlog. + // + // ApplyThreadIsInsideApplier() is an acquire load of the flag PipeApplier raises for + // the whole of ApplyOne - stamp, decode and LeaveApplier included - so "not inside" + // is exactly "no record is being applied" and not merely "not decoding". + if (isBarrieredFill && !ApplyThreadIsInsideApplier()) return; if (Server::ServerLoop::OnApplyThread()) return; // the applier owns the block MGLOG_F("MGPipe: Fatal{RoleViolation, \"gPipeInputs\"} - the GL thread touched " - "gPipeInputs (%s) on a RUN-AHEAD session outside a barriered fill. With " - "the client running ahead the block is the server's to read for as long " - "as an unbarriered record is in flight (CONTRACT-P5E §3), and this thread " - "has no wait that would make the touch quiescent", - surface); + "gPipeInputs (%s) on a RUN-AHEAD session %s. With the client running ahead " + "the block is the server's to read for as long as ANY record is in flight " + "(CONTRACT-P5E §3), so the only legal GL-thread write is a barriered fill " + "that has first taken §2.5's forced wait (QuiesceApplierBeforeFill)", + surface, + isBarrieredFill ? "in a barriered fill that did not first wait for the " + "applier to catch up" + : "outside a barriered fill"); std::abort(); } // MOBILEGL_IPC_BATCH_WAITS=1 makes "the fill runs while the apply thread applies an diff --git a/MobileGL/MG_Remote/Client/ClientSession.h b/MobileGL/MG_Remote/Client/ClientSession.h index a0a0f2c7..7a031bbb 100644 --- a/MobileGL/MG_Remote/Client/ClientSession.h +++ b/MobileGL/MG_Remote/Client/ClientSession.h @@ -208,11 +208,18 @@ namespace MobileGL::MG_Remote::Client { // P5e (ra, CONTRACT-P5E §3.5) RE-DERIVES IT UNDER RUN-AHEAD, and `isBarrieredFill` is // what the caller says about ITSELF: "this touch is the residual fill of a record this // thread is about to park behind". With run-ahead armed the block is server-role - // memory for every other touch, so the rule stops being "not while the applier is - // inside" - which is unanswerable when the client never waits - and becomes "not at - // all, unless you are that fill". The MOBILEGL_IPC_BATCH_WAITS early return goes with + // memory for every other touch. The MOBILEGL_IPC_BATCH_WAITS early return goes with // it: batching is about how many round trips a lockstep client pays, and it says // nothing about a client that pays none. + // + // P5e (ra2) KEEPS THE ARGUMENT AND STOPS TAKING IT ON TRUST. "About to park behind it" + // is a claim about the future; the write is now. Between the fill and the park the + // apply thread is still draining the unbarriered records the client ran ahead of, so an + // unconditional exemption made this guard unfireable on exactly the class it was + // written for - measured as the apply thread aborting on a field whose stamp the GL + // thread had just withdrawn. So the rule is "not while the applier is inside" again, + // for barriered fills too; what changed is that the fill sites MAKE it true first, with + // §2.5's forced wait (PipeFill.cpp's QuiesceApplierBeforeFill), instead of asserting it. static void RefusePipeInputsTouchWhileApplierOwnsIt(const char* surface, Bool isBarrieredFill = false); diff --git a/MobileGL/MG_Test/Wire/RemoteClientTest.cpp b/MobileGL/MG_Test/Wire/RemoteClientTest.cpp index 84f2e2aa..a87e0df3 100644 --- a/MobileGL/MG_Test/Wire/RemoteClientTest.cpp +++ b/MobileGL/MG_Test/Wire/RemoteClientTest.cpp @@ -2297,6 +2297,10 @@ TEST(RemoteGuards, ClientPipeInputsTouchOutsideABarrieredFillUnderRunAheadIsFata // ... and the control: the barriered fill says so and is let through. Without this the case // above would pass just as well against "abort unconditionally". +// +// P5e (ra2): THIS CONTROL IS NOW ALSO HALF OF A PAIR, and the half it does NOT state is the +// one that cost the flip its lane. "The applier is not inside" is true here because nothing in +// this child ever entered it; the case below is the same call with that one fact reversed. TEST(RemoteGuards, ClientPipeInputsTouchInsideABarrieredFillUnderRunAheadIsAllowed) { const auto child = RunInChild([] { StartRunAheadSession(); @@ -2307,6 +2311,69 @@ TEST(RemoteGuards, ClientPipeInputsTouchInsideABarrieredFillUnderRunAheadIsAllow ExpectChildSuccess(child); } +// ---- P5e (ra2): THE FLIP'S OWN RED-ONCE, AND THE CLASS THE STRICT LANE CANNOT SEE ---------- +// +// The strict lane runs under LOCKSTEP - ApplyOne stamps every record barriered - so nothing +// that happens only once the client stops waiting is visible to it, by construction (ID-132). +// This pair is what measures it instead, and it runs in the ordinary unit lane on any head, +// flip thrown or not, because StartRunAheadSession arms run-ahead itself. +// +// WHAT IT PINS: `isBarrieredFill` is the caller's sentence "this touch is the residual fill of +// a record this thread is about to park behind". That is a claim about the FUTURE. The order at +// the validate point is fill, then emit, then park, so at the instant of the write the apply +// thread is still draining the UNBARRIERED records the client ran ahead of - and the exemption +// that took the claim on trust made this guard unfireable on precisely the class it exists for. +// Measured on the flipped head: the GL thread bumped CurrentVerbSerial, withdrew the server's +// stamp and renamed m_currentVerb underneath a record the applier was inside, and the applier +// aborted with `Fatal{UnmigratedPipeInput, "@"}` - a verb no applier +// stamp can produce, which is what identifies the writer. +// +// THE PROBE IS THE GUARD CALL ITSELF (ID-102): the flag is raised by the same raw entry point +// PipeApplier's ScopedApplierEntry uses, on the client thread, BEFORE the call under test - so +// what aborts is this probe and not some object built on the wrong thread. +// +// THE RED: in ClientSession::RefusePipeInputsTouchWhileApplierOwnsIt put the run-ahead arm back +// to `if (isBarrieredFill) return;` and this case stops aborting, while the control above keeps +// passing - which is the difference between a rule and an exemption. +TEST(RemoteGuards, ClientBarrieredFillWhileTheApplierIsInsideUnderRunAheadIsFatalByName) { + const auto child = RunInChild([] { + StartRunAheadSession(); + ClientSession::NoteApplyThreadEnteredApplier(); + ClientSession::RefusePipeInputsTouchWhileApplierOwnsIt("ra-test-surface", + /*isBarrieredFill=*/true); + ClientSession::NoteApplyThreadLeftApplier(); + ClientSessionInstance().Stop(); + }); + ExpectNamedAbort(child, "Fatal{RoleViolation, \"gPipeInputs\"}"); +} + +// ---- P5e (ra2): AND THE OTHER HALF - AN INDEXED DRAW IS AN UNBARRIERED VERB ---------------- +// +// MGP_VERB_OP_LIST joins the whole draw family through ONE row, `DrawVbo -> DrawArrays`, so +// inverting it verb-first answers kOpCount for DrawElements and for every other indexed / +// instanced / multi / indirect verb. The "unknown verb answers barriered" default then made the +// client FILL for each of them while never parking, because the wait is decided per record and +// the record is a draw_vbo (kWaitNone). On the flipped lane that was 21 of the 69 red entries. +// +// The case states it where it is decidable without a GL context: the guard runs before the +// validate point's null-context return, so a verb that answers UNBARRIERED never reaches the +// guard at all and the child exits 0 even with the applier flag up. +// +// THE RED: delete the kDraw fallback in ClientVerbIsBarriered (PipeFill.cpp) and DrawElements +// answers barriered again - the fill runs, the guard above it sees the raised flag, and this +// case aborts with Fatal{RoleViolation, "gPipeInputs"} instead of exiting 0. +TEST(RemoteRunAhead, AnIndexedDrawVerbIsUnbarrieredAndTouchesPipeInputsNotAtAll) { + const auto child = RunInChild([] { + StartRunAheadSession(); + ClientSession::NoteApplyThreadEnteredApplier(); + MGPipeValidateForVerb(MGPipeVerb::DrawElements); + ClientSession::NoteApplyThreadLeftApplier(); + MGPipeLeaveVerb(); + ClientSessionInstance().Stop(); + }); + ExpectChildSuccess(child); +} + // §3.3's RED-ONCE, and the one that turns the strict lane into a gate. The probe is the // sticky forward itself, run on the apply thread inside a server-stamped verb with the // current record marked UNBARRIERED - which is the state the sink puts that thread in for