diff --git a/MobileGL/MG_Remote/Client/ClientSession.cpp b/MobileGL/MG_Remote/Client/ClientSession.cpp index 06d9601e..4fb94dd1 100644 --- a/MobileGL/MG_Remote/Client/ClientSession.cpp +++ b/MobileGL/MG_Remote/Client/ClientSession.cpp @@ -249,15 +249,20 @@ namespace MobileGL::MG_Remote::Client { Transport::RingControl* control = m_shm.CmdControl(); m_cmd = Transport::RingProducer(control, m_shm.CmdRingBase(), m_shm.CmdRingCapacity(), Transport::RingCursorSet::Cmd); - m_stage = Transport::RingProducer(control, m_shm.StageBase(), m_shm.StageCapacity(), - Transport::RingCursorSet::Stage); - if (!m_cmd.Valid() || !m_stage.Valid()) { + // NO STAGE RING. SEG_STAGE is package w1's encoder-local LINEAR ALLOCATOR: + // a staged byte run carries no RingRecordHeader, nothing consumes SEG_STAGE, + // and the allocator reclaims on retiredSeq. A RingProducer over + // RingCursorSet::Stage would publish stageHead with nothing advancing the + // two tails, so FreeBytes() would fall to zero the first time the head + // lapped the capacity and never recover - a guaranteed hang. See + // RingControl's stage triple in Ring.h. + if (!m_cmd.Valid()) { Stop(); return MOBILEGL_ERR_INVALID_ARGUMENT; } // PeerDoorbell() is the bell the SERVER parks on and this side rings; SelfDoorbell() is // this side's own. Which is which is the session's knowledge, not the transport's. - m_producer.Attach(control, &m_cmd, &m_stage, &m_clientTransport->PeerDoorbell(), + m_producer.Attach(control, &m_cmd, &m_clientTransport->PeerDoorbell(), &m_clientTransport->SelfDoorbell(), SpinUsFromConfig()); m_replies = Transport::ReplySlotPool(m_shm.ReplyBase(), m_shm.ReplyBytes(), @@ -289,13 +294,18 @@ namespace MobileGL::MG_Remote::Client { m_segments.Install(Wire::kSegCmd, Wire::SegmentView{m_shm.CmdRingBase(), m_shm.CmdRingCapacity()}); m_segments.Install(Wire::kSegStage, - Wire::SegmentView{m_shm.StageBase(), m_shm.StageCapacity()}); + Wire::SegmentView{m_shm.StageBase(), m_shm.StageBytes()}); m_segments.Install(Wire::kSegReply, Wire::SegmentView{m_shm.ReplyBase(), m_shm.ReplyBytes()}); m_segments.Install(Wire::kSegEvent, Wire::SegmentView{m_shm.EventSegmentBase(), m_shm.AnnouncedSize(Transport::SessionSegmentSlot::Event)}); - m_encoder = Wire::PipeWireEncoder(control, &m_cmd, &m_stage, &m_segments); + // nullptr for the stage producer, and that is the honest value: c0's + // signature predates w1's ruling that SEG_STAGE is a linear allocator, and + // the encoder reaches its bytes through the SegmentTable above. Handing it + // a live RingProducer over a cursor triple nobody consumes would be the + // half-wired shape this session exists not to have. + m_encoder = Wire::PipeWireEncoder(control, &m_cmd, nullptr, &m_segments); // ---- 7. the first CapsSnapshot, if the server had a backend to publish one from. if (m_transport->PeekFrameSize() != 0) { @@ -395,7 +405,6 @@ namespace MobileGL::MG_Remote::Client { m_events = Transport::EventRingConsumer(); m_replies = Transport::ReplySlotPool(); m_cmd = Transport::RingProducer(); - m_stage = Transport::RingProducer(); m_shm.Close(); Server::ServerSessionInstance().Close(); m_clientTransport.reset(); diff --git a/MobileGL/MG_Remote/Client/ClientSession.h b/MobileGL/MG_Remote/Client/ClientSession.h index dd818421..ad1cdae6 100644 --- a/MobileGL/MG_Remote/Client/ClientSession.h +++ b/MobileGL/MG_Remote/Client/ClientSession.h @@ -149,7 +149,6 @@ namespace MobileGL::MG_Remote::Client { std::unique_ptr m_serverTransport; Transport::SessionSegments m_shm; Transport::RingProducer m_cmd; - Transport::RingProducer m_stage; Transport::SessionProducer m_producer; Transport::EventRingConsumer m_events; Transport::ReplySlotPool m_replies; diff --git a/MobileGL/MG_Remote/Server/ServerSession.cpp b/MobileGL/MG_Remote/Server/ServerSession.cpp index cfbe95aa..6fa3a55b 100644 --- a/MobileGL/MG_Remote/Server/ServerSession.cpp +++ b/MobileGL/MG_Remote/Server/ServerSession.cpp @@ -23,6 +23,29 @@ namespace MobileGL::MG_Remote::Server { + // THE THREE FLAG-SPACE COLLISIONS, AS TRIPWIRES RATHER THAN AS A COMMENT. + // + // MGPipeCallFlags (MG_Pipe/MGPipe.h:42-54) and RingRecordFlags (Ring.h) are separate spaces + // that overlap, and three bits mean DIFFERENT things in each. Ring.h's enum carries the + // table; these are the assertions that break the build if either enum is renumbered, so the + // collision can never become news again. They live here because this is the nearest .cpp + // that legally sees both headers - nothing under Transport/ may reach MobileGL/Includes.h. + static_assert(static_cast(MG_Pipe::kVarTail) == Transport::kRecPad, + "MGPipeCallFlags::kVarTail and kRecPad share bit 2: an encoder that copies call " + "flags into RingRecordHeader::flags makes every var-tail record read as a wrap " + "filler. RingConsumer::Pop requires kind == kRingPadRecordKind as well, which is " + "what keeps that from eating the record - do not relax it"); + static_assert(static_cast(MG_Pipe::kHostSpan) == Transport::kRecBorrowSlot, + "MGPipeCallFlags::kHostSpan and kRecBorrowSlot share bit 3: a host-span record " + "would read as borrowed into the GPU timeline and stop the consumer reclaiming " + "ring bytes behind it. SessionConsumer counts and names every sighting"); + static_assert(static_cast(MG_Pipe::kReplySlot) == Transport::kRecVarTail, + "MGPipeCallFlags::kReplySlot and kRecVarTail share bit 4"); + static_assert(static_cast(MG_Pipe::kNeedsAck) == Transport::kRecNeedsAck && + static_cast(MG_Pipe::kHasBlob) == Transport::kRecHasBlob, + "the two bits that DO mean the same thing in both spaces have drifted apart, " + "which is a different and worse problem than the three that collide"); + namespace { // A control-plane frame is small by construction (ITransport.h:56-58: bulk bytes @@ -97,7 +120,7 @@ namespace MobileGL::MG_Remote::Server { const Uint64 ringMb = MG_Config::Ipc.RingMb == 0 ? 8u : MG_Config::Ipc.RingMb; const Uint64 stageMb = MG_Config::Ipc.StageMb == 0 ? 32u : MG_Config::Ipc.StageMb; sizes.CmdRingBytes = ringMb * 1024ull * 1024ull; - sizes.StageRingBytes = stageMb * 1024ull * 1024ull; + sizes.StageBytes = stageMb * 1024ull * 1024ull; #endif return sizes; } @@ -304,7 +327,7 @@ namespace MobileGL::MG_Remote::Server { m_segments.Install(Wire::kSegCmd, Wire::SegmentView{m_shm.CmdRingBase(), m_shm.CmdRingCapacity()}); m_segments.Install(Wire::kSegStage, - Wire::SegmentView{m_shm.StageBase(), m_shm.StageCapacity()}); + Wire::SegmentView{m_shm.StageBase(), m_shm.StageBytes()}); m_segments.Install(Wire::kSegReply, Wire::SegmentView{m_shm.ReplyBase(), m_shm.ReplyBytes()}); m_segments.Install(Wire::kSegEvent, Wire::SegmentView{m_shm.EventSegmentBase(), diff --git a/MobileGL/MG_Remote/Transport/Ring.cpp b/MobileGL/MG_Remote/Transport/Ring.cpp index b5ff5d6f..60188ae8 100644 --- a/MobileGL/MG_Remote/Transport/Ring.cpp +++ b/MobileGL/MG_Remote/Transport/Ring.cpp @@ -260,7 +260,16 @@ namespace MobileGL::MG_Remote::Transport { return false; } - if ((header.flags & kRecPad) != 0) { + // BOTH, not just the flag. kRecPad (1<<2) is the same bit as + // MGPipeCallFlags::kVarTail, so an encoder that copied a call's flags + // into this framing field verbatim would have every var-tail record + // skipped HERE, silently, with the record lost and nothing logged on + // either side. A genuine filler is always kind kRingPadRecordKind - + // Reserve writes it two dozen lines above - and a call record always + // carries a real opcode, because the catalogue starts at 1. Requiring + // the pair costs one comparison and turns that collision from a lost + // record into a record the decoder gets and can reject by name. + if ((header.flags & kRecPad) != 0 && header.kind == kRingPadRecordKind) { m_localTail += size; continue; } @@ -405,11 +414,10 @@ namespace MobileGL::MG_Remote::Transport { // SessionProducer // ----------------------------------------------------------------------- - void SessionProducer::Attach(RingControl* control, RingProducer* cmd, RingProducer* stage, - Doorbell* peerBell, Doorbell* selfBell, std::uint32_t spinUs) { + void SessionProducer::Attach(RingControl* control, RingProducer* cmd, Doorbell* peerBell, + Doorbell* selfBell, std::uint32_t spinUs) { m_control = control; m_cmd = cmd; - m_stage = stage; m_peerBell = peerBell; m_selfBell = selfBell; m_spinUs = spinUs; @@ -418,7 +426,6 @@ namespace MobileGL::MG_Remote::Transport { void SessionProducer::Detach() { m_control = nullptr; m_cmd = nullptr; - m_stage = nullptr; m_peerBell = nullptr; m_selfBell = nullptr; m_lastPublishedSeq = 0; @@ -428,11 +435,9 @@ namespace MobileGL::MG_Remote::Transport { if (!Valid()) { return; } - // 1. the records themselves. + // 1. the records themselves. ONLY SEG_CMD: SEG_STAGE is not a ring and + // RingCursorSet::Stage is driven by nobody (Ring.h's stage triple). m_cmd->Publish(); - if (m_stage != nullptr) { - m_stage->Publish(); - } // Kept locally as well as on the shared page: the shared watermark is // allowed to lag (Ring.h:72-77), and teardown's drain must not. // @@ -493,14 +498,6 @@ namespace MobileGL::MG_Remote::Transport { return Park([cmd, bytes] { return cmd->FreeBytes() >= bytes; }, timeoutMs); } - SessionWait SessionProducer::WaitForStageSpace(std::uint64_t bytes, std::uint32_t timeoutMs) { - if (!Valid() || m_stage == nullptr) { - return SessionWait::TimedOut; - } - RingProducer* stage = m_stage; - return Park([stage, bytes] { return stage->FreeBytes() >= bytes; }, timeoutMs); - } - // ----------------------------------------------------------------------- // SessionConsumer // ----------------------------------------------------------------------- @@ -560,6 +557,19 @@ namespace MobileGL::MG_Remote::Transport { NotifyClient(); } + void SessionConsumer::NoteBorrowedRecord(std::uint16_t kind, std::uint16_t flags) { + ++m_borrowedSeen; + if (m_borrowedSeen == 1) { + MGLOG_E("MG_Remote ring: record kind %u carries kRecBorrowSlot (flags=0x%04X). P5 " + "implements NO borrowed slots, and that bit is also MGPipeCallFlags::" + "kHostSpan, which P5's reduced path is ruled to produce none of either. " + "Nothing past this record will be reclaimed until RetireBorrowedUpTo " + "releases it, so a producer that then wedges on a full ring is THIS line's " + "fault and not the ring's", + static_cast(kind), static_cast(flags)); + } + } + void SessionConsumer::RetireBorrowedUpTo(std::uint64_t cursor) { if (!Valid()) { return; diff --git a/MobileGL/MG_Remote/Transport/Ring.h b/MobileGL/MG_Remote/Transport/Ring.h index d6be4af0..967cbbf0 100644 --- a/MobileGL/MG_Remote/Transport/Ring.h +++ b/MobileGL/MG_Remote/Transport/Ring.h @@ -103,7 +103,30 @@ namespace MobileGL::MG_Remote::Transport { alignas(64) std::atomic cmdAppliedTail; // consumer: bytes decoded/copied out std::atomic cmdRetiredTail; // consumer: borrowed slots released - // ---- SEG_STAGE cursors ---------------------------------------------- + // ---- SEG_STAGE cursors ------------------------------------------------ + // + // DEAD IN P5, DELIBERATELY, AND NOBODY MAY WIRE THEM UP HALFWAY. + // + // SEG_STAGE is NOT a ring any more. Package w1's encoder owns staging as + // an ENCODER-LOCAL LINEAR ALLOCATOR: a staged byte run carries no + // RingRecordHeader, there is no consumer walking SEG_STAGE, and the + // allocator reclaims on `retiredSeq` - the sequence watermark below - + // rather than on these three cursors. So all three stay ZERO for the + // whole of P5, `RingCursorSet::Stage` has no producer and no consumer, + // and `SessionTest.TheStageCursorTripleStaysDeadAcrossAWholeSession` + // pins that rather than leaving it to be noticed. + // + // They are kept rather than deleted because RingCursorSet, the three + // cursor accessors in Ring.cpp and RingTest's fixture are all written + // against a two-triple page, and P8/P11's shadow and adopt segments are + // the ring-shaped users this triple was reserved for. What is NOT + // acceptable is the middle state: a producer publishing `stageHead` with + // nothing advancing the two tails makes FreeBytes() fall to zero the + // first time the head laps the capacity and never recover, which is a + // guaranteed hang rather than a slow path. Five watermarks already spent + // a whole phase declared-and-written-by-nobody; this is the sixth, and + // it is declared-and-written-by-nobody ON PURPOSE, which is only + // different if it is written down. alignas(64) std::atomic stageHead; alignas(64) std::atomic stageAppliedTail; std::atomic stageRetiredTail; @@ -140,6 +163,33 @@ namespace MobileGL::MG_Remote::Transport { }; static_assert(sizeof(RingRecordHeader) == 8, "RecHeader is 8 bytes on the wire"); + // THESE ARE RING FLAGS AND THEY ARE NOT MGPipeCallFlags, AND THREE OF THE + // BITS COLLIDE WITH A DIFFERENT MEANING. `MGPipeCallFlags` (MG_Pipe/MGPipe.h: + // 42-54) is a SEPARATE SPACE that happens to overlap this one, and an encoder + // that copies `MGPipeCallFlagsFor(op)` into RingRecordHeader::flags without + // translating puts a call's bits into a framing field: + // + // bit 0 kNeedsAck == kRecNeedsAck same meaning, harmless + // bit 1 kHasBlob == kRecHasBlob same meaning, harmless + // bit 2 kVarTail == kRecPad WORST: a var-tail record would read + // as a WRAP FILLER and be skipped + // silently by Pop, losing the record + // with nothing logged anywhere + // bit 3 kHostSpan == kRecBorrowSlot a host-span record would read as + // borrowed into the GPU timeline, and + // the consumer would stop reclaiming + // ring bytes behind it for ever + // bit 4 kReplySlot == kRecVarTail a blocking call would read as having + // a tail it does not have + // bit 5 kOptional == (unused here) + // + // Translating is the ENCODER's job. Two things on this side make the first + // two of those survivable anyway rather than trusting it: `Pop` requires a + // filler to carry BOTH kRecPad AND kind == kRingPadRecordKind, so a real + // record with bit 2 set is delivered rather than eaten (a call record always + // has a real opcode kind, the catalogue starts at 1); and SessionConsumer + // counts and NAMES every kRecBorrowSlot it sees, because P5 produces no + // borrowed slots at all and the bit arriving means the collision did. enum RingRecordFlags : std::uint16_t { kRecNone = 0, kRecNeedsAck = 1u << 0, diff --git a/MobileGL/MG_Remote/Transport/SessionRings.h b/MobileGL/MG_Remote/Transport/SessionRings.h index 381bdc7b..4a78f2ef 100644 --- a/MobileGL/MG_Remote/Transport/SessionRings.h +++ b/MobileGL/MG_Remote/Transport/SessionRings.h @@ -79,10 +79,15 @@ namespace MobileGL::MG_Remote::Transport { // CONTRACT-P5's; MOBILEGL_IPC_RING_MB / MOBILEGL_IPC_STAGE_MB move the first // two, and ServerSession applies them unless SetSegmentSizes overrode them. struct SessionSegmentSizes { - std::uint64_t CmdRingBytes = 8ull * 1024 * 1024; // + one control page - std::uint64_t StageRingBytes = 32ull * 1024 * 1024; // no control page - std::uint64_t ReplyBytes = 8ull * 1024 * 1024; // not a ring - std::uint64_t EventRingBytes = 256ull * 1024; // + one control page + std::uint64_t CmdRingBytes = 8ull * 1024 * 1024; // + one control page + // SEG_STAGE IS NOT A RING. Package w1's encoder owns it as an + // encoder-local LINEAR ALLOCATOR that reclaims on retiredSeq, so this is + // a plain byte count: no control page, and no rounding down to a power of + // two either. Rounding was a ring requirement and keeping it would have + // silently turned an operator's MOBILEGL_IPC_STAGE_MB=24 into 16. + std::uint64_t StageBytes = 32ull * 1024 * 1024; + std::uint64_t ReplyBytes = 8ull * 1024 * 1024; // slot pool, not a ring + std::uint64_t EventRingBytes = 256ull * 1024; // + one control page std::uint32_t ReplySlotCount = kDefaultReplySlotCount; }; @@ -155,8 +160,11 @@ namespace MobileGL::MG_Remote::Transport { void* CmdRingBase() const { return m_cmdRingBase; } std::uint64_t CmdRingCapacity() const { return m_cmdRingCapacity; } + // The whole SEG_STAGE mapping, for w1's linear allocator and for the + // SegmentTable view the decoder resolves blobrefs against. There is no + // stage RING and no RingProducer/RingConsumer over RingCursorSet::Stage. void* StageBase() const { return m_stageBase; } - std::uint64_t StageCapacity() const { return m_stageCapacity; } + std::uint64_t StageBytes() const { return m_stageBytes; } void* ReplyBase() const { return m_replyBase; } std::uint64_t ReplyBytes() const { return m_replyBytes; } @@ -185,7 +193,7 @@ namespace MobileGL::MG_Remote::Transport { void* m_cmdRingBase = nullptr; std::uint64_t m_cmdRingCapacity = 0; void* m_stageBase = nullptr; - std::uint64_t m_stageCapacity = 0; + std::uint64_t m_stageBytes = 0; void* m_replyBase = nullptr; std::uint64_t m_replyBytes = 0; std::uint32_t m_replySlotCount = kDefaultReplySlotCount; @@ -276,8 +284,14 @@ namespace MobileGL::MG_Remote::Transport { // `peerBell` is the bell the SERVER parks on and this side rings; // `selfBell` is this side's own. InProcessTransport::PeerDoorbell() and // SelfDoorbell() are exactly that pair, from the client endpoint. - void Attach(RingControl* control, RingProducer* cmd, RingProducer* stage, Doorbell* peerBell, - Doorbell* selfBell, std::uint32_t spinUs); + // NO STAGE RING. SEG_STAGE is w1's encoder-local linear allocator and + // RingCursorSet::Stage has no producer and no consumer in P5 - see + // RingControl's stage triple in Ring.h. A producer here would publish + // stageHead with nothing advancing the two tails, so FreeBytes() would + // fall to zero the first time the head lapped the capacity and never + // recover: a guaranteed hang, not a slow path. + void Attach(RingControl* control, RingProducer* cmd, Doorbell* peerBell, Doorbell* selfBell, + std::uint32_t spinUs); void Detach(); bool Valid() const { return m_control != nullptr && m_cmd != nullptr; } @@ -310,11 +324,9 @@ namespace MobileGL::MG_Remote::Transport { // enough free bytes can only mean "too big, chunk", and waiting on it // stalls forever. SessionWait WaitForCmdSpace(std::uint64_t bytes, std::uint32_t timeoutMs); - SessionWait WaitForStageSpace(std::uint64_t bytes, std::uint32_t timeoutMs); RingControl* Control() const { return m_control; } RingProducer* Cmd() const { return m_cmd; } - RingProducer* Stage() const { return m_stage; } Doorbell* PeerDoorbell() const { return m_peerBell; } Doorbell* SelfDoorbell() const { return m_selfBell; } std::uint32_t SpinUs() const { return m_spinUs; } @@ -325,7 +337,6 @@ namespace MobileGL::MG_Remote::Transport { RingControl* m_control = nullptr; RingProducer* m_cmd = nullptr; - RingProducer* m_stage = nullptr; Doorbell* m_peerBell = nullptr; Doorbell* m_selfBell = nullptr; std::uint32_t m_spinUs = kDefaultSpinUs; @@ -386,6 +397,17 @@ namespace MobileGL::MG_Remote::Transport { if ((view.flags & kRecBorrowSlot) == 0 && !m_borrowHeld) { m_retirableCursor = view.cursor + sizeof(RingRecordHeader) + view.payloadSize; } else { + if ((view.flags & kRecBorrowSlot) != 0) { + // P5 PRODUCES NO BORROWED SLOTS AT ALL, so this bit arriving + // is either a borrow nobody implemented or MGPipeCallFlags:: + // kHostSpan wearing kRecBorrowSlot's bit (Ring.h's collision + // table) - and P5's reduced path is ruled to produce zero + // host spans too. Either way the conservative arm is taken + // (nothing past it is reclaimed) and the sighting is NAMED, + // because the alternative is a producer that wedges on the + // first full ring with no line anywhere saying why. + NoteBorrowedRecord(view.kind, view.flags); + } m_borrowHeld = true; } NotifyClient(); @@ -425,6 +447,9 @@ namespace MobileGL::MG_Remote::Transport { void NotifyClient(); std::uint64_t AppliedSeq() const { return m_appliedSeq; } + // How many kRecBorrowSlot records this consumer has seen. Non-zero in P5 + // is a finding, not a statistic. + std::uint64_t BorrowedRecordsSeen() const { return m_borrowedSeen; } RingControl* Control() const { return m_control; } RingConsumer* Cmd() const { return m_cmd; } Doorbell* PeerDoorbell() const { return m_peerBell; } @@ -439,7 +464,12 @@ namespace MobileGL::MG_Remote::Transport { std::uint32_t m_spinUs = kDefaultSpinUs; std::uint64_t m_appliedSeq = 0; std::uint64_t m_retirableCursor = 0; + std::uint64_t m_borrowedSeen = 0; bool m_borrowHeld = false; + + // Out of line so ApplyOne, which is a template in a header this layer + // keeps free of MobileGL/Includes.h, can still log. + void NoteBorrowedRecord(std::uint16_t kind, std::uint16_t flags); }; // ----------------------------------------------------------------------- diff --git a/MobileGL/MG_Remote/Transport/ShmSegment.cpp b/MobileGL/MG_Remote/Transport/ShmSegment.cpp index e07be3ea..3ec969ae 100644 --- a/MobileGL/MG_Remote/Transport/ShmSegment.cpp +++ b/MobileGL/MG_Remote/Transport/ShmSegment.cpp @@ -201,7 +201,7 @@ namespace MobileGL::MG_Remote::Transport { // two grows. const Spec specs[kSlotCount] = { {"mgl-cmd", SegmentBytesForRing(sizes.CmdRingBytes)}, - {"mgl-stage", LargestPowerOfTwoAtMost(sizes.StageRingBytes)}, + {"mgl-stage", sizes.StageBytes}, {"mgl-reply", sizes.ReplyBytes}, {"mgl-event", SegmentBytesForRing(sizes.EventRingBytes)}, }; @@ -341,10 +341,11 @@ namespace MobileGL::MG_Remote::Transport { m_cmdRingBase = cmdBase + sizeof(RingControl); m_cmdRingCapacity = RingCapacityForSegment(m_segments[0]->Size()); - // SEG_STAGE carries no control page of its own: RingControl holds TWO - // cursor triples and the stage triple is the second (Ring.h:106-109). + // SEG_STAGE IS NOT A RING: no control page, no cursor triple, no power-of- + // two rounding. Package w1's encoder owns it as a linear allocator that + // reclaims on retiredSeq, so the whole mapping is usable bytes. m_stageBase = m_segments[1]->Data(); - m_stageCapacity = LargestPowerOfTwoAtMost(m_segments[1]->Size()); + m_stageBytes = m_segments[1]->Size(); m_replyBase = m_segments[2]->Data(); m_replyBytes = m_segments[2]->Size(); @@ -360,14 +361,14 @@ namespace MobileGL::MG_Remote::Transport { m_mappedBytes += m_segments[index]->Size(); } - if (m_cmdRingCapacity == 0 || m_stageCapacity == 0 || m_eventRingCapacity == 0 || + if (m_cmdRingCapacity == 0 || m_stageBytes == 0 || m_eventRingCapacity == 0 || m_replyBytes == 0) { MGLOG_E("MG_Remote session: segment sizes leave no usable ring (cmd cap=%llu stage " "cap=%llu event cap=%llu reply=%llu). A ring is the largest POWER OF TWO that " "fits after the 4096 byte control page, so a segment must be strictly larger " "than one page plus the smallest ring", static_cast(m_cmdRingCapacity), - static_cast(m_stageCapacity), + static_cast(m_stageBytes), static_cast(m_eventRingCapacity), static_cast(m_replyBytes)); return; @@ -392,7 +393,7 @@ namespace MobileGL::MG_Remote::Transport { m_cmdRingBase = nullptr; m_cmdRingCapacity = 0; m_stageBase = nullptr; - m_stageCapacity = 0; + m_stageBytes = 0; m_replyBase = nullptr; m_replyBytes = 0; m_eventControl = nullptr; diff --git a/MobileGL/MG_Test/Wire/SessionTest.cpp b/MobileGL/MG_Test/Wire/SessionTest.cpp index c0de8e7c..e294d892 100644 --- a/MobileGL/MG_Test/Wire/SessionTest.cpp +++ b/MobileGL/MG_Test/Wire/SessionTest.cpp @@ -53,7 +53,7 @@ namespace { // RING sizes. The SEG_CMD and SEG_EVENT segments are each one control page bigger. SessionSegmentSizes sizes; sizes.CmdRingBytes = 32ull * 1024; - sizes.StageRingBytes = 64ull * 1024; + sizes.StageBytes = 48ull * 1024; sizes.ReplyBytes = 64ull * 1024; // -> 8 slots of 8 KiB sizes.EventRingBytes = 16ull * 1024; sizes.ReplySlotCount = 8; @@ -69,7 +69,8 @@ namespace { SessionSegments serverSegments; SessionSegments clientSegments; RingProducer cmdProducer; - RingProducer stageProducer; + // NO stageProducer and NO stage consumer: SEG_STAGE is w1's encoder-local linear + // allocator and RingCursorSet::Stage is driven by nobody (Ring.h's stage triple). RingConsumer cmdConsumer; SessionProducer producer; SessionConsumer consumer; @@ -93,18 +94,15 @@ namespace { RingControl* serverControl = serverSegments.CmdControl(); cmdProducer = RingProducer(clientControl, clientSegments.CmdRingBase(), clientSegments.CmdRingCapacity(), RingCursorSet::Cmd); - stageProducer = RingProducer(clientControl, clientSegments.StageBase(), - clientSegments.StageCapacity(), RingCursorSet::Stage); cmdConsumer = RingConsumer(serverControl, serverSegments.CmdRingBase(), serverSegments.CmdRingCapacity(), RingCursorSet::Cmd); - if (!cmdProducer.Valid() || !stageProducer.Valid() || !cmdConsumer.Valid()) { + if (!cmdProducer.Valid() || !cmdConsumer.Valid()) { return false; } // PeerDoorbell is the bell the OTHER end parks on; SelfDoorbell is this end's own. // Which is which is the session's knowledge, never ITransport's (contract ยง3.9). - producer.Attach(clientControl, &cmdProducer, &stageProducer, - &clientTransport->PeerDoorbell(), &clientTransport->SelfDoorbell(), - kDefaultSpinUs); + producer.Attach(clientControl, &cmdProducer, &clientTransport->PeerDoorbell(), + &clientTransport->SelfDoorbell(), kDefaultSpinUs); consumer.Attach(serverControl, &cmdConsumer, &serverTransport->PeerDoorbell(), &serverTransport->SelfDoorbell(), kDefaultSpinUs); replies = ReplySlotPool(serverSegments.ReplyBase(), serverSegments.ReplyBytes(), @@ -199,14 +197,14 @@ TEST(SessionTest, TheDefaultGeometryIsTheFourContractRingSizes) { SessionSegments segments; ASSERT_EQ(segments.Create(SessionSegmentSizes{}, MemoryRole::Server), MOBILEGL_OK); EXPECT_EQ(segments.CmdRingCapacity(), 8ull * 1024 * 1024); - EXPECT_EQ(segments.StageCapacity(), 32ull * 1024 * 1024); + EXPECT_EQ(segments.StageBytes(), 32ull * 1024 * 1024); EXPECT_EQ(segments.ReplyBytes(), 8ull * 1024 * 1024); EXPECT_EQ(segments.EventRingCapacity(), 256ull * 1024); EXPECT_EQ(segments.AnnouncedSize(SessionSegmentSlot::Cmd), 8ull * 1024 * 1024 + sizeof(RingControl)); - // SEG_STAGE carries no control page: RingControl holds both cursor triples, so the whole - // segment is ring and 32 MiB is already a power of two. SEG_REPLY is not a ring at all. + // SEG_STAGE is not a ring at all - no control page, no cursor triple, no power-of-two + // rounding - and neither is SEG_REPLY, so both announce exactly what was asked for. EXPECT_EQ(segments.AnnouncedSize(SessionSegmentSlot::Stage), 32ull * 1024 * 1024); EXPECT_EQ(segments.AnnouncedSize(SessionSegmentSlot::Reply), 8ull * 1024 * 1024); EXPECT_EQ(segments.AnnouncedSize(SessionSegmentSlot::Event), @@ -973,6 +971,102 @@ TEST(SessionTest, CapsSnapshotFieldIdsAreFrozenAndTheRetiredSlotsStayBurned) { EXPECT_EQ(static_cast(::MobileGL::Wire::Welcome::VT_ABIFINGERPRINT), 20); } +// --------------------------------------------------------------------------- +// Fix round 2 - SEG_STAGE stopped being a ring, and the flag-space collisions +// --------------------------------------------------------------------------- + +// Package w1's encoder owns SEG_STAGE as a LINEAR ALLOCATOR that reclaims on retiredSeq: a staged +// byte run carries no RingRecordHeader and nothing walks SEG_STAGE. So RingCursorSet::Stage has no +// producer and no consumer, and all three of its cursors stay ZERO for the whole of a session. +// +// This case exists because "declared and written by nobody" is exactly the state the five sequence +// watermarks were in for a whole phase before this one. The stage triple is in that state ON +// PURPOSE now, and the difference between "on purpose" and "forgotten" is that one of them is +// pinned. A half-wiring - a producer publishing stageHead with nothing advancing the two tails - +// would make FreeBytes() fall to zero on the first lap and never recover: a hang, not a slow path. +TEST(SessionTest, TheStageCursorTripleStaysDeadAcrossAWholeSession) { + SessionFixture session; + ASSERT_TRUE(session.Build(TestSizes())); + + // SEG_STAGE is still mapped and still usable bytes - w1's allocator needs them, and the + // decoder resolves blobrefs against the same view - it is just not a ring. + ASSERT_NE(session.serverSegments.StageBase(), nullptr); + EXPECT_EQ(session.serverSegments.StageBytes(), 48ull * 1024) + << "SEG_STAGE is no longer rounded down to a power of two; that was a ring requirement " + "and keeping it would silently turn MOBILEGL_IPC_STAGE_MB=24 into 16"; + + // Drive a full session's worth of traffic through SEG_CMD. + constexpr int kRecords = 500; + int applied = 0; + for (int index = 0; index < kRecords; ++index) { + void* payload = nullptr; + while ((payload = session.cmdProducer.Reserve(1, kRecNone, 32)) == nullptr) { + while (session.consumer.ApplyOne([&](const RingRecordView&) { ++applied; })) { + } + session.consumer.RetireThrough(session.consumer.AppliedSeq()); + } + std::memset(payload, index & 0xFF, 32); + session.producer.PublishAndNotify(static_cast(index + 1)); + } + while (session.consumer.ApplyOne([&](const RingRecordView&) { ++applied; })) { + } + session.consumer.RetireThrough(session.consumer.AppliedSeq()); + ASSERT_EQ(applied, kRecords); + + // The command triple moved. The stage triple did not, and nothing in the session touches it. + EXPECT_GT(session.Control().cmdHead.load(), 0u); + EXPECT_EQ(session.Control().stageHead.load(), 0u); + EXPECT_EQ(session.Control().stageAppliedTail.load(), 0u); + EXPECT_EQ(session.Control().stageRetiredTail.load(), 0u); + // retiredSeq is the watermark w1's allocator reclaims behind, and it is a SEQUENCE - not one + // of the three byte cursors above. + EXPECT_GT(session.Control().retiredSeq.load(), 0u); +} + +// The kVarTail/kRecPad collision, made harmless. Those two are bit 2 of two different flag spaces, +// so an encoder that copied MGPipeCallFlagsFor(op) into RingRecordHeader::flags verbatim would +// have every var-tail record SKIPPED by Pop, silently, with the record lost and nothing logged on +// either side. Pop now requires a filler to carry both the flag AND kind == kRingPadRecordKind, so +// a real record wearing that bit is delivered instead of eaten. +TEST(SessionTest, ARecordWearingThePadBitIsDeliveredRatherThanEatenAsAFiller) { + SessionFixture session; + ASSERT_TRUE(session.Build(TestSizes())); + + // kRecPad is MGPipeCallFlags::kVarTail's bit. Kind 7 is a real opcode, not a filler. + void* payload = session.cmdProducer.Reserve(7, kRecPad, 16); + ASSERT_NE(payload, nullptr); + std::memset(payload, 0xAB, 16); + session.producer.PublishAndNotify(1); + + int seen = 0; + std::uint16_t seenKind = 0; + while (session.consumer.ApplyOne([&](const RingRecordView& view) { + seenKind = view.kind; + ++seen; + })) { + } + EXPECT_EQ(seen, 1) << "the record was skipped as a wrap filler because it wore bit 2"; + EXPECT_EQ(seenKind, 7); + EXPECT_EQ(session.Control().appliedSeq.load(), 1u); +} + +// The kHostSpan/kRecBorrowSlot collision, made loud. P5 implements no borrowed slots and is ruled +// to produce no host spans either, so the bit arriving means the collision did - and the +// conservative arm it takes (stop reclaiming) would otherwise wedge the producer on the first full +// ring with nothing in any log saying why. +TEST(SessionTest, ABorrowSlotSightingIsCountedRatherThanJustActedOn) { + SessionFixture session; + ASSERT_TRUE(session.Build(TestSizes())); + EXPECT_EQ(session.consumer.BorrowedRecordsSeen(), 0u); + + ASSERT_NE(session.cmdProducer.Reserve(3, kRecBorrowSlot, 16), nullptr); + session.producer.PublishAndNotify(1); + while (session.consumer.ApplyOne([](const RingRecordView&) {})) { + } + EXPECT_EQ(session.consumer.BorrowedRecordsSeen(), 1u) + << "a kRecBorrowSlot record went by unnamed; in P5 that bit can only be kHostSpan"; +} + // m-4. The reply pool refuses a geometry whose slots would not be 8-aligned: the fences order // the payload against the stamp, but the stamp's own 8-byte Seq has to be untorn for the // wrong-slot self-check to mean anything, and that is only true while it is naturally aligned.