On Thu, Feb 20, 2025 at 02:46:44PM +0100, Dumitru Ceara wrote: > 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.
Hi Dumitru, thanks a lot for the review. > > > 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. Sounds good. > > > + > > +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? Yes that is better. > > > + 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'? i'll change both of these. > > > +}; > > + > > +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 > > > I'll address all of these in the next version. Thanks a lot, Felix > Thanks, > Dumitru > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
