Skip to content

[Bugfix][TE] Respect remote_accessible in RDMA verbs MR registration - #3151

Merged
alogfans merged 5 commits into
kvcache-ai:mainfrom
RuiqingFeng:fix/rdma-mr-size-and-remote-accessible
Jul 30, 2026
Merged

[Bugfix][TE] Respect remote_accessible in RDMA verbs MR registration#3151
alogfans merged 5 commits into
kvcache-ai:mainfrom
RuiqingFeng:fix/rdma-mr-size-and-remote-accessible

Conversation

@RuiqingFeng

Copy link
Copy Markdown
Contributor

Description

RdmaTransport::registerLocalMemoryInternal() previously discarded the remote_accessible argument ((void)remote_accessible;) and unconditionally granted remote read/write permissions (IBV_ACCESS_REMOTE_READ | IBV_ACCESS_REMOTE_WRITE). This violated the API contract and exposed local-only memory to remote peers.

This PR fixes the issue by:

  1. Setting IBV_ACCESS_REMOTE_* permissions only when remote_accessible is true. When false, registers with IBV_ACCESS_LOCAL_WRITE only and leaves the rkey vector empty.
  2. Filtering out RDMA buffers with empty rkey vectors during metadata encoding so local-only memory is not advertised across the network.
  3. Adding bounds checks in selectPeerDevice() for buffer_id and device_id against the rkey array size to prevent out-of-bounds indexing on local-only memory.

Module

  • Transfer Engine (mooncake-transfer-engine)
  • Mooncake Store (mooncake-store)
  • Mooncake EP (mooncake-ep)
  • Mooncake PG (mooncake-pg)
  • Integration (mooncake-integration)
  • P2P Store (mooncake-p2p-store)
  • Python Wheel (mooncake-wheel)
  • Common (mooncake-common)
  • Mooncake RL (mooncake-rl)
  • CI/CD
  • Docs
  • Other

Type of Change

  • Bug fix
  • New feature
  • Refactor
  • Breaking change
  • Documentation update
  • Performance improvement
  • Other

How Has This Been Tested?

Added unit tests in rdma_context_reprobe_test.cpp and transfer_metadata_test.cpp:

  1. RdmaMemoryRegistrationPolicyTest.LocalOnlyBufferHasNoPublishedRkey: Verifies that registering local memory with remote_accessible=false keeps its rkey empty.
  2. TransferMetadataPublicationTest.OmitsBufferWithoutRkey: Verifies that buffers with empty rkeys are excluded when encoding P2P segment descriptions for network transfer.

Test commands:

cmake --build build-ci-local --target rdma_context_reprobe_test transfer_metadata_test
./build-ci-local/mooncake-transfer-engine/tests/rdma_context_reprobe_test --gtest_filter='*RegistrationPolicy*'
./build-ci-local/mooncake-transfer-engine/tests/transfer_metadata_test --gtest_filter='TransferMetadataPublicationTest.*'

@RuiqingFeng
RuiqingFeng force-pushed the fix/rdma-mr-size-and-remote-accessible branch from dd2ae23 to fa0797e Compare July 28, 2026 05:18
@github-actions github-actions Bot added documentation Improvements or additions to documentation run-ci Transfer Engine labels Jul 28, 2026

@staryxchen staryxchen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Protocol coverage — Metadata filtering only skips empty rkey when protocol == "rdma", while barex / efa / cxi share the same encode path but are not filtered. EFA/CXI also still ignore remote_accessible. Is this PR intentionally RDMA-only?
  2. PR title — The title looks concatenated (...registrationFix/rdma...); worth cleaning up before merge.

Comment thread mooncake-transfer-engine/src/transport/rdma_transport/rdma_transport.cpp Outdated
@RuiqingFeng RuiqingFeng changed the title [Bugfix][TE] Respect remote_accessible in RDMA memory registrationFix/rdma mr size and remote accessible [Bugfix][TE] Respect remote_accessible in RDMA memory registration Jul 29, 2026
@RuiqingFeng RuiqingFeng changed the title [Bugfix][TE] Respect remote_accessible in RDMA memory registration [Bugfix][TE] Respect remote_accessible in RDMA verbs MR registration Jul 29, 2026
@RuiqingFeng

RuiqingFeng commented Jul 29, 2026

Copy link
Copy Markdown
Contributor Author
  1. Protocol coverage — Metadata filtering only skips empty rkey when protocol == "rdma", while barex / efa / cxi share the same encode path but are not filtered. EFA/CXI also still ignore remote_accessible. Is this PR intentionally RDMA-only?
  2. PR title — The title looks concatenated (...registrationFix/rdma...); worth cleaning up before merge.

Thanks for the review!

Protocol coverage——Yes, this PR is intentionally scoped to RDMA verbs MR registration, as it addresses an immediate requirement in Broadcom RDMA NIC environment. I will follow up with EFA / CXI / barex support in a separate PR.
PR title——Updated. Cleaned up the title to [Bugfix][TE] Respect remote_accessible for RDMA verbs MR registration.

