Skip to content

Fix race when falling back from RDMA to TCP#3406

Open
legionxiong wants to merge 1 commit into
apache:masterfrom
legionxiong:fix-rdma-fallback-race
Open

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

Conversation

@legionxiong

Copy link
Copy Markdown
Contributor

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.

What problem does this PR solve?

Issue Number: #3405

Problem Summary:

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

In addition, RdmaConnect::StartConnect() publishes FALLBACK_TCP
before setting RdmaTransport::_rdma_state to RDMA_OFF. A concurrent
TCP event may therefore observe a partially published fallback state and
start TCP message processing while the transport still appears to use
RDMA.

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.
  • Use a relaxed atomic load for the _state check in
    RdmaEndpoint::HandleCompletion(), because the check does not consume
    data published by the state transition.

Side effects:

  • Performance effects:

    The handshake and TCP fallback paths use the default sequentially
    consistent ordering. These paths are not hot after the handshake
    completes. The completion hot path uses a relaxed load, so it does not
    introduce unnecessary ordering overhead.

  • Breaking backward compatibility:

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


Check List:

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 <[email protected]>

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.

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

3 participants