UCT: prioritize routes with longer subnet masks in the reachability check by amastbaum · Pull Request #11033 · openucx/ucx

amastbaum · 2025-12-02T13:44:55Z

What?

Use a default GW routes only when no specific routes exist.

Found in a CI issue when running ./test/gtest/gtest --gtest_filter=*all/test_ucp_sockaddr_protocols_diff_net_devices.restricted_net_devices/0*

amastbaum · 2025-12-02T13:49:44Z

 int ucs_netlink_route_exists(int if_index, const struct sockaddr *sa_remote,
-                             int allow_default_gw);
+                             int *has_default_gw_p);


Currently the information about default gw rules is retrieved implicitly via the out parameter but I can create a separate function for that.

gleon99 · 2025-12-02T15:29:59Z

@amastbaum

The title is a bit clumsy, plz rephrase (for example, from the "description").
Use what/why/how template.
plz explain not that "it was found in CI", but what is the root issue, when it appears, what is the impact..

gleon99 · 2025-12-02T17:29:59Z

+ *                                 interface, as assigned by the system
+ *                                 (e.g., obtained via if_nametoindex()).
+ * @param [in]  sa_remote          Pointer to the destination address.
+ * @param [out] has_default_gw_p   Optional pointer to store whether a default


wdyt of: instead of returning 0/1 and an optional _p, have a single return value with an enum like {None,Route,Gateway} (roughly)?

gleon99 · 2025-12-02T17:30:50Z

