Any change to a nexthop object that is not an FDB one makes 'route_exchange' recompute, which redumps every watched route table and reconciles all the learned routes against the southbound database. The kernel nexthop table is global though, so nexthop objects belonging to route tables we do not watch, e.g. those a routing daemon maintains for the main table, cause that work just as much as the ones we do use.
Track which nexthop ids the learned routes were resolved through and recompute only when one of them is reported as changed. A route depends on the members of the group it refers to as well, since replacing a member moves the route without the group itself being announced. Ids that cannot be resolved are recorded too, so that a route starts being learned once the object it names shows up. The whole table is redumped when it starts being tracked, and there is no way to tell what changed in it then, so keep recomputing in that case. This makes 'is_fdb' unused, the id comparison covers it: an FDB nexthop is only ever named by a bridge FDB entry, never by a route. Assisted-by: Claude Opus 5, Cursor Signed-off-by: Han Zhou <[email protected]> --- controller/nexthop-exchange.c | 5 +-- controller/nexthop-exchange.h | 52 ++++++++++++++++++++++++-- controller/ovn-controller.c | 57 ++++++++++++++++++++--------- controller/route-exchange-netlink.c | 26 +++++++++++-- controller/route-exchange-netlink.h | 6 ++- controller/route-exchange.c | 3 +- controller/route-exchange.h | 5 +++ tests/system-ovn-netlink.at | 8 ++++ tests/system-ovn.at | 11 +++++- tests/test-ovn-netlink.c | 40 +++++++++++++++++++- 10 files changed, 179 insertions(+), 34 deletions(-) diff --git a/controller/nexthop-exchange.c b/controller/nexthop-exchange.c index ff2766e7bf40..c464b6913cb3 100644 --- a/controller/nexthop-exchange.c +++ b/controller/nexthop-exchange.c @@ -56,8 +56,7 @@ nexthop_grp_weight(const struct nexthop_grp *entry) /* Populates 'nexthops' with all nexthop entries (struct nexthop_entry) that * exist in the kernel nexthop table. Both the FDB nexthops used by EVPN and - * the nexthops referenced by routes through a nexthop id are included, use - * 'is_fdb' to tell them apart. */ + * the nexthops referenced by routes through a nexthop id are included. */ void nexthops_sync(struct hmap *nexthops) { @@ -210,7 +209,6 @@ nh_table_parse__(struct ofpbuf *buf, size_t ofs, const struct nlmsghdr *nlmsg, static const struct nl_policy policy[] = { [NHA_ID] = { .type = NL_A_U32 }, - [NHA_FDB] = { .type = NL_A_FLAG, .optional = true }, [NHA_BLACKHOLE] = { .type = NL_A_FLAG, .optional = true }, [NHA_OIF] = { .type = NL_A_U32, .optional = true }, [NHA_GROUP] = { .type = NL_A_UNSPEC, .optional = true, @@ -264,7 +262,6 @@ nh_table_parse__(struct ofpbuf *buf, size_t ofs, const struct nlmsghdr *nlmsg, .id = nl_attr_get_u32(attrs[NHA_ID]), .addr = addr, .is_blackhole = nl_attr_get_flag(attrs[NHA_BLACKHOLE]), - .is_fdb = nl_attr_get_flag(attrs[NHA_FDB]), .n_grps = n_grps, }; diff --git a/controller/nexthop-exchange.h b/controller/nexthop-exchange.h index ff6d39b0d729..aff4f205dc94 100644 --- a/controller/nexthop-exchange.h +++ b/controller/nexthop-exchange.h @@ -20,8 +20,11 @@ #include <netinet/in.h> #include <stdbool.h> #include <stdint.h> +#include <stdlib.h> +#include "hash.h" #include "openvswitch/hmap.h" +#include "util.h" struct ds; struct ofpbuf; @@ -47,9 +50,6 @@ struct nexthop_entry { char ifname[IFNAMSIZ + 1]; /* True if the nexthop discards the traffic sent to it. */ bool is_blackhole; - /* True if the nexthop belongs to a bridge FDB, i.e. it is used by EVPN - * rather than by the routing table. */ - bool is_fdb; /* Number of group entries, "0" in case of gateway entry. */ size_t n_grps; /* Array of group entries. */ @@ -65,6 +65,52 @@ struct nh_table_msg { struct nexthop_entry *nhe; }; +/* Member of a set of kernel nexthop object ids. */ +struct nexthop_id_node { + struct hmap_node hmap_node; + uint32_t id; +}; + +/* Adds 'id' to the set 'ids' if it is not there yet. */ +static inline void +nexthop_ids_add(struct hmap *ids, uint32_t id) +{ + uint32_t hash = hash_int(id, 0); + + struct nexthop_id_node *node; + HMAP_FOR_EACH_WITH_HASH (node, hmap_node, hash, ids) { + if (node->id == id) { + return; + } + } + + node = xmalloc(sizeof *node); + node->id = id; + hmap_insert(ids, &node->hmap_node, hash); +} + +static inline bool +nexthop_ids_contains(const struct hmap *ids, uint32_t id) +{ + const struct nexthop_id_node *node; + HMAP_FOR_EACH_WITH_HASH (node, hmap_node, hash_int(id, 0), ids) { + if (node->id == id) { + return true; + } + } + + return false; +} + +static inline void +nexthop_ids_clear(struct hmap *ids) +{ + struct nexthop_id_node *node; + HMAP_FOR_EACH_POP (node, hmap_node, ids) { + free(node); + } +} + void nexthops_sync(struct hmap *nexthops); void nexthop_entry_format(struct ds *ds, const struct nexthop_entry *nhe); struct nexthop_entry *nexthop_entry_find(const struct hmap *nexthops, diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index 7df828be454e..e221f60af118 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -5733,9 +5733,11 @@ struct ed_type_nexthop_exchange { bool users[NEXTHOP_EXCHANGE_USER_MAX]; bool enabled; bool recompute; - /* True if the last run changed any nexthop that is not an EVPN FDB one, - * i.e. any nexthop a route may be referencing. */ - bool routing_changed; + /* Ids (uint32_t) of the nexthops the last run changed. */ + struct vector changed_ids; + /* True if the last run rebuilt the whole table, in which case + * 'changed_ids' is not usable as any nexthop may have changed. */ + bool resynced; }; static void nexthop_exchange_update(struct ed_type_nexthop_exchange *, @@ -5747,6 +5749,9 @@ struct ed_type_route_exchange { /* Set to true when SB is readonly and we have routes that need * to be inserted into SB. */ bool sb_changes_pending; + /* Ids (struct nexthop_id_node) of the kernel nexthop objects the routes + * learned during the last run depend on. */ + struct hmap referenced_nhids; }; static enum engine_node_state @@ -5790,6 +5795,8 @@ en_route_exchange_run(struct engine_node *node, void *data) = chassis_lookup_by_name(sbrec_chassis_by_name, chassis_id); ovs_assert(chassis); + nexthop_ids_clear(&re->referenced_nhids); + /* Routes we learn may reference their next hop through a nexthop id, which * we can only resolve while the kernel nexthop table is tracked. */ nexthop_exchange_update(nhe_data, NEXTHOP_EXCHANGE_USER_ROUTE, @@ -5806,6 +5813,7 @@ en_route_exchange_run(struct engine_node *node, void *data) struct route_exchange_ctx_out r_ctx_out = { .sb_changes_pending = false, .route_table_watches = &rt_notify->watches, + .referenced_nhids = &re->referenced_nhids, }; route_exchange_run(&r_ctx_in, &r_ctx_out); @@ -5817,20 +5825,28 @@ en_route_exchange_run(struct engine_node *node, void *data) } static enum engine_input_handler_result -route_exchange_nexthop_handler(struct engine_node *node, - void *data OVS_UNUSED) +route_exchange_nexthop_handler(struct engine_node *node, void *data) { + struct ed_type_route_exchange *re = data; struct ed_type_nexthop_exchange *nhe_data = engine_get_input_data("nexthop_exchange", node); - /* Only the nexthops a route can reference through a nexthop id matter - * here, changes limited to the FDB nexthops used by EVPN cannot affect - * the routes we learn. */ - if (!nhe_data->routing_changed) { - return EN_HANDLED_UNCHANGED; + /* The whole table was rebuilt, we cannot tell what changed in it. */ + if (nhe_data->resynced) { + return EN_UNHANDLED; } - return EN_UNHANDLED; + /* Only the nexthop objects the routes we learned depend on can affect + * them. Anything else, e.g. the FDB nexthops used by EVPN or the ones + * used by routes in tables we do not watch, is not worth a recompute. */ + uint32_t id; + VECTOR_FOR_EACH (&nhe_data->changed_ids, id) { + if (nexthop_ids_contains(&re->referenced_nhids, id)) { + return EN_UNHANDLED; + } + } + + return EN_HANDLED_UNCHANGED; } static enum engine_input_handler_result @@ -5852,12 +5868,16 @@ en_route_exchange_init(struct engine_node *node OVS_UNUSED, struct ed_type_route_exchange *re = xzalloc(sizeof *re); re->sb_idl = arg->sb_idl; + hmap_init(&re->referenced_nhids); return re; } static void -en_route_exchange_cleanup(void *data OVS_UNUSED) +en_route_exchange_cleanup(void *data) { + struct ed_type_route_exchange *re = data; + nexthop_ids_clear(&re->referenced_nhids); + hmap_destroy(&re->referenced_nhids); } /* The route_table_notify node is an input node, but the watches are @@ -6539,6 +6559,7 @@ en_nexthop_exchange_init(struct engine_node *node OVS_UNUSED, struct ed_type_nexthop_exchange *nhe_data = xmalloc(sizeof *nhe_data); *nhe_data = (struct ed_type_nexthop_exchange) { .nexthops = HMAP_INITIALIZER(&nhe_data->nexthops), + .changed_ids = VECTOR_EMPTY_INITIALIZER(uint32_t), .enabled = false, .recompute = true, }; @@ -6552,6 +6573,7 @@ en_nexthop_exchange_cleanup(void *data) struct ed_type_nexthop_exchange *nhe_data = data; nexthops_destroy(&nhe_data->nexthops); hmap_destroy(&nhe_data->nexthops); + vector_destroy(&nhe_data->changed_ids); } static enum engine_node_state @@ -6563,6 +6585,9 @@ en_nexthop_exchange_run(struct engine_node *node OVS_UNUSED, void *data) return EN_UNCHANGED; } + vector_clear(&nhe_data->changed_ids); + nhe_data->resynced = false; + if (nhe_data->recompute) { nexthops_destroy(&nhe_data->nexthops); nexthops_sync(&nhe_data->nexthops); @@ -6571,19 +6596,15 @@ en_nexthop_exchange_run(struct engine_node *node OVS_UNUSED, void *data) ovn_netlink_notifier_flush(OVN_NL_NOTIFIER_NEXTHOP); nhe_data->recompute = false; - nhe_data->routing_changed = true; + nhe_data->resynced = true; return EN_UPDATED; } struct vector *msgs = ovn_netlink_get_msgs(OVN_NL_NOTIFIER_NEXTHOP); - nhe_data->routing_changed = false; const struct nh_table_msg *msg; VECTOR_FOR_EACH_PTR (msgs, msg) { - if (!msg->nhe->is_fdb) { - nhe_data->routing_changed = true; - break; - } + vector_push(&nhe_data->changed_ids, &msg->nhe->id); } bool updated = nexthops_handle_changes(&nhe_data->nexthops, msgs); diff --git a/controller/route-exchange-netlink.c b/controller/route-exchange-netlink.c index afb94157d139..501a78e69164 100644 --- a/controller/route-exchange-netlink.c +++ b/controller/route-exchange-netlink.c @@ -226,14 +226,21 @@ learn_route_via_nexthop(const struct nexthop_entry *nhe, /* Resolves the kernel nexthop object identified by 'id' against 'nexthops' * and appends a learned route for the prefix in 'rd' to 'learned_routes' for * each usable next hop. A nexthop group yields one learned route per - * member. */ + * member. + * + * Every nexthop id the route depends on is added to 'referenced_nhids', even + * the ones that cannot be resolved right now, so that the caller knows which + * changes to the kernel nexthop table may affect this route. */ static void learn_routes_via_nexthop_id(const struct hmap *nexthops, uint32_t id, const struct route_data *rd, - struct vector *learned_routes) + struct vector *learned_routes, + struct hmap *referenced_nhids) { static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20); + nexthop_ids_add(referenced_nhids, id); + const struct nexthop_entry *nhe = nexthop_entry_find(nexthops, id); if (!nhe) { VLOG_DBG_RL(&rl, "could not resolve nexthop id %"PRIu32, id); @@ -248,6 +255,10 @@ learn_routes_via_nexthop_id(const struct hmap *nexthops, uint32_t id, for (size_t i = 0; i < nhe->n_grps; i++) { const struct nexthop_grp_entry *grp = &nhe->grps[i]; + /* Replacing a member does not change the group itself, so the route + * depends on the members just as much as on the group. */ + nexthop_ids_add(referenced_nhids, grp->id); + /* The kernel does not allow a nexthop group to contain other groups, * so a single level of indirection is all we have to follow. */ if (!grp->gateway) { @@ -270,6 +281,10 @@ struct route_msg_handle_data { * that reference their next hop(s) through a nexthop id (RTA_NH_ID). * Must be set whenever 'learned_routes' is. */ const struct hmap *nexthops; + + /* Collects the nexthop ids (struct nexthop_id_node) the learned routes + * depend on. Must be set whenever 'learned_routes' is. */ + struct hmap *referenced_nhids; }; static void @@ -306,7 +321,8 @@ handle_route_msg(const struct route_table_msg *msg, /* The next hop(s) are not encoded in the route itself, they are * described by a separate kernel nexthop object. */ learn_routes_via_nexthop_id(handle_data->nexthops, rd->rta_nhid, - rd, handle_data->learned_routes); + rd, handle_data->learned_routes, + handle_data->referenced_nhids); return; } struct route_data_nexthop *nexthop; @@ -394,7 +410,8 @@ re_nl_encode_nexthop(struct ofpbuf *request, bool dst_is_ipv4, int re_nl_sync_routes(uint32_t table_id, const struct hmap *routes, const struct hmap *nexthops, - struct vector *learned_routes) + struct vector *learned_routes, + struct hmap *referenced_nhids) { struct hmapx routes_to_advertise = HMAPX_INITIALIZER(&routes_to_advertise); struct vector stale_routes = @@ -414,6 +431,7 @@ re_nl_sync_routes(uint32_t table_id, const struct hmap *routes, .learned_routes = learned_routes, .stale_routes = &stale_routes, .nexthops = nexthops, + .referenced_nhids = referenced_nhids, }; route_table_dump_one_table(table_id, AF_INET, handle_route_msg, &data); route_table_dump_one_table(table_id, AF_INET6, handle_route_msg, &data); diff --git a/controller/route-exchange-netlink.h b/controller/route-exchange-netlink.h index cff4a6df83ac..be0496d5e0ab 100644 --- a/controller/route-exchange-netlink.h +++ b/controller/route-exchange-netlink.h @@ -60,10 +60,12 @@ void re_route_format(struct ds *, uint32_t table_id, /* Syncs the routes OVN advertises in 'table_id' and collects the routes * learned from it into 'learned_routes'. 'nexthops' contains the kernel * nexthop objects (struct nexthop_entry) used to resolve learned routes that - * reference their next hop through a nexthop id. */ + * reference their next hop through a nexthop id, the ids used in the process + * are collected into 'referenced_nhids' (struct nexthop_id_node). */ int re_nl_sync_routes(uint32_t table_id, const struct hmap *routes, const struct hmap *nexthops, - struct vector *learned_routes); + struct vector *learned_routes, + struct hmap *referenced_nhids); int re_nl_cleanup_routes(uint32_t table_id); diff --git a/controller/route-exchange.c b/controller/route-exchange.c index e2bf51914ba7..3ce2b990ad05 100644 --- a/controller/route-exchange.c +++ b/controller/route-exchange.c @@ -396,7 +396,8 @@ route_exchange_run(const struct route_exchange_ctx_in *r_ctx_in, struct vector received_routes = VECTOR_EMPTY_INITIALIZER(struct re_nl_received_route_node); error = re_nl_sync_routes(arte->table_id, arte->routes, - r_ctx_in->nexthops, &received_routes); + r_ctx_in->nexthops, &received_routes, + r_ctx_out->referenced_nhids); SET_ROUTE_EXCHANGE_NL_STATUS(error); struct ovsdb_idl_index *sbrec_learned_route_by_datapath = diff --git a/controller/route-exchange.h b/controller/route-exchange.h index 64d3e3f08ad3..5565e4c635a2 100644 --- a/controller/route-exchange.h +++ b/controller/route-exchange.h @@ -38,6 +38,11 @@ struct route_exchange_ctx_in { struct route_exchange_ctx_out { struct vector *route_table_watches; bool sb_changes_pending; + + /* Collects the ids (struct nexthop_id_node) of the kernel nexthop objects + * the learned routes depend on. A change to any other nexthop object + * cannot affect them. */ + struct hmap *referenced_nhids; }; void route_exchange_run(const struct route_exchange_ctx_in *, diff --git a/tests/system-ovn-netlink.at b/tests/system-ovn-netlink.at index 1d0f3dab99c6..1c2e2995a7ce 100644 --- a/tests/system-ovn-netlink.at +++ b/tests/system-ovn-netlink.at @@ -637,6 +637,14 @@ Route table_id=$table_id dst=10.10.30.0 plen=24 nexthop=20.0.0.2 Route table_id=$table_id dst=fd20:100:: plen=64 nexthop=fd20::1 ]) +dnl The learned routes depend on the nexthop objects they name and, for a +dnl group, on its members as well: replacing a member moves the routes without +dnl the group itself being reported as changed. +AT_CHECK_UNQUOTED([ovstest test-ovn-netlink route-sync-nhids $table_id | sort], + [0], [$(printf 'Referenced nexthop id=%s\n' \ + $nh_v4 $nh_v4_b $nh_v6 $nh_grp | sort) +]) + dnl Replacing a nexthop object changes where the routes referencing it point, dnl without the kernel touching the routes themselves. check ip nexthop replace id $nh_v4 via 20.0.0.9 dev lo-test diff --git a/tests/system-ovn.at b/tests/system-ovn.at index 27ffddb3a4ed..18c06031a1a9 100644 --- a/tests/system-ovn.at +++ b/tests/system-ovn.at @@ -20998,7 +20998,16 @@ check ovn-nbctl --wait=hv sync OVS_WAIT_UNTIL([settled=$(route_recomputes); sleep 2; test "$settled" = "$(route_recomputes)"]) -# Replacing the object the route names moves the route, even though the +# No route names this object, so there is nothing to relearn for it. +re_recompute=$(route_recomputes) +nh_unused=$(nexthop_alloc) +check ip nexthop add id $nh_unused via 20.0.0.26 dev local-bgp-port +on_exit "ip nexthop del id $nh_unused" +check ovn-nbctl --wait=hv sync +AT_CHECK_UNQUOTED([route_recomputes], [0], [$re_recompute +]) + +# Replacing the object the route does name moves the route, even though the # kernel reports nothing about the route itself. Relearning has to happen for # this change, waiting for the route to move would also accept it happening # later on for an unrelated reason. diff --git a/tests/test-ovn-netlink.c b/tests/test-ovn-netlink.c index 072d7ee64085..aff9ba1d0474 100644 --- a/tests/test-ovn-netlink.c +++ b/tests/test-ovn-netlink.c @@ -202,6 +202,7 @@ test_route_sync(struct ovs_cmdl_context *ctx) } struct hmap routes_to_advertise = HMAP_INITIALIZER(&routes_to_advertise); + struct hmap referenced_nhids = HMAP_INITIALIZER(&referenced_nhids); struct hmap nexthops = HMAP_INITIALIZER(&nexthops); struct vector received_routes = VECTOR_EMPTY_INITIALIZER(struct re_nl_received_route_node); @@ -237,7 +238,7 @@ test_route_sync(struct ovs_cmdl_context *ctx) nexthops_sync(&nexthops); ovs_assert(re_nl_sync_routes(table_id, &routes_to_advertise, &nexthops, - &received_routes) == 0); + &received_routes, &referenced_nhids) == 0); struct ds msg = DS_EMPTY_INITIALIZER; @@ -254,6 +255,8 @@ done: free(e); } hmap_destroy(&routes_to_advertise); + nexthop_ids_clear(&referenced_nhids); + hmap_destroy(&referenced_nhids); nexthops_destroy(&nexthops); hmap_destroy(&nexthops); vector_destroy(&received_routes); @@ -341,6 +344,40 @@ test_nexthop_table_notify(struct ovs_cmdl_context *ctx) ovn_netlink_notifiers_destroy(); } +/* Reports the kernel nexthop objects that the routes learned from 'table_id' + * depend on. */ +static void +test_route_sync_nhids(struct ovs_cmdl_context *ctx) +{ + unsigned int table_id; + + if (!test_read_uint_value(ctx, 1, "table id", &table_id)) { + return; + } + + struct hmap routes_to_advertise = HMAP_INITIALIZER(&routes_to_advertise); + struct hmap referenced_nhids = HMAP_INITIALIZER(&referenced_nhids); + struct hmap nexthops = HMAP_INITIALIZER(&nexthops); + struct vector received_routes = + VECTOR_EMPTY_INITIALIZER(struct re_nl_received_route_node); + + nexthops_sync(&nexthops); + ovs_assert(re_nl_sync_routes(table_id, &routes_to_advertise, &nexthops, + &received_routes, &referenced_nhids) == 0); + + const struct nexthop_id_node *node; + HMAP_FOR_EACH (node, hmap_node, &referenced_nhids) { + printf("Referenced nexthop id=%"PRIu32"\n", node->id); + } + + nexthop_ids_clear(&referenced_nhids); + hmap_destroy(&referenced_nhids); + nexthops_destroy(&nexthops); + hmap_destroy(&nexthops); + hmap_destroy(&routes_to_advertise); + vector_destroy(&received_routes); +} + /* Dumps the nexthop table after applying the changes caused by running * 'shell_command' to it. Unlike "nexthop-sync", which builds the table from * scratch, this goes through the incremental update path. */ @@ -391,6 +428,7 @@ test_ovn_netlink(int argc, char *argv[]) test_neighbor_table_notify, OVS_RO}, {"host-if-monitor", NULL, 2, 3, test_host_if_monitor, OVS_RO}, {"route-sync", NULL, 1, INT_MAX, test_route_sync, OVS_RO}, + {"route-sync-nhids", NULL, 1, 1, test_route_sync_nhids, OVS_RO}, {"route-table-notify", NULL, 1, 1, test_route_table_notify, OVS_RO}, {"nexthop-sync", NULL, 0, 0, test_nexthop_sync, OVS_RO}, -- 2.38.1 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
