A route notification was reduced to the id of the table the route belongs to, because 'struct route_data' describes the next hops with a list that points back into itself and therefore cannot be copied. Whoever wanted to know what the change was had to read the whole table again.
Digest the message into a self-contained 'struct ovn_route_msg' instead, using the same trailing array as 'struct nexthop_entry', and hand the ones belonging to a watched table to the route_table_notify node. No behaviour change: route_exchange still recomputes on any notification and reads the tables again. This only makes the content available to it, which the next patch uses to learn routes incrementally. Assisted-by: Claude Opus 5, Cursor Signed-off-by: Han Zhou <[email protected]> --- controller/ovn-controller.c | 62 ++++++++++++++++----------- controller/ovn-netlink-notifier.c | 18 +++++--- controller/route-exchange-netlink.c | 66 +++++++++++++++++++++++++++++ controller/route-exchange-netlink.h | 38 +++++++++++++++++ tests/system-ovn-netlink.at | 10 ++++- tests/test-ovn-netlink.c | 24 +++++++---- 6 files changed, 177 insertions(+), 41 deletions(-) diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index 931fb95dccb3..7fa8f2adb563 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -94,6 +94,7 @@ #include "acl-ids.h" #include "route.h" #include "route-exchange.h" +#include "route-exchange-netlink.h" #include "route-table.h" #include "garp_rarp.h" #include "host-if-monitor.h" @@ -5712,8 +5713,21 @@ route_table_notify_update(struct vector *watches) struct ed_type_route_table_notify { /* Vector of ordered 'uint32_t' representing table_ids. */ struct vector watches; + /* Routes ('struct ovn_route_msg *', owned) the last run was told about, + * limited to the tables in 'watches'. */ + struct vector changed_routes; }; +static void +route_table_notify_clear_changes(struct ed_type_route_table_notify *rtn) +{ + struct ovn_route_msg *msg; + VECTOR_FOR_EACH (&rtn->changed_routes, msg) { + free(msg); + } + vector_clear(&rtn->changed_routes); +} + /* The kernel nexthop table is shared by the features below, each of them * independently declares whether it needs it to be tracked. */ enum nexthop_exchange_user { @@ -5888,40 +5902,35 @@ en_route_exchange_cleanup(void *data) static enum engine_node_state en_route_table_notify_run(struct engine_node *node OVS_UNUSED, void *data) { + static const enum ovn_netlink_notifier_type route_notifiers[] = { + OVN_NL_NOTIFIER_ROUTE_V4, OVN_NL_NOTIFIER_ROUTE_V6, + }; enum engine_node_state state = EN_UNCHANGED; struct ed_type_route_table_notify *rtn = data; - struct vector *msgs; - uint32_t *table_id; - /* We cannot tell whether a table we watch was among the changes we - * missed, so assume it was. */ - if (ovn_netlink_notifier_lost(OVN_NL_NOTIFIER_ROUTE_V4) || - ovn_netlink_notifier_lost(OVN_NL_NOTIFIER_ROUTE_V6)) { - state = EN_UPDATED; - } + route_table_notify_clear_changes(rtn); - if (state != EN_UPDATED) { - msgs = ovn_netlink_get_msgs(OVN_NL_NOTIFIER_ROUTE_V4); - VECTOR_FOR_EACH_PTR (msgs, table_id) { - if (vector_bsearch(&rtn->watches, table_id, table_id_cmp)) { - state = EN_UPDATED; - break; - } + for (size_t i = 0; i < ARRAY_SIZE(route_notifiers); i++) { + /* We cannot tell whether a table we watch was among the changes we + * missed, so assume it was. */ + if (ovn_netlink_notifier_lost(route_notifiers[i])) { + state = EN_UPDATED; } - } - if (state != EN_UPDATED) { - msgs = ovn_netlink_get_msgs(OVN_NL_NOTIFIER_ROUTE_V6); - VECTOR_FOR_EACH_PTR (msgs, table_id) { - if (vector_bsearch(&rtn->watches, table_id, table_id_cmp)) { - state = EN_UPDATED; - break; + struct vector *msgs = ovn_netlink_get_msgs(route_notifiers[i]); + struct ovn_route_msg *msg; + VECTOR_FOR_EACH (msgs, msg) { + if (!vector_bsearch(&rtn->watches, &msg->table_id, table_id_cmp)) { + continue; } + + struct ovn_route_msg *changed_route = ovn_route_msg_clone(msg); + vector_push(&rtn->changed_routes, &changed_route); + state = EN_UPDATED; } - } - ovn_netlink_notifier_flush(OVN_NL_NOTIFIER_ROUTE_V4); - ovn_netlink_notifier_flush(OVN_NL_NOTIFIER_ROUTE_V6); + ovn_netlink_notifier_flush(route_notifiers[i]); + } return state; } @@ -5935,6 +5944,7 @@ en_route_table_notify_init(struct engine_node *node OVS_UNUSED, *rtn = (struct ed_type_route_table_notify) { .watches = VECTOR_EMPTY_INITIALIZER(uint32_t), + .changed_routes = VECTOR_EMPTY_INITIALIZER(struct ovn_route_msg *), }; return rtn; } @@ -5943,6 +5953,8 @@ static void en_route_table_notify_cleanup(void *data) { struct ed_type_route_table_notify *rtn = data; + route_table_notify_clear_changes(rtn); + vector_destroy(&rtn->changed_routes); vector_destroy(&rtn->watches); } diff --git a/controller/ovn-netlink-notifier.c b/controller/ovn-netlink-notifier.c index 04db42d0e109..c772bdb7ca2c 100644 --- a/controller/ovn-netlink-notifier.c +++ b/controller/ovn-netlink-notifier.c @@ -66,13 +66,13 @@ static void ovn_netlink_notifier_report_lost(struct ovn_netlink_notifier *); static struct ovn_netlink_notifier notifiers[OVN_NL_NOTIFIER_MAX] = { [OVN_NL_NOTIFIER_ROUTE_V4] = { .group = RTNLGRP_IPV4_ROUTE, - .msgs = VECTOR_EMPTY_INITIALIZER(uint32_t), + .msgs = VECTOR_EMPTY_INITIALIZER(struct ovn_route_msg *), .change_handler = ovn_netlink_route_change_handler, .name = "route-ipv4", }, [OVN_NL_NOTIFIER_ROUTE_V6] = { .group = RTNLGRP_IPV6_ROUTE, - .msgs = VECTOR_EMPTY_INITIALIZER(uint32_t), + .msgs = VECTOR_EMPTY_INITIALIZER(struct ovn_route_msg *), .change_handler = ovn_netlink_route_change_handler, .name = "route-ipv6", }, @@ -149,9 +149,9 @@ ovn_netlink_route_change_handler(const void *change_, void *aux) struct route_data *rd = &change->route.rd; if (rd->rtm_protocol != RTPROT_OVN) { - /* We just cannot copy the whole route_data because it has reference - * to self for the nexthop list. */ - vector_push(¬ifier->msgs, &rd->rta_table_id); + struct ovn_route_msg *msg = + ovn_route_msg_from_route_data(change->route.nlmsg_type, rd); + vector_push(¬ifier->msgs, &msg); } route_data_destroy(rd); @@ -288,7 +288,13 @@ ovn_netlink_notifier_flush(enum ovn_netlink_notifier_type type) break; } case OVN_NL_NOTIFIER_ROUTE_V4: - case OVN_NL_NOTIFIER_ROUTE_V6: + case OVN_NL_NOTIFIER_ROUTE_V6: { + struct ovn_route_msg *msg; + VECTOR_FOR_EACH (¬ifier->msgs, msg) { + free(msg); + } + break; + } case OVN_NL_NOTIFIER_NEIGHBOR: case OVN_NL_NOTIFIER_MAX: break; diff --git a/controller/route-exchange-netlink.c b/controller/route-exchange-netlink.c index 501a78e69164..56c998d3b3b6 100644 --- a/controller/route-exchange-netlink.c +++ b/controller/route-exchange-netlink.c @@ -200,6 +200,72 @@ re_nl_delete_route(uint32_t table_id, const struct advertise_route_entry *re) return modify_route(RTM_DELROUTE, 0, table_id, re); } +static size_t +ovn_route_msg_size(const struct ovn_route_msg *msg) +{ + return sizeof *msg + msg->n_nexthops * sizeof msg->nexthops[0]; +} + +/* Returns a self contained copy of the route 'rd' reported by a message of + * type 'nlmsg_type'. The caller takes ownership of it. */ +struct ovn_route_msg * +ovn_route_msg_from_route_data(uint16_t nlmsg_type, + const struct route_data *rd) +{ + size_t n_nexthops = ovs_list_size(&rd->nexthops); + struct ovn_route_msg *msg = + xzalloc(sizeof *msg + n_nexthops * sizeof msg->nexthops[0]); + + msg->nlmsg_type = nlmsg_type; + msg->table_id = rd->rta_table_id; + msg->prefix = rd->rta_dst; + msg->plen = rd->rtm_dst_len; + msg->protocol = rd->rtm_protocol; + msg->priority = rd->rta_priority; + msg->nhid = rd->rta_nhid; + msg->n_nexthops = n_nexthops; + + struct ovn_route_nexthop *nh = msg->nexthops; + const struct route_data_nexthop *rdnh; + LIST_FOR_EACH (rdnh, nexthop_node, &rd->nexthops) { + nh->addr = rdnh->addr; + memcpy(nh->ifname, rdnh->ifname, IFNAMSIZ); + nh++; + } + + return msg; +} + +struct ovn_route_msg * +ovn_route_msg_clone(const struct ovn_route_msg *msg) +{ + return xmemdup(msg, ovn_route_msg_size(msg)); +} + +void +ovn_route_msg_format(struct ds *ds, const struct ovn_route_msg *msg) +{ + ds_put_format(ds, "table_id=%"PRIu32" dst=", msg->table_id); + ipv6_format_mapped(&msg->prefix, ds); + ds_put_format(ds, " plen=%u proto=%u priority=%"PRIu32, + msg->plen, msg->protocol, msg->priority); + + if (msg->nhid) { + ds_put_format(ds, " nhid=%"PRIu32, msg->nhid); + return; + } + + for (size_t i = 0; i < msg->n_nexthops; i++) { + const struct ovn_route_nexthop *nh = &msg->nexthops[i]; + + ds_put_cstr(ds, " nexthop="); + ipv6_format_mapped(&nh->addr, ds); + if (nh->ifname[0]) { + ds_put_format(ds, ",dev=%s", nh->ifname); + } + } +} + /* Appends a learned route for the prefix in 'rd' reachable through the leaf * nexthop object 'nhe' to 'learned_routes'. */ static void diff --git a/controller/route-exchange-netlink.h b/controller/route-exchange-netlink.h index be0496d5e0ab..26aa8de9328e 100644 --- a/controller/route-exchange-netlink.h +++ b/controller/route-exchange-netlink.h @@ -36,9 +36,47 @@ struct in6_addr; struct hmap; +struct route_data; struct vector; struct advertise_route_entry; +/* One of the next hops of a route as reported by the kernel. */ +struct ovn_route_nexthop { + struct in6_addr addr; + /* Adding 1 to this to be sure we actually have a terminating '\0' */ + char ifname[IFNAMSIZ + 1]; +}; + +/* A digested version of a route message sent down by the kernel to indicate + * that a route has changed. Unlike 'struct route_data', which points into + * itself to describe the next hops, this is self contained, so it stays valid + * after the message it was built from is gone. */ +struct ovn_route_msg { + /* E.g. RTM_NEWROUTE, RTM_DELROUTE. */ + uint16_t nlmsg_type; + /* Routing table the route belongs to. */ + uint32_t table_id; + /* Prefix the route is for. */ + struct in6_addr prefix; + unsigned int plen; + /* Routing protocol that installed the route, e.g. RTPROT_BGP. */ + unsigned char protocol; + /* Metric of the route. The kernel allows several routes for one prefix + * that differ only by this, so it is part of a route's identity. */ + uint32_t priority; + /* Id of the kernel nexthop object the route resolves through, 0 if the + * next hops are described by 'nexthops' instead. */ + uint32_t nhid; + /* Number of next hops described by the route itself. */ + size_t n_nexthops; + struct ovn_route_nexthop nexthops[]; +}; + +struct ovn_route_msg *ovn_route_msg_from_route_data( + uint16_t nlmsg_type, const struct route_data *); +struct ovn_route_msg *ovn_route_msg_clone(const struct ovn_route_msg *); +void ovn_route_msg_format(struct ds *, const struct ovn_route_msg *); + struct re_nl_received_route_node { struct in6_addr prefix; unsigned int plen; diff --git a/tests/system-ovn-netlink.at b/tests/system-ovn-netlink.at index 1c2e2995a7ce..5b6772da38ad 100644 --- a/tests/system-ovn-netlink.at +++ b/tests/system-ovn-netlink.at @@ -702,13 +702,19 @@ check ip link set up lo-test dnl Should notify if an IPv4 route is added to a table monitored by OVN. AT_CHECK_UNQUOTED([ovstest test-ovn-netlink route-table-notify \ "ip route add 10.10.10.0/24 via 20.0.0.1 vrf vrf-$table_id"], [0], [dnl -Notification v4 table_id=$table_id +Notification v4 add route table_id=$table_id dst=10.10.10.0 plen=24 proto=3 priority=0 nexthop=20.0.0.1,dev=lo-test ]) dnl Should notify if an IPv6 route is added to a table monitored by OVN. AT_CHECK_UNQUOTED([ovstest test-ovn-netlink route-table-notify \ "ip -6 route add fd10::/64 via fd20::1 vrf vrf-$table_id"], [0], [dnl -Notification v6 table_id=$table_id +Notification v6 add route table_id=$table_id dst=fd10:: plen=64 proto=3 priority=1024 nexthop=fd20::1,dev=lo-test +]) + +dnl Should report which route was removed, not just that something changed. +AT_CHECK_UNQUOTED([ovstest test-ovn-netlink route-table-notify \ + "ip route del 10.10.10.0/24 via 20.0.0.1 vrf vrf-$table_id"], [0], [dnl +Notification v4 delete route table_id=$table_id dst=10.10.10.0 plen=24 proto=3 priority=0 nexthop=20.0.0.1,dev=lo-test ]) dnl Should NOT notify if a route with proto ovn is added. diff --git a/tests/test-ovn-netlink.c b/tests/test-ovn-netlink.c index aff9ba1d0474..ba779938028f 100644 --- a/tests/test-ovn-netlink.c +++ b/tests/test-ovn-netlink.c @@ -277,18 +277,26 @@ test_route_table_notify(struct ovs_cmdl_context *ctx) ovn_netlink_update_notifier(OVN_NL_NOTIFIER_ROUTE_V6, true); run_command_under_notifier(cmd); - uint32_t table_id; + static const char *families[] = {"v4", "v6"}; + static const enum ovn_netlink_notifier_type types[] = { + OVN_NL_NOTIFIER_ROUTE_V4, OVN_NL_NOTIFIER_ROUTE_V6, + }; + struct ds ds = DS_EMPTY_INITIALIZER; - struct vector *msgs = ovn_netlink_get_msgs(OVN_NL_NOTIFIER_ROUTE_V4); - VECTOR_FOR_EACH (msgs, table_id) { - printf("Notification v4 table_id=%"PRIu32"\n", table_id); - } + for (size_t i = 0; i < ARRAY_SIZE(types); i++) { + struct vector *msgs = ovn_netlink_get_msgs(types[i]); + struct ovn_route_msg *msg; - msgs = ovn_netlink_get_msgs(OVN_NL_NOTIFIER_ROUTE_V6); - VECTOR_FOR_EACH (msgs, table_id) { - printf("Notification v6 table_id=%"PRIu32"\n", table_id); + VECTOR_FOR_EACH (msgs, msg) { + ds_clear(&ds); + ovn_route_msg_format(&ds, msg); + printf("Notification %s %s route %s\n", families[i], + msg->nlmsg_type == RTM_NEWROUTE ? "add" : "delete", + ds_cstr(&ds)); + } } + ds_destroy(&ds); ovn_netlink_notifiers_destroy(); } -- 2.38.1 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
