This patch achieves zero recompute for VIF updates and deletions in common scenarios. The performance gain for these scenarios is similar to the patch "northd: Incremental processing of VIF additions in 'lflow' node."
Signed-off-by: Han Zhou <hz...@ovn.org> Acked-by: Numan Siddique <num...@ovn.org> --- northd/northd.c | 235 +++++++++++++++++++++++++++++--------------- tests/ovn-northd.at | 4 +- 2 files changed, 156 insertions(+), 83 deletions(-) diff --git a/northd/northd.c b/northd/northd.c index 9df1a49cbfce..b9605862e498 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -16308,6 +16308,115 @@ void build_lflows(struct ovsdb_idl_txn *ovnsb_txn, hmap_destroy(&mcast_groups); } +static void +sync_lsp_lflows_to_sb(struct ovsdb_idl_txn *ovnsb_txn, + struct lflow_input *lflow_input, + struct hmap *lflows, + struct ovn_lflow *lflow) +{ + size_t n_datapaths; + struct ovn_datapath **datapaths_array; + if (ovn_stage_to_datapath_type(lflow->stage) == DP_SWITCH) { + n_datapaths = ods_size(lflow_input->ls_datapaths); + datapaths_array = lflow_input->ls_datapaths->array; + } else { + n_datapaths = ods_size(lflow_input->lr_datapaths); + datapaths_array = lflow_input->lr_datapaths->array; + } + uint32_t n_ods = bitmap_count1(lflow->dpg_bitmap, n_datapaths); + ovs_assert(n_ods == 1); + /* There is only one datapath, so it should be moved out of the + * group to a single 'od'. */ + size_t index = bitmap_scan(lflow->dpg_bitmap, true, 0, + n_datapaths); + + bitmap_set0(lflow->dpg_bitmap, index); + lflow->od = datapaths_array[index]; + + /* Logical flow should be re-hashed to allow lookups. */ + uint32_t hash = hmap_node_hash(&lflow->hmap_node); + /* Remove from lflows. */ + hmap_remove(lflows, &lflow->hmap_node); + hash = ovn_logical_flow_hash_datapath(&lflow->od->sb->header_.uuid, + hash); + /* Add back. */ + hmap_insert(lflows, &lflow->hmap_node, hash); + + /* Sync to SB. */ + const struct sbrec_logical_flow *sbflow; + /* Note: uuid_random acquires a global mutex. If we parallelize the sync to + * SB this may become a bottleneck. */ + lflow->sb_uuid = uuid_random(); + sbflow = sbrec_logical_flow_insert_persist_uuid(ovnsb_txn, + &lflow->sb_uuid); + const char *pipeline = ovn_stage_get_pipeline_name(lflow->stage); + uint8_t table = ovn_stage_get_table(lflow->stage); + sbrec_logical_flow_set_logical_datapath(sbflow, lflow->od->sb); + sbrec_logical_flow_set_logical_dp_group(sbflow, NULL); + sbrec_logical_flow_set_pipeline(sbflow, pipeline); + sbrec_logical_flow_set_table_id(sbflow, table); + sbrec_logical_flow_set_priority(sbflow, lflow->priority); + sbrec_logical_flow_set_match(sbflow, lflow->match); + sbrec_logical_flow_set_actions(sbflow, lflow->actions); + if (lflow->io_port) { + struct smap tags = SMAP_INITIALIZER(&tags); + smap_add(&tags, "in_out_port", lflow->io_port); + sbrec_logical_flow_set_tags(sbflow, &tags); + smap_destroy(&tags); + } + sbrec_logical_flow_set_controller_meter(sbflow, lflow->ctrl_meter); + /* Trim the source locator lflow->where, which looks something like + * "ovn/northd/northd.c:1234", down to just the part following the + * last slash, e.g. "northd.c:1234". */ + const char *slash = strrchr(lflow->where, '/'); +#if _WIN32 + const char *backslash = strrchr(lflow->where, '\\'); + if (!slash || backslash > slash) { + slash = backslash; + } +#endif + const char *where = slash ? slash + 1 : lflow->where; + + struct smap ids = SMAP_INITIALIZER(&ids); + smap_add(&ids, "stage-name", ovn_stage_to_str(lflow->stage)); + smap_add(&ids, "source", where); + if (lflow->stage_hint) { + smap_add(&ids, "stage-hint", lflow->stage_hint); + } + sbrec_logical_flow_set_external_ids(sbflow, &ids); + smap_destroy(&ids); +} + +static bool +delete_lflow_for_lsp(struct ovn_port *op, bool is_update, + const struct sbrec_logical_flow_table *sb_lflow_table, + struct hmap *lflows) +{ + struct lflow_ref_node *lfrn; + const char *operation = is_update ? "updated" : "deleted"; + LIST_FOR_EACH_SAFE (lfrn, lflow_list_node, &op->lflows) { + VLOG_DBG("Deleting SB lflow "UUID_FMT" for %s port %s", + UUID_ARGS(&lfrn->lflow->sb_uuid), operation, op->key); + + const struct sbrec_logical_flow *sblflow = + sbrec_logical_flow_table_get_for_uuid(sb_lflow_table, + &lfrn->lflow->sb_uuid); + if (sblflow) { + sbrec_logical_flow_delete(sblflow); + } else { + static struct vlog_rate_limit rl = + VLOG_RATE_LIMIT_INIT(1, 1); + VLOG_WARN_RL(&rl, "SB lflow "UUID_FMT" not found when handling " + "%s port %s. Recompute.", + UUID_ARGS(&lfrn->lflow->sb_uuid), operation, op->key); + return false; + } + + ovn_lflow_destroy(lflows, lfrn->lflow); + } + return true; +} + bool lflow_handle_northd_ls_changes(struct ovsdb_idl_txn *ovnsb_txn, struct tracked_ls_changes *ls_changes, struct lflow_input *lflow_input, @@ -16315,13 +16424,6 @@ bool lflow_handle_northd_ls_changes(struct ovsdb_idl_txn *ovnsb_txn, { struct ls_change *ls_change; LIST_FOR_EACH (ls_change, list_node, &ls_changes->updated) { - if (!ovs_list_is_empty(&ls_change->updated_ports) || - !ovs_list_is_empty(&ls_change->deleted_ports)) { - /* XXX: implement lflow index so that we can handle updated and - * deleted LSPs incrementally. */ - return false; - } - const struct sbrec_multicast_group *sbmc_flood = mcast_group_lookup(lflow_input->sbrec_mcast_group_by_name_dp, MC_FLOOD, ls_change->od->sb); @@ -16333,6 +16435,47 @@ bool lflow_handle_northd_ls_changes(struct ovsdb_idl_txn *ovnsb_txn, MC_UNKNOWN, ls_change->od->sb); struct ovn_port *op; + LIST_FOR_EACH (op, list, &ls_change->deleted_ports) { + if (!delete_lflow_for_lsp(op, false, + lflow_input->sbrec_logical_flow_table, + lflows)) { + return false; + } + + /* No need to update SB multicast groups, thanks to weak + * references. */ + } + + LIST_FOR_EACH (op, list, &ls_change->updated_ports) { + /* Delete old lflows. */ + if (!delete_lflow_for_lsp(op, true, + lflow_input->sbrec_logical_flow_table, + lflows)) { + return false; + } + + /* Generate new lflows. */ + struct ds match = DS_EMPTY_INITIALIZER; + struct ds actions = DS_EMPTY_INITIALIZER; + build_lswitch_and_lrouter_iterate_by_lsp(op, lflow_input->ls_ports, + lflow_input->lr_ports, + lflow_input->meter_groups, + &match, &actions, + lflows); + ds_destroy(&match); + ds_destroy(&actions); + + /* SB port_binding is not deleted, so don't update SB multicast + * groups. */ + + /* Sync the new flows to SB. */ + struct lflow_ref_node *lfrn; + LIST_FOR_EACH (lfrn, lflow_list_node, &op->lflows) { + sync_lsp_lflows_to_sb(ovnsb_txn, lflow_input, lflows, + lfrn->lflow); + } + } + LIST_FOR_EACH (op, list, &ls_change->added_ports) { struct ds match = DS_EMPTY_INITIALIZER; struct ds actions = DS_EMPTY_INITIALIZER; @@ -16343,6 +16486,8 @@ bool lflow_handle_northd_ls_changes(struct ovsdb_idl_txn *ovnsb_txn, lflows); ds_destroy(&match); ds_destroy(&actions); + + /* Update SB multicast groups for the new port. */ if (!sbmc_flood) { sbmc_flood = create_sb_multicast_group(ovnsb_txn, ls_change->od->sb, MC_FLOOD, OVN_MCAST_FLOOD_TUNNEL_KEY); @@ -16369,80 +16514,8 @@ bool lflow_handle_northd_ls_changes(struct ovsdb_idl_txn *ovnsb_txn, /* Sync the newly added flows to SB. */ struct lflow_ref_node *lfrn; LIST_FOR_EACH (lfrn, lflow_list_node, &op->lflows) { - struct ovn_lflow *lflow = lfrn->lflow; - size_t n_datapaths; - struct ovn_datapath **datapaths_array; - if (ovn_stage_to_datapath_type(lflow->stage) == DP_SWITCH) { - n_datapaths = ods_size(lflow_input->ls_datapaths); - datapaths_array = lflow_input->ls_datapaths->array; - } else { - n_datapaths = ods_size(lflow_input->lr_datapaths); - datapaths_array = lflow_input->lr_datapaths->array; - } - uint32_t n_ods = bitmap_count1(lflow->dpg_bitmap, n_datapaths); - ovs_assert(n_ods == 1); - /* There is only one datapath, so it should be moved out of the - * group to a single 'od'. */ - size_t index = bitmap_scan(lflow->dpg_bitmap, true, 0, - n_datapaths); - - bitmap_set0(lflow->dpg_bitmap, index); - lflow->od = datapaths_array[index]; - - /* Logical flow should be re-hashed to allow lookups. */ - uint32_t hash = hmap_node_hash(&lflow->hmap_node); - /* Remove from lflows. */ - hmap_remove(lflows, &lflow->hmap_node); - hash = ovn_logical_flow_hash_datapath( - &lflow->od->sb->header_.uuid, hash); - /* Add back. */ - hmap_insert(lflows, &lflow->hmap_node, hash); - - /* Sync to SB. */ - const struct sbrec_logical_flow *sbflow; - /* Note: uuid_random acquires a global mutex. If we parallelize - * the sync to SB this may become a bottleneck. */ - lflow->sb_uuid = uuid_random(); - sbflow = sbrec_logical_flow_insert_persist_uuid( - ovnsb_txn, &lflow->sb_uuid); - const char *pipeline = ovn_stage_get_pipeline_name( - lflow->stage); - uint8_t table = ovn_stage_get_table(lflow->stage); - sbrec_logical_flow_set_logical_datapath(sbflow, lflow->od->sb); - sbrec_logical_flow_set_logical_dp_group(sbflow, NULL); - sbrec_logical_flow_set_pipeline(sbflow, pipeline); - sbrec_logical_flow_set_table_id(sbflow, table); - sbrec_logical_flow_set_priority(sbflow, lflow->priority); - sbrec_logical_flow_set_match(sbflow, lflow->match); - sbrec_logical_flow_set_actions(sbflow, lflow->actions); - if (lflow->io_port) { - struct smap tags = SMAP_INITIALIZER(&tags); - smap_add(&tags, "in_out_port", lflow->io_port); - sbrec_logical_flow_set_tags(sbflow, &tags); - smap_destroy(&tags); - } - sbrec_logical_flow_set_controller_meter(sbflow, - lflow->ctrl_meter); - /* Trim the source locator lflow->where, which looks something - * like "ovn/northd/northd.c:1234", down to just the part - * following the last slash, e.g. "northd.c:1234". */ - const char *slash = strrchr(lflow->where, '/'); -#if _WIN32 - const char *backslash = strrchr(lflow->where, '\\'); - if (!slash || backslash > slash) { - slash = backslash; - } -#endif - const char *where = slash ? slash + 1 : lflow->where; - - struct smap ids = SMAP_INITIALIZER(&ids); - smap_add(&ids, "stage-name", ovn_stage_to_str(lflow->stage)); - smap_add(&ids, "source", where); - if (lflow->stage_hint) { - smap_add(&ids, "stage-hint", lflow->stage_hint); - } - sbrec_logical_flow_set_external_ids(sbflow, &ids); - smap_destroy(&ids); + sync_lsp_lflows_to_sb(ovnsb_txn, lflow_input, lflows, + lfrn->lflow); } } } diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index 5d871b2873b6..36f3b91a54c1 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -9540,11 +9540,11 @@ for i in $(seq 10); do check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats check ovn-nbctl --wait=hv lsp-del lsp${i}-1 - check_recompute_counter 0 1 || continue + check_recompute_counter 0 0 || continue check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats check ovn-nbctl --wait=hv lsp-set-addresses lsp${i}-2 "aa:aa:aa:00:00:88 192.168.0.88" - check_recompute_counter 0 1 || continue + check_recompute_counter 0 0 || continue # No change, no recompute check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats -- 2.30.2 _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev