You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brpc.apache.org by "chenBright (via GitHub)" <gi...@apache.org> on 2026/06/19 15:54:38 UTC
[PR] Support end-to-end ECE negotiation in RDMA handshake V3 (brpc)
chenBright opened a new pull request, #3352:
URL: https://github.com/apache/brpc/pull/3352
### What problem does this PR solve?
Issue Number: resolve
Problem Summary:
In #3255, `BringUpQp()` only did a local `ibv_query_ece` + `ibv_set_ece`
roundtrip and never exchanged ECE capabilities with the peer.
### What is changed and the side effects?
Changed:
This PR implements an end-to-end ECE negotiation in the v3 handshake.
- Client (`RdmaHandshakeClientV3::SendLocalHello`): query local ECE,
store it in `_outgoing_ece`, advertise it in the v3 hello.
- Server (`BringUpQp`): apply the client's ECE during INIT->RTR (`ibv_set_ece`).
After QP reaches RTS, `ibv_query_ece` to obtain the reduced/negotiated ECE
(the subset both peers support) and store it in `_outgoing_ece`.
- Server (`SendLocalHello` → `FillLocalRdmaHello`): advertise the negotiated ECE
in the reply hello.
- Client (`BringUpQp`): apply the server's reduced ECE during INIT->RTR.
Side effects:
- Performance effects:
- Breaking backward compatibility:
---
### Check List:
- Please make sure your changes are compilable.
- When providing us with a new feature, it is best to add related tests.
- Please follow [Contributor Covenant Code of Conduct](https://github.com/apache/brpc/blob/master/CODE_OF_CONDUCT.md).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Support end-to-end ECE negotiation in RDMA handshake V3 (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on PR #3352:
URL: https://github.com/apache/brpc/pull/3352#issuecomment-5014191118
@yanglimingcn PTAL
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Support end-to-end ECE negotiation in RDMA handshake V3 (brpc)
Posted by "yanglimingcn (via GitHub)" <gi...@apache.org>.
yanglimingcn commented on PR #3352:
URL: https://github.com/apache/brpc/pull/3352#issuecomment-5032080060
LGTM
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Support end-to-end ECE negotiation in RDMA handshake V3 (brpc)
Posted by "yanglimingcn (via GitHub)" <gi...@apache.org>.
yanglimingcn merged PR #3352:
URL: https://github.com/apache/brpc/pull/3352
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Support end-to-end ECE negotiation in RDMA handshake V3 (brpc)
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3352:
URL: https://github.com/apache/brpc/pull/3352#discussion_r3443515984
##########
src/brpc/rdma/rdma_endpoint.cpp:
##########
@@ -1215,18 +1216,22 @@ int RdmaEndpoint::BringUpQp(uint16_t lid, ibv_gid gid, uint32_t qp_num) {
return -1;
}
- if (FLAGS_rdma_ece) {
- struct ibv_ece ece;
- int err = IbvQueryEce(_resource->qp, &ece);
+ // ECE negotiation, done while the QP is in INIT state (must be set
+ // before the RTR transition).
+ //
+ // End-to-end model:
+ // - server: `remote->ece' is the client's queried ECE;
+ // set it here, then after RTS we query the reduced/negotiated ECE
+ // and send it back in the server hello.
+ // - client: `remote->ece' is the server's reduced ECE;
+ // just set it here.
Review Comment:
The comment refers to `remote->ece`, but `remote` is a reference (not a pointer). This is confusing when reading the new end-to-end ECE logic.
##########
test/brpc_rdma_unittest.cpp:
##########
@@ -1750,6 +1751,161 @@ TEST_F(RdmaTest, v3_server_invalid_sq_size_falls_back) {
StopServer();
}
+// RAII guard to toggle FLAGS_rdma_ece for a single test and restore it.
+class EceFlagGuard {
+public:
+ explicit EceFlagGuard(bool v) : _saved(rdma::FLAGS_rdma_ece) {
+ rdma::FLAGS_rdma_ece = v;
+ }
+ ~EceFlagGuard() {
+ rdma::FLAGS_rdma_ece = _saved;
+ }
+private:
+ bool _saved;
+};
+
+// Build a valid v3 hello that also carries an ECE block.
+rdma::RdmaHello MakeValidV3HelloWithEce(uint32_t vendor_id,
+ uint32_t options,
+ uint32_t comp_mask) {
+ rdma::RdmaHello msg = MakeValidV3Hello();
+ rdma::RdmaEce* ece = msg.mutable_ece();
+ ece->set_vendor_id(vendor_id);
+ ece->set_options(options);
+ ece->set_comp_mask(comp_mask);
+ return msg;
+}
+
+// Read the server's v3 reply hello (4B magic + 4B pb_size + body) and parse
+// it into `reply`. Asserts the framing along the way.
+static void ReadServerV3Reply(int fd, rdma::RdmaHello* reply) {
+ uint8_t reply_hdr[8];
+ ASSERT_EQ(8, read(fd, reply_hdr, 8));
+ ASSERT_EQ(0, memcmp(reply_hdr, "RDM3", 4));
+ uint32_t reply_pb_size = butil::NetToHost32(*reinterpret_cast<uint32_t*>(reply_hdr + 4));
+ ASSERT_GT(reply_pb_size, 0u);
+ ASSERT_LE(reply_pb_size, 4096u);
+ std::string reply_body(reply_pb_size, '\0');
+ ASSERT_EQ((ssize_t)reply_pb_size,
+ read(fd, &reply_body[0], reply_pb_size));
+ ASSERT_TRUE(reply->ParseFromString(reply_body));
+}
Review Comment:
`ReadServerV3Reply` assumes `read()` returns the full requested byte count and uses an unaligned `reinterpret_cast<uint32_t*>` on `reply_hdr + 4`. On sockets, short reads are legal and the unaligned cast is UB on some architectures, which can make this test flaky or crash.
##########
src/brpc/rdma/rdma_endpoint.cpp:
##########
@@ -1215,18 +1216,22 @@ int RdmaEndpoint::BringUpQp(uint16_t lid, ibv_gid gid, uint32_t qp_num) {
return -1;
}
- if (FLAGS_rdma_ece) {
- struct ibv_ece ece;
- int err = IbvQueryEce(_resource->qp, &ece);
+ // ECE negotiation, done while the QP is in INIT state (must be set
+ // before the RTR transition).
+ //
+ // End-to-end model:
+ // - server: `remote->ece' is the client's queried ECE;
+ // set it here, then after RTS we query the reduced/negotiated ECE
+ // and send it back in the server hello.
+ // - client: `remote->ece' is the server's reduced ECE;
+ // just set it here.
+ // FLAGS_rdma_ece here.
Review Comment:
The standalone `// FLAGS_rdma_ece here.` comment looks like a leftover note and doesn’t describe any actual condition (the code is currently gated by `remote.ece` and `IbvSetEce`). Please remove or replace it with a precise explanation.
##########
src/brpc/rdma/rdma_endpoint.cpp:
##########
@@ -1283,6 +1288,20 @@ int RdmaEndpoint::BringUpQp(uint16_t lid, ibv_gid gid, uint32_t qp_num) {
return -1;
}
+ // On the server side, now that the QP reached RTS, query the reduced/negotiated
+ // ECE (the subset of enhancements supported by both peers) so it can be returned
+ // to the client in the server hello.
+ if (is_server && IbvQueryEce != NULL && remote.ece.has_value()) {
+ ibv_ece ece;
+ int qerr = IbvQueryEce(_resource->qp, &ece);
Review Comment:
If `IbvSetEce()` fails above, the code still proceeds to `IbvQueryEce()` and may advertise whatever the local QP reports as “negotiated” ECE. To match the “continue without ECE” intent, the server should only query/advertise negotiated ECE when `IbvSetEce()` succeeded (or otherwise track that ECE was actually enabled for this QP).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org