issue: 4219010 XLIO support for kernel 6.10 #278

tomerdbz · 2024-12-19T08:24:29Z

Description

Kernel 6.10 netlink has breaked XLIO functionality. Transitioned to libnl - an abstraction that wraps netlink.

This both solves the issue and makes us more robust.

What

Build routing and rules table using libnl instead of netlink.

Why ?

solves https://redmine.mellanox.com/issues/4219010

How ?

Tested on kernel 5.21 and kernel 6.10

Change type

What kind of change does this PR introduce?

Check list

Code follows the style de facto guidelines of this project
Comments have been inserted in hard to understand places
Documentation has been updated (if necessary)
Test has been added (if possible)

tomerdbz · 2025-02-03T08:55:07Z

bot:retest

pasis · 2025-02-10T21:13:27Z

[question] Will this implementation handle multipath routes? AFAIR, in the previous manual parsing didn't support the RTA_MULTIPATH attribute and the routing was broken in this case. Will the libnl implementation use the 1st nexthop regardless of the type?

galnoam · 2025-02-27T20:37:06Z

bot:retest

tomerdbz · 2025-03-31T09:39:23Z

bot:retest

pasis

review is not finished yet

pasis · 2025-03-31T11:06:57Z

src/core/proto/netlink_socket_mgr.cpp

    }

-    delete[] buf;
-    __log_dbg("Done");
+    parse_tbl(cache_state);
 }


the cache needs to be destroyed at the end.

pasis · 2025-03-31T11:07:14Z

src/core/proto/netlink_socket_mgr.cpp

 #include <unistd.h> // getpid()

+#include <netlink/route/route.h>
+#include <netlink/route/rule.h>
+#include <netlink/route/link.h>


Looks like link.h is unused

pasis · 2025-03-31T11:11:10Z

src/core/proto/route_table_mgr.cpp

@@ -152,113 +152,117 @@ void route_table_mgr::update_tbl(nl_data_t data_type)
    netlink_socket_mgr::update_tbl(data_type);
 }

-void route_table_mgr::parse_entry(struct nlmsghdr *nl_header)
+void route_table_mgr::parse_entry(struct nl_object *nl_obj)


add netlink/route/route.h and netlink/netlink.h includes.
Currently the route.h is included implicitly from netlink_event.h which can be rewritten in the future to generalize the netlink related code. Besides, we can do forward declarations for few netlink structures and avoid netlink in the XLIO headers.

pasis · 2025-03-31T11:12:25Z

src/core/proto/rule_table_mgr.cpp

-//		p_val		: custom object that contain parsed rule data.
-// return true if its not related to local or default table, false otherwise.
-void rule_table_mgr::parse_entry(struct nlmsghdr *nl_header)
+void rule_table_mgr::parse_entry(struct nl_object *nl_obj)


add netlink/route/rule.h and netlink/netlink.h includes. See rationale at route_table_mgr

pasis · 2025-03-31T11:12:58Z

tests/gtest/tcp/tcp_send.cc

@@ -184,6 +184,8 @@ TEST_F(tcp_send, null_iov_elements)
                    vec[1].iov_len = 1000U;
                    rcs = sendmsg(fd, &msg, 0);
                    EXPECT_LE_ERRNO(rcs, 0);
+                    EXPECT_EQ(0, rcs);
+                    EXPECT_EQ(14, errno); // TODO - remove after check


should it be removed as TODO says?

yup - removed it

pasis · 2025-03-31T11:14:40Z

src/core/proto/rule_table_mgr.cpp

-        break;
+    // FRA_PRIORITY: Rule Priority
+    uint32_t priority = rtnl_rule_get_prio(rule);
+    if (priority) {


is 0 a valid priority?

yes - fixed it

pasis · 2025-03-31T11:28:07Z

src/core/proto/rule_table_mgr.cpp

+    if (table_id) {
+        val.set_table_id(table_id);
+    }
+
 #if DEFINED_FRA_OIFNAME


Remove this ifdef and remove its definition in configure.ac. All libnl-3 versions provide rtnl_rule_get_oif().

tomerdbz · 2025-04-02T11:38:02Z

bot:retest

tomerdbz · 2025-04-02T16:41:57Z

bot:retest

pasis · 2025-04-03T03:53:34Z

src/core/proto/route_table_mgr.cpp

+    }
+
+    // Gateway Address (Next Hop)
+    struct rtnl_nexthop *nh = rtnl_route_nexthop_n(route, 0); // Assuming the first nexthop


