On Tue, Nov 18, 2025 at 12:56 AM Lorenzo Bianconi via dev < [email protected]> wrote:
Hi Lorenzo, thank you for the patch, I have a couple of additional comments. Introduce logical-flows incremental processing support for > logical_switch creation/deletion. > > The patch is missing Reported-at. > Signed-off-by: Lorenzo Bianconi <[email protected]> > --- > northd/en-lflow.c | 13 ++-- > northd/en-ls-stateful.c | 6 +- > northd/lflow-mgr.c | 8 ++- > northd/northd.c | 141 ++++++++++++++++++++++++++++++++-------- > northd/northd.h | 8 ++- > tests/ovn-northd.at | 16 ++++- > 6 files changed, 152 insertions(+), 40 deletions(-) > > diff --git a/northd/en-lflow.c b/northd/en-lflow.c > index 30bf7e35c..14a211bbb 100644 > --- a/northd/en-lflow.c > +++ b/northd/en-lflow.c > @@ -145,10 +145,6 @@ lflow_northd_handler(struct engine_node *node, > return EN_UNHANDLED; > } > > - if (northd_has_lswitches_in_tracked_data(&northd_data->trk_data)) { > - return EN_UNHANDLED; > - } > - > const struct engine_context *eng_ctx = engine_get_context(); > struct lflow_data *lflow_data = data; > > @@ -162,6 +158,13 @@ lflow_northd_handler(struct engine_node *node, > return EN_UNHANDLED; > } > > + if (!lflow_handle_northd_ls_changes(eng_ctx->ovnsb_idl_txn, > + > &northd_data->trk_data.trk_switches, > + &lflow_input, > + lflow_data->lflow_table)) { > + return EN_UNHANDLED; > + } > + > if (!lflow_handle_northd_port_changes(eng_ctx->ovnsb_idl_txn, > &northd_data->trk_data.trk_lsps, > &lflow_input, > @@ -214,6 +217,7 @@ lflow_ls_stateful_handler(struct engine_node *node, > void *data) > return EN_UNHANDLED; > } > > + struct northd_data *northd_data = engine_get_input_data("northd", > node); > const struct engine_context *eng_ctx = engine_get_context(); > struct lflow_data *lflow_data = data; > struct lflow_input lflow_input; > @@ -221,6 +225,7 @@ lflow_ls_stateful_handler(struct engine_node *node, > void *data) > lflow_get_input_data(node, &lflow_input); > if (!lflow_handle_ls_stateful_changes(eng_ctx->ovnsb_idl_txn, > &ls_sful_data->trk_data, > + > &northd_data->trk_data.trk_switches, > &lflow_input, > lflow_data->lflow_table)) { > return EN_UNHANDLED; > diff --git a/northd/en-ls-stateful.c b/northd/en-ls-stateful.c > index a9a685504..0dea24aee 100644 > --- a/northd/en-ls-stateful.c > +++ b/northd/en-ls-stateful.c > @@ -161,8 +161,10 @@ ls_stateful_northd_handler(struct engine_node *node, > void *data_) > const struct ovn_datapath *od = hmapx_node->data; > > if (!ls_stateful_table_find_(&data->table, od->nbs)) { > - ls_stateful_record_create(&data->table, od, > - input_data.ls_port_groups); > + struct ls_stateful_record *ls_stateful_rec = > + ls_stateful_record_create(&data->table, od, > + input_data.ls_port_groups); > + hmapx_add(&data->trk_data.crupdated, ls_stateful_rec); > } > } > > diff --git a/northd/lflow-mgr.c b/northd/lflow-mgr.c > index d1fbc2196..f2eb2851c 100644 > --- a/northd/lflow-mgr.c > +++ b/northd/lflow-mgr.c > @@ -745,6 +745,10 @@ lflow_table_add_lflow(struct lflow_table *lflow_table, > hmap_insert(&lflow_ref->lflow_ref_nodes, &lrn->ref_node, > hash); > } > > + if (lrn->dpgrp_lflow) { > + dynamic_bitmap_realloc(&lrn->dpgrp_bitmap, dp_bitmap_len); > + } > + > if (!lrn->linked) { > if (lrn->dpgrp_lflow) { > ovs_assert(lrn->dpgrp_bitmap.capacity == dp_bitmap_len); > @@ -1277,7 +1281,9 @@ ovn_sb_insert_or_update_logical_dp_group( > BITMAP_FOR_EACH_1 (index, ods_size(datapaths), dpg_bitmap) { > struct ovn_datapath *od = vector_get(&datapaths->dps, index, > struct ovn_datapath *); > - sb[n++] = od->sdp->sb_dp; > + if (od) { > + sb[n++] = od->sdp->sb_dp; > + } > } > if (!dp_group) { > struct uuid dpg_uuid = uuid_random(); > diff --git a/northd/northd.c b/northd/northd.c > index cdf12ec86..c8f526660 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -5729,10 +5729,13 @@ enum mirror_filter { > > static void > build_mirror_default_lflow(struct ovn_datapath *od, > - struct lflow_table *lflows) > + struct lflow_table *lflows, > + struct lflow_ref *lflow_ref) > { > - ovn_lflow_add(lflows, od, S_SWITCH_IN_MIRROR, 0, "1", "next;", NULL); > - ovn_lflow_add(lflows, od, S_SWITCH_OUT_MIRROR, 0, "1", "next;", NULL); > + ovn_lflow_add(lflows, od, S_SWITCH_IN_MIRROR, 0, "1", "next;", > + lflow_ref); > + ovn_lflow_add(lflows, od, S_SWITCH_OUT_MIRROR, 0, "1", "next;", > + lflow_ref); > } > > static void > @@ -18564,6 +18567,7 @@ struct lswitch_flow_build_info { > struct hmap *route_policies; > struct simap *route_tables; > const struct sbrec_acl_id_table *sbrec_acl_id_table; > + struct lflow_ref *igmp_lflow_ref; > I'm worried about passing the igmp_lflow ref. That one is not resynced at the end, but it actually can't be because we would need to refresh all igmp flows. Maybe we need to actually track if any incrementally processed switch is part of igmp group and in that case process all igmp groups. It shouldn't cost us much as the number of igmp groups is limited. }; > > /* Helper function to combine all lflow generation which is iterated by > @@ -18577,31 +18581,39 @@ build_lswitch_and_lrouter_iterate_by_ls(struct > ovn_datapath *od, > struct lswitch_flow_build_info > *lsi) > { > ovs_assert(od->nbs); > - build_mirror_default_lflow(od, lsi->lflows); > + build_mirror_default_lflow(od, lsi->lflows, od->datapath_lflows); > build_lswitch_lflows_pre_acl_and_acl(od, lsi->lflows, > - lsi->meter_groups, NULL); > - build_network_function(od, lsi->lflows, lsi->ls_port_groups, NULL); > - build_fwd_group_lflows(od, lsi->lflows, NULL); > - build_lswitch_lflows_admission_control(od, lsi->lflows, NULL); > - build_lswitch_learn_fdb_od(od, lsi->lflows, NULL); > - build_lswitch_arp_nd_responder_default(od, lsi->lflows, NULL); > + lsi->meter_groups, > + od->datapath_lflows); > + build_network_function(od, lsi->lflows, lsi->ls_port_groups, > + od->datapath_lflows); > + build_fwd_group_lflows(od, lsi->lflows, od->datapath_lflows); > + build_lswitch_lflows_admission_control(od, lsi->lflows, > + od->datapath_lflows); > + build_lswitch_learn_fdb_od(od, lsi->lflows, od->datapath_lflows); > + build_lswitch_arp_nd_responder_default(od, lsi->lflows, > + od->datapath_lflows); > build_lswitch_dns_lookup_and_response(od, lsi->lflows, > lsi->meter_groups, > - NULL); > - build_lswitch_dhcp_and_dns_defaults(od, lsi->lflows, NULL); > + od->datapath_lflows); > + build_lswitch_dhcp_and_dns_defaults(od, lsi->lflows, > od->datapath_lflows); > build_lswitch_destination_lookup_bmcast(od, lsi->lflows, > &lsi->actions, > - lsi->meter_groups, NULL); > - build_lswitch_output_port_sec_od(od, lsi->lflows, NULL); > + lsi->meter_groups, > + od->datapath_lflows); > + build_lswitch_output_port_sec_od(od, lsi->lflows, > od->datapath_lflows); > /* CT extraction flows are built with stateful flows, but default > rule is > * always needed */ > ovn_lflow_add(lsi->lflows, od, S_SWITCH_IN_CT_EXTRACT, 0, "1", > "next;", > - NULL); > - build_lswitch_lb_affinity_default_flows(od, lsi->lflows, NULL); > + od->datapath_lflows); > + build_lswitch_lb_affinity_default_flows(od, lsi->lflows, > + od->datapath_lflows); > if (od->has_evpn_vni) { > - build_lswitch_lflows_evpn_l2_unknown(od, lsi->lflows, NULL); > + build_lswitch_lflows_evpn_l2_unknown(od, lsi->lflows, > + od->datapath_lflows); > } else { > - build_lswitch_lflows_l2_unknown(od, lsi->lflows, NULL); > + build_lswitch_lflows_l2_unknown(od, lsi->lflows, > od->datapath_lflows); > } > - build_mcast_flood_lswitch(od, lsi->lflows, &lsi->actions, NULL); > + build_mcast_flood_lswitch(od, lsi->lflows, &lsi->actions, > + lsi->igmp_lflow_ref); > } > > /* Helper function to combine all lflow generation which is iterated by > @@ -18954,7 +18966,8 @@ build_lswitch_and_lrouter_flows( > 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) > + const struct sbrec_acl_id_table *sbrec_acl_id_table, > + struct lflow_ref *igmp_lflow_ref) > { > > char *svc_check_match = xasprintf("eth.dst == %s", svc_monitor_mac); > @@ -18995,6 +19008,7 @@ build_lswitch_and_lrouter_flows( > lsiv[index].route_tables = route_tables; > lsiv[index].route_policies = route_policies; > lsiv[index].sbrec_acl_id_table = sbrec_acl_id_table; > + lsiv[index].igmp_lflow_ref = igmp_lflow_ref; > ds_init(&lsiv[index].match); > ds_init(&lsiv[index].actions); > > @@ -19045,6 +19059,7 @@ build_lswitch_and_lrouter_flows( > .match = DS_EMPTY_INITIALIZER, > .actions = DS_EMPTY_INITIALIZER, > .sbrec_acl_id_table = sbrec_acl_id_table, > + .igmp_lflow_ref = igmp_lflow_ref, > }; > > /* Combined build - all lflow generation from lswitch and lrouter > @@ -19216,7 +19231,8 @@ void build_lflows(struct ovsdb_idl_txn *ovnsb_txn, > input_data->route_data, > input_data->route_policies, > input_data->route_tables, > - input_data->sbrec_acl_id_table); > + input_data->sbrec_acl_id_table, > + input_data->igmp_lflow_ref); > build_igmp_lflows(input_data->igmp_groups, > &input_data->ls_datapaths->datapaths, > lflows, input_data->igmp_lflow_ref); > @@ -19336,6 +19352,59 @@ lflow_handle_northd_lr_changes(struct > ovsdb_idl_txn *ovnsb_txn, > return handled; > } > > +bool > +lflow_handle_northd_ls_changes(struct ovsdb_idl_txn *ovnsb_txn, > + struct tracked_dps *tracked_ls, > + struct lflow_input *lflow_input, > + struct lflow_table *lflows) > +{ > + struct hmapx_node *hmapx_node; > + struct ovn_datapath *od; > + bool handled = true; > + > + HMAPX_FOR_EACH (hmapx_node, &tracked_ls->deleted) { > + od = hmapx_node->data; > + if (!lflow_ref_resync_flows( > + od->datapath_lflows, lflows, ovnsb_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)) { > + return false; > + } > + } > + > + struct lswitch_flow_build_info lsi = { > + .meter_groups = lflow_input->meter_groups, > + .igmp_lflow_ref = lflow_input->igmp_lflow_ref, > + .ls_port_groups = lflow_input->ls_port_groups, > + .lflows = lflows, > + .match = DS_EMPTY_INITIALIZER, > + .actions = DS_EMPTY_INITIALIZER, > + }; > + > + HMAPX_FOR_EACH (hmapx_node, &tracked_ls->crupdated) { > + od = hmapx_node->data; > + build_lswitch_and_lrouter_iterate_by_ls(od, &lsi); > + handled = lflow_ref_sync_lflows( > + od->datapath_lflows, lflows, ovnsb_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); > This has the same performance problem as with LR. It should be possible to do the same thing as in the LR fix [0]. > + if (!handled) { > + break; > + } > + } > + > + ds_destroy(&lsi.actions); > + ds_destroy(&lsi.match); > + > + return handled; > +} > + > bool > lflow_handle_northd_port_changes(struct ovsdb_idl_txn *ovnsb_txn, > struct tracked_ovn_ports *trk_lsps, > @@ -19653,6 +19722,7 @@ exit: > bool > lflow_handle_ls_stateful_changes(struct ovsdb_idl_txn *ovnsb_txn, > struct ls_stateful_tracked_data *trk_data, > + struct tracked_dps *tracked_ls, > struct lflow_input *lflow_input, > struct lflow_table *lflows) > { > @@ -19695,13 +19765,28 @@ lflow_handle_ls_stateful_changes(struct > ovsdb_idl_txn *ovnsb_txn, > > HMAPX_FOR_EACH (hmapx_node, &trk_data->deleted) { > struct ls_stateful_record *ls_stateful_rec = hmapx_node->data; > - const struct ovn_datapath *od = > - ovn_datapaths_find_by_index(lflow_input->ls_datapaths, > - ls_stateful_rec->ls_index); > - ovs_assert(od->nbs && uuid_equals(&od->nbs->header_.uuid, > - &ls_stateful_rec->nbs_uuid)); > - > - lflow_ref_unlink_lflows(ls_stateful_rec->lflow_ref); > + const struct ovn_datapath *od = NULL; > + struct hmapx_node *n; > + HMAPX_FOR_EACH (n, &tracked_ls->deleted) { > + const struct ovn_datapath *iter = n->data; > + if (iter->index == ls_stateful_rec->ls_index) { > + od = iter; > + break; > + } > + } > + if (od) { > + ovs_assert(od->nbs && uuid_equals(&od->nbs->header_.uuid, > + > &ls_stateful_rec->nbs_uuid)); > + if (!lflow_ref_resync_flows( > + ls_stateful_rec->lflow_ref, lflows, ovnsb_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)) { > + return false; > + } > + } > } > > return true; > diff --git a/northd/northd.h b/northd/northd.h > index 32134d36e..2af238eea 100644 > --- a/northd/northd.h > +++ b/northd/northd.h > @@ -473,8 +473,7 @@ struct ovn_datapath { > /* Map of ovn_port objects belonging to this datapath. > * This map doesn't include derived ports. */ > struct hmap ports; > - /* Reference to the lflows belonging to this datapath currently router > - * only lflows. */ > + /* Reference to the lflows belonging to this datapath. */ > struct lflow_ref *datapath_lflows; > }; > > @@ -941,6 +940,10 @@ bool lflow_handle_northd_lr_changes(struct > ovsdb_idl_txn *ovnsh_txn, > struct tracked_dps *, > struct lflow_input *, > struct lflow_table *lflows); > +bool lflow_handle_northd_ls_changes(struct ovsdb_idl_txn *ovnsb_txn, > + struct tracked_dps *, > + struct lflow_input *, > + struct lflow_table *lflows); > bool lflow_handle_northd_port_changes(struct ovsdb_idl_txn *ovnsb_txn, > struct tracked_ovn_ports *, > struct lflow_input *, > @@ -955,6 +958,7 @@ bool lflow_handle_lr_stateful_changes(struct > ovsdb_idl_txn *, > struct lflow_table *lflows); > bool lflow_handle_ls_stateful_changes(struct ovsdb_idl_txn *, > struct ls_stateful_tracked_data *, > + struct tracked_dps *, > struct lflow_input *, > struct lflow_table *lflows); > bool northd_handle_sb_port_binding_changes( > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index a88b71ca8..f40ac11ab 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -2935,6 +2935,9 @@ OVN_FOR_EACH_NORTHD_NO_HV([ > AT_SETUP([check VXLAN mode disabling]) > ovn_start > > +# FIXME > +OVS_CTL_TIMEOUT=120 > With the fix none of those should be needed. > + > # Create a fake chassis with vxlan encap to implicitly enable VXLAN mode. > check_uuid ovn-sbctl \ > --id=@e create encap chassis_name=hv1 ip="192.168.0.1" type="vxlan" \ > @@ -2969,6 +2972,9 @@ OVN_FOR_EACH_NORTHD_NO_HV([ > AT_SETUP([check datapath tunnel ids exhaustion]) > ovn_start > > +# FIXME > +OVS_CTL_TIMEOUT=120 > + > # Create a fake chassis with vxlan encap to lower MAX DP tunnel key to > 2^12 > check_uuid ovn-sbctl \ > --id=@e create encap chassis_name=hv1 ip="192.168.0.1" type="vxlan" \ > @@ -3030,6 +3036,9 @@ OVN_FOR_EACH_NORTHD_NO_HV([ > AT_SETUP([check LS/LR single line configuration]) > ovn_start > > +# FIXME > +OVS_CTL_TIMEOUT=120 > + > cmd="" > for i in {1..2048..1}; do > cmd="${cmd} -- ls-add lsw-${i}" > @@ -3119,11 +3128,12 @@ check_row_count sb:logical_dp_group 1 > echo "$sw2_sb_uuid" >> sw_sb_uuids > > AT_CHECK_UNQUOTED([ovn-sbctl --bare --columns _uuid,datapaths find > Logical_DP_Group dnl > - | grep -A1 $ls_dpg_uuid | tail -1 | tr ' ' '\n' | > sort], [0], [dnl > + | tail -1 | tr ' ' '\n' | sort], [0], [dnl > $(cat sw_sb_uuids | sort) > ]) > > dnl Add two routers and check that they are in their new separate group. > +ls_dpg_uuid=$(ovn-sbctl --bare --columns _uuid list Logical_DP_Group .) > check ovn-nbctl --wait=sb lr-add lr0 -- lr-add lr1 > lr0_sb_uuid=$(fetch_column datapath_binding _uuid external_ids:name=lr0) > lr1_sb_uuid=$(fetch_column datapath_binding _uuid external_ids:name=lr1) > @@ -14558,7 +14568,7 @@ check as northd ovn-appctl -t ovn-northd > inc-engine/clear-stats > check ovn-nbctl --wait=sb ls-add sw0 > check_engine_stats northd norecompute compute > check_engine_stats ls_stateful norecompute compute > -check_engine_stats lflow recompute nocompute > +check_engine_stats lflow norecompute compute > > # For the below engine nodes, en_northd is input. So check > # their engine status. > @@ -14596,7 +14606,7 @@ check as northd ovn-appctl -t ovn-northd > inc-engine/clear-stats > check ovn-nbctl --wait=sb ls-del sw0 > check_engine_stats northd norecompute compute > check_engine_stats ls_stateful norecompute compute > -check_engine_stats lflow recompute nocompute > +check_engine_stats lflow norecompute compute > > # For the below engine nodes, en_northd is input. So check > # their engine status. > -- > 2.51.1 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > [0] https://patchwork.ozlabs.org/project/ovn/patch/[email protected]/ Regards, Ales _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
