On 20 Dec 2024, at 11:53, Frode Nordahl wrote:
> fre. 20. des. 2024, 11:35 skrev Eelco Chaudron <[email protected]>: > >> >> >> On 18 Dec 2024, at 20:21, Frode Nordahl wrote: >> >>> On Wed, Dec 18, 2024 at 8:12 PM Frode Nordahl <[email protected]> >> wrote: >>>> >>>> In preparation for adding support for parsing routes with multiple >>>> next hops per destination, we need dynamic storage for them. >>>> >>>> Storage for the primary next hop is set aside in struct route_data >>>> to avoid unnecessary memory allocations for the common case of one >>>> next hop. >>>> >>>> The ovs-router module does not support storing multiple next hop >>>> information, and the existing parser does not understand them, so >>>> we retain this behavior by only passing single next hop routes for >>>> storage in ovs-router. >>>> >>>> This patch makes this change in isolation so that it is easier to >>>> review subsequent patches. >>>> >>>> Signed-off-by: Frode Nordahl <[email protected]> >>>> --- >>>> lib/route-table.c | 94 ++++++++++++++++++++++++++++++++++++++++------- >>>> 1 file changed, 81 insertions(+), 13 deletions(-) >>>> >>>> diff --git a/lib/route-table.c b/lib/route-table.c >>>> index 4e8f35199..b6f0223c0 100644 >>>> --- a/lib/route-table.c >>>> +++ b/lib/route-table.c >>>> @@ -32,6 +32,7 @@ >>>> #include "netlink.h" >>>> #include "netlink-notifier.h" >>>> #include "netlink-socket.h" >>>> +#include "openvswitch/list.h" >>>> #include "openvswitch/ofpbuf.h" >>>> #include "ovs-router.h" >>>> #include "packets.h" >>>> @@ -47,7 +48,34 @@ VLOG_DEFINE_THIS_MODULE(route_table); >>>> >>>> COVERAGE_DEFINE(route_table_dump); >>>> >>>> +struct route_data_nexthop { >>>> + struct ovs_list nexthop_node; >>>> + >>>> + struct in6_addr addr; >>>> + char ifname[IFNAMSIZ]; /* Interface name. */ >>>> +}; >>>> + >>>> struct route_data { >>>> + /* Routes can have multiple next hops per destination. >>>> + * >>>> + * Each next hop has its own set of attributes such as address >> family, >>>> + * interface and IP address. >>>> + * >>>> + * When retrieving information about a route from the kernel, in >> the case >>>> + * of multiple next hops, information is provided as nested >> attributes. >>>> + * >>>> + * A linked list with struct route_data_nexthop entries is used to >> store >>>> + * this information as we parse each attribute. >>>> + * >>>> + * For the common case of one next hop, the nexthops list will >> contain a >>>> + * single entry pointing to the struct route_data _primary_next_hop >>>> + * element. >>>> + * >>>> + * Any dynamically allocated list elements can be freed with a >> call to the >>>> + * route_data_destroy function. */ >>>> + struct ovs_list nexthops; >>>> + struct route_data_nexthop _primary_next_hop; >>>> + >>>> /* Copied from struct rtmsg. */ >>>> unsigned char rtm_dst_len; >>>> unsigned char rtm_protocol; >>>> @@ -56,8 +84,6 @@ struct route_data { >>>> /* Extracted from Netlink attributes. */ >>>> struct in6_addr rta_dst; /* 0 if missing. */ >>>> struct in6_addr rta_prefsrc; /* 0 if missing. */ >>>> - struct in6_addr rta_gw; >>>> - char ifname[IFNAMSIZ]; /* Interface name. */ >>>> uint32_t mark; >>>> uint32_t rta_table_id; /* 0 if missing. */ >>>> uint32_t rta_priority; /* 0 if missing. */ >>>> @@ -89,12 +115,24 @@ static bool route_table_valid = false; >>>> static void route_table_reset(void); >>>> static void route_table_handle_msg(const struct route_table_msg *); >>>> static int route_table_parse(struct ofpbuf *, void *change); >>>> -static void route_table_change(const struct route_table_msg *, void *); >>>> +static void route_table_change(struct route_table_msg *, void *aux); >>>> static void route_map_clear(void); >>>> >>>> static void name_table_init(void); >>>> static void name_table_change(const struct rtnetlink_change *, void *); >>>> >>>> +static void >>>> +route_data_destroy(struct route_data *rd) >>>> +{ >>>> + struct route_data_nexthop *rdnh; >>>> + >>>> + LIST_FOR_EACH_POP (rdnh, nexthop_node, &rd->nexthops) { >>>> + if (rdnh && rdnh != &rd->_primary_next_hop) { >>>> + free(rdnh); >>>> + } >>>> + } >>>> +} >>>> + >>>> uint64_t >>>> route_table_get_change_seq(void) >>>> { >>>> @@ -190,6 +228,7 @@ route_table_dump_one_table(unsigned char id) >>>> filtered = false; >>>> } >>>> route_table_handle_msg(&msg); >>>> + route_data_destroy(&msg.rd); >>>> } >>>> } >>>> ofpbuf_uninit(&buf); >>>> @@ -222,13 +261,12 @@ route_table_reset(void) >>>> } >>>> } >>>> >>>> -/* Return RTNLGRP_IPV4_ROUTE or RTNLGRP_IPV6_ROUTE on success, 0 on >> parse >>>> - * error. */ >>>> static int >>>> route_table_parse__(struct ofpbuf *buf, size_t ofs, >>>> const struct nlmsghdr *nlmsg, >>>> const struct rtmsg *rtm, struct route_table_msg >> *change) >>>> { >>>> + struct route_data_nexthop *rdnh = NULL; >>>> bool parsed, ipv4 = false; >>>> >>>> static const struct nl_policy policy[] = { >>>> @@ -269,6 +307,12 @@ route_table_parse__(struct ofpbuf *buf, size_t ofs, >>>> int rta_oif; /* Output interface index. */ >>>> >>>> memset(change, 0, sizeof *change); >>>> + >>>> + /* ovs_list_init / ovs_list_insert does not allocate any >> memory */ >>>> + ovs_list_init(&change->rd.nexthops); >>>> + rdnh = &change->rd._primary_next_hop; >>>> + ovs_list_insert(&change->rd.nexthops, &rdnh->nexthop_node); >>>> + >>>> change->relevant = true; >>>> >>>> if (rtm->rtm_scope == RT_SCOPE_NOWHERE) { >>>> @@ -292,7 +336,7 @@ route_table_parse__(struct ofpbuf *buf, size_t ofs, >>>> if (attrs[RTA_OIF]) { >>>> rta_oif = nl_attr_get_u32(attrs[RTA_OIF]); >>>> >>>> - if (!if_indextoname(rta_oif, change->rd.ifname)) { >>>> + if (!if_indextoname(rta_oif, rdnh->ifname)) { >>>> int error = errno; >>>> >>>> VLOG_DBG_RL(&rl, "Could not find interface name[%u]: >> %s", >>>> @@ -300,7 +344,7 @@ route_table_parse__(struct ofpbuf *buf, size_t ofs, >>>> if (error == ENXIO) { >>>> change->relevant = false; >>>> } else { >>>> - return 0; >>>> + goto error_out; >>>> } >>>> } >>>> } >>>> @@ -330,9 +374,9 @@ route_table_parse__(struct ofpbuf *buf, size_t ofs, >>>> if (ipv4) { >>>> ovs_be32 gw; >>>> gw = nl_attr_get_be32(attrs[RTA_GATEWAY]); >>>> - in6_addr_set_mapped_ipv4(&change->rd.rta_gw, gw); >>>> + in6_addr_set_mapped_ipv4(&rdnh->addr, gw); >>>> } else { >>>> - change->rd.rta_gw = >> nl_attr_get_in6_addr(attrs[RTA_GATEWAY]); >>>> + rdnh->addr = nl_attr_get_in6_addr(attrs[RTA_GATEWAY]); >>>> } >>>> } >>>> if (attrs[RTA_MARK]) { >>>> @@ -343,13 +387,27 @@ route_table_parse__(struct ofpbuf *buf, size_t >> ofs, >>>> } >>>> } else { >>>> VLOG_DBG_RL(&rl, "received unparseable rtnetlink route >> message"); >>>> - return 0; >>>> + goto error_out; >>>> } >>>> >>>> /* Success. */ >>>> return ipv4 ? RTNLGRP_IPV4_ROUTE : RTNLGRP_IPV6_ROUTE; >>>> + >>>> +error_out: >>>> + route_data_destroy(&change->rd); >>>> + return 0; >>>> } >>>> >>>> +/* Parse Netlink message in buf, which is expected to contain UAPI >> rtmsg >>>> + * header and associated route attributes. >>>> + * >>>> + * Return RTNLGRP_IPV4_ROUTE or RTNLGRP_IPV6_ROUTE on success, 0 on >> parse >>>> + * error. >>>> + * >>>> + * On success, memory may have be allocated, and it is the callers >>>> + * responsibility to free it with a call to route_data_destroy. >>>> + * >>>> + * In case of error, any allocated memory will be freed before return. >> */ >>>> static int >>>> route_table_parse(struct ofpbuf *buf, void *change) >>>> { >>>> @@ -373,7 +431,7 @@ route_table_standard_table(uint32_t table_id) >>>> } >>>> >>>> static void >>>> -route_table_change(const struct route_table_msg *change OVS_UNUSED, >>>> +route_table_change(struct route_table_msg *change, >>>> void *aux OVS_UNUSED) >>>> { >>>> if (!change >>>> @@ -381,16 +439,26 @@ route_table_change(const struct route_table_msg >> *change OVS_UNUSED, >>>> && route_table_standard_table(change->rd.rta_table_id))) { >>>> route_table_valid = false; >>>> } >>>> + if (change) { >>>> + route_data_destroy(&change->rd); >>>> + } >>>> } >>>> >>>> static void >>>> route_table_handle_msg(const struct route_table_msg *change) >>>> { >>>> - if (change->relevant && change->nlmsg_type == RTM_NEWROUTE) { >>>> + if (change->relevant && change->nlmsg_type == RTM_NEWROUTE >>>> + && ovs_list_is_singleton(&change->rd.nexthops)) { >>> >>> Note that this is changed to !ovs_list_is_empty(&change->rd.nexthops) >>> in patch 15. >>> >>> I missed that I touched this in both patches, hope that's OK. >> >> I guess this needs to be fixed in this patch. > > > Ok, I'll hold off a bit in case there are other comments. > > However, please look at the robot page, as some patches (4, 6, and 7) are >> failing CI. >> >> https://patchwork.ozlabs.org/project/openvswitch/list/?series=437419 > > >> > I did take a look, and it looks like flaky tests to me, subsequent patches > have no such failures. They do not look like flaky tests (at least I did not see them in the past). I’ve requested the robot to recheck them. Please take a look once they are completed. //Eelco >>>> const struct route_data *rd = &change->rd; >>>> + const struct route_data_nexthop *rdnh; >>>> + >>>> + /* The ovs-router module does currently not implement lookup >> nor >>>> + * storage for routes with multiple next hops. */ >>>> + rdnh = CONTAINER_OF(ovs_list_front(&change->rd.nexthops), >>>> + const struct route_data_nexthop, >> nexthop_node); >>>> >>>> ovs_router_insert(rd->mark, &rd->rta_dst, rd->rtm_dst_len, >>>> - rd->local, rd->ifname, &rd->rta_gw, >>>> + rd->local, rdnh->ifname, &rdnh->addr, >>>> &rd->rta_prefsrc); >>>> } >>>> } >>>> -- >>>> 2.45.2 >>>> >> >> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
