Feature/shm payload transport - #36
Merged
Merged
Conversation
Payloads at or above 64 KiB no longer travel inside the UDS datagram. The publisher writes the serialized bytes once into a lazily-created per-publisher /dev/shm ring and fans out a 32-byte descriptor (the WireHeader msg_type gains a high SHM_PAYLOAD_FLAG bit); subscribers map the ring read-only on first use and copy the payload out under a per-record seqlock — the same odd/even protocol the discovery registry uses — so a publisher that laps the ring is a detected, clean drop, never a torn message. Small messages, TRANSIENT_LOCAL topics (their replay cache must outlive the ring) and service traffic keep the inline datagram path, which is also the fallback when shared memory is unavailable. What this buys: - The payload crosses the kernel zero times: one memcpy into the ring, one out per subscriber (1+N copies for N subscribers vs 2N kernel copies inline), no syscall proportional to the message size. - The net.core.wmem_max per-datagram cap (EMSGSIZE at ~208 KiB stock) no longer applies to large topic messages; the sysctl requirement stays only for the inline path. The ring holds at least 4 records of the largest payload seen (min 8 MiB) and is recreated larger under a new segment generation when outgrown; the old segment is unlinked, and readers that already mapped it finish safely (unlink removes the name, not the mapping). Hardening that came out of review: - Segment names mix in a time component (the make_socket_path idiom): a recycled PID must not regenerate a dead publisher's name, or a subscriber's cached mapping would alias the new ring. - The ring header is validated once, overflow-safe, at map time and its capacity snapshotted; descriptors are bounds-checked against the snapshot, so neither a hostile descriptor nor a rewritten live header can push reads outside the mapping. Segments are 0644 — single writer, world readable. - The seqlock reader re-checks seq behind an acquire fence so the payload loads cannot sink past the check on weakly-ordered CPUs. - posix_fallocate reserves the ring pages up front: a full /dev/shm becomes a clean inline fallback instead of SIGBUS mid-memcpy. - The reader cache sweeps mappings of vanished segments once it grows past a handful of publishers, so publisher churn cannot pin unbounded tmpfs memory in long-lived subscribers. - Orphan segments from ungraceful exits are reclaimed at rmw_init with the same dead-PID sweep as the registry and socket files. Measured (perf_test ROS2 plugin, 1:1, 100 Hz, RELIABLE, KEEP_LAST/16, quiet host): Array64k 237-273 -> 219-228 us, Array256k 503-530 -> 348-356 us (-32%), Array1m 1447-1512 -> 955-1012 us (-33%), Array4m 3669-3957 -> 3090-3155 us, with max latency roughly halved across sizes and 100% delivery throughout. 91 tests pass: 8 shm_transport unit tests (round-trip, ring lap, segment growth, writer-close semantics, hostile descriptors, orphan cleanup), an end-to-end 300 KB pub/sub round-trip, a pin that TRANSIENT_LOCAL large messages never touch the ring, and a pin that large payloads bypass a shrunken send buffer where they used to die with EMSGSIZE (EMSGSIZE propagation itself is still pinned below the threshold).
Large TRANSIENT_LOCAL (latched) payloads were always sent inline: the TL branch in rmw_publish returns before the shm ring fork, so a >~4 MB latched message hit the kernel's single-datagram cap (EMSGSIZE / ENOBUFS) and was silently dropped — a late joiner received nothing. The ring could not be reused for TL because it cycles and would lap a record still awaited by a late joiner. Add a dedicated, immutable, per-cache-entry durable shm segment (shm_stage_durable + DurableShmSegment RAII handle). A large TL payload is staged once into its own segment; the cache entry holds the descriptor (SHM_PAYLOAD_FLAG set) and owns the segment, which is unlinked when the entry is evicted. Replay to late joiners fans out the small descriptor, and the existing shm_fetch_payload reader path resolves it unchanged. Small payloads still cache inline; the cycling ring is never used for TL. Reader side is untouched: durable segments share the ring's on-disk layout, each gets a unique owner id (so the reader's per-owner stale-mapping sweep keeps them), and orphan cleanup already covers the shared name prefix. Tests: add TransientLocalHugeMessageLateJoiner (5 MB latched -> late joiner, fails on the old inline path, passes here) and repurpose the former TransientLocalLargeMessageStaysInline into TransientLocalLargeMessageUsesDurableShm to pin the new invariant (durable segment used, ring untouched, replay works). Known gap (unchanged, pre-existing): rmw_publish_serialized_message does not feed the TL replay cache, so large latched messages published via that entry point are still bounded by the datagram cap.
Follow-up to the durable-shm change: the authoritative DESIGN.md and three in-code comments still described the just-fixed behavior (TL always inline, large messages bounded by the datagram cap) as current. Correct them: - DESIGN.md: the per-message-cap section and "Large payloads" now state that large TRANSIENT_LOCAL payloads published via rmw_publish go through a durable per-cache-entry shm segment, and add a "Durable segments for latched replay" subsection describing the mechanism, lifetime, orphan reclaim, and the one behavioural divergence (publisher destroyed before drain -> lost). The cap still applies to large service payloads, large TL via rmw_publish_serialized_message (does not feed the replay cache), and the shm-failure inline fallback — spelled out rather than blanket-claimed. - rmw_publisher.cpp: the "always sends inline" parenthetical contradicted the durable-staging branch it sits next to; corrected. - shm_transport.cpp: the map_segment "same as inline exiting publisher" comment now notes the durable-TL divergence. - shm_transport.hpp: DurableShmSegment is non-copyable and pinned (user dtor suppresses moves); the no-double-free guarantee comes from the owning unique_ptr, not the type being movable. Comment corrected. No behavior change. Build clean, all 92 tests still pass.
Addresses the review findings on the durable-shm TRANSIENT_LOCAL change: - Return code (finding #1): the TL publish path ignored send_to's result and always returned RMW_RET_OK, even when the live send of the current message was rejected by the kernel size cap (EMSGSIZE) — a lying success, and inconsistent with the non-TL path. Extract the TL cache+replay+send logic into transient_local_publish(), which now surfaces ConfigError as RMW_RET_ERROR. (ENOBUFS/oversize on a raised-buffer machine stays a soft drop in BOTH paths, unchanged — a shared, pre-existing limitation.) - Serialized TL gap (completeness): rmw_publish_serialized_message did not feed the replay cache at all, so a large latched payload published through it was sent inline once and dropped for late joiners. It now routes TL through the same transient_local_publish() helper (durable staging + replay) and gained the same known-subscriber prune-on-refresh as rmw_publish. - Fallback coverage (finding #6): add a cold-path test seam (env var RMW_UDS_TEST_FORCE_SHM_FAILURE, checked only on large latched staging) so a test can force the shm-unavailable path and assert the payload is cached inline (no durable segment, no flag) and still delivered. Tests: TransientLocalPublishReturnsErrorOnEMSGSIZE and TransientLocalSerializedLargeMessageLateJoiner both confirmed to FAIL on the pre-change publisher and pass here; TransientLocalLargeMessageInlineFallbackWhenShmUnavailable covers the fallback. Full suite: 95 tests, clean build.
The previous doc commit described large TRANSIENT_LOCAL via rmw_publish_serialized_message as a known gap (still inline, still capped). The follow-up commit closed that gap by routing both publish entry points through the shared transient_local_publish path, so update the four DESIGN.md statements accordingly: large latched payloads go through a durable shm segment whether published via rmw_publish or rmw_publish_serialized_message. The only latched payloads still bound by the datagram cap are those that fall back to inline because shm staging failed.
From the final adversarial PR review (verdict: ready-with-nits, no blocker): - Test the serialized-path known-subscriber prune. rmw_publish_serialized_message carries its own copy of the prune-on-refresh logic, previously unexercised (KnownSubscriberPathsPrunedOnChurn drives rmw_publish only). Add TransientLocalSerializedKnownSubscriberPathsPrunedOnChurn as a divergence guard. - Harden the test seam: match RMW_UDS_TEST_FORCE_SHM_FAILURE only when it is exactly "1" (strcmp) rather than any-value presence, so a stray "0"/"" export cannot silently force the inline fallback. - Pin the inline-path assumption of TransientLocalPublishReturnsErrorOnEMSGSIZE with static_assert(32 KiB < SHM_PAYLOAD_THRESHOLD), matching its volatile sibling PublishReturnsErrorOnEMSGSIZE instead of a prose-only comment. Full suite: 96 tests, clean build.
Services and clients sent requests/responses inline, so a >~4 MB request or response hit the AF_UNIX datagram cap and was dropped. Extend the ephemeral shm ring to the service path (non-latched, so the cycling ring — not a durable segment — is the right fit): - UdsService/UdsClient each gain a shm_ring (outbound) + shm_cache (inbound) + shm_mutex, mirroring the publisher/subscription pair. - rmw_send_request / rmw_send_response stage payloads >= SHM_PAYLOAD_THRESHOLD into the ring and send a descriptor with SHM_PAYLOAD_FLAG; inline fallback on shm failure. rmw_take_request / rmw_take_response mask the flag in the msg_type filter and resolve the descriptor via shm_fetch_payload. - Destroy paths close the ring and reader cache. The wire format already reserved SHM_PAYLOAD_FLAG as the msg_type high bit and the receive checks already mask it, so no wire/layout change. Test: LargeRequestAndResponseViaShm round-trips a 5 MB request and 5 MB response (both above the datagram cap, so success proves the ring carried them); confirmed to fail on the pre-change service/client. Full suite: 97 tests.
… helpers The "stage into ring + set flag, else send inline" and "resolve descriptor or drop" logic was inline-duplicated across 8 sites (topic publish x2, service response, client request; subscription drain, wait drain, service take, client take). Extract two helpers in the transport layer: OutboundPayload shm_prepare_send(ring, mtx, domain, payload, size, hdr, desc) bool shm_resolve_incoming(cache, domain, hdr, payload) Every sender and receiver now calls one of these, so each site is ~4 lines and the inline-vs-shm decision lives in exactly one place. Also fixes a latent bug this unification exposed: rmw_wait's drain_socket resolved shm only for subscriptions (services/clients passed a null cache), so a large service request/response drained during wait — the path real executors always take — had its descriptor dropped and the message lost. drain_socket now takes the reader cache by reference and every caller passes its own, so services and clients resolve large payloads on the wait path too. test_transport gains shm_transport.cpp (transport.cpp now calls into it). New test LargeRequestDeliveredThroughWaitDrain covers the wait-path fix. Full suite: 98 tests, clean build, no behavior change beyond the bug fix.
… indent From the correctness review of the service-shm change: - DESIGN.md: the service-shm commit made three statements false — "all service traffic" is inline (L165), "the cap still bites large service payloads" (L187), and "Service traffic never uses shared memory" (L200). Correct all three: large (>= SHM_PAYLOAD_THRESHOLD) service requests/responses now ride the ephemeral ring; only sub-threshold traffic and the shm-failure fallback send inline. Note the best-effort ring-lapping drop this introduces even under RELIABLE service QoS. - rmw_wait.cpp: the two post-epoll service/client drain_socket continuation lines were indented call+0 instead of call+2 like every sibling site; align to match. No behavior change. Build clean, 98 tests pass.
…ader cache
Three measured wins from the performance/scaling review (all correctness-verified;
98 tests still pass):
1. transient_local_publish: stage the durable segment BEFORE taking cache_mutex,
and destruct trimmed entries after releasing it. shm_stage_durable does
shm_open+posix_fallocate+mmap (~120us at 256 KiB to ~2.3 ms at 4 MiB) and touches
no cache_mutex-guarded state, so holding the lock across it was a regression this
feature introduced: cache_mutex nests under the process-wide
transient_local_pubs_mutex on the rmw_wait replay path, so one large latched
publish could stall whole-process graph replay/publisher lifecycle during launch
bursts. Now the lock only covers push_back + trim + the send loops. Cache order is
unchanged (push_back still under the lock, in lock-acquisition order).
2. SHM_RING_MIN_BYTES 8 MiB -> 1 MiB. posix_fallocate commits the whole ring's RAM
up front, and this PR grew the payers from P publishers to P+S+C senders (services
and clients now have rings). The 4-records-of-largest invariant is enforced by the
independent max(floor, 4*record) term, so the floor only sets the minimum for
sub-256 KiB payloads (1 MiB still holds 16 records at the 64 KiB threshold). ~8x
less resident /dev/shm per large-sender and ~8x faster first-publish create.
3. ShmReaderCache keyed by a 16-byte POD {owner_pid, owner_id, segment_id} instead of
the segment-path string. domain_id is constant per cache, so those three wire
fields uniquely identify a segment. Removes a per-received-large-message string
rebuild (~117 ns) + hash + heap alloc on the receive hot path, and simplifies the
generation-supersede sweep to integer compares. The name is built only on a miss
and kept in Mapping.shm_name for the liveness probe. No wire/ABI change.
Also documents the durable create/destroy-per-publish cost as a known characteristic
(latched data is low-rate by nature; high-rate large streams should use VOLATILE).
Six accuracy fixes, no behavior change: - DESIGN.md ring paragraph still said "(minimum 8 MiB)" after e451e29 lowered SHM_RING_MIN_BYTES to 1 MiB; now matches the code and explains why the floor is modest. - DESIGN.md msg_type bullet said SHM_PAYLOAD_FLAG is "only ever combined with 0" — false since e6de9a6: services and clients set it on msg_type 1 and 2. Same falsehood in the shm_transport.hpp flag comment; both now state the flag combines with all three types and receivers mask it off. - shm_transport.hpp file-header block framed the shm path as "large topic messages" written by "the publisher"; generalized to all three senders. - "1 MiB still holds 16 records at the 64 KiB threshold" was off by one: align_up(8 + 65536, 64) = 65600, and floor(1 MiB / 65600) = 15. Corrected with the arithmetic shown. - DESIGN.md mixed-build limitation extended to service peers: old-build services/clients also silently drop flagged large requests/responses from new-build senders.
Closes the top follow-up from the merge-readiness audit: every other binary runs sender and receiver in one process, so the shared-memory path was never exercised across a real PID boundary. New binary test_rmw_cross_process with two tests, both pushing 5 MB payloads (above the ~4 MB AF_UNIX datagram cap, so byte-identical arrival proves the bytes crossed through /dev/shm, not the socket): - LargeTransientLocalLateJoiner: parent latches 5 MB (durable segment), a forked child inits on the same domain, subscribes late, and receives the replay byte-equal — the child maps the parent's durable segment cross-PID. - LargeServiceRoundTrip: client child sends a 5 MB request, service parent takes it (maps the client's ring), responds with 5 MB, child verifies (maps the service's ring). Both ring directions crossed. CI-safety choreography: fork() before either process touches rmw (no inherited registry mapping/fds/mutex state); pipe handshakes with bounded poll(); child alarm() as last-resort kill and _exit() with distinct diagnostic codes (no gtest in the child); parent reaps via bounded WNOHANG loop with SIGKILL fallback; a ChildGuard kills+reaps the child if an early ASSERT bails out of the test (prevents ctest stalling on the inherited stdio pipe and re-run poisoning); SIGPIPE ignored so a dead child fails the test, not the binary; children tear down cleanly on success so nothing outlives a green run. Private domains 91/92 keep it isolated from the single-process suite (99). Sensitivity-verified: with RMW_UDS_TEST_FORCE_SHM_FAILURE=1 the latched test fails (child recv timeout) because the 5 MB replay goes inline and the kernel rejects it — the test genuinely detects a broken cross-process shm path. Full suite: 100 tests across 14 binaries; cross-process binary green 10/10 consecutive runs, no leftover /dev/shm segments.
Large messages were serialized into a fresh heap vector (whose resize() zero-initializes every byte), then memcpy'd into the ring — three full passes over the payload plus an allocation, per publish. Now the size walk picks the destination up front and the CDR serializer writes straight into the reserved ring record; the intermediate payload never exists. - serialization: serialized_size() (size walk without serializing) and serialize_into() (serialize into caller memory; wire-equivalent to serialize() — CDR padding bytes are unspecified since fastCDR skips them, and decoders skip the same bytes). - shm_transport: two-phase staging — shm_stage_reserve() marks the record's seqlock odd and returns its payload area; shm_stage_commit() publishes the ACTUAL serialized length (cursor advances by actual, so a size-walk overestimate wastes nothing); shm_stage_abort() leaves the seqlock odd and the cursor unchanged, so a failed serialize is never observable and the slot is reused. shm_stage_payload() is now reserve + memcpy + commit — one protocol implementation. - transport: shm_serialize_prepare_send() composes size walk -> reserve -> serialize-into -> commit, falling back to the inline vector path on any shm failure (and to it for sub-threshold payloads; exactly one size walk in all paths). Used by rmw_publish (non-latched), rmw_send_request, rmw_send_response. TRANSIENT_LOCAL keeps serialize-then-stage (its replay cache owns the bytes); rmw_publish_serialized_message has no serialize step. Measured with the committed benchmark (bench_serialize_into_ring, real fastCDR, UnboundedSequences): staging 2.0-3.2x faster — 15 us saved per 64 KiB message, 264 us per 1 MiB, 1.29 ms per 4 MiB. Tests: serialize_into wire-equivalence (incl. field-heavy MultiNested) and capacity-overflow rejection; reserve/commit round-trip; commit-smaller-than- reserved packs tight; abort leaves the slot reusable and unobservable; a descriptor into a reserved-but-uncommitted slot drops cleanly; over- reservation commit rejected. Full suite: 109 tests across 14 binaries.
From the R7 adversarial review (verdict: needs-minor-changes): - The inline path's payload.resize(est) in shm_serialize_prepare_send had no exception containment, so bad_alloc/length_error escaped across the extern "C" boundary (rmw_publish, rmw_send_request, rmw_send_response) and killed the node — the exact failure serialize()'s try/catch existed to contain, deterministically reachable when est > UINT32_MAX forces the inline fallback. Contained; callers map false to RMW_RET_ERROR as before. - Extend the RMW_UDS_TEST_FORCE_SHM_FAILURE seam into shm_stage_reserve and add VolatileLargeMessageInlineFallbackWhenShmUnavailable: a large VOLATILE publish under forced shm failure must create no ring, carry no flag, and deliver byte-equal inline — pins the reserve-failure branch and the contained resize. - CommitOverReservationIsRejected now also pins cursor/index non-advance (the rejection behaves like an abort). - DESIGN.md: the Staging-and-fanout parenthetical wrongly said TRANSIENT_LOCAL stages via shm_stage_payload; it uses shm_stage_durable's immutable segment (matching the Durable-segments paragraph). Corrected. - test/perf/README.md: run command for bench_serialize_into_ring. Full suite: 110 tests across 14 binaries.
perf(shm): serialize CDR directly into the ring record (R7)
…-payload feat(shm): shm-backed large payloads for latched topics and services
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.
No description provided.