refactor(umbp): make distributed mode backend- and transport-agnostic - #540
Open
TianDi101 wants to merge 15 commits into
Open
refactor(umbp): make distributed mode backend- and transport-agnostic#540TianDi101 wants to merge 15 commits into
TianDi101 wants to merge 15 commits into
Conversation
First step of the backend-agnostic refactor (see src/umbp/doc/design-backend-agnostic-refactor.md): cuts SSD out of the distributed PoolClient/PeerServiceServer/routing path so the data plane collapses to one shape (async RDMA page slots) ahead of generalizing it into a MediumBackend interface in later phases. - Delete the SSD read-staging lease coupling: ssd_read_lease.h, PrepareSsdRead/ReleaseSsdLease RPCs (proto tags reserved, not freed), the peer-side read-slot state machine, StagingMetrics, and the SSD metrics block in master_metrics.h. - PoolClient no longer builds a PeerSsdManager/SsdCopyPipeline or an SSD staging buffer; BatchGetPlan collapses from 4 buckets to 2 (remote_groups/local_indices). - PeerServiceServer's constructor drops to (dram_alloc, engine_desc_bytes, master_client). - distributed_client.cpp advertises DRAM-only tier_capacities — nothing serves SSD anymore, so advertising it would route into a dead path. - PeerSsdManager/SsdCopyPipeline move under distributed/peer/ssd/, stay compiled and tested (dormant), and PeerSsdManager drops its OwnedLocationSource base while keeping the same-shaped methods for a future SsdBackend adapter. - Delete the coupling-only tests (test_peer_ssd_read_rpc, test_ssd_read_lease_gating, test_ssd_reliability, test_peer_service — the latter's entire content turned out to be SSD lease RPC tests). Verified: umbp_common/umbp_master/umbp_client/umbp_standalone_server build clean; ctest 35/35 pass, including the local SSD suites and the 3 dormant adapter tests unmodified, plus the real-RDMA integration tests (umbp_cross_node_smoke, umbp_pool_client_batch_put). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…boundary
Adds include/umbp/distributed/peer/medium_backend.h: MediumBackend, its
value types, and BackendRegistry. Nothing implements it yet; the gate is
that the tree compiles.
Also revises the plan doc, which was missing the criterion that decides
where code belongs and so had allocation and registration sitting outside
the abstraction they belong to:
- §1 gains a fourth coupling: memory ownership lives entirely outside the
backend. Allocation is in DistributedClient, registration in
PoolClient::Init with MemoryLocationType::CPU hardcoded, the local copy
is a host memcpy over a buffer table with no tier dimension. Adding HBM
today needs four call-site edits before the registry line, which is what
put Phase 5's acceptance test out of reach.
- §2 (new) states the rule: descriptors are medium-agnostic, raw pointers
are medium-specific, registration is the conversion and belongs to
whoever allocated. This is why the remote path already works against an
HBM peer unmodified, and why the local fast path does not.
- §3 drops byte movement from the interface and records the three things
proposed and rejected (LocalCopyIn/Out, a staging pool, a "not ready"
resolve state) so they are not re-added.
- §4 (new) abstracts the transfer engine inside UMBP, with mori-io as one
unmodified implementation. Registration and transfer share one
TransferEngine type; CompositeTransferEngine fans registration out across
transports. Bounce buffers live here because this is the only layer that
can observe completion.
- §5 splits Phase 2 into map-split plus ownership move, gates real HBM on
the new Phase 6, and keeps the local fast path untouched until then.
- §8 (new) records what none of this fixes: Location{node_id, tier} cannot
describe shared media, so S3/3FS need control-plane work.
Effort: Phase 2 2-3 -> 4-5 days, Phase 6 added at 4-6, SSD re-add drops to
1-1.5 now that the staging pool moved to the transfer layer.
Verified: header is self-contained and warning-clean under the project's
clang++ flags, clang-format clean, umbp_core/umbp_common build unchanged.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
PeerDramAllocator held an internal map<TierType, ...> but served exactly one
live tier: PoolClient::Init default-constructed an empty hbm_cfg, so HBM, its
routing rank and the tier map were unexercised scaffolding. Invert that map —
one backend instance per medium — and relocate the generality to a registry
where it is exercised.
PeerDramAllocator -> PageBackend : MediumBackend
Every method loses its TierType parameter (the tier is now the identity of the
object), which makes the file smaller.
Ownership moves down with it. Allocation, RDMA registration and capacity
advertisement lived outside the "backend", so it was a bookkeeper over memory
it did not own: DistributedClient called HostMemAllocator, PoolClient::Init
registered the buffer with MemoryLocationType::CPU hardcoded, and capacity was
a {DRAM, {size,size}} literal. PageBackend::Init(TransferEngine*) now
self-allocates its pool with its own hugepage/NUMA policy and registers it,
choosing the location type itself. Consequently:
- PoolClientConfig::dram_buffers and ExportableDram are deleted; no buffer
pointer crosses into PoolClientConfig
- BuildDramTierConfig is deleted
- tier capacities are aggregated over BackendRegistry::Capacity() after Init
instead of being passed in. This also drops the old mapped_size-vs-
allocatable-tail discrepancy: capacity is now bitmap-derived, so master's
view and the allocator's agree by construction.
TransferEngine is a ~30-line shim over IOEngine::RegisterMemory for now; Phase 6
replaces it with CompositeTransferEngine and no backend changes, which is why
Init's signature takes it from the start.
MockBackend is registered for HBM so the registry dispatches to more than one
backend. It advertises zero capacity, so routing never selects it and DRAM
behavior is unchanged.
Phase gate: DRAM behavior unchanged with a second backend registered; no buffer
pointer in PoolClientConfig. ctest 36/36, including the local SSD suites and
the 3 dormant SSD adapter tests Phase 0 requires to stay green.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… go agnostic
The peer service held one typed PageBackend* and served every RPC from it, so
it could only ever talk to DRAM. It now holds a BackendRegistry* and dispatches
on the request's tier tag; no concrete backend type is named in peer_service at
all (it no longer even includes page_backend.h). Same for PoolClient's local
paths and MasterClient's heartbeat.
Commit/Abort carry no tier on the wire. Every backend numbers its slots from 1
independently, so a bare slot_id is ambiguous the moment a second medium is
live. umbp_peer.proto documents the id as opaque and echoed back, so the peer
now tags the tier into its high byte and strips it on return: dispatch works
with no proto change and no client change. The tag is peer-local — the local
fast path talks to a backend directly and never sees a tagged id.
Resolve and Evict carry no tier either, and are handled by shape rather than by
assumption: Resolve walks this peer's media and takes the first hit, Evict fans
out to every medium and sums the freed bytes, so a mirrored key is dropped
everywhere instead of only from DRAM.
MediumBackend absorbs OwnedLocationSource, which is deleted along with
AddOwnedLocationSource/owned_sources_ (design doc §3). MasterClient aggregates
capacity, owned-key counts, events, the clear gate and the auto-flush hook over
the registry, so a newly registered backend participates in the heartbeat with
no change there. Its aggregation tests move to test_mock_backend.cpp.
Single-key RPCs are served by one-element batches, so there is no separate
single-key path on the interface to keep in sync.
Two limits stay, deliberately, and are documented where they are decided:
- GetPeerInfo and BatchResolveKeys can express only ONE medium per response
(dram_memory_descs is a flat buffer_index space, dram_page_size a single
field). A second buffer-owning backend is reported, not silently merged
into a colliding index space. The tier dimension is a Phase 6 prerequisite.
- The local fast path still memcpys, so it needs a raw base pointer that
MediumBackend deliberately does not expose; PoolClient keeps one concrete
handle for it, deleted in Phase 6.
Also fixed while here, all three found by review of Phase 2:
- PageBackend::Init leaked RDMA MRs when a later buffer's allocation failed:
the unwind cleared owned_mem_descs_ without deregistering, and owns_memory_
never became true so Shutdown() could not clean up either.
- The local copy loop took the backend's allocator mutex once per page (the
same lock every Allocate/Commit/Resolve and the heartbeat snapshot contend
for). Bases are immutable after Init, so they are snapshotted once.
- MockBackend is no longer registered in the production PoolClient::Init. It
was inert only because nothing dispatched by tier; with Phase 3 an
HBM-tagged request would have reached it and "succeeded" with no pages and
no descs, publishing a key backed by nothing. Registry dispatch is proven
by tests instead.
New test_peer_service_dispatch.cpp drives the real gRPC surface with two
backends registered: per-tier routing, the tier-tagged slot_id round trip,
mixed-tier batch ordering, and the read-walk / evict fan-out.
Phase gate: bench_pool_client_batch_get throughput unchanged (measured against
the last committed state; deltas within run-to-run noise). ctest 37/37.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…rders
Two tier orders were the last medium-specific knowledge in the routing plane:
TierPriorityRouteGetStrategy's HBM > DRAM > SSD, and put's kPutTierOrder =
{HBM, DRAM} with SSD excluded as a non-direct-put target.
Scope decision: delete them, do not replace them. The plan was to re-express
both as an advertised BackendProperties {read_rank, put_eligible}. But every
medium live in the system today is equivalent, so an advertised order would be
scaffolding nothing exercises — the same mistake §1 calls out about
PeerDramAllocator's unexercised tier map. So BackendProperties,
MediumBackend::Properties() and BackendRegistry::ByReadRank() are gone too; the
last was redundant with All(), which already iterates in a deterministic
ascending-TierType order.
TierPriorityRouteGetStrategy -> LocalPreferringRouteGetStrategy
It keeps the requester-local preference — the thing that makes
cache_remote_fetches pay off, since a node that re-cached a block reads its own
copy with no RDMA — and drops the ranking. The old name described only the
half that was deleted. RoutePut now scores every (node, tier) pair that has
room, on free space alone.
EvictionManager's `if (tier == SSD) continue` goes with them. It existed
because EvictKey only ever reached the peer's DRAM allocator, so evicting on an
SSD overload would have dropped the DRAM copy instead; since Phase 3 EvictKey
fans out to every backend by key, so an overloaded medium now evicts from
itself. The guard was a special case justified by a condition that no longer
held.
Four behavior changes, all invisible while DRAM is the only live medium:
- a requester's own replica wins even on a "slower" medium (was: remote
faster tier)
- on a node with HBM 10G / DRAM 400G, a put picks DRAM (was: HBM)
- a tier that is the only one with room is used, including SSD (was:
unroutable)
- under same-node affinity, a spill stays on the anchor node and changes tier
(was: jumped to a remote node's faster tier)
When SSD returns as an SsdBackend it must re-assert put_eligible=false itself:
the router no longer assumes it. Noted in the design doc's re-add path.
Phase gate, revised: the original — "test_tier_priority_route_get and
test_route_put_strategy pass unmodified" — cannot hold, because those tests ARE
the hardcode. test_tier_priority_route_get is replaced by
test_local_preferring_route_get, and 6 assertions across the put/get suites are
inverted; each inverted test names the expectation it replaces so the diff reads
as a deliberate contract change. Everything else passes untouched: ctest 37/37,
and bench_pool_client_batch_get is unchanged within run-to-run noise.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The last medium-specific knowledge outside a backend was PoolClient's byte
moving: it held a raw base pointer per DRAM buffer and memcpy'd through it, so
the local path was host-DRAM-only, PoolClient had to name a concrete backend
type to obtain those pointers, and a second live medium would have needed a tier
branch in the copy loop. There is now exactly one byte-moving path, behind
TransferEngine, and engine selection is a function of the (src, dst) pair.
transfer/transfer_engine.h TransferRef, MemoryRegistrar, PeerDirectory,
TransferItem/Plan/Handle, TransferEngine
transfer/mori_io_engine RDMA; owns mori::io::IOEngine
transfer/local_copy_engine both endpoints local; NT-AVX2 block copy
transfer/composite_* fan-out registration, per-pair dispatch
mori-io is NOT modified, per §6: UMBP owns TransferRef, so mori-io never has to
learn a non-memory endpoint and its other consumers see no blast radius.
Moved out of PoolClient into MoriIoEngine, each because it had to know about the
wire: GroupTransfersByPair (now Plan()), the bounce buffer and its mutex, peer
engine registration, and the peer buffer-descriptor cache that used to live in
PeerConnection. RemoteDramScatterWrite/Read and GroupPagesByBuffer are deleted
outright — dead since the batch path landed, and a second byte-moving path is
what §6 forbids.
MediumBackend::LocalBufferViews() -> BufferRef(buffer_index) + BufferCount()
That is the change that lets the local path fold in. A TransferRef is
medium-agnostic (§2); a raw base pointer is not. ExecuteLocalPut/Get now build
TransferItems and hand them to the same planner as everything else, and
PoolClient drops local_copy_backend_, local_buffers_, CanCopyLocally,
LocalPutPages, LocalGetPages and LocalCopyBlock.
Four deliberate deviations from §4, each argued in the design doc:
- TransferRef is a struct of merged per-transport handles, not a variant.
Registration fans OUT — the same buffer is a raw pointer AND an RDMA MR at
once — and a variant makes the local path inexpressible. mori::io::
MemoryDesc already resolves this the same way (ipcHandle beside
fabricHandle).
- no FileRef/ObjectRef and no RegisterFile. No engine consumes one yet, and
§3 already refused an abstraction nothing exercises once (BackendProperties).
SsdBackend brings its own, as a one-file change to this header.
- Wait lives on the handle, not the engine: the RDMA backend holds raw
TransferStatus* into the handle's status vector, so the drain-on-early-
destroy safety net has to sit with the statuses.
- the schedulers stay in PoolClient. Submit-all-then-wait spans several
Submit calls with local reads in the gap, and UMBP_DRAM_{READ,WRITE}_THREADS
parallelize across the keys of a batch — a batch is not a concept the engine
has. What is in the base class is Transfer(), for callers with no overlap.
Phase 5 Rules A and C close here, as §5 predicted, both as types rather than
lint. MemoryRegistrar (register/deregister) and TransferEngine : MemoryRegistrar
(+ CanHandle/Plan/Submit) mean "a backend must not move bytes" is a compile
error; deleting LocalBufferViews() removed the last concrete-typed call, so
PoolClient::Init builds through MakePageBackend() -> unique_ptr<MediumBackend>.
Reaching mori-io's peer handshake needed a MoriIoEngine* at first; those six
calls became the PeerDirectory interface, so a second remote transport
implements an interface instead of editing pool_client.cpp. PoolClient::Init is
now the only file naming any concrete backend or engine. Rule B's lint tool is
still outstanding — it is not in tree.
Two behavior changes, both improvements, both asserted by tests:
- a batch mixing registered and unregistered caller buffers works (was: a
contract violation, failed wholesale)
- a staged batch larger than the bounce buffer is chunked into pool-sized
round trips (was: the whole peer batch failed)
Both fall out of moving staging into the engine. The old code reserved the
batch's staging up front and held one mutex from submit to wait, so a submit-all
over several staging peers would deadlock — hence permit_staging, the all-zc-or-
all-staging contract, and the two-armed fork in ExecuteBatch{Put,Get}Plan. All
gone: a plan needing the pool completes INSIDE Submit, so the lock is never held
across a return. test_cross_node_smoke's PutStagingOverflowFailsBatchCleanly
asserted the old behavior and is replaced by
PutStagingLargerThanPoolIsChunkedNotFailed plus
PutPageLargerThanStagingPoolFailsBatchCleanly — the failure that is still a
failure is a single page larger than the entire pool, which cannot be chunked.
Fixes a pre-existing correctness bug found while restructuring the path:
ExecuteLocalGet returned kSuccess for a key no local medium held, reporting a
HIT with an untouched dst. Reachable — PartitionBatchGetTargets sends a key
master has no route for down the local path as a fallback — and the caller
cannot tell the difference, so it hands stale bytes to its own caller. Now
returns kRetry, plus a resolved.size != size guard matching the remote path.
Layout: §3's three components are three directories, so the boundary is visible
in the tree and the Phase 5 lint rules can be scoped by directory rather than by
filename list. distributed/transfer/ is a SIBLING of peer/, not a child: the
include graph says so, since peer_service — the reason peer/ exists — references
the transfer layer zero times (a peer hands out descriptors; the initiator moves
the bytes). transfer/ depends on nothing but types.h while backend/ and
pool_client depend on it, so nesting the lowest layer inside a higher-level
sibling would invert the layering, and LocalCopyEngine settles it from the other
side — a memcpy between two of this node's own buffers has no peer in it.
backend/ does belong under peer/: PeerServiceServer dispatches every
Allocate/Commit/Resolve/Evict into BackendRegistry, and capacity, eviction and
the heartbeat outbox are statements about what this node holds for the cluster.
Also deletes distributed/pool_allocator.h: the pre-page-model byte-offset
allocator, superseded by PageBitmapAllocator and with no includer since 158c7e8.
New test_transfer_engine.cpp covers the planner (grouping, coalescing, bounds
rejection) and composite selection without gRPC or RDMA; MoriIoEngine stays
covered end-to-end by test_cross_node_smoke. ctest 38/38.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
- Delete src/umbp/include/umbp/common/error_code.h — never included anywhere in the repo. - Delete src/umbp/include/umbp/distributed/obs_counters.h — its macros (MORI_UMBP_OBS_INC/ADD/TEST_VIRTUAL) are never invoked. - Remove MasterClient::SetPeerSsdManager and the ssd_manager_ field — Phase 0 scaffolding for SSD capacity reporting that was never wired up; SsdBackend now reports capacity through the standard MediumBackend/BackendRegistry path in SnapshotAndCacheTierCapacities, making this dead. - Collapse the duplicate ctest target in tests/cpp/umbp/local/CMakeLists.txt: test_umbp_local_eviction and test_prefix_aware_eviction both built from the same source file and ran the same tests under two names; keep the latter, whose name matches the source file per this CMakeLists' own convention. Verified via full umbp_core/umbp_common rebuild + ctest (39/39 passing, was 40/40 before collapsing the duplicate target) inside the umbp-refactor-ditian12 dev container.
* feat(umbp): HBM + SSD backends, HbmCopyEngine, and the two how-to skills
Exercises the Phase 1-6 backend/transfer abstraction by actually adding a
medium and an engine through it, and writes down the resulting recipe. Two
findings came out of doing it rather than describing it.
FINDING 1: the interface was right, its one implementation was not.
MediumBackend was already medium-agnostic, and PageBackend's class comment
already claimed to serve "DRAM or HBM" — truthfully for the slot lifecycle,
bitmap allocator, event outbox, reaper, read leases and copy pins, none of
which consult the tier. But Init/Shutdown/BuildBufferRefs hardcoded
HostMemAllocator, MemoryLocationType::CPU and device=-1, so the claim did not
survive contact with a second medium.
PageMemorySource Allocate/Release/LocationType/Device/Name
Five methods holding exactly what differs per medium. HostPageMemorySource is
the old inline body, moved; the OwnershipConfig constructor now funnels into
the PageMemorySource one, so DRAM and HBM share ONE Init path rather than two
kept in step. LocationType/Device are the load-bearing pair: PageBackend
mirrors them into every published TransferRef, and that is what selects the
engine. Getting them wrong fails at neither build nor init.
So HbmPageMemorySource IS the HBM backend — no HbmBackend type, ~100 lines,
zero duplication of the 800 that already worked.
FINDING 2: HBM's local path was unservable by any engine in tree.
LocalCopyEngine requires BOTH endpoints loc == CPU
MoriIoEngine `if (src_remote == dst_remote) return false;`
A both-local pair with a GPU endpoint was claimed by nobody, so an HBM
backend's local Put/Get could not complete at all. The REMOTE side already
worked unmodified, exactly as §2 predicted ("why the remote path already works
against an HBM peer, and why the local fast path does not") — this closes the
local half.
HbmCopyEngine H2D + D2H + D2D, hipMemcpy, ~200 lines
No new TransferRef field needed: host_ptr is documented as the "process-local
view", and for hipMalloc'd memory the device pointer IS that view. The three
engines now partition (src, dst) with no overlap, so composite order stays
documentation rather than a tie-break — asserted, not assumed. Synchronous by
choice: the parallelism that matters is across keys and already exists in
PoolClient's executors, and a shared stream would need its own synchronization
for no win at KV-block sizes. Restores the caller's current device, since
Submit runs on threads that are not ours to leave re-pointed.
SSD: staged, and deliberately NOT a FileRef.
transfer_engine.h reserves a file endpoint for SsdBackend. Not taken. SSD bytes
are not addressable, and closing that gap in the transfer layer means a new
TransferRef kind, a PosixFileEngine, AND chaining in CompositeTransferEngine
for the remote reader — which that class explicitly does not implement. Instead
SsdBackend publishes ordinary registered host DRAM and spills behind it:
Allocate lends a staging page, Commit spills it to PeerSsdManager and returns
the page, Resolve fills a page under a read lease and the reaper reclaims it.
Cost is one host copy per side; benefit is SSD reaching the data plane with
ZERO transfer-layer change, and a remote peer needing no code at all. A FileRef
backend stays the right answer for GDS; this does not block it.
PeerSsdManager needed one accessor (SizeOf) — PrepareRead picks its staging
buffer before the read, so the reader must know the size while holding only the
key. Its header anticipated this adapter; Phase 0's dormancy ends here.
Two honest limits, both pinned by tests so a fix changes them deliberately:
- one key, one page. PrepareRead takes a single (ptr, cap), not a scatter
list. Fine while master's page_size IS the KV block size.
- staging exhaustion degrades a Get to found=false, which makes the client
retry another peer for a key this node does hold. This is the "not ready,
retry here" state medium_backend.h records as proposed and rejected.
Skills, written from what the work actually required:
.claude/skills/umbp-add-backend two shapes: 5-method
PageMemorySource, or full
MediumBackend + staging
.claude/skills/umbp-add-transfer-engine pair dispatch, disjointness,
Plan/Submit/Wait, PeerDirectory
Both carry the contracts with no compile-time protection (one event bundle per
seq, full-sync clearing the outbox in the same critical section, kFailedNoSpace
vs kFailed, Evict's positional results) and the build's real constraint: umbp
needs protoc/grpc_cpp_plugin from the mori image, not the bare host.
Verified: ctest 38/38 (36 pre-existing + 2 new), including test_page_backend
and test_transfer_engine unchanged. test_hbm_backend runs real H2D/D2H/D2D and
a Put/Get round trip on an MI355X; test_ssd_backend runs 21 cases against a
real posix SSD tier. clang-format clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat(umbp-bench): --tier/--dst-loc flags and HBM wiring through DistributedClient
Migrated from the worktree-umbp-bench-tier branch (commit b634ae4a plus the
uncommitted work that sat on top of it).
- UMBPHbmConfig (enabled/device/capacity_bytes) on UMBPDistributedConfig,
lowered by ToPoolClientConfig into PoolClientConfig::hbm, so PoolClient::Init
registers an HBM PageBackend. Unlike dram/ssd there is no ownership struct to
thread through: hipMalloc has no hugepage/NUMA/prefault dimension.
- IUMBPClient::RegisterMemory grows loc/device describing the CALLER's
allocation, so a GPU-resident src/dst routes through HbmCopyEngine instead of
being assumed host memory. StandaloneProcessClient rejects non-CPU rather
than silently mis-registering.
- umbp_bench.py: --tier {dram,hbm,ssd}, --dst-loc {host,gpu}, --hbm-device.
DeviceBuffer allocates the read destination via raw HIP (ctypes on
libamdhip64) so --tier hbm --dst-loc gpu exercises the D2D hipMemcpy path
without pulling in a torch dependency.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat(umbp): per-backend buffer indices (backend_id) for multi-medium peers
Migrated from the uncommitted work-in-progress on feat/umbp-hbm-ssd-backends
in the main checkout, so HBM/SSD can actually be benchmarked: PoolClient::Init
always registers DRAM, so --tier hbm/ssd produces a two-backend node, which is
exactly the case this fixes.
buffer_index is backend-local, but EnsurePeerServiceConnection folded every
backend's buffers into one flat dram_memory_descs list. One medium's buffers
were published, the rest unreachable -- yet HasRemoteBuffers still went true,
so the next resolve asked the peer to omit descriptors and the missing media's
pages were read against the published medium's memory. Now each desc names its
backend, and Build{Put,Get}Transfers snapshot per backend the batch touches
rather than once per peer.
pool_client.{h,cpp} carried changes from both this work and the bench-tier
migration; both are preserved (SlotPlan::backend_id plus the loc/device
RegisterMemory signature).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat(umbp): wire the SSD tier through DistributedClient, and stop --tier lying
SsdBackend was already registered by PoolClient::Init from
PoolClientConfig::ssd -- the only thing missing was DistributedClient lowering
UMBPConfig::ssd into it, so the tier stayed dark however it was configured.
The bench's "SSD is unwired" refusal was describing the old PeerSsdManager
world and is now wrong, so it is gone.
Opt-in is UMBPDistributedConfig::enable_ssd_tier, deliberately NOT
UMBPConfig::ssd.enabled -- that defaults to true, so keying off it would make
every existing distributed deployment start advertising SSD capacity.
Also fixes the benchmark measuring the wrong medium: DRAM is always
registered, and routing has no tier order (kMostAvailable picks whichever
medium has the most free bytes), so --tier hbm against the 8 GiB DRAM default
and a 4 GiB HBM pool routed every put to DRAM and labelled it HBM. Non-DRAM
tiers now shrink DRAM to a 256 MiB floor so most-available can only pick the
requested medium.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(umbp-bench): size the SSD staging arena, and let tier runs disable re-cache
Two things that made a tier benchmark measure something other than the tier.
SSD reads stage through a registered host arena of ssd_staging_buffer_slots
pages; the default 16 caps read concurrency at 16 keys, and a slot stays
pinned for UMBP_SSD_READ_LEASE_MS when the reader's best-effort
ReleaseSsdLease is lost. A 1024-key sweep therefore returned hits=17
misses=1007 -- the overflow degrades to MISS, not to a slow hit, so it reads
as a correctness failure rather than a capacity limit. ssd_backend.cpp says it
directly: "Sizing the arena for read concurrency is the mitigation." Slots and
arena bytes are now env-tunable and default to 512 / 2 GiB, which takes the
same sweep to 1024 hits / 0 misses / 0 mismatches.
cache_remote_fetches defaults on, and the reader gets its own pool of the tier
under test, so after pass 1 most reads were served from the reader's own
memory instead of RDMA out of the writer's tier. UMBP_CACHE_REMOTE_FETCHES=0
pins every pass to a genuine remote read; the default keeps production
behavior.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…g::medium (#543) PoolClient::Init registered DRAM unconditionally and then added HBM/SSD beside it, so `--tier hbm` or `--tier ssd` produced a two-backend node. That reads like a tier stack and is not one: since Phase 4 the master treats every advertised tier as an equally valid put target, so the second medium is mirrored into by free capacity rather than sitting behind the first. A node's keys split across two pools for no gain, and a tier benchmark ends up measuring DRAM. Replace the per-medium `enabled` flags with a single selector. A node picks exactly one of DRAM / HBM / SSD; heterogeneity comes from different nodes picking differently, which the routing plane already handles. Adding a medium is now a `case` in one switch, not another `if`. - UMBPDistributedConfig::medium (new UMBPMedium enum) replaces enable_ssd_tier and UMBPHbmConfig::enabled. Defaults to DRAM, so an existing deployment is bit-identical. - UMBPConfig::Validate checks only the selected medium's sizing; the unselected blocks are ignored, not validated, so one deployment template can carry all three. - PoolClient caches the live tier as medium_ and exposes Medium(). Also removes the two DRAM literals the design doc listed as outstanding: - the re-cache installer hardcoded TierType::DRAM, which would have installed into a tier an HBM/SSD node does not have; - PartitionBatchPutTargets filtered remote routes to DRAM/HBM, silently dropping every put master routed to a peer's SSD even though SSD publishes ordinary registered staging pages. New test CrossNodeBatchPutLandsOnSsdPeer covers exactly this. Tests: 20/20 umbp ctest targets pass (incl. the new test_umbp_medium_selection: lowering, per-medium validation, one-backend registry, and the cross-node SSD put/get). Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…izes nothing The DistributedClient init line printed "staging=<ssd_staging_buffer_size>MB/<slots>slots", but ssd_staging_buffer_size does not size the SsdBackend arena — that is staging_pages * page_size. A run with 2048 slots and a 2 MiB page logged "staging=6144MB" against a real 4096 MiB arena, i.e. it advertised capacity the node did not have. SsdBackend's own "[SsdBackend] Init ... arena_bytes=" line is authoritative and already correct. Print the slot count alone, since that is the number an operator needs: it is the SSD medium's read-concurrency limit, and a BatchGet wider than it degrades to MISSES rather than backpressure (ssd_backend.h reports staging exhaustion as found=false). Observed on n06-21: a 2048-key BatchGet against a 2048-slot arena all-missed in 4ms, and a 128-key sweep missed 49%, while batch<=32 was a clean 0-miss ~190 MiB/s. ssd_staging_buffer_size is now referenced only by the pre-refactor PeerSsdManager path; leaving it in place, but it should not appear in a line describing the live SSD backend. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
CORRECTION to the previous commit's message: it blamed the SSD misses on
staging-arena exhaustion. That was wrong. The arena logs a warning on every
page-acquisition failure and the failing run had zero of them; reruns at the
same scale, the same batch width, and the same 2048-slot override all passed
with 0 misses. (The log-line fix itself stands — the printed number really was
wrong.)
The actual cause is here: _sync_recv_until defaulted to a flat 600s wait for
the reader's DONE. A 4 GiB SSD sweep reads at ~190 MiB/s and takes ~14 minutes,
so the writer gave up mid-run, and its exit killed the umbp_master it spawned
(PR_SET_PDEATHSIG) and unregistered its keys. The still-running reader then
missed every subsequent read. Timeline from the failing run: WRITER_READY at
07:51:34, writer gone at 08:01:34 — exactly 600s — and the reader's misses
began in the leg covering that moment. DRAM and HBM finish in seconds and never
reached it, which is why only SSD "failed".
The failure was invisible from both ends: the writer logged no error and never
printed READER_DONE, and the reader reported it only as hits=0 in a RESULT
line. So this does two things:
- The reader sends a keepalive every 30s for the whole read phase, so the
writer's wait is an IDLE timeout rather than a run-duration budget. No
number to tune per medium: the reader proves liveness however slow it is.
Sends are serialized with the final DONE under a lock, since interleaved
sendall() from two threads could split the token.
- A timeout now raises with a specific message naming the idle window,
instead of dying into a wall of unexplained misses.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
segment::CrcUpdate was a bitwise, table-less CRC-32/ISO-HDLC -- 8 shift/XOR per byte, ~152 MB/s on one core -- run over every byte of every record on both the read and the write path. On the SSD tier it dominated everything else: the checksum, not the drive, set the tier's ceiling. Two changes, both confined to the segment layer: * CRC-32C via the SSE4.2 crc32 instruction, with __builtin_cpu_supports runtime dispatch and a byte-identical portable table fallback. The fallback matters for correctness, not just portability: a segment written on a host with hardware support must still verify on one without, so both paths are pinned to the same reflected polynomial (0x82F63B78) and tested against an independent bitwise reference. ~152 MB/s -> ~13.4 GB/s. No -msse4.2 on the translation unit; the dispatch is per-call and predicted. * Split segment::Writer::Prepare into Build (checksum + record assembly) and Reserve (index reservation). SSDTier now runs Build outside mu_ and only Reserve under it, so a write batch no longer blocks concurrent reads on the same drive for the whole of its CRC + copy time. Prepare is kept as Build+Reserve for callers already holding the lock. Build leaves `generation` zero and Reserve patches it in place, since it is the one header field that does not exist until the reservation is taken. kRecordVersion 1 -> 2: the polynomial changed, so v1 checksums must not be verified with the v2 routine. The scanner's existing version check drops them and the segment refills -- this tier is a cache, so its contents are always re-fetchable. Tests: new test_segment_crc pins the standard CRC-32C check value 0xE3069283 (which also catches an accidental revert to ISO-HDLC, whose check value is 0xCBF43926), verifies the selected path against an independent bitwise reference at every tail length mod 8 and at unaligned starts, checks streaming composition, and covers the Build/Reserve generation stamping. test_ssd_tier passes unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…tic SSD path (#554) - Add multi-drive SSD backend
- add arena auto sizing for ssd backend benchmark
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Makes UMBP's distributed mode backend- and transport-agnostic, so adding a storage medium or a byte-moving path is a registration rather than an edit spread across the data plane.
Before this, four couplings meant a second medium could not be added without touching unrelated code: the peer allocator carried an internal
map<TierType, ...>, the routing plane hardcoded two tier orders,PoolClientmemcpy'd through raw per-buffer base pointers (so the local path was host-DRAM-only), and memory ownership lived entirely outside the "backend" — allocation inDistributedClient, registration inPoolClient::InitwithMemoryLocationType::CPUhardcoded.The two interfaces
MediumBackend— a backend OWNS bytes and PUBLISHES DESCRIPTORS for them. It does not move them. One instance == one medium, so no method takes aTierType; the tier is the identity of the object andBackendRegistrydispatches on it.TransferEngine— there is exactly one byte-moving path and it is here. Engine selection is a function of the(src, dst)PAIR, never of either endpoint alone.The rule connecting them: descriptors are medium-agnostic, raw pointers are medium-specific, and registration is the one-way conversion — which belongs to whoever allocated. That is why the remote path already worked against an HBM peer unmodified while the local fast path did not.
Phases
MediumBackendinterface + the backend/transfer boundaryPhase 5 has no commit of its own: it was the lint rules, and two of the three closed as types rather than lint in Phase 6 —
MediumBackend::Initreceives aMemoryRegistrar(which has noSubmit), so "a backend must not move bytes" is a compile error, and deletingLocalBufferViews()removed the last concrete-typed call soPoolClient::Initbuilds throughMakePageBackend() -> unique_ptr<MediumBackend>. Rule B's lint tool is still outstanding and is not in tree.Transfer layer (Phase 6)
mori-io is NOT modified. UMBP owns
TransferRef, so mori-io never has to learn a non-memory endpoint and its other consumers see no blast radius.transfer/transfer_engine.h—TransferRef,MemoryRegistrar,PeerDirectory,TransferItem/Plan/Handletransfer/mori_io_engine— RDMA; ownsmori::io::IOEnginetransfer/local_copy_engine— both endpoints local; NT-AVX2 block copytransfer/composite_*— fan-out registration, per-pair dispatchFour deliberate deviations from the design doc's §4 sketch, each argued in the doc:
TransferRefis a struct of merged per-transport handles rather than a variant (registration fans out — the same buffer is a raw pointer AND an RDMA MR at once, and a variant makes the local path inexpressible); noFileRef/RegisterFileuntil a backend consumes one;Waitlives on the handle, not the engine (the RDMA backend holds rawTransferStatus*into the handle's status vector); and the schedulers stay inPoolClient.Behavior changes
Two improvements, both asserted by tests:
Both fall out of moving staging into the engine — a plan needing the pool completes inside
Submit, so the lock is never held across a return, which removespermit_stagingand the all-zc-or-all-staging contract entirely.Also fixes a pre-existing correctness bug found while restructuring:
ExecuteLocalGetreturnedkSuccessfor a key no local medium held, reporting a HIT with an untouched dst. Reachable, and the caller could not tell the difference, so it handed stale bytes upward. Now returnskRetry.Verified
ctest36/36 with-E '^cco_' -LE integration. Newtest_transfer_engine.cppcovers the planner and composite selection without gRPC or RDMA;MoriIoEnginestays covered end-to-end bytest_cross_node_smoke, which isintegration-labeled and needs a real fabric, so it has not been run here.Design doc:
src/umbp/doc/design-backend-agnostic-refactor.md— §2 the descriptor/pointer rule, §3 the three-component split, §4 the transfer engine, §8 what none of this fixes (Location{node_id, tier}cannot describe shared media, so S3/3FS need control-plane work).Follow-up
A branch on top of this (
feat/umbp-hbm-ssd-backends) exercises the abstraction by adding an HBM backend, an SSD backend and anHbmCopyEnginethrough it, and is held for a separate PR.🤖 Generated with Claude Code