wwbmmm commented on code in PR #3144:
URL: https://github.com/apache/brpc/pull/3144#discussion_r4245003091
##########
src/brpc/rdma_transport.cpp:
##########
@@ -30,10 +30,25 @@ DECLARE_bool(usercode_in_pthread);
extern SocketVarsCollector *g_vars;
-void RdmaTransport::Init(Socket *socket, const SocketOptions &options) {
+void RdmaTransport::DoInit(Socket *socket, const SocketOptions &options, bool
use_gdr) {
CHECK(_rdma_ep == nullptr);
- if (options.socket_mode == SOCKET_MODE_RDMA) {
- _rdma_ep = new rdma::RdmaEndpoint(socket);
+ // gdr mode is a special mode of rdma mode.
+ // both rdma mode and gdr mode need init rdma::RdmaEndpoint.
+ if (options.socket_mode == SOCKET_MODE_RDMA ||
+ options.socket_mode == SOCKET_MODE_GDR) {
+ _rdma_ep = new(std::nothrow)rdma::RdmaEndpoint(socket, use_gdr);
+ if (!_rdma_ep) {
+ const int saved_errno = errno;
+ PLOG(ERROR) << "Fail to create RdmaEndpoint";
+ socket->SetFailed(
+ saved_errno, "Fail to create RdmaEndpoint: %s",
berror(saved_errno));
+ // No endpoint: later code paths must not assume RDMA is active
+ // and dereference _rdma_ep. Mark the transport as not using RDMA.
+ _rdma_state = RDMA_OFF;
+ _socket = socket;
+ _default_connect = options.app_connect;
+ return;
Review Comment:
[replied by brpc-oncall robot] This early return happens before
`_on_edge_trigger` is set and before `_tcp_transport` is created/initialized,
while `_rdma_state` is set to `RDMA_OFF`.
`RdmaTransport::CutFromIOBuf`/`CutFromIOBufList`/`WaitEpollOut` dereference
`_tcp_transport` unconditionally, so any later call on this transport would
crash. Even though the socket was just `SetFailed`, please leave the transport
in a fully initialized state (e.g. fall through to the common tail of `DoInit`)
rather than returning in the middle.
##########
src/brpc/acceptor.h:
##########
@@ -135,7 +135,7 @@ friend class Server;
bool _force_ssl;
std::shared_ptr<SocketSSLContext> _ssl_ctx;
- // Choose to use a certain socket: 0 TCP, 1 RDMA
+ // Choose to use a certain socket: 0 TCP, 1 RDMA, 2 UBRING, 3 GDR
Review Comment:
[replied by brpc-oncall robot] The merge-conflict resolution reverted this
comment to `2 UBRING, 3 GDR`, but the enum is `SOCKET_MODE_URMA = 3,
SOCKET_MODE_GDR = 4` (see `src/brpc/socket_mode.h`). Please restore `0 TCP, 1
RDMA, 2 UBRING, 3 URMA, 4 GDR` so it stays in sync with the enum.
##########
src/brpc/rdma/rdma_helper.cpp:
##########
@@ -588,7 +593,41 @@ static void GlobalRdmaInitializeOrDieImpl() {
g_rdma_available.store(true, butil::memory_order_relaxed);
}
+static void GlobalGdrInitializeOrDieImpl() {
+#if BRPC_WITH_GDR
+ // GPUDirect RDMA requires all received data to be written into the GPU
+ // memory buffer specified at post-receive time. When MLX5_SCATTER_TO_CQE
+ // is enabled, small messages are inlined directly into host memory, which
+ // breaks this requirement and causes operations to fail. Force it to 0
+ // regardless of the user-provided value.
+ if (setenv("MLX5_SCATTER_TO_CQE", "0", 1) != 0) {
Review Comment:
[replied by brpc-oncall robot] `MLX5_SCATTER_TO_CQE` is set here, i.e. after
the RDMA context has already been created: `TransportFactory::ContextInitOrDie`
calls `RdmaTransport::ContextInitOrDie()` (which calls
`GlobalRdmaInitializeOrDie()` → `ibv_open_device`) before
`GdrContextInitOrDie()`, and the example also calls
`GlobalRdmaInitializeOrDie()` first. libmlx5 reads the `MLX5_*` environment
variables when the device context is created, so setting it after
`ibv_open_device` has no effect. Please move it before the RDMA global
initialization (or drop it and keep the value as an externally documented
prerequisite).
--
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]
