wwbmmm commented on code in PR #3626:
URL: https://github.com/apache/brpc/pull/3626#discussion_r4322644057
##########
src/brpc/urma/urma_endpoint.cpp:
##########
@@ -952,6 +952,23 @@ ssize_t UrmaEndpoint::HandleCompletion(const urma_cr_t&
cr) {
uint16_t wnd = 1; // We signal every WR (complete_enable=1).
uint16_t old =
_sq_window_size.load(butil::memory_order_relaxed);
+ if (old >= _local_window_capacity) {
Review Comment:
[replied by brpc-oncall robot] This capacity check plus its warning is now
duplicated: the same check is still inside the `while (true)` CAS loop below.
The in-loop branch is now only reachable after the buffer has already been
cleared and `_sq_sent` advanced, in which case the credit is dropped while the
state was already mutated. Since `_sq_window_size` is only incremented here
(writers only decrement it), consider doing the single pre-check and then
publishing with `_sq_window_size.fetch_add(wnd, butil::memory_order_relaxed)`
after the barrier, removing the now-redundant retry loop so reclamation and
credit publication stay in one place.
##########
test/brpc_urma_unittest.cpp:
##########
@@ -41,6 +43,91 @@ extern butil::atomic<bool> g_urma_available;
} // namespace urma
} // namespace brpc
+class UrmaSendCompletionTest : public ::testing::Test {
+protected:
+ void SetUp() override {
+ SocketOptions options;
+ SocketId id;
+ ASSERT_EQ(0, Socket::Create(options, &id));
+ ASSERT_EQ(0, Socket::Address(id, &_socket));
+ }
+
+ void TearDown() override {
+ if (_socket) {
+ _socket->SetFailed();
+ }
+ }
+
+ static urma_cr_t SendCompletion() {
+ urma_cr_t cr{};
+ cr.status = URMA_CR_SUCCESS;
+ cr.user_ctx = 1;
+ return cr;
+ }
+
+ SocketUniquePtr _socket;
+};
+
+TEST_F(UrmaSendCompletionTest, reclaim_buffer_before_publishing_credit) {
+ char payload = 'x';
+ bool released = false;
+ urma::UrmaEndpoint endpoint(_socket.get());
+ endpoint._sbuf.resize(endpoint._sq_size -
Review Comment:
[replied by brpc-oncall robot] These tests cannot compile when the tree is
built with `WITH_URMA=ON`: `_sbuf`, `_sq_size`, `_sq_sent`, `_sq_window_size`,
`_local_window_capacity` and `HandleCompletion()` are all private in
`UrmaEndpoint` (see `src/brpc/urma/urma_endpoint.h`: everything after the
`private:` section that starts at the `State` enum), and the fixture is not
declared as a friend. CI does not catch this because
`test/brpc_urma_unittest.cpp` compiles to an empty translation unit unless
`BRPC_WITH_URMA` is 1. Please grant access explicitly, e.g. add `#include
<gtest/gtest_prod.h>` plus `FRIEND_TEST(UrmaSendCompletionTest,
reclaim_buffer_before_publishing_credit);` (and the other two test names) in
`UrmaEndpoint`, or expose a small test-only accessor, so that the URMA
unit-test build actually compiles.
--
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]
