[TE] Add rdma_twosided control-plane notify channel by zhtshr · Pull Request #3440 · kvcache-ai/Mooncake

zhtshr · 2026-08-14T02:34:11Z

Description

Second slice of the classic TE two-sided work (RFC #3377). Follows merged PR1 (#3324).

Adds an opt-in rdma_twosided transport whose control plane is a per-peer CtrlChannel (RC QP SEND/RECV). Classic rdma stays one-sided; a process installs either rdma or rdma_twosided.

This PR:

Adds RdmaTwoSidedTransport (MC_USE_RDMA_TWOSIDED=1) and wires MultiTransport mutual exclusion
Implements CtrlChannel + handshake fields (notify_qp_num / notify_rq_depth / ctrl_channel)
Routes sendNotify* over RDMA NOTIFY_COMPAT frames, with OOB fallback (MC_RDMA_NOTIFY_OOB_FALLBACK)
Adds rdma_notify_test (correctness + latency smoke; self-skips without an RDMA device)

No MsgChannel / bounce / managed-buffer data path yet (PR3). TE submitTransfer two-sided path remains PR4.

Module

Type of Change

How Has This Been Tested?

Test commands:

ninja ctrl_frame_test sender_credit_test rdma_notify_test
./mooncake-transfer-engine/tests/ctrl_frame_test
./mooncake-transfer-engine/tests/sender_credit_test
MC_TE_FILTERS=erdma_0 MC_TEST_DEVICE_NAME=erdma_0 MC_MTU=1024 \
  ./mooncake-transfer-engine/tests/rdma_notify_test

Test results:

Unit tests pass (ctrl_frame_test / sender_credit_test)
rdma_notify_test 4/4 on erdma_0 (single / bidi / latency smoke ~10k ops/s / burst 128)
Integration tests pass (if applicable)
Manual testing done (describe below)

Notify latency smoke on eRDMA: 2000 notifies in ~191 ms, ~10.5k ops/s, avg ~95 µs.

Checklist

I have performed a self-review of my own code
I have formatted my code using ./scripts/code_format.sh
I have run pre-commit on touched files and hooks pass
I have updated the documentation (if applicable)
I have added tests to prove my changes are effective
For changes >500 LOC: I have filed an RFC issue

RFC: #3377

AI Assistance Disclosure

No AI tools were used
AI tools were used (specify below)

Cursor agent assisted implementing the rdma_twosided control-plane channel. Human submitter reviewed the PR2 scope (CtrlChannel + notify path only).

Made with Cursor

Install RdmaTwoSidedTransport as an opt-in alternative to classic rdma and route sendNotify over a per-peer CtrlChannel, with OOB fallback. Co-authored-by: Cursor <[email protected]>

Copilot

Pull request overview

This PR adds an opt-in rdma_twosided Transfer Engine transport that introduces a per-peer RDMA RC SEND/RECV control-plane CtrlChannel to carry notify messages as typed control frames, while keeping the existing classic rdma one-sided data path unchanged and mutually exclusive with the new transport.

Changes:

Introduces RdmaTwoSidedTransport and CtrlChannel, wiring handshake fields (notify_qp_num, notify_rq_depth, ctrl_channel) and a background poll worker for CtrlChannel completions.
Routes sendNotify* through the CtrlChannel RDMA notify path with configurable OOB fallback behavior.
Adds an RDMA notify correctness/latency smoke test (rdma_notify_test) and integrates it into the test build.

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
mooncake-transfer-engine/tests/rdma_notify_test.cpp	Adds RDMA notify correctness + latency smoke tests (self-skips without an RDMA device).
mooncake-transfer-engine/tests/CMakeLists.txt	Builds/registers `rdma_notify_test` and labels it as RDMA.
mooncake-transfer-engine/src/transport/rdma_twosided/rdma_twosided_transport.cpp	Implements the new transport wrapper and hooks CtrlChannel setup into connection establishment.
mooncake-transfer-engine/src/transport/rdma_twosided/ctrl_plane.cpp	Implements ctrl-channel setup, notify routing, and the ctrl worker polling loop.
mooncake-transfer-engine/src/transport/rdma_twosided/ctrl_channel.cpp	Implements the per-peer RC QP CtrlChannel (resource lifecycle, handshake connect, SEND/RECV, CQ polling).
mooncake-transfer-engine/src/transfer_metadata.cpp	Extends handshake JSON encode/decode for CtrlChannel fields; adds `pushNotify()` to inject notifies from non-OOB paths.
mooncake-transfer-engine/src/transfer_engine_impl.cpp	Routes notify sending through RDMA CtrlChannel when available, with optional OOB fallback.
mooncake-transfer-engine/src/multi_transport.cpp	Enforces mutual exclusion between `rdma` and `rdma_twosided`; routes “rdma” segments to twosided transport when installed.
mooncake-transfer-engine/src/config.cpp	Adds env-driven configuration for installing `rdma_twosided` and tuning RDMA notify behavior.
mooncake-transfer-engine/include/transport/rdma_twosided/rdma_twosided_transport.h	Declares `RdmaTwoSidedTransport` public API and ctrl-worker/channel management.
mooncake-transfer-engine/include/transport/rdma_twosided/ctrl_channel.h	Declares the per-peer CtrlChannel interface (connect, send frame/notify, poll, disconnect).
mooncake-transfer-engine/include/transport/rdma_transport/rdma_transport.h	Makes `onSetupRdmaConnections` virtual to allow transport override.
mooncake-transfer-engine/include/transfer_metadata.h	Adds CtrlChannel handshake fields and `pushNotify()` API.
mooncake-transfer-engine/include/transfer_engine_impl.h	Adds config dependency for choosing `rdma_twosided` as default protocol when enabled.
mooncake-transfer-engine/include/config.h	Adds new global config toggles/knobs for `rdma_twosided` + RDMA notify path.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

+        int processed = 0;
+        for (auto &channel : channels) {
+            processed += channel->pollCompletions(16);
+        }
+        if (processed == 0) {
+            std::this_thread::sleep_for(std::chrono::microseconds(100));
+        }


+        if (root.isMember("notify_rq_depth") &&
+            root["notify_rq_depth"].isUInt()) {
+            desc.notify_rq_depth =
+                static_cast<uint16_t>(root["notify_rq_depth"].asUInt());
+        }


Reject out-of-range notify_rq_depth, avoid spinning on poll errors, and self-skip rdma_notify_test when CI enumerates an unusable mlx5_0. Co-authored-by: Cursor <[email protected]>

alogfans · 2026-08-17T02:57:57Z

+        return nullptr;
+    }
+    lock.lock();
+    auto again = ctrl_channels_.find(peer_server_name);


