Skip to content

fix(umbp): stop standalone client_mu_ serializing the whole node behind one rank - #700

Draft
isytwu wants to merge 3 commits into
ROCm:mainfrom
isytwu:fix/umbp-standalone-client-mu-pin-clean
Draft

isytwu wants to merge 3 commits into
ROCm:mainfrom
isytwu:fix/umbp-standalone-client-mu-pin-clean

Conversation

@isytwu

@isytwu isytwu commented Sep 23, 2026 •

Copy link
Copy Markdown
Collaborator

StandaloneServer::client_mu_ is one mutex per node, not per rank — the server address is keyed on UMBP_NODE_ID and a bootstrap lock keeps a single server process, so all 8 TP ranks share it. Two paths held it exclusively across work that is not short, and every BatchExists on the node queued behind them:

  1. Memory registration — the dominant cause of the startup stall.
    RegisterFd → RegisterBackendMemory wrapped the inner RegisterMemory, which ends in an RDMA MR pin over the worker's whole host KV pool. standalone_process_client.cpp records the measured cost: "1187840 MiB in 599.6 s", plus per-GPU IPC registration each well over a minute. Ranks register at staggered times during warmup, so for minutes at a stretch the whole node's data plane sat behind one rank's pin. This matches the observed symptom exactly: confined to the startup window, 70-260 s, self-clearing, varying run to run.

  2. Bulk writes. The put handlers held it until the backend call returned, covering the data copy and the BatchRoutePut round trip to master.

Why the lock could not simply be narrowed

