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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
