On Wed, Aug 19, 2026 at 5:15 AM Dumitru Ceara <[email protected]> wrote: > > Hi Han, > > Thanks for the patch!
Hi Dumitru, Thanks a lot for the review! All of your comments are addressed in v2, details below. > > On 7/21/26 9:03 AM, Han Zhou wrote: > > Routes can carry lightweight tunnel (LWT) encapsulation metadata; for IP > > encapsulation this includes a tunnel id (e.g. a VXLAN VNI). FRR uses > > this to attach the L3 VNI to EVPN type-5 routes. > > > > Parse the LWT IP/IP6 encap tunnel id (LWTUNNEL_IP_ID) from RTA_ENCAP into > > a new 'vni' field in struct route_data, so that users of the route-table > > library (such as OVN) can learn it. > > Why only parse the VNI though? Even the test you're adding is setting > an explicit LWTUNNEL_IP_DST. > > I understand that in your OVN use case only the VNI is set on the route > but this is a generic library. I think it might be nicer if we parsed > all the lwtunnel attributes. > Agreed, v2 parses all attributes of the LWTUNNEL_ENCAP_IP and LWTUNNEL_ENCAP_IP6 encapsulations (id, dst, src, ttl/hoplimit, tos/tc and flags) into a new struct route_data_lwt_tunnel. I did not go as far as parsing LWTUNNEL_IP_OPTS (the nested Geneve/VXLAN/ERSPAN options); it seemed better to leave that to whoever first needs it. Happy to add it if you'd prefer completeness here. > > > > Assisted-by: Claude Opus 4.8, Cursor> > Signed-off-by: Han Zhou < [email protected]> > > --- > > lib/route-table.c | 42 ++++++++++++++++++++++++++++++++++++ > > lib/route-table.h | 6 ++++++ > > tests/system-route.at | 14 ++++++++++++ > > tests/test-lib-route-table.c | 6 +++++- > > 4 files changed, 67 insertions(+), 1 deletion(-) > > > > diff --git a/lib/route-table.c b/lib/route-table.c > > index 2a13a5cc7d93..ecaf6fc4e42b 100644 > > --- a/lib/route-table.c > > +++ b/lib/route-table.c > > @@ -27,6 +27,7 @@ > > #include <linux/rtnetlink.h> > > #include <net/if.h> > > > > +#include "byte-order.h" > > #include "coverage.h" > > #include "hash.h" > > #include "netdev.h" > > @@ -48,6 +49,16 @@ > > #define FRA_SUPPRESS_PREFIXLEN 14 /* Linux 3.12 */ > > #define FRA_TABLE 15 /* Linux 2.6.19 */ > > #define FRA_PROTOCOL 21 /* Linux 4.17 */ > > +#define RTA_ENCAP_TYPE 21 /* Linux 4.3 */ > > +#define RTA_ENCAP 22 /* Linux 4.3 */ > > + > > +/* Lightweight tunnel encapsulation types and attributes (Linux 4.3+), > > + * from linux/lwtunnel.h. Defined here so we can parse EVPN type-5 routes > > + * that carry the L3 VNI without depending on newer build headers. */ > > +#define OVN_LWTUNNEL_ENCAP_IP 2 > > +#define OVN_LWTUNNEL_ENCAP_IP6 4 > > Why don't you use the uapi/linux/lwtunnel.h definitions directly > (LWTUNNEL_ENCAP_IP and LWTUNNEL_ENCAP_IP6)? Having OVN specific macro > names here reads weird. Ack. Fixed in v2. > > > +/* LWTUNNEL_IP_ID and LWTUNNEL_IP6_ID share the same attribute number (1). */ > > +#define OVN_LWTUNNEL_IP_ID 1 > > > > /* Linux 4.1 added RTA_VIA. */ > > #ifndef HAVE_RTA_VIA > > @@ -468,6 +479,8 @@ route_table_parse__(struct ofpbuf *buf, size_t ofs, > > [RTA_PRIORITY] = { .type = NL_A_U32, .optional = true }, > > [RTA_VIA] = { .type = NL_A_RTA_VIA, .optional = true }, > > [RTA_MULTIPATH] = { .type = NL_A_NESTED, .optional = true }, > > + [RTA_ENCAP_TYPE] = { .type = NL_A_U16, .optional = true }, > > + [RTA_ENCAP] = { .type = NL_A_NESTED, .optional = true }, > > }; > > > > static const struct nl_policy policy6[] = { > > @@ -480,6 +493,8 @@ route_table_parse__(struct ofpbuf *buf, size_t ofs, > > [RTA_PRIORITY] = { .type = NL_A_U32, .optional = true }, > > [RTA_VIA] = { .type = NL_A_RTA_VIA, .optional = true }, > > [RTA_MULTIPATH] = { .type = NL_A_NESTED, .optional = true }, > > + [RTA_ENCAP_TYPE] = { .type = NL_A_U16, .optional = true }, > > + [RTA_ENCAP] = { .type = NL_A_NESTED, .optional = true }, > > }; > > > > struct nlattr *attrs[ARRAY_SIZE(policy)]; > > @@ -585,6 +600,27 @@ route_table_parse__(struct ofpbuf *buf, size_t ofs, > > if (attrs[RTA_PRIORITY]) { > > change->rd.rta_priority = nl_attr_get_u32(attrs[RTA_PRIORITY]); > > } > > + if (attrs[RTA_ENCAP_TYPE] && attrs[RTA_ENCAP]) { > > + uint16_t encap_type = nl_attr_get_u16(attrs[RTA_ENCAP_TYPE]); > > + > > + if (encap_type == OVN_LWTUNNEL_ENCAP_IP || > > + encap_type == OVN_LWTUNNEL_ENCAP_IP6) { > > + static const struct nl_policy encap_policy[] = { > > + [OVN_LWTUNNEL_IP_ID] = { .type = NL_A_BE64, > > + .optional = true }, > > + }; > > + struct nlattr *encap_attrs[ARRAY_SIZE(encap_policy)]; > > + > > + if (nl_parse_nested(attrs[RTA_ENCAP], encap_policy, > > + encap_attrs, ARRAY_SIZE(encap_policy)) > > + && encap_attrs[OVN_LWTUNNEL_IP_ID]) { > > + ovs_be64 id = > > + nl_attr_get_be64(encap_attrs[OVN_LWTUNNEL_IP_ID]); > > + change->rd.vni = ntohll(id); > > We truncate a 64bit integer into 32bits. Is that acceptable? I > understand for VXLAN VNI that's fine but is that true in general? > Fixed in v2. > > + change->rd.vni_present = true; > > + } > > + } > > + } > > if (attrs[RTA_VIA]) { > > const struct rtvia *rtvia = nl_attr_get(attrs[RTA_VIA]); > > ovs_be32 addr; > > @@ -668,6 +704,12 @@ route_table_parse__(struct ofpbuf *buf, size_t ofs, > > } > > ovs_list_push_back_all(&change->rd.nexthops, > > &mp_change.rd.nexthops); > > + /* Per-nexthop LWT encap may carry the VNI. Adopt the first > > + * one seen for the route as a whole. */ > > + if (mp_change.rd.vni_present && !change->rd.vni_present) { > > + change->rd.vni = mp_change.rd.vni; > > + change->rd.vni_present = true; > > + } > > } > > } > > if (route_type_needs_nexthop(rtm->rtm_type) > > diff --git a/lib/route-table.h b/lib/route-table.h > > index b49fbb14ebe0..672d4e96d1d7 100644 > > --- a/lib/route-table.h > > +++ b/lib/route-table.h > > @@ -141,6 +141,12 @@ struct route_data { > > uint32_t rta_mark; /* 0 if missing. */ > > uint32_t rta_table_id; /* 0 if missing. */ > > uint32_t rta_priority; /* 0 if missing. */ > > + > > + /* Lightweight tunnel (LWT) encapsulation, e.g. as used by EVPN type-5 > > + * routes to carry the L3 VNI (RTA_ENCAP_TYPE == LWTUNNEL_ENCAP_IP/IP6 > > + * with a nested LWTUNNEL_IP_ID/LWTUNNEL_IP6_ID tunnel id). */ > > + bool vni_present; /* True if 'vni' was extracted. */ > > Tunnel ID 0 is probably valid so I guess that's why we need this bool. > Yes. > > + uint32_t vni; /* Valid only if 'vni_present'. */ > > Maybe VNI is too VXLAN-specific? In the end this is just a tunnel ID. > Agreed, it's 'id' now, documented as a tunnel id with the VXLAN VNI as just one example. > > }; > > > > struct rule_data { > > diff --git a/tests/system-route.at b/tests/system-route.at > > index a074c51f9fd0..ed70a2443e76 100644 > > --- a/tests/system-route.at > > +++ b/tests/system-route.at > > @@ -331,6 +331,20 @@ AT_CHECK([ovstest test-lib-route-table-dump | \ > > 192.168.10.12/32 rtm_protocol: RTPROT_BOOT > > ]) > > > > +dnl Add route with a lightweight-tunnel (LWT) IP encapsulation tunnel id, as > > +dnl used to carry an EVPN L3 VNI. The tunnel id must be parsed into the route's > > +dnl vni field. > > +AT_CHECK([ip route add 192.168.10.13/32 encap ip id 5000 dst 10.0.0.19 \ > > + dev p1-route via 10.0.0.18], [0], [stdout]) > > +AT_CHECK([ovstest test-lib-route-table-dump | grep '^192.168.10.13' | \ > > + grep -o 'vni: [[0-9]]*'], [0], [dnl > > +vni: 5000 > > +]) > > + > > +dnl A route without an encap id has no vni field. > > +AT_CHECK([ovstest test-lib-route-table-dump | grep '^192.168.10.12' | \ > > + grep 'vni:'], [1]) > > + > > I think it would probably be a good idea to add tests for ip6 encaps too. Ack Please take a look at v2: https://patchwork.ozlabs.org/project/openvswitch/patch/[email protected]/ Thanks, Han > > > AT_CLEANUP > > > > dnl Checks that OVS ignores unsupported routing rules. > > diff --git a/tests/test-lib-route-table.c b/tests/test-lib-route-table.c > > index f99f056c8ddc..3bdaf517f132 100644 > > --- a/tests/test-lib-route-table.c > > +++ b/tests/test-lib-route-table.c > > @@ -86,11 +86,15 @@ test_lib_route_table_handle_msg(const struct route_table_msg *change, > > > > printf("%s/%u relevant: %d nlmsg_type: %d rtm_protocol: %s (%u) " > > "rtn_local: %d rta_prefsrc: %s rta_mark: %"PRIu32" " > > - "rta_table_id: %s rta_priority: %"PRIu32"\n", > > + "rta_table_id: %s rta_priority: %"PRIu32, > > ds_cstr(&rta_dst), rd->rtm_dst_len, change->relevant, > > change->nlmsg_type, rt_prot_name(rd->rtm_protocol), > > rd->rtm_protocol, rd->rtn_local, ds_cstr(&rta_prefsrc), > > rd->rta_mark, rt_table_name(rd->rta_table_id), rd->rta_priority); > > + if (rd->vni_present) { > > + printf(" vni: %"PRIu32, rd->vni); > > + } > > + printf("\n"); > > > > LIST_FOR_EACH (rdnh, nexthop_node, &rd->nexthops) { > > ds_clear(&nexthop_addr); > > Regards, > Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