Holding client_mu_ across the backend call was also, incidentally, the lifetime barrier for resolved host/GPU mappings — nothing could munmap a buffer, or tear down its RDMA MR, underneath a copy in flight. (PoolClient hands out copies of a region's TransferRef, so the inner client has no pin of its own.)

The fix

standalone_server.cpp — a per-region pin replaces the lock as the lifetime barrier. memory_ holds shared_ptr<Region> (immutable RegisteredMemory + atomic count). ResolveRange/ResolveRanges — the single choke point every data path goes through — pin what they resolve while still holding memory_mu_, which is what makes the acquisition race-free. ReleaseRegisteredMemory waits for the count to drain, holding neither lock. Data handlers and RegisterBackendMemory now take client_mu_ shared; writes reuse the read path's ConditionalDataLock, so the SSD medium keeps serializing exactly as before and only DRAM gains concurrency. Clear and shutdown stay exclusive.

pool_client.cpp — the same shape one layer down: the slow transfer_engine_->RegisterMemory ran under registered_mem_mutex_, the lock FindRegisteredMemory takes once per range on every transfer. Moved that exclusion to a new registration_mutex_ held across the engine call — that is what IOEngine actually needs, its memPool/backends carrying no lock of their own. Moved, not relaxed.

Why the inner client tolerates the new concurrency

DistributedClient already had exactly this discipline (op_mutex_ shared for every data op and register/deregister, exclusive only for Clear/Close). PoolClient serializes just its staging arenas; PeerPool keeps backend calls outside operation_mutex_; each transfer engine carries its own lock. Pinning until the call returns is sufficient because the inner data-plane calls are synchronous w.r.t. caller pointers — the remote submit-then-wait handles are function-local and always waited before return, with the destructor draining on exceptional exit.

Testing

test_standalone_shm_ipc 15/15, stable over 5 consecutive runs. New cases: multi-region ranged deregistration race, concurrent puts, registration churn under load, bounded-latency exists probe. Plus ConcurrentRegistrationsAndLookupsStayConsistent in test_pool_client_batch_put.

Verified by mutation — the new tests have teeth:

mutation result
WaitForPinsZero returns immediately segfault 3/3
RegionPins::Add pins only the first region segfault 5/5

The second one is worth calling out: the multi-region test initially passed with that mutation, because a teardown releases regions in registration order and that ordering accidentally shielded the unpinned region. The test now lists its ranges last-region-first on purpose, which removes the accident and leaves the pin as the only thing standing between the copy and an unmapped buffer.

TSAN: 15/15 pass. 67 warnings, all with their peer frame in uninstrumented prebuilt libs (libgrpc/libprotobuf/libhsa-runtime64); zero frames in any changed function — and those are instrumented, so the negative is meaningful. Running only the pre-existing 11 tests yields 13 of the same class.

Known environment dependency: PoolClientRangesTest.RemoteRoundTrip*, StaleSelfLocationIsExcludedBeforeRemoteFetch, BatchPutWarnTest.StagingFallbackSucceedsWithoutWarn and RegisteredSrcsNoWarn need a working RDMA device and fail identically with and without this change on a host without one.

Relationship to #678

#678 arms these RPCs with a client-side deadline — a bound on an unbounded hang, and explicitly a stopgap ("not a fix for the lock itself"). This PR is that root cause. The two are independent: no file overlap, either can merge first.

The stall was traced to client_mu_ being a per-node lock held exclusively across memory registration and bulk writes; the details are in the commit message of aef415a4.

Validation (independent, at a874b736 — base branch + this fix + the RegisterMemory deadline fix)

  • Built and unit-tested independently of the author:
    test_standalone_shm_ipc and test_pool_client_batch_put both pass except
    the 2 tests with a documented RDMA-device environment dependency
    (StagingFallbackSucceedsWithoutWarn, RegisteredSrcsNoWarn), which fail
    identically with and without this change on a host lacking a working RDMA
    device — not a regression.
  • End-to-end reproduction on crsuse2-m2m-v2-015 and crsuse2-m2m-v2-012
    (Kimi-K3, TP8, DCP8, CONC=24, ARM=umbp), rebuilding the pinned image at
    this commit: the startup-window freeze this fix targets (aiperf's
    returned/in_flight frozen for 70–260+ s right after server ready,
    confirmed via py-spy --native to be blocked on client_mu_) did not
    reproduce. The unfixed image reliably froze in the same window under the
    same recipe; the fixed image kept advancing through it.
  • No sglang-side ("compat") changes required — this fix has no pybind/API
    surface change; both UMBP call sites in sglang-k3 are unaffected.
  • One independent, unrelated finding surfaced by testing at real conc=24 for
    the first time (this fix removes an incidental throttle the client_mu_ bug
    was applying to actual concurrency): an sglang-k3-side mamba-cache
    exhaustion (Can not alloc mamba cache) under sustained long-context
    chunked-prefill load. Out of scope for this PR — not caused by it, no file
    overlap — tracked separately in
    /apps/yutongwu/store/dcp/MAMBA-CACHE-EXHAUSTION-REPORT.md.

Note on the version currently on GitHub: the body ends with
"⚠️ Not yet validated end-to-end. The GPU IPC registration path in
particular has only negative-case unit coverage — e2e validation is in
flight." Delete that line — the validation above supersedes it, and leaving
both in contradicts itself.

The standalone server's client_mu_ is one mutex per NODE -- the server
address is keyed on UMBP_NODE_ID and a bootstrap lock keeps a single
server process, so all 8 TP ranks share it. Two paths held it
exclusively across work that is not short, and every BatchExists on the
node queued behind them:

  - RegisterBackendMemory wrapped the inner RegisterMemory, which ends in
    an RDMA MR pin over the worker's whole host KV pool. Measured in the
    minutes (see RegisterMemoryRpcTimeoutMs's comment: 1187840 MiB in
    599.6 s, plus per-GPU IPC registration each well over a minute).
    Ranks register at staggered times during warmup, which is what made
    the stall a startup-window phenomenon that clears by itself.
  - The put handlers held it until the backend call returned, covering
    the data copy and the BatchRoutePut round trip to master.

The lock could not simply be narrowed because it doubled as the lifetime
barrier for resolved host/GPU mappings: ReleaseRegisteredMemory took it
exclusively, so nothing could munmap a buffer -- or, more importantly,
tear down its RDMA MR, since PoolClient hands out copies of a region's
TransferRef -- underneath a copy in flight.

Replace the barrier with a per-region pin. memory_ holds shared_ptr to an
immutable region plus an atomic count; ResolveRange/ResolveRanges, the
one choke point every data path goes through, pin what they resolve while
still holding memory_mu_, which is what makes the acquisition race-free.
ReleaseRegisteredMemory waits for the count to drain before deregistering
or unmapping, holding neither lock while it waits. Data handlers and
RegisterBackendMemory now take client_mu_ shared; writes reuse the read
path's ConditionalDataLock so the SSD medium keeps serializing exactly as
before, and only DRAM gains concurrency. Clear and shutdown stay
exclusive.

PoolClient had the same shape one layer down: the slow
transfer_engine_->RegisterMemory ran under registered_mem_mutex_, the
lock FindRegisteredMemory takes once per range on every transfer. Move
that exclusion to a new registration_mutex_ held across the engine call
-- that is what IOEngine actually needs, its memPool and backends
carrying no lock of their own -- and leave registered_mem_mutex_ for the
table insert/erase only.

Pinning until the call returns is sufficient because the inner data-plane
calls are synchronous with respect to caller pointers: the remote
submit-then-wait handles are function-local and always waited before
return, with the handle's destructor draining on exceptional exit.

Tests: a multi-region ranged variant of the deregistration race (its
ranges are listed last-region-first on purpose -- a teardown releases
regions in registration order, so a pin covering only the first region
resolved would otherwise be shielded by that ordering), concurrent puts,
registration churn under load, and a bounded-latency exists probe.
Verified by mutation: removing the pin wait segfaults the pre-existing
deregistration test 3 runs out of 3, and pinning only the first region
segfaults the new ranged test 5 out of 5.
…d thread

ConcurrentRegistrationsAndLookupsStayConsistent runs BatchPut on a raw
std::thread as a load generator; its sibling tests
(StagingFallbackSucceedsWithoutWarn, RegisteredSrcsNoWarn) call the same
cross-node BatchPut from the main thread, where gtest's exception
handler turns a throw -- e.g. "no active RDMA device on this host" --
into a graceful test failure. An exception escaping a std::thread has no
such handler and calls std::terminate, taking down the whole binary
instead of just this test.

Wrap the call in try/catch, same "not asserted" posture as the plain
false result this load generator already ignores.
The write-up was an investigation record, not reference material the
repo needs to carry: it is mostly a narrative of how the stall was
found, and the parts worth keeping -- why client_mu_ cannot be the
lifetime barrier, what the pin replaces it with, and what the regression
tests are actually pinning down -- already live in the code comments and
the commit that made the change.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant