Copilot commented on code in PR #3137:
URL: https://github.com/apache/brpc/pull/3137#discussion_r2493597474
##########
src/brpc/rdma/rdma_endpoint.cpp:
##########
@@ -904,6 +915,13 @@ ssize_t RdmaEndpoint::CutFromIOBufList(butil::IOBuf**
from, size_t ndata) {
return total_len;
}
+struct WrIdGen {
+ bool init;
+ uint32_t seq;
+ uint64_t current_random;
+ butil::FastRandSeed seed;
+};
+
Review Comment:
The `WrIdGen` struct is defined but never used in the code. This appears to
be dead code that should either be removed or integrated if it was intended to
be part of the implementation.
```suggestion
```
##########
src/brpc/rdma/rdma_endpoint.h:
##########
@@ -261,6 +261,10 @@ friend class brpc::Socket;
butil::atomic<uint16_t> _window_size;
// The number of new WRs posted in the local Recv Queue
butil::atomic<uint16_t> _new_rq_wrs;
+ // The number of pending signaled send WRs
+ butil::atomic<uint16_t> _pending_signaled_wrs;
+ // The number of pending acks
+ uint16_t _pending_acks;
Review Comment:
The `_pending_acks` member is not thread-safe (plain uint16_t instead of
atomic). In polling mode, multiple poller threads can call `HandleCompletion`
concurrently for the same endpoint, leading to race conditions on lines 997,
1016, and 1018 where this variable is read and written without synchronization.
```suggestion
butil::atomic<uint16_t> _pending_acks;
```
##########
src/brpc/rdma/rdma_endpoint.cpp:
##########
@@ -936,9 +955,13 @@ int RdmaEndpoint::SendImm(uint32_t imm) {
ssize_t RdmaEndpoint::HandleCompletion(ibv_wc& wc) {
bool zerocopy = FLAGS_rdma_recv_zerocopy;
+ uint16_t pending_signaled_wrs = 2;
Review Comment:
The variable `pending_signaled_wrs` is initialized to 2, which appears to be
a magic number. This initialization value doesn't reflect the actual number of
pending signaled WRs and relies on being overwritten in the IBV_WC_SEND case.
Consider initializing it to the actual value from `_pending_signaled_wrs` or
using a constant with a descriptive name to clarify its purpose as a sentinel
value.
```suggestion
uint16_t pending_signaled_wrs =
_pending_signaled_wrs.load(butil::memory_order_relaxed);
```
--
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]
