diff --git a/MobileGL/MG_Backend/MGPipe/PipeInputs.cpp b/MobileGL/MG_Backend/MGPipe/PipeInputs.cpp index 939c03b5..bc8f9741 100644 --- a/MobileGL/MG_Backend/MGPipe/PipeInputs.cpp +++ b/MobileGL/MG_Backend/MGPipe/PipeInputs.cpp @@ -58,22 +58,50 @@ namespace MobileGL::MG_Pipe { // existing filter on Fatal{UnmigratedPipeInput still matches, plus the class and the // phase that retires it - a strict abort that did not say which phase owes the answer // would leave the reader exactly where the gate found them. - [[noreturn]] void StrictBarrierPullFatal(MGPipeInputField field, MGPipeVerb verb) { + // P5e (ra): `why` is the second half of the marker, because the lane's allowlist step + // reads this line and the two reasons mean different things to it. "MOBILEGL_IPC_ + // STRICT_ERRORS=1" is the operator asking for a real, ordered value to be loud; + // "UNBARRIERED" is the value not existing (§3.3). The `@` prefix and the + // Fatal{UnmigratedPipeInput tag are unchanged, so every filter written since P5c still + // matches. + [[noreturn]] void StrictBarrierPullFatal(MGPipeInputField field, MGPipeVerb verb, + const char* why) { const SizeT index = static_cast(field); - MGLOG_F("MGPipe: Fatal{UnmigratedPipeInput, \"%s@%s\"} [BARRIER-PULLED, " - "MOBILEGL_IPC_STRICT_ERRORS=1, retires in %s]", - kMGPipeInputFieldNames[index], MGPipeVerbName(verb), kMGPipeFieldRetiringPhase[index]); + MGLOG_F("MGPipe: Fatal{UnmigratedPipeInput, \"%s@%s\"} [BARRIER-PULLED, %s, " + "retires in %s]", + kMGPipeInputFieldNames[index], MGPipeVerbName(verb), why, + kMGPipeFieldRetiringPhase[index]); std::abort(); } // One place decides what a BARRIER-PULLED read does, so the field accessors and the // seven sticky forwards cannot drift apart on it. + // + // ---- P5e (ra), CONTRACT-P5E §3.3: THE DETECTOR IS UNCONDITIONAL UNDER AN UNBARRIERED + // RECORD, AND THAT IS WHAT MAKES THE STRICT LANE A GATE ------------------------------- + // + // The knob exists because under LOCKSTEP a pulled row is a real, ordered, fresh value: + // the client filled it and then parked, so "count it and carry on" is an honest + // measurement of remaining debt and MOBILEGL_IPC_STRICT_ERRORS is the operator asking + // for the debt to be loud instead. None of that survives run-ahead. With the client + // running ahead of this apply, the row was either never filled for this verb (§3.1 + // skips the fill) or is being overwritten by a verb two frames later - so the value is + // torn or stale BY CONSTRUCTION and there is nothing for a counter to count. A + // "count it" arm here would be a wrong picture with a number beside it. + // + // rsp is therefore 0 on unbarriered records in every scenario summary (§7's pin): a + // non-zero count would be an abort that did not fire. void CountBarrierPull(MGPipeInputField field, MGPipeVerb verb) { + if (!MGPipeApplierCurrentRecordIsBarriered()) { + StrictBarrierPullFatal(field, verb, "UNBARRIERED, the client did not fill it"); + } ++g_residualPulls; if (MG_Util::PipeStats::Enabled()) { MG_Util::PipeStats::AddCalls(MG_Util::PipeStats::CallClass::ResidualPulls, 1); } - if (MG_Config::Ipc.StrictErrors) StrictBarrierPullFatal(field, verb); + if (MG_Config::Ipc.StrictErrors) { + StrictBarrierPullFatal(field, verb, "MOBILEGL_IPC_STRICT_ERRORS=1"); + } } // The verb's OWN may-read table (FillPoints.def, kMGPipeClassFieldMask). The stamp diff --git a/MobileGL/MG_Impl/GLImpl/Exporting/Definitions.cpp b/MobileGL/MG_Impl/GLImpl/Exporting/Definitions.cpp index ed94936a..2275a89d 100644 --- a/MobileGL/MG_Impl/GLImpl/Exporting/Definitions.cpp +++ b/MobileGL/MG_Impl/GLImpl/Exporting/Definitions.cpp @@ -108,7 +108,18 @@ DECLARE_GL_FUNCTION_HEAD(void, DrawArrays, GLenum mode, GLint first, GLsizei cou DECLARE_GL_FUNCTION_HEAD(void, DrawElements, GLenum mode, GLsizei count, GLenum type, const void* indices) DECLARE_GL_FUNCTION_END_NO_RETURN(void, DrawElements, mode, count, type, indices) DECLARE_GL_FUNCTION_HEAD(void, Enable, GLenum cap) DECLARE_GL_FUNCTION_END_NO_RETURN(void, Enable, cap) DECLARE_GL_FUNCTION_HEAD(void, EnableVertexAttribArray, GLuint index) DECLARE_GL_FUNCTION_END_NO_RETURN(void, EnableVertexAttribArray, index) -MOBILEGL_GL_API void glFinish() { MGLOG_D("Implementing function: %s(...)", __FUNCTION__); } +// P5e (ra), CONTRACT-P5E §2.5. STILL NOTHING ON THE WIRE - Flush and Finish have no record +// (ARCHITECTURE §11) and this stays a no-op on every monolith build. What it is not any more +// is a no-op on a RUN-AHEAD client: "the commands issued so far have completed" is a promise a +// queue the client never waits on can break, and this is the only place an application can ask +// for it. Under lockstep the client had already waited out every command it issued, so the +// call really did have nothing to do; MGPipeClientFinish answers false-cheap there too. +MOBILEGL_GL_API void glFinish() { + MGLOG_D("Implementing function: %s(...)", __FUNCTION__); +#if MOBILEGL_BUILD_DISAGGREGATED + MobileGL::MG_Pipe::MGPipeClientFinish(); +#endif +} MOBILEGL_GL_API void glFlush() { MGLOG_D("Implementing function: %s(...)", __FUNCTION__); } DECLARE_GL_FUNCTION_HEAD(void, FramebufferRenderbuffer, GLenum target, GLenum attachment, GLenum renderbuffertarget, GLuint renderbuffer) DECLARE_GL_FUNCTION_END_NO_RETURN(void, FramebufferRenderbuffer, target, attachment, renderbuffertarget, renderbuffer) DECLARE_GL_FUNCTION_HEAD(void, FramebufferTexture2D, GLenum target, GLenum attachment, GLenum textarget, GLuint texture, GLint level) DECLARE_GL_FUNCTION_END_NO_RETURN(void, FramebufferTexture2D, target, attachment, textarget, texture, level) diff --git a/MobileGL/MG_Impl/Pipe/PipeFill.cpp b/MobileGL/MG_Impl/Pipe/PipeFill.cpp index fe0999ef..34d23700 100644 --- a/MobileGL/MG_Impl/Pipe/PipeFill.cpp +++ b/MobileGL/MG_Impl/Pipe/PipeFill.cpp @@ -85,6 +85,20 @@ namespace MobileGL::MG_Pipe { return inputs.m_currentVertexAttribute; } +#if MOBILEGL_BUILD_DISAGGREGATED + // P5e (ra, CONTRACT-P5E §2.3). THE FOUR O-CLASS ROWS, AND ONLY THEY: these are the only + // members of this block that own a frontend object rather than point into one, so they + // are the only ones through which the apply thread can become a last owner. The raw + // pointer rows (the binding-slot bases) are borrowed from a live GLContext and own + // nothing, so releasing them would buy no lifetime and lose the monolith's arms. + static void ReleaseObjectPins(PipeInputs& inputs) { + inputs.m_boundVertexArray.reset(); + inputs.m_programForDispatch.reset(); + inputs.m_programForDraw.reset(); + inputs.m_transformFeedbackProgram.reset(); + } +#endif + static void CopyField(PipeInputs& dst, GLContext& ctx, MGPipeInputField field) { using F = MGPipeInputField; using MG_State::GLState::BufferBindPointTargets; @@ -330,6 +344,64 @@ namespace MobileGL::MG_Pipe { namespace { GLContext* LiveContext() { return MG_State::pGLContext.get(); } +#if MOBILEGL_BUILD_DISAGGREGATED + // ---- P5e (ra): the fill decision (CONTRACT-P5E §3.1) ----------------------------- + // + // Is this process a run-ahead CLIENT right now? Three questions, cheapest first, and + // the last one is the latch ClientSession took at its first caps adoption - so this is + // one pointer test and one bool load on the verb path once the first two are constant. + Bool ClientRunsAhead() { + if (MG_Config::Transport == MG_Config::TransportMode::Monolith) return false; + if (MG_Remote::Client::RunsAsTheServerRole()) return false; + const MG_Remote::Client::ClientSession* session = MG_Remote::Client::ClientSession::Active(); + return session != nullptr && session->RunAheadArmed(); + } + + // THE WIRE OP A VERB BOUNDARY BECOMES, which is the join MGPipeVerbForWireOp draws in + // the other direction. Built by walking the op space once at compile time rather than + // written out, because a second hand-written table is a second thing to forget a row + // in - and the generator already refuses a verb-shaped op with no row. + constexpr MGPWireOp WireOpForVerb(MGPipeVerb verb) { + for (SizeT i = 0; i < static_cast(MGPWireOp::kOpCount); ++i) { + const auto op = static_cast(i); + if (MGPipeVerbForWireOp(op) == verb) return op; + } + return MGPWireOp::kOpCount; + } + + // CONTRACT-P5E §2.1's predicate, ASKED AT THE VALIDATE POINT - which is before the + // record exists, so it is asked of the verb and the live context rather than of the + // payload. The two halves must agree with MGPipeBarriered(op, payload, applierState), + // which is what the server computes, so each clause is the same clause: + // + // 1. the static WaitClass column - the same generated table, same op; + // 2. kCtxVerb inside an open XFB span - ctx.IsTransformFeedbackActive() here, + // the applied MGPContextValues mirror there, and set_context_values precedes the + // verb on the ring, so the two read the same value (§2.1); + // 3. a draw carrying kDrawClientArrays - NOT asked here, because under run-ahead + // such a draw never reaches a record at all: vi refuses it on this very thread in + // EmitDrawRecord's array arm (§5.1, ruling 2). Asking it here would be a second + // spelling of vi's "does any enabled attribute lack a buffer" walk, and two + // spellings of an escalation is precisely what ruling 3 replaced. + // + // AN OP WITH NO VERB ROW ANSWERS BARRIERED. A verb the join does not know is one this + // file cannot reason about, and the safe answer - fill it, wait for it - is also the + // pre-P5e answer. + Bool ClientVerbIsBarriered(MGPipeVerb verb, GLContext& ctx) { + const MGPWireOp op = WireOpForVerb(verb); + if (op == MGPWireOp::kOpCount) return true; + if (MGPipeWaitClassFor(op) != kWaitNone) return true; + if (MGPipeCallClassFor(op) == kCtxVerb && ctx.IsTransformFeedbackActive()) return true; + return false; + } + + // 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 + // the role violation §3.5 names. + Bool g_lastFillWasBarriered = true; +#endif + template const SharedPtr& NullShared() { static const SharedPtr null; @@ -546,10 +618,18 @@ namespace MobileGL::MG_Pipe { void MGPipeNoteFrontendMutation(MGPipeInputField field) { PipeInputs& inputs = gPipeInputs; #if MOBILEGL_BUILD_DISAGGREGATED + // P5e (ra, CONTRACT-P5E §3.1): THIS REFRESHES ONE FIELD OF THE LAST FILL, so with no + // fill behind it there is nothing to refresh. Under run-ahead the last verb may have + // been unbarriered, in which case the block describes an older verb the server is no + // longer being asked about and a write here would be a GL-thread touch of server-role + // memory. It returns instead - the same answer, one line earlier, as the class-mask + // test below gives for a field the verb never pushed. + if (!g_lastFillWasBarriered) return; // P5c (gt, layer 2): the single-field refresh is a client write into gPipeInputs too - - // same gate as the fill. + // 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). MG_Remote::Client::ClientSession::RefusePipeInputsTouchWhileApplierOwnsIt( - "MGPipeNoteFrontendMutation"); + "MGPipeNoteFrontendMutation", /*isBarrieredFill=*/true); #endif auto* ctx = LiveContext(); if (ctx == nullptr) return; @@ -999,6 +1079,29 @@ namespace MobileGL::MG_Pipe { Bool MGPipeDeferDestroyAndFreeIfOnApplyThread(MGPipeKind kind, Uint64 lifetimeId) { if (MG_Config::Transport == MG_Config::TransportMode::Monolith) return false; if (!MG_Remote::Client::RunsAsTheServerRole()) return false; + // P5e (ra), CONTRACT-P5E §2.7 / ruling 13. The queue stays - a BARRIERED record's apply + // may still pin a frontend object (its fill's O-class rows, XFB's targets), so the + // apply thread can still be a last owner and this is still the belt that keeps that + // death off the client's allocator. But an enqueue from an UNBARRIERED record is a + // FINDING, not a service: rule F says such an apply names no client memory at all, so + // a SharedPtr it could be the last owner of means some site is still pinning a + // frontend object across a record and the migration this phase believes it finished is + // not finished. Named once with the kind so the site is findable, and Fatal under + // strict so the lane owns the red rather than a log nobody reads. + if (!MG_Pipe::MGPipeApplierCurrentRecordIsBarriered()) { + if (MG_Config::Ipc.StrictErrors) { + MGLOG_F("MGPipe: Fatal{RoleViolation, \"deferred-destroy\"} - an UNBARRIERED " + "apply was the last owner of a frontend object of kind %u (lifetime " + "%llu). CONTRACT-P5E rule F says an unbarriered apply reads no client " + "memory, so nothing it touched should have been a SharedPtr at all", + static_cast(kind), static_cast(lifetimeId)); + std::abort(); + } + MGLOG_E_ONCE("MGPipe: an UNBARRIERED apply deferred the destruction of a frontend " + "object of kind %u - CONTRACT-P5E §2.7's finding: some apply-thread " + "site still holds a frontend SharedPtr across a record", + static_cast(kind)); + } { std::lock_guard lock(g_deferredDestroyMutex); g_deferredDestroys.push_back(MGPipeDeferredDestroy{kind, lifetimeId}); @@ -1007,6 +1110,30 @@ namespace MobileGL::MG_Pipe { return true; } + // P5e (ra), CONTRACT-P5E §2.3. Declared in PipeMutation.h, where its WHY is argued. + // + // NO GUARD ON THE CALLER'S BEHALF: ClientSession calls this only after a barriered apply + // has returned on a run-ahead session, and adding a second "is run-ahead armed" test here + // would be a copy of a decision that belongs on the other side. What this side owns is + // WHICH rows go, and that answer is ReleaseObjectPins' four. + // + // THE STAMPS ARE LEFT ALONE, deliberately. A released row reads back as null, and a + // BARRIERED verb's fill re-copies it before that verb's apply can pull it (§3.1 skips the + // fill only for UNBARRIERED records); an unbarriered apply may not read it at all, and + // §3.3's detector is what says so by name. Clearing the stamps as well would trade that + // named abort for the poison Fatal, which names the field but not the rule it broke. + void MGPipeReleaseResidualFillPins() { MGPipeFillAccess::ReleaseObjectPins(gPipeInputs); } + + // P5e (ra), CONTRACT-P5E §2.5. Declared in PipeMutation.h; see there for why it is not a + // ClientSession call at the GL entry point. + void MGPipeClientFinish() { + if (MG_Config::Transport == MG_Config::TransportMode::Monolith) return; + if (MG_Remote::Client::RunsAsTheServerRole()) return; + if (MG_Remote::Client::ClientSession* session = MG_Remote::Client::ClientSession::Active()) { + session->Finish(); + } + } + void MGPipeDrainDeferredDestroys() { if (g_deferredDestroyCount.load(std::memory_order_acquire) == 0) return; Vector drained; @@ -1955,9 +2082,20 @@ namespace MobileGL::MG_Pipe { void MGPipeLeaveVerb() { PipeInputs& inputs = gPipeInputs; #if MOBILEGL_BUILD_DISAGGREGATED - // Same layer-2 gate as the fill: the serial bump and the verb reset below are writes - // into gPipeInputs (gt). - MG_Remote::Client::ClientSession::RefusePipeInputsTouchWhileApplierOwnsIt("MGPipeLeaveVerb"); + // P5e (ra, §3.1): a verb whose fill was skipped has no stamps of this thread's to + // retire, and the bump below would move a serial the SERVER's stamp owns. Leave + // returns, and the tracker's base-instance clear - which is frontend state, not block + // state - still runs at the bottom. + if (g_lastFillWasBarriered) { + // Same layer-2 gate as the fill: the serial bump and the verb reset below are + // writes into gPipeInputs (gt). + MG_Remote::Client::ClientSession::RefusePipeInputsTouchWhileApplierOwnsIt( + "MGPipeLeaveVerb", /*isBarrieredFill=*/true); + } + if (!g_lastFillWasBarriered) { + MGPipeTrackerInstance().ClearPendingBaseInstance(); + return; + } #endif #if MOBILEGL_PIPE_POISON // Same bump the next fill would make, without a verb to fill from: no field is @@ -2686,6 +2824,18 @@ namespace MobileGL::MG_Pipe { // Did the applier reproduce it? Byte for byte, over the attributes this call // named - anything less would be a mirror that disagrees with the frontend in a // window no gate looks at. + // + // P5e (ra, CONTRACT-P5E §3.4): NOT UNDER RUN-AHEAD. The mirror is the APPLIER's + // copy of this record, and set_vertex_attrib_defaults is a kWaitNone row - so + // under run-ahead this thread has not waited for the apply and the read races it, + // and the repair below (a CopyField straight into the block) is precisely the + // GL-thread write §3.5 refuses. The client's own authority is `resolved` and + // `g_attribDefaultLastHeader`, which is what it just published; there is nothing + // the mirror could add that the wire does not already carry. A build that ever + // needs the repair arm again has to earn it with a barriered row. +#if MOBILEGL_BUILD_DISAGGREGATED + if (ClientRunsAhead()) return sizeof(MGPVertexAttribDefaults) + header.Count * sizeof(MGPAttribValue); +#endif const auto* mirror = MGPipeFillAccess::VertexAttribDefaultsOf(gPipeInputs); Bool reproduced = true; for (SizeT i = 0; i < kAttribs && reproduced; ++i) { @@ -3004,13 +3154,20 @@ namespace MobileGL::MG_Pipe { void MGPipeValidateForVerb(MGPipeVerb verb) { PipeInputs& inputs = gPipeInputs; #if MOBILEGL_BUILD_DISAGGREGATED - // P5c (gt, CONTRACT-P5C §6 layer 2): the residual fill is THE client-side write into - // gPipeInputs, and it is legal only because it runs before the record is published - - // under an armed barrier the apply thread's in-applier flag is provably down here. If - // that ever stops being true this is the check that says so, rather than the applier - // reading a half-written fill. - MG_Remote::Client::ClientSession::RefusePipeInputsTouchWhileApplierOwnsIt( - "MGPipeValidateForVerb"); + // ---- P5e (ra), CONTRACT-P5E §3: WHO OWNS gPipeInputs FOR THIS VERB ---------------- + // + // Under run-ahead the block is SERVER-ROLE MEMORY for an unbarriered record: the + // client does not fill it, does not stamp it and does not withdraw the server's stamp, + // because it will not be parked while the apply runs and every one of those writes + // would race the applier's own reads. What still runs, unchanged, is the tracker walk + // and the emitters (steps 2 and 3): those PRODUCE RECORDS, which is the whole of what + // an unbarriered verb is allowed to hand the server. + // + // `fillOwed` is deliberately a separate name from `barriered`, and that is what makes + // the red-once one line: setting it to `true` restores the old behaviour (a fill for + // every verb) and the guard below then fires Fatal{RoleViolation, "gPipeInputs"} on + // the first unbarriered verb, by name. + const Bool runAhead = ClientRunsAhead(); #endif ParsePoisonOmissionKnob(); #if MOBILEGL_PIPE_VERIFY @@ -3023,24 +3180,64 @@ namespace MobileGL::MG_Pipe { "(configure with -DMOBILEGL_PIPE_VERIFY=ON)"); } #endif + auto* ctx = LiveContext(); + // THE IDENTITY / LIVENESS PAIR IS WRITTEN ON EVERY VERB, run-ahead or not, and that is + // a NAMED DEVIATION from CONTRACT-P5E §3.1's list (see the report): `m_live` and + // `m_contextIdentity` are not FIELDS of the residual model - no FieldOwnership.def row + // owns them, no applier record carries them, and no server stamp can answer them - + // they are "does this process still have a GL context", which every null-context guard + // in the backend reads and which only this thread can know. Two stores, no freshness + // and no stamp, so an unbarriered apply reading them reads a fact about the client's + // process rather than a value the wire owes it. + MGPipeFillAccess::SetIdentity(inputs, ctx); +#if MOBILEGL_BUILD_DISAGGREGATED + // §2.1's predicate, client-side (see ClientVerbIsBarriered). A null context makes the + // verb a no-op below anyway, so it answers barriered and the old path runs whole. + const Bool barriered = !runAhead || ctx == nullptr || ClientVerbIsBarriered(verb, *ctx); + // THE ONE LINE THE RED-ONCE FLIPS: `= true` here is "fill for every verb", the + // pre-P5e behaviour, and it turns the guard below into the abort §3.5 names. + const Bool fillOwed = barriered; + g_lastFillWasBarriered = fillOwed; + if (fillOwed) { + // P5c (gt, CONTRACT-P5C §6 layer 2) / P5e §3.5: the residual fill is THE + // client-side write into gPipeInputs. Under lockstep it is legal because it runs + // before the record is published - the apply thread's in-applier flag is provably + // down here. Under run-ahead it is legal because this record is BARRIERED: this + // 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. + MG_Remote::Client::ClientSession::RefusePipeInputsTouchWhileApplierOwnsIt( + "MGPipeValidateForVerb", /*isBarrieredFill=*/barriered); + } +#else + constexpr Bool fillOwed = true; +#endif + if (fillOwed) { #if MOBILEGL_PIPE_POISON - MGPipeFilledState& filled = MGPipeFillAccess::Filled(inputs); - // Starts at 1: FilledGen == 0 is "never filled", and MGPipeInputFieldIsFresh refuses - // it on both branches, so a read before this first bump is - // Fatal{UnmigratedPipeInput, "@"} rather than default storage. - ++filled.CurrentVerbSerial; + // Starts at 1: FilledGen == 0 is "never filled", and MGPipeInputFieldIsFresh + // refuses it on both branches, so a read before this first bump is + // Fatal{UnmigratedPipeInput, "@"} rather than default storage. + ++MGPipeFillAccess::Filled(inputs).CurrentVerbSerial; #endif #if MOBILEGL_BUILD_DISAGGREGATED - // The client is filling, so whatever the server stamped at its last verb boundary is - // withdrawn: the stamps below are the CLIENT's again and a stale read is a defect, not - // a residual pull. Disarming here rather than at the end of the applier's work is what - // makes the arming flag say "the current stamps are the server's" no matter which of - // the two roles ran last. - MGPipeServerClearVerbBoundary(); + // The client is filling, so whatever the server stamped at its last verb boundary + // is withdrawn: the stamps below are the CLIENT's again and a stale read is a + // defect, not a residual pull. Disarming here rather than at the end of the + // applier's work is what makes the arming flag say "the current stamps are the + // server's" no matter which of the two roles ran last. + // + // P5e (§3.2): under run-ahead this runs only inside a barriered fill, which is the + // whole of the E note's hazard (b.4) - a GL thread withdrawing the SERVER's stamp + // while the apply thread is inside a record that depends on it. For an unbarriered + // verb the stamp is not touched at all, and the applier's own LeaveApplier is then + // its only writer. + MGPipeServerClearVerbBoundary(); +#endif + MGPipeFillAccess::SetVerb(inputs, verb); + } +#if MOBILEGL_PIPE_POISON + MGPipeFilledState& filled = MGPipeFillAccess::Filled(inputs); #endif - MGPipeFillAccess::SetVerb(inputs, verb); - auto* ctx = LiveContext(); - MGPipeFillAccess::SetIdentity(inputs, ctx); if (ctx == nullptr) { // The pending base instance belongs to THIS verb, and this exit skips step 3's // clear, so it has to make the same promise here: a base-instanced draw with no @@ -3286,12 +3483,20 @@ namespace MobileGL::MG_Pipe { // does read. The two halves of P5c rv's gate still read the ONE `contextValuesWireLive` // computed at the top of this function - it is the memo's key AND its argument - so they // cannot disagree about who supplies the eight value-class fields. + // + // P5e (ra, §3.1): AND IT DOES NOT RUN AT ALL FOR AN UNBARRIERED RECORD. Every write + // below - the CopyField and the FilledGen stamp alike - is a GL-thread write into a + // block the apply thread is about to read without this thread being parked, which is + // exactly what rule F forbids. The server does not go without an answer: it stamps its + // 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. 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. const MGPipeFieldMask supplied = SuppliedFieldMask(pushMask, applierDerives, contextValuesWireLive); - for (SizeT i = 0; i < kMGPipeInputFieldCount; ++i) { + for (SizeT i = 0; fillOwed && i < kMGPipeInputFieldCount; ++i) { const auto field = static_cast(i); if (!MGPipeFieldMaskHas(mask, field)) continue; #if MOBILEGL_PIPE_POISON diff --git a/MobileGL/MG_Pipe/PipeMutation.h b/MobileGL/MG_Pipe/PipeMutation.h index d367a4bf..c9f6fb25 100644 --- a/MobileGL/MG_Pipe/PipeMutation.h +++ b/MobileGL/MG_Pipe/PipeMutation.h @@ -231,8 +231,38 @@ namespace MobileGL::MG_Pipe { // BeforeReadOnlyVerb) through MGPipeDrainDeferredDestroys. A delayed free is safe: // slots are plentiful, reuse is delayed rather than corrupted, and the replay runs the // helper itself, so every helper's own three-step order is untouched. + // P5e (ra), CONTRACT-P5E §2.7 / ruling 13 (ID-91): THE QUEUE STAYS LIVE, AND AN + // UNBARRIERED ENQUEUE IS A FINDING. After the family packages land, an unbarriered apply + // holds no frontend SharedPtr at all - every object is resolved from a handle - so the + // only apply thread that can still be a last owner is one inside a BARRIERED record (a + // fill's O-class rows, XFB's pinned targets). An enqueue from an unbarriered record + // therefore means some site still pins a frontend object across a record, which is a + // migration this phase believes it finished: it logs once with the kind, and is Fatal + // under MOBILEGL_IPC_STRICT_ERRORS so the strict lane owns the red. The queue itself is + // NOT removed - it is the belt that keeps the barriered case from corrupting the + // allocator while the finding is being read. Bool MGPipeDeferDestroyAndFreeIfOnApplyThread(MGPipeKind kind, Uint64 lifetimeId); void MGPipeDrainDeferredDestroys(); + + // P5e (ra), CONTRACT-P5E §2.3: drop the four O-class SharedPtr rows the residual fill left + // in gPipeInputs (the bound VAO and the three programs, PipeInputs.h's O block). Called by + // ClientSession on the GL thread once a BARRIERED apply has returned - the one instant + // under run-ahead at which the applier is provably idle and this block has no other + // reader. Under lockstep nothing calls it: the next verb's fill overwrites those rows + // within a verb, and dropping them early would only cost a re-pin. + // + // It is HERE rather than in ClientSession because the rows are PipeInputs' private storage + // and MGPipeFillAccess - the client-side door to it - lives in MG_Impl (PipeFill.cpp). + // MG_Remote says when; MG_Impl says what. + void MGPipeReleaseResidualFillPins(); + + // P5e (ra), CONTRACT-P5E §2.5: glFinish's whole body under a transport. Declared here + // rather than on ClientSession so that MG_Impl's GL entry points - which are BELOW + // MG_Remote - can reach it without learning that a session exists; the implementation in + // PipeFill.cpp is one forward to ClientSession::Finish. A no-op with no live session, on a + // monolith transport, and on a lockstep one (there the client waited out every command it + // issued before it could reach the call). + void MGPipeClientFinish(); #endif void MGPipeEmitResourceCreate(MG_State::GLState::BufferObject& buffer); diff --git a/MobileGL/MG_Pipe/PipeRoute.h b/MobileGL/MG_Pipe/PipeRoute.h index 7e794bf9..63f4b0d5 100644 --- a/MobileGL/MG_Pipe/PipeRoute.h +++ b/MobileGL/MG_Pipe/PipeRoute.h @@ -267,12 +267,41 @@ namespace MobileGL::MG_Pipe { } // ---- resources --------------------------------------------------------------------- + + // P5e (ra), CONTRACT-P5E §2.5 / ruling 15 (ID-93): DOES THIS SUB-DATA RECORD WANT ITS + // ANSWER? One predicate, two callers - this route and the wire emit table - because the + // route cannot pass a flag through the thunk (the call table's signature is the + // catalogue's) and two hand-written copies of "is this the buffer half" is exactly the + // drift that would make the client wait for an answer the emitter told the server not to + // bother with, or read a slot that was never posted. + // + // THE BUFFER HALF DOES NOT: its Bool is discarded at the only call site there is + // (MGPipeEmitResourceSubData), and because the row carries kReplySlot the client used to + // wait for it anyway - one full round trip per 64 KB persistent-map block, whose answer + // nobody looked at. THE TEXTURE HALF DOES: DrainTextureSubData clears the level's dirty + // flag on an ACCEPTED reply, so its answer is load-bearing (D-D5; making that half + // fire-and-forget is a trailing item, not this package's). + // + // The test is MGPSubData::Target == kMGPipeResourceTargetBuffer, whole field, which is the + // invariant MGPipeTypes.h asserts beside the packer and which the applier's own + // SubDataNamesABuffer already reads. + inline Bool MGPipeSubDataWantsItsReply(const MGPSubData& record) { + return record.Target != kMGPipeResourceTargetBuffer; + } + inline Bool MGPipeRouteResourceSubData(const MGPSubData& record, const void* bytes, Uint64 byteCount, const MGPSubRegion* regions = nullptr) { + // THE SLOT IS STILL MINTED AND STILL TAKEN, even for the buffer half. The row keeps + // kReplySlot on the wire, the server still posts, and the mailbox's "every minted slot + // is taken" rule is what the next row's ReplyOverrun Fatal is looking for - so the + // half that does not WAIT still has to collect. What changes is that under run-ahead + // the emit table publishes and returns, and the answer it collects is the emitter's + // accept-by-construction rather than the server's. MGPReplySlot reply = MGPipeMintReplySlot(); MGP_ResourceSubData(&record, bytes, byteCount, regions, record.RegionCount, &reply); - return MGPipeTakeReplyBool(reply, "resource_subdata"); + const Bool accepted = MGPipeTakeReplyBool(reply, "resource_subdata"); + return MGPipeSubDataWantsItsReply(record) ? accepted : true; } inline void MGPipeRouteBufferSubDataResident(const MGPSubData& record, const void* bytes, Uint64 byteCount) { diff --git a/MobileGL/MG_Remote/Client/BackendObject_Remote.cpp b/MobileGL/MG_Remote/Client/BackendObject_Remote.cpp index bf1193e0..c7a739aa 100644 --- a/MobileGL/MG_Remote/Client/BackendObject_Remote.cpp +++ b/MobileGL/MG_Remote/Client/BackendObject_Remote.cpp @@ -172,6 +172,27 @@ namespace MobileGL::MG_Remote::Client { return CapsMirrorInstance().Backend(); } + namespace { + // P5e (ra), CONTRACT-P5E §2.5: THE WAIT BEFORE EVERY `Server*` EGL FORWARDER. + // + // These calls do not travel on SEG_CMD. They go through ServerLoop's control mailbox, + // which is pumped BETWEEN drain batches (ServerLoop.cpp's PumpControlRequest, above + // DrainRing) - so a make-current, a surface creation or a resize can land between two + // records the client published and never waited for. Under lockstep that was + // impossible: the client had waited out every record it issued before it could reach + // this line. Under run-ahead it is one queued frame wide, and a context switch applied + // in the middle of another context's draws is not a wrong pixel, it is a wrong + // everything. + // + // ServerSwapEGLBuffers is deliberately NOT on this list, and its own virtual says why: + // present IS the swap and it travels as a record, in order, with its own credit. + void WaitForApplyBeforeEglForwarder(const char* forwarder) { + if (ClientSession* session = ClientSession::Active()) { + session->WaitForApplyToCatchUp(forwarder); + } + } + } // namespace + // ---- the nine EGL lifecycle virtuals --------------------------------------------------- // // FORWARD FIRST, THEN RUN THE BASE. The server has to own the context before the client's @@ -180,6 +201,7 @@ namespace MobileGL::MG_Remote::Client { // InitCapabilities has run. Bool BackendObject_Remote::InitializeEGLDisplay(EGLDisplay dpy, EGLint* major, EGLint* minor) { + WaitForApplyBeforeEglForwarder("InitializeEGLDisplay"); if (!Server::ServerInitializeEGLDisplay(dpy, major, minor)) return false; return MG_Backend::BackendObject::InitializeEGLDisplay(dpy, major, minor); } @@ -188,6 +210,7 @@ namespace MobileGL::MG_Remote::Client { const MG_Backend::WindowHandle& handle) { // The handle first: the server's backend has to know which window it is about to make // a surface for, and ServerSetWindowHandle is the only way to tell it. + WaitForApplyBeforeEglForwarder("CreateEGLWindowSurface"); Server::ServerSetWindowHandle(handle); if (!Server::ServerCreateEGLWindowSurface(surface, handle)) return false; // The server's surface init published the default framebuffer's shape as a @@ -202,6 +225,7 @@ namespace MobileGL::MG_Remote::Client { } Bool BackendObject_Remote::ResizeEGLWindowSurface(EGLSurface surface, Uint32 width, Uint32 height) { + WaitForApplyBeforeEglForwarder("ResizeEGLWindowSurface"); if (!Server::ServerResizeEGLWindowSurface(surface, width, height)) return false; // A resize re-creates the server's swapchain, which re-posts the surface-changed // event - same drain, same reason as CreateEGLWindowSurface. @@ -210,6 +234,7 @@ namespace MobileGL::MG_Remote::Client { } Bool BackendObject_Remote::CreateEGLPbufferSurface(EGLSurface surface, EGLint width, EGLint height) { + WaitForApplyBeforeEglForwarder("CreateEGLPbufferSurface"); if (!Server::ServerCreateEGLPbufferSurface(surface, width, height)) return false; // Same drain as the window surface: InitPbufferSurface publishes the default // framebuffer's depth/stencil format on SEG_EVENT from inside this very RPC. @@ -223,6 +248,7 @@ namespace MobileGL::MG_Remote::Client { // class latches "the surface is initialised" and calls InitCapabilities, because // InitCapabilities' answer comes from a snapshot the server can only publish once its // own InitCapabilities has run - and ServerMakeEGLCurrent is what publishes it. + WaitForApplyBeforeEglForwarder("MakeEGLCurrent"); if (!Server::ServerMakeEGLCurrent(dpy, draw, read, ctx)) return false; if (!MG_Backend::BackendObject::MakeEGLCurrent(dpy, draw, read, ctx)) return false; diff --git a/MobileGL/MG_Remote/Client/ClientSession.cpp b/MobileGL/MG_Remote/Client/ClientSession.cpp index bd749387..b1332132 100644 --- a/MobileGL/MG_Remote/Client/ClientSession.cpp +++ b/MobileGL/MG_Remote/Client/ClientSession.cpp @@ -139,7 +139,12 @@ namespace MobileGL::MG_Remote::Client { return timeoutNs / 1000000 + (timeoutNs % 1000000 != 0) + kBarrierTimeoutMs; } + // Defined below; declared here because the applied wait has to be able to drain + // SEG_EVENT while it is parked (P5e §2.6). + Uint32 DrainEventRing(Transport::EventRingConsumer& events); + Transport::SessionWait WaitForAppliedBudget(Transport::SessionProducer& producer, + Transport::EventRingConsumer& events, Uint64 seq, Uint64 remainingMs) { // The transport takes Uint32 milliseconds with UINT32_MAX meaning forever. // Keep even the largest GL timeout finite, using bounded chunks and preserving @@ -148,7 +153,23 @@ namespace MobileGL::MG_Remote::Client { for (;;) { const Uint32 chunkMs = remainingMs > kMaxFiniteWaitMs ? kMaxFiniteWaitMs : static_cast(remainingMs); - const auto wait = producer.WaitForApplied(seq, chunkMs); + // P5e (ra, CONTRACT-P5E §2.6): THE PARK BREAKS ON TWO THINGS. A server that + // ran out of SEG_EVENT stops producing and therefore stops applying, so a + // waiter that could only be woken by appliedSeq would wait for a watermark + // nothing is going to move - and it is holding the only drain there is. The + // second exit is that case: drain, clear the latch, ring the server, park + // again. `remainingMs` is deliberately NOT charged for it; a wake that did + // the queue's work is not the caller's budget being spent. + const auto wait = producer.WaitForAppliedOrEventBacklog(seq, chunkMs); + if (wait == Transport::SessionWait::Reached) { + Transport::RingControl* control = producer.Control(); + if (control == nullptr || + Transport::Watermark::Reached(control->appliedSeq, seq)) { + return wait; + } + DrainEventRing(events); + continue; + } if (wait != Transport::SessionWait::TimedOut || remainingMs <= chunkMs) return wait; remainingMs -= chunkMs; } @@ -592,6 +613,9 @@ namespace MobileGL::MG_Remote::Client { m_events = Transport::EventRingConsumer(m_shm.EventControl(), control, m_shm.EventRingBase(), m_shm.EventRingCapacity(), m_shm.EventSegmentBase()); + // P5e (ra, §2.6): the drain's other half. A server parked on a full SEG_EVENT is woken + // by the drain that emptied it, not by the next thing this client happens to publish. + m_events.SetServerDoorbell(&m_clientTransport->PeerDoorbell()); if (!m_replies.Valid() || !m_events.Valid()) { Stop(); return MOBILEGL_ERR_INVALID_ARGUMENT; @@ -629,6 +653,10 @@ namespace MobileGL::MG_Remote::Client { // half-wired shape this session exists not to have. m_encoder = Wire::PipeWireEncoder(control, &m_cmd, nullptr, &m_segments); m_encoder.SetStageRetirementDoorbell(m_producer.SelfDoorbell()); + // P5e (ra, §2.6): the stage bell is a client wait and every client wait drains. The + // encoder owns the park; the session owns SEG_EVENT, so the session lends it a drain. + m_encoder.SetStageWaitHook( + +[](void* self) { DrainEventRing(static_cast(self)->Events()); }, this); // ---- 7. the first CapsSnapshot, if the server had a backend to publish one from. // ONE DRAIN, ONE ADOPTER (c1): PumpControlPlane below is the only thing in the client @@ -814,7 +842,8 @@ namespace MobileGL::MG_Remote::Client { Uint64 ClientSession::EmitAndWaitTails(MG_Pipe::MGPWireOp op, const void* payload, Uint64 payloadBytes, const Wire::WireTail* tails, Uint32 tailCount, void* replyOut, Uint64 replyBytes, - Int32* statusOut, Uint64* replySizeOut) { + Int32* statusOut, Uint64* replySizeOut, + Bool wantReply) { Uint64 varTailBytes = 0; for (Uint32 i = 0; i < tailCount; ++i) varTailBytes += tails[i].Size; if (statusOut != nullptr) *statusOut = Wire::ReplySink::kStatusError; @@ -863,6 +892,12 @@ namespace MobileGL::MG_Remote::Client { wait == Transport::SessionWait::ShutDown ? "shutdown" : "timeout"); std::abort(); } + // P5e (ra, §2.6): EVERY WAIT EXIT DRAINS. The cmd-space wait is one of the two + // safety nets under run-ahead (the stage bell is the other), and a client parked + // here without draining is the other half of the flow-control deadlock: the server + // stops applying on a full event ring, so retiredSeq stops moving, so this wait + // never completes. One drain on the way out closes it. + DrainEventRing(m_events); seq = m_encoder.EncodeRecord(op, payload, payloadBytes, tails, tailCount); if (seq == Wire::kInvalidSeq) { MGLOG_F("MGPipe: Fatal{RingOverrun, \"SEG_CMD\"} - %s refused after sufficient " @@ -880,8 +915,63 @@ namespace MobileGL::MG_Remote::Client { // (R-16 / ID-31) - fourteen rows now, because the four Bool acceptance entry points // gained the flag. Asking the catalogue rather than the caller is what stops a caller // that forgot to pass a buffer from silently turning an answer into a guess. - const Bool ownsReplySlot = + // + // P5e (ra) ADDS ONE CALLER-SIDE CONJUNCT AND NO SECOND SOURCE OF TRUTH: `wantReply`. + // The catalogue still says which rows CARRY a slot; a caller that passes false says it + // will not read this one (resource_subdata's buffer half, §2.5 - the only such caller). + // The server posts either way, so the wire is unchanged and the row keeps its flag. + const Bool rowCarriesReplySlot = (MG_Pipe::MGPipeCallFlagsFor(op) & static_cast(MG_Pipe::kReplySlot)) != 0; + const Bool ownsReplySlot = rowCarriesReplySlot && wantReply; + + // ---- THE P5e WAIT RULE (CONTRACT-P5E §2.3) --------------------------------------- + // + // IT STANDS BEFORE THE BATCH SKIP AND REPLACES IT WHOLE, rather than adding a clause to + // it. The batch skip is a rule about CALL CLASSES ("a set/CSO/object record carries its + // whole value in the payload"); the wait class is a rule about WHAT AN APPLY READS, and + // it is generated from the same PipeCalls.def row the class comes from + // (MGPipeWaitClassFor, c0e). Two rules layered would mean a row could be skipped by the + // class rule and waited by the column, and the column is the one the SERVER's + // MGPipeBarriered reads - so the two halves of the wire would disagree about whether + // the client is parked behind this record, which is the single fact §4.4's exemptions + // and §3.3's detector both rest on. + // + // WITH RUN-AHEAD DISARMED NOTHING BELOW RUNS and the file's original shape continues - + // Magma, a monolith transport, MOBILEGL_IPC_RUN_AHEAD=0 and every build before the + // integration commit take the old path byte for byte, which is what makes RUN_AHEAD=0 + // a pure wait-rule A/B on identical server code (§5.8). + if (RunAheadArmed() && !ownsReplySlot) { + MG_Pipe::MGPipeWaitClass waitClass = MG_Pipe::MGPipeWaitClassFor(op); + // §2.5's named relaxation, and the ONLY place a static column answer is overridden: + // a kWaitReply row whose caller does not want the answer has nothing to wait FOR. + // It is spelled here rather than in the column because the column is per-op and + // this is per-call - the texture half of the same row still waits. + if (!wantReply && waitClass == MG_Pipe::kWaitReply) waitClass = MG_Pipe::kWaitNone; + switch (waitClass) { + case MG_Pipe::kWaitNone: + // Published and gone. Rule F is what makes this legal: the apply of an + // unbarriered record names no client memory at all (§3, §4, §5). + return seq; + case MG_Pipe::kWaitPresent: + // The credit wait already ran, in EmitPresent, BEFORE the encode (§2.4). There + // is nothing left to do here and waiting would turn the credit into a barrier. + return seq; + case MG_Pipe::kWaitApplied: + // §2.2's barriered rows: applier_reset, generate_mipmap (until tx2 lands its + // VerbMipRes arm), set_storage_block_binding, the five XFB span verbs and the + // CopyTex endpoint. Each still pulls a row or probes a registry, and the + // client's wait is exactly what keeps P5C's semantics legal for it (ruling 4). + // Falls through to the wait below. + break; + case MG_Pipe::kWaitReply: + case MG_Pipe::kWaitClassCount: + // Unreachable: `ownsReplySlot` is false here, and gen_pipe refuses a kWaitReply + // row without a slot and a kReplySlot row that does not wait (c0e's three + // negative controls). Treated as a wait rather than as a skip, because the + // wrong answer to "does this hang" is the recoverable one. + break; + } + } // MOBILEGL_IPC_BATCH_WAITS (default 1): a value-class record is published WITHOUT // waiting for its own apply. The barrier's one load-bearing reader is the backend @@ -912,7 +1002,13 @@ namespace MobileGL::MG_Remote::Client { // (set_texture_params, resource_subdata, resource_readback and get_texture_image // own reply slots and wait anyway; the resource_* transfers and object_death resolve // by handle). - if (!ownsReplySlot && MG_Config::Ipc.BatchWaits != 0 && + // + // AND IT IS DISARMED WHOLE UNDER RUN-AHEAD (P5e §2.3): the wait class above is the + // rule there, and a second skip rule reading a different column is how the two halves + // of the wire end up disagreeing about whether the client is parked. Written as a + // conjunct here rather than left to "the classes happen not to overlap", because the + // classes overlapping is one PipeCalls.def row away at any time. + if (!ownsReplySlot && !RunAheadArmed() && MG_Config::Ipc.BatchWaits != 0 && op != MG_Pipe::MGPWireOp::GenerateMipmap) { const MG_Pipe::MGPipeCallClass callClass = MG_Pipe::MGPipeCallClassFor(op); if (callClass == MG_Pipe::kCtxState || callClass == MG_Pipe::kCtxCso || @@ -931,7 +1027,8 @@ namespace MobileGL::MG_Remote::Client { const BarrierWaitScope waiting; const Uint64 waitBudgetMs = AppliedWaitBudgetMs(op, payload); - const Transport::SessionWait wait = WaitForAppliedBudget(m_producer, seq, waitBudgetMs); + const Transport::SessionWait wait = + WaitForAppliedBudget(m_producer, m_events, seq, waitBudgetMs); if (wait == Transport::SessionWait::ShutDown) { // The doorbell died: the server went away. The only thing that returns from a // kWaitForever park, and therefore the only way a client blocked in the barrier @@ -965,7 +1062,17 @@ namespace MobileGL::MG_Remote::Client { // clears when the writeback lands, and the writeback lands on this ring. DrainEventRing(m_events); - if (!ownsReplySlot) return seq; + if (!ownsReplySlot) { + // P5e (ra, §2.3). A barriered record's apply has returned and this thread is the + // only runnable one, so the four O-class SharedPtr rows the last fill left in + // gPipeInputs can go. Under lockstep they were harmless - the next verb's fill + // overwrote them - but under run-ahead the next fill may be a frame away, and a + // frontend VAO or program whose LAST owner is that block is one the apply thread + // can end up destroying (§2.7's deferred-destroy hazard). Released here rather + // than at the fill, because here is where the applier is provably idle. + if (RunAheadArmed()) ReleaseFillPins(); + return seq; + } // THE SAME WAIT, NOT A SECOND ONE. appliedSeq >= seq already means the server wrote // this record's answer, because it writes the slot before it advances the watermark. @@ -1000,6 +1107,166 @@ namespace MobileGL::MG_Remote::Client { Bool ClientSession::BarrierArmed() const { return m_barrierArmed; } + // ---- P5e (ra): the run-ahead latch and the waits it leaves behind --------------------- + + Bool ClientSession::RunAheadArmed() const { return m_runAheadArmed; } + // THE PRODUCER'S NUMBER, NOT A SECOND ONE. SessionProducer records it inside + // PublishAndNotify, which is the only place a record becomes published at all; a copy kept + // here would be a second thing to forget to write on a path that publishes without waiting + // - which, after P5e, is most of them. + Uint64 ClientSession::LastPublishedSeq() const { return m_producer.LastPublishedSeq(); } + Uint64 ClientSession::PresentCreditWaits() const { return m_presentCreditWaits; } + + void ClientSession::LatchRunAheadFromCaps() { +#if MOBILEGL_BUILD_DISAGGREGATED + // THE CONJUNCTION, IN THE CONTRACT'S OWN ORDER (§1). The caps read is last because it + // is the only one that costs a mirror load, and because it is the only one whose answer + // can change: the other two are process configuration. + const Bool armed = m_barrierArmed && MG_Config::Ipc.RunAhead != 0 && + Caps().HasCap(MG_Pipe::kCapRunAheadApply); + if (!m_runAheadLatched) { + m_runAheadLatched = true; + m_runAheadArmed = armed; + if (armed) { + MGLOG_I("MG_Remote client: run-ahead ARMED - the server publishes " + "kCapRunAheadApply, so an unbarriered record is published and not " + "waited for (CONTRACT-P5E §2.3), paced by a present credit of %u", + MG_Config::Ipc.PresentCredit); + } else if (MG_Config::Ipc.RunAhead != 0 && m_barrierArmed) { + // THE KNOB IS NOT A SWITCH ON ITS OWN, and an operator who set it has to be + // told that, or an A/B arm that measured nothing looks like an A/B arm that + // measured no difference (Config.h's own argument for parsing it everywhere). + MGLOG_W_ONCE("MG_Remote client: run-ahead requested, server does not publish " + "kCapRunAheadApply - running lockstep. This is Magma always, and " + "Espryt until the P5e integration commit flips " + "kMGPipeP5eRunAheadReady"); + } + return; + } + // A LATER SNAPSHOT MAY ONLY TURN IT OFF. Promotion would mean the records already + // published without a wait were published against a rule this server never agreed to, + // and nothing can un-publish them; demotion only adds waits, so it is safe and is + // exactly what a server that lost its run-ahead backend needs. + if (!armed && m_runAheadArmed) { + m_runAheadArmed = false; + MGLOG_W("MG_Remote client: run-ahead DISARMED by a later caps snapshot - every " + "record from here waits as it did before P5e"); + } +#endif + } + + void ClientSession::ReleaseFillPins() { +#if MOBILEGL_BUILD_DISAGGREGATED + // The block's own door: PipeInputs' O-class rows are private and MG_Remote has no + // business knowing which four they are. MG_Impl owns the fill, so MG_Impl owns the + // un-fill (PipeFill.cpp), and this side only says WHEN. + MG_Pipe::MGPipeReleaseResidualFillPins(); +#endif + } + + void ClientSession::WaitForApplyToCatchUp(const char* why) { +#if MOBILEGL_BUILD_DISAGGREGATED + // A NO-OP WITHOUT RUN-AHEAD, and that is not an optimisation: under lockstep the + // client already waited for every record it published, so appliedSeq is at + // LastPublishedSeq by construction and a second wait would only add a park on a + // watermark that is already reached. + if (!m_runAheadArmed || !m_started) return; + const Uint64 target = m_producer.LastPublishedSeq(); + if (target == 0) return; + const BarrierWaitScope waiting; + const Transport::SessionWait wait = + WaitForAppliedBudget(m_producer, m_events, target, kBarrierTimeoutMs); + if (wait == Transport::SessionWait::ShutDown) { + // The doorbell died: teardown, not a fault - the same answer EmitAndWaitTails + // gives, and for the same reason. + MGLOG_E("MG_Remote client: the forced wait for %s woke on a dead doorbell; the " + "server is gone", why); + return; + } + if (wait != Transport::SessionWait::Reached) { + MGLOG_F("MGPipe: Fatal{BarrierTimeout, \"%s\"} - a forced run-ahead wait did not " + "reach appliedSeq %llu within %u ms. The forced waits of CONTRACT-P5E §2.5 " + "are the points at which the client must be in step again, so a wedge here " + "is a wedge in the queue and not a slow frame", + why, static_cast(target), kBarrierTimeoutMs); + std::abort(); + } + // EVERY WAIT EXIT DRAINS (§2.6). It is what the flow-control deadlock argument rests + // on: the server blocks only on a full event ring, the client blocks only on + // watermarks the server advances, and every client wait drains - so at most one side + // is parked at any instant. + DrainEventRing(m_events); + ReleaseFillPins(); +#else + (void)why; +#endif + } + + void ClientSession::Finish() { + // glFinish. Nothing goes on the wire - Flush and Finish have no record (ARCHITECTURE + // §11) - and under lockstep there was nothing to do, because the client had already + // waited out every command it issued. Under run-ahead "the commands issued so far have + // completed" is a promise the queue can break, so this is where it is kept. + WaitForApplyToCatchUp("glFinish"); + } + + Uint64 ClientSession::AcquirePresentCredit() { +#if MOBILEGL_BUILD_DISAGGREGATED + // MINTED CLIENT-SIDE AND 1-BASED (§1). Before P5e the client sent 0 and the server + // stamped its own frame count, which was honest while nothing paced on it; a credit + // needs an id space only the payer can advance, and that is this one. + const Uint64 serial = m_presentsSent + 1; + if (!m_runAheadArmed) { + // Lockstep: the present record's own barrier IS the pacing, one frame deep by + // construction. The serial is still minted so that both arms carry the same field + // and the server's ReturnPresentCredit is fed on both - an A/B whose two arms + // disagree about a wire field is not an A/B (Config.h's rule for the knobs). + m_presentsSent = serial; + return serial; + } + const Uint32 credit = MG_Config::Ipc.PresentCredit; + if (m_presentsSent >= credit) { + // Wait for swap number (serial - credit) to have come back. With credit 1 that is + // the previous frame: the client publishes frame N+1's records while the server + // applies and swaps frame N, and blocks here until swap N returned - one frame of + // overlap and at most one frame of added latency (ruling 4 / ID-92). + const Uint64 awaited = serial - credit; + ++m_presentCreditWaits; + const BarrierWaitScope waiting; + Transport::SessionWait wait = Transport::SessionWait::TimedOut; + for (;;) { + // §2.6 again: the credit park breaks on a full SEG_EVENT too, for the same + // reason the applied park does - a server that stopped producing stopped + // swapping, and this waiter holds the only drain. + wait = m_producer.WaitForPresentAckOrEventBacklog(awaited, kBarrierTimeoutMs); + if (wait != Transport::SessionWait::Reached) break; + Transport::RingControl* control = m_producer.Control(); + if (control == nullptr || + Transport::Watermark::Reached(control->presentAckSerial, awaited)) { + break; + } + DrainEventRing(m_events); + } + if (wait == Transport::SessionWait::TimedOut) { + MGLOG_F("MGPipe: Fatal{PresentCreditTimeout} - present %llu waited %u ms for " + "swap %llu to come back with a credit of %u. The credit, never the " + "ring's bytes, is what paces a run-ahead client (CONTRACT-P5E §2.4), " + "so a credit that never returns is a server that stopped swapping", + static_cast(serial), kBarrierTimeoutMs, + static_cast(awaited), credit); + std::abort(); + } + // ShutDown is teardown and returns: the present did not happen, the same answer + // the barrier gives on a dead doorbell. + if (wait == Transport::SessionWait::Reached) DrainEventRing(m_events); + } + m_presentsSent = serial; + return serial; +#else + return ++m_presentsSent; +#endif + } + // R-1's mutual-exclusion invariant, as two probes that answer honestly. // // THE CLIENT'S FLAG IS THREAD-LOCAL AND THE SERVER'S IS NOT, and the asymmetry is the @@ -1012,7 +1279,8 @@ namespace MobileGL::MG_Remote::Client { return g_applyThreadInsideApplier.load(std::memory_order_acquire); } - void ClientSession::RefusePipeInputsTouchWhileApplierOwnsIt(const char* surface) { + void ClientSession::RefusePipeInputsTouchWhileApplierOwnsIt(const char* surface, + Bool isBarrieredFill) { // THE CHAIN IS A CONJUNCTION, SO ITS ORDER IS FREE - AND IT IS ORDERED CHEAPEST-FIRST // (P5d round 3, package D). Every line below is a pure predicate whose only effect is // "no violation, go home"; the Fatal fires on the AND of all of them, so reordering @@ -1028,6 +1296,28 @@ namespace MobileGL::MG_Remote::Client { // both miss do we pay for a role probe, two singleton reads, a shared atomic and the // thread_local at the end. if (MG_Config::Transport == MG_Config::TransportMode::Monolith) return; + // ---- P5e (ra), CONTRACT-P5E §3.5: THE RUN-AHEAD RULE IS A DIFFERENT RULE ----------- + // + // It is first, before the BatchWaits return, because BatchWaits' argument does not + // survive run-ahead: "the pull-verbs' own wait keeps their pulled reads fenced" is a + // statement about a client that waits at every verb, and this one does not. Under + // run-ahead the block is SERVER-ROLE memory (§3) and the single legal GL-thread write + // is the residual fill of a BARRIERED record - the one the client is about to park + // behind. Every other touch races the applier by construction, so there is no + // in-applier flag to consult and no "count it" arm: the value would be torn. + // + // This is where a fill left standing for an unbarriered record lands, by name. + if (ClientSessionInstance().RunAheadArmed()) { + if (isBarrieredFill) 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); + std::abort(); + } // MOBILEGL_IPC_BATCH_WAITS=1 makes "the fill runs while the apply thread applies an // earlier value record" the INTENDED shape: the fill writes only fields no record // supplies (the wire-live skips are the same answer the emitters use), the applier @@ -1090,6 +1380,14 @@ namespace MobileGL::MG_Remote::Client { } if (AdoptCapsSnapshot(envelope->msg_as_CapsSnapshot())) ++adopted; } + // P5e (ra, §1): the run-ahead latch rides R-12's invalidation edge, because "the first + // caps adoption after Start" and "the one place a CapsSnapshot becomes a mirror + // generation" are the same instant, and a latch taken anywhere else would be reading a + // mirror whose generation nobody had adopted yet. Called even when `adopted` is 0: the + // first call in Start() is what takes the latch, and a pump that adopted nothing still + // has to leave it taken (a handshake with no snapshot latches "not armed", which is + // the honest answer for a server that never said what it consumes). + if (adopted != 0 || !m_runAheadLatched) LatchRunAheadFromCaps(); return adopted; } @@ -1167,9 +1465,9 @@ namespace MobileGL::MG_Remote::Client { MGLOG_I("MG_Remote client: wire ledger: maxrec=%llu maxrecop=%s cap=%llu (%llu.%llu%% of " "RingProducer::MaxRecordBytes, half of a %llu byte SEG_CMD) cmdbytes=%llu " "ringwraps=%llu ringpads=%llu " - "ringwaits=%llu emitseq=%llu cliwait=%llu clipark=%llu - R-10's proof obligation, " - "R-9's producer readings and P5d round 3's client wait pair, published from the " - "session that produced them", + "ringwaits=%llu emitseq=%llu cliwait=%llu clipark=%llu credit-waits=%llu - " + "R-10's proof obligation, R-9's producer readings, P5d round 3's client wait " + "pair and P5e's present credit, published from the session that produced them", static_cast(maxRecord), m_encoder.MaxRecordOpName(), static_cast(cap), static_cast(permille / 10), @@ -1185,7 +1483,13 @@ namespace MobileGL::MG_Remote::Client { // profile runs did not set MOBILEGL_PIPE_STATS either, and "did the barrier spin // or did it park" is the question their logs have to be able to answer. static_cast(m_producer.Waits()), - static_cast(m_producer.Parks())); + static_cast(m_producer.Parks()), + // P5e (ra, §2.4). The number the device exit reads to say whether the present + // credit is what paces this workload: 0 with credit 1 means the server is + // never the frame's critical path, and a count near the frame count means it + // always is. It is on this line rather than only on the stats line for + // LogWireLedger's own reason - a trace replay never reaches a stats summary. + static_cast(m_presentCreditWaits)); } #undef MGP5_C0_STUB diff --git a/MobileGL/MG_Remote/Client/ClientSession.h b/MobileGL/MG_Remote/Client/ClientSession.h index b4ff76df..a0a0f2c7 100644 --- a/MobileGL/MG_Remote/Client/ClientSession.h +++ b/MobileGL/MG_Remote/Client/ClientSession.h @@ -112,15 +112,79 @@ namespace MobileGL::MG_Remote::Client { // or its MGPDrawIndirect block behind the range array). The one-tail form above is this // one with a single WireTail; the barrier policy lives in exactly one body. A distinct // name rather than an overload, because `nullptr, 0` would match both. + // P5e (ra, CONTRACT-P5E §2.5): `wantReply` is the ONE caller-side relaxation of the + // catalogue's reply rule, and it exists for exactly one row - resource_subdata's + // BUFFER half, whose Bool the only caller discards (PipeFill.cpp's + // MGPipeEmitResourceSubData). The row keeps kReplySlot on the wire (the server still + // posts; an unread slot is harmless, ReplySlot.h:16, 105), so nothing about the + // protocol moves - what moves is that under run-ahead this client neither waits for + // the answer nor reads it, which is what makes the 64 KB persistent-map push + // fire-and-forget instead of a hidden round trip per block. It is NOT a licence to + // skip an answer a caller uses: R-5 still forbids re-deriving one locally, and every + // other kReplySlot row passes true (the default) and waits. Uint64 EmitAndWaitTails(MG_Pipe::MGPWireOp op, const void* payload, Uint64 payloadBytes, const Wire::WireTail* tails, Uint32 tailCount, void* replyOut, Uint64 replyBytes, Int32* statusOut, - Uint64* replySizeOut = nullptr); + Uint64* replySizeOut = nullptr, Bool wantReply = true); // MOBILEGL_IPC_VERB_BARRIER. False is the R-1 negative control and is EXPECTED to be // red; it must be run once and the way it goes red recorded. Bool BarrierArmed() const; + // ---- P5e (ra): the wait rule (CONTRACT-P5E §1, §2.3, §2.4, §2.5) ------------------- + + // A CONJUNCTION OF THREE, LATCHED ONCE: `m_barrierArmed && Ipc.RunAhead && + // Caps().HasCap(kCapRunAheadApply)`. The third conjunct is the server's own statement + // that it applies an unbarriered record without reading client memory, so run-ahead is + // never something the client can turn on by itself - on Magma, and on Espryt until the + // integration commit publishes bit 10, this answers false and EmitAndWaitTails runs + // today's lockstep path byte for byte. + // + // LATCHED AT THE FIRST CAPS ADOPTION AFTER Start, AND A LATER SNAPSHOT MAY ONLY TURN IT + // OFF. A caps re-run cannot promote a lockstep server to a run-ahead one mid-frame: + // records already published without a wait would have been published against the wrong + // rule, and there is no way to un-publish them. Demotion is safe in the other + // direction - it only adds waits - so it is allowed. + Bool RunAheadArmed() const; + + // §2.3: the four O-class SharedPtr rows of gPipeInputs (PipeInputs.h:801-809 - the + // bound VAO and the three programs), dropped on the GL thread once a barriered apply + // has returned. Under lockstep the next fill overwrites them within a verb; under + // run-ahead the next fill may be a frame away, and a pinned VAO whose last owner ends + // up being the apply thread is exactly the deferred-destroy hazard §2.7 keeps a queue + // for. Releasing them at the one instant the applier is provably idle costs four + // refcount drops per barriered record and removes the pin entirely. + void ReleaseFillPins(); + + // §2.5's forced wait, as one named entry point: publish nothing, wait for the apply + // thread to reach LastPublishedSeq, drain the reverse channel. `why` names the caller + // in the timeout Fatal, because "which forced wait wedged" is the whole diagnostic. + // A no-op when run-ahead is not armed (the client is already in step by construction). + void WaitForApplyToCatchUp(const char* why); + + // glFinish. Flush and Finish stay no-ops ON THE WIRE (ARCHITECTURE §11, :415) - there + // is no record to emit - but "the GL commands issued so far have completed" cannot be + // true while a run-ahead queue still holds them, so under run-ahead glFinish becomes + // exactly WaitForApplyToCatchUp + the drain that comes with it. + void Finish(); + + // The last seq this session published, whether or not it waited for it. The forced + // waits above are defined against it. + Uint64 LastPublishedSeq() const; + + // §2.4's counter, published on the stats line as `credit-waits`: how often the client + // actually blocked for a swap to come back. Zero with a credit of 1 means the server + // is never the frame's critical path; a number close to the frame count means the + // credit is what paces this workload, which is what the device exit measures. + Uint64 PresentCreditWaits() const; + + // §2.4, called by EmitPresent immediately before it encodes: pay the credit (parking + // if this client already has `MOBILEGL_IPC_PRESENT_CREDIT` presents in flight), then + // mint and return this frame's 1-based MGPPresent::FrameSerial. The credit wait is + // BEFORE the encode on purpose: a client that encoded first would hold a SEG_CMD + // reservation across the park. + Uint64 AcquirePresentCredit(); + // R-1's invariant made checkable rather than only written down: true while this // thread is inside a barrier wait. The apply thread sets its own flag on entry to the // applier; a debug/verify build asserts the two are never both true, and that the @@ -140,7 +204,17 @@ namespace MobileGL::MG_Remote::Client { // split session is live, when the caller IS the apply thread, and when the barrier is // disarmed - MOBILEGL_IPC_VERB_BARRIER=0 is R-1's negative control and its red belongs // to Fatal{BarrierViolation}, not to this check. - static void RefusePipeInputsTouchWhileApplierOwnsIt(const char* surface); + // + // 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 + // it: batching is about how many round trips a lockstep client pays, and it says + // nothing about a client that pays none. + static void RefusePipeInputsTouchWhileApplierOwnsIt(const char* surface, + Bool isBarrieredFill = false); // ---- c1's additions --------------------------------------------------------------- @@ -250,9 +324,20 @@ namespace MobileGL::MG_Remote::Client { void LogWireLedger() const; private: + // P5e (ra): the latch, taken from PumpControlPlane after every adoption. Private + // because "when may run-ahead start" is this class's own rule and not a caller's. + void LatchRunAheadFromCaps(); + Wire::PipeWireEncoder m_encoder; Wire::SegmentTable m_segments; Bool m_barrierArmed = true; + // P5e (ra): the latch of RunAheadArmed() and the fact that it has been taken once. + Bool m_runAheadArmed = false; + Bool m_runAheadLatched = false; + // §2.4: presents published (the 1-based FrameSerial space) and how often the credit + // wait really blocked. + Uint64 m_presentsSent = 0; + Uint64 m_presentCreditWaits = 0; std::unique_ptr m_clientTransport; std::unique_ptr m_serverTransport; diff --git a/MobileGL/MG_Remote/Client/EmitTables.cpp b/MobileGL/MG_Remote/Client/EmitTables.cpp index ac5dc9e4..3cc9451f 100644 --- a/MobileGL/MG_Remote/Client/EmitTables.cpp +++ b/MobileGL/MG_Remote/Client/EmitTables.cpp @@ -1034,10 +1034,20 @@ namespace MobileGL::MG_Remote::Client { } MG_Pipe::MGPPresent record{}; - // FrameSerial 0 = "the server stamps its own". P5 has no client-side present credit - // (MOBILEGL_IPC_PRESENT_CREDIT is P6's), so a client-minted serial would be a second - // id space with no consumer. - record.FrameSerial = 0; + // P5e (ra, CONTRACT-P5E §1, §2.4). FrameSerial USED TO BE 0 - "the server stamps + // its own" - and that was honest while nothing paced on it. It is now minted here, + // 1-based, by AcquirePresentCredit, which also PAYS the credit: if this client + // already has MOBILEGL_IPC_PRESENT_CREDIT presents in flight it parks until the + // server's OnPresent returns one, and only then does it mint. + // + // THE CREDIT WAIT IS BEFORE THE ENCODE, WHICH IS NOT A DETAIL: EmitAndWait + // reserves SEG_CMD bytes as its first act, so a client that encoded and then + // parked would hold a ring reservation across a whole frame of server time. It + // also drains SEG_EVENT on its way out, like every other wait (§2.6). + // + // With run-ahead disarmed this is a counter and nothing else, and the record below + // travels exactly as it did - the present row's own barrier is the pacing there. + record.FrameSerial = session.AcquirePresentCredit(); session.EmitAndWait(MG_Pipe::MGPWireOp::Present, &record, sizeof(record), nullptr, 0, nullptr, 0, nullptr); diff --git a/MobileGL/MG_Remote/Client/WireTables.cpp b/MobileGL/MG_Remote/Client/WireTables.cpp index 947fb771..ae11348b 100644 --- a/MobileGL/MG_Remote/Client/WireTables.cpp +++ b/MobileGL/MG_Remote/Client/WireTables.cpp @@ -340,15 +340,33 @@ namespace MobileGL::MG_Remote::Client { ClientSession& session = RequireSession("ResourceSubData"); MG_Pipe::MGPSubData record = *payload; record.Blob = StageOptional(session, blobBytes, blobByteCount); + // P5e (ra, CONTRACT-P5E §2.5 / ruling 15): the BUFFER half does not want its + // answer, and MGPipeSubDataWantsItsReply is the one place that decides - the same + // function the route reads, so the two halves of this call cannot disagree about + // whether a wait is owed. Under run-ahead the record is published and this thread + // returns; the persistent-map push (PersistentMapTracker's 64 KB blocks) stops + // being a hidden round trip per block, which is the single biggest wait left on + // the steady path that is not a draw. + // + // THE ANSWER IS ACCEPT-BY-CONSTRUCTION, AND THAT IS HONEST HERE AND NOWHERE ELSE: + // the only caller discards it (PipeFill.cpp's MGPipeEmitResourceSubData), so + // "accepted" is not a re-derivation of a server decision - it is the absence of a + // question. The server still posts the real answer into the slot; nothing reads + // it, which ReplySlot.h:16, 105 makes legal. A row whose acceptance a caller USES + // may never take this path - R-5 has not moved. + const Bool wantReply = MG_Pipe::MGPipeSubDataWantsItsReply(record); Int32 status = 0; - const Uint64 seq = session.EmitAndWait( - MGPWireOp::ResourceSubData, &record, sizeof(record), varTail, - static_cast(varTailCount) * sizeof(MG_Pipe::MGPSubRegion), nullptr, 0, - &status); + const Wire::WireTail tail{varTail, static_cast(varTailCount) * + sizeof(MG_Pipe::MGPSubRegion)}; + const Uint64 seq = session.EmitAndWaitTails( + MGPWireOp::ResourceSubData, &record, sizeof(record), + varTail != nullptr ? &tail : nullptr, varTail != nullptr ? 1u : 0u, nullptr, 0, + &status, nullptr, wantReply); reply->Id = seq; - MG_Pipe::MGPipePostReply(*reply, status, status == 0 ? 1u : 0u); + MG_Pipe::MGPipePostReply(*reply, wantReply ? status : 0, + (wantReply ? status == 0 : true) ? 1u : 0u); ++g_emitted; - if (status == 1) ++g_declined; + if (wantReply && status == 1) ++g_declined; } void Wire_BufferSubDataResident(const MG_Pipe::MGPSubData* payload, const void* blobBytes, diff --git a/MobileGL/MG_Remote/Server/PipeApplier.cpp b/MobileGL/MG_Remote/Server/PipeApplier.cpp index cfd3f735..645e9f22 100644 --- a/MobileGL/MG_Remote/Server/PipeApplier.cpp +++ b/MobileGL/MG_Remote/Server/PipeApplier.cpp @@ -10,6 +10,7 @@ #include "PipeApplier.h" +#include "ServerSession.h" #include "../Transport/ReplySlot.h" #include @@ -29,6 +30,20 @@ namespace MobileGL::MG_Remote::Server { + namespace { + // P5e (ra, CONTRACT-P5E §1 / §6): DOES THIS SERVER PUBLISH kCapRunAheadApply? The + // question is asked of the server's OWN CallMask and not of a build constant, because + // "the client may run ahead" is exactly what that bit says and Magma never sets it. + // A session with no CallMask yet (the bring-up window, a fixture that never called + // SetCapabilityBits) answers false: no client can have latched run-ahead against a + // snapshot that was never published. + Bool ServerPublishesRunAhead() { + const ServerSession* session = ServerSession::Active(); + if (session == nullptr || !session->CallMaskIsSet()) return false; + return (session->CallMask() & static_cast(MG_Pipe::kCapRunAheadApply)) != 0; + } + } // namespace + ReplyPool::ReplyPool(void* base, Uint64 sizeBytes, Uint32 slotCount, Uint32 slotBytes) : m_base(static_cast(base)), m_size(sizeBytes), m_slots(slotCount), m_slotBytes(slotBytes) {} @@ -288,7 +303,30 @@ namespace MobileGL::MG_Remote::Server { ++m_presents; // FrameSerial 0 means "the server stamps its own" (c1-v1 8.3): P5 has no client-side // present credit, so the client sends 0 and the frame count on this side IS the serial. + // + // P5e (ra, CONTRACT-P5E §1, §2.4): IT IS THE CLIENT'S NOW, 1-BASED AND MINTED BY THE + // PAYER. A credit can only be paced in an id space the waiter advances, and the waiter + // is the client (`WaitForPresentAck(m_presentsSent + 1 - credit)`), so a server-stamped + // serial would be the server acknowledging its own count. The 0 arm survives for a + // peer that has not been re-built - and on a server that PUBLISHES the run-ahead cap it + // is a protocol fault, because such a client is pacing on this answer and a 0 would + // acknowledge a frame nobody asked about. + if (present.FrameSerial == 0 && ServerPublishesRunAhead()) { + MGLOG_F("MGPipe: Fatal{ProtocolCorruption, \"Present.FrameSerial\"} - a run-ahead " + "server was handed present serial 0. The client mints this 1-based and " + "waits on it for its credit (CONTRACT-P5E §2.4); returning a credit for " + "serial 0 would release a wait that is asking about frame N"); + std::abort(); + } m_lastPresentSerial = present.FrameSerial != 0 ? present.FrameSerial : m_presents; + // §2.4's other half, and the reason ServerSession::ReturnPresentCredit has had no + // production caller since v1 wrote it: ONE CREDIT PER SWAP, returned after Present() + // has returned rather than before it, because what the client is waiting for is the + // swap and not the record's apply. It advances presentAckSerial AND rings the client's + // bell - a client parked in WaitForPresentAck(kWaitForever) needs the pair. + if (ServerSession* session = ServerSession::Active()) { + session->ReturnPresentCredit(m_lastPresentSerial); + } return true; } diff --git a/MobileGL/MG_Remote/Wire/PipeWireCodec.cpp b/MobileGL/MG_Remote/Wire/PipeWireCodec.cpp index 342b3e6e..30cc44ce 100644 --- a/MobileGL/MG_Remote/Wire/PipeWireCodec.cpp +++ b/MobileGL/MG_Remote/Wire/PipeWireCodec.cpp @@ -885,11 +885,19 @@ namespace MobileGL::MG_Remote::Wire { // The allocation still cannot progress after reclamation. Count this // blocked allocation once, not each watermark poll or each reclaimed mark. if (!waited) { ++m_stageReclaimWaits; waited = true; } - if (!m_stageRetirementBell->Wait(m_control->producerParked, ready, 0, 5000)) { + // P5e (ra, CONTRACT-P5E §2.6): drain BEFORE parking and again after waking. + // Before, because the ring may already be full and the server already stopped + // - in which case retiredSeq will never move and there is nothing to wake us; + // after, because the wake may have come from a server that is about to fill it + // again. The hook is null in every standalone codec, which has no session and + // therefore no reverse channel to drain. + if (m_stageWaitHook != nullptr) m_stageWaitHook(m_stageWaitSelf); + if (!ready() && !m_stageRetirementBell->Wait(m_control->producerParked, ready, 0, 5000)) { MGLOG_F("MGPipe: Fatal{RetirementWaitFailed, \"SEG_STAGE\"} producer wait " "ended before the pending allocation retired (shutdown or timeout)"); std::abort(); } + if (m_stageWaitHook != nullptr) m_stageWaitHook(m_stageWaitSelf); } ReclaimStagedBytes(); } diff --git a/MobileGL/MG_Remote/Wire/PipeWireCodec.h b/MobileGL/MG_Remote/Wire/PipeWireCodec.h index 05d42005..2bf944c3 100644 --- a/MobileGL/MG_Remote/Wire/PipeWireCodec.h +++ b/MobileGL/MG_Remote/Wire/PipeWireCodec.h @@ -340,6 +340,20 @@ namespace MobileGL::MG_Remote::Wire { // without a consumer cannot wait for retirement and retain the named refusal. void SetStageRetirementDoorbell(Transport::Doorbell* bell) { m_stageRetirementBell = bell; } + // P5e (ra, CONTRACT-P5E §2.6): THE STAGE BELL IS A CLIENT WAIT, so it has to drain the + // reverse channel like every other one. The encoder cannot drain it itself - SEG_EVENT + // and its consumers belong to the session - so the session installs a hook and this + // layer only says WHEN. Without it the flow-control deadlock is one buffer upload + // wide: the server stops applying on a full event ring, retiredSeq stops moving, and a + // client parked here for staged bytes never drains the ring that would release it. + // A raw function pointer rather than std::function: this header sits below MG_Impl and + // the call is on a path that is already about to park. + using StageWaitHook = void (*)(void* self); + void SetStageWaitHook(StageWaitHook hook, void* self) { + m_stageWaitHook = hook; + m_stageWaitSelf = self; + } + // Bytes this encoder has ever written into SEG_CMD, pad fillers included: the // producer's monotonic head cursor. It is the DENOMINATOR the wrap count only means // anything against - "0 wraps" is a defect when the run pushed more bytes than the ring @@ -383,6 +397,9 @@ namespace MobileGL::MG_Remote::Wire { Uint64 m_cmdWrapPads = 0; Uint64 m_stageReclaimWaits = 0; Transport::Doorbell* m_stageRetirementBell = nullptr; + // P5e (ra): the session's event drain, called around the stage-bell park (§2.6). + StageWaitHook m_stageWaitHook = nullptr; + void* m_stageWaitSelf = nullptr; Vector m_stageMarks; SizeT m_stageMarkFront = 0; Uint8* m_stageBase = nullptr;