since libnl treats non-multipath nexthop as multipath internally, can we assume that that the multipath loop below handles all the cases and current code block is redundant?

relying on this internal behavior may not be robust in the long term.

The internal implementation details of libnl could change in future updates, which might break our code if we assume this behavior.

To ensure the robustness and maintainability of our code, it is safer to explicitly handle non-multipath nexthop cases separately. This approach will protect us against any potential changes in libnl's internal handling of nexthops.

pasis · 2025-04-03T03:58:52Z

src/core/proto/rule_table_mgr.h

 #include "core/proto/netlink_socket_mgr.h"
 #include "route_rule_table_key.h"
 #include "rule_entry.h"
 #include "rule_val.h"
+#include <netlink/route/rule.h>
+#include <netlink/route/link.h>


looks like unused header

tomerdbz · 2025-04-20T06:27:25Z

bot:retest

tomerdbz · 2025-04-21T06:25:12Z

/review

pr-review-bot-app · 2025-04-21T06:25:54Z

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review Possible Exception Handling Issue The new `update_tbl` function uses `throw_xlio_exception` for error handling, but it does not log specific details about the error (e.g., `err` values). Consider adding more detailed logging to aid debugging. // Update data in a table void netlink_socket_mgr::update_tbl(nl_data_t data_type) { nl_sock sockfd = nullptr; BULLSEYE_EXCLUDE_BLOCK_START sockfd = nl_socket_alloc(); if (sockfd == nullptr) { __log_err("NL socket Creation: "); throw_xlio_exception("Failed nl_socket_alloc"); } if (nl_connect(sockfd, NETLINK_ROUTE) < 0) { __log_err("NL socket Connection: "); nl_socket_free(sockfd); throw_xlio_exception("Failed nl_connect"); } struct nl_cache cache_state = {0}; int err = 0; // cache allocation fetches the latest existing rules/routes if (data_type == RULE_DATA_TYPE) { err = rtnl_rule_alloc_cache(sockfd, AF_INET, &cache_state); } else if (data_type == ROUTE_DATA_TYPE) { err = rtnl_route_alloc_cache(sockfd, AF_INET, 0, &cache_state); } if (err < 0) { nl_socket_free(sockfd); throw_xlio_exception("Failed to allocate route cache"); } parse_tbl(cache_state); nl_cache_free(cache_state); nl_socket_free(sockfd); } Multi-path Handling Logic The multi-path handling logic in `parse_attr` uses a lambda to determine the best next hop based on weight. Ensure this logic is robust and handles edge cases, such as missing or invalid weights. struct nexthop_iterator_context { struct rtnl_nexthop best_next_hop; uint8_t best_next_hop_weight; } best_next_hop_context = {.best_next_hop = nullptr, .best_next_hop_weight = 0xff}; // Multi-path rtnl_route_foreach_nexthop( route, [](struct rtnl_nexthop next_hop, void context) { nexthop_iterator_context best_next_hop = (nexthop_iterator_context )context; const uint8_t current_nh_weight = rtnl_route_nh_get_weight(next_hop); // min valid weight is 1 if (current_nh_weight > 0 && current_nh_weight < best_next_hop->best_next_hop_weight) { best_next_hop->best_next_hop_weight = current_nh_weight; best_next_hop->best_next_hop = next_hop; } }, &best_next_hop_context); if (best_next_hop_context.best_next_hop != nullptr) { struct rtnl_nexthop best_next_hop = best_next_hop_context.best_next_hop; const auto nh_gateway = rtnl_route_nh_get_gateway(best_next_hop); const ip_address &gw_addr = ip_address(nl_addr_get_binary_addr(nh_gateway), nl_addr_get_family(nh_gateway)); val.set_if_index(rtnl_route_nh_get_ifindex(best_next_hop)); char nh_if_name[IFNAMSIZ] = {0}; if_indextoname(val.get_if_index(), nh_if_name); val.set_if_name(nh_if_name); val.set_gw(gw_addr); // Set destination mask and prefix length struct nl_addr dst = rtnl_route_nh_get_newdst(best_next_hop); if (dst) { val.set_dst_pref_len(nl_addr_get_prefixlen(dst)); } } } Error Handling in Rule Parsing* The `parse_entry` function throws an exception if `rtnl_rule_get_protocol` fails. Consider whether this is the best approach or if fallback logic should be implemented for resilience. // Parse received rule entry into custom object (rule_val). void rule_table_mgr::parse_entry(struct nl_object nl_obj) { int err = 0; rule_val val; // Cast the generic nl_object to a specific route or rule object struct rtnl_rule rule = reinterpret_cast<struct rtnl_rule *>(nl_obj); // Set rule properties in p_val using libnl getters uint8_t protocol = 0; err = rtnl_rule_get_protocol(rule, &protocol); if (err < 0) { throw_xlio_exception("Failed to get rule protocol"); } val.set_family(rtnl_rule_get_family(rule)); val.set_protocol(protocol); val.set_tos(rtnl_rule_get_dsfield(rule)); val.set_table_id(rtnl_rule_get_table(rule)); parse_attr(rule, val); val.set_state(true); rule_table_t &table = val.get_family() == AF_INET ? m_table_in4 : m_table_in6; table.push_back(val); }

