On 2/18/25 3:02 PM, Felix Huettner via dev wrote: > In order to support incremental processing of route changes we need to > extract the ecmp grouping to before the actual lflow generation. > > Signed-off-by: Felix Huettner <[email protected]> > ---
Hi Felix, This looks OK overall, I only have a few small comments below. > lib/stopwatch-names.h | 1 + > northd/automake.mk | 2 + > northd/en-group-ecmp-route.c | 289 +++++++++++++++++++++++++++++++++ > northd/en-group-ecmp-route.h | 79 +++++++++ > northd/en-learned-route-sync.c | 11 -- > northd/en-lflow.c | 6 +- > northd/inc-proc-northd.c | 8 +- > northd/northd.c | 209 ++---------------------- > northd/northd.h | 2 +- > tests/ovn-northd.at | 19 +++ > 10 files changed, 416 insertions(+), 210 deletions(-) > create mode 100644 northd/en-group-ecmp-route.c > create mode 100644 northd/en-group-ecmp-route.h > > diff --git a/lib/stopwatch-names.h b/lib/stopwatch-names.h > index 13aa5e7bf..f3a931c40 100644 > --- a/lib/stopwatch-names.h > +++ b/lib/stopwatch-names.h > @@ -37,5 +37,6 @@ > #define ADVERTISED_ROUTE_SYNC_RUN_STOPWATCH_NAME "advertised_route_sync" > #define LEARNED_ROUTE_SYNC_RUN_STOPWATCH_NAME "learned_route_sync" > #define DYNAMIC_ROUTES_RUN_STOPWATCH_NAME "dynamic_routes" > +#define GROUP_ECMP_ROUTE_RUN_STOPWATCH_NAME "group_ecmp_route" We're missing a stopwatch_create() for this one. > > #endif > diff --git a/northd/automake.mk b/northd/automake.mk > index c5772e03c..9a7165529 100644 > --- a/northd/automake.mk > +++ b/northd/automake.mk > @@ -44,6 +44,8 @@ northd_ovn_northd_SOURCES = \ > northd/en-advertised-route-sync.h \ > northd/en-learned-route-sync.c \ > northd/en-learned-route-sync.h \ > + northd/en-group-ecmp-route.c \ > + northd/en-group-ecmp-route.h \ > northd/inc-proc-northd.c \ > northd/inc-proc-northd.h \ > northd/ipam.c \ > diff --git a/northd/en-group-ecmp-route.c b/northd/en-group-ecmp-route.c > new file mode 100644 > index 000000000..e8a882004 > --- /dev/null > +++ b/northd/en-group-ecmp-route.c > @@ -0,0 +1,289 @@ > +/* > + * Copyright (c) 2025, STACKIT GmbH & Co. KG > + * > + * Licensed under the Apache License, Version 2.0 (the "License"); > + * you may not use this file except in compliance with the License. > + * You may obtain a copy of the License at: > + * > + * http://www.apache.org/licenses/LICENSE-2.0 > + * > + * Unless required by applicable law or agreed to in writing, software > + * distributed under the License is distributed on an "AS IS" BASIS, > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. > + * See the License for the specific language governing permissions and > + * limitations under the License. > + */ > + > +#include <config.h> > +#include <stdbool.h> > + > +#include "openvswitch/vlog.h" > +#include "stopwatch.h" > +#include "northd.h" > + > +#include "en-group-ecmp-route.h" > +#include "en-learned-route-sync.h" > +#include "lib/stopwatch-names.h" > +#include "openvswitch/hmap.h" > + > +VLOG_DEFINE_THIS_MODULE(en_group_ecmp_route); > + > +static void > +ecmp_groups_destroy(struct hmap *ecmp_groups) > +{ > + struct ecmp_groups_node *eg; > + HMAP_FOR_EACH_SAFE (eg, hmap_node, ecmp_groups) { > + struct ecmp_route_list_node *er; > + LIST_FOR_EACH_SAFE (er, list_node, &eg->route_list) { > + ovs_list_remove(&er->list_node); > + free(er); > + } > + hmap_remove(ecmp_groups, &eg->hmap_node); > + sset_destroy(&eg->selection_fields); > + free(eg); > + } > + hmap_destroy(ecmp_groups); > +} > + > +static void > +unique_routes_destroy(struct hmap *unique_routes) > +{ > + struct unique_routes_node *ur; > + HMAP_FOR_EACH_SAFE (ur, hmap_node, unique_routes) { > + hmap_remove(unique_routes, &ur->hmap_node); > + free(ur); > + } > + hmap_destroy(unique_routes); > +} Let's move the definition of unique_routes_destroy() after the one of unique_routes_add() so they're closer together in the code. We probably need some forward declarations. > + > +static void > +group_ecmp_route_clear(struct group_ecmp_route_data *data) > +{ > + struct group_ecmp_route_node *n; > + HMAP_FOR_EACH_POP (n, hmap_node, &data->routes) { > + unique_routes_destroy(&n->unique_routes); > + ecmp_groups_destroy(&n->ecmp_groups); > + free(n); > + } > +} > + > +static void > +group_ecmp_route_init(struct group_ecmp_route_data *data) > +{ > + hmap_init(&data->routes); > +} > + > +void *en_group_ecmp_route_init(struct engine_node *node OVS_UNUSED, > + struct engine_arg *arg OVS_UNUSED) > +{ > + struct group_ecmp_route_data *data = xmalloc(sizeof *data); > + group_ecmp_route_init(data); > + return data; > +} > + > +void en_group_ecmp_route_cleanup(void *_data) > +{ > + struct group_ecmp_route_data *data = _data; > + group_ecmp_route_clear(data); > + hmap_destroy(&data->routes); > +} > + > +void > +en_group_ecmp_route_clear_tracked_data(void *data OVS_UNUSED) > +{ > +} > + > +struct group_ecmp_route_node * > +group_ecmp_route_lookup(const struct group_ecmp_route_data *data, > + const struct ovn_datapath *od) > +{ > + struct group_ecmp_route_node *n; > + size_t hash = uuid_hash(&od->key); > + HMAP_FOR_EACH_WITH_HASH (n, hmap_node, hash, &data->routes) { > + if (n->od == od) { > + return n; > + } > + } > + return NULL; > +} > + > +static struct group_ecmp_route_node * > +group_ecmp_route_add(struct group_ecmp_route_data *data, > + const struct ovn_datapath *od) > +{ > + struct group_ecmp_route_node *n = group_ecmp_route_lookup(data, od); > + if (n) { > + return n; > + } > + > + size_t hash = uuid_hash(&od->key); > + n = xmalloc(sizeof *n); > + n->od = od; > + hmap_init(&n->ecmp_groups); > + hmap_init(&n->unique_routes); > + hmap_insert(&data->routes, &n->hmap_node, hash); > + return n; > +} > + > +static void > +unique_routes_add(struct group_ecmp_route_node *gn, > + const struct parsed_route *route) > +{ > + struct unique_routes_node *ur = xmalloc(sizeof *ur); > + ur->route = route; > + hmap_insert(&gn->unique_routes, &ur->hmap_node, route->hash); > +} > + > +/* Remove the unique_routes_node from the group, and return the parsed_route > + * pointed by the removed node. */ > +static const struct parsed_route * > +unique_routes_remove(struct group_ecmp_route_node *gn, > + const struct parsed_route *route) > +{ > + struct unique_routes_node *ur; > + HMAP_FOR_EACH_WITH_HASH (ur, hmap_node, route->hash, &gn->unique_routes) > { > + if (ipv6_addr_equals(&route->prefix, &ur->route->prefix) && > + route->plen == ur->route->plen && > + route->is_src_route == ur->route->is_src_route && > + route->source == ur->route->source && > + route->route_table_id == ur->route->route_table_id) { > + hmap_remove(&gn->unique_routes, &ur->hmap_node); > + const struct parsed_route *existed_route = ur->route; > + free(ur); > + return existed_route; > + } > + } > + return NULL; > +} > + > +static void > +ecmp_groups_add_route(struct ecmp_groups_node *group, > + const struct parsed_route *route) > +{ > + if (group->route_count == UINT16_MAX) { > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); > + VLOG_WARN_RL(&rl, "too many routes in a single ecmp group."); > + return; > + } > + > + struct ecmp_route_list_node *er = xmalloc(sizeof *er); > + er->route = route; > + er->id = ++group->route_count; > + > + if (group->route_count == 1) { > + sset_clone(&group->selection_fields, &route->ecmp_selection_fields); > + } else { > + sset_intersect(&group->selection_fields, > + &route->ecmp_selection_fields); > + } > + > + ovs_list_insert(&group->route_list, &er->list_node); > +} > + > +static struct ecmp_groups_node * > +ecmp_groups_add(struct group_ecmp_route_node *gn, > + const struct parsed_route *route) > +{ > + if (hmap_count(&gn->ecmp_groups) == UINT16_MAX) { > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); > + VLOG_WARN_RL(&rl, "too many ecmp groups."); > + return NULL; > + } > + > + struct ecmp_groups_node *eg = xzalloc(sizeof *eg); > + hmap_insert(&gn->ecmp_groups, &eg->hmap_node, route->hash); > + > + eg->id = hmap_count(&gn->ecmp_groups); > + eg->prefix = route->prefix; > + eg->plen = route->plen; > + eg->is_src_route = route->is_src_route; > + eg->source = route->source; > + eg->route_table_id = route->route_table_id; > + sset_init(&eg->selection_fields); > + ovs_list_init(&eg->route_list); > + ecmp_groups_add_route(eg, route); > + > + return eg; > +} > + > +static struct ecmp_groups_node * > +ecmp_groups_find(struct group_ecmp_route_node *gn, > + const struct parsed_route *route) > +{ > + struct ecmp_groups_node *eg; > + HMAP_FOR_EACH_WITH_HASH (eg, hmap_node, route->hash, &gn->ecmp_groups) { > + if (ipv6_addr_equals(&eg->prefix, &route->prefix) && > + eg->plen == route->plen && > + eg->is_src_route == route->is_src_route && > + eg->route_table_id == route->route_table_id && > + eg->source == route->source) { > + return eg; > + } > + } > + return NULL; > +} > + > +static void > +add_route(struct group_ecmp_route_data *data, const struct parsed_route *pr) > +{ > + struct group_ecmp_route_node *gn = group_ecmp_route_add(data, pr->od); > + > + if (pr->source == ROUTE_SOURCE_CONNECTED) { > + unique_routes_add(gn, pr); > + return; > + } > + > + struct ecmp_groups_node *group = ecmp_groups_find(gn, pr); > + if (group) { > + ecmp_groups_add_route(group, pr); > + } else { > + const struct parsed_route *existed_route = > + unique_routes_remove(gn, pr); > + if (existed_route) { > + group = ecmp_groups_add(gn, existed_route); > + if (group) { > + ecmp_groups_add_route(group, pr); > + } > + } else if (pr->ecmp_symmetric_reply) { > + /* Traffic for symmetric reply routes has to be conntracked > + * even if there is only one next-hop, in case another next-hop > + * is added later. */ > + ecmp_groups_add(gn, pr); > + } else { > + unique_routes_add(gn, pr); > + } > + } > +} > + > +static void > +group_ecmp_route(struct group_ecmp_route_data *data, > + const struct routes_data *routes_data, > + const struct learned_route_sync_data *learned_route_data) > +{ > + const struct parsed_route *pr; > + HMAP_FOR_EACH (pr, key_node, &routes_data->parsed_routes) { > + add_route(data, pr); > + } > + > + HMAP_FOR_EACH (pr, key_node, &learned_route_data->parsed_routes) { > + add_route(data, pr); > + } > +} > + > +void en_group_ecmp_route_run(struct engine_node *node, void *_data) > +{ > + struct group_ecmp_route_data *data = _data; > + group_ecmp_route_clear(data); > + > + struct routes_data *routes_data > + = engine_get_input_data("routes", node); > + struct learned_route_sync_data *learned_route_data > + = engine_get_input_data("learned_route_sync", node); > + > + stopwatch_start(GROUP_ECMP_ROUTE_RUN_STOPWATCH_NAME, time_msec()); > + > + group_ecmp_route(data, routes_data, learned_route_data); > + > + stopwatch_stop(GROUP_ECMP_ROUTE_RUN_STOPWATCH_NAME, time_msec()); > + engine_set_node_state(node, EN_UPDATED); > +} > diff --git a/northd/en-group-ecmp-route.h b/northd/en-group-ecmp-route.h > new file mode 100644 > index 000000000..c84bb11ee > --- /dev/null > +++ b/northd/en-group-ecmp-route.h > @@ -0,0 +1,79 @@ > +/* > + * Copyright (c) 2025, STACKIT GmbH & Co. KG > + * > + * Licensed under the Apache License, Version 2.0 (the "License"); > + * you may not use this file except in compliance with the License. > + * You may obtain a copy of the License at: > + * > + * http://www.apache.org/licenses/LICENSE-2.0 > + * > + * Unless required by applicable law or agreed to in writing, software > + * distributed under the License is distributed on an "AS IS" BASIS, > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. > + * See the License for the specific language governing permissions and > + * limitations under the License. > + */ > +#ifndef EN_GROUP_ECMP_ROUTE_H > +#define EN_GROUP_ECMP_ROUTE_H 1 > + > +#include "lib/inc-proc-eng.h" > +#include "openvswitch/hmap.h" > +#include "openvswitch/list.h" > +#include "northd/northd.h" > +#include <netinet/in.h> > + > +struct ecmp_route_list_node { > + struct ovs_list list_node; > + uint16_t id; /* starts from 1 */ > + const struct parsed_route *route; > +}; > + > +struct ecmp_groups_node { > + struct hmap_node hmap_node; /* In ecmp_groups */ > + uint16_t id; /* starts from 1 */ > + struct in6_addr prefix; > + unsigned int plen; > + bool is_src_route; > + enum route_source source; > + uint32_t route_table_id; > + uint16_t route_count; > + struct ovs_list route_list; /* Contains ecmp_route_list_node */ > + struct sset selection_fields; > +}; > + > +struct unique_routes_node { > + struct hmap_node hmap_node; > + const struct parsed_route *route; > +}; > + > +struct group_ecmp_route_node { Nit: I think the name should make it clear that this is a per datapath structure. Maybe 'struct group_ecmp_datapath'? What do you think? > + struct hmap_node hmap_node; > + > + /* The datapath for which this node is relevant. */ > + const struct ovn_datapath *od; > + > + /* Contains all routes that are part of an ecmp group. > + * Contains struct ecmp_groups_node. */ > + struct hmap ecmp_groups; > + > + /* Contains all routes that are not part of an ecmp group. > + * Contains struct unique_routes_node. */ > + struct hmap unique_routes; > +}; > + > +struct group_ecmp_route_data { > + /* Contains struct group_ecmp_route_node. > + * Each entry represents the routes of a single datapath. */ This second part of the comment should actually go above, in the definition of the node structure. > + struct hmap routes; These are not actual routes, they're routes per datapath, right? Should we just call it 'datapaths'? > +}; > + > +void *en_group_ecmp_route_init(struct engine_node *, struct engine_arg *); > +void en_group_ecmp_route_cleanup(void *data); > +void en_group_ecmp_route_clear_tracked_data(void *data); > +void en_group_ecmp_route_run(struct engine_node *, void *data); > + > +struct group_ecmp_route_node * group_ecmp_route_lookup( Nit: struct group_ecmp_route_node *group_ecmp_route_lookup( > + const struct group_ecmp_route_data *data, > + const struct ovn_datapath *od); > + > +#endif /* EN_GROUP_ECMP_ROUTE_H */ > diff --git a/northd/en-learned-route-sync.c b/northd/en-learned-route-sync.c > index 312141208..ae698a632 100644 > --- a/northd/en-learned-route-sync.c > +++ b/northd/en-learned-route-sync.c > @@ -31,7 +31,6 @@ VLOG_DEFINE_THIS_MODULE(en_learned_route_sync); > static void > routes_table_sync( > const struct sbrec_learned_route_table *sbrec_learned_route_table, > - const struct hmap *parsed_routes, > const struct hmap *lr_ports, > const struct ovn_datapaths *lr_datapaths, > struct hmap *parsed_routes_out); > @@ -128,8 +127,6 @@ en_learned_route_sync_run(struct engine_node *node, void > *data) > routes_sync_clear(data); > > struct learned_route_sync_data *routes_sync_data = data; > - struct routes_data *routes_data > - = engine_get_input_data("routes", node); > const struct sbrec_learned_route_table *sbrec_learned_route_table = > EN_OVSDB_GET(engine_get_input("SB_learned_route", node)); > struct northd_data *northd_data = engine_get_input_data("northd", node); > @@ -137,7 +134,6 @@ en_learned_route_sync_run(struct engine_node *node, void > *data) > stopwatch_start(LEARNED_ROUTE_SYNC_RUN_STOPWATCH_NAME, time_msec()); > > routes_table_sync(sbrec_learned_route_table, > - &routes_data->parsed_routes, > &northd_data->lr_ports, > &northd_data->lr_datapaths, > &routes_sync_data->parsed_routes); > @@ -217,7 +213,6 @@ parse_route_from_sbrec_route(struct hmap > *parsed_routes_out, > static void > routes_table_sync( > const struct sbrec_learned_route_table *sbrec_learned_route_table, > - const struct hmap *parsed_routes, > const struct hmap *lr_ports, > const struct ovn_datapaths *lr_datapaths, > struct hmap *parsed_routes_out) > @@ -234,12 +229,6 @@ routes_table_sync( > sb_route); > > } > - > - const struct parsed_route *route; > - HMAP_FOR_EACH (route, key_node, parsed_routes) { > - hmap_insert(parsed_routes_out, &parsed_route_clone(route)->key_node, > - parsed_route_hash(route)); > - } > } > > static struct parsed_route * > diff --git a/northd/en-lflow.c b/northd/en-lflow.c > index b79dcf95c..f0117d078 100644 > --- a/northd/en-lflow.c > +++ b/northd/en-lflow.c > @@ -48,8 +48,8 @@ lflow_get_input_data(struct engine_node *node, > engine_get_input_data("bfd_sync", node); > struct routes_data *routes_data = > engine_get_input_data("routes", node); > - struct learned_route_sync_data *learned_route_sync_data = > - engine_get_input_data("learned_route_sync", node); > + struct group_ecmp_route_data *group_ecmp_route_data = > + engine_get_input_data("group_ecmp_route", node); > struct route_policies_data *route_policies_data = > engine_get_input_data("route_policies", node); > struct port_group_data *pg_data = > @@ -86,7 +86,7 @@ lflow_get_input_data(struct engine_node *node, > lflow_input->lb_datapaths_map = &northd_data->lb_datapaths_map; > lflow_input->svc_monitor_map = &northd_data->svc_monitor_map; > lflow_input->bfd_ports = &bfd_sync_data->bfd_ports; > - lflow_input->parsed_routes = &learned_route_sync_data->parsed_routes; > + lflow_input->route_data = group_ecmp_route_data; > lflow_input->route_tables = &routes_data->route_tables; > lflow_input->route_policies = &route_policies_data->route_policies; > lflow_input->igmp_groups = &multicat_igmp_data->igmp_groups; > diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c > index bf0fbc62b..712aebaa1 100644 > --- a/northd/inc-proc-northd.c > +++ b/northd/inc-proc-northd.c > @@ -46,6 +46,7 @@ > #include "en-acl-ids.h" > #include "en-advertised-route-sync.h" > #include "en-learned-route-sync.h" > +#include "en-group-ecmp-route.h" > #include "unixctl.h" > #include "util.h" > > @@ -177,6 +178,7 @@ static ENGINE_NODE(advertised_route_sync, > "advertised_route_sync"); > static ENGINE_NODE_WITH_CLEAR_TRACK_DATA(learned_route_sync, > "learned_route_sync"); > static ENGINE_NODE(dynamic_routes, "dynamic_routes"); > +static ENGINE_NODE_WITH_CLEAR_TRACK_DATA(group_ecmp_route, > "group_ecmp_route"); > > void inc_proc_northd_init(struct ovsdb_idl_loop *nb, > struct ovsdb_idl_loop *sb) > @@ -307,12 +309,14 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, > engine_add_input(&en_advertised_route_sync, &en_northd, > advertised_route_sync_northd_change_handler); > > - engine_add_input(&en_learned_route_sync, &en_routes, NULL); > engine_add_input(&en_learned_route_sync, &en_sb_learned_route, > learned_route_sync_learned_route_change_handler); > engine_add_input(&en_learned_route_sync, &en_northd, > learned_route_sync_northd_change_handler); > > + engine_add_input(&en_group_ecmp_route, &en_routes, NULL); > + engine_add_input(&en_group_ecmp_route, &en_learned_route_sync, NULL); > + > engine_add_input(&en_sync_meters, &en_nb_acl, NULL); > engine_add_input(&en_sync_meters, &en_nb_meter, NULL); > engine_add_input(&en_sync_meters, &en_sb_meter, NULL); > @@ -333,7 +337,7 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, > /* XXX: This causes a full lflow recompute on each change to any route. > * At least for learned routes we should add incremental processing here. > * */ > - engine_add_input(&en_lflow, &en_learned_route_sync, NULL); > + engine_add_input(&en_lflow, &en_group_ecmp_route, NULL); > engine_add_input(&en_lflow, &en_global_config, > node_global_config_handler); > > diff --git a/northd/northd.c b/northd/northd.c > index 52bb67c0e..893e4c8fb 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -45,6 +45,7 @@ > #include "memory.h" > #include "northd.h" > #include "en-global-config.h" > +#include "en-group-ecmp-route.h" > #include "en-lb-data.h" > #include "en-lr-nat.h" > #include "en-lr-stateful.h" > @@ -11538,154 +11539,6 @@ build_lb_parsed_routes(const struct ovn_datapath > *od, > } > > } > -struct ecmp_route_list_node { > - struct ovs_list list_node; > - uint16_t id; /* starts from 1 */ > - const struct parsed_route *route; > -}; > - > -struct ecmp_groups_node { > - struct hmap_node hmap_node; /* In ecmp_groups */ > - uint16_t id; /* starts from 1 */ > - struct in6_addr prefix; > - unsigned int plen; > - bool is_src_route; > - enum route_source source; > - uint32_t route_table_id; > - uint16_t route_count; > - struct ovs_list route_list; /* Contains ecmp_route_list_node */ > - struct sset selection_fields; > -}; > - > -static void > -ecmp_groups_add_route(struct ecmp_groups_node *group, > - const struct parsed_route *route) > -{ > - if (group->route_count == UINT16_MAX) { > - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); > - VLOG_WARN_RL(&rl, "too many routes in a single ecmp group."); > - return; > - } > - > - struct ecmp_route_list_node *er = xmalloc(sizeof *er); > - er->route = route; > - er->id = ++group->route_count; > - > - if (group->route_count == 1) { > - sset_clone(&group->selection_fields, &route->ecmp_selection_fields); > - } else { > - sset_intersect(&group->selection_fields, > - &route->ecmp_selection_fields); > - } > - > - ovs_list_insert(&group->route_list, &er->list_node); > -} > - > -static struct ecmp_groups_node * > -ecmp_groups_add(struct hmap *ecmp_groups, > - const struct parsed_route *route) > -{ > - if (hmap_count(ecmp_groups) == UINT16_MAX) { > - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); > - VLOG_WARN_RL(&rl, "too many ecmp groups."); > - return NULL; > - } > - > - struct ecmp_groups_node *eg = xzalloc(sizeof *eg); > - hmap_insert(ecmp_groups, &eg->hmap_node, route->hash); > - > - eg->id = hmap_count(ecmp_groups); > - eg->prefix = route->prefix; > - eg->plen = route->plen; > - eg->is_src_route = route->is_src_route; > - eg->source = route->source; > - eg->route_table_id = route->route_table_id; > - sset_init(&eg->selection_fields); > - ovs_list_init(&eg->route_list); > - ecmp_groups_add_route(eg, route); > - > - return eg; > -} > - > -static struct ecmp_groups_node * > -ecmp_groups_find(struct hmap *ecmp_groups, struct parsed_route *route) > -{ > - struct ecmp_groups_node *eg; > - HMAP_FOR_EACH_WITH_HASH (eg, hmap_node, route->hash, ecmp_groups) { > - if (ipv6_addr_equals(&eg->prefix, &route->prefix) && > - eg->plen == route->plen && > - eg->is_src_route == route->is_src_route && > - eg->route_table_id == route->route_table_id && > - eg->source == route->source) { > - return eg; > - } > - } > - return NULL; > -} > - > -static void > -ecmp_groups_destroy(struct hmap *ecmp_groups) > -{ > - struct ecmp_groups_node *eg; > - HMAP_FOR_EACH_SAFE (eg, hmap_node, ecmp_groups) { > - struct ecmp_route_list_node *er; > - LIST_FOR_EACH_SAFE (er, list_node, &eg->route_list) { > - ovs_list_remove(&er->list_node); > - free(er); > - } > - hmap_remove(ecmp_groups, &eg->hmap_node); > - sset_destroy(&eg->selection_fields); > - free(eg); > - } > - hmap_destroy(ecmp_groups); > -} > - > -struct unique_routes_node { > - struct hmap_node hmap_node; > - const struct parsed_route *route; > -}; > - > -static void > -unique_routes_add(struct hmap *unique_routes, > - const struct parsed_route *route) > -{ > - struct unique_routes_node *ur = xmalloc(sizeof *ur); > - ur->route = route; > - hmap_insert(unique_routes, &ur->hmap_node, route->hash); > -} > - > -/* Remove the unique_routes_node from the hmap, and return the parsed_route > - * pointed by the removed node. */ > -static const struct parsed_route * > -unique_routes_remove(struct hmap *unique_routes, > - const struct parsed_route *route) > -{ > - struct unique_routes_node *ur; > - HMAP_FOR_EACH_WITH_HASH (ur, hmap_node, route->hash, unique_routes) { > - if (ipv6_addr_equals(&route->prefix, &ur->route->prefix) && > - route->plen == ur->route->plen && > - route->is_src_route == ur->route->is_src_route && > - route->source == ur->route->source && > - route->route_table_id == ur->route->route_table_id) { > - hmap_remove(unique_routes, &ur->hmap_node); > - const struct parsed_route *existed_route = ur->route; > - free(ur); > - return existed_route; > - } > - } > - return NULL; > -} > - > -static void > -unique_routes_destroy(struct hmap *unique_routes) > -{ > - struct unique_routes_node *ur; > - HMAP_FOR_EACH_SAFE (ur, hmap_node, unique_routes) { > - hmap_remove(unique_routes, &ur->hmap_node); > - free(ur); > - } > - hmap_destroy(unique_routes); > -} > > static char * > build_route_prefix_s(const struct in6_addr *prefix, unsigned int plen) > @@ -13969,7 +13822,7 @@ build_ip_routing_pre_flows_for_lrouter(struct > ovn_datapath *od, > static void > build_route_flows_for_lrouter( > struct ovn_datapath *od, struct lflow_table *lflows, > - struct hmap *parsed_routes, > + const struct group_ecmp_route_data *route_data, > struct simap *route_tables, const struct sset *bfd_ports, > struct lflow_ref *lflow_ref) > { > @@ -13982,47 +13835,19 @@ build_route_flows_for_lrouter( > REG_ECMP_GROUP_ID" == 0", "next;", > lflow_ref); > > - struct hmap ecmp_groups = HMAP_INITIALIZER(&ecmp_groups); > - struct hmap unique_routes = HMAP_INITIALIZER(&unique_routes); > - struct ecmp_groups_node *group; > - > for (int i = 0; i < od->nbr->n_ports; i++) { > build_route_table_lflow(od, lflows, od->nbr->ports[i], > route_tables, lflow_ref); > } > > - struct parsed_route *route; > - HMAP_FOR_EACH_WITH_HASH (route, key_node, uuid_hash(&od->key), > - parsed_routes) { > - if (od != route->od) { > - continue; > - } > - if (route->source == ROUTE_SOURCE_CONNECTED) { > - unique_routes_add(&unique_routes, route); > - continue; > - } > - group = ecmp_groups_find(&ecmp_groups, route); > - if (group) { > - ecmp_groups_add_route(group, route); > - } else { > - const struct parsed_route *existed_route = > - unique_routes_remove(&unique_routes, route); > - if (existed_route) { > - group = ecmp_groups_add(&ecmp_groups, existed_route); > - if (group) { > - ecmp_groups_add_route(group, route); > - } > - } else if (route->ecmp_symmetric_reply) { > - /* Traffic for symmetric reply routes has to be conntracked > - * even if there is only one next-hop, in case another > next-hop > - * is added later. */ > - ecmp_groups_add(&ecmp_groups, route); > - } else { > - unique_routes_add(&unique_routes, route); > - } > - } > + const struct group_ecmp_route_node *route_node = > + group_ecmp_route_lookup(route_data, od); > + if (!route_node) { > + return; > } > - HMAP_FOR_EACH (group, hmap_node, &ecmp_groups) { > + > + struct ecmp_groups_node *group; > + HMAP_FOR_EACH (group, hmap_node, &route_node->ecmp_groups) { > /* add a flow in IP_ROUTING, and one flow for each member in > * IP_ROUTING_ECMP. */ > build_ecmp_route_flow(lflows, od, group, lflow_ref, NULL); > @@ -14037,11 +13862,9 @@ build_route_flows_for_lrouter( > } > } > const struct unique_routes_node *ur; > - HMAP_FOR_EACH (ur, hmap_node, &unique_routes) { > + HMAP_FOR_EACH (ur, hmap_node, &route_node->unique_routes) { > build_route_flow(lflows, od, ur->route, bfd_ports, lflow_ref); > } > - ecmp_groups_destroy(&ecmp_groups); > - unique_routes_destroy(&unique_routes); > } > > static void > @@ -17600,7 +17423,7 @@ struct lswitch_flow_build_info { > size_t thread_lflow_counter; > const char *svc_monitor_mac; > const struct sampling_app_table *sampling_apps; > - struct hmap *parsed_routes; > + const struct group_ecmp_route_data *route_data; > struct hmap *route_policies; > struct simap *route_tables; > const struct sbrec_acl_id_table *sbrec_acl_id_table; > @@ -17650,7 +17473,7 @@ build_lswitch_and_lrouter_iterate_by_lr(struct > ovn_datapath *od, > build_ND_RA_flows_for_lrouter(od, lsi->lflows, NULL); > build_ip_routing_pre_flows_for_lrouter(od, lsi->lflows, NULL); > build_route_flows_for_lrouter(od, lsi->lflows, > - lsi->parsed_routes, lsi->route_tables, > + lsi->route_data, lsi->route_tables, > lsi->bfd_ports, NULL); > build_mcast_lookup_flows_for_lrouter(od, lsi->lflows, &lsi->match, NULL); > build_ingress_policy_flows_for_lrouter(od, lsi->lflows, lsi->lr_ports, > @@ -17960,7 +17783,7 @@ build_lswitch_and_lrouter_flows( > const struct chassis_features *features, > const char *svc_monitor_mac, > const struct sampling_app_table *sampling_apps, > - struct hmap *parsed_routes, > + const struct group_ecmp_route_data *route_data, > struct hmap *route_policies, > struct simap *route_tables, > const struct sbrec_acl_id_table *sbrec_acl_id_table) > @@ -17997,7 +17820,7 @@ build_lswitch_and_lrouter_flows( > lsiv[index].thread_lflow_counter = 0; > lsiv[index].svc_monitor_mac = svc_monitor_mac; > lsiv[index].sampling_apps = sampling_apps; > - lsiv[index].parsed_routes = parsed_routes; > + lsiv[index].route_data = route_data; > lsiv[index].route_tables = route_tables; > lsiv[index].route_policies = route_policies; > lsiv[index].sbrec_acl_id_table = sbrec_acl_id_table; > @@ -18040,7 +17863,7 @@ build_lswitch_and_lrouter_flows( > .svc_check_match = svc_check_match, > .svc_monitor_mac = svc_monitor_mac, > .sampling_apps = sampling_apps, > - .parsed_routes = parsed_routes, > + .route_data = route_data, > .route_tables = route_tables, > .route_policies = route_policies, > .match = DS_EMPTY_INITIALIZER, > @@ -18208,7 +18031,7 @@ void build_lflows(struct ovsdb_idl_txn *ovnsb_txn, > input_data->features, > input_data->svc_monitor_mac, > input_data->sampling_apps, > - input_data->parsed_routes, > + input_data->route_data, > input_data->route_policies, > input_data->route_tables, > input_data->sbrec_acl_id_table); > diff --git a/northd/northd.h b/northd/northd.h > index 9402e4a4b..b83efdb63 100644 > --- a/northd/northd.h > +++ b/northd/northd.h > @@ -236,7 +236,7 @@ struct lflow_input { > bool ovn_internal_version_changed; > const char *svc_monitor_mac; > const struct sampling_app_table *sampling_apps; > - struct hmap *parsed_routes; > + struct group_ecmp_route_data *route_data; > struct hmap *route_policies; > struct simap *route_tables; > struct hmap *igmp_groups; > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index bb77f2abc..8d77b3534 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -15516,6 +15516,7 @@ check_engine_compute northd recompute > check_engine_compute routes recompute > check_engine_compute advertised_route_sync recompute > check_engine_compute learned_route_sync recompute > +check_engine_compute group_ecmp_route recompute > check_engine_compute lflow recompute > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > @@ -15530,6 +15531,7 @@ check_engine_compute northd recompute > check_engine_compute routes recompute > check_engine_compute advertised_route_sync recompute > check_engine_compute learned_route_sync recompute > +check_engine_compute group_ecmp_route recompute > check_engine_compute lflow recompute > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > @@ -15540,6 +15542,7 @@ check_engine_compute northd recompute > check_engine_compute routes recompute > check_engine_compute advertised_route_sync recompute > check_engine_compute learned_route_sync recompute > +check_engine_compute group_ecmp_route recompute > check_engine_compute lflow recompute > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > @@ -15549,6 +15552,7 @@ check_engine_compute northd recompute > check_engine_compute routes recompute > check_engine_compute advertised_route_sync recompute > check_engine_compute learned_route_sync recompute > +check_engine_compute group_ecmp_route recompute > check_engine_compute lflow recompute > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > @@ -15559,6 +15563,7 @@ check_engine_compute northd recompute > check_engine_compute routes recompute > check_engine_compute advertised_route_sync recompute > check_engine_compute learned_route_sync recompute > +check_engine_compute group_ecmp_route recompute > check_engine_compute lflow recompute > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > @@ -15568,6 +15573,7 @@ check_engine_compute northd recompute > check_engine_compute routes recompute > check_engine_compute advertised_route_sync recompute > check_engine_compute learned_route_sync recompute > +check_engine_compute group_ecmp_route recompute > check_engine_compute lflow recompute > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > @@ -15577,6 +15583,7 @@ check_engine_compute northd recompute > check_engine_compute routes recompute > check_engine_compute advertised_route_sync recompute > check_engine_compute learned_route_sync recompute > +check_engine_compute group_ecmp_route recompute > check_engine_compute lflow recompute > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > @@ -15587,6 +15594,7 @@ check_engine_compute northd recompute > check_engine_compute routes recompute > check_engine_compute advertised_route_sync recompute > check_engine_compute learned_route_sync recompute > +check_engine_compute group_ecmp_route recompute > check_engine_compute lflow recompute > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > @@ -15597,6 +15605,7 @@ check_engine_compute northd recompute > check_engine_compute routes recompute > check_engine_compute advertised_route_sync recompute > check_engine_compute learned_route_sync recompute > +check_engine_compute group_ecmp_route recompute > check_engine_compute lflow recompute > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > @@ -15607,6 +15616,7 @@ check_engine_compute northd recompute > check_engine_compute routes recompute > check_engine_compute advertised_route_sync recompute > check_engine_compute learned_route_sync recompute > +check_engine_compute group_ecmp_route recompute > check_engine_compute lflow recompute > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > @@ -15617,6 +15627,7 @@ check_engine_compute northd recompute > check_engine_compute routes recompute > check_engine_compute advertised_route_sync recompute > check_engine_compute learned_route_sync recompute > +check_engine_compute group_ecmp_route recompute > check_engine_compute lflow recompute > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > @@ -15631,6 +15642,7 @@ check_engine_compute northd unchanged > check_engine_compute routes unchanged > check_engine_compute advertised_route_sync unchanged > check_engine_compute learned_route_sync incremental > +check_engine_compute group_ecmp_route recompute > check_engine_compute lflow recompute > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > @@ -15641,6 +15653,7 @@ check_engine_compute northd unchanged > check_engine_compute routes unchanged > check_engine_compute advertised_route_sync unchanged > check_engine_compute learned_route_sync incremental > +check_engine_compute group_ecmp_route recompute > check_engine_compute lflow recompute > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > @@ -15651,6 +15664,7 @@ check_engine_compute northd recompute > check_engine_compute routes recompute > check_engine_compute advertised_route_sync recompute > check_engine_compute learned_route_sync recompute > +check_engine_compute group_ecmp_route recompute > check_engine_compute lflow recompute > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > @@ -15661,6 +15675,7 @@ check_engine_compute northd incremental > check_engine_compute routes incremental > check_engine_compute advertised_route_sync recompute > check_engine_compute learned_route_sync incremental > +check_engine_compute group_ecmp_route unchanged > check_engine_compute lflow incremental > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > @@ -15674,6 +15689,7 @@ check_engine_compute northd recompute > check_engine_compute routes recompute > check_engine_compute advertised_route_sync recompute > check_engine_compute learned_route_sync recompute > +check_engine_compute group_ecmp_route recompute > check_engine_compute lflow recompute > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > @@ -15683,6 +15699,7 @@ check_engine_compute northd incremental > check_engine_compute routes incremental > check_engine_compute advertised_route_sync recompute > check_engine_compute learned_route_sync incremental > +check_engine_compute group_ecmp_route unchanged > check_engine_compute lflow incremental > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > @@ -15692,6 +15709,7 @@ check_engine_compute northd recompute > check_engine_compute routes recompute > check_engine_compute advertised_route_sync recompute > check_engine_compute learned_route_sync recompute > +check_engine_compute group_ecmp_route recompute > check_engine_compute lflow recompute > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > @@ -15701,6 +15719,7 @@ check_engine_compute northd recompute > check_engine_compute routes recompute > check_engine_compute advertised_route_sync recompute > check_engine_compute learned_route_sync recompute > +check_engine_compute group_ecmp_route recompute > check_engine_compute lflow recompute > CHECK_NO_CHANGE_AFTER_RECOMPUTE > Thanks, Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
