tcp btl: Fix multiple-link connection establishment. #4247
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
|
Can one of the admins verify this patch? |
|
ok to test |
|
I have reproduced the issue reported by the mellanox build, which is caused by mutliple NICs. |
|
bot:retest |
jocherry
force-pushed
the
btlTcpLinksBugFix
branch
from
24c6129 to
e20fc6c
Compare
|
bot:ompi:retest |
|
bot:mellanox:retest |
1 similar comment
|
bot:mellanox:retest |
|
@bwbarrett did you received ifconfig info that I sent you some time ago? Do you need further assistance? |
jocherry
force-pushed
the
btlTcpLinksBugFix
branch
from
c2ab37a to
840d7d5
Compare
jocherry
force-pushed
the
btlTcpLinksBugFix
branch
from
840d7d5 to
2d60350
Compare
|
All bugs have been fixed, this is alright to be looked at now. |
bosilca
reviewed
Feb 25, 2018
opal/mca/btl/tcp/btl_tcp.h
Outdated
| @@ -167,7 +167,8 @@ struct mca_btl_tcp_module_t { | |||
| #if 0 | |||
| int tcp_ifindex; /**< BTL interface index */ | |||
| #endif | |||
| struct sockaddr_storage tcp_ifaddr; /**< BTL interface address */ | |||
| struct sockaddr_storage tcp_ifaddr; /**< First IPv4 address discovered for this interface, bound as sending address for this BTL */ | |||
| struct sockaddr_storage tcp_ifaddr_6; /**< First IPv6 address discovered for this interface, bound as sending address for this BTL */ | |||
There was a problem hiding this comment.
IPV6 code should be protected by OPAL_ENABLE_IPV6.
Fix case where the btl_tcp_links MCA parameter is used to create multiple TCP connections between peers.
Three issues were resulting in hangs during large message transfer:
* The 2nd..btl_tcp_link connections were dropped during establishment because the per-process
address check was binary, rather than a count
* The accept handler would not skip a btl module that was already in use, resulting in all
connections for a given address being vectored to a single btl
* Multiple addresses in the same subnet caused connections to be
stalled, as the receiver would always use the same (first) address
found. Binding the outgoing connection solves this issue
* Lastly fix race condition created by connections being started at the exact same time
by accpeting connections not in the closed state, allowing endpoint_accept to resolve
dispute
Signed-off-by: Jordan Cherry <cherryj@amazon.com>
jocherry
force-pushed
the
btlTcpLinksBugFix
branch
from
2d60350 to
d7e7e3a
Compare
|
updated based on @bosilca feedback |
bwbarrett
approved these changes
Mar 7, 2018
| * can properly pair btl modules, even in cases where Linux | ||
| * might do something unexpected with routing */ | ||
| opal_socklen_t sockaddr_addrlen = sizeof(struct sockaddr_storage); | ||
| if (endpoint_addr.ss_family == AF_INET) { |
There was a problem hiding this comment.
Minor nit, but I would change this to have a struct sockaddr* temporary variable with assignment from either tcp_ifaddr or tcp_ifaddr6, to avoid duplicating the bind() and error handling code.
bosilca
approved these changes
Mar 7, 2018
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.
Fix case where the btl_tcp_links MCA parameter is used to create multiple TCP connections between peers.
Two issues were resulting in hangs during large message transfer:
address check was binary, rather than a count
connections for a given address being vectored to a single btl
Signed-off-by: Jordan Cherry cherryj@amazon.com