On Wed, Nov 26, 2025 at 9:16 AM Dumitru Ceara <[email protected]> wrote:
> On 11/25/25 6:11 PM, Ales Musil wrote: > > Do a partial revert of the I-P handling for LR addition and deletion. > > This causes issues with DP groups processing, when there new flows > > added northd is still holding multiple references to the existing DP > > group. In that case we cannot resize it, because we don't know if the > > DP group will split. This might lead to DP groups being deleted and > > recreated as we slowly remove the references and at the end all flows > > migrated to the new DP group leaving the old one empty. This is > > particularly bad for SB performance as we are hammering it with DP > > group creation/deletion + updates of DP group references in all > > affected flows. > > > > The lflows_ref is intentionally left NULL in the ovn_datapath struct > > so we don't have to revert all those changes to pass it along, and > > so we don't waste any space to maintain them as they are not used > > by anything right now. > > > > This reverts commit 9ec96d0d85b66a6d2c9e66da694c9cfe710bdf46. > > This reverts commit cf92fb76ed487862054b8d10c89a46f5fb796072. > > > > Signed-off-by: Ales Musil <[email protected]> > > --- > > Hi Ales, > Hi Dumitru, thank you for the review. > Thanks for the patch. The change looks good to me but should we also > add a test that ensures the datapath groups don't get recreated when a > logical router is added? > Yes that makes sense, I'll extend the test that we have for DPG over LS. > Regards, > Dumitru > > > northd/en-lflow.c | 11 +++---- > > northd/northd.c | 71 --------------------------------------------- > > northd/northd.h | 7 +++-- > > tests/ovn-northd.at | 5 ++-- > > 4 files changed, 12 insertions(+), 82 deletions(-) > > > > diff --git a/northd/en-lflow.c b/northd/en-lflow.c > > index 30bf7e35c..fcccc6265 100644 > > --- a/northd/en-lflow.c > > +++ b/northd/en-lflow.c > > @@ -149,19 +149,16 @@ lflow_northd_handler(struct engine_node *node, > > return EN_UNHANDLED; > > } > > > > + if (northd_has_lrouters_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; > > > > struct lflow_input lflow_input; > > lflow_get_input_data(node, &lflow_input); > > > > - if (!lflow_handle_northd_lr_changes(eng_ctx->ovnsb_idl_txn, > > - > &northd_data->trk_data.trk_routers, > > - &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, > > diff --git a/northd/northd.c b/northd/northd.c > > index 011f449ec..eb85a79cb 100644 > > --- a/northd/northd.c > > +++ b/northd/northd.c > > @@ -556,7 +556,6 @@ ovn_datapath_create(struct hmap *datapaths, const > struct uuid *key, > > od->ipam_info_initialized = false; > > od->tunnel_key = sdp->sb_dp->tunnel_key; > > init_mcast_info_for_datapath(od); > > - od->datapath_lflows = lflow_ref_create(); > > return od; > > } > > > > @@ -585,7 +584,6 @@ ovn_datapath_destroy(struct ovn_datapath *od) > > destroy_mcast_info_for_datapath(od); > > destroy_ports_for_datapath(od); > > sset_destroy(&od->router_ips); > > - lflow_ref_destroy(od->datapath_lflows); > > free(od); > > } > > } > > @@ -19256,7 +19254,6 @@ lflow_reset_northd_refs(struct lflow_input > *lflow_input) > > struct ls_stateful_record *ls_stateful_rec; > > struct ovn_lb_datapaths *lb_dps; > > struct ovn_port *op; > > - const struct ovn_datapath *od; > > > > LR_STATEFUL_TABLE_FOR_EACH (lr_stateful_rec, > > lflow_input->lr_stateful_table) { > > @@ -19281,74 +19278,6 @@ lflow_reset_northd_refs(struct lflow_input > *lflow_input) > > HMAP_FOR_EACH (lb_dps, hmap_node, lflow_input->lb_datapaths_map) { > > lflow_ref_clear(lb_dps->lflow_ref); > > } > > - > > - HMAP_FOR_EACH (od, key_node, &lflow_input->lr_datapaths->datapaths) > { > > - lflow_ref_clear(od->datapath_lflows); > > - } > > - > > - HMAP_FOR_EACH (od, key_node, &lflow_input->ls_datapaths->datapaths) > { > > - lflow_ref_clear(od->datapath_lflows); > > - } > > -} > > - > > -bool > > -lflow_handle_northd_lr_changes(struct ovsdb_idl_txn *ovnsb_txn, > > - struct tracked_dps *tracked_lrs, > > - struct lflow_input *lflow_input, > > - struct lflow_table *lflows) > > -{ > > - bool handled = true; > > - struct hmapx_node *hmapx_node; > > - HMAPX_FOR_EACH (hmapx_node, &tracked_lrs->deleted) { > > - struct ovn_datapath *od = hmapx_node->data; > > - handled = 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); > > - if (!handled) { > > - return handled; > > - } > > - } > > - > > - struct lswitch_flow_build_info lsi = { > > - .lr_datapaths = lflow_input->lr_datapaths, > > - .lr_stateful_table = lflow_input->lr_stateful_table, > > - .lflows =lflows, > > - .route_data = lflow_input->route_data, > > - .route_tables = lflow_input->route_tables, > > - .route_policies = lflow_input->route_policies, > > - .match = DS_EMPTY_INITIALIZER, > > - .actions = DS_EMPTY_INITIALIZER, > > - }; > > - HMAPX_FOR_EACH (hmapx_node, &tracked_lrs->crupdated) { > > - struct ovn_datapath *od = hmapx_node->data; > > - > > - lflow_ref_unlink_lflows(od->datapath_lflows); > > - build_lswitch_and_lrouter_iterate_by_lr(od, &lsi); > > - } > > - > > - /* We need to make sure that all datapath groups are allocated > before > > - * trying to sync logical flows. Otherwise, we would need to > recompute > > - * those datapath groups within those flows over and over again. */ > > - HMAPX_FOR_EACH (hmapx_node, &tracked_lrs->crupdated) { > > - struct ovn_datapath *od = hmapx_node->data; > > - > > - 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); > > - if (!handled) { > > - break; > > - } > > - } > > - > > - ds_destroy(&lsi.actions); > > - ds_destroy(&lsi.match); > > - return handled; > > } > > > > bool > > diff --git a/northd/northd.h b/northd/northd.h > > index 32134d36e..40d845e4f 100644 > > --- a/northd/northd.h > > +++ b/northd/northd.h > > @@ -473,8 +473,11 @@ 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. */ > > + > > + /* XXX Reference to the lflows belonging to this datapath currently > router > > + * only lflows. This is NULL for now just to not waste any space > > + * as it's not used by anything right now, but it wasn't worth > reverting > > + * all the related changes. */ > > struct lflow_ref *datapath_lflows; > > }; > > > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > > index f7ec6a33a..7b1779569 100644 > > --- a/tests/ovn-northd.at > > +++ b/tests/ovn-northd.at > > @@ -12627,17 +12627,18 @@ check_engine_stats lr_nat norecompute compute > > check_engine_stats lr_stateful norecompute compute > > check_engine_stats sync_to_sb_pb norecompute compute > > check_engine_stats sync_to_sb_lb norecompute compute > > -check_engine_stats lflow norecompute compute > > +check_engine_stats lflow recompute nocompute > > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > > > check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats > > check ovn-nbctl --wait=sb lr-del lr0 > > + > > check_engine_stats northd norecompute compute > > check_engine_stats lr_nat norecompute compute > > check_engine_stats lr_stateful norecompute compute > > check_engine_stats sync_to_sb_pb norecompute compute > > check_engine_stats sync_to_sb_lb norecompute compute > > -check_engine_stats lflow norecompute compute > > +check_engine_stats lflow recompute nocompute > > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > > > check ovn-nbctl --wait=sb lr-add lr0 > > Regards, Ales _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
