You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brpc.apache.org by GitBox <gi...@apache.org> on 2022/11/20 04:03:27 UTC
[GitHub] [incubator-brpc] Tuvie opened a new pull request, #2005: allow IOBuf::attach_user_data specify any valid address in registered rdma memory region
Tuvie opened a new pull request, #2005:
URL: https://github.com/apache/incubator-brpc/pull/2005
### What problem does this PR solve?
Issue Number: [1995](https://github.com/apache/incubator-brpc/issues/1995)
Problem Summary: When using IOBuf::append_user_data, if the application specify a buffer with an offset over the base address registered with rdma::RegisterMemoryForRdma, brpc reports a failure for invalid memory region.
### What is changed and the side effects?
Changed: Add a new function named append_user_data_with_meta in IOBuf. Allow application specifying the lkey as meta to explicitly inform brpc. This removes the necessity to lookup the lkey for the IOBuf Block in rpc processing.
Side effects:
- Performance effects(性能影响):
- Breaking backward compatibility(向后兼容性): To store the meta/lkey in IOBuf Block, the portal_next field is reused when flags is non-0. It is acceptable because portal_next is only used for tls cache of IOBuf, which does not work for user defined Blocks.
---
### 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](../../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
[GitHub] [incubator-brpc] 372046933 commented on a diff in pull request #2005: allow IOBuf::append_user_data_with_meta to specify the lkey of rdma memory region
Posted by GitBox <gi...@apache.org>.
372046933 commented on code in PR #2005:
URL: https://github.com/apache/incubator-brpc/pull/2005#discussion_r1035702725
##########
src/butil/iobuf.cpp:
##########
@@ -1206,7 +1211,14 @@ int IOBuf::appendv(const const_iovec* vec, size_t n) {
return 0;
}
-int IOBuf::append_user_data(void* data, size_t size, void (*deleter)(void*)) {
+int IOBuf::append_user_data_with_meta(void* data,
+ size_t size,
+ void (*deleter)(void*),
+ uint64_t meta) {
+ if (size == 0) {
+ LOG(WARNING) << "data_size should not be 0";
+ return -1;
Review Comment:
@Tuvie Return 0 has been merged into master
https://github.com/apache/incubator-brpc/blob/b788325528d886639e66aa0d46c571c163ec959a/src/butil/iobuf.cpp#L1214-L1220
--
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
[GitHub] [incubator-brpc] Tuvie commented on a diff in pull request #2005: allow IOBuf::append_user_data_with_meta to specify the lkey of rdma memory region
Posted by GitBox <gi...@apache.org>.
Tuvie commented on code in PR #2005:
URL: https://github.com/apache/incubator-brpc/pull/2005#discussion_r1028788588
##########
src/butil/iobuf.cpp:
##########
@@ -1219,11 +1227,23 @@ int IOBuf::append_user_data(void* data, size_t size, void (*deleter)(void*)) {
deleter = ::free;
}
IOBuf::Block* b = new (mem) IOBuf::Block((char*)data, size, deleter);
+ b->u.data_meta = meta;
const IOBuf::BlockRef r = { 0, b->cap, b };
_move_back_ref(r);
return 0;
}
+uint64_t IOBuf::get_first_data_meta() {
+ if (_ref_num() == 0) {
+ return 0;
+ }
+ IOBuf::BlockRef const& r = _ref_at(0);
+ if (!r.block->flags) {
Review Comment:
OK
##########
src/butil/iobuf.h:
##########
@@ -246,7 +247,18 @@ friend class IOBufCutter;
// Append the user-data to back side WITHOUT copying.
// The user-data can be split and shared by smaller IOBufs and will be
// deleted using the deleter func when no IOBuf references it anymore.
- int append_user_data(void* data, size_t size, void (*deleter)(void*));
+ inline int append_user_data(void* data, size_t size, void (*deleter)(void*)) {
Review Comment:
OK
--
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
[GitHub] [incubator-brpc] wwbmmm commented on pull request #2005: allow IOBuf::append_user_data_with_meta to specify the lkey of rdma memory region
Posted by GitBox <gi...@apache.org>.
wwbmmm commented on PR #2005:
URL: https://github.com/apache/incubator-brpc/pull/2005#issuecomment-1321385642
`append_user_data_with_meta` not found in files changed
--
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
[GitHub] [incubator-brpc] Tuvie commented on a diff in pull request #2005: allow IOBuf::append_user_data_with_meta to specify the lkey of rdma memory region
Posted by GitBox <gi...@apache.org>.
Tuvie commented on code in PR #2005:
URL: https://github.com/apache/incubator-brpc/pull/2005#discussion_r1028788462
##########
test/brpc_rdma_unittest.cpp:
##########
@@ -80,6 +80,7 @@ extern bool g_skip_rdma_init;
}
}
+static const size_t MAX_USER_MRS = 16;
Review Comment:
should remove
##########
src/butil/iobuf.h:
##########
@@ -66,6 +66,7 @@ friend class IOBufCutter;
public:
static const size_t DEFAULT_BLOCK_SIZE = 8192;
static const size_t INITIAL_CAP = 32; // must be power of 2
+ static const size_t BLOCK_META_OFFSET = 16;
Review Comment:
should remove
--
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
[GitHub] [incubator-brpc] Tuvie commented on pull request #2005: allow IOBuf::append_user_data_with_meta to specify the lkey of rdma memory region
Posted by GitBox <gi...@apache.org>.
Tuvie commented on PR #2005:
URL: https://github.com/apache/incubator-brpc/pull/2005#issuecomment-1335067131
Has resolved conflict. Any plan to merge?
--
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
[GitHub] [incubator-brpc] 372046933 commented on a diff in pull request #2005: allow IOBuf::append_user_data_with_meta to specify the lkey of rdma memory region
Posted by GitBox <gi...@apache.org>.
372046933 commented on code in PR #2005:
URL: https://github.com/apache/incubator-brpc/pull/2005#discussion_r1029957001
##########
src/butil/iobuf.cpp:
##########
@@ -1206,7 +1211,14 @@ int IOBuf::appendv(const const_iovec* vec, size_t n) {
return 0;
}
-int IOBuf::append_user_data(void* data, size_t size, void (*deleter)(void*)) {
+int IOBuf::append_user_data_with_meta(void* data,
+ size_t size,
+ void (*deleter)(void*),
+ uint64_t meta) {
+ if (size == 0) {
+ LOG(WARNING) << "data_size should not be 0";
+ return -1;
Review Comment:
Did `size == 0` have any side effect? If not, can we return 0 here?
--
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
[GitHub] [incubator-brpc] wwbmmm commented on pull request #2005: allow IOBuf::append_user_data_with_meta to specify the lkey of rdma memory region
Posted by GitBox <gi...@apache.org>.
wwbmmm commented on PR #2005:
URL: https://github.com/apache/incubator-brpc/pull/2005#issuecomment-1324465314
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
[GitHub] [incubator-brpc] wwbmmm commented on a diff in pull request #2005: allow IOBuf::append_user_data_with_meta to specify the lkey of rdma memory region
Posted by GitBox <gi...@apache.org>.
wwbmmm commented on code in PR #2005:
URL: https://github.com/apache/incubator-brpc/pull/2005#discussion_r1027626417
##########
src/butil/iobuf.h:
##########
@@ -66,6 +66,7 @@ friend class IOBufCutter;
public:
static const size_t DEFAULT_BLOCK_SIZE = 8192;
static const size_t INITIAL_CAP = 32; // must be power of 2
+ static const size_t BLOCK_META_OFFSET = 16;
Review Comment:
Where is it used?
##########
src/butil/iobuf.cpp:
##########
@@ -1219,11 +1227,23 @@ int IOBuf::append_user_data(void* data, size_t size, void (*deleter)(void*)) {
deleter = ::free;
}
IOBuf::Block* b = new (mem) IOBuf::Block((char*)data, size, deleter);
+ b->u.data_meta = meta;
const IOBuf::BlockRef r = { 0, b->cap, b };
_move_back_ref(r);
return 0;
}
+uint64_t IOBuf::get_first_data_meta() {
+ if (_ref_num() == 0) {
+ return 0;
+ }
+ IOBuf::BlockRef const& r = _ref_at(0);
+ if (!r.block->flags) {
Review Comment:
flags & IOBUF_BLOCK_FLAGS_USER_DATA
##########
src/butil/iobuf.h:
##########
@@ -246,7 +247,18 @@ friend class IOBufCutter;
// Append the user-data to back side WITHOUT copying.
// The user-data can be split and shared by smaller IOBufs and will be
// deleted using the deleter func when no IOBuf references it anymore.
- int append_user_data(void* data, size_t size, void (*deleter)(void*));
+ inline int append_user_data(void* data, size_t size, void (*deleter)(void*)) {
Review Comment:
This method should be defined in `iobuf_inl.h`
##########
test/brpc_rdma_unittest.cpp:
##########
@@ -80,6 +80,7 @@ extern bool g_skip_rdma_init;
}
}
+static const size_t MAX_USER_MRS = 16;
Review Comment:
Where is it used?
--
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
[GitHub] [incubator-brpc] Tuvie commented on pull request #2005: allow IOBuf::append_user_data_with_meta to specify the lkey of rdma memory region
Posted by GitBox <gi...@apache.org>.
Tuvie commented on PR #2005:
URL: https://github.com/apache/incubator-brpc/pull/2005#issuecomment-1326354421
@wwbmmm is this PR qualified to be merged? It seems that the Travis often fails due to some irrelevant reason.
--
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
[GitHub] [incubator-brpc] wwbmmm merged pull request #2005: allow IOBuf::append_user_data_with_meta to specify the lkey of rdma memory region
Posted by GitBox <gi...@apache.org>.
wwbmmm merged PR #2005:
URL: https://github.com/apache/incubator-brpc/pull/2005
--
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
[GitHub] [incubator-brpc] Tuvie commented on a diff in pull request #2005: allow IOBuf::append_user_data_with_meta to specify the lkey of rdma memory region
Posted by GitBox <gi...@apache.org>.
Tuvie commented on code in PR #2005:
URL: https://github.com/apache/incubator-brpc/pull/2005#discussion_r1029985331
##########
src/butil/iobuf.cpp:
##########
@@ -1206,7 +1211,14 @@ int IOBuf::appendv(const const_iovec* vec, size_t n) {
return 0;
}
-int IOBuf::append_user_data(void* data, size_t size, void (*deleter)(void*)) {
+int IOBuf::append_user_data_with_meta(void* data,
+ size_t size,
+ void (*deleter)(void*),
+ uint64_t meta) {
+ if (size == 0) {
+ LOG(WARNING) << "data_size should not be 0";
+ return -1;
Review Comment:
This should be unexpected. return -1 is a better choice.
--
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