Add domain object to transports #665
Open
+721
−349
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/domain-cleanup
branch
2 times, most recently
from
d1684a6
to
c89b019
Compare
|
bot:aws:retest |
bwbarrett
force-pushed
the
feature/domain-cleanup
branch
from
c89b019
to
21c7c1d
Compare
|
bot:aws:retest |
bwbarrett
force-pushed
the
feature/domain-cleanup
branch
from
21c7c1d
to
66daac7
Compare
|
p5 ub2004 NCCL-Tests are failing CI with: |
|
bot:aws:retest |
|
bot:aws:retest I can't replicate the crash @a-szegel copied, which could have just been a network fault. |
|
Builds 3 - 8 have all had issues with p5's. The main suspect is the PR at this point. |
|
bot:aws:retest |
1 similar comment
|
bot:aws:retest |
bwbarrett
force-pushed
the
feature/domain-cleanup
branch
3 times, most recently
from
3f6e828
to
23cae5f
Compare
|
bot:aws:retest |
|
Switching to draft; we don't want to include this in the 1.13.0 release. |
bwbarrett
force-pushed
the
feature/domain-cleanup
branch
2 times, most recently
from
1573bf6
to
2662dba
Compare
bwbarrett
force-pushed
the
feature/domain-cleanup
branch
from
2662dba
to
702f3c6
Compare
|
rebased here: aws-nslick@796e96a issues are resolved when running against ofiwg/libfabric#10543 |
bwbarrett
force-pushed
the
feature/domain-cleanup
branch
2 times, most recently
from
ceb48dc
to
c1c6557
Compare
bwbarrett
force-pushed
the
feature/domain-cleanup
branch
from
c1c6557
to
3e7cefc
Compare
aws-nslick
previously approved these changes
Nov 22, 2024
rauteric
reviewed
Nov 22, 2024
It turns out that with the domain being the primary boundary of threading in libfabric, it's not great assuming a 1:1 device to domain mapping. We previously hacked something up to make the domain be part of the endpoint, sometimes, but that makes for some really icky code. This patch adds a domain structure to the object hierarchy and unifies the when to create a new domain code between the two transports. Signed-off-by: Brian Barrett <[email protected]>
bwbarrett
force-pushed
the
feature/domain-cleanup
branch
from
3e7cefc
to
254821c
Compare
rauteric
reviewed
Nov 23, 2024
| fi_close(&domain->domain_rails[i].cq->fid); | ||
| domain->domain_rails[i].cq = NULL; | ||
| } | ||
| } |
| } | ||
|
|
||
| for (int i = 0 ; i < domain->num_rails ; ++i) { | ||
| fi_close(&domain->domain_rails[i].domain->fid); |
There was a problem hiding this comment.
Shouldn't you close the cq before closing the domain?
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.
Add a domain object to the transport interface, to reflect the need for fi_domain (and the related MR cache / rkey pool) to either be a single domain per process or a domain per initializing thread, depending on platform. Previously, this was kind of hacked into the code, with extra fi_domain pointers in various structs. Cut a bunch of that out, and move a bunch of the logic into one place, so that the two protocols behave the same.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.