+                             int *has_default_gw_p);
+
+/**
+ * Check whether a specific route exists for a given destination address


Where does the word "specific" come from? Is it used in netlink terminology?

So not really. It's only relative to the "default GW/route" term. I need to emphasize their difference (can also be "non-default route")

gleon99 · 2025-12-02T17:31:33Z

-    return 1;
+    /* No route found at all */
+    if (lo_status == UCS_OK) {
+        uct_iface_fill_info_str_buf(params,


Can eliminate code duplication here?

gleon99 · 2025-12-02T17:33:24Z

+        return 1;
+    }
+
+    /* Going to search for a default GW route on this interface. If found, and


Please move to lines 286-293 block, make more concise (w/o "going to..", only explain the reasoning)

gleon99 · 2025-12-02T17:34:14Z

-                                        UCT_IB_IFACE_ARG(iface),
-                                        ndev_index, lo_ndev_index);
-            return 0;
+    /* Going to search a default GW route on this interface. If found, and


This comment appears twice.

@gleon99 It appears once in tcp_iface.c and once in ib_iface.c.
You mean to eliminate duplications between them? Maybe need some ip category, or tcp_iface.c is enough?

yosefe · 2025-12-02T18:23:54Z

should we prioritize routes by subnet prefix length in general (i.e longest prefix matching), not just for default gw?

roiedanino · 2025-12-03T11:25:34Z

+int ucs_netlink_has_any_specific_route(const struct sockaddr *sa_remote)
+{
+    ucs_netlink_rt_rules_t iface_rules;
+    ucs_netlink_route_info_t info = {.sa_remote = sa_remote};


Suggested change

ucs_netlink_route_info_t info = {.sa_remote = sa_remote};

ucs_netlink_route_info_t info = {

.sa_remote = sa_remote,

.found = 0,

.found_default_gw = 0

};

roiedanino · 2025-12-03T11:39:45Z

+    ucs_netlink_route_info_t info    = {.if_index  = if_index,
+                                        .sa_remote = sa_remote};


initialize found_defualt_gw to 0

roiedanino · 2025-12-03T12:02:45Z

+/**
+ * Check whether a specific route exists for a given destination address
+ * in ANY network interface.
+ *
+ * @param [in]  sa_remote         Pointer to the destination address.
+ *
+ * @return 1 if a specific route exists in any interface, or 0 otherwise.
+ */
+int ucs_netlink_has_any_specific_route(const struct sockaddr *sa_remote);


Suggested change

/**

* Check whether a specific route exists for a given destination address

* in ANY network interface.

*

* @param [in] sa_remote Pointer to the destination address.

*

* @return 1 if a specific route exists in any interface, or 0 otherwise.

*/

int ucs_netlink_has_any_specific_route(const struct sockaddr *sa_remote);

/**

* Check whether a non-default route exists for a given destination address

* in ANY network interface.

*

* @param [in] sa_remote Pointer to the destination address.

*

* @return 1 if a non-default route exists in any interface, or 0 otherwise.

*/

int ucs_netlink_has_non_default_route(const struct sockaddr *sa_remote);

roiedanino · 2025-12-03T12:05:28Z

-            uct_iface_fill_info_str_buf(params,
-                                        "loopback iface index is not found");
-            return 0;
+    if (ucs_netlink_route_exists(ndev_index, sa_remote, &has_default_gw)) {


has_default_gw is not used, so it would be a bit cleaner to use an enum as a return type like @gleon99 suggested

roiedanino · 2025-12-03T13:33:28Z

This PR might solve #11035?

yosefe

If it is not trivial to fix the review comments, pls submit a PR to workaround the failing CI test in meantime, or push this PR as is and then rework/revert it

yosefe · 2025-12-07T04:01:38Z

+        /* No route found - first, try loopback interface for reachability
+        * check, because it may be used for routing in case of an interface with
+        * VRF is configured and a RoCE IP interface uses this VRF table for
+        * routing. */
+        if (uct_ib_iface_get_loopback_ndev_index(&lo_ndev_index) == UCS_OK) {


BTW, shouldn't VRF be inside "netlink" code because it is also relevant for TCP?

amastbaum · 2025-12-07T12:47:49Z


-int ucs_netlink_route_exists(int if_index, const struct sockaddr *sa_remote,
-                             int allow_default_gw)
+static void ucs_netlink_init_routing_table_cache(void)


I could actually leave it as before and only trigger the cache initialization from ucs_netlink_route_exists, because ucs_netlink_route_exists is eventually called from ucs_netlink_is_best_route as well.
I just wasn't sure what is the best way to initialize the cache because I want to promise it will be triggered from each api call, and it's currently done quite implicitly. @yosefe @gleon99

Maybe init the cache in ucs_netlink_lookup_route?

yosefe · 2025-12-07T16:16:33Z

-    info.sa_remote        = sa_remote;
-    info.found            = 0;
-    info.allow_default_gw = allow_default_gw;
+static int ucs_netlink_has_better_route(const struct sockaddr *sa_remote,


maybe return the longest matching netmask length (-1 if not found)?

So we should rename this function to ucs_netlink_max_netmask_len

yosefe · 2025-12-07T16:22:01Z

    }

-    allow_default_gw = !!(tcp_dev_addr->flags &
-                          UCT_TCP_DEVICE_ADDR_FLAG_ALLOW_DEFAULT_GW);


can we remove UCT_TCP_DEVICE_ADDR_FLAG_ALLOW_DEFAULT_GW flag and related logic?

yosefe · 2025-12-07T16:35:18Z

+int ucs_netlink_is_this_the_route(int if_index,
+                                  const struct sockaddr *sa_remote);


ucs_netlink_is_preferred_route

@yosefe This is outdated, currently called ucs_netlink_is_best_route.
preferred is better?

roiedanino · 2025-12-08T11:33:18Z

-    int                    found;
-    int                    allow_default_gw; /* Allow matching default
-                                                gateway routes */
+    int                    found_netmask_len;


Or maybe just

Suggested change

int found_netmask_len;

int netmask_len;

yosefe · 2025-12-09T06:58:34Z


    ucs_array_for_each(curr_entry, iface_rules) {
-        if (curr_entry->subnet_prefix_len > info->found_netmask_len &&
+        if (curr_entry->subnet_prefix_len > found_netmask_len &&


roiedanino · 2025-12-09T13:02:11Z

+    ucs_netlink_route_info_t info = {.if_index    = if_index,
+                                     .sa_remote   = sa_remote,
+                                     .netmask_len = -1};


Minor: formatting

Suggested change

ucs_netlink_route_info_t info = {.if_index = if_index,

.sa_remote = sa_remote,

.netmask_len = -1};

ucs_netlink_route_info_t info = {

.if_index = if_index,

.sa_remote = sa_remote,

.netmask_len = -1

};

It seems that there are multiple clang-format issues
check git-clang-format --diff HEAD~13

Other than that, it's approved on my side

yosefe · 2025-12-09T20:06:55Z

        }
-    });
+    })
+        ;


can we remove this ;

gleon99 · 2025-12-10T11:07:53Z

@amastbaum please squash

amastbaum added the WIP-DNM Work in progress / Do not review label Dec 2, 2025

amastbaum commented Dec 2, 2025

View reviewed changes

amastbaum force-pushed the deprioritize_default_gw_routes branch from 133dd3e to 379c114 Compare December 2, 2025 13:58

amastbaum added Ready for Review and removed WIP-DNM Work in progress / Do not review labels Dec 2, 2025

amastbaum requested a review from gleon99 December 2, 2025 13:59

amastbaum changed the title ~~Deprioritize default gw routes~~ UCT: prioritize non-default GW rules in the reachability check Dec 2, 2025

gleon99 reviewed Dec 2, 2025

View reviewed changes

gleon99 requested a review from roiedanino December 2, 2025 17:35

roiedanino reviewed Dec 3, 2025

View reviewed changes

amastbaum requested review from gleon99 and roiedanino December 4, 2025 13:21

yosefe reviewed Dec 7, 2025

View reviewed changes

amastbaum closed this Dec 7, 2025

amastbaum deleted the deprioritize_default_gw_routes branch December 7, 2025 09:23

amastbaum restored the deprioritize_default_gw_routes branch December 7, 2025 09:30

amastbaum reopened this Dec 7, 2025

amastbaum force-pushed the deprioritize_default_gw_routes branch from 1bd31fa to 37bcdb7 Compare December 7, 2025 12:39

amastbaum requested a review from yosefe December 7, 2025 12:40

amastbaum commented Dec 7, 2025

View reviewed changes

yosefe reviewed Dec 7, 2025

View reviewed changes

amastbaum requested a review from yosefe December 8, 2025 08:55

roiedanino reviewed Dec 8, 2025

View reviewed changes

yosefe reviewed Dec 9, 2025

View reviewed changes

amastbaum requested review from roiedanino and yosefe December 9, 2025 09:00

roiedanino reviewed Dec 9, 2025

View reviewed changes

roiedanino previously approved these changes Dec 9, 2025

View reviewed changes

yosefe reviewed Dec 9, 2025

View reviewed changes

Comment thread src/ucs/sys/netlink.c Outdated

}

});

})

