diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index b9bbd91f..3727fffa 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -1380,6 +1380,66 @@ jobs: fi echo "integration-magma-split is red by design on ${marker}" + # ---- P5e (ra2), ID-134 / ID-82: THE CLIENT-ARRAY LANE, EXPECTED RED UNDER RUN-AHEAD ---- + # + # The two DirectGLES.Split.ClientVertexArrayScenario entries draw from a vertex array in + # the application's OWN memory. The server uploads those bytes by dereferencing a raw + # client pointer per draw, which is exactly the read rule F forbids once the client stops + # waiting - so ID-82 makes the draw a NAMED REFUSAL under run-ahead and gives the staging + # form to P8. Aborting is therefore the DESIGNED behaviour, and a lane that demanded these + # entries green would be demanding that the refusal not fire. + # + # THE LABEL, AND WHY IT IS NOT `integration-split-clientarrays` (ID-131): `ctest -L` is a + # REGEX. That spelling is matched by `-L integration-split` as a substring, so the entries + # would still be in the lane they were moved out of while every listing showed them + # renamed. Counted, not assumed: `-L integration-split` went 181 -> 179 and + # `-L integration-gpu` 1359 -> 1357 when this label landed. + # + # IT BRANCHES ON WHETHER RUN-AHEAD IS ACTUALLY ARMED, and that is not hedging - it is the + # only formulation that is a real assertion on BOTH heads. kMGPipeP5eRunAheadReady is a + # BUILD constant (MG_Backend/Init.cpp): on a head where it is false no environment can arm + # run-ahead, the refusal is unreachable, and these entries are green - so a step that + # simply demanded red would fail for the one reason that is not a defect. The client says + # which world it is in, in its own log line, and the step reads that and then demands the + # matching outcome. When the flip lands for good, delete the green branch. + - name: Integration-clientarrays-split - expected red under run-ahead (ID-82 / ID-134) + working-directory: build-split + env: + MOBILEGL_ITEST_REQUIRE_GPU: "1" + MOBILEGL_TRANSPORT: "inproc" + run: | + ulimit -c unlimited + logs=MobileGL/MG_IntegrationTest/split-logs + rm -f "${logs}"/*ClientVertexArrayScenario*.log 2>/dev/null || true + rc=0 + ctest -L integration-clientarrays-split --no-tests=error -j 1 \ + > "${RUNNER_TEMP}/clientarrays.txt" 2>&1 || rc=$? + tail -5 "${RUNNER_TEMP}/clientarrays.txt" + if ! grep -rqF "run-ahead ARMED" "${logs}"/*ClientVertexArrayScenario*.log 2>/dev/null; then + # kMGPipeP5eRunAheadReady is false on this head: the client is lockstep, vi's arm is + # alive and these entries are the positive control they were registered as. + if [ "${rc}" -ne 0 ]; then + echo "::error::integration-clientarrays-split is red WITHOUT run-ahead armed. Under lockstep the client-memory array path is legal and vi keeps it working, so this is a real regression in that arm and not ID-82's refusal." + exit 1 + fi + echo "integration-clientarrays-split is green: run-ahead is not armed on this build, so ID-82's refusal is unreachable" + exit 0 + fi + if [ "${rc}" -eq 0 ]; then + echo "::error::integration-clientarrays-split went GREEN with run-ahead ARMED. ID-82 refuses a client-memory vertex array under run-ahead by name, so a green run means the refusal did not fire and the server dereferenced a client pointer from an unbarriered record. If P8 has landed the staged form, delete this step and fold the entries back into integration-split." + exit 1 + fi + marker='Fatal{UnmigratedVerb, "' + if ! grep -rhoE 'Fatal\{UnmigratedVerb, "(Multi)?DrawArrays\+CLIENT_ARRAYS"\}' \ + "${logs}"/*ClientVertexArrayScenario*.log 2>/dev/null | sort -u | grep -q CLIENT_ARRAYS; then + echo "::error::integration-clientarrays-split is red but no private log carries ${marker}...+CLIENT_ARRAYS\"} - the lane failed for some OTHER reason, which is a defect rather than the expected-red control" + grep -rhoE '(Fatal|Admitted)\{[A-Za-z]*, "[^"]*"\}' "${logs}"/*ClientVertexArrayScenario*.log 2>/dev/null | sort | uniq -c | sort -rn | head + exit 1 + fi + echo "integration-clientarrays-split is red by design on:" + grep -rhoE 'Fatal\{UnmigratedVerb, "(Multi)?DrawArrays\+CLIENT_ARRAYS"\}' \ + "${logs}"/*ClientVertexArrayScenario*.log | sort -u + - name: Upload strict lane logs if: always() uses: actions/upload-artifact@v7 diff --git a/MobileGL/MG_Backend/DirectGLES/MultiDraw.cpp b/MobileGL/MG_Backend/DirectGLES/MultiDraw.cpp index 7229ec79..c8f0060c 100644 --- a/MobileGL/MG_Backend/DirectGLES/MultiDraw.cpp +++ b/MobileGL/MG_Backend/DirectGLES/MultiDraw.cpp @@ -84,13 +84,8 @@ namespace MobileGL::MG_Backend::DirectGLES::MultiDrawImpl { constexpr SizeT kMaxComputeWorkGroups = 65535; constexpr SizeT kMaxComputeFlattenedIndices = kMaxComputeWorkGroups * kComputeWorkGroupSize; - Uint BoundDrawIndirectBufferId() { - const auto& indirect = - MGB_CTX->GetBufferBindingSlot(BufferTarget::DrawIndirect).GetBoundObject(); - if (!indirect) return 0; - const auto* resource = BufferImpl::EnsureBufferResource(indirect); - return resource ? resource->id : 0; - } + // BoundDrawIndirectBufferId MOVED (P5e ra2, ID-136) to sit beside BoundIndexBufferId, + // which is the same question about the other target and now has the same two arms. // --------------------------------------------------------------------------- // The bound index buffer, and WHICH SIDE ANSWERS FOR IT @@ -150,6 +145,24 @@ namespace MobileGL::MG_Backend::DirectGLES::MultiDrawImpl { std::abort(); } + // ---- P5e (ra2), ID-136: THE SAME REFUSAL FOR THE INDIRECT TARGET ------------------- + // + // A sibling of mv's rather than a second idiom, because it is the same sentence about + // the other buffer target: a missing record on the handle arm aborts by name and never + // falls back to the frontend binding slot, which answers for whatever the CLIENT has + // bound NOW - a later verb than the one being applied. + [[noreturn]] void RefuseMissingIndirectBufferRecord(const char* entry, + MG_Pipe::MGPipeHandle res) { + MGLOG_F("MGPipe: Fatal{RoleViolation, \"multidraw-indirect-buffer-arm\"} - %s found no " + "backend resource for the indirect buffer {%u, %u} this verb's record named. " + "The indirect buffer of this family is MGPipeApplier().VerbIndirectBuffer " + "(CONTRACT-P5E §2.1) and the arm is selected by Transport != Monolith " + "(ID-81); falling back to the frontend GL_DRAW_INDIRECT_BUFFER slot here " + "would restore whatever the CLIENT has bound now", + entry, res.Slot, res.Gen); + std::abort(); + } + // Does the applier's record say the application SUPPLIED this store's content? The M-3 // rule ScopedRestartIndexSubstitution spells: for a supplied store a coverage gap in the // server shadow is a missing record and Fatal by name; for one the application ORPHANED @@ -247,6 +260,57 @@ namespace MobileGL::MG_Backend::DirectGLES::MultiDrawImpl { return ResolveBoundIndexBuffer(IndexBufferQuestion::DriverName, "BoundIndexBufferId").Id; } + // The GL name on GL_DRAW_INDIRECT_BUFFER, i.e. what a tier that swaps in its own scratch + // COMMAND buffer has to put back - the exact twin of BoundIndexBufferId above, and now + // with the same two arms. + // + // ---- P5e (ra2), ID-136: THE SEAT THIS FILE'S OWN RULE HAD MISSED ------------------- + // + // This was the LAST unguarded frontend read on the multi-draw apply path, and it sat + // twenty lines above the function that retired its neighbour. It asked + // `MGB_CTX->GetBufferBindingSlot(BufferTarget::DrawIndirect)` and then + // `EnsureBufferResource()` - a registry lookup keyed by the client's + // identity, which is §4.4's rule and not only the allocator's. Under run-ahead that is + // `Fatal{UnmigratedPipeInput, "GetBufferBindingSlot@DrawArrays"}` on every batch the + // indirect tiers execute: 18 lane entries, and the only thing standing between the flip + // and a green lane once the fill race was fixed. + // + // WHAT IT IS NOT: a data dependency. The tier does not want the client's indirect + // buffer - it never reads a byte of it. It binds its OWN scratch command buffer + // (g_indirectCommands) and wants to put back the name that was there. So the answer is + // not "migrate the value" (which is what P8 owes for the ordinary indirect draw path); + // it is "ask the side that did the binding". ID-133's escalation (iii) legalised the + // pull instead, at the price of a rendezvous on every plain glMultiDraw* on the DEFAULT + // tier - a real cost on the shipping arm, paid to make a lane green. ID-136 withdraws + // that escalation and retires the read, in the commit that adds this arm. + // + // WHY MGPipeApplier().VerbIndirectBuffer IS THE WHOLE ANSWER ON THIS ARM: with a + // transport, the ONLY writer of this process's GL_DRAW_INDIRECT_BUFFER outside this + // function is DirectGLES.cpp's DrawSyncBit::IndirectBuffer arm, which binds from exactly + // that handle and nothing else. So "what was bound" IS "what the verb's record named", + // and a null handle is "this verb bound none" - the same 0 the monolith arm returns for + // an empty slot, and not the arm test (ID-110: the arm was decided by the transport + // above, never inferred from a null). + Uint BoundDrawIndirectBufferId() { +#if MOBILEGL_BUILD_DISAGGREGATED + if (MG_Config::Transport != MG_Config::TransportMode::Monolith) { + const MG_Pipe::MGPipeHandle res = MG_Pipe::MGPipeApplier().VerbIndirectBuffer; + if (MG_Pipe::MGPipeHandleIsNull(res)) return 0; + auto* resource = BufferImpl::EnsureBufferResourceForHandle(nullptr, res); + if (resource == nullptr) { + RefuseMissingIndirectBufferRecord("BoundDrawIndirectBufferId", res); + } + return resource->id; + } +#endif + // MONOLITH GLUE from here down, token for token what this function did before. + const auto& indirect = + MGB_CTX->GetBufferBindingSlot(BufferTarget::DrawIndirect).GetBoundObject(); + if (!indirect) return 0; + const auto* resource = BufferImpl::EnsureBufferResource(indirect); + return resource ? resource->id : 0; + } + // --------------------------------------------------------------------------- // Scratch GL objects // diff --git a/MobileGL/MG_Backend/Init.cpp b/MobileGL/MG_Backend/Init.cpp index 341e7ece..6d947032 100644 --- a/MobileGL/MG_Backend/Init.cpp +++ b/MobileGL/MG_Backend/Init.cpp @@ -95,7 +95,7 @@ namespace MobileGL::MG_Backend { // server into a run-ahead one, because the failure mode is not a slow frame - it is // the apply thread reading client memory that has already moved, which renders wrong // rather than aborting. - constexpr Bool kMGPipeP5eRunAheadReady = false; + constexpr Bool kMGPipeP5eRunAheadReady = true; // P5e (sb, ID-106). DirectGLES GAINS BIT 13 - the indexed buffer binding points - and // Magma deliberately does not. c0e landed the phase constant and the dirty-bit map with diff --git a/MobileGL/MG_Impl/Pipe/PipeFill.cpp b/MobileGL/MG_Impl/Pipe/PipeFill.cpp index 8e05a588..72ff9c24 100644 --- a/MobileGL/MG_Impl/Pipe/PipeFill.cpp +++ b/MobileGL/MG_Impl/Pipe/PipeFill.cpp @@ -394,7 +394,48 @@ 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"); + // P5e (ra2): THE CLIENT'S HALF OF ESCALATION (iii) WAS HERE AND WENT WITH IT + // (ID-133, withdrawn by ID-136). It named MultiDrawArrays / MultiDrawElements / + // MultiDrawElementsBaseVertex so the client would FILL for the records the server + // was escalating - a fill the server then read as a barriered pull. Retiring that + // pull outright (MultiDraw.cpp's BoundDrawIndirectBufferId takes a handle arm) left + // nothing for the fill to serve, so a plain multi-draw is an ordinary kDraw verb + // again and takes the draw_vbo answer below. The pair is the phase's own lesson + // written twice: a wait added to make a lane green is paid by an arm, and here the + // arm was the default tier the phone ships. + 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 +445,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 +712,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 +3398,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 +3724,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_IntegrationTest/CMakeLists.txt b/MobileGL/MG_IntegrationTest/CMakeLists.txt index 4d57f2de..87e49e2e 100644 --- a/MobileGL/MG_IntegrationTest/CMakeLists.txt +++ b/MobileGL/MG_IntegrationTest/CMakeLists.txt @@ -2192,13 +2192,34 @@ if (MOBILEGL_BUILD_DISAGGREGATED) # them by dereferencing a client pointer per draw - and it is therefore both the positive # control for vi's decision to KEEP that arm alive under lockstep and the only entry that # can reach the run-ahead refusal when ra publishes the caps bit. + # + # ---- P5e (ra2), ID-134: AND WITH THE CAPS BIT PUBLISHED IT IS A LANE OF ITS OWN ---------- + # + # "When ra publishes the caps bit" is now, and what these two entries reach is + # `Fatal{UnmigratedVerb, "DrawArrays+CLIENT_ARRAYS"}` / `"MultiDrawArrays+CLIENT_ARRAYS"` - + # ID-82's NAMED REFUSAL, working exactly as ruled. They cannot be green under run-ahead and + # they should not be: a lane that demanded them green would be demanding that the refusal + # not fire, which is the opposite of what vi registered them for. Staging the bytes is P8's. + # + # THE LABEL IS `integration-clientarrays-split` AND THE SPELLING IS THE POINT (ID-131, and + # gl's `integration-magma-split` above is the worked example): `ctest -L` takes a REGEX, so a + # label spelled `integration-split-clientarrays` is still MATCHED by `-L integration-split` + # and the entries would never have left the lane they were moved out of - a split that reads + # as done in every listing and is not. Counted rather than assumed: `ctest -N -L + # integration-split` is 179 after this change and was 181 before, and `-L integration-gpu` + # is 1357 against 1359. `integration-gpu` goes with it for the same reason `integration- + # magma-split` dropped it - a lane that must be green cannot contain an entry that must not. + # + # The two entries keep their private, distinct log paths (SplitLogPaths.cmake.in lists + # MGL_SPLIT_VI_CLIENT_ARRAY_TESTS by name), which is what lets the expected-red CI step + # assert the MARKER rather than merely tolerate a failure. gtest_discover_tests(MobileGLIntegrationTest TEST_PREFIX "DirectGLES.Split." TEST_LIST MGL_SPLIT_VI_CLIENT_ARRAY_TESTS TEST_FILTER "ClientVertexArrayScenario.*" DISCOVERY_TIMEOUT 30 PROPERTIES - LABELS "integration-gpu\;integration-split" + LABELS "integration-clientarrays-split" TIMEOUT ${MGL_ITEST_TIMEOUT} ENVIRONMENT "${MGL_ITEST_GLES_SPLIT_ENVIRONMENT}" ) diff --git a/MobileGL/MG_Pipe/PipeApply.cpp b/MobileGL/MG_Pipe/PipeApply.cpp index 2fca9c03..c10c8153 100644 --- a/MobileGL/MG_Pipe/PipeApply.cpp +++ b/MobileGL/MG_Pipe/PipeApply.cpp @@ -3015,6 +3015,27 @@ namespace MobileGL::MG_Pipe { if (op == MGPWireOp::DrawVbo && payload != nullptr) { const auto& draw = *static_cast(payload); if ((draw.Flags & static_cast(kDrawClientArrays)) != 0) return true; + // ---- ESCALATION (iii) WAS HERE AND IS WITHDRAWN (P5e ra2, ID-133 then ID-136) --- + // + // ID-133 escalated a PLAIN multi-draw (`NumDraws > 1 && !kDrawIsIndirect`) so that + // MultiDrawImpl::RunIndirect's read of the client's GL_DRAW_INDIRECT_BUFFER binding + // became a legal barriered pull instead of an unbarriered Fatal. It worked, and it + // cost a rendezvous on EVERY plain glMultiDraw* on the DEFAULT tier: there is one + // draw opcode - all twenty entry points collapse onto draw_vbo - and the tier is + // chosen on the server per batch (MultiDraw.cpp's ResolveTierForBatch), so no + // predicate both roles can compute names the arm that actually needed it. + // + // THE CHECK ON AN ESCALATION IS "WHICH ARM PAYS FOR IT", NOT "WHICH LANE GOES + // GREEN", and that is the rule this pair of rulings exists to record. The read was + // a save/restore of a GL binding NAME around the tier's own scratch buffer, not a + // data dependency, so the answer was to ask the side that did the binding: + // BoundDrawIndirectBufferId now takes the handle arm its neighbour + // ResolveBoundIndexBuffer already had, and the pull is GONE rather than legalised. + // + // Both halves landed in ONE commit on purpose: the pull retired without this clause + // withdrawn is a cost with no reason, and this clause withdrawn without the pull + // retired puts 18 lane entries back on the unbarriered arm. Neither is a state to + // gate or to measure, so neither was ever a head. } return false; } diff --git a/MobileGL/MG_Pipe/PipeApply.h b/MobileGL/MG_Pipe/PipeApply.h index a791c97b..7d41d405 100644 --- a/MobileGL/MG_Pipe/PipeApply.h +++ b/MobileGL/MG_Pipe/PipeApply.h @@ -921,6 +921,14 @@ namespace MobileGL::MG_Pipe { // contract, not a condition somebody adds at a call site - because a fourth clause that only // one side computes is the same silent failure one level down. // + // THERE WAS AN ESCALATION (iii) AND IT WAS WITHDRAWN, which is worth a line here rather than + // only in the history: ID-133 barriered a plain multi-draw to legalise a pull in Espryt's + // indirect multi-draw tier, ID-136 retired the pull instead (MultiDraw.cpp's + // BoundDrawIndirectBufferId) and took the clause back out. The rule it leaves behind is the + // one to apply to the next candidate: ask WHICH ARM PAYS for an escalation, not which lane + // it turns green - (iii) was keyed on the only wire fact available and therefore charged the + // default tier for a read only two opt-in tiers made. + // // Escalation (ii) is a REFUSAL under run-ahead (§5.1), so on a run-ahead server it never // reaches the sink at all; it is here so the predicate is TOTAL and so the lockstep arm, // where such a draw is legal, gets the barrier it needs. @@ -992,7 +1000,7 @@ namespace MobileGL::MG_Pipe { // Computing the predicate anyway (PipeApplier::ApplyOne) is deliberate: it keeps the // function exercised on every record for the whole phase rather than first run on the day // it starts deciding. - inline constexpr Bool kMGPipeP5eClientWaitRuleLanded = false; + inline constexpr Bool kMGPipeP5eClientWaitRuleLanded = true; // P5c (rv): the two serials the PipeInputs texture-shutter accessors answer with under a // server-stamped verb (FieldOwnership.def, APPLIER_DERIVED). Free functions rather than diff --git a/MobileGL/MG_Remote/CONTRACT-P5E.md b/MobileGL/MG_Remote/CONTRACT-P5E.md index 36a3dac0..d87f3814 100644 --- a/MobileGL/MG_Remote/CONTRACT-P5E.md +++ b/MobileGL/MG_Remote/CONTRACT-P5E.md @@ -103,6 +103,32 @@ must not be told a draw is unbarriered on that account. Escalation (ii) is a ref (§5.1), so on a run-ahead server it never reaches the sink; it is listed so the predicate is total. **No other runtime escalation exists**; adding one is an integrator ruling and a row here. +**There was an escalation (iii) and it was WITHDRAWN — ID-133 added it, ID-136 took it back out, +and the reason is the most reusable thing P5e learned about escalations.** ID-133 barriered a +plain multi-draw (`NumDraws > 1 && !kDrawIsIndirect`) so that `MultiDrawImpl::RunIndirect`'s read +of the client's `GL_DRAW_INDIRECT_BUFFER` binding became a legal barriered pull instead of +`Fatal{UnmigratedPipeInput, "GetBufferBindingSlot@DrawArrays"}` on 18 lane entries. It worked, and +it was the wrong instrument: + +- There is exactly **one draw opcode** — all twenty draw entry points collapse onto `draw_vbo` — + and the multi-draw tier is resolved on the SERVER, per batch (`MultiDraw.cpp`'s + `ResolveTierForBatch`), from driver caps the client does not hold. So "escalate the indirect + multi-draw op" has no op to name and no predicate both roles can compute, and the narrowest + available key charged **every plain `glMultiDraw*` on every tier**, including the `auto` → `ext` + default the phone ships — a per-batch rendezvous on the shipping arm, added to satisfy a lane. +- And the read was never a data dependency: `BoundDrawIndirectBufferId` SAVES AND RESTORES a GL + binding name around the tier's own scratch command buffer. Giving it the handle arm its + neighbour `ResolveBoundIndexBuffer` already had retires the pull outright. + +**The rule this leaves for the next candidate escalation: ask WHICH ARM PAYS FOR IT, not which +lane it turns green.** A wait added to make a lane green is a real cost on a real path; if the +predicate cannot name the arm that needs it, the escalation is charging arms that do not. + +The two halves — retiring the pull and withdrawing the clause — landed in one commit (ID-136 +overrode ID-113's package boundary for it): the pull retired without the clause withdrawn is a +cost with no reason, and the clause withdrawn without the pull retired puts the 18 entries back +on the unbarriered arm. + ### 2.2 The static column, row by row | class | rows | why | @@ -212,6 +238,39 @@ UNBARRIERED apply is a finding: `MGLOG_E_ONCE` with the kind, Fatal under strict outside a barriered fill is `Fatal{RoleViolation, "gPipeInputs"}` by name. `Fatal{BarrierViolation}` (`:840`) stays for `BatchWaits==0 && !RunAhead`. + **AMENDED by ra2 (ID-132, and it is the sentence this section got wrong).** "Outside a + barriered fill" was the whole test, and a barriered fill was exempted on the strength of what + the caller SAID about itself: *"this touch is the residual fill of a record this thread is + about to park behind"*. 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, so at the instant + of the write the apply thread is still draining the unbarriered records the client ran ahead + of. **A guard whose exemption cannot be false on the class it exists for is not a guard.** It + never fired on any of the 70 red lane entries; what fired instead was the apply thread, on + `Fatal{UnmigratedPipeInput, "@"}`, after the GL thread had bumped + `CurrentVerbSerial`, withdrawn `m_serverStampedVerb` and renamed `m_currentVerb` underneath a + record it was inside. + + **The rule as landed:** under `RunAheadArmed` a GL-thread write to the block is legal only + while `ApplyThreadIsInsideApplier()` is false, barriered fill or not; and the three fill sites + (`MGPipeValidateForVerb` phase 1, its step-4 residual walk, `MGPipeNoteFrontendMutation`) + establish that by taking §2.5's forced wait first (`QuiesceApplierBeforeFill`, + `PipeFill.cpp`). **Two waits per filling verb, not one**, because the block is written in two + phases that straddle publication — the serial bump / stamp withdrawal / verb rename before the + emitters, the 63-field walk after them — and the records those emitters published are records + whose apply reads the block. The red-once is deleting a wait; the guard then aborts by name on + the first filling verb behind a run-ahead backlog. Pinned by + `RemoteGuards.ClientBarrieredFillWhileTheApplierIsInsideUnderRunAheadIsFatalByName` with its + green control beside it; both run in the `unit` lane, because the strict lane runs under + lockstep and cannot see this class by construction. + + The same correction applies to **§3.2 above, which is UNLANDED and was never marked as such**: + "the server's stamp is server-private … never into the shared block" describes storage the + applier owns, and `MGPipeServerStampVerbBoundary` still writes `m_filled` / `m_currentVerb` / + `m_serverStampedVerb` into `gPipeInputs` itself. That gap is exactly what made the race + possible, and splitting the stamp is the P11 item — but note that per-role stamps **without** + versioning the BARRIER_PULLED values would be worse than today, because it turns a loud + `Fatal{UnmigratedPipeInput}` into a silent stale read. Both, or the values retired first. + --- ## §4 id — handle-keyed twins 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