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