Does multiple callers can still create channels concurrently?

Yes. The old re-check after handshake did not serialize in-flight connects.

We now insert a placeholder into ctrl_channels_ before the handshake. Other callers wait on ctrl_cv_ until the entry is connected, replaced, or erased, instead of starting another connectActive().

alogfans · 2026-08-17T03:01:01Z

+void CtrlChannel::disconnect() {
+    std::lock_guard<std::mutex> lock(resource_mutex_);
+    destroyResources();
+}


This introduces a lock-order inversion: sendCtrlFrame() holds send_mutex_ before acquiring resource_mutex_, while disconnect() holds resource_mutex_ and then calls destroyResources(), which acquires send_mutex_. Concurrent send and channel replacement can therefore deadlock. Please use a consistent lock order or split shutdown into non-nested state-transition and resource-destruction phases.

Agreed. sendCtrlFrame() took send_mutex_ then resource_mutex_, while disconnect() nested them the other way.

disconnect() no longer nests the locks: it wakes send waiters, drops send_mutex_, then tears down IB resources under resource_mutex_ only.

Placeholder entries stop concurrent active handshakes to the same peer, and disconnect no longer nests send/resource mutexes in reverse order. Co-authored-by: Cursor <[email protected]>

alogfans · 2026-08-18T11:21:28Z

+    std::unique_lock<std::mutex> lock(ctrl_mutex_);
+    while (true) {
+        auto it = ctrl_channels_.find(peer_server_name);
+        if (it != ctrl_channels_.end() && it->second) {
+            if (it->second->connected()) return it->second;
+            // Active or passive connect is already in flight for this peer.
+            ctrl_cv_.wait(lock);
+            continue;
+        }


How to avoid ctrl_cv_ wait indefinitely?

alogfans · 2026-08-18T11:23:47Z

+    return 0;
+}
+
+int CtrlChannel::sendNotify(const NotifyDesc &notify) {


This should be called after receiving WCs instead of submitting WRs.

[TE] Add rdma_twosided control-plane notify channel

04874ec

Install RdmaTwoSidedTransport as an opt-in alternative to classic rdma and route sendNotify over a per-peer CtrlChannel, with OOB fallback. Co-authored-by: Cursor <[email protected]>

zhtshr requested review from alogfans, chestnut-Q and doujiang24 as code owners August 14, 2026 02:34

Copilot AI lite review requested due to automatic review settings August 14, 2026 02:34

zhtshr requested a review from staryxchen as a code owner August 14, 2026 02:34

zhtshr mentioned this pull request Aug 14, 2026

[RFC]: Classic TE RDMA Two-Sided Transport (rdma_twosided) #3377

Open

Copilot started reviewing on behalf of zhtshr August 14, 2026 02:37 View session

github-actions Bot added run-ci Transfer Engine labels Aug 14, 2026

Copilot AI reviewed Aug 14, 2026

View reviewed changes

[TE] Harden twosided handshake decode and skip phantom RDMA NICs

e3121f0

Reject out-of-range notify_rq_depth, avoid spinning on poll errors, and self-skip rdma_notify_test when CI enumerates an unusable mlx5_0. Co-authored-by: Cursor <[email protected]>

alogfans reviewed Aug 17, 2026

View reviewed changes

[TE] Serialize CtrlChannel connect and fix disconnect lock order

e69739f

Placeholder entries stop concurrent active handshakes to the same peer, and disconnect no longer nests send/resource mutexes in reverse order. Co-authored-by: Cursor <[email protected]>

alogfans reviewed Aug 18, 2026

View reviewed changes

Navigation Menu

Uh oh!

[TE] Add rdma_twosided control-plane notify channel - #3440

[TE] Add rdma_twosided control-plane notify channel#3440
zhtshr wants to merge 3 commits into
kvcache-ai:mainfrom
zhtshr:feature/te-rdma-twosided-pr2

zhtshr commented Aug 14, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

alogfans Aug 17, 2026

Uh oh!

zhtshr Aug 17, 2026

Uh oh!

alogfans Aug 17, 2026

Uh oh!

zhtshr Aug 17, 2026

Uh oh!

alogfans Aug 18, 2026

Uh oh!

alogfans Aug 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

zhtshr commented Aug 14, 2026

Description

Module

Type of Change

How Has This Been Tested?

Checklist

AI Assistance Disclosure

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

alogfans Aug 17, 2026

Choose a reason for hiding this comment

Uh oh!

zhtshr Aug 17, 2026

Choose a reason for hiding this comment

Uh oh!

alogfans Aug 17, 2026

Choose a reason for hiding this comment

Uh oh!

zhtshr Aug 17, 2026

Choose a reason for hiding this comment

Uh oh!

alogfans Aug 18, 2026

Choose a reason for hiding this comment

Uh oh!

alogfans Aug 18, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants