Skip to content

Fix race when falling back from RDMA to TCP#3406

Merged
zyearn merged 1 commit into
apache:masterfrom
legionxiong:fix-rdma-fallback-race
Jul 27, 2026
Merged

Fix race when falling back from RDMA to TCP#3406
zyearn merged 1 commit into
apache:masterfrom
legionxiong:fix-rdma-fallback-race

Conversation

@legionxiong

@legionxiong legionxiong commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

What problem does this PR solve?

Issue Number: #3405

Problem Summary:

RdmaEndpoint::_state is accessed concurrently by the RDMA handshake,
TCP event handling, and completion handling paths, but was not atomic.
This causes a C++ data race.

During TCP fallback, RdmaTransport::_rdma_state and
RdmaEndpoint::_state also form a logical state transition. A TCP event
must not observe FALLBACK_TCP before the preceding RDMA_OFF update is
visible, otherwise it may start normal TCP message processing with a
partially published transport state.

What is changed and the side effects?

Changed:

  • Change RdmaEndpoint::_state to butil::atomic<State>.
  • Set RdmaTransport::_rdma_state to RDMA_OFF before publishing
    RdmaEndpoint::_state as FALLBACK_TCP.
  • Publish FALLBACK_TCP with memory_order_release.
  • Load the handshake state with memory_order_acquire in
    OnNewDataFromTcp(). When it observes FALLBACK_TCP, this acquire load
    pairs with the release store and makes the preceding RDMA_OFF update
    visible before normal TCP message processing starts.
  • Use memory_order_relaxed for all other handshake-state transitions and
    observations because they do not publish or consume related data.
  • Add comments documenting the acquire/release synchronization and why
    relaxed ordering is sufficient for the other accesses.

Side effects:

  • Performance effects:

    The acquire load is limited to the TCP event path, while release stores
    are only used when falling back to TCP. Other handshake-state accesses,
    including the completion hot path, use relaxed ordering to avoid
    unnecessary synchronization overhead.

  • Breaking backward compatibility:

    None. This change does not modify any public API, wire protocol, or
    externally visible configuration.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes a concurrency correctness issue in the RDMA transport handshake/fallback path by making the RDMA handshake state thread-safe and ensuring the TCP fallback state is published in a safe order, preventing TCP event handling from observing a partially published fallback.

Changes:

  • Change rdma::RdmaEndpoint::_state from a plain enum to butil::atomic<State> to eliminate a C++ data race across handshake/TCP/completion paths.
  • Reorder fallback publication in RdmaConnect::StartConnect() to set RdmaTransport::_rdma_state = RDMA_OFF before publishing RdmaEndpoint::_state = FALLBACK_TCP.
  • Use a relaxed atomic load for the completion-path invariant check in RdmaEndpoint::HandleCompletion() to avoid adding unnecessary ordering overhead on a hot path.

Reviewed changes

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

File Description
src/brpc/rdma/rdma_endpoint.h Makes handshake state (_state) atomic to remove data races between concurrent RDMA/TCP/completion execution contexts.
src/brpc/rdma/rdma_endpoint.cpp Fixes fallback publication ordering and updates the completion-path state check to use an explicit relaxed atomic load.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@chenBright chenBright left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

When falling back to TCP, the require/release memory order is sufficient, and comments should be added. otherwise, use relaxed ordering.

Make the handshake state atomic and publish RDMA_OFF before
FALLBACK_TCP. This prevents TCP event handling from observing a
partially published fallback state. Use a relaxed load for the
completion-path check because it does not consume related data.

Signed-off-by: Lijin Xiong <legion.xiong@gmail.com>
@legionxiong
legionxiong force-pushed the fix-rdma-fallback-race branch from 735f4df to 6e5b15a Compare July 27, 2026 01:56
@legionxiong

Copy link
Copy Markdown
Contributor Author

When falling back to TCP, the require/release memory order is sufficient, and comments should be added. otherwise, use relaxed ordering.

Done.

@chenBright chenBright left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@zyearn

zyearn commented Jul 27, 2026

Copy link
Copy Markdown
Member

LGTM. Merging

@zyearn
zyearn merged commit 5f228c0 into apache:master Jul 27, 2026
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[RDMA] Data race and partial state publication during TCP fallback

4 participants