On 2/18/25 3:02 PM, Felix Huettner via dev wrote: > Previously we always recomputed lflows on any kind of route changes. > With these changes we can now incrementally handle these changes. > However because of limitations of the group_ecmp_route engine node we > can currently only handle learned routes incrementally. > > Signed-off-by: Felix Huettner <[email protected]> > ---
Hi Felix, I only have a few minor comments below. If that's the only thing that changes in v3 for this patch, feel free to add my ack: Acked-by: Dumitru Ceara <[email protected]> > northd/en-lflow.c | 66 +++++++++++++++++++++++++++++++++++++++- > northd/en-lflow.h | 1 + > northd/inc-proc-northd.c | 3 +- > northd/northd.c | 49 +++++++++++++++++++---------- > northd/northd.h | 5 +++ > tests/ovn-northd.at | 4 +-- > 6 files changed, 108 insertions(+), 20 deletions(-) > > diff --git a/northd/en-lflow.c b/northd/en-lflow.c > index f0117d078..fe79d083c 100644 > --- a/northd/en-lflow.c > +++ b/northd/en-lflow.c > @@ -27,7 +27,7 @@ > #include "en-northd.h" > #include "en-meters.h" > #include "en-sampling-app.h" > -#include "en-learned-route-sync.h" > +#include "en-group-ecmp-route.h" > #include "lflow-mgr.h" > > #include "lib/inc-proc-eng.h" > @@ -268,6 +268,70 @@ lflow_multicast_igmp_handler(struct engine_node *node, > void *data) > return true; > } > > +bool > +lflow_group_ecmp_route_handler(struct engine_node *node, void *data > OVS_UNUSED) > +{ > + struct group_ecmp_route_data *group_ecmp_route_data = > + engine_get_input_data("group_ecmp_route", node); > + > + /* If we do not have tracked data we need to recompute. */ > + if (!group_ecmp_route_data->tracked) { > + return false; > + } > + > + const struct engine_context *eng_ctx = engine_get_context(); > + struct lflow_data *lflow_data = data; > + > + struct lflow_input lflow_input; > + lflow_get_input_data(node, &lflow_input); > + > + struct group_ecmp_route_node *route_node; > + struct hmapx_node *hmapx_node; > + > + /* We need to handle deletions before additions as they could potentially > + * overlap. */ > + HMAPX_FOR_EACH (hmapx_node, > + &group_ecmp_route_data->trk_data.deleted_routes) { > + route_node = hmapx_node->data; > + lflow_ref_unlink_lflows(route_node->lflow_ref); > + > + bool handled = lflow_ref_sync_lflows( > + route_node->lflow_ref, lflow_data->lflow_table, > + eng_ctx->ovnsb_idl_txn, lflow_input.ls_datapaths, > + lflow_input.lr_datapaths, > + lflow_input.ovn_internal_version_changed, > + lflow_input.sbrec_logical_flow_table, > + lflow_input.sbrec_logical_dp_group_table); > + if (!handled) { > + return false; > + } > + } > + > + /* Now we handle created or updated route nodes. */ > + HMAPX_FOR_EACH (hmapx_node, > + &group_ecmp_route_data->trk_data.crupdated_routes) { > + route_node = hmapx_node->data; > + lflow_ref_unlink_lflows(route_node->lflow_ref); > + build_route_data_flows_for_lrouter( > + route_node->od, lflow_data->lflow_table, > + route_node, lflow_input.bfd_ports); > + > + bool handled = lflow_ref_sync_lflows( > + route_node->lflow_ref, lflow_data->lflow_table, > + eng_ctx->ovnsb_idl_txn, lflow_input.ls_datapaths, > + lflow_input.lr_datapaths, > + lflow_input.ovn_internal_version_changed, > + lflow_input.sbrec_logical_flow_table, > + lflow_input.sbrec_logical_dp_group_table); > + if (!handled) { > + return false; > + } > + } > + > + engine_set_node_state(node, EN_UPDATED); > + return true; > +} > + > void *en_lflow_init(struct engine_node *node OVS_UNUSED, > struct engine_arg *arg OVS_UNUSED) > { > diff --git a/northd/en-lflow.h b/northd/en-lflow.h > index f90f5c61c..2d9b0a540 100644 > --- a/northd/en-lflow.h > +++ b/northd/en-lflow.h > @@ -23,5 +23,6 @@ bool lflow_port_group_handler(struct engine_node *, void > *data); > bool lflow_lr_stateful_handler(struct engine_node *, void *data); > bool lflow_ls_stateful_handler(struct engine_node *node, void *data); > bool lflow_multicast_igmp_handler(struct engine_node *node, void *data); > +bool lflow_group_ecmp_route_handler(struct engine_node *node, void *data); > > #endif /* EN_LFLOW_H */ > diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c > index 147f54da0..656b56df9 100644 > --- a/northd/inc-proc-northd.c > +++ b/northd/inc-proc-northd.c > @@ -338,7 +338,8 @@ 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. > * */ I guess we should update this comment too. > - engine_add_input(&en_lflow, &en_group_ecmp_route, NULL); > + engine_add_input(&en_lflow, &en_group_ecmp_route, > + lflow_group_ecmp_route_handler); Other patches in this series used the "_change_handler" suffix. Maybe we should call this "lflow_group_ecmp_route_change_handler". > engine_add_input(&en_lflow, &en_global_config, > node_global_config_handler); > > diff --git a/northd/northd.c b/northd/northd.c > index 45e43ce37..27149a526 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -11693,7 +11693,7 @@ find_static_route_outport(const struct ovn_datapath > *od, > > static void > add_ecmp_symmetric_reply_flows(struct lflow_table *lflows, > - struct ovn_datapath *od, > + const struct ovn_datapath *od, > const char *port_ip, > const struct ovn_port *out_port, > const struct parsed_route *route, > @@ -11790,7 +11790,8 @@ add_ecmp_symmetric_reply_flows(struct lflow_table > *lflows, > } > > static void > -build_ecmp_route_flow(struct lflow_table *lflows, struct ovn_datapath *od, > +build_ecmp_route_flow(struct lflow_table *lflows, > + const struct ovn_datapath *od, > struct ecmp_groups_node *eg, struct lflow_ref > *lflow_ref, > const char *protocol) > > @@ -11909,7 +11910,7 @@ build_ecmp_route_flow(struct lflow_table *lflows, > struct ovn_datapath *od, > } > > static void > -add_route(struct lflow_table *lflows, struct ovn_datapath *od, > +add_route(struct lflow_table *lflows, const struct ovn_datapath *od, > const struct ovn_port *op, const char *lrp_addr_s, > const char *network_s, int plen, const struct in6_addr *gateway, > bool is_src_route, const uint32_t rtb_id, > @@ -11983,10 +11984,9 @@ add_route(struct lflow_table *lflows, struct > ovn_datapath *od, > } > > static void > -build_route_flow(struct lflow_table *lflows, struct ovn_datapath *od, > - const struct parsed_route *route, > - const struct sset *bfd_ports, > - struct lflow_ref *lflow_ref) > +build_route_flow(struct lflow_table *lflows, const struct ovn_datapath *od, > + const struct parsed_route *route, > + const struct sset *bfd_ports, struct lflow_ref *lflow_ref) > { > bool is_ipv4_prefix = IN6_IS_ADDR_V4MAPPED(&route->prefix); > bool is_ipv4_nexthop = route->nexthop > @@ -13820,12 +13820,10 @@ build_ip_routing_pre_flows_for_lrouter(struct > ovn_datapath *od, > } > > static void > -build_route_flows_for_lrouter( > +build_default_route_flows_for_lrouter( > struct ovn_datapath *od, struct lflow_table *lflows, > - const struct group_ecmp_route_data *route_data, > - struct simap *route_tables, const struct sset *bfd_ports) > + struct simap *route_tables) > { > - ovs_assert(od->nbr); > ovn_lflow_add_default_drop(lflows, od, S_ROUTER_IN_IP_ROUTING_ECMP, > NULL); > ovn_lflow_add_default_drop(lflows, od, S_ROUTER_IN_IP_ROUTING, > @@ -13839,12 +13837,14 @@ build_route_flows_for_lrouter( > route_tables, NULL); > } > > - const struct group_ecmp_route_node *route_node = > - group_ecmp_route_lookup(route_data, od); > - if (!route_node) { > - return; > - } > +} > > +void > +build_route_data_flows_for_lrouter( > + const struct ovn_datapath *od, struct lflow_table *lflows, > + const struct group_ecmp_route_node *route_node, > + const struct sset *bfd_ports) > +{ > 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 > @@ -13870,6 +13870,23 @@ build_route_flows_for_lrouter( > } > } > > +static void > +build_route_flows_for_lrouter( > + struct ovn_datapath *od, struct lflow_table *lflows, > + const struct group_ecmp_route_data *route_data, > + struct simap *route_tables, const struct sset *bfd_ports) > +{ > + ovs_assert(od->nbr); > + build_default_route_flows_for_lrouter(od, lflows, route_tables); > + > + const struct group_ecmp_route_node *route_node = > + group_ecmp_route_lookup(route_data, od); > + if (!route_node) { > + return; > + } > + build_route_data_flows_for_lrouter(od, lflows, route_node, bfd_ports); > +} > + > static void > build_lrouter_ip_mcast_igmp_mld(struct ovn_igmp_group *igmp_group, > struct lflow_table *lflows, > diff --git a/northd/northd.h b/northd/northd.h > index b83efdb63..949589939 100644 > --- a/northd/northd.h > +++ b/northd/northd.h > @@ -859,11 +859,16 @@ void bfd_sync_destroy(struct bfd_sync_data *); > struct lflow_table; > struct lr_stateful_tracked_data; > struct ls_stateful_tracked_data; > +struct group_ecmp_route_node; > > void build_lflows(struct ovsdb_idl_txn *ovnsb_txn, > struct lflow_input *input_data, > struct lflow_table *); > void lflow_reset_northd_refs(struct lflow_input *); > +void build_route_data_flows_for_lrouter( > + const struct ovn_datapath *, struct lflow_table *, > + const struct group_ecmp_route_node *, const struct sset *); It's not clear just by looking at this declaration what the sset is about. Let's name the argument, i.e., const struct sset *bfd_ports. > + > > bool lflow_handle_northd_port_changes(struct ovsdb_idl_txn *ovnsb_txn, > struct tracked_ovn_ports *, > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index b70deade3..71b7ed944 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -15643,7 +15643,7 @@ 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 incremental > -check_engine_compute lflow recompute > +check_engine_compute lflow incremental > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats > @@ -15654,7 +15654,7 @@ 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 incremental > -check_engine_compute lflow recompute > +check_engine_compute lflow incremental > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats Thanks, Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
