Separate endpoint for control messages #543
Merged
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
rajachan
force-pushed
the
control-qp-rebase
branch
2 times, most recently
from
6898e99 to
ca7884f
Compare
|
bot:aws:retest |
bwbarrett
reviewed
Aug 26, 2024
|
The CI failure looks real; it looks like tracing broke. |
rauteric
reviewed
Aug 26, 2024
|
The previous PR (#321 (comment)) had some unresolved perf issues. Have we resolved those at this point? |
I tested up to 16 nodes and did not see any perf issues with the series. Was there any specific case where we saw issues earlier, @bwbarrett ? |
Large message Allreduce, but we've cleaned up a bunch of Libfabric and EFA issues since then, so if we don't see changes, we don't see changes. |
rajachan
force-pushed
the
control-qp-rebase
branch
2 times, most recently
from
dcef637 to
0f0343c
Compare
|
Comments addressed and tracing breakage fixed. The previous round of AWS-CI runs had a functional test timeout, will watch this run and test locally to make sure that wasn't CI infra being CI infra. |
rauteric
approved these changes
Aug 28, 2024
rauteric
reviewed
Aug 29, 2024
ghost
reviewed
Aug 30, 2024
|
bot:aws:retest |
a-szegel
force-pushed
the
control-qp-rebase
branch
from
c11ee00 to
1de991c
Compare
|
rebased via GUI to re-kick off PR CI. |
|
bot:aws:retest |
rauteric
requested changes
Sep 6, 2024
rajachan
force-pushed
the
control-qp-rebase
branch
from
1de991c to
b16b8f3
Compare
rauteric
previously approved these changes
Sep 6, 2024
rajachan
force-pushed
the
control-qp-rebase
branch
from
b16b8f3 to
cf659bd
Compare
|
CI cleared, this is ready to go. |
rauteric
reviewed
Sep 7, 2024
The control messages are not going to be scheduled over multiple rails, so remove the unnecessary logic around it. Signed-off-by: Brian Barrett <bbarrett@amazon.com> Signed-off-by: Raghu Raja <raghunch@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> Signed-off-by: Raghu Raja <raghunch@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> Signed-off-by: Raghu Raja <raghunch@amazon.com>
rajachan
force-pushed
the
control-qp-rebase
branch
from
cf659bd to
ce1654a
Compare
rauteric
approved these changes
Sep 7, 2024
AmedeoSapio
added a commit
to AmedeoSapio/aws-ofi-nccl
that referenced
this pull request
Oct 18, 2024
PR aws#543 has moved the control message to its own dedicated endpoint on a single rail. As a result, the control message is not sent on all rails in a round-robin fashion anymore. This has impacted performance in some cases, so this is adding an environment variable to optionally enable the separate control message endpoint, but for now we use as a default the old behavior of sending the control message round-robining across rails. Signed-off-by: Amedeo Sapio <asapio@amazon.com>
AmedeoSapio
added a commit
to AmedeoSapio/aws-ofi-nccl
that referenced
this pull request
Oct 18, 2024
PR aws#543 has moved the control message to its own dedicated endpoint on a single rail. As a result, the control message is not sent on all rails in a round-robin fashion anymore. This has impacted performance in some cases, so this is adding an environment variable to optionally enable the separate control message endpoint, but for now we use as a default the old behavior of sending the control message round-robining across rails. Signed-off-by: Amedeo Sapio <asapio@amazon.com>
AmedeoSapio
added a commit
to AmedeoSapio/aws-ofi-nccl
that referenced
this pull request
Oct 22, 2024
PR aws#543 has moved the control message to its own dedicated endpoint on a single rail and with its own CQ. To avoid the cost of polling an additional CQ, this commit is making the control endpoint share the CQ with rail 0. Signed-off-by: Amedeo Sapio <asapio@amazon.com>
AmedeoSapio
added a commit
to AmedeoSapio/aws-ofi-nccl
that referenced
this pull request
Oct 22, 2024
PR aws#543 has moved the control message to its own dedicated endpoint on a single rail. As a result, the control message is not sent on all rails in a round-robin fashion anymore. This has impacted performance in some cases, so we want the option to still distribute the control messages across all rails. This commit is providing an environment variable to choose between a) using a single dedicated endpoint for control messages, sharing the CQ with the data endpoint on rail 0; or b) use as many additional endpoints for the control messages as the number of rails, sharing the CQs with the data endpoints, and use the scheduler to round robin the control messages across all rails. Signed-off-by: Amedeo Sapio <asapio@amazon.com>
AmedeoSapio
added a commit
to AmedeoSapio/aws-ofi-nccl
that referenced
this pull request
Oct 23, 2024
PR aws#543 has moved the control message to its own dedicated endpoint on a single rail. As a result, the control message is not sent on all rails in a round-robin fashion anymore. This has impacted performance in some cases, so we want the option to still distribute the control messages across all rails. This commit is providing an environment variable to choose between a) using a single dedicated endpoint for control messages, sharing the CQ with the data endpoint on rail 0; or b) use as many additional endpoints for the control messages as the number of rails, sharing the CQs with the data endpoints, and use the scheduler to round robin the control messages across all rails. Signed-off-by: Amedeo Sapio <asapio@amazon.com>
AmedeoSapio
added a commit
to AmedeoSapio/aws-ofi-nccl
that referenced
this pull request
Oct 24, 2024
PR aws#543 has moved the control message to its own dedicated endpoint on a single rail and with its own CQ. To avoid the cost of polling an additional CQ, this commit is making the control endpoint share the CQ with rail 0. Signed-off-by: Amedeo Sapio <asapio@amazon.com>
AmedeoSapio
added a commit
to AmedeoSapio/aws-ofi-nccl
that referenced
this pull request
Oct 24, 2024
PR aws#543 has moved the control message to its own dedicated endpoint on a single rail. As a result, the control message is not sent on all rails in a round-robin fashion anymore. This has impacted performance in some cases, so we want the option to still distribute the control messages across all rails. This commit is providing an environment variable to choose between a) using a single dedicated endpoint for control messages, sharing the CQ with the data endpoint on rail 0; or b) use as many additional endpoints for the control messages as the number of rails, sharing the CQs with the data endpoints, and use the scheduler to round robin the control messages across all rails. Signed-off-by: Amedeo Sapio <asapio@amazon.com>
AmedeoSapio
added a commit
to AmedeoSapio/aws-ofi-nccl
that referenced
this pull request
Oct 26, 2024
PR aws#543 has moved the control message to its own dedicated endpoint on a single rail and with its own CQ. To avoid the cost of polling an additional CQ, this commit is making the control endpoint share the CQ with rail 0. Signed-off-by: Amedeo Sapio <asapio@amazon.com>
AmedeoSapio
added a commit
to AmedeoSapio/aws-ofi-nccl
that referenced
this pull request
Oct 26, 2024
PR aws#543 has moved the control message to its own dedicated endpoint on a single rail. As a result, the control message is not sent on all rails in a round-robin fashion anymore. This has impacted performance in some cases, so we want the option to still distribute the control messages across all rails. This commit is providing an environment variable to choose between a) using a single dedicated endpoint for control messages, sharing the CQ with the data endpoint on rail 0; or b) use as many additional endpoints for the control messages as the number of rails, sharing the CQs with the data endpoints, and use the scheduler to round robin the control messages across all rails. Signed-off-by: Amedeo Sapio <asapio@amazon.com>
rajachan
pushed a commit
that referenced
this pull request
Oct 26, 2024
PR #543 has moved the control message to its own dedicated endpoint on a single rail and with its own CQ. To avoid the cost of polling an additional CQ, this commit is making the control endpoint share the CQ with rail 0. Signed-off-by: Amedeo Sapio <asapio@amazon.com>
rajachan
pushed a commit
that referenced
this pull request
Oct 26, 2024
PR #543 has moved the control message to its own dedicated endpoint on a single rail. As a result, the control message is not sent on all rails in a round-robin fashion anymore. This has impacted performance in some cases, so we want the option to still distribute the control messages across all rails. This commit is providing an environment variable to choose between a) using a single dedicated endpoint for control messages, sharing the CQ with the data endpoint on rail 0; or b) use as many additional endpoints for the control messages as the number of rails, sharing the CQs with the data endpoints, and use the scheduler to round robin the control messages across all rails. Signed-off-by: Amedeo Sapio <asapio@amazon.com>
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.
Rebased and reworked #321 on master.
The commits are largely the same with adaptations to recent changes around MR (with the addition of the cache) and fix for a bug that was causing an fi_av_insert() on the control QP's address to fail (as the conn message did not carry the correct address).
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.