Move rdma control channel traffic to its own endpoint #321
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
bwbarrett
force-pushed
the
feature/control-channel-qp
branch
from
cfe79c6 to
da3fb7c
Compare
rauteric
reviewed
Jan 24, 2024
bwbarrett
force-pushed
the
feature/control-channel-qp
branch
from
da3fb7c to
c2143e6
Compare
bwbarrett
added
the
BuildTriggerRequest
rajachan
previously approved these changes
Jan 25, 2024
bwbarrett
force-pushed
the
feature/control-channel-qp
branch
from
c2143e6 to
16784f0
Compare
|
rebased against the master branch and resolved conflicts from the id pool changes. |
rauteric
reviewed
Feb 9, 2024
bwbarrett
force-pushed
the
feature/control-channel-qp
branch
from
16784f0 to
8a22c74
Compare
bwbarrett
force-pushed
the
feature/control-channel-qp
branch
from
8a22c74 to
91d3d8e
Compare
|
Fixed an |
rauteric
reviewed
Feb 20, 2024
| /* look for control messages and then retry the message search | ||
| to avoid unnecessary polling / queueing. */ | ||
| if (OFI_UNLIKELY(!polled_cq && !have_ctrl)) { | ||
| ofi_process_cq_rail(ep, &ep->control_rail); |
rauteric
reviewed
Feb 20, 2024
| @@ -4659,6 +4661,14 @@ static int send(nccl_net_ofi_send_comm_t *send_comm, void *data, int size, int t | |||
| goto error; | |||
| } | |||
|
|
|||
| /* look for control messages and then retry the message search | |||
| to avoid unnecessary polling / queueing. */ | |||
| if (OFI_UNLIKELY(!polled_cq && !have_ctrl)) { | |||
There was a problem hiding this comment.
Because it's going to take forever, so optimize for the fast path.
AvivBenchorin
added
BuildTriggerRequest
bwbarrett
force-pushed
the
feature/control-channel-qp
branch
from
91d3d8e to
0300d4e
Compare
|
Converting this PR to a draft. #344 includes all but the last two patches, which actually create the control QP/CQ. Those two patches impact performance in a way that needs more work. Leaving this PR so that we don't lose the work, but in the mean time, we'll get the refactoring code in as part of #344. |
bwbarrett
force-pushed
the
feature/control-channel-qp
branch
from
0300d4e to
a088255
Compare
a-szegel
removed
the
BuildTriggerRequest
|
Please merge master. |
Signed-off-by: Brian Barrett <bbarrett@amazon.com>
For the long message RDMA protocol, we want to make sure that we never starve the sender for data to move, which means prioritizing control messages from the receiver to the sender. This patch moves both the communicator setup and recv control messages to a new endpoint, which is always on device rail 0. Future patches will optimize polling of the control message cq in the send path and setting priority bits on the control cq. Signed-off-by: Brian Barrett <bbarrett@amazon.com>
If a match isn't found for the current send, poll the control cq to see if the match can be found. While this extends the current send() call, it potentially lowers the time until data transfer starts. Signed-off-by: Brian Barrett <bbarrett@amazon.com>
bwbarrett
force-pushed
the
feature/control-channel-qp
branch
from
a088255 to
cd4a393
Compare
|
This is now so hopelessly out of date that I'm going to close the PR. @rajachan is working on refactoring to work with HEAD of master. |
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.
This is a series of cleanup patches that includes splitting some of the initialization of Libfabric between the send/recv and rdma transports, with the end goal of making it easier to move the control channel data from the data endpoint to its own endpoint (always on "NIC 0"), so that we can eventually increase the priority level of control messages over data messages (when Libfabric/rdma-core supports the service level feature through to the card).
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.