swung0x48/MobileGL
1
0
Fork 0
mirror of https://github.com/MobileGL-Dev/MobileGL synced 2026-09-07 19:58:32 +09:00
Code Issues Packages Projects Releases Wiki Activity

[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

Browse Source 
- 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.
This commit is contained in:
swung0x48
2026-09-05 20:16:49 -04:00
parent 10315e71f3
commit bdd4bed431
2 changed files with 141 additions and 25 deletions
Download Patch File Download Diff File Expand all files Collapse all files
MobileGL/MG_Remote/Transport/Doorbell.cpp
+63 -10
View File
@@ -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<unsigned>(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<unsigned>(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<std::uint64_t>(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
MobileGL/MG_Remote/Transport/Doorbell.h
+78 -15
View File
@@ -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 <class Ready>
@@ -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<std::uint32_t>(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<std::uint32_t>& 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