@RuiqingFeng
RuiqingFeng force-pushed the fix/rdma-mr-size-and-remote-accessible branch from fa0797e to 74ef3a1 Compare July 29, 2026 02:32
@RuiqingFeng
RuiqingFeng requested a review from staryxchen July 29, 2026 02:34
@RuiqingFeng
RuiqingFeng force-pushed the fix/rdma-mr-size-and-remote-accessible branch from 74ef3a1 to af950e9 Compare July 29, 2026 02:37
@codecov-commenter

codecov-commenter commented Jul 29, 2026

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 71.42857% with 20 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...ngine/src/transport/rdma_transport/worker_pool.cpp 0.00% 17 Missing ⚠️
...ne/src/transport/rdma_transport/rdma_transport.cpp 40.00% 3 Missing ⚠️

📢 Thoughts on this report? Let us know!

@staryxchen staryxchen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
@alogfans pls double chck

@alogfans alogfans left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM. However, when the peer side rejects this memory region, it should print to log clearly about this to avoid slient bugs.

@RuiqingFeng
RuiqingFeng force-pushed the fix/rdma-mr-size-and-remote-accessible branch from af950e9 to c719721 Compare July 29, 2026 04:48
@RuiqingFeng

RuiqingFeng commented Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

Overall LGTM. However, when the peer side rejects this memory region, it should print to log clearly about this to avoid slient bugs.

Added an explicit error log [RDMA] No rkey for MR access: seg=... addr=... len=... in selectPeerDevice() when a peer memory region lacks remote access. Thanks!

@RuiqingFeng
RuiqingFeng requested a review from alogfans July 29, 2026 04:50

@alogfans alogfans left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of omitting local-only RDMA buffers from the segment metadata, could we keep their address ranges registered and explicitly mark them, while leaving the rkey unavailable? This allows the peer to reject such regions deterministically and distinguish them from missing or stale metadata, reducing the risk of accidental fallback or misuse.

@RuiqingFeng
RuiqingFeng force-pushed the fix/rdma-mr-size-and-remote-accessible branch from c719721 to 73dd7d7 Compare July 29, 2026 06:52
@RuiqingFeng

RuiqingFeng commented Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

Instead of omitting local-only RDMA buffers from the segment metadata, could we keep their address ranges registered and explicitly mark them, while leaving the rkey unavailable? This allows the peer to reject such regions deterministically and distinguish them from missing or stale metadata, reducing the risk of accidental fallback or misuse.

Agreed! Updated to preserve local-only buffers in metadata with an empty rkey list ([]). Peers can now match the address range and fail with error No rkey for MR access without unnecessary reloads. Good idea, thanks!

@RuiqingFeng
RuiqingFeng requested a review from alogfans July 29, 2026 06:55
@RuiqingFeng
RuiqingFeng force-pushed the fix/rdma-mr-size-and-remote-accessible branch from 73dd7d7 to c076a14 Compare July 29, 2026 13:12
@RuiqingFeng
RuiqingFeng requested a review from UNIDY2002 as a code owner July 29, 2026 13:12
@RuiqingFeng
RuiqingFeng force-pushed the fix/rdma-mr-size-and-remote-accessible branch from c076a14 to a94a212 Compare July 30, 2026 02:02
Signed-off-by: Ruiqing Feng <ruiqing.feng@broadcom.com>
RdmaTransport::registerLocalMemoryInternal() previously discarded the
`remote_accessible` argument and unconditionally requested remote read
and write permissions (IBV_ACCESS_REMOTE_READ | IBV_ACCESS_REMOTE_WRITE).
This violated the API contract and exposed local-only memory to remote peers.
Fix this issue with the following changes:
1. Only grant IBV_ACCESS_REMOTE_* rights when remote_accessible is true.
   When false, register with IBV_ACCESS_LOCAL_WRITE only and leave the
   rkey vector empty.
2. Filter out RDMA buffers with empty rkey vectors during metadata encoding
   so local-only memory is not advertised across the network.
3. Add bounds checks for buffer_id and device_id against the rkey array size
   in selectPeerDevice() to safely handle local-only memory selection.

Signed-off-by: Ruiqing Feng <ruiqing.feng@broadcom.com>
Signed-off-by: Ruiqing Feng <ruiqing.feng@broadcom.com>
- Print kernel-style error log in selectPeerDevice() when rkey is missing
- Remove unused buffer_desc variable in registerLocalMemoryInternal()

Signed-off-by: Ruiqing Feng <ruiqing.feng@broadcom.com>
Keep local-only RDMA buffers in segment metadata so peers match
address ranges deterministically and fail without metadata reloads.

Signed-off-by: Ruiqing Feng <ruiqing.feng@broadcom.com>
@alogfans
alogfans merged commit bac7b62 into kvcache-ai:main Jul 30, 2026
28 checks passed
@RuiqingFeng
RuiqingFeng deleted the fix/rdma-mr-size-and-remote-accessible branch August 4, 2026 04:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation run-ci Transfer Engine

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants