Copilot commented on code in PR #3145:
URL: https://github.com/apache/brpc/pull/3145#discussion_r2507715252
##########
src/brpc/rdma/rdma_endpoint.cpp:
##########
@@ -102,6 +102,12 @@ static const uint32_t ACK_MSG_RDMA_OK = 0x1;
static butil::Mutex* g_rdma_resource_mutex = NULL;
static RdmaResource* g_rdma_resource_list = NULL;
+enum SendType {
+ SEND_TYPE_RDMA_DATA = 0,
+ SEND_TYPE_RDMA_IMM,
+};
+
+
Review Comment:
[nitpick] Extra blank line detected. Only one blank line is needed between
the enum definition and the struct definition.
```suggestion
```
##########
src/brpc/rdma/rdma_endpoint.cpp:
##########
@@ -894,11 +913,14 @@ ssize_t RdmaEndpoint::CutFromIOBufList(butil::IOBuf**
from, size_t ndata) {
_sq_current = 0;
}
- // Update _window_size. Note that _window_size will never be negative.
+ // Update `_remote_rq_window_size' and `_sq_window_size'. Note that
+ // `_remote_rq_window_size' and `_sq_window_size' will never be
negative.
// Because there is at most one thread can enter this function for each
// Socket, and the other thread of HandleCompletion can only add this
// counter.
Review Comment:
The comment refers to "this counter" but the code now updates two counters
(`_remote_rq_window_size` and `_sq_window_size`). The comment should be updated
to use the plural form:
```cpp
// Because there is at most one thread can enter this function for each
// Socket, and the other thread of HandleCompletion can only add these
// counters.
```
```suggestion
// Socket, and the other thread of HandleCompletion can only add
these
// counters.
```
##########
src/brpc/rdma/rdma_endpoint.cpp:
##########
@@ -227,7 +234,7 @@ void RdmaEndpoint::Reset() {
_sq_unsignaled = 0;
_local_window_capacity = 0;
_remote_window_capacity = 0;
- _window_size.store(0, butil::memory_order_relaxed);
+ _remote_rq_window_size.store(0, butil::memory_order_relaxed);
Review Comment:
The newly introduced `_sq_window_size` atomic variable is not being reset to
0 in the `Reset()` function. Since `_remote_rq_window_size` is reset on line
237, `_sq_window_size` should also be reset here to maintain consistency and
prevent stale values when the endpoint is reused.
Add the following line after line 237:
```cpp
_sq_window_size.store(0, butil::memory_order_relaxed);
```
```suggestion
_remote_rq_window_size.store(0, butil::memory_order_relaxed);
_sq_window_size.store(0, butil::memory_order_relaxed);
```
##########
src/brpc/rdma/rdma_endpoint.cpp:
##########
@@ -950,8 +981,7 @@ ssize_t RdmaEndpoint::HandleCompletion(ibv_wc& wc) {
CHECK(_state != FALLBACK_TCP);
if (zerocopy) {
butil::IOBuf tmp;
Review Comment:
The local variable `tmp` on line 983 is no longer used after the
optimization on line 984 that directly cuts into `_socket->_read_buf`. This
unused variable declaration should be removed.
```suggestion
```
--
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]
