Copilot commented on code in PR #3155:
URL: https://github.com/apache/brpc/pull/3155#discussion_r2564084084
##########
src/brpc/rdma/block_pool.cpp:
##########
@@ -332,66 +331,74 @@ bool InitBlockPool(RegisterCallback cb) {
return false;
}
+static void MoveExpansionList2EmptyIdleList(int block_type, size_t index) {
+ CHECK(NULL == g_info->idle_list[block_type][index]);
+
+ g_info->idle_list[block_type][index] =
g_info->expansion_list[block_type][index];
+ g_info->idle_size[block_type][index] +=
g_info->expansion_size[block_type][index];
+ g_info->expansion_list[block_type][index] = NULL;
+ g_info->expansion_size[block_type][index] = 0;
+}
+
static void* AllocBlockFrom(int block_type) {
bool locked = false;
if (BAIDU_UNLIKELY(g_dump_enable)) {
g_dump_mutex->lock();
locked = true;
}
+ BUTIL_SCOPE_EXIT {
+ if (locked) {
+ g_dump_mutex->unlock();
+ }
+ };
+
void* ptr = NULL;
- if (block_type == 0 && tls_idle_list != NULL){
+ if (0 == block_type && NULL != tls_idle_list) {
CHECK(tls_idle_num > 0);
IdleNode* n = tls_idle_list;
tls_idle_list = n->next;
ptr = n->start;
butil::return_object<IdleNode>(n);
tls_idle_num--;
- if (locked) {
- g_dump_mutex->unlock();
- }
return ptr;
}
- uint64_t index = butil::fast_rand() % g_buckets;
+ size_t index = butil::fast_rand() % g_buckets;
BAIDU_SCOPED_LOCK(*g_info->lock[block_type][index]);
IdleNode* node = g_info->idle_list[block_type][index];
- if (!node) {
+ if (NULL == node) {
BAIDU_SCOPED_LOCK(g_info->extend_lock);
node = g_info->idle_list[block_type][index];
- if (!node) {
- // There is no block left, extend a new region
- if (!ExtendBlockPool(FLAGS_rdma_memory_pool_increase_size_mb,
- block_type)) {
+ if (NULL == node && NULL != g_info->expansion_list[block_type][index])
{
+ MoveExpansionList2EmptyIdleList(block_type, index);
+ node = g_info->idle_list[block_type][index];
+ }
+ if (NULL == node) {
+ // There is no block left, extend a new region.
+ if (!ExtendBlockPool(FLAGS_rdma_memory_pool_increase_size_mb,
block_type)) {
LOG_EVERY_SECOND(ERROR) << "Fail to extend new region. "
<< "You can set the size of memory
pool larger. "
<< "Refer to the help message of these
flags: "
<< "rdma_memory_pool_initial_size_mb, "
<< "rdma_memory_pool_increase_size_mb,
"
<< "rdma_memory_pool_max_regions.";
- if (locked) {
- g_dump_mutex->unlock();
- }
return NULL;
}
+ MoveExpansionList2EmptyIdleList(block_type, index);
Review Comment:
Missing test coverage for the new dynamic expansion functionality with
multiple buckets (`FLAGS_rdma_memory_pool_buckets > 1`). The PR description
states that the main problem being solved is "When
`FLAGS_rdma_memory_pool_buckets > 1`, the block pool cannot be dynamically
expanded." However, the existing `extend` test only uses 1 bucket.
Consider adding a test that:
1. Sets `FLAGS_rdma_memory_pool_buckets` to a value > 1 (e.g., 4)
2. Verifies that dynamic expansion works correctly with multiple buckets
3. Ensures that each bucket correctly retrieves memory from its
corresponding index in `expansion_list`
##########
src/brpc/rdma/block_pool.cpp:
##########
@@ -222,24 +210,27 @@ 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) {
+void* ExtendBlockPoolByUser(void* region_base, size_t region_size, int
block_type) {
+ auto region_base_guard = butil::MakeScopeGuard([region_base]() {
+ free(region_base);
+ });
+
+ if (!FLAGS_rdma_memory_pool_user_specified_memory) {
LOG_EVERY_SECOND(ERROR) << "User extend memory is disabled";
return NULL;
}
if (reinterpret_cast<uintptr_t>(region_base) % 4096 != 0) {
LOG_EVERY_SECOND(ERROR) << "region_base must be 4096 aligned";
+ errno = EINVAL;
return NULL;
}
- uint64_t index = butil::fast_rand() % g_buckets;
- BAIDU_SCOPED_LOCK(*g_info->lock[block_type][index]);
- BAIDU_SCOPED_LOCK(g_info->extend_lock);
region_size =
region_size * BYTES_IN_MB / g_block_size[block_type] / g_buckets;
region_size *= g_block_size[block_type] * g_buckets;
+ region_base_guard.dismiss();
Review Comment:
The scope guard dismissal occurs too early, before input validation for
`block_type` is performed. If `block_type` is invalid (e.g., < 0 or >=
BLOCK_SIZE_COUNT), the code could crash in `ExtendBlockPoolImpl` when accessing
arrays like `g_block_size[block_type]`, `g_info->expansion_list[block_type]`,
etc.
The `region_base_guard.dismiss()` call should be moved to just before the
return statement after all validation has passed, or the scope guard should not
be used at all (see the related issue about calling `free()` on user-provided
memory).
##########
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;
- }
+ });
Review Comment:
Memory management bug: The scope guard in `ExtendBlockPoolImpl` will call
`free()` on `region_base` if the function fails. This is problematic because
`ExtendBlockPoolImpl` is called from both:
1. `ExtendBlockPool` - where memory is allocated with `posix_memalign` (line
205)
2. `ExtendBlockPoolByUser` - where memory is provided by the user
When called from `ExtendBlockPoolByUser`, the user-provided memory (e.g.,
from hugepages or custom pools) should not be freed by this function. Calling
`free()` on non-malloc'd memory can cause crashes.
Suggested fix: Remove the scope guard from `ExtendBlockPoolImpl` and let the
caller manage memory cleanup. `ExtendBlockPool` should call `free()` if
`ExtendBlockPoolImpl` fails.
##########
src/brpc/rdma/block_pool.cpp:
##########
@@ -557,7 +561,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_size[i][j] << "}\n";
Review Comment:
[nitpick] Minor race condition in debug code: `DumpMemoryPoolInfo` reads
`expansion_size[i][j]` without holding `extend_lock`, while
`ExtendBlockPoolImpl` can modify it concurrently (lines 169-176). This could
result in inconsistent or partially updated values in the debug output.
While not critical since this is a monitoring function, consider either:
1. Acquiring `extend_lock` during the dump (may impact performance)
2. Documenting that the dump values may be inconsistent during concurrent
modifications
3. Using atomic operations for size tracking
##########
src/brpc/rdma/block_pool.cpp:
##########
@@ -332,66 +331,74 @@ bool InitBlockPool(RegisterCallback cb) {
return false;
}
+static void MoveExpansionList2EmptyIdleList(int block_type, size_t index) {
+ CHECK(NULL == g_info->idle_list[block_type][index]);
Review Comment:
[nitpick] The `CHECK` assertion at line 335 will cause the program to crash
if `idle_list[block_type][index]` is not NULL. However, this assumption may not
always hold, especially in concurrent scenarios or if there's a bug in the
calling logic.
Consider either:
1. Adding a more descriptive assertion message to aid debugging: `CHECK(NULL
== g_info->idle_list[block_type][index]) << "Expected empty idle_list at index
" << index;`
2. Handling this case gracefully with a LOG and return, rather than crashing
3. If this is truly an invariant that should never be violated, document why
this must always be true in a comment above the function
```suggestion
CHECK(NULL == g_info->idle_list[block_type][index])
<< "Expected idle_list[" << block_type << "][" << index << "] to be
NULL, but got non-NULL. "
<< "This may indicate a logic error or concurrent modification.";
```
##########
src/brpc/rdma/block_pool.cpp:
##########
@@ -222,24 +210,27 @@ 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) {
+void* ExtendBlockPoolByUser(void* region_base, size_t region_size, int
block_type) {
+ auto region_base_guard = butil::MakeScopeGuard([region_base]() {
+ free(region_base);
+ });
+
+ if (!FLAGS_rdma_memory_pool_user_specified_memory) {
LOG_EVERY_SECOND(ERROR) << "User extend memory is disabled";
return NULL;
}
if (reinterpret_cast<uintptr_t>(region_base) % 4096 != 0) {
LOG_EVERY_SECOND(ERROR) << "region_base must be 4096 aligned";
+ errno = EINVAL;
return NULL;
}
- uint64_t index = butil::fast_rand() % g_buckets;
- BAIDU_SCOPED_LOCK(*g_info->lock[block_type][index]);
- BAIDU_SCOPED_LOCK(g_info->extend_lock);
region_size =
region_size * BYTES_IN_MB / g_block_size[block_type] / g_buckets;
region_size *= g_block_size[block_type] * g_buckets;
+ region_base_guard.dismiss();
+ BAIDU_SCOPED_LOCK(g_info->extend_lock);
return ExtendBlockPoolImpl(region_base, region_size, block_type);
}
Review Comment:
Missing test coverage for `ExtendBlockPoolByUser` function. The PR
description mentions fixing issues with user-specified memory
(`FLAGS_rdma_memory_pool_user_specified_memory = true`), but there are no tests
covering this functionality.
Consider adding tests that:
1. Test `ExtendBlockPoolByUser` with valid aligned memory
2. Test the alignment validation (line 222-226)
3. Test the interaction with multiple buckets
4. Test error cases (e.g., max regions reached, invalid block_type)
##########
src/brpc/rdma/block_pool.cpp:
##########
@@ -222,24 +210,27 @@ 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) {
+void* ExtendBlockPoolByUser(void* region_base, size_t region_size, int
block_type) {
+ auto region_base_guard = butil::MakeScopeGuard([region_base]() {
+ free(region_base);
+ });
Review Comment:
[nitpick] The scope guard at lines 214-216 is unnecessary and confusing.
Since `region_base_guard.dismiss()` is always called at line 232 before any
early returns, the scope guard will never execute its cleanup function. This
adds complexity without providing any benefit.
The scope guard should be removed entirely. Memory cleanup is the user's
responsibility since they provided the memory.
##########
src/brpc/rdma/block_pool.cpp:
##########
@@ -222,24 +210,27 @@ static void* ExtendBlockPool(size_t region_size, int
block_type) {
return ExtendBlockPoolImpl(region_base, region_size, block_type);
Review Comment:
Race condition: `ExtendBlockPool` calls `ExtendBlockPoolImpl` without
holding `extend_lock`, but `ExtendBlockPoolImpl` modifies `expansion_list`
(lines 169-176). Meanwhile, `AllocBlockFrom` reads and modifies
`expansion_list` while holding `extend_lock` (line 370).
This can lead to concurrent unsynchronized access to `expansion_list` and
`expansion_size`, potentially causing data corruption or crashes.
Fix: Acquire `extend_lock` before calling `ExtendBlockPoolImpl`:
```cpp
BAIDU_SCOPED_LOCK(g_info->extend_lock);
return ExtendBlockPoolImpl(region_base, region_size, block_type);
```
##########
src/brpc/rdma/block_pool.cpp:
##########
@@ -222,24 +210,27 @@ 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) {
+void* ExtendBlockPoolByUser(void* region_base, size_t region_size, int
block_type) {
+ auto region_base_guard = butil::MakeScopeGuard([region_base]() {
+ free(region_base);
+ });
+
+ if (!FLAGS_rdma_memory_pool_user_specified_memory) {
LOG_EVERY_SECOND(ERROR) << "User extend memory is disabled";
return NULL;
}
if (reinterpret_cast<uintptr_t>(region_base) % 4096 != 0) {
LOG_EVERY_SECOND(ERROR) << "region_base must be 4096 aligned";
+ errno = EINVAL;
return NULL;
}
- uint64_t index = butil::fast_rand() % g_buckets;
- BAIDU_SCOPED_LOCK(*g_info->lock[block_type][index]);
- BAIDU_SCOPED_LOCK(g_info->extend_lock);
region_size =
region_size * BYTES_IN_MB / g_block_size[block_type] / g_buckets;
region_size *= g_block_size[block_type] * g_buckets;
Review Comment:
Missing validation for `region_size` before regularization. If the user
provides a `region_size` that's too small, the regularization at lines 228-230
could result in `region_size = 0`, leading to division by zero or allocation of
useless zero-length nodes.
Add validation similar to `ExtendBlockPool`:
```cpp
if (region_size < 1) {
LOG_EVERY_SECOND(ERROR) << "region_size must be at least 1 MB";
errno = EINVAL;
return NULL;
}
```
Additionally, consider validating after regularization to ensure the result
is non-zero:
```cpp
region_size = region_size * BYTES_IN_MB / g_block_size[block_type] /
g_buckets;
region_size *= g_block_size[block_type] * g_buckets;
if (region_size == 0) {
LOG_EVERY_SECOND(ERROR) << "region_size too small after regularization";
errno = EINVAL;
return NULL;
}
```
##########
src/brpc/rdma/block_pool.cpp:
##########
@@ -222,24 +210,27 @@ 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) {
+void* ExtendBlockPoolByUser(void* region_base, size_t region_size, int
block_type) {
Review Comment:
Missing input validation for `block_type`. The parameter should be validated
to ensure it's within the valid range [0, BLOCK_SIZE_COUNT) before being used
to index arrays.
Add validation like:
```cpp
if (block_type < 0 || block_type >= BLOCK_SIZE_COUNT) {
LOG_EVERY_SECOND(ERROR) << "Invalid block_type: " << block_type;
errno = EINVAL;
return NULL;
}
```
```suggestion
void* ExtendBlockPoolByUser(void* region_base, size_t region_size, int
block_type) {
if (block_type < 0 || block_type >= BLOCK_SIZE_COUNT) {
LOG_EVERY_SECOND(ERROR) << "Invalid block_type: " << block_type;
errno = EINVAL;
return NULL;
}
```
--
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]
