UCT/IB/GDAKI: Relax RoCE route check for GPU-associated NICs - #11835
UCT/IB/GDAKI: Relax RoCE route check for GPU-associated NICs#11835amastbaum wants to merge 4 commits into
Conversation
|
🤖 Starting review — findings will be posted here when done. |
| return (info.netmask_len > -1); | ||
| } | ||
|
|
||
| int ucs_netlink_route_exists_with_default_fallback( |
There was a problem hiding this comment.
This adds a new public API (ucs_netlink_route_exists_with_default_fallback) and a new reachability branch, but there is no unit test for netlink route logic in test/gtest/ucs/, and no CI job exercises the relaxed path. The default-vs-specific fallback decision is the whole point of the change and is easy to get subtly wrong. can we add a focused test for the default-fallback logic (specific route accepted regardless of other ifaces; default-only route rejected when another iface has a more-specific route)? there's currently no netlink route test to catch a regression here. If a hermetic test isn't practical (netlink needs a real routing table), it's worth the author explaining the coverage gap.
| int ucs_netlink_route_exists(int if_index, const struct sockaddr *sa_remote, | ||
| int *netmask_len_p); | ||
|
|
||
| /** |
There was a problem hiding this comment.
minor: the @return line says "1 if such a route exists ... 0 otherwise", but a default route through this iface can still return 0 because another interface has a more-specific route. The body text is accurate; only the @return line slightly overstates. Maybe reword to match the body ("1 if the route is accepted under the default-fallback rule").
| ucs_debug("%s", ucs_string_buffer_cstr(&info)); | ||
| } | ||
|
|
||
| static int uct_ib_iface_roce_ndev_has_route(uct_ib_iface_t *iface, |
There was a problem hiding this comment.
can we unify both cases like below? this could replace netlink is best route function.
int ucs_netlink_route_matches(int if_index, const struct sockaddr *sa_remote,
int relaxed_check)
{
int netmask_len;
/* No route to the destination through this interface, not even a
* default one */
if (!ucs_netlink_route_exists(if_index, sa_remote, &netmask_len)) {
return 0;
}
/* Relaxed mode: accept a non-default route unconditionally */
if (relaxed_check && (netmask_len > 0)) {
return 1;
}
/* Best route overall; a default route qualifies only as a fallback */
return ucs_netlink_max_netmask_len(sa_remote) == netmask_len;
}| uct_ib_iface_set_path_mtu(self, config); | ||
|
|
||
| self->config.send_overhead = config->send_overhead; | ||
| self->config.relaxed_route_check = |
There was a problem hiding this comment.
maybe roce_relaxed_route_check since it is specific?
| UCT_IB_DDP_SUPPORTED = UCS_BIT(3) | ||
| UCT_IB_DDP_SUPPORTED = UCS_BIT(3), | ||
|
|
||
| /* Whether to accept a specific route through the current NIC even when |
There was a problem hiding this comment.
accept any non-default route, accept default route if no best?
| init_attr.qp_type = IBV_QPT_RC; | ||
| init_attr.dev_name = ib_name; | ||
| init_attr.max_rd_atomic = IBV_DEV_ATTR(&md->super.dev, max_qp_rd_atom); | ||
| /* A better route through a NIC associated with another GPU does not make |
| ucs_debug("%s", ucs_string_buffer_cstr(&info)); | ||
| } | ||
|
|
||
| static int uct_ib_iface_roce_ndev_has_route(uct_ib_iface_t *iface, |
There was a problem hiding this comment.
can we add gtest for relaxed and non-relaxed?
yosefe
left a comment
yosefe
left a comment
There was a problem hiding this comment.
IMO need to see why default GW logic ins not good enough, i'd expect the logic to be the same eventually for GDA and non-GDA
@yosefe I agree that the logic should be the same for GDA and non-GDA RoCE. |
|
🤖 Starting review — findings will be posted here when done. |
| init_attr.seg_size = config->super.super.seg_size; | ||
| init_attr.qp_type = IBV_QPT_RC; | ||
| init_attr.dev_name = ib_name; | ||
| init_attr.max_rd_atomic = IBV_DEV_ATTR(&md->super.dev, max_qp_rd_atom); |
There was a problem hiding this comment.
minor: this blank-line removal is unrelated to the route-matching change, pls drop it from this PR.
|
|
||
| if (ucs_netlink_is_best_route(ndev_index, sa_remote)) { | ||
| /* This interface has the best route */ | ||
| if (ucs_netlink_route_matches(ndev_index, sa_remote, 1)) { |
There was a problem hiding this comment.
can we use a named constant/enum instead of a bare 1 here? ..., 1) is opaque at the call site vs the 0 used by the tcp caller.
|
is the new relaxed RoCE reachability path covered by an existing CI job, or can we add a focused test? there's no unit test for the netlink route matching today. |
| { | ||
| int netmask_len; | ||
|
|
||
| if (!ucs_netlink_route_exists(if_index, sa_remote, &netmask_len)) { |
There was a problem hiding this comment.
i think it could help to add comments for each three step, mentioning the logic
| if (!ucs_netlink_is_best_route(ndev_index, | ||
| (const struct sockaddr*)&remote_addr)) { | ||
| if (!ucs_netlink_route_matches(ndev_index, | ||
| (const struct sockaddr*)&remote_addr, 0)) { |
There was a problem hiding this comment.
i tend to agree here with relaxed=0 for keeping best route scheme since anyway kernel is going to pick best route (with or without src addr bind / configurable) so we try to make ucx understand that it uses the best interface as a lane.
| * @param [in] if_index Network interface index. | ||
| * @param [in] sa_remote Pointer to the destination address. | ||
| * @param [in] relaxed_check If nonzero, accept any non-default route and | ||
| * accept a default route only if no better route |
There was a problem hiding this comment.
maybe: When set, accept any non-default route, or a default route if nothing better exists. Otherwise, accept only the best route.
There was a problem hiding this comment.
Maybe: When set, accept any route. Otherwise accept only best route.
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 Review complete — no issues found. |
5 participants
What?
Allow rc_gda to use a valid route through its GPU-associated NIC even when another NIC has a more-specific route.
Why?
The regular best-route check can select a NIC associated with another GPU and incorrectly mark rc_gda as unreachable.
How?
For GDAKI interfaces, add a route check that accepts any matching non-default route, while allowing a default route only as a fallback if no other rules exist for this destination.