Copilot commented on code in PR #3155:
URL: https://github.com/apache/brpc/pull/3155#discussion_r2536140222
##########
src/brpc/rdma/block_pool.cpp:
##########
@@ -222,24 +203,19 @@ static void* ExtendBlockPool(size_t region_size, int
block_type) {
return ExtendBlockPoolImpl(region_base, region_size, block_type);
}
-void* ExtendBlockPoolByUser(void* region_base, size_t region_size,
- int block_type) {
- if (FLAGS_rdma_memory_pool_user_specified_memory == false) {
- LOG_EVERY_SECOND(ERROR) << "User extend memory is disabled";
- return NULL;
- }
+void* ExtendBlockPoolByUser(void* region_base, size_t region_size, int
block_type) {
Review Comment:
Documentation: The function comment for `ExtendBlockPoolByUser` (lines 78-84
in block_pool.h) mentions that `FLAGS_rdma_memory_pool_user_specified_memory`
must be true, but this restriction has been removed in this PR. The
documentation should be updated to reflect that this function can now be called
regardless of the flag value.
##########
src/brpc/rdma/block_pool.cpp:
##########
@@ -557,7 +546,8 @@ void DumpMemoryPoolInfo(std::ostream& os) {
for (int i = 0; i < BLOCK_SIZE_COUNT; ++i) {
os << "\tFor block size " << GetBlockSize(i) << ":\n";
for (size_t j = 0; j < g_buckets; ++j) {
- os << "\t\tBucket " << j << ": " << g_info->idle_size[i][j] <<
"\n";
+ os << "\t\tBucket " << j << ": {" << g_info->idle_size[i][j]
+ << ", " << g_info->expansion_list[i][j] << "}\n";
Review Comment:
Incorrect format: The output is printing a pointer address
`g_info->expansion_list[i][j]` instead of the actual expansion size
`g_info->expansion_size[i][j]`. This should be `g_info->expansion_size[i][j]`
to show the size of memory in the expansion list, not the pointer address.
```suggestion
<< ", " << g_info->expansion_size[i][j] << "}\n";
```
##########
src/brpc/rdma/block_pool.cpp:
##########
@@ -129,36 +130,20 @@ uint32_t GetRegionId(const void* buf) {
return r->id;
}
-// When both rdma_memory_pool_max_regions and rdma_memory_pool_buckets are
-// greater than 1, dynamic memory expansion may cause concurrent modification
-// issues in the memory linked list due to lock contention problems. To address
-// this, we increase the region_num count for each block_type. Dynamic memory
-// expansion is only permitted when both of the following conditions are met:
-// rdma_memory_pool_buckets equals 1
-// g_info->region_num[block_type] is less than 1
-static bool CanExtendBlockRuntime(int block_type) {
- return FLAGS_rdma_memory_pool_buckets == 1 ||
- g_info->region_num[block_type] < 1;
-}
-
-static void* ExtendBlockPoolImpl(void* region_base, size_t region_size,
- int block_type) {
- if (CanExtendBlockRuntime(block_type) == false) {
- LOG(INFO) << "Runtime extend memory only support one bucket or region "
- "num is zero for per block_type";
+static void* ExtendBlockPoolImpl(void* region_base, size_t region_size, int
block_type) {
+ auto region_base_guard = butil::MakeScopeGuard([region_base]() {
free(region_base);
- errno = ENOMEM;
- return NULL;
- }
+ });
+
if (g_region_num == FLAGS_rdma_memory_pool_max_regions) {
- LOG(INFO) << "Memory pool reaches max regions";
- free(region_base);
+ PLOG_EVERY_SECOND(ERROR) << "Memory pool reaches max regions";
Review Comment:
Incorrect log macro: `PLOG_EVERY_SECOND` is used for system error logging
(with errno), but this error is not related to a system call failure. The
message "Memory pool reaches max regions" is a business logic error, not a
system error. Use `LOG_EVERY_SECOND(ERROR)` instead of
`PLOG_EVERY_SECOND(ERROR)`.
```suggestion
LOG_EVERY_SECOND(ERROR) << "Memory pool reaches max regions";
```
##########
src/brpc/rdma/block_pool.cpp:
##########
@@ -40,7 +39,7 @@ DEFINE_int32(rdma_memory_pool_max_regions, 3, "Max number of
regions");
DEFINE_int32(rdma_memory_pool_buckets, 4, "Number of buckets to reduce race");
DEFINE_int32(rdma_memory_pool_tls_cache_num, 128, "Number of cached block in
tls");
DEFINE_bool(rdma_memory_pool_user_specified_memory, false,
- "If true, the user must call UserExtendBlockPool() to extend "
+ "[DEPRECATED]If true, the user must call UserExtendBlockPool() to
extend "
"memory. bRPC will not handle memory extension.");
Review Comment:
[nitpick] Documentation clarity: The deprecation notice "[DEPRECATED]" is
added but the flag is still functional and used in line 308. If the flag is
truly deprecated, consider adding information about what should be used instead
or when it will be removed. Alternatively, if dynamic expansion now works
correctly with this flag, explain why it's deprecated.
```suggestion
"[DEPRECATED] This flag is deprecated because dynamic memory
pool expansion "
"is now handled automatically by bRPC. Users should no longer
call "
"UserExtendBlockPool(); instead, rely on bRPC to manage memory
extension. "
"This flag will be removed in a future release.");
```
##########
src/brpc/rdma/block_pool.cpp:
##########
@@ -222,24 +203,19 @@ static void* ExtendBlockPool(size_t region_size, int
block_type) {
return ExtendBlockPoolImpl(region_base, region_size, block_type);
}
-void* ExtendBlockPoolByUser(void* region_base, size_t region_size,
- int block_type) {
- if (FLAGS_rdma_memory_pool_user_specified_memory == false) {
- LOG_EVERY_SECOND(ERROR) << "User extend memory is disabled";
- return NULL;
- }
+void* ExtendBlockPoolByUser(void* region_base, size_t region_size, int
block_type) {
if (reinterpret_cast<uintptr_t>(region_base) % 4096 != 0) {
LOG_EVERY_SECOND(ERROR) << "region_base must be 4096 aligned";
+ free(region_base);
Review Comment:
Incorrect error handling: When `region_base` is not aligned, the code calls
`free(region_base)`. However, if the caller allocated this memory using a
custom allocator (e.g., hugepages), calling `free()` may be incorrect. The
caller should be responsible for freeing their own memory. Consider removing
the `free(region_base)` call here.
```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]
