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
