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