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. 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 >> 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