galnoam · 2025-05-05T11:26:47Z

bot:retest

tomerdbz · 2025-05-12T08:46:59Z

bot:retest

tomerdbz · 2025-05-14T11:12:56Z

bot:retest

tomerdbz · 2025-05-14T16:44:13Z

bot:retest

tomerdbz · 2025-05-15T07:44:02Z

bot:retest

dpressle · 2025-05-15T09:01:37Z

bot:retest

tomerdbz · 2025-06-09T08:14:03Z

bot:retest

tomerdbz · 2025-06-10T06:13:15Z

bot:retest

Kernel 6.10 netlink has breaked XLIO functionality. Transitioned to libnl - an abstraction that wraps netlink. This both solves the issue and makes us more robust. Signed-off-by: Tomer Cabouly <[email protected]>

tomerdbz · 2025-07-08T06:11:51Z

bot:retest

tomerdbz · 2025-07-10T12:02:37Z

bot:retest

tomerdbz requested review from galnoam, iftahl and AlexanderGrissik December 19, 2024 08:24

tomerdbz force-pushed the routing_to_libnl branch from e2450d0 to e54e7f3 Compare December 19, 2024 16:06

tomerdbz force-pushed the routing_to_libnl branch from e54e7f3 to 224e851 Compare January 5, 2025 09:02

tomerdbz requested review from svc-codecritic and removed request for iftahl January 5, 2025 17:45

tomerdbz requested a review from pasis February 3, 2025 08:55

pasis requested changes Mar 31, 2025

View reviewed changes

tomerdbz force-pushed the routing_to_libnl branch 3 times, most recently from 76995b1 to b4415b6 Compare April 1, 2025 06:27

pasis reviewed Apr 3, 2025

View reviewed changes

tomerdbz force-pushed the routing_to_libnl branch from b4415b6 to ff8f15e Compare April 3, 2025 06:25

pasis approved these changes Apr 3, 2025

View reviewed changes

tomerdbz force-pushed the routing_to_libnl branch from ff8f15e to f414448 Compare April 21, 2025 06:24

pr-review-bot-app bot added the Review effort 4/5 label Apr 21, 2025

tomerdbz force-pushed the routing_to_libnl branch from f414448 to 8b4ad1a Compare July 7, 2025 09:09

issue: 4219010 XLIO support for kernel 6.10

3e71874

Kernel 6.10 netlink has breaked XLIO functionality. Transitioned to libnl - an abstraction that wraps netlink. This both solves the issue and makes us more robust. Signed-off-by: Tomer Cabouly <[email protected]>

tomerdbz force-pushed the routing_to_libnl branch from 8b4ad1a to 3e71874 Compare July 7, 2025 13:07

issue: 4219010 XLIO support for kernel 6.10 #278

Are you sure you want to change the base?

issue: 4219010 XLIO support for kernel 6.10 #278

Uh oh!

Conversation

tomerdbz commented Dec 19, 2024

Description

What

Why ?

How ?

Change type

Check list

Uh oh!

tomerdbz commented Feb 3, 2025

Uh oh!

pasis commented Feb 10, 2025

Uh oh!

galnoam commented Feb 27, 2025

Uh oh!

tomerdbz commented Mar 31, 2025

Uh oh!

pasis left a comment

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!

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!

tomerdbz commented Apr 2, 2025

Uh oh!

tomerdbz commented Apr 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!

tomerdbz commented Apr 20, 2025

Uh oh!

tomerdbz commented Apr 21, 2025

Uh oh!

pr-review-bot-app bot commented Apr 21, 2025

PR Reviewer Guide 🔍

Uh oh!

galnoam commented May 5, 2025

Uh oh!

tomerdbz commented May 12, 2025

Uh oh!

tomerdbz commented May 14, 2025

Uh oh!

tomerdbz commented May 14, 2025

Uh oh!

tomerdbz commented May 15, 2025