Copilot commented on code in PR #3144:
URL: https://github.com/apache/brpc/pull/3144#discussion_r4327545951
##########
example/rdma_performance/client.cpp:
##########
@@ -278,6 +322,12 @@ int main(int argc, char* argv[]) {
if (FLAGS_use_rdma) {
brpc::rdma::GlobalRdmaInitializeOrDie();
}
+#ifdef BRPC_WITH_GDR
+ else if (FLAGS_use_gdr) {
+ brpc::rdma::GlobalRdmaInitializeOrDie();
+ brpc::rdma::GlobalGdrInitializeOrDie();
+ }
Review Comment:
When `--use_gdr=true` and the default `--use_rdma=true`, the first branch
runs and GDR is never initialized, even though the code later switches the
channel/socket mode to `SOCKET_MODE_GDR`. Make `use_gdr` imply (and include)
the GDR init path (e.g., prioritize `use_gdr` over `use_rdma`, or call
`GlobalGdrInitializeOrDie()` whenever `use_gdr` is set).
##########
src/brpc/policy/baidu_rpc_protocol_gpu.cpp:
##########
@@ -0,0 +1,244 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#if BRPC_WITH_GDR
+
+#include <google/protobuf/descriptor.h> // MethodDescriptor
+#include <google/protobuf/message.h> // Message
+#include <google/protobuf/io/zero_copy_stream_impl_lite.h>
+#include <google/protobuf/io/coded_stream.h>
+#include <google/protobuf/text_format.h>
+
+#include "butil/gpu/gpu_block_pool.h"
+#include "butil/iobuf.h" // butil::IOBuf
+#include "butil/logging.h" // LOG()
+#include "butil/memory/scope_guard.h"
+#include "butil/raw_pack.h" // RawPacker RawUnpacker
+#include "butil/strings/string_util.h"
+
+#include "json2pb/json_to_pb.h"
+#include "json2pb/pb_to_json.h"
+#include "brpc/controller.h" // Controller
+#include "brpc/socket.h" // Socket
+#include "brpc/server.h" // Server
+#include "brpc/span.h"
+#include "brpc/compress.h" // ParseFromCompressedData
+#include "brpc/checksum.h"
+#include "brpc/stream_impl.h"
+#include "brpc/rpc_dump.h" // SampledRequest
+#include "brpc/rpc_pb_message_factory.h"
+#include "brpc/policy/baidu_rpc_meta.pb.h" // RpcRequestMeta
+#include "brpc/policy/baidu_rpc_protocol.h"
+#include "brpc/policy/most_common_message.h"
+#include "brpc/policy/streaming_rpc_protocol.h"
+#include "brpc/details/usercode_backup_pool.h"
+#include "brpc/details/controller_private_accessor.h"
+#include "brpc/details/server_private_accessor.h"
+
+namespace brpc {
+namespace policy {
+
+// Notes:
+// 1. 12-byte header [PRPC][body_size][meta_size]
+// 2. body_size and meta_size are in network byte order
+// 3. Use service->full_name() + method_name to specify the method to call
+// 4. `attachment_size' is set iff request/response has attachment
+// 5. Not supported: chunk_info
+
+// Pack header into `buf'
+
+const int header_size = 12;
+// if we recv data into gpu, the header/meta/body will be copied to cpu and
processed.
+// in to to limit the count of d2h, we will prefetch 512B from gpu to cpu.
+// if header_size + meta_size + body_size(without attachment) is less than
512, then one
+// d2h is enough for one rpc.
+
+const int prefetch_d2h_size = 512;
Review Comment:
`prefetch_d2h_size` is hard-coded to 512 and implicitly tied to the CPU
block pool block size (also set to 512 elsewhere). If either changes
independently, host allocations can start failing (single-block limit) or the
optimization becomes ineffective. Consider deriving `prefetch_d2h_size` from
`host_allocator->get_block_size()` (or making it a gflag) so the prefetch size
and allocator constraints stay consistent.
##########
src/brpc/rdma/rdma_helper.cpp:
##########
@@ -474,6 +479,20 @@ static ibv_context* OpenDevice(int num_total, int*
num_available_devices) {
}
static void GlobalRdmaInitializeOrDieImpl() {
+#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. libmlx5 reads
+ // MLX5_* environment variables when the device context is created (i.e.
+ // inside ibv_open_device below), so it must be forced to 0 before any
+ // device is opened, regardless of the user-provided value.
+ if (setenv("MLX5_SCATTER_TO_CQE", "0", 1) != 0) {
+ PLOG(ERROR) << "Fail to set MLX5_SCATTER_TO_CQE to 0";
+ ExitWithError();
+ }
+#endif // if BRPC_WITH_GDR
Review Comment:
This forces `MLX5_SCATTER_TO_CQE=0` for *all* processes built with
`BRPC_WITH_GDR`, even when running plain RDMA mode (or when users explicitly
configured the env var). If GDR correctness requires the env var only when GDR
is actually used, consider adding an explicit runtime switch (gflag) that
enables this behavior and ensure the GDR init path is invoked early enough to
set it before `ibv_open_device` (e.g., a combined 'RDMA-for-GDR' init
entrypoint).
--
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]
