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,
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?
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
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
