Skip to content

detect and negotiate mtu#3304

Open
randomkang wants to merge 5 commits into
apache:masterfrom
randomkang:dynamic_mtu
Open

detect and negotiate mtu#3304
randomkang wants to merge 5 commits into
apache:masterfrom
randomkang:dynamic_mtu

Conversation

@randomkang
Copy link
Copy Markdown
Contributor

What problem does this PR solve?

Issue Number: resolve

Problem Summary:

What is changed and the side effects?

Changed:

Side effects:

  • Performance effects:

The bandwith of rdma perf will be increased 25%(in my case, from 33GB/s to 41GB/s ).

  • Breaking backward compatibility:

Check List:

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Comment thread src/brpc/rdma/rdma_endpoint.cpp Outdated
Comment thread src/brpc/rdma/rdma_helper.cpp Outdated
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

Comment on lines +462 to +464

if (ibv_query_port(ctx, port_num, &port_attr)) {
LOG(ERROR) << "ibv_query_port failed";
Comment on lines +93 to 94
static uint16_t g_rdma_hello_msg_len = 42; // In Byte
static uint16_t g_rdma_hello_version = 2;
Comment on lines +1259 to +1267
LOG(INFO) << "negotiated mtu is 256";
} else if (mtu_type == IBV_MTU_512) {
LOG(INFO) << "negotiated mtu is 512";
} else if (mtu_type == IBV_MTU_1024) {
LOG(INFO) << "negotiated mtu is 1024";
} else if (mtu_type == IBV_MTU_2048) {
LOG(INFO) << "negotiated mtu is 2048";
} else if (mtu_type == IBV_MTU_4096) {
LOG(INFO) << "negotiated mtu is 4096";
Comment on lines +548 to 551
// use the minimum of local mtu type and remote mtu type
uint16_t min_mtu_type = std::min(local_mtu_type, remote_msg.mtu_type);
if (ep->BringUpQp(remote_msg.lid, remote_msg.gid, remote_msg.qp_num, min_mtu_type) < 0) {
LOG(WARNING) << "Fail to bringup QP, fallback to tcp:" << s->description();
Comment thread src/brpc/rdma/rdma_endpoint.cpp Outdated
Comment on lines 697 to 703
// Only happens in UT
local_msg.qp_num = 0;
}
local_msg.mtu_type = local_mtu_type;
}
memcpy(data, MAGIC_STR, 4);
local_msg.Serialize((char*)data + 4);
return NULL;
}

if (remote_msg.msg_len > HELLO_MSG_LEN_MIN) {
Copy link
Copy Markdown
Contributor

@chenBright chenBright May 20, 2026

Choose a reason for hiding this comment

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

We need to be compatible with scenarios where one peer does not support mtu_type. Keeping HELLO_MSG_LEN_MIN unchanged at 40 and using mtu_type as an extended field for parsing is more appropriate.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ok, we need to be compatible with scenarios with the old versions.

@chenBright
Copy link
Copy Markdown
Contributor

Please update the RDMA UTs in brpc_rdma_unittest.cpp.

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.

3 participants