From da8f030459c72ae9a59b2323b2b5bafee165f949 Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Fri, 11 Sep 2026 15:04:54 -0400 Subject: [PATCH] [Fix] (MG_Remote, Transport): the knob names the ring and the segment adds a control page, a backwards watermark is Fatal rather than a permanent hang, RetireThrough stops at a borrowed slot, the inproc peer gets a real second mapping through dup and Adopt, and the producer remembers its own published seq --- MobileGL/MG_Remote/Transport/ReplySlot.h | 27 +++- MobileGL/MG_Remote/Transport/Ring.cpp | 88 ++++++++++- MobileGL/MG_Remote/Transport/RoleMemory.h | 6 +- MobileGL/MG_Remote/Transport/SessionRings.h | 162 +++++++++++++++----- MobileGL/MG_Remote/Transport/ShmSegment.cpp | 72 ++++++++- 5 files changed, 303 insertions(+), 52 deletions(-) diff --git a/MobileGL/MG_Remote/Transport/ReplySlot.h b/MobileGL/MG_Remote/Transport/ReplySlot.h index 068dbb72..5595d6a6 100644 --- a/MobileGL/MG_Remote/Transport/ReplySlot.h +++ b/MobileGL/MG_Remote/Transport/ReplySlot.h @@ -50,6 +50,14 @@ #include #include +// P9's account, named here rather than left to be rediscovered: the seq stamp +// catches a sequence space drifted by anything that is NOT a multiple of +// slotCount. A drift of exactly 8, 16, ... lands on the same slot with a matching +// stamp and reads as this call's answer. Under R-1's verb barrier the in-flight +// depth is one and a drift cannot open at all; P9 is what removes the barrier, +// and it is what has to widen the stamp (a generation beside the seq) or bound +// the drift some other way. + namespace MobileGL::MG_Remote::Transport { // CONTRACT-P5 table 0, "reply slot header". @@ -106,8 +114,24 @@ namespace MobileGL::MG_Remote::Transport { static_cast(sizeof(ReplySlotHeader))); return; } + // EVERY SLOT MUST START 8-ALIGNED. The header is written by one thread + // and read by another; the fences below 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. A geometry whose slotBytes is not a + // multiple of 8 puts later slots on odd boundaries, so it is refused + // here rather than left to a future caller to discover. + if ((slotBytes % 8) != 0 || + (reinterpret_cast(base) % alignof(ReplySlotHeader)) != 0) { + WireLogError("MG_Remote reply pool: rejected, a %llu byte slot at base alignment " + "%llu would put a slot header on an unaligned address, and the seq " + "stamp the wrong-slot check reads has to be untorn", + static_cast(slotBytes), + static_cast( + reinterpret_cast(base) % alignof(ReplySlotHeader))); + return; + } m_base = static_cast(base); - m_size = sizeBytes; m_slots = slotCount; m_mask = slotCount - 1; // Truncated to 32 bits deliberately: the header's Size field is @@ -242,7 +266,6 @@ namespace MobileGL::MG_Remote::Transport { } std::uint8_t* m_base = nullptr; - std::uint64_t m_size = 0; std::uint32_t m_slots = 0; std::uint32_t m_mask = 0; std::uint32_t m_slotBytes = 0; diff --git a/MobileGL/MG_Remote/Transport/Ring.cpp b/MobileGL/MG_Remote/Transport/Ring.cpp index b54f4635..b5ff5d6f 100644 --- a/MobileGL/MG_Remote/Transport/Ring.cpp +++ b/MobileGL/MG_Remote/Transport/Ring.cpp @@ -12,6 +12,7 @@ #include +#include #include namespace MobileGL::MG_Remote::Transport { @@ -320,6 +321,14 @@ namespace MobileGL::MG_Remote::Transport { return value; } + std::uint64_t SegmentBytesForRing(std::uint64_t ringBytes) { + const std::uint64_t ring = LargestPowerOfTwoAtMost(ringBytes); + if (ring < kMinRingCapacity || ring > kMaxRingCapacity) { + return 0; + } + return ring + sizeof(RingControl); + } + std::uint64_t RingCapacityForSegment(std::uint64_t segmentBytes) { if (segmentBytes <= sizeof(RingControl)) { return 0; @@ -333,18 +342,26 @@ namespace MobileGL::MG_Remote::Transport { namespace { // A watermark may be published LATE but never EARLY, and it may never // move BACKWARDS. Backwards is the half that is mechanically - // detectable from inside, so it is refused loudly here; "early" can - // only be caught at the call site, which is why every advance below - // has exactly one caller and a named unit case. + // detectable from inside, and it is FATAL rather than logged: the + // consumer keeps its own counter, so a shared watermark left behind + // makes every later advance a no-op and every WaitForApplied on + // kWaitForever - the verb barrier and every reply wait - block for + // ever. A hang with one ERROR line in the log is strictly worse than + // an abort at the instruction that caused it, and Ring.h:181-185 + // already rules the same way for the cursor invariants on this page. + // "Early" can only be caught at the call site, which is why every + // advance below has exactly one caller and a named unit case. void AdvanceMonotonic(std::atomic& watermark, std::uint64_t to, const char* name) { const std::uint64_t current = watermark.load(std::memory_order_relaxed); if (to < current) { - MGLOG_E("MG_Remote watermark: refusing to move %s backwards, %llu -> %llu; a " - "waiter that already resumed on the higher value cannot be un-resumed", + MGLOG_F("MGPipe: Fatal{ProtocolCorruption, \"watermark\"} %s moved backwards, " + "%llu -> %llu. A waiter that already resumed on the higher value cannot " + "be un-resumed, and every later advance of this watermark would be a " + "no-op, so the verb barrier and every reply wait would block for ever", name, static_cast(current), static_cast(to)); - return; + std::abort(); } if (to == current) { return; @@ -404,6 +421,7 @@ namespace MobileGL::MG_Remote::Transport { m_stage = nullptr; m_peerBell = nullptr; m_selfBell = nullptr; + m_lastPublishedSeq = 0; } void SessionProducer::PublishAndNotify(std::uint64_t submittedSeq) { @@ -415,8 +433,20 @@ namespace MobileGL::MG_Remote::Transport { 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. + // + // Clamped UP, not passed through. A caller that republishes an older + // bound - teardown does exactly that, and so does any batched publisher + // that lost track - is publishing LATE, which R-9 permits; it is not the + // same thing as a watermark moving backwards, which is Fatal. Doing the + // clamp here keeps that distinction at the one boundary where a stale + // argument is legitimate. + if (submittedSeq > m_lastPublishedSeq) { + m_lastPublishedSeq = submittedSeq; + } // 2. the diagnostic watermark, after the bytes it describes. - Watermark::AdvanceSubmitted(*m_control, submittedSeq); + Watermark::AdvanceSubmitted(*m_control, m_lastPublishedSeq); // 3. and only now the bell. Publish-then-ring, never ring-then-publish. if (m_peerBell != nullptr) { NotifyIfParked(*m_peerBell, m_control->consumerParked); @@ -483,6 +513,8 @@ namespace MobileGL::MG_Remote::Transport { m_selfBell = selfBell; m_spinUs = spinUs; m_appliedSeq = control == nullptr ? 0 : control->appliedSeq.load(std::memory_order_acquire); + m_retirableCursor = cmd == nullptr ? 0 : cmd->LocalTail(); + m_borrowHeld = false; } void SessionConsumer::Detach() { @@ -490,6 +522,8 @@ namespace MobileGL::MG_Remote::Transport { m_cmd = nullptr; m_peerBell = nullptr; m_selfBell = nullptr; + m_retirableCursor = 0; + m_borrowHeld = false; } SessionWait SessionConsumer::WaitForWork(std::uint32_t timeoutMs) { @@ -515,7 +549,45 @@ namespace MobileGL::MG_Remote::Transport { return; } Watermark::AdvanceRetired(*m_control, seq); - m_cmd->PublishRetired(); + // PublishRetiredUpTo, NOT PublishRetired: the latter stores m_localTail + // into BOTH tails, i.e. it hands back every byte the consumer has popped + // whether or not a record among them was borrowed into the GPU timeline. + // That would defeat the whole reason the ring carries two tails + // (Ring.h:24-27) and would let the producer overwrite a slot the GPU is + // still reading. m_retirableCursor stops at the first borrowed record. + m_cmd->PublishApplied(); + m_cmd->PublishRetiredUpTo(m_retirableCursor); + NotifyClient(); + } + + void SessionConsumer::RetireBorrowedUpTo(std::uint64_t cursor) { + if (!Valid()) { + return; + } + if (cursor > m_retirableCursor) { + m_retirableCursor = cursor; + // A release that reaches everything popped so far clears the latch; + // anything still unreleased keeps it set, so a second borrow behind + // the first is not skipped. + m_borrowHeld = cursor < m_cmd->LocalTail(); + } + m_cmd->PublishRetiredUpTo(m_retirableCursor); + NotifyClient(); + } + + void SessionConsumer::CompleteFrame(std::uint64_t serial) { + if (!Valid()) { + return; + } + Watermark::AdvanceCompletedFrame(*m_control, serial); + NotifyClient(); + } + + void SessionConsumer::ReturnPresentCredit(std::uint64_t serial) { + if (!Valid()) { + return; + } + Watermark::AdvancePresentAck(*m_control, serial); NotifyClient(); } diff --git a/MobileGL/MG_Remote/Transport/RoleMemory.h b/MobileGL/MG_Remote/Transport/RoleMemory.h index fcbe30ad..4fc61061 100644 --- a/MobileGL/MG_Remote/Transport/RoleMemory.h +++ b/MobileGL/MG_Remote/Transport/RoleMemory.h @@ -77,8 +77,10 @@ namespace MobileGL::MG_Remote::Transport { RoleMemorySample SampleRoleMemory(MemoryRole role); - // Emits one line at ERROR level (the wire layer's only level - WireLog.h) so - // t1's harness can grep it out of a lane log without a new log sink. + // Emits one line at INFO level, which is what every P5 lane builds at, so + // t1's harness can grep it out of a lane log without a new log sink. Not + // DEBUG, which the INFO build compiles out; not ERROR, which this is not. + // The grep tag is `MG_Remote memory[`. // `phase` is a short tag: "handshake", "first-frame", "teardown". void LogRoleMemory(const char* phase, const RoleMemorySample& sample); diff --git a/MobileGL/MG_Remote/Transport/SessionRings.h b/MobileGL/MG_Remote/Transport/SessionRings.h index a52429e3..381bdc7b 100644 --- a/MobileGL/MG_Remote/Transport/SessionRings.h +++ b/MobileGL/MG_Remote/Transport/SessionRings.h @@ -28,27 +28,37 @@ // phase is trying not to repeat. // // --------------------------------------------------------------------------- -// THE RING CAPACITY IS HALF THE SEGMENT, AND THAT IS ARITHMETIC, NOT A CHOICE. +// THE KNOB NAMES THE RING; THE SEGMENT IS THE RING PLUS ONE CONTROL PAGE. // -// Ring.h:11-13 puts RingControl at the HEAD of SEG_CMD, and RingProducer requires -// a POWER-OF-TWO capacity (Ring.cpp:89-103, the mask is the indexing). A segment -// of 8 MiB therefore has 8 MiB - 4096 bytes left for records, and the largest -// power of two that fits is 4 MiB. A record may be at most half the ring -// (RingProducer::MaxRecordBytes), so the real cap on one record is 2 MiB. +// Ring.h:11-13 puts RingControl at the HEAD of SEG_CMD and RingProducer requires +// a POWER-OF-TWO capacity (Ring.cpp:89-103 - the mask IS the indexing). Those two +// facts together mean a segment and its ring cannot both be 8 MiB, and one of the +// two numbers has to give. // -// CONTRACT-P5 §5 and Config.h's MOBILEGL_IPC_RING_MB comment both say "8 MiB caps -// one record at 4 MiB". That arithmetic assumed the whole segment is ring bytes -// and did not subtract the control page. The number here is HALF of theirs, and -// the deviation is deliberately in the SAFE direction: R-10's obligation is to -// PROVE no record ever approaches the cap, and a lower cap makes that proof fire -// earlier and louder rather than later and silently. The alternatives were both -// worse - announcing SegmentRef.sizeBytes as 4096 + 8 MiB breaks the four sizes -// ProtocolSmokeTest.cpp:72 pins, and moving RingControl out of SEG_CMD needs a -// fifth SegmentRef that Welcome does not have. +// The one that gives is the SEGMENT: SEG_CMD is `MOBILEGL_IPC_RING_MB` MiB PLUS +// 4096, so the ring inside it is exactly MOBILEGL_IPC_RING_MB MiB and +// RingProducer::MaxRecordBytes() is exactly half of that. CONTRACT-P5 §5 and +// Config.h's MOBILEGL_IPC_RING_MB comment - "A RECORD MAY BE AT MOST HALF OF +// THIS, so 8 MiB caps one record at 4 MiB" - are then TRUE AS WRITTEN, which +// matters because that sentence is what every other package sizes against. +// +// The first version of this file did the opposite: an 8 MiB segment with a 4 MiB +// ring and a 2 MiB record cap, on the grounds that ProtocolSmokeTest.cpp:72 pinned +// the four announced sizes. That was wrong on the facts - that test builds four +// SegmentRefs from its own literals and round-trips them through the schema; it +// says nothing about what a session announces, and it never mentions +// SessionSegments at all. So the alternative was available at no cost, and the +// version that made two live documents false and left half of SEG_CMD mapped and +// unreachable was the worse of the two. +// +// SegmentRef.sizeBytes therefore announces the MAPPING size (ring + page), which +// is what a spawn peer must mmap. The four numbers a reader recognises - 8 MiB / +// 32 MiB / 8 MiB / 256 KiB - are the RING sizes, which is what the knobs name. // // SEG_STAGE has no control page of its own: RingControl carries TWO cursor -// triples (Ring.h:101-109) and the stage triple is the second. So SEG_STAGE's -// capacity is its whole segment, and 32 MiB is already a power of two. +// triples (Ring.h:101-109) and the stage triple is the second. So SEG_STAGE is +// exactly its ring, and 32 MiB is already a power of two. SEG_REPLY is not a ring +// at all. // --------------------------------------------------------------------------- #pragma once @@ -65,22 +75,29 @@ namespace MobileGL::MG_Remote::Transport { - // The four sizes are CONTRACT-P5's and are pinned by ProtocolSmokeTest.cpp:72. - // MOBILEGL_IPC_RING_MB / MOBILEGL_IPC_STAGE_MB move the first two. + // RING sizes, not segment sizes - see the header block. The four defaults are + // 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 CmdBytes = 8ull * 1024 * 1024; - std::uint64_t StageBytes = 32ull * 1024 * 1024; - std::uint64_t ReplyBytes = 8ull * 1024 * 1024; - std::uint64_t EventBytes = 256ull * 1024; + 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::uint32_t ReplySlotCount = kDefaultReplySlotCount; }; // Largest power of two <= `bytes`, or 0 when there is none. The ring's - // indexing is a mask, so this is what any segment's usable ring area is. + // indexing is a mask, so this is what a ring capacity has to be rounded to. std::uint64_t LargestPowerOfTwoAtMost(std::uint64_t bytes); - // Usable ring capacity of a segment that carries a RingControl page at its - // head. See the header block above for why this is half the segment. + // How big a segment has to be to hold `ringBytes` of ring behind its control + // page. `ringBytes` is rounded DOWN to a power of two first, so an operator + // who asks for 6 MiB gets a 4 MiB ring in a 4 MiB + 4096 segment rather than + // a segment whose tail can never be addressed. + std::uint64_t SegmentBytesForRing(std::uint64_t ringBytes); + + // The usable ring inside a segment that carries a RingControl page at its + // head. The inverse of SegmentBytesForRing for any size it produced. std::uint64_t RingCapacityForSegment(std::uint64_t segmentBytes); enum class SessionSegmentSlot : std::uint32_t { @@ -111,9 +128,24 @@ namespace MobileGL::MG_Remote::Transport { // and SEG_EVENT's), and books the mapping in `role`'s ledger. MobileGLResult Create(const SessionSegmentSizes& sizes, MemoryRole role); - // The inproc peer's view: the SAME mapping, booked under the OTHER role. - // It does not re-init the control pages - there is one shared page and - // re-initialising it would zero the owner's cursors under it. + // The inproc peer's view of the owner's four segments, booked under the + // OTHER role. It does NOT re-init the control pages - there is one shared + // page per ring and re-initialising it would zero the owner's cursors out + // from under whoever is already using them. + // + // ON POSIX THIS IS A REAL SECOND MAPPING, NOT AN ALIAS: each descriptor is + // dup()ed and adopted through ShmSegment::Adopt + Map, so the peer gets + // its own virtual addresses over the same memfd. That is the same reason + // inproc uses ShmSegment at all (see the header block): the ATTACH half is + // the half P6 replaces with an SCM_RIGHTS Adopt, and aliasing the owner's + // ShmSegment objects would leave it first exercised on the day the second + // process appears - which is exactly the criticism this file levels at + // new[]. It also removes a raw lifetime coupling: an aliased view holds + // pointers into the owner's members with no ownership, so the two Closes + // have to be ordered by hand. + // + // Windows has no Adopt (ShmSegment::Adopt is POSIX-only; the section name + // travels in SegmentRef instead), so there it still aliases and says so. MobileGLResult AttachInProcess(SessionSegments& owner, MemoryRole role); void Close(); @@ -177,10 +209,19 @@ namespace MobileGL::MG_Remote::Transport { // THE ONE RULE THAT MATTERS: a watermark may be published LATE but NEVER // EARLY. Late costs a waiter some latency; early makes every waiter a silent // use of work that has not happened, and there is no checksum anywhere on - // this ring that would catch it. So the advances below REFUSE to move a - // watermark backwards (that is the detectable half) and the callers are - // responsible for never calling them before the work is done (that is the - // half only a call-site review and R-9's unit cases can enforce). + // this ring that would catch it. The callers are responsible for never + // calling an advance before the work is done - that is the half only a + // call-site review and R-9's unit cases can enforce. + // + // THE HALF THAT IS MECHANICALLY DETECTABLE - a watermark moving BACKWARDS - + // IS FATAL, not logged-and-ignored. Logging it and returning was the first + // version of this file and it was worse than useless: SessionConsumer keeps + // its own counter, so once the shared appliedSeq is behind, every later + // advance is a no-op for ever and every WaitForApplied(seq, kWaitForever) - + // the verb barrier and every reply wait - blocks permanently. The user sees a + // hang and the only evidence is one ERROR line. This is the same class as + // RingCursorsValid returning false, and Ring.h:181-185 already calls that "a + // Fatal{ProtocolCorruption}, never a retry". namespace Watermark { // Producer, after Publish. Nobody waits on it - it is the answer to "how @@ -247,6 +288,17 @@ namespace MobileGL::MG_Remote::Transport { // call order; this is the one place production code performs it. void PublishAndNotify(std::uint64_t submittedSeq); + // The last seq THIS producer published, kept locally rather than read back + // out of RingControl::submittedSeq. Teardown's drain needs it: Ring.h:72-77 + // explicitly permits submittedSeq to be published LAZILY and Ring.h:243 + // encourages batching the publish, so the shared watermark may lag the + // emitter - and a drain that waits for `appliedSeq >= submittedSeq` would + // then under-wait and free an emitter's var-tail while a record still + // names it. With the verb barrier armed the two are equal; with + // MOBILEGL_IPC_VERB_BARRIER=0, R-1's negative control which the phase has + // to run once, they are not. + std::uint64_t LastPublishedSeq() const { return m_lastPublishedSeq; } + // The verb barrier's wait, AND the reply's wait: they are the same wait // (R-3/R-5), which is why a blocking ReadPixels, MapPersistent's decline // and the four Bool acceptances cost ZERO extra round trips. @@ -277,6 +329,7 @@ namespace MobileGL::MG_Remote::Transport { Doorbell* m_peerBell = nullptr; Doorbell* m_selfBell = nullptr; std::uint32_t m_spinUs = kDefaultSpinUs; + std::uint64_t m_lastPublishedSeq = 0; }; // ----------------------------------------------------------------------- @@ -322,15 +375,50 @@ namespace MobileGL::MG_Remote::Transport { ++m_appliedSeq; Watermark::AdvanceApplied(*m_control, m_appliedSeq); m_cmd->PublishApplied(); + // THE BYTE CURSOR RetireThrough MAY RECLAIM TO, which is NOT simply + // "everything popped". A record carrying kRecBorrowSlot has been lent + // into the GPU timeline and its slot can only be recycled after + // completedFrameSerial (Ring.h:24-27), so the reclaimable cursor stops + // AT the first borrowed record and does not move again until + // RetireBorrowedUpTo releases it. Nothing sets kRecBorrowSlot yet; + // this is here so that the day something does, the producer does not + // overwrite a slot the GPU is still reading. + if ((view.flags & kRecBorrowSlot) == 0 && !m_borrowHeld) { + m_retirableCursor = view.cursor + sizeof(RingRecordHeader) + view.payloadSize; + } else { + m_borrowHeld = true; + } NotifyClient(); return true; } - // Records without kRecBorrowSlot retire as soon as they are applied; a - // borrowed slot retires on completedFrameSerial, which is why this is a - // separate call and not folded into ApplyOne. + // Publish the retire watermark and hand back every byte up to the first + // still-borrowed record. + // + // IT IS MANDATORY, NOT OPTIONAL. RingProducer::FreeBytes() reclaims against + // retiredTail ONLY (Ring.cpp:110-115) and nothing else in this class + // publishes it, so an apply loop that calls ApplyOne and never this wedges + // the producer on the first full ring. Call it once per drain batch. void RetireThrough(std::uint64_t seq); + // Release borrowed slots up to `cursor` once completedFrameSerial has + // passed them. `cursor` is a RingRecordView::cursor the apply loop kept. + // This is the only thing that moves the reclaim point past a borrowed + // record - see ApplyOne. + void RetireBorrowedUpTo(std::uint64_t cursor); + + // The byte cursor RetireThrough would reclaim to right now. Diagnostic; + // a borrow that is never released shows up as this number standing still. + std::uint64_t RetirableCursor() const { return m_retirableCursor; } + + // completedFrameSerial and presentAckSerial, advanced AND rung. The free + // functions in namespace Watermark advance only: a v1 caller that used one + // directly would leave a client parked in WaitForPresentAck(kWaitForever) + // with nothing to wake it, because the advance and the doorbell are two + // separate stores and only the pair is a wakeup. + void CompleteFrame(std::uint64_t serial); + void ReturnPresentCredit(std::uint64_t serial); + // Ring the client's bell, but only when it said it is parked: a store to // a shared cache line otherwise burns a big core for a whole frame on a // phone (Doorbell.h:13-22). @@ -350,6 +438,8 @@ namespace MobileGL::MG_Remote::Transport { Doorbell* m_selfBell = nullptr; std::uint32_t m_spinUs = kDefaultSpinUs; std::uint64_t m_appliedSeq = 0; + std::uint64_t m_retirableCursor = 0; + bool m_borrowHeld = false; }; // ----------------------------------------------------------------------- diff --git a/MobileGL/MG_Remote/Transport/ShmSegment.cpp b/MobileGL/MG_Remote/Transport/ShmSegment.cpp index 54a7f715..e07be3ea 100644 --- a/MobileGL/MG_Remote/Transport/ShmSegment.cpp +++ b/MobileGL/MG_Remote/Transport/ShmSegment.cpp @@ -25,6 +25,10 @@ #include #include +#if !defined(_WIN32) +#include +#endif + namespace MobileGL::MG_Remote::Transport { ShmSegment::~ShmSegment() { Close(); } @@ -180,17 +184,37 @@ namespace MobileGL::MG_Remote::Transport { MobileGLResult SessionSegments::Create(const SessionSegmentSizes& sizes, MemoryRole role) { Close(); + // Set BEFORE the loop, so that Close() on a failure INSIDE it really + // closes what has already been created: Close only walks m_owned when + // m_owns is true, and setting it afterwards left a failure at segment 3 + // holding segments 0-2's descriptors and mappings open with Valid() + // false, against ShmSegment.h:66's "unmaps and releases the descriptor". + m_owns = true; struct Spec { const char* name; std::uint64_t bytes; }; + // The sizes are RING sizes; a segment that carries a control page at its + // head is that much bigger. SEG_STAGE drives the SECOND cursor triple of + // SEG_CMD's page and SEG_REPLY is not a ring at all, so neither of those + // two grows. const Spec specs[kSlotCount] = { - {"mgl-cmd", sizes.CmdBytes}, - {"mgl-stage", sizes.StageBytes}, + {"mgl-cmd", SegmentBytesForRing(sizes.CmdRingBytes)}, + {"mgl-stage", LargestPowerOfTwoAtMost(sizes.StageRingBytes)}, {"mgl-reply", sizes.ReplyBytes}, - {"mgl-event", sizes.EventBytes}, + {"mgl-event", SegmentBytesForRing(sizes.EventRingBytes)}, }; + for (std::size_t index = 0; index < kSlotCount; ++index) { + if (specs[index].bytes == 0) { + MGLOG_E("MG_Remote session: segment %s was asked for a ring size that cannot be " + "made into one (a ring is a power of two between %llu and %llu bytes)", + specs[index].name, static_cast(kMinRingCapacity), + static_cast(kMaxRingCapacity)); + Close(); + return MOBILEGL_ERR_INVALID_ARGUMENT; + } + } for (std::size_t index = 0; index < kSlotCount; ++index) { const MobileGLResult created = @@ -215,7 +239,6 @@ namespace MobileGL::MG_Remote::Transport { } } - m_owns = true; m_replySlotCount = sizes.ReplySlotCount; DeriveViews(); if (!m_valid) { @@ -240,10 +263,51 @@ namespace MobileGL::MG_Remote::Transport { if (!owner.Valid()) { return MOBILEGL_ERR_NOT_INITIALIZED; } +#if !defined(_WIN32) + // A REAL SECOND MAPPING, not an alias. dup + Adopt + Map is byte for byte + // the call sequence P6's SCM_RIGHTS client runs, so mapping, the fstat + // size check inside Adopt (ShmSegmentPosix.cpp:119-141), alignment and the + // peer's own lifetime are all exercised now rather than on the day the + // second process appears. Aliasing the owner's ShmSegment objects would + // leave the attach half untested for exactly the reason this file refuses + // to allocate the rings with new[]. + m_owns = true; + for (std::size_t index = 0; index < kSlotCount; ++index) { + const ShmSegment* theirs = owner.m_segments[index]; + const int duplicate = theirs == nullptr ? -1 : ::dup(theirs->Fd()); + if (duplicate < 0) { + MGLOG_E("MG_Remote session: could not dup the owner's descriptor for segment %zu", + index); + Close(); + return MOBILEGL_ERR_INVALID_ARGUMENT; + } + // Adopt takes ownership of `duplicate` on success only. + const MobileGLResult adopted = + ShmSegment::Adopt(duplicate, theirs->Size(), m_owned[index]); + if (adopted != MOBILEGL_OK) { + ::close(duplicate); + Close(); + return adopted; + } + // Read/write: under inproc the client writes SEG_CMD and SEG_STAGE and + // reads SEG_REPLY and SEG_EVENT, and one ShmSegment maps the whole + // thing one way. The per-segment read-only peer view is P6's, where + // the roles are separable. + const MobileGLResult mapped = m_owned[index].Map(false); + if (mapped != MOBILEGL_OK) { + Close(); + return mapped; + } + } +#else + // Windows has no Adopt (ShmSegment::Adopt is POSIX-only; a Windows peer + // resolves the section by the name carried in SegmentRef). Alias, and say + // so: this arm does not exercise the attach path P6 replaces. for (std::size_t index = 0; index < kSlotCount; ++index) { m_segments[index] = owner.m_segments[index]; } m_owns = false; +#endif m_replySlotCount = owner.m_replySlotCount; DeriveViews(); if (!m_valid) {