On Sun, Jun 18, 2023 at 11:48 AM Han Zhou <hz...@ovn.org> wrote: > > 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> > --- > northd/northd.c | 231 +++++++++++++++++++++++++++++--------------- > tests/ovn-northd.at | 4 +- > 2 files changed, 154 insertions(+), 81 deletions(-) > > diff --git a/northd/northd.c b/northd/northd.c > index aa0f853ce2db..c0c948fcea4b 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -16303,6 +16303,113 @@ 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; > + 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, > @@ -16310,13 +16417,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); > @@ -16328,6 +16428,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); > + } > + } > +
The code to handle added_ports and updated_ports seems to be the same except for updating the multicast group for the new port. I think this can be refactored. Also I don't really see the node having two separate lists in ls_change. Perhaps just one list - added_updated_lports (or a better name) and the internal struct will indicate if it's new or updated. Like how we do in controller/local_lport.h (struct tracked_lport) But I'm fine including these refactoring when I submit my patches. > LIST_FOR_EACH (op, list, &ls_change->added_ports) { > struct ds match = DS_EMPTY_INITIALIZER; > struct ds actions = DS_EMPTY_INITIALIZER; > @@ -16338,6 +16479,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); > @@ -16364,78 +16507,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; > - 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 ada2d2a4aa5e..d4a781bea700 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 || break > + check_recompute_counter 0 0 || break > > 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 || break > + check_recompute_counter 0 0 || break > > # No change, no recompute > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats I think it's better to add test cases or enhance this one to check for the expected lflows when a lport is added/deleted/claimed/released. With the test cases added Acked-by: Numan Siddique num...@ovn.org> It would be great if you can refactor the added/updated lport code. Thanks Numan > -- > 2.30.2 > > _______________________________________________ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev