On Thu, Sep 10, 2026 at 9:51 AM Han Zhou <[email protected]> wrote:
> 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 a later patch uses to learn routes incrementally.
>
> Assisted-by: Claude Opus 5, Cursor
> Signed-off-by: Han Zhou <[email protected]>
> ---
>
Hi Han,
thank you for the patch. I have one small comment below.
controller/ovn-controller.c | 62 +++++++++++++++++------------
> controller/ovn-netlink-notifier.c | 18 ++++++---
> controller/route-exchange-netlink.c | 60 ++++++++++++++++++++++++++++
> controller/route-exchange-netlink.h | 38 ++++++++++++++++++
> tests/system-ovn-netlink.at | 10 ++++-
> tests/test-ovn-netlink.c | 24 +++++++----
> 6 files changed, 171 insertions(+), 41 deletions(-)
>
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index 700158eef53a..244489b18d50 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);
> +}
> +
> struct ed_type_route_exchange {
> /* We need the idl to check if the Learned_Route table exists. */
> struct ovsdb_idl *sb_idl;
> @@ -5816,40 +5830,35 @@ en_route_exchange_cleanup(void *data OVS_UNUSED)
> 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;
> }
> @@ -5863,6 +5872,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;
> }
> @@ -5871,6 +5881,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 2d066858a596..070c079a81ba 100644
> --- a/controller/route-exchange-netlink.c
> +++ b/controller/route-exchange-netlink.c
> @@ -199,6 +199,66 @@ 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->n_nexthops = n_nexthops;
>
We should use the designated initializer.
+
> + 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);
> +
> + 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);
> + }
> + }
> +}
> +
> struct route_msg_handle_data {
> struct hmapx *routes_to_advertise;
> struct vector *learned_routes;
> diff --git a/controller/route-exchange-netlink.h
> b/controller/route-exchange-netlink.h
> index bb72cc0e5cd2..f046dbbe12df 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 dedfd14f2d6f..d534814a2ff5 100644
> --- a/tests/system-ovn-netlink.at
> +++ b/tests/system-ovn-netlink.at
> @@ -590,13 +590,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 c6943ee0b05c..94dff880ba6a 100644
> --- a/tests/test-ovn-netlink.c
> +++ b/tests/test-ovn-netlink.c
> @@ -273,18 +273,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
>
>
With that addressed:
Acked-by: Ales Musil <[email protected]>
Regards,
Ales
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
