Copilot commented on code in PR #3300:
URL: https://github.com/apache/brpc/pull/3300#discussion_r3256122376
##########
src/brpc/rdma/rdma_helper.cpp:
##########
@@ -178,7 +178,7 @@ void* UserExtendBlockPool(void* region_base, size_t
region_size,
uint32_t RdmaRegisterMemory(void* buf, size_t size) {
// Register the memory as callback in block_pool
// The thread-safety should be guaranteed by the caller
- ibv_mr* mr = IbvRegMr(g_pd, buf, size, IBV_ACCESS_LOCAL_WRITE);
+ ibv_mr* mr = IbvRegMr(g_pd, buf, size, IBV_ACCESS_LOCAL_WRITE |
IBV_ACCESS_RELAXED_ORDERING);
Review Comment:
`IBV_ACCESS_RELAXED_ORDERING` is not available in all libibverbs header
versions; using it unconditionally can break compilation when the macro is
undefined. Also, some devices/drivers may reject this access flag at runtime
(ibv_reg_mr returning NULL), which would make RDMA memory-pool initialization
fail. Consider compiling it conditionally (e.g. `#ifdef
IBV_ACCESS_RELAXED_ORDERING`) and/or retrying registration without
RELAXED_ORDERING when registration fails due to unsupported flags (e.g.
EOPNOTSUPP/EINVAL).
##########
src/brpc/rdma/rdma_helper.cpp:
##########
@@ -594,7 +594,7 @@ void GlobalRdmaInitializeOrDie() {
}
uint32_t RegisterMemoryForRdma(void* buf, size_t len) {
- ibv_mr* mr = IbvRegMr(g_pd, buf, len, IBV_ACCESS_LOCAL_WRITE);
+ ibv_mr* mr = IbvRegMr(g_pd, buf, len, IBV_ACCESS_LOCAL_WRITE
|IBV_ACCESS_RELAXED_ORDERING);
Review Comment:
`RegisterMemoryForRdma` is part of the public RDMA helper API; adding
`IBV_ACCESS_RELAXED_ORDERING` unconditionally may cause `ibv_reg_mr` to fail on
systems where the flag is unsupported (or where the macro is missing at compile
time), breaking previously working callers. Consider making this conditional
and/or adding a fallback path that retries with the previous access flags when
registration fails due to unsupported flags.
##########
src/brpc/rdma/rdma_helper.cpp:
##########
@@ -594,7 +594,7 @@ void GlobalRdmaInitializeOrDie() {
}
uint32_t RegisterMemoryForRdma(void* buf, size_t len) {
- ibv_mr* mr = IbvRegMr(g_pd, buf, len, IBV_ACCESS_LOCAL_WRITE);
+ ibv_mr* mr = IbvRegMr(g_pd, buf, len, IBV_ACCESS_LOCAL_WRITE
|IBV_ACCESS_RELAXED_ORDERING);
Review Comment:
Please add spaces around the `|` operator for consistency with the same
access-flag expression in `RdmaRegisterMemory` above (e.g. `... | ...`).
--
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]
