| Message ID | 20230618061755.3962521-4-hzhou@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | ovn-northd incremental processing for VIF udpates and deletions end-to-end. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | fail | github build: failed |
On Sun, Jun 18, 2023 at 11:48 AM Han Zhou <hzhou@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 <hzhou@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 numans@ovn.org> It would be great if you can refactor the added/updated lport code. Thanks Numan > -- > 2.30.2 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Thu, Jun 29, 2023 at 10:54 PM Numan Siddique <numans@ovn.org> wrote: > > On Sun, Jun 18, 2023 at 11:48 AM Han Zhou <hzhou@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 <hzhou@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) > 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? > With the test cases added > Acked-by: Numan Siddique numans@ovn.org> > Thank you! Han > It would be great if you can refactor the added/updated lport code. > > Thanks > Numan > > > -- > > 2.30.2 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
On Sat, Jul 1, 2023 at 7:33 AM Han Zhou <hzhou@ovn.org> wrote: > > On Thu, Jun 29, 2023 at 10:54 PM Numan Siddique <numans@ovn.org> wrote: > > > > On Sun, Jun 18, 2023 at 11:48 AM Han Zhou <hzhou@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 <hzhou@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) > > > > 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 numans@ovn.org> > > > Thank you! > Han > > > It would be great if you can refactor the added/updated lport code. > > > > Thanks > > Numan > > > > > -- > > > 2.30.2 > > > > > > _______________________________________________ > > > dev mailing list > > > dev@openvswitch.org > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On Fri, Jun 30, 2023 at 10:27 PM Numan Siddique <numans@ovn.org> wrote: > > On Sat, Jul 1, 2023 at 7:33 AM Han Zhou <hzhou@ovn.org> wrote: > > > > On Thu, Jun 29, 2023 at 10:54 PM Numan Siddique <numans@ovn.org> wrote: > > > > > > On Sun, Jun 18, 2023 at 11:48 AM Han Zhou <hzhou@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 <hzhou@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) > > > > > > > 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 > Hi Numan, I used a different but more generic and effective way IMHO for the test. Since it covers the other I-P test case so I made it a separate patch, and sent a new series, together with a followup patch that avoids recompute when there are in-flight transactions. PTAL: https://patchwork.ozlabs.org/project/ovn/list/?series=362671 Thanks, Han > > > > > With the test cases added > > > Acked-by: Numan Siddique numans@ovn.org> > > > > > Thank you! > > Han > > > > > It would be great if you can refactor the added/updated lport code. > > > > > > Thanks > > > Numan > > > > > > > -- > > > > 2.30.2 > > > > > > > > _______________________________________________ > > > > dev mailing list > > > > dev@openvswitch.org > > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
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); + } + } + 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
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 <hzhou@ovn.org> --- northd/northd.c | 231 +++++++++++++++++++++++++++++--------------- tests/ovn-northd.at | 4 +- 2 files changed, 154 insertions(+), 81 deletions(-)