Copilot commented on code in PR #3303:
URL: https://github.com/apache/brpc/pull/3303#discussion_r3270903031
##########
src/brpc/rdma/block_pool.cpp:
##########
@@ -325,7 +327,7 @@ bool InitBlockPool(RegisterCallback cb) {
}
if (ExtendBlockPool(FLAGS_rdma_memory_pool_initial_size_mb,
- BLOCK_DEFAULT) != NULL) {
+ GetRdmaBlockType()) != NULL) {
return true;
Review Comment:
GetRdmaBlockType() can return -1 on invalid FLAGS_rdma_recv_block_type, but
InitBlockPool passes it directly into ExtendBlockPool(). This value is later
used as an index into g_block_size/locks/lists, leading to out-of-bounds access
and undefined behavior. Validate the returned type (e.g., in [0,
BLOCK_SIZE_COUNT)) and fail InitBlockPool with errno=EINVAL (or default to
BLOCK_DEFAULT) before calling ExtendBlockPool.
##########
src/brpc/rdma/block_pool.cpp:
##########
@@ -541,6 +543,34 @@ size_t GetBlockSize(int type) {
return g_block_size[type];
}
+size_t GetRdmaBlockSize() {
+ if (FLAGS_rdma_recv_block_type == "default") {
+ return GetBlockSize(0);
+ } else if (FLAGS_rdma_recv_block_type == "large") {
+ return GetBlockSize(1);
+ } else if (FLAGS_rdma_recv_block_type == "huge") {
+ return GetBlockSize(2);
+ } else {
+ LOG(ERROR) << "rdma_recv_block_type incorrect "
+ << "(valid value: default/large/huge)";
+ return -1;
Review Comment:
GetRdmaBlockSize() returns size_t but uses `return -1` on error. That wraps
to a huge positive size and can silently pass later checks/casts (e.g.,
subtracting headers, casting to uint32_t), producing bogus block sizes. Use a
signed type (ssize_t/int) or return 0/optional+errno on error so callers can
reliably detect invalid rdma_recv_block_type.
##########
src/butil/iobuf.cpp:
##########
@@ -42,6 +42,20 @@
#include "butil/iobuf_profiler.h"
namespace butil {
+static size_t default_block_size = 8192;
+
+size_t GetDefaultBlockSize() {
+ return default_block_size;
+}
+
+void SetDefaultBlockSize(size_t block_size) {
+ if (block_size / 4096 * 4096 != block_size) {
+ LOG(FATAL) << "block_size " << block_size << " should be multiply of
4096!!!";
+ }
+ LOG(INFO) << "Update default_block_size from " << default_block_size << "
to " << block_size;
+ default_block_size = block_size;
Review Comment:
default_block_size is a mutable global accessed by
GetDefaultBlockSize()/SetDefaultBlockSize() without synchronization. Since
SetDefaultBlockSize is public, concurrent reads/writes create a data race (UB).
Consider making it an atomic (relaxed is likely enough) and/or clearly
documenting/enforcing that SetDefaultBlockSize must only be called during
single-threaded initialization.
##########
src/butil/iobuf.cpp:
##########
@@ -42,6 +42,20 @@
#include "butil/iobuf_profiler.h"
namespace butil {
+static size_t default_block_size = 8192;
+
+size_t GetDefaultBlockSize() {
+ return default_block_size;
+}
+
+void SetDefaultBlockSize(size_t block_size) {
+ if (block_size / 4096 * 4096 != block_size) {
+ LOG(FATAL) << "block_size " << block_size << " should be multiply of
4096!!!";
+ }
Review Comment:
SetDefaultBlockSize() should validate more than 4KB alignment: reject 0 and
sizes smaller than sizeof(IOBuf::Block), and cap to <= 0xFFFFFFFF to avoid
later LOG(FATAL) in create_block(). Also, the error message says
"multiply"/"!!!"; consider using clearer wording ("multiple") and avoid
excessive punctuation.
##########
src/brpc/rdma/block_pool.h:
##########
@@ -100,6 +100,10 @@ uint32_t GetRegionId(const void* buf);
// type=3: BLOCK_HUGE(2MB)
size_t GetBlockSize(int type);
+size_t GetRdmaBlockSize();
+
+int GetRdmaBlockType();
Review Comment:
The comment for GetBlockSize() documents type values as 1/2/3, but the
implementation uses 0/1/2 (BLOCK_DEFAULT=0, etc.). Update the comment (or the
API) so callers don't pass an off-by-one type and index g_block_size
incorrectly.
##########
src/butil/iobuf.h:
##########
@@ -52,6 +52,10 @@ struct ssl_st;
namespace butil {
+size_t GetDefaultBlockSize();
+
+void SetDefaultBlockSize(size_t block_size);
+
Review Comment:
Removing the public constant IOBuf::DEFAULT_BLOCK_SIZE is a source-breaking
API change for downstream users (even if internally migrated). Consider keeping
a deprecated alias (e.g., kDefaultBlockSize=8192) or providing a class-scoped
accessor to preserve compatibility while enabling runtime configuration via
Get/SetDefaultBlockSize().
##########
src/brpc/rdma/rdma_helper.cpp:
##########
@@ -550,6 +550,7 @@ static void GlobalRdmaInitializeOrDieImpl() {
}
// Initialize RDMA memory pool (block_pool)
+ butil::SetDefaultBlockSize(GetRdmaBlockSize());
if (!InitBlockPool(RdmaRegisterMemory)) {
PLOG(ERROR) << "Fail to initialize RDMA memory pool";
Review Comment:
Calling SetDefaultBlockSize(GetRdmaBlockSize()) before validating
rdma_recv_block_type means an invalid flag can trigger LOG(FATAL) (or set an
unusable size) during initialization, and it globally changes IOBuf allocation
behavior for the whole process. Consider validating the block type/size first
and failing gracefully (or limiting the scope of the change to RDMA-specific
buffers) before updating the global default.
--
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]
