You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brpc.apache.org by "chenBright (via GitHub)" <gi...@apache.org> on 2025/11/18 02:52:19 UTC
[PR] Support dynamic expansion of RDMA block pool (brpc)
chenBright opened a new pull request, #3155:
URL: https://github.com/apache/brpc/pull/3155
### What problem does this PR solve?
Issue Number:
Problem Summary:
1. When `FLAGS_rdma_memory_pool_buckets > 1`, the block pool cannot be dynamically expanded.
2. When `FLAGS_rdma_memory_pool_user_specified_memory = true`, it is difficult for users to correctly expand the block pool.
### What is changed and the side effects?
Changed:
Dynamically expanded memory is stored in an `expansion_list` protected by `extend_lock`, and each bucket only retrieves memory from the `expansion_list` at the corresponding index.
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] Support dynamic expansion of RDMA block pool (brpc)
Posted by "yanglimingcn (via GitHub)" <gi...@apache.org>.
yanglimingcn commented on PR #3155:
URL: https://github.com/apache/brpc/pull/3155#issuecomment-3547264869
In this scenario, the memory should be pre-registered, and dynamic allocation requires the user to provide memory too.
--
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] Support dynamic expansion of RDMA block pool (brpc)
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: 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] Support dynamic expansion of RDMA block pool (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on PR #3155:
URL: https://github.com/apache/brpc/pull/3155#issuecomment-3547237590
> FLAGS_rdma_memory_pool_user_specified_memory is used to allow users to register specific memory, such as SPDK. In these scenarios, memory needs to be allocated by SPDK and then registered in RDMA, which can achieve zero copy.
Ok, I will revert it.
When the AllocBlockFrom function is called and the idle_list is out of memory, it seems that the user cannot immediately dynamically expand specific memory.
--
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] Support dynamic expansion of RDMA block pool (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on PR #3155:
URL: https://github.com/apache/brpc/pull/3155#issuecomment-3547830820
> > FLAGS_rdma_memory_pool_user_specified_memory is used to allow users to register specific memory, such as SPDK. In these scenarios, memory needs to be allocated by SPDK and then registered in RDMA, which can achieve zero copy.
>
> Ok, I will revert it.
@yanglimingcn done
--
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] Support dynamic expansion of RDMA block pool (brpc)
Posted by "yanglimingcn (via GitHub)" <gi...@apache.org>.
yanglimingcn commented on PR #3155:
URL: https://github.com/apache/brpc/pull/3155#issuecomment-3547157198
FLAGS_rdma_memory_pool_user_specified_memory is used to allow users to register specific memory, such as SPDK. In these scenarios, memory needs to be allocated by SPDK and then registered in RDMA, which can achieve zero copy.
--
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] Support dynamic expansion of RDMA block pool (brpc)
Posted by "yanglimingcn (via GitHub)" <gi...@apache.org>.
yanglimingcn commented on PR #3155:
URL: https://github.com/apache/brpc/pull/3155#issuecomment-3580215673
LGTM
--
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] Support dynamic expansion of RDMA block pool (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright merged PR #3155:
URL: https://github.com/apache/brpc/pull/3155
--
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] Support dynamic expansion of RDMA block pool (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on PR #3155:
URL: https://github.com/apache/brpc/pull/3155#issuecomment-3544838735
> When FLAGS_rdma_memory_pool_buckets is greater than 1, a bug may occur due to lock granularity issues.
This PR can solve this problem.
--
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] Support dynamic expansion of RDMA block pool (brpc)
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: 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] Support dynamic expansion of RDMA block pool (brpc)
Posted by "yanglimingcn (via GitHub)" <gi...@apache.org>.
yanglimingcn commented on PR #3155:
URL: https://github.com/apache/brpc/pull/3155#issuecomment-3544806282
When FLAGS_rdma_memory_pool_buckets is greater than 1, a bug may occur due to lock granularity issues.
--
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