From bdd4bed431949b16e01a33e2c3a81f0fe58cb0a6 Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Sat, 5 Sep 2026 14:42:44 -0400 Subject: [PATCH] [Fix] (MG_Remote, Transport): close the doorbell's lost-wakeup window with two seq_cst fences and stop a hung-up peer turning every park into a spin - The header claimed the park flag's own seq_cst store and load closed the lost-wakeup window. They cannot: that Dekker argument needs all FOUR accesses in the seq_cst total order, and the other two are not in it - the watermark publish is a release store (RingProducer::Publish) and the condition re-test is an acquire load. There was no atomic_thread_fence anywhere under MG_Remote. On x86 a release store and a seq_cst load are both plain MOVs, so the notifier can read parked==0 while its head store still sits in the store buffer, and the waiter then parks on a stale watermark forever; ARMv8 survived only because STLR->LDAR is RCsc, which is luck, not the design. Doorbell::Wait now fences after announcing and NotifyIfParked fences before reading the flag - the pairing the standard actually guarantees ([atomics.order]) - and the header says so, including the other half of the contract: publish the watermark BEFORE ringing, because a fence only orders what precedes it. This is the claim the whole P5/P6 wait discipline (present credit, kNeedsAck blocking requests, full-ring escalation) will be built on, and its failure mode is a silent cross-process hang. Inherited design, plan section 8.1 (PLAN.md section 6.2/6.2a: bidirectional doorbell, MOBILEGL_IPC_SPIN_US default 50us, condvar for inproc). - SocketDoorbell::Park treated any poll() return > 0 as a wakeup and never looked at revents. Measured on this machine: an AF_UNIX SOCK_STREAM socketpair whose peer has closed returns revents=POLLIN|POLLHUP with recv()==0 immediately and forever. Park therefore returned true, Wait stored parked=0, found its condition still false and re-parked - so a waiter with kWaitForever burned a big core at full clock with no bound. That is exactly the pathology the bidirectional doorbell exists to prevent (a whole 16.6ms frame of a big core on a phone, competing with the GPU and the game's JVM), reached from the other side. Park now branches on revents, a new Drain() latches death on EOF (and on ECONNRESET/EPIPE from Notify), and the new Doorbell::Dead() lets Wait give up instead of re-parking on a descriptor that can never deliver another wakeup. - Same commit, same defect class: `fd` is documented as one end of an AF_UNIX socket pair, not "a socket or pipe end". Notify uses send(MSG_DONTWAIT| MSG_NOSIGNAL) and Park uses poll()+recv(), which a pipe end refuses with ENOTSOCK, and a SOCK_DGRAM pair reports no readiness at all when the peer closes (measured), so the spawn transport wants SOCK_STREAM. - Evidence: build-linux-split rebuilt clean; the wire suite is green (47/47). Negative control - reinstating the revents-blind Park makes FdPassingTest.SocketDoorbellStopsParkingWhenThePeerHangsUp fail on both Park assertions, and restoring this code turns it green again. --- MobileGL/MG_Remote/Transport/Doorbell.cpp | 73 +++++++++++++++--- MobileGL/MG_Remote/Transport/Doorbell.h | 93 +++++++++++++++++++---- 2 files changed, 141 insertions(+), 25 deletions(-) diff --git a/MobileGL/MG_Remote/Transport/Doorbell.cpp b/MobileGL/MG_Remote/Transport/Doorbell.cpp index 1d02da81..8c530195 100644 --- a/MobileGL/MG_Remote/Transport/Doorbell.cpp +++ b/MobileGL/MG_Remote/Transport/Doorbell.cpp @@ -103,8 +103,11 @@ namespace MobileGL::MG_Remote::Transport { // one pending, which is all a doorbell promises. return; } - if (written < 0 && errno == EPIPE) { - return; // peer gone; the waiter learns it from its own read + if (written < 0 && (errno == EPIPE || errno == ECONNRESET)) { + // The peer is gone: it can never ring back either, so latch it + // here too rather than waiting for a Park to discover it. + m_dead = true; + return; } MGLOG_D("MG_Remote doorbell: send failed (errno=%d)", errno); return; @@ -112,7 +115,7 @@ namespace MobileGL::MG_Remote::Transport { } bool SocketDoorbell::Park(std::uint32_t timeoutMs) { - if (m_fd < 0) { + if (m_fd < 0 || m_dead) { return false; } const auto start = std::chrono::steady_clock::now(); @@ -139,28 +142,78 @@ namespace MobileGL::MG_Remote::Transport { if (ready == 0) { return false; // timed out } - Reset(); - return true; + // revents has to be inspected, not just `ready > 0`. Once the peer + // closes its end the descriptor is permanently poll-ready with + // nothing to read (measured on Linux: revents=POLLIN|POLLHUP, + // recv()==0), so treating any readiness as a wakeup turns every + // park on a dead peer into a 100% CPU spin - unbounded, because + // Doorbell::Wait re-parks until its deadline and kWaitForever has + // none. + if ((pfd.revents & (POLLERR | POLLNVAL)) != 0) { + MGLOG_D("MG_Remote doorbell: fd %d unusable (revents=0x%X)", m_fd, + static_cast(pfd.revents)); + m_dead = true; + return false; + } + if ((pfd.revents & POLLIN) != 0) { + if (Drain() != 0) { + return true; // a real wakeup byte + } + if (m_dead) { + return false; // EOF, not an event + } + // Ready but empty and still alive: someone else drained it. + // Report the wakeup and let the caller re-test its condition. + return true; + } + if ((pfd.revents & POLLHUP) != 0) { + m_dead = true; + return false; + } + // Readiness with no bit we requested or recognise: there is + // nothing to consume and no way to make progress, so refuse to + // poll this descriptor again. + MGLOG_D("MG_Remote doorbell: fd %d ready with revents=0x%X", m_fd, + static_cast(pfd.revents)); + m_dead = true; + return false; } } - void SocketDoorbell::Reset() { - if (m_fd < 0) { - return; - } + std::uint64_t SocketDoorbell::Drain() { // Level-triggered to edge-triggered: swallow every queued byte so one // stale wakeup cannot make later Parks return without an event. + std::uint64_t consumed = 0; std::uint8_t scratch[64]; for (;;) { const ssize_t got = ::recv(m_fd, scratch, sizeof(scratch), MSG_DONTWAIT); if (got > 0) { + consumed += static_cast(got); continue; } - if (got < 0 && errno == EINTR) { + if (got == 0) { + // Orderly shutdown on a stream socket: the peer is gone and + // will never ring again. + m_dead = true; + return consumed; + } + if (errno == EINTR) { continue; } + if (errno == EAGAIN || errno == EWOULDBLOCK) { + return consumed; // drained + } + MGLOG_D("MG_Remote doorbell: recv failed (errno=%d)", errno); + m_dead = true; + return consumed; + } + } + + void SocketDoorbell::Reset() { + if (m_fd < 0 || m_dead) { return; } + (void)Drain(); } #endif // !_WIN32 diff --git a/MobileGL/MG_Remote/Transport/Doorbell.h b/MobileGL/MG_Remote/Transport/Doorbell.h index 78fc48c4..ec37678c 100644 --- a/MobileGL/MG_Remote/Transport/Doorbell.h +++ b/MobileGL/MG_Remote/Transport/Doorbell.h @@ -26,10 +26,30 @@ // - CondVarDoorbell for `inproc` (one process, two threads), // - SocketDoorbell for `spawn` (one byte on a socket; POSIX only). // -// The lost-wakeup window is closed by ordering, not by luck: the waiter stores -// its park flag and THEN re-tests the condition, while the notifier publishes -// the watermark and THEN tests the park flag. Both use seq_cst on those two -// accesses, so at least one of the two sees the other. +// The lost-wakeup window is closed by two seq_cst FENCES, not by the ordering +// of the park flag's own load and store: +// - the waiter sets the flag, executes std::atomic_thread_fence(seq_cst), +// and THEN re-tests the condition (Doorbell::Wait); +// - the notifier publishes its watermark, executes the same fence, and THEN +// reads the flag (NotifyIfParked). +// Both fences sit in the single seq_cst total order, so one precedes the +// other, and [atomics.order] then forces at least one side to observe the +// other's store. The flag's own accesses may be relaxed: they are not what +// closes the window. +// +// A seq_cst store paired with a seq_cst load would NOT be enough, which is +// why the fences are here and why neither may be removed. That Dekker +// argument needs all FOUR accesses in the total order, and the other two are +// not: the watermark publish is a release store (RingProducer::Publish) and +// the condition re-test is an acquire load. On x86 the gap is concrete rather +// than theoretical - a release store is a plain MOV that can still sit in the +// store buffer while the load of the park flag, also a plain MOV, reads 0, so +// the notifier skips the ring and the waiter parks on a stale watermark +// forever. (ARMv8 survives it only because STLR->LDAR is RCsc, i.e. by luck.) +// +// The other half of the contract is ordering between the caller and the +// fence: NotifyIfParked must be called AFTER the watermark is published. A +// fence only orders what precedes it. #pragma once @@ -84,6 +104,14 @@ namespace MobileGL::MG_Remote::Transport { // does not make the next Park return spuriously forever. virtual void Reset() = 0; + // True once the wakeup channel is permanently unusable, e.g. the peer + // closed its end of the socket. A dead doorbell can never deliver + // another wakeup AND its descriptor is permanently poll-ready, so Wait + // must stop re-parking on it: otherwise a waiter with no deadline + // burns a big core at full clock, which is the exact pathology the + // bidirectional doorbell exists to prevent. + virtual bool Dead() const { return false; } + // Spin `spinUs`, then park until `ready()` or the deadline. // `parked` is the RingControl flag the peer tests before ringing. template @@ -106,16 +134,17 @@ namespace MobileGL::MG_Remote::Transport { } for (;;) { - // Announce, THEN re-test: the notifier publishes and then reads - // this flag, so one of the two orderings always sees the other. - parked.store(1, std::memory_order_seq_cst); + // Announce, FENCE, then re-test. The fence is the mechanism - + // see the file header - so setting the flag itself is relaxed. + parked.store(1, std::memory_order_relaxed); + std::atomic_thread_fence(std::memory_order_seq_cst); if (ready()) { - parked.store(0, std::memory_order_seq_cst); + parked.store(0, std::memory_order_relaxed); return true; } const auto now = std::chrono::steady_clock::now(); if (now >= deadline) { - parked.store(0, std::memory_order_seq_cst); + parked.store(0, std::memory_order_relaxed); return ready(); } std::uint32_t chunkMs = kWaitForever; @@ -125,10 +154,22 @@ namespace MobileGL::MG_Remote::Transport { chunkMs = remaining <= 0 ? 0 : static_cast(remaining); } Park(chunkMs); - parked.store(0, std::memory_order_seq_cst); + // Clearing is relaxed on purpose: a notifier that reads a + // stale 1 only rings a bell nobody is waiting on, which the + // doorbell remembers and the next Park consumes. The dangerous + // direction - a notifier reading 0 while the waiter is really + // parked - is the one the fence above rules out. + parked.store(0, std::memory_order_relaxed); if (ready()) { return true; } + if (Dead()) { + // Nothing can ring this bell again and parking on it no + // longer blocks, so looping here would spin at full clock + // for as long as the caller is willing to wait - which, + // with kWaitForever, is forever. + return false; + } if (timeoutMs != kWaitForever && std::chrono::steady_clock::now() >= deadline) { return false; } @@ -139,10 +180,17 @@ namespace MobileGL::MG_Remote::Transport { Doorbell() = default; }; - // Rings `bell` only when the peer said it is parked. The seq_cst load pairs - // with the waiter's seq_cst store of the same flag. + // Rings `bell` only when the peer said it is parked. + // + // PRECONDITION: whatever the waiter's condition reads - the ring head, a + // sequence watermark, a queue push - is ALREADY published when this is + // called. The fence only orders what precedes it, so ringing before + // publishing reopens the window this closes. The fence pairs with the one + // in Doorbell::Wait; see the file header for why the flag's own memory + // order is not what makes this sound. inline void NotifyIfParked(Doorbell& bell, std::atomic& parked) { - if (parked.load(std::memory_order_seq_cst) != 0) { + std::atomic_thread_fence(std::memory_order_seq_cst); + if (parked.load(std::memory_order_relaxed) != 0) { bell.Notify(); } } @@ -168,21 +216,36 @@ namespace MobileGL::MG_Remote::Transport { // and is not part of this skeleton. class SocketDoorbell final : public Doorbell { public: - // `fd` must be a socket or pipe end. When `ownsFd` the descriptor is - // closed with this object. `code` is the byte written by Notify. + // `fd` must be one end of an AF_UNIX socket pair, not a pipe: Notify + // uses send() with MSG_DONTWAIT|MSG_NOSIGNAL and Park uses + // poll()+recv(), which a pipe end refuses with ENOTSOCK. Prefer + // SOCK_STREAM for the spawn transport - measured on Linux, a closed + // peer makes a stream end report POLLIN|POLLHUP with recv()==0, which + // is how death is detected, while a SOCK_DGRAM end reports no + // readiness at all and a waiter with no deadline would simply hang. + // When `ownsFd` the descriptor is closed with this object. `code` is + // the byte written by Notify. SocketDoorbell(int fd, std::uint8_t code, bool ownsFd); ~SocketDoorbell() override; void Notify() override; bool Park(std::uint32_t timeoutMs) override; void Reset() override; + bool Dead() const override { return m_dead; } int Fd() const { return m_fd; } private: + // Consumes every queued wakeup byte and returns how many. Latches + // m_dead on EOF: recv returning 0 on a stream socket is the peer's + // hangup, not a wakeup, and the descriptor stays poll-ready forever + // afterwards. + std::uint64_t Drain(); + int m_fd; std::uint8_t m_code; bool m_ownsFd; + bool m_dead = false; }; #endif