Copilot commented on code in PR #2965:
URL: https://github.com/apache/brpc/pull/2965#discussion_r2217172111
##########
src/brpc/rdma/block_pool.h:
##########
@@ -73,7 +73,15 @@ typedef uint32_t (*RegisterCallback)(void*, size_t);
// region. It should be the memory registration in brpc. However,
// in block_pool, we just abstract it into a function to get region id.
// Return the first region's address, NULL if failed and errno is set.
-void* InitBlockPool(RegisterCallback cb);
+bool InitBlockPool(RegisterCallback cb);
+
+// In scenarios where users need to manually specify memory regions (e.g.,
using
+// hugepages or custom memory pools), when
+// FLAGS_rdma_memory_pool_user_specified_memory is true, user is
responsibility
+// of extending memory blocks , this ensuring flexibility for advanced use
Review Comment:
Grammar error: 'this ensuring' should be 'thus ensuring' for proper sentence
structure.
```suggestion
// of extending memory blocks , thus ensuring flexibility for advanced use
```
##########
src/brpc/rdma/block_pool.cpp:
##########
@@ -36,9 +36,12 @@ DEFINE_int32(rdma_memory_pool_initial_size_mb, 1024,
"Initial size of memory pool for RDMA (MB)");
DEFINE_int32(rdma_memory_pool_increase_size_mb, 1024,
"Increased size of memory pool for RDMA (MB)");
-DEFINE_int32(rdma_memory_pool_max_regions, 4, "Max number of regions");
+DEFINE_int32(rdma_memory_pool_max_regions, 1, "Max number of regions");
Review Comment:
Changing the default value from 4 to 1 for rdma_memory_pool_max_regions is a
breaking change that could impact existing deployments. Consider documenting
this change more prominently or providing a migration path.
##########
src/brpc/rdma/block_pool.cpp:
##########
@@ -178,23 +163,79 @@ static void* ExtendBlockPool(size_t region_size, int
block_type) {
for (size_t i = 0; i < g_buckets; ++i) {
node[i]->start = (void*)(region->start + i * (region_size /
g_buckets));
node[i]->len = region_size / g_buckets;
- node[i]->next = NULL;
+ node[i]->next = g_info->idle_list[block_type][i];
g_info->idle_list[block_type][i] = node[i];
g_info->idle_size[block_type][i] += node[i]->len;
}
return region_base;
}
-void* InitBlockPool(RegisterCallback cb) {
- if (!cb) {
+// Extend the block pool with a new region (with different region ID)
+static void* ExtendBlockPool(size_t region_size, int block_type) {
+ if (region_size < 1) {
errno = EINVAL;
return NULL;
}
+
+ if (FLAGS_rdma_memory_pool_user_specified_memory) {
+ LOG_EVERY_SECOND(ERROR) << "Fail to extend new region, "
+ "rdma_memory_pool_user_specified_memory is "
+ "true, ExtendBlockPool is disabled";
+ return NULL;
+ }
+
+ // Regularize region size
+ region_size = region_size * BYTES_IN_MB / g_block_size[block_type] /
g_buckets;
+ region_size *= g_block_size[block_type] * g_buckets;
+
+ LOG(INFO) << "Start extend rdma memory " << region_size / BYTES_IN_MB <<
"MB";
+
+ void* region_base = NULL;
+ if (posix_memalign(®ion_base, 4096, region_size) != 0) {
+ PLOG_EVERY_SECOND(ERROR) << "Memory not enough";
+ return NULL;
+ }
+
+ 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;
+ }
+ if (reinterpret_cast<uintptr_t>(region_base) % 4096 != 0) {
+ LOG_EVERY_SECOND(ERROR) << "region_base must be 4096 aligned";
+ return NULL;
+ }
+
+ uint64_t index = butil::fast_rand() % g_buckets;
+ BAIDU_SCOPED_LOCK(*g_info->lock[block_type][index]);
Review Comment:
Using a random bucket selection followed by locking all buckets doesn't make
sense. If you're going to lock all buckets anyway (lines 218-221), the random
selection is unnecessary and could be confusing.
```suggestion
```
##########
src/brpc/rdma/block_pool.cpp:
##########
@@ -178,23 +163,79 @@ static void* ExtendBlockPool(size_t region_size, int
block_type) {
for (size_t i = 0; i < g_buckets; ++i) {
node[i]->start = (void*)(region->start + i * (region_size /
g_buckets));
node[i]->len = region_size / g_buckets;
- node[i]->next = NULL;
+ node[i]->next = g_info->idle_list[block_type][i];
g_info->idle_list[block_type][i] = node[i];
g_info->idle_size[block_type][i] += node[i]->len;
}
return region_base;
}
-void* InitBlockPool(RegisterCallback cb) {
- if (!cb) {
+// Extend the block pool with a new region (with different region ID)
+static void* ExtendBlockPool(size_t region_size, int block_type) {
+ if (region_size < 1) {
errno = EINVAL;
return NULL;
}
+
+ if (FLAGS_rdma_memory_pool_user_specified_memory) {
+ LOG_EVERY_SECOND(ERROR) << "Fail to extend new region, "
+ "rdma_memory_pool_user_specified_memory is "
+ "true, ExtendBlockPool is disabled";
+ return NULL;
+ }
+
+ // Regularize region size
+ region_size = region_size * BYTES_IN_MB / g_block_size[block_type] /
g_buckets;
+ region_size *= g_block_size[block_type] * g_buckets;
+
+ LOG(INFO) << "Start extend rdma memory " << region_size / BYTES_IN_MB <<
"MB";
+
+ void* region_base = NULL;
+ if (posix_memalign(®ion_base, 4096, region_size) != 0) {
+ PLOG_EVERY_SECOND(ERROR) << "Memory not enough";
+ return NULL;
+ }
+
+ 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;
+ }
+ if (reinterpret_cast<uintptr_t>(region_base) % 4096 != 0) {
+ LOG_EVERY_SECOND(ERROR) << "region_base must be 4096 aligned";
+ 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);
+
+ if (g_region_num > 1 && FLAGS_rdma_memory_pool_buckets > 1) {
Review Comment:
This condition checks g_region_num > 1, but g_region_num is incremented
after this check in ExtendBlockPoolImpl. This means the first extension will
pass this check incorrectly. The condition should be g_region_num >= 1.
```suggestion
if (g_region_num >= 1 && FLAGS_rdma_memory_pool_buckets > 1) {
```
--
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-unsubscr...@brpc.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@brpc.apache.org
For additional commands, e-mail: dev-h...@brpc.apache.org
