On Thu, Feb 20, 2025 at 02:47:08PM +0100, Dumitru Ceara wrote: > 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:
Hi Dumitru, thanks a lot for the review. > > 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". I'll change both. > > > 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. Sounds good. > > > + > > > > 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 I'll change them in the next version. Thanks a lot, Felix > > Thanks, > Dumitru > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
