msg/async/rdma: add an option for choosing different RoCE protocol #31517
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@rouming Please help review. |
|
@tchaikov Please help review. |
|
Looks good to me. |
tchaikov
reviewed
Nov 11, 2019
src/msg/async/rdma/Infiniband.cc
Outdated
| @@ -39,7 +40,7 @@ Port::Port(CephContext *cct, struct ibv_context* ictxt, uint8_t ipn): ctxt(ictxt | |||
| } | |||
|
|
|||
| lid = port_attr.lid; | |||
|
|
|||
| assert(gid_idx < port_attr.gid_tbl_len); | |||
There was a problem hiding this comment.
i'd suggest use ceph_assert() here instead. because assert() will be optimized out on any Release build. and we do need to fail no matter running Release or Debug build.
There was a problem hiding this comment.
Agree. Thanks a lot
src/common/options.cc
Outdated
| @@ -1148,6 +1148,10 @@ std::vector<Option> get_global_options() { | |||
| .set_default(1000) | |||
| .set_description(""), | |||
|
|
|||
| Option("ms_async_rdma_gid_idx", Option::TYPE_INT, Option::LEVEL_ADVANCED) | |||
| .set_default(0) | |||
| .set_description(""), | |||
There was a problem hiding this comment.
could you add a short description?
There was a problem hiding this comment.
I'll refine it.
src/common/legacy_config_opts.h
Outdated
| @@ -153,6 +153,7 @@ OPTION(ms_async_rdma_receive_queue_len, OPT_U32) | |||
| OPTION(ms_async_rdma_support_srq, OPT_BOOL) | |||
| OPTION(ms_async_rdma_port_num, OPT_U32) | |||
| OPTION(ms_async_rdma_polling_us, OPT_U32) | |||
| OPTION(ms_async_rdma_gid_idx, OPT_INT) | |||
There was a problem hiding this comment.
if this option is not read on critical path, probably we don't need to add it to legacy_config_opts.h.
There was a problem hiding this comment.
I'll remove it from legacy_config_opts.h since rdma is not on critical path.
1. use gid_idx as parameter to get gid instead of getting the first(0) gid by default. int ibv_query_gid(struct ibv_context *context, uint8_t port_num, int index, union ibv_gid *gid) 2. gid is initialized in class type declaration, there's no need to initialized in the member construction list. Signed-off-by: Changcheng Liu <changcheng.liu@aliyun.com>
With Mellanox NIC, different gid_idx maps to different gid combined with different RoCE protocol e.g. RoCEv1 or RoCEv2 1. Add configuration ms_async_rdma_gid_idx to set gid_idx. 1. Use "0" as default gid_idx to keep compatibility. 2. Initialize gid_idx in class member construction list. Signed-off-by: Changcheng Liu <changcheng.liu@aliyun.com>
changchengx
force-pushed
the
rdma_gid
branch
from
9c6f777
to
b971cff
Compare
tchaikov
changed the title
tchaikov
approved these changes
Nov 11, 2019
tchaikov
added
messenger
|
|
Is there any documents about RoCE protocol of ceph? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Checklist
Show available Jenkins commands
jenkins retest this pleasejenkins test crimson perfjenkins test signedjenkins test make checkjenkins test make check arm64jenkins test submodulesjenkins test dashboardjenkins test dashboard backendjenkins test docsjenkins render docsjenkins test ceph-volume alljenkins test ceph-volume tox