Fix rdma handshake failing the socket instead of falling back to TCP - #3424
Fix rdma handshake failing the socket instead of falling back to TCP#3424chenBright wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes RDMA handshake error handling so that certain RDMA initialization failures (notably CQ arming via ibv_req_notify_cq) correctly trigger a graceful fallback to plain TCP instead of prematurely failing the underlying socket, and ensures the connect callback reports success after fallback by clearing errno on the recoverable path.
Changes:
- Add a
fatal_on_errorswitch toRdmaEndpoint::ReqNotifyCqso handshake-time CQ arming failures don’t fail the TCP socket, while runtime re-arm failures remain fatal. - Clear
errnoon the client handshake’s recoverableAllocateResources()failure path to avoid reporting a failed connect after switching to TCP fallback. - Add unit tests covering client/server fallback-to-TCP behavior when resource allocation is forced to fail in UT mode.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/brpc/rdma/rdma_endpoint.cpp |
Adjusts handshake fallback behavior (no socket failure on CQ arm failure) and normalizes errno on recoverable fallback. |
src/brpc/rdma/rdma_endpoint.h |
Updates ReqNotifyCq signature to include fatal_on_error for handshake vs runtime behavior. |
test/brpc_rdma_unittest.cpp |
Adds UT coverage for client/server TCP fallback when RDMA resource allocation fails. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ef08c22 to
d80503f
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/brpc/rdma/rdma_endpoint.cpp:59
g_fail_resource_alloc_for_testis a plain globalboolthat is written by tests while other server/client bthreads may already be running, and read fromDoAllocateResources()on handshake threads. This introduces a C++ data race (no synchronization between the write and reads). Make this flag atomic (or otherwise synchronized) so the UT injection is thread-safe.
// Only for UT: force AllocateResources() to fail, so that the "fallback to TCP" path
// of the handshake can be tested without a real RDMA device.
bool g_fail_resource_alloc_for_test = false;
test/brpc_rdma_unittest.cpp:1931
ResourceAllocFailGuardtogglesrdma::g_fail_resource_alloc_for_test, which is read by handshake threads. If this flag remains a plainbool, the test introduces a data race (write in the test thread vs reads in server/client bthreads). Prefer making the flag atomic and using.load()/.store()here.
class ResourceAllocFailGuard {
public:
explicit ResourceAllocFailGuard(bool v)
: _saved(rdma::g_fail_resource_alloc_for_test) {
rdma::g_fail_resource_alloc_for_test = v;
}
~ResourceAllocFailGuard() {
rdma::g_fail_resource_alloc_for_test = _saved;
}
|
LGTM |
What problem does this PR solve?
Issue Number: resolve #3416
Problem Summary:
During the RDMA handshake,
RdmaEndpoint::AllocateResources()arms the send/recvCQs through
ReqNotifyCq(). Ifibv_req_notify_cq()fails,ReqNotifyCq()calls_socket->SetFailed()immediately. However, both handshake paths(
ProcessHandshakeAtClient()andExecuteServerHandshake()) treatAllocateResources() < 0as a recoverable error: they only turn RDMA off(
RDMA_OFF) and move the endpoint toFALLBACK_TCP, expecting the connection tokeep working over plain TCP. Since the socket has already been failed, the
connection cannot carry TCP any more, so a single CQ arm failure turns a
"graceful degradation to TCP" into a "broken connection".
While fixing this, a second issue was found on the same path.
RdmaConnect::Run()reports the connect result with
_done(errno, _data), i.e. it uses the currenterrnoas the connect error code. The client-side fallback branch returns rightafter
AllocateResources()fails, leavingerrnoset to the allocation error,while every other path in
ProcessHandshakeAtClient()ends witherrno = 0. As aresult the connection is reported as failed to the upper layer even after the
endpoint has entered
FALLBACK_TCP. Fixing onlyReqNotifyCq()is therefore notenough to make the fallback work.
What is changed and the side effects?
Changed:
Side effects:
Performance effects:
Breaking backward compatibility:
Check List: