detect and negotiate mtu - #3304
Open
randomkang wants to merge 2 commits into
Open
Conversation
chenBright
reviewed
May 19, 2026
chenBright
reviewed
May 20, 2026
Contributor
|
Please update the RDMA UTs in brpc_rdma_unittest.cpp. |
chenBright
reviewed
May 21, 2026
Contributor
Author
I can't find a way to check the rdma communication between the current version including mtu negotiation and without mtu negotiation in ut, any advice? |
Contributor
You can refer to client_hello_msg_invalid_sq_rq_block_size, server_hello_invalid_sq_rq_size, etc. |
Contributor
|
With #3326 , mtu should be negotiated through V3. |
chenBright
reviewed
Aug 4, 2026
| // before C_HELLO_SEND); | ||
| // Server: the negotiated MTU = min(local_active_mtu, client_mtu) | ||
| // (filled in BringUpQp). | ||
| butil::optional<uint32_t> _outgoing_mtu; |
Contributor
There was a problem hiding this comment.
Why use uint32_t instead of ibv_mtu?
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
src/brpc/rdma/rdma_handshake.cpp:420
- Client v3 handshake never advertises MTU in the default unit-test configuration because this code is gated on g_skip_rdma_init. In this repo's tests, main() sets g_skip_rdma_init=true when FLAGS_rdma_test_enable is false, which makes TEST_F(RdmaTest, v3_client_hello_includes_mtu) fail and also prevents exercising MTU negotiation logic in UT mode. Since GetRdmaActiveMtu() already has a safe default (IBV_MTU_1024), it can be advertised even when RDMA init is skipped.
// Best-effort: any failure just means we won't advertise MTU
// (the peer falls back to IBV_MTU_1024).
if (!g_skip_rdma_init) {
_ep->_outgoing_mtu = GetRdmaActiveMtu();
}
test/brpc_rdma_unittest.cpp:1975
- This test assumes the server computes and advertises negotiated MTU even when g_skip_rdma_init=true (the default when FLAGS_rdma_test_enable is not set). However, in that mode RdmaEndpoint::BringUpQp() returns early before MTU negotiation runs, leaving _outgoing_mtu unset; the server hello will omit mtu and EXPECT_TRUE(reply.has_mtu()) will fail. Either make the negotiation run in skip-init mode or skip this test unless RDMA runtime is enabled.
// The server reply must carry a negotiated MTU when the client advertised one.
// Since UT skips real QP bring-up, the server computes min(local, client) and
// stores it in _outgoing_mtu; FillLocalRdmaHello then includes it in the reply.
TEST_F(RdmaTest, v3_server_reply_has_negotiated_mtu) {
StartServer();
Comment on lines
+1197
to
+1208
| uint32_t negotiated_mtu = IBV_MTU_1024; | ||
| if (remote.path_mtu.has_value()) { | ||
| if (is_server) { | ||
| uint32_t local_mtu = GetRdmaActiveMtu(); | ||
| negotiated_mtu = std::min(local_mtu, *remote.path_mtu); | ||
| // Store the negotiated MTU for the server hello reply. | ||
| _outgoing_mtu = negotiated_mtu; | ||
| } else { | ||
| negotiated_mtu = *remote.path_mtu; | ||
| } | ||
| } | ||
| attr.path_mtu = static_cast<ibv_mtu>(negotiated_mtu); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: resolve
Problem Summary:
What is changed and the side effects?
Changed:
Side effects:
The bandwith of rdma perf will be increased 25%(in my case, from 33GB/s to 41GB/s ).
Check List: