Repository navigation
Add IPC_V2 data path for UBRing IPC - #3573
Merged
Merged
Conversation
Add IPC_V2 data path for UBRing IPC
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Format fallback can leave active server resources behind, and a client setup failure can retain shared-memory mappings.
Review effort: Balanced
Findings: 1
Open (2)
What changed in this PR
This PR adds a negotiated IPC_V2 data path to UBRing so local IPC can copy directly into larger shared-memory slots. UBS retains the legacy path, and peers with different formats fall back to TCP.
Changes:
- Add 4096-byte IPC_V2 slots and an SPSC ring for direct
iovecreads and writes. - Select a data format by shared-memory backend without changing the handshake wire layout.
- Add layout, ring, dispatch, and format-selection tests.
| File | Description |
|---|---|
test/brpc_ubring_unittest.cpp |
Adds IPC_V2 and format-selection tests. |
src/brpc/ubshm/ubr_trx.h |
Stores format and read-offset state. |
src/brpc/ubshm/ubr_msg_v2.h |
Declares the IPC_V2 slot and ring interfaces. |
src/brpc/ubshm/ubr_msg_v2.cpp |
Implements IPC_V2 queue operations. |
src/brpc/ubshm/ubr_data_format.h |
Defines the IPC_V2 format identifier. |
src/brpc/ubshm/ub_ring.h |
Declares format-specific ring operations. |
src/brpc/ubshm/ub_ring.cpp |
Adds IPC_V2 dispatch and queue setup. |
src/brpc/ubshm/ub_endpoint.h |
Declares format-selection helpers. |
src/brpc/ubshm/ub_endpoint.cpp |
Negotiates formats during the handshake. |
src/brpc/ubshm/shm/shm_mgr.h |
Exposes the selected shared-memory backend. |
src/brpc/ubshm/shm/shm_mgr.cpp |
Implements the backend getter. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| local_extension.format_id = | ||
| static_cast<uint16_t>(selected_format); | ||
| } else { | ||
| ub_transport->_ub_state = UBShmTransport::UB_OFF; |
Contributor
Author
There was a problem hiding this comment.
Agreed. The format-mismatch fallback needs explicit rollback of the provisional server resources. This overlaps with #3509, which changes timer cancellation, cleanup ownership, and transaction slot reuse. I plan to update this PR after #3509 lands, then implement the rollback using the new lifecycle APIs and add coverage for this fallback path.
Comment on lines
+1223
to
+1226
| if (UNLIKELY(rc != UBRING_OK)) { | ||
| LOG(ERROR) << "Initialize client data format failed, local_name=" | ||
| << local_name << ", format=" << format; | ||
| return rc; |
Contributor
Author
There was a problem hiding this comment.
Agreed that this failure path needs explicit cleanup. The specific “fewer than two slots” example may not be reachable through the normal IPC allocation path because shared memory is required to be at least 4 MiB and allocation-unit aligned, but the error path should still release all provisional resources. I’ll address it after updating this PR with #3509 and add failure-path coverage.
Contributor
|
LGTM |
Contributor
Author
|
@chenBright Do you have any further comments on this PR? If not, I'll go ahead and merge it. |
Sherlock0203
added a commit
to Sherlock0203/brpc
that referenced
this pull request
Oct 16, 2026
Bring the negotiation and IPC_V2 work that landed on master (apache#3507, apache#3573) into this branch so it can be merged back without conflicts. Those master changes were built on the timerfd-era close path, which this branch had replaced with the bthread timer facade, generation-checked pool teardown and the cleanup worker, so both sides evolved the same ubshm core after 9334389. Resolution: - Close/clear core (UbrTrxClose, UbrTrxCloseCallback, UbrClearResourceCheck, ClearTrxResource, UbrPassiveClearTrx, UbrTrxCloseCheck) keeps this branch's versions. Master's extra start_time/op parameters were unused, and UbrCloseType only chose between DeleteTimerSafe and DeleteTimer, which the facade subsumes: UbrTimerDelAndWait degrades to the non-blocking delete inside a callback. The unused UbrCloseType enum is therefore dropped. - Master's format-aware initialization is kept: UbrServerTrxInit keeps the UbrDataFormat parameter and the Initialize*LegacyFormat split, UbrAllocateServerShm/UbrMapRemoteShm(UbrMapRemoteShmAddTimer) keep passing it, and ApplyAndMapLocalShm no longer presets the rx capacity (InitializeClientLocalLegacyFormat owns that now). All IPC_V2 ring code and the negotiation handshake are master's, unchanged. - Master-side UNLIKELY/LIKELY are converted to BAIDU_UNLIKELY/ BAIDU_LIKELY, matching the module convention from this branch's style pass; common.h keeps the plain aliases. - This branch's fixes that master lacked merge on top: the poller re-ADD refresh, butil::atomic for the poller-group flag, the lazy bthread timers replacing TimerInit, and the cleanup-worker changes (leaked queue, start retry instead of the terminal latch). - test/brpc_ubring_unittest.cpp: master's IPC_V2 assertions on the removed timer_fd/hb_timer_fd fields now check the unarmed close_timer/hb_timer slots instead. The UbrCleanupWorkerTest that PR LinQuickDev#74's squash dropped is restored (the commit describes it and the g_ub_test_worker_start_rc hook it exercises, but only the two source files made it into the squash), so both sides' suites run: 31 tests. Tested: cmake --build build --target brpc_ubring_unittest (WITH_UBRING =ON, BUILD_UNIT_TESTS=ON) and ./build/test/brpc_ubring_unittest (31/31), also under ASAN (detect_leaks=0, 31/31). e2e with example/ubring_performance --use_ubring=true over the IPC backend: UBRING_* mappings confirmed, the negotiated IPC_V2 data path sustains ~148k QPS at ~6us, phase teardowns are clean, and a server kill -9 is handled without errors or residue.
Sherlock0203
added a commit
to Sherlock0203/brpc
that referenced
this pull request
Oct 19, 2026
Bring the apache master line up to f0239d7 into this branch in a single merge, replacing the two stacked merges with master@36c2d12e and the macOS fix only. Everything the branch needs from that line is included: the data-format negotiation (apache#3507), the IPC_V2 data path (apache#3573), the macOS OpenSSL 4.0 build fix (apache#3616: X509 accessors return const pointers in the current macos-26-arm64 runner image, fixed with auto*), and the newer fixes, notably the null-termination of the peer shm_name in HelloMessage::Deserialize (apache#3605), the MPSC queue node release ordering (apache#3620) and the MultiFlatMap bucket-head erasure fix (apache#3617). Those master changes were built on the timerfd-era close path, which this branch had replaced with the bthread timer facade, generation-checked pool teardown and the cleanup worker, so both sides evolved the same ubshm core after 9334389. Resolution: - Close/clear core (UbrTrxClose, UbrTrxCloseCallback, UbrClearResourceCheck, ClearTrxResource, UbrPassiveClearTrx, UbrTrxCloseCheck) keeps this branch's versions. Master's extra start_time/op parameters were unused, and UbrCloseType only chose between DeleteTimerSafe and DeleteTimer, which the facade subsumes: UbrTimerDelAndWait degrades to the non-blocking delete inside a callback. The unused UbrCloseType enum is therefore dropped. - Master's format-aware initialization is kept: UbrServerTrxInit keeps the UbrDataFormat parameter and the Initialize*LegacyFormat split, UbrAllocateServerShm/UbrMapRemoteShm(UbrMapRemoteShmAddTimer) keep passing it, and ApplyAndMapLocalShm no longer presets the rx capacity (InitializeClientLocalLegacyFormat owns that now). All IPC_V2 ring code and the negotiation handshake are master's, unchanged, with master-side UNLIKELY/LIKELY converted to the module's BAIDU_* macros. - This branch's fixes that master lacked merge on top: the poller re-ADD refresh, butil::atomic for the poller-group flag, the lazy bthread timers replacing TimerInit, and the cleanup-worker changes (leaked queue, start retry instead of the terminal latch). - test/brpc_ubring_unittest.cpp: master's IPC_V2 assertions on the removed timer_fd/hb_timer_fd fields check the unarmed close_timer/hb_timer slots instead, and the UbrCleanupWorkerTest that PR LinQuickDev#74's squash dropped is restored alongside master's apache#3605 hardening of the full_shm_name test. Both sides' suites run. Tested: the close/cleanup/IPC_V2 resolution was verified on the equivalent tree before apache#3605 came in -- cmake WITH_UBRING=ON build, 31/31 unit tests, the same under ASAN (detect_leaks=0), and an example/ubring_performance e2e over the IPC backend (negotiated IPC_V2 path at ~148k QPS, clean teardowns, server kill -9 handled). The new master commits are additive to that resolution; cmake --build build --target brpc_ubring_unittest and ./build/test/brpc_ubring_unittest pass on this exact tree as well.
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
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.
What problem does this PR solve?
Issue Number: #3463
Problem Summary:
This is the second PR in Phase 3 of Issue #3463.
The first Phase 3 PR (#3507) introduced backward-safe UBRing data-format negotiation, but both IPC and UBS still use the existing
LEGACY_64data path. It divides data into 60-byte payload chunks and stages each chunk throughlocal_msg_spacebefore publishing a 64-byte slot withCopy64Byte.This design keeps the two shared-memory backends on a common data path, but it prevents local IPC from using larger slots and copying
iovecdata directly into shared memory.This PR adds the negotiated
IPC_V2format for the IPC backend.IPC_V2uses 4096-byte slots and a dedicated SPSC ring with directiovec-to-shared-memory copying. It is enabled only when both peers selectIPC_V2; otherwise, the connection safely falls back to TCP.The UBS backend continues to select
LEGACY_64, and its existing data path remains unchanged.What is changed and the side effects?
Changed:
UBR_DATA_FORMAT_IPC_V2.IPC_V2.LEGACY_64.IPC_V2slot layout:EMPTYandREADYownership states.IPC_V2ring using release/acquire publication and recycling semantics.iovecdata directly intoIPC_V2shared-memory slots instead of first copying it into the legacy 64-byte intermediate message buffer.IPC_V2support for:writevandreadv;IPC_V2data path.IPC_V2layout, queue mapping, initialization, format selection, read/write dispatch, boundary conditions, partial reads, wrap-around, backpressure, invalid input, error mapping, and concurrent SPSC operation.LEGACY_64slot layout, 60-byte payload,Copy64Byte, and UBS data path unchanged.The existing handshake wire format is not changed by this PR:
Tests:
Smoke-test results:
IPC_V2client and server successfully negotiated UBRing.Side effects:
Performance effects:
IPC_V2reduces the number of slots required for larger messages and removes the legacylocal_msg_space -> Copy64Bytestaging copy from the IPC path.The 4096-byte slot size was selected from the evaluated
IPC_V2microbenchmark candidates as a balance between throughput and the number of available slots.Small messages still occupy one 4096-byte
IPC_V2slot, so the number of simultaneously queued small messages is lower than with the legacy 64-byte layout.UBS has no data-path performance change because it continues to use
LEGACY_64.Breaking backward compatibility:
No wire-format compatibility break is introduced.
IPC_V2is enabled only when both peers select it. AnIPC_V2peer connected to a peer that only supportsLEGACY_64falls back to TCP.IPC and UBS peers also fall back to TCP when their selected data formats differ.
The existing Base Hello and V3 format-extension layouts remain unchanged, so no additional bytes can remain unread in the TCP stream during fallback.
Timer and teardown lifecycle changes are intentionally out of scope and remain covered by the separate Phase 2 work in #3509.
Check List: