On Thu, Sep 10, 2026 at 9:52 AM Han Zhou <[email protected]> wrote: > Learning a route was one pass over the netlink dump of a routing table: the > routes OVN cares about were turned into Learned_Route rows as they were > read. Anything that made the result change therefore meant reading the > whole table again. > > Keep the routes of the table OVN may learn from as they were reported, and > resolve them into Learned_Route rows in a second step. Reading the table > and deciding what it means to OVN are now separate, so the next patch can > apply a notification to what was read and redo just the second step. > > Along the way, give the two rules the learn path is built on one home each: > whether a route is one OVN learns, which was spread over the dump handler, > and which datapaths distribute routes into a table, which was inlined in > route_exchange_run(). > > No functional change intended. > > Assisted-by: Claude Opus 5, Cursor > Signed-off-by: Han Zhou <[email protected]> > --- > controller/route-exchange-netlink.c | 164 +++++++++++++++++++++++----- > controller/route-exchange-netlink.h | 22 +++- > controller/route-exchange.c | 146 +++++++++++++++++-------- > tests/test-ovn-netlink.c | 37 +++++-- > 4 files changed, 282 insertions(+), 87 deletions(-) > > diff --git a/controller/route-exchange-netlink.c > b/controller/route-exchange-netlink.c > index 070c079a81ba..d2b8fb01e2a3 100644 > --- a/controller/route-exchange-netlink.c > +++ b/controller/route-exchange-netlink.c > @@ -259,13 +259,133 @@ ovn_route_msg_format(struct ds *ds, const struct > ovn_route_msg *msg) > } > } > > +/* Returns true if 'msg' describes a route OVN may learn: one installed > into > + * its table by a dynamic routing protocol, rather than by OVN itself or > by a > + * user. Protocol values above RTPROT_STATIC are the ones used by the > dynamic > + * routing protocols. */ > +static bool > +route_is_learn_relevant(const struct ovn_route_msg *msg) > +{ > + return msg->protocol != RTPROT_OVN > + && msg->protocol > RTPROT_STATIC > + && !prefix_is_link_local(&msg->prefix, msg->plen); > +} > + > +/* Two routes for one prefix may differ only by their metric, so it is > part of > + * a route's identity. */ > +static uint32_t > +cached_route_hash(const struct ovn_route_msg *msg) > +{ > + uint32_t hash = hash_bytes(&msg->prefix, sizeof msg->prefix, 0); > + > + hash = hash_int(msg->plen, hash); > + return hash_int(msg->priority, hash); > +} > + > +static struct re_nl_cached_route * > +cached_route_find(const struct hmap *routes, const struct ovn_route_msg > *msg) > +{ > + struct re_nl_cached_route *cr; > + HMAP_FOR_EACH_WITH_HASH (cr, node, cached_route_hash(msg), routes) { > + if (cr->msg->plen == msg->plen > + && cr->msg->priority == msg->priority > + && ipv6_addr_equals(&cr->msg->prefix, &msg->prefix)) { > + return cr; > + } > + } > + > + return NULL; > +} > + > +/* Applies the change 'msg' to the routes cached for its table: a route > + * reported as added replaces the one it has the identity of, a route > reported > + * as removed drops it. Changes to routes OVN does not learn from are > ignored. > + * > + * Returns true if 'routes' changed. */ > +bool > +re_nl_cached_routes_apply(struct hmap *routes, const struct ovn_route_msg > *msg) > +{ > + if (!route_is_learn_relevant(msg)) { > + return false; > + } > + > + struct re_nl_cached_route *cr = cached_route_find(routes, msg); > + > + if (msg->nlmsg_type == RTM_DELROUTE) { > + if (!cr) { > + return false; > + } > + > + hmap_remove(routes, &cr->node); > + free(cr->msg); > + free(cr); > + return true; > + } > + > + if (cr) { > + size_t size = ovn_route_msg_size(msg); > + if (size == ovn_route_msg_size(cr->msg) > + && !memcmp(cr->msg, msg, size)) { > + return false; > + } > + > + free(cr->msg); > + cr->msg = ovn_route_msg_clone(msg); > + return true; > + } > + > + cr = xmalloc(sizeof *cr); > + cr->msg = ovn_route_msg_clone(msg); > + hmap_insert(routes, &cr->node, cached_route_hash(msg)); > + > + return true; > +} > + > +void > +re_nl_cached_routes_clear(struct hmap *routes) > +{ > + struct re_nl_cached_route *cr; > + HMAP_FOR_EACH_POP (cr, node, routes) { > + free(cr->msg); > + free(cr); > + } > +} > + > +void > +re_nl_resolve_route(const struct ovn_route_msg *msg, > + struct vector *learned_routes) > +{ > + for (size_t i = 0; i < msg->n_nexthops; i++) { > + const struct ovn_route_nexthop *nh = &msg->nexthops[i]; > + > + if (ipv6_is_zero(&nh->addr)) { > + /* This is most likely an address on the local link. As we > just > + * want to learn remote routes we do not need it. */ > + continue; > + } > + > + struct re_nl_received_route_node rr; > + rr = (struct re_nl_received_route_node) { > + .prefix = msg->prefix, > + .plen = msg->plen, > + .nexthop = nh->addr, > + }; > + ovs_strlcpy(rr.ifname, nh->ifname, sizeof rr.ifname); > + > + vector_push(learned_routes, &rr); > + } > +} > + > struct route_msg_handle_data { > struct hmapx *routes_to_advertise; > - struct vector *learned_routes; > struct vector *stale_routes; > /* Vector of "const struct hmap *", each holding advertise_route_entry > * nodes for a datapath sharing this routing table. */ > const struct vector *route_tables; > + > + /* Routes of the table OVN may learn from (struct re_nl_cached_route), > + * rebuilt from the dump. NULL if the caller does not learn routes. > */ > + struct hmap *learned_routes; > }; > > static void > @@ -284,38 +404,16 @@ handle_route_msg(const struct route_table_msg *msg, > return; > } > > - /* This route is not from us, learn it only if it's > RTPROT_STATIC, > - * those protocol values are used by dynamic routing protocols. > - * This should prevent us from learning static routes installed > - * by users in the VRF. */ > + /* This route is not from us, so it is one we may learn. */ > if (rd->rtm_protocol != RTPROT_OVN) { > - if (rd->rtm_protocol <= RTPROT_STATIC) { > - return; > - } > if (!handle_data->learned_routes) { > return; > } > - if (prefix_is_link_local(&rd->rta_dst, rd->rtm_dst_len)) { > - return; > - } > - struct route_data_nexthop *nexthop; > - LIST_FOR_EACH (nexthop, nexthop_node, &rd->nexthops) { > - if (ipv6_is_zero(&nexthop->addr)) { > - /* This is most likely an address on the local link. > - * As we just want to learn remote routes we do not need > it.*/ > - continue; > - } > - struct re_nl_received_route_node rr; > - rr = (struct re_nl_received_route_node) { > - .prefix = rd->rta_dst, > - .plen = rd->rtm_dst_len, > - .nexthop = nexthop->addr, > - }; > - memcpy(rr.ifname, nexthop->ifname, IFNAMSIZ); > - rr.ifname[IFNAMSIZ] = '\0'; > - > - vector_push(handle_data->learned_routes, &rr); > - } > + > + struct ovn_route_msg *route_msg = > + ovn_route_msg_from_route_data(RTM_NEWROUTE, rd); > + re_nl_cached_routes_apply(handle_data->learned_routes, route_msg); > + free(route_msg); > return; > } > > @@ -385,7 +483,7 @@ re_nl_encode_nexthop(struct ofpbuf *request, bool > dst_is_ipv4, > > int > re_nl_sync_routes(uint32_t table_id, const struct vector *route_tables, > - struct vector *learned_routes) > + struct hmap *learned_routes) > { > struct hmapx routes_to_advertise = > HMAPX_INITIALIZER(&routes_to_advertise); > struct vector stale_routes = > @@ -417,6 +515,12 @@ re_nl_sync_routes(uint32_t table_id, const struct > vector *route_tables, > n_prev++; > } > > + if (learned_routes) { > + /* The dump below tells us about every route of the table, so > whatever > + * we knew about it is replaced. */ > + re_nl_cached_routes_clear(learned_routes); > + } > + > /* Remove routes from the system that are not in any of the route > tables > * and remove entries from routes_to_advertise that match routes > already > * installed in the system. */ > diff --git a/controller/route-exchange-netlink.h > b/controller/route-exchange-netlink.h > index f046dbbe12df..736a05c0fc97 100644 > --- a/controller/route-exchange-netlink.h > +++ b/controller/route-exchange-netlink.h > @@ -77,6 +77,18 @@ struct ovn_route_msg *ovn_route_msg_from_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 *); > > +/* A route of a kernel routing table OVN may learn from, kept as the > kernel > + * reported it so that it can be resolved again without reading the table > + * anew. */ > +struct re_nl_cached_route { > + struct hmap_node node; > + struct ovn_route_msg *msg; > +}; > + > +bool re_nl_cached_routes_apply(struct hmap *routes, > + const struct ovn_route_msg *); > +void re_nl_cached_routes_clear(struct hmap *routes); > + > struct re_nl_received_route_node { > struct in6_addr prefix; > unsigned int plen; > @@ -85,6 +97,11 @@ struct re_nl_received_route_node { > char ifname[IFNAMSIZ + 1]; > }; > > +/* Turns the route 'msg' into the routes OVN learns from it, appending > them to > + * 'learned_routes'. */ > +void re_nl_resolve_route(const struct ovn_route_msg *, > + struct vector *learned_routes); > + > int re_nl_create_vrf(const char *ifname, uint32_t table_id); > int re_nl_delete_vrf(const char *ifname); > > @@ -95,8 +112,11 @@ void re_route_format(struct ds *, uint32_t table_id, > const struct in6_addr *dst, unsigned int plen, > const struct in6_addr *nexthop, int err); > > +/* Syncs the routes OVN advertises in 'table_id' with the kernel and, > unless > + * 'learned_routes' is NULL, rebuilds it from the routes of the table OVN > may > + * learn from (struct re_nl_cached_route). */ > int re_nl_sync_routes(uint32_t table_id, const struct vector > *route_tables, > - struct vector *learned_routes); > + struct hmap *learned_routes); > > int re_nl_cleanup_routes(uint32_t table_id); > > diff --git a/controller/route-exchange.c b/controller/route-exchange.c > index d816a66863bd..492697503381 100644 > --- a/controller/route-exchange.c > +++ b/controller/route-exchange.c > @@ -309,6 +309,94 @@ struct advertised_routes_entry { > uint32_t table_id; > }; > > +/* Records that 'ad' distributes routes into the table 'table_id', > creating the > + * entry for the table in 'advertised_routes' if it is the first datapath > to do > + * so. */ > +static void > +advertised_routes_add(struct hmap *advertised_routes, > + const struct advertise_datapath_entry *ad, > + uint32_t table_id) > +{ > + struct advertised_routes_entry *entry = NULL; > + uint32_t hash = maintained_route_table_hash(table_id); > + HMAP_FOR_EACH_WITH_HASH (entry, node, hash, advertised_routes) { > + if (entry->table_id == table_id) { > + break; > + } > + } > + > + if (entry == NULL) { > + entry = xmalloc(sizeof *entry); > + *entry = (struct advertised_routes_entry) { > + .datapaths = HMAPX_INITIALIZER(&entry->datapaths), > + .table_id = table_id, > + }; > + hmap_insert(advertised_routes, &entry->node, hash); > + } > + > + hmapx_add(&entry->datapaths, CONST_CAST(void *, ad)); > +} > + > +/* Collects the route tables of all datapaths in 'datapaths' ('struct > + * advertise_datapath_entry *') into 'route_tables', so that a routing > table > + * shared by several of them is synced as a single authoritative set. */ > +static void > +advertised_routes_tables(const struct hmapx *datapaths, > + struct vector *route_tables) > +{ > + struct hmapx_node *dp_node; > + HMAPX_FOR_EACH (dp_node, datapaths) { > + const struct advertise_datapath_entry *adpe = dp_node->data; > + const struct hmap *routes = &adpe->routes; > + > + vector_push(route_tables, &routes); > + } > +} > + > +static void > +advertised_routes_destroy(struct hmap *advertised_routes) > +{ > + struct advertised_routes_entry *arte; > + HMAP_FOR_EACH_POP (arte, node, advertised_routes) { > + hmapx_destroy(&arte->datapaths); > + free(arte); > + } > + hmap_destroy(advertised_routes); > +} > + > +/* Turns 'learned_routes', the routes of a kernel routing table OVN may > learn > + * from, into Learned_Route rows of every datapath in 'datapaths' ('struct > + * advertise_datapath_entry *'). */ > +static void > +resolve_and_sync_learned_routes( > + const struct hmap *learned_routes, const struct hmapx *datapaths, > + const struct route_exchange_ctx_in *r_ctx_in, > + struct route_exchange_ctx_out *r_ctx_out) > +{ > + struct vector received_routes = > + VECTOR_EMPTY_INITIALIZER(struct re_nl_received_route_node); > + > + const struct re_nl_cached_route *cr; > + HMAP_FOR_EACH (cr, node, learned_routes) { > + re_nl_resolve_route(cr->msg, &received_routes); > + } > + > + struct hmapx_node *dp_node; > + HMAPX_FOR_EACH (dp_node, datapaths) { > + const struct advertise_datapath_entry *adpe = dp_node->data; > + > + sb_sync_learned_routes(&received_routes, adpe->db, > + &adpe->bound_ports, > + r_ctx_in->ovnsb_idl_txn, > + r_ctx_in->sbrec_port_binding_by_name, > + r_ctx_in->sbrec_learned_route_by_datapath, > + &r_ctx_out->sb_changes_pending, > + r_ctx_in->chassis); > + } > + > + vector_destroy(&received_routes); > +} > + > void > route_exchange_run(const struct route_exchange_ctx_in *r_ctx_in, > struct route_exchange_ctx_out *r_ctx_out) > @@ -353,67 +441,29 @@ route_exchange_run(const struct > route_exchange_ctx_in *r_ctx_in, > sset_find_and_delete(&old_maintained_vrfs, ad->vrf_name); > } > > - struct advertised_routes_entry *entry = NULL; > - uint32_t hash = maintained_route_table_hash(table_id); > - HMAP_FOR_EACH_WITH_HASH (entry, node, hash, &advertised_routes) { > - if (entry->table_id == table_id) { > - break; > - } > - } > - > - if (entry == NULL) { > - entry = xmalloc(sizeof *entry); > - *entry = (struct advertised_routes_entry) { > - .datapaths = HMAPX_INITIALIZER(&entry->datapaths), > - .table_id = table_id, > - }; > - hmap_insert(&advertised_routes, &entry->node, hash); > - } > - > - hmapx_add(&entry->datapaths, CONST_CAST(void *, ad)); > + advertised_routes_add(&advertised_routes, ad, table_id); > } > > + struct hmap learned_routes = HMAP_INITIALIZER(&learned_routes); > struct advertised_routes_entry *arte; > - HMAP_FOR_EACH_POP (arte, node, &advertised_routes) { > + HMAP_FOR_EACH (arte, node, &advertised_routes) { > maintained_route_table_add(arte->table_id); > > - struct hmapx_node *dp_node; > - > - /* Collect the route tables of all datapaths sharing this routing > - * table so they are synced together as a single authoritative > set. */ > struct vector route_tables = > VECTOR_EMPTY_INITIALIZER(const struct hmap *); > - HMAPX_FOR_EACH (dp_node, &arte->datapaths) { > - const struct advertise_datapath_entry *adpe = dp_node->data; > - const struct hmap *routes = &adpe->routes; > - vector_push(&route_tables, &routes); > - } > + advertised_routes_tables(&arte->datapaths, &route_tables); > > - struct vector received_routes = > - VECTOR_EMPTY_INITIALIZER(struct re_nl_received_route_node); > error = re_nl_sync_routes(arte->table_id, &route_tables, > - &received_routes); > + &learned_routes); > SET_ROUTE_EXCHANGE_NL_STATUS(error); > vector_destroy(&route_tables); > > - struct ovsdb_idl_index *sbrec_learned_route_by_datapath = > - r_ctx_in->sbrec_learned_route_by_datapath; > - HMAPX_FOR_EACH (dp_node, &arte->datapaths) { > - const struct advertise_datapath_entry *adpe = dp_node->data; > - sb_sync_learned_routes(&received_routes, adpe->db, > - &adpe->bound_ports, > - r_ctx_in->ovnsb_idl_txn, > - r_ctx_in->sbrec_port_binding_by_name, > - sbrec_learned_route_by_datapath, > - &r_ctx_out->sb_changes_pending, > - r_ctx_in->chassis); > - } > + resolve_and_sync_learned_routes(&learned_routes, &arte->datapaths, > + r_ctx_in, r_ctx_out); > vector_push(r_ctx_out->route_table_watches, &arte->table_id); > - vector_destroy(&received_routes); > - > - hmapx_destroy(&arte->datapaths); > - free(arte); > } > + re_nl_cached_routes_clear(&learned_routes); > + hmap_destroy(&learned_routes); > > /* Remove routes in tables previously maintained by us. */ > struct maintained_route_table_entry *mrt; > @@ -444,7 +494,7 @@ route_exchange_run(const struct route_exchange_ctx_in > *r_ctx_in, > sset_delete(&old_maintained_vrfs, SSET_NODE_FROM_NAME(vrf_name)); > } > sset_destroy(&old_maintained_vrfs); > - hmap_destroy(&advertised_routes); > + advertised_routes_destroy(&advertised_routes); > } > > void > diff --git a/tests/test-ovn-netlink.c b/tests/test-ovn-netlink.c > index 94dff880ba6a..3c2c14dc5066 100644 > --- a/tests/test-ovn-netlink.c > +++ b/tests/test-ovn-netlink.c > @@ -190,6 +190,33 @@ test_host_if_monitor(struct ovs_cmdl_context *ctx) > sset_destroy(&if_names); > } > > +/* Syncs the routes OVN advertises in 'table_id' and resolves the routes > it > + * learns from it into 'received_routes', as route_exchange does. */ > +static int > +sync_and_resolve_routes(uint32_t table_id, > + const struct hmap *routes_to_advertise, > + struct vector *received_routes) > +{ > + struct hmap learned_routes = HMAP_INITIALIZER(&learned_routes); > + struct vector route_tables = > + VECTOR_EMPTY_INITIALIZER(const struct hmap *); > + > + vector_push(&route_tables, &routes_to_advertise); > + > + int err = re_nl_sync_routes(table_id, &route_tables, &learned_routes); > + vector_destroy(&route_tables); > + > + const struct re_nl_cached_route *cr; > + HMAP_FOR_EACH (cr, node, &learned_routes) { > + re_nl_resolve_route(cr->msg, received_routes); > + } > + > + re_nl_cached_routes_clear(&learned_routes); > + hmap_destroy(&learned_routes); > + > + return err; > +} > + > static void > test_route_sync(struct ovs_cmdl_context *ctx) > { > @@ -231,14 +258,8 @@ test_route_sync(struct ovs_cmdl_context *ctx) > advertise_route_hash(&ar->addr, &ar->nexthop, > ar->plen)); > } > > - struct vector route_tables = > - VECTOR_EMPTY_INITIALIZER(const struct hmap *); > - const struct hmap *routes = &routes_to_advertise; > - vector_push(&route_tables, &routes); > - > - ovs_assert(re_nl_sync_routes(table_id, &route_tables, > - &received_routes) == 0); > - vector_destroy(&route_tables); > + ovs_assert(sync_and_resolve_routes(table_id, &routes_to_advertise, > + &received_routes) == 0); > > struct ds msg = DS_EMPTY_INITIALIZER; > > -- > 2.38.1 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Looks good to me, thanks.
Acked-by: Ales Musil <[email protected]> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
