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
