Skip to content

Revert check for HCA reachability like rc_mlx5 (#11781) - master #11838

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ofirfarjun7 wants to merge 1 commit into from
+1 −2
Closed

Revert check for HCA reachability like rc_mlx5 (#11781) - master #11838

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/uct/ib/mlx5/gdaki/gdaki.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1058,7 +1058,7 @@ static uct_rc_iface_ops_t uct_rc_gdaki_internal_ops = {
.ep_query = (uct_ep_query_func_t)ucs_empty_function_return_unsupported,
.ep_invalidate = (uct_ep_invalidate_func_t)ucs_empty_function_return_unsupported,
.ep_connect_to_ep_v2 = uct_rc_gdaki_ep_connect_to_ep_v2,
.iface_is_reachable_v2 = uct_ib_iface_is_reachable_v2,
.iface_is_reachable_v2 = (uct_iface_is_reachable_v2_func_t)ucs_empty_function_return_one_int,

@svc-nvidia-pr-review svc-nvidia-pr-review Aug 26, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not keep uct_ib_iface_is_reachable_v2 here? gdaki packs a real IB device address and its ep_connect_to_ep_v2 builds the AH from that address via uct_ib_iface_fill_ah_attr_from_addr. With the stub (always return 1), gdaki reports itself reachable to any peer, including a different link layer (IB vs RoCE), a different subnet without FLID, or a mismatched pkey — cases the real check would reject. Since rc_gda participates in UCP wireup (in the ib alias, used as "rc,rc_gda"), UCP could select the gdaki lane toward a genuinely unreachable peer. ep_is_connected still does real device matching and gdaki is normally paired with a primary rc lane that does its own reachability check, so this may be acceptable by design — but pls explain in the description why bypassing reachability is safe for wireup lane selection.

@svc-nvidia-pr-review svc-nvidia-pr-review Aug 27, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making reachability unconditionally return 1 skips valid peer filtering during wireup. The change replaces uct_ib_iface_is_reachable_v2 with ucs_empty_function_return_one_int, so uct_iface_is_reachable_v2() for the gda transport now always reports "reachable". The gda device address is still a standard IB address (iface_get_device_address = uct_ib_iface_get_device_address), so the base IB check works correctly here and every other IB mlx5 transport (rc_mlx5, dc_mlx5, ud_mlx5, gga_mlx5) still uses it. ucp_wireup_select_transport() / ucp_wireup_is_reachable() rely on this to filter out peers with a mismatched pkey, a different IB subnet, or an incompatible/unroutable RoCE address. Unconditionally returning reachable means a gda lane can now be selected toward a peer that the base check would reject, which can defer the failure to connect/QP-transition time instead of lane selection. This warrants an explanation of the "Why?" plus confirmation this is intentional and safe rather than papering over a symptom.

Suggested inline comment: gda device address is a normal IB address and every other IB mlx5 TL keeps uct_ib_iface_is_reachable_v2. why disable the reachability check entirely instead of keeping it? unconditionally returning reachable lets wireup pick a gda lane toward a peer with mismatched pkey / different subnet / unroutable RoCE, moving the failure to connect time.

.ep_is_connected = uct_rc_gdaki_ep_is_connected,
.ep_get_device_ep = uct_rc_gdaki_ep_get_device_ep,
.ep_outstanding_purge = (uct_ep_outstanding_purge_func_t)ucs_empty_function_return_unsupported
Expand Down
1 change: 0 additions & 1 deletion test/gtest/uct/test_uct_iface.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,4 +98,3 @@ UCS_TEST_P(test_uct_iface, is_reachable)
}

UCT_INSTANTIATE_TEST_CASE(test_uct_iface)

@svc-nvidia-pr-review svc-nvidia-pr-review Aug 26, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dropping _UCT_INSTANTIATE_TEST_CASE(test_uct_iface, rc_gda) removes the only reachability coverage for gdaki. The corrupted-address branch cannot pass once the stub is used, but the positive branch (EXPECT_TRUE(is_reachable)) is still valid and worth keeping. Consider keeping a reduced gdaki-specific test that only checks the always-reachable contract, so the behavior is asserted rather than silently untested.

_UCT_INSTANTIATE_TEST_CASE(test_uct_iface, rc_gda)