On Sat, Jul 1, 2023 at 7:33 AM Han Zhou <[email protected]> wrote: > > On Thu, Jun 29, 2023 at 10:54 PM Numan Siddique <[email protected]> wrote: > > > > On Sun, Jun 18, 2023 at 11:48 AM Han Zhou <[email protected]> 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 <[email protected]> > > > --- > > > 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) > > > > Thanks Numan for reviewing! > > Thanks for your suggestion for refactor. However, I have a different > opinion here. Syncing multicast group is one of the differences between the > two branches, yet another difference is, for update, we need to delete the > existing lflows. > Yes, we can still combine them with more conditional checks, but I don't > see an obvious benefit except it may save us a few lines of code. > Similarly, I think that having two lists gives us a clear view of what's > new and what's updated. We know that it needs very careful handling for > I-P, and I feel it kind of help to reduce the burden of our brains when > having a clear separation. > > > 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. > > > Sure, I will add some basic checks to make sure the changes are handled, > but probably not a complete validation of all the expected flows, because > that should be covered by the feature tests. Does this make sense?
For a normal vif lport with no dhcp options etc it would result in only 2 or 3 flows per lport. A couple in logical switch pipeline and one in router pipleine (lr_in_arp_resolve). I'd suggest at least check for these. Numan > > > With the test cases added > > Acked-by: Numan Siddique [email protected]> > > > Thank you! > Han > > > It would be great if you can refactor the added/updated lport code. > > > > Thanks > > Numan > > > > > -- > > > 2.30.2 > > > > > > _______________________________________________ > > > dev mailing list > > > [email protected] > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