;

yosefe Dec 9, 2025

Copy link
Copy Markdown

Member

Choose a reason for hiding this comment

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

can we remove this ;

amastbaum reacted with thumbs up emoji

amastbaum dismissed roiedanino’s stale review via e67e15b December 10, 2025 08:27

amastbaum requested a review from yosefe December 10, 2025 08:27

yosefe approved these changes Dec 10, 2025

View reviewed changes

UCT: prioritize routes with longer subnet masks

9cce62c

amastbaum force-pushed the deprioritize_default_gw_routes branch from e67e15b to 9cce62c Compare December 10, 2025 11:19

amastbaum changed the title ~~UCT: prioritize non-default GW rules in the reachability check~~ UCT: prioritize routes with longer subnet masks in the reachability check Dec 10, 2025

yosefe enabled auto-merge December 10, 2025 16:57

yosefe merged commit 992ee5f into openucx:master Dec 10, 2025
147 checks passed

amastbaum mentioned this pull request Dec 11, 2025

UCT: Prioritize longer netmask routes - v1.20.x #11051

Merged

amastbaum mentioned this pull request Aug 27, 2026

UCT/IB/GDAKI: Relax RoCE route check for GPU-associated NICs #11835

Merged

paochiang mentioned this pull request Oct 10, 2026

[Bug] Unknown UCX TCP error for p1d1 deployment of deepseek v3.2 sgl-project/sglang#17860

Closed

4 of 5 tasks

		ucs_netlink_route_info_t info = {.if_index = if_index,
		.sa_remote = sa_remote};

		int ucs_netlink_is_this_the_route(int if_index,
		const struct sockaddr *sa_remote);

Conversation

amastbaum commented Dec 2, 2025

What?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gleon99 commented Dec 2, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yosefe commented Dec 2, 2025

Uh oh!

roiedanino Dec 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roiedanino Dec 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roiedanino Dec 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roiedanino commented Dec 3, 2025

Uh oh!

yosefe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amastbaum Dec 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amastbaum Dec 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

roiedanino Dec 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roiedanino Dec 3, 2025 •

edited

Loading

roiedanino Dec 3, 2025 •

edited

Loading

roiedanino Dec 3, 2025 •

edited

Loading

amastbaum Dec 7, 2025 •

edited

Loading

amastbaum Dec 8, 2025 •

edited

Loading

roiedanino Dec 9, 2025 •

edited

Loading