You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brpc.apache.org by "yanglimingcn (via GitHub)" <gi...@apache.org> on 2025/11/04 07:25:26 UTC
[PR] add pending signaled wrs checking (brpc)
yanglimingcn opened a new pull request, #3137:
URL: https://github.com/apache/brpc/pull/3137
### What problem does this PR solve?
Issue Number: #3132
Problem Summary:
### What is changed and the side effects?
Changed:
Side effects:
- Performance effects:
- Breaking backward compatibility:
---
### Check List:
- Please make sure your changes are compilable.
- When providing us with a new feature, it is best to add related tests.
- Please follow [Contributor Covenant Code of Conduct](https://github.com/apache/brpc/blob/master/CODE_OF_CONDUCT.md).
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] add pending signaled wrs checking (brpc)
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] add pending signaled wrs checking (brpc)
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3137:
URL: https://github.com/apache/brpc/pull/3137#discussion_r2492591693
##########
src/brpc/rdma/rdma_endpoint.cpp:
##########
@@ -938,7 +971,9 @@ ssize_t RdmaEndpoint::HandleCompletion(ibv_wc& wc) {
bool zerocopy = FLAGS_rdma_recv_zerocopy;
switch (wc.opcode) {
case IBV_WC_SEND: { // send completion
- // Do nothing
+ if (wc.wr_id == 0) {
Review Comment:
The condition checks if `wc.wr_id == 0`, but `GenerateWrId()` explicitly generates non-zero IDs (comment at line 930 states '0 is an invalid Id'). The signaled WRs in `CutFromIOBufList` at line 880 don't set `wr.wr_id`, leaving it as 0 after `memset` at line 808. This logic appears inverted - it should decrement when `wc.wr_id != 0` (for SendImm) or the counter should be incremented for signaled sends in `CutFromIOBufList` that have `wr_id == 0`.
```suggestion
if (wc.wr_id != 0) {
```
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] add pending signaled wrs checking (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on code in PR #3137:
URL: https://github.com/apache/brpc/pull/3137#discussion_r2507621498
##########
src/brpc/rdma/rdma_endpoint.cpp:
##########
@@ -995,6 +1004,17 @@ ssize_t RdmaEndpoint::HandleCompletion(ibv_wc& wc) {
<< wc.opcode;
return -1;
}
+ if (pending_signaled_wrs <= 0) {
Review Comment:
It seems that _window_size may still be larger than the actual sq size.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] add pending signaled wrs checking (brpc)
Posted by "yanglimingcn (via GitHub)" <gi...@apache.org>.
yanglimingcn closed pull request #3137: add pending signaled wrs checking
URL: https://github.com/apache/brpc/pull/3137
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] add pending signaled wrs checking (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on code in PR #3137:
URL: https://github.com/apache/brpc/pull/3137#discussion_r2492631751
##########
src/brpc/rdma/rdma_endpoint.cpp:
##########
@@ -972,10 +1007,12 @@ ssize_t RdmaEndpoint::HandleCompletion(ibv_wc& wc) {
// Update window
uint32_t wnd_thresh = _local_window_capacity / 8;
- if (_window_size.fetch_add(acks, butil::memory_order_relaxed) >= wnd_thresh
- || acks >= wnd_thresh) {
+ if (_pending_signaled_wrs.load(butil::memory_order_relaxed) <= 1 &&
Review Comment:
When _pending_signaled_wrs is greater than 1, _window_size is not updated, and acks will be lost.
##########
src/brpc/rdma/rdma_endpoint.cpp:
##########
@@ -918,6 +950,7 @@ int RdmaEndpoint::SendImm(uint32_t imm) {
ibv_send_wr wr;
memset(&wr, 0, sizeof(wr));
+ wr.wr_id = GenerateWrId();
Review Comment:
Would it be sufficient to set `wr_id` to a fixed value for `Imm`?
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] add pending signaled wrs checking (brpc)
Posted by "yanglimingcn (via GitHub)" <gi...@apache.org>.
yanglimingcn commented on code in PR #3137:
URL: https://github.com/apache/brpc/pull/3137#discussion_r2510099622
##########
src/brpc/rdma/rdma_endpoint.cpp:
##########
@@ -995,6 +1004,17 @@ ssize_t RdmaEndpoint::HandleCompletion(ibv_wc& wc) {
<< wc.opcode;
return -1;
}
+ if (pending_signaled_wrs <= 0) {
Review Comment:
Could you give an example to describe it?
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] add pending signaled wrs checking (brpc)
Posted by "yanglimingcn (via GitHub)" <gi...@apache.org>.
yanglimingcn commented on PR #3137:
URL: https://github.com/apache/brpc/pull/3137#issuecomment-3514740421
> I think we should use two windows: one representing the local sending window size and the other representing the remote receiving window size. Sending should only occur if both window values are greater than 0.
>
> See #3145 . We encountered the same problem, and the above method solved it.
Yes, I think your PR is better.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] add pending signaled wrs checking (brpc)
Posted by "yanglimingcn (via GitHub)" <gi...@apache.org>.
yanglimingcn commented on code in PR #3137:
URL: https://github.com/apache/brpc/pull/3137#discussion_r2493536948
##########
src/brpc/rdma/rdma_endpoint.cpp:
##########
@@ -889,6 +896,10 @@ ssize_t RdmaEndpoint::CutFromIOBufList(butil::IOBuf** from, size_t ndata) {
return -1;
}
+ if (signaled) {
Review Comment:
this line, I'm not sure if send cqe can generate energy after this atomic operation.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] add pending signaled wrs checking (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on code in PR #3137:
URL: https://github.com/apache/brpc/pull/3137#discussion_r2512867233
##########
src/brpc/rdma/rdma_endpoint.cpp:
##########
@@ -995,6 +1004,17 @@ ssize_t RdmaEndpoint::HandleCompletion(ibv_wc& wc) {
<< wc.opcode;
return -1;
}
+ if (pending_signaled_wrs <= 0) {
Review Comment:
After the client sends _local_window_capacity / 4 + 1 requests, it receives an ACK and a send CQE from the server. At this point, _window_size increases by _local_window_capacity / 4 + 1, and sq size increases by _local_window_capacity / 4.
The issue of inconsistency between _window_size and sq size still exists.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] add pending signaled wrs checking (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on code in PR #3137:
URL: https://github.com/apache/brpc/pull/3137#discussion_r2492631751
##########
src/brpc/rdma/rdma_endpoint.cpp:
##########
@@ -972,10 +1007,12 @@ ssize_t RdmaEndpoint::HandleCompletion(ibv_wc& wc) {
// Update window
uint32_t wnd_thresh = _local_window_capacity / 8;
- if (_window_size.fetch_add(acks, butil::memory_order_relaxed) >= wnd_thresh
- || acks >= wnd_thresh) {
+ if (_pending_signaled_wrs.load(butil::memory_order_relaxed) <= 1 &&
Review Comment:
When `_pending_signaled_wrs` is greater than 1, `_window_size` is not updated, and acks will be lost.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] add pending signaled wrs checking (brpc)
Posted by "yanglimingcn (via GitHub)" <gi...@apache.org>.
yanglimingcn commented on code in PR #3137:
URL: https://github.com/apache/brpc/pull/3137#discussion_r2493482119
##########
src/brpc/rdma/rdma_endpoint.cpp:
##########
@@ -918,6 +950,7 @@ int RdmaEndpoint::SendImm(uint32_t imm) {
ibv_send_wr wr;
memset(&wr, 0, sizeof(wr));
+ wr.wr_id = GenerateWrId();
Review Comment:
Maybe, just use a fixed value
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org