From patchwork Thu Jan 20 18:06:53 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Han Zhou X-Patchwork-Id: 1582283 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=openvswitch.org (client-ip=140.211.166.137; helo=smtp4.osuosl.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Received: from smtp4.osuosl.org (smtp4.osuosl.org [140.211.166.137]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by bilbo.ozlabs.org (Postfix) with ESMTPS id 4Jfr7l6630z9t3b for ; Fri, 21 Jan 2022 05:07:15 +1100 (AEDT) Received: from localhost (localhost [127.0.0.1]) by smtp4.osuosl.org (Postfix) with ESMTP id 63E9D4016F; Thu, 20 Jan 2022 18:07:13 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from smtp4.osuosl.org ([127.0.0.1]) by localhost (smtp4.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id ZgDnoZyaBfEI; Thu, 20 Jan 2022 18:07:12 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [IPv6:2605:bc80:3010:104::8cd3:938]) by smtp4.osuosl.org (Postfix) with ESMTPS id 2EB69401DD; Thu, 20 Jan 2022 18:07:11 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id 029B5C0039; Thu, 20 Jan 2022 18:07:11 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@lists.linuxfoundation.org Received: from smtp1.osuosl.org (smtp1.osuosl.org [140.211.166.138]) by lists.linuxfoundation.org (Postfix) with ESMTP id 8C73FC002F for ; Thu, 20 Jan 2022 18:07:09 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by smtp1.osuosl.org (Postfix) with ESMTP id 875B280C50 for ; Thu, 20 Jan 2022 18:07:09 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from smtp1.osuosl.org ([127.0.0.1]) by localhost (smtp1.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 334iFhp4Fax7 for ; Thu, 20 Jan 2022 18:07:08 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.8.0 Received: from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net [217.70.183.194]) by smtp1.osuosl.org (Postfix) with ESMTPS id DAD6680C13 for ; Thu, 20 Jan 2022 18:07:07 +0000 (UTC) Received: (Authenticated sender: hzhou@ovn.org) by mail.gandi.net (Postfix) with ESMTPSA id 5509E40008; Thu, 20 Jan 2022 18:07:01 +0000 (UTC) From: Han Zhou To: dev@openvswitch.org Date: Thu, 20 Jan 2022 10:06:53 -0800 Message-Id: <20220120180653.314866-1-hzhou@ovn.org> X-Mailer: git-send-email 2.30.2 MIME-Version: 1.0 Subject: [ovs-dev] [PATCH ovn] ovn-controller: Avoid reprocessing same lflows in the same I-P run. X-BeenThere: ovs-dev@openvswitch.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: ovs-dev-bounces@openvswitch.org Sender: "dev" For I-P node lflow_output, different change handlers can trigger reprocessing lflows. However, it is a waste to reprocess the same lflow multiple times in the same run, because each processing of the lflow is against the same input data. For example, if a lflow references an addresset A and a port-group P, it is possible that both A and P are changed in the same run, the same lflow reprocess would be triggered by both lflow_output_addr_sets_handler() and lflow_output_port_groups_handler(). Another example may incur a even bigger waste is that when a NB port-group include ports across a big number of datapaths, so the lflow is referencing a big number of SB port-groups with DP group including all those DPs. When there are multiple port changes in the NB port-group, there can be multiple small SB port-group changes, and so the lflows can be reprocessed multiple times. This patch avoid reprocessing the same lflow in the same I-P run, which should improve performance in above scenarios. There is only one exception when a lflow may still be reprocessed more than once: if a lflow A is processed, which results in a compound conjunction added to an existed flow generated by an exited lflow B, and then lflow B needs to be reprocessed in the same run, which would cause flood-remove and reprocess lflow A. In this case lflow A is processed twice. Apart from the performance gains, there is also a potential problem of DP group fixed by this patch. If there is addrset/pg change and at the same run there is also a new local datapath monitored, then the existed code would firstly handle the addrset/pg changes causing a lflow being processed against the DP group including the new DP, which could have conjunction flows, but later the same lflow is reprocessed by lflow_output_runtime_data_handler()->lflow_add_flows_for_datapath() for the new DP only. Because lflows adding conjunction flows will not be checked against redundancy but only tries to combine the conjunction actions, it would result in redundanct conjunction actions added to the same flow, which is also linked to the same SB lflow twice. The mechanism of this patch will avoid this problem. Signed-off-by: Han Zhou Acked-by: Mark Michelson --- controller/lflow.c | 143 +++++++++++++++++++++++++++++++----- controller/lflow.h | 7 ++ controller/ovn-controller.c | 18 ++++- 3 files changed, 147 insertions(+), 21 deletions(-) diff --git a/controller/lflow.c b/controller/lflow.c index 933e2f3cc..b976c7d56 100644 --- a/controller/lflow.c +++ b/controller/lflow.c @@ -78,8 +78,16 @@ consider_logical_flow(const struct sbrec_logical_flow *lflow, struct hmap *dhcp_opts, struct hmap *dhcpv6_opts, struct hmap *nd_ra_opts, struct controller_event_options *controller_event_opts, + bool is_recompute, struct lflow_ctx_in *l_ctx_in, struct lflow_ctx_out *l_ctx_out); +static struct lflow_processed_node * +lflows_processed_find(struct hmap *lflows_processed, + const struct uuid *lflow_uuid); +static void lflows_processed_add(struct hmap *lflows_processed, + const struct uuid *lflow_uuid); +static void lflows_processed_remove(struct hmap *lflows_processed, + struct lflow_processed_node *node); static void lflow_resource_add(struct lflow_resource_ref *, enum ref_type, const char *ref_name, const struct uuid *); static struct ref_lflow_node *ref_lflow_lookup(struct hmap *ref_lflow_table, @@ -366,7 +374,7 @@ add_logical_flows(struct lflow_ctx_in *l_ctx_in, SBREC_LOGICAL_FLOW_TABLE_FOR_EACH (lflow, l_ctx_in->logical_flow_table) { consider_logical_flow(lflow, &dhcp_opts, &dhcpv6_opts, - &nd_ra_opts, &controller_event_opts, + &nd_ra_opts, &controller_event_opts, true, l_ctx_in, l_ctx_out); } @@ -413,6 +421,12 @@ lflow_handle_changed_flows(struct lflow_ctx_in *l_ctx_in, struct ofctrl_flood_remove_node *ofrn, *next; SBREC_LOGICAL_FLOW_TABLE_FOR_EACH_TRACKED (lflow, l_ctx_in->logical_flow_table) { + if (lflows_processed_find(l_ctx_out->lflows_processed, + &lflow->header_.uuid)) { + VLOG_DBG("lflow "UUID_FMT"has been processed, skip.", + UUID_ARGS(&lflow->header_.uuid)); + continue; + } VLOG_DBG("delete lflow "UUID_FMT, UUID_ARGS(&lflow->header_.uuid)); ofrn = xmalloc(sizeof *ofrn); ofrn->sb_uuid = lflow->header_.uuid; @@ -437,8 +451,20 @@ lflow_handle_changed_flows(struct lflow_ctx_in *l_ctx_in, if (lflow) { VLOG_DBG("re-add lflow "UUID_FMT, UUID_ARGS(&lflow->header_.uuid)); + + /* For the extra lflows that need to be reprocessed because of the + * flood remove, remove it from lflows_processed. */ + struct lflow_processed_node *lfp_node = + lflows_processed_find(l_ctx_out->lflows_processed, + &lflow->header_.uuid); + if (lfp_node) { + VLOG_DBG("lflow "UUID_FMT"has been processed, now reprocess.", + UUID_ARGS(&lflow->header_.uuid)); + lflows_processed_remove(l_ctx_out->lflows_processed, lfp_node); + } + consider_logical_flow(lflow, &dhcp_opts, &dhcpv6_opts, - &nd_ra_opts, &controller_event_opts, + &nd_ra_opts, &controller_event_opts, false, l_ctx_in, l_ctx_out); } } @@ -473,15 +499,22 @@ lflow_handle_changed_ref(enum ref_type ref_type, const char *ref_name, *changed = false; bool ret = true; - hmap_remove(&l_ctx_out->lfrr->ref_lflow_table, &rlfn->node); + struct ovs_list lflows_todo = OVS_LIST_INITIALIZER(&lflows_todo); - struct lflow_ref_list_node *lrln, *next; - /* Detach the rlfn->lflow_uuids nodes from the lfrr table and clean - * up all other nodes related to the lflows that uses the resource, - * so that the old nodes won't interfere with updating the lfrr table - * when reparsing the lflows. */ + struct lflow_ref_list_node *lrln, *lrln_uuid, *lrln_uuid_next; HMAP_FOR_EACH (lrln, hmap_node, &rlfn->lflow_uuids) { - ovs_list_remove(&lrln->list_node); + if (lflows_processed_find(l_ctx_out->lflows_processed, + &lrln->lflow_uuid)) { + continue; + } + /* Use lflow_ref_list_node as list node to store the uuid. + * Other fields are not used here. */ + lrln_uuid = xmalloc(sizeof *lrln_uuid); + lrln_uuid->lflow_uuid = lrln->lflow_uuid; + ovs_list_push_back(&lflows_todo, &lrln_uuid->list_node); + } + if (ovs_list_is_empty(&lflows_todo)) { + return true; } struct hmap dhcp_opts = HMAP_INITIALIZER(&dhcp_opts); @@ -509,17 +542,19 @@ lflow_handle_changed_ref(enum ref_type ref_type, const char *ref_name, /* Re-parse the related lflows. */ /* Firstly, flood remove the flows from desired flow table. */ struct hmap flood_remove_nodes = HMAP_INITIALIZER(&flood_remove_nodes); - struct ofctrl_flood_remove_node *ofrn, *ofrn_next; - HMAP_FOR_EACH (lrln, hmap_node, &rlfn->lflow_uuids) { + LIST_FOR_EACH_SAFE (lrln_uuid, lrln_uuid_next, list_node, &lflows_todo) { VLOG_DBG("Reprocess lflow "UUID_FMT" for resource type: %d," " name: %s.", - UUID_ARGS(&lrln->lflow_uuid), + UUID_ARGS(&lrln_uuid->lflow_uuid), ref_type, ref_name); - ofctrl_flood_remove_add_node(&flood_remove_nodes, &lrln->lflow_uuid); + ofctrl_flood_remove_add_node(&flood_remove_nodes, + &lrln_uuid->lflow_uuid); + free(lrln_uuid); } ofctrl_flood_remove_flows(l_ctx_out->flow_table, &flood_remove_nodes); /* Secondly, for each lflow that is actually removed, reprocessing it. */ + struct ofctrl_flood_remove_node *ofrn, *ofrn_next; HMAP_FOR_EACH (ofrn, hmap_node, &flood_remove_nodes) { lflow_resource_destroy_lflow(l_ctx_out->lfrr, &ofrn->sb_uuid); lflow_conj_ids_free(l_ctx_out->conj_ids, &ofrn->sb_uuid); @@ -535,8 +570,19 @@ lflow_handle_changed_ref(enum ref_type ref_type, const char *ref_name, continue; } + /* For the extra lflows that need to be reprocessed because of the + * flood remove, remove it from lflows_processed. */ + struct lflow_processed_node *lfp_node = + lflows_processed_find(l_ctx_out->lflows_processed, + &lflow->header_.uuid); + if (lfp_node) { + VLOG_DBG("lflow "UUID_FMT"has been processed, now reprocess.", + UUID_ARGS(&lflow->header_.uuid)); + lflows_processed_remove(l_ctx_out->lflows_processed, lfp_node); + } + consider_logical_flow(lflow, &dhcp_opts, &dhcpv6_opts, - &nd_ra_opts, &controller_event_opts, + &nd_ra_opts, &controller_event_opts, false, l_ctx_in, l_ctx_out); *changed = true; } @@ -546,12 +592,6 @@ lflow_handle_changed_ref(enum ref_type ref_type, const char *ref_name, } hmap_destroy(&flood_remove_nodes); - HMAP_FOR_EACH_SAFE (lrln, next, hmap_node, &rlfn->lflow_uuids) { - hmap_remove(&rlfn->lflow_uuids, &lrln->hmap_node); - free(lrln); - } - ref_lflow_node_destroy(rlfn); - dhcp_opts_destroy(&dhcp_opts); dhcp_opts_destroy(&dhcpv6_opts); nd_ra_opts_destroy(&nd_ra_opts); @@ -969,11 +1009,54 @@ done: free(matches); } +static struct lflow_processed_node * +lflows_processed_find(struct hmap *lflows_processed, + const struct uuid *lflow_uuid) +{ + struct lflow_processed_node *node; + HMAP_FOR_EACH_WITH_HASH (node, hmap_node, uuid_hash(lflow_uuid), + lflows_processed) { + if (uuid_equals(&node->lflow_uuid, lflow_uuid)) { + return node; + } + } + return NULL; +} + +static void +lflows_processed_add(struct hmap *lflows_processed, + const struct uuid *lflow_uuid) +{ + struct lflow_processed_node *node = xmalloc(sizeof *node); + node->lflow_uuid = *lflow_uuid; + hmap_insert(lflows_processed, &node->hmap_node, uuid_hash(lflow_uuid)); +} + +static void +lflows_processed_remove(struct hmap *lflows_processed, + struct lflow_processed_node *node) +{ + hmap_remove(lflows_processed, &node->hmap_node); + free(node); +} + +void +lflows_processed_destroy(struct hmap *lflows_processed) +{ + struct lflow_processed_node *node, *next; + HMAP_FOR_EACH_SAFE (node, next, hmap_node, lflows_processed) { + hmap_remove(lflows_processed, &node->hmap_node); + free(node); + } + hmap_destroy(lflows_processed); +} + static void consider_logical_flow(const struct sbrec_logical_flow *lflow, struct hmap *dhcp_opts, struct hmap *dhcpv6_opts, struct hmap *nd_ra_opts, struct controller_event_options *controller_event_opts, + bool is_recompute, struct lflow_ctx_in *l_ctx_in, struct lflow_ctx_out *l_ctx_out) { @@ -987,6 +1070,13 @@ consider_logical_flow(const struct sbrec_logical_flow *lflow, } ovs_assert(!dp_group || !dp); + if (!is_recompute) { + ovs_assert(!lflows_processed_find(l_ctx_out->lflows_processed, + &lflow->header_.uuid)); + lflows_processed_add(l_ctx_out->lflows_processed, + &lflow->header_.uuid); + } + if (dp) { consider_logical_flow__(lflow, dp, dhcp_opts, dhcpv6_opts, nd_ra_opts, @@ -1923,6 +2013,12 @@ lflow_add_flows_for_datapath(const struct sbrec_datapath_binding *dp, const struct sbrec_logical_flow *lflow; SBREC_LOGICAL_FLOW_FOR_EACH_EQUAL ( lflow, lf_row, l_ctx_in->sbrec_logical_flow_by_logical_datapath) { + if (lflows_processed_find(l_ctx_out->lflows_processed, + &lflow->header_.uuid)) { + continue; + } + lflows_processed_add(l_ctx_out->lflows_processed, + &lflow->header_.uuid); consider_logical_flow__(lflow, dp, &dhcp_opts, &dhcpv6_opts, &nd_ra_opts, &controller_event_opts, l_ctx_in, l_ctx_out); @@ -1949,6 +2045,13 @@ lflow_add_flows_for_datapath(const struct sbrec_datapath_binding *dp, sbrec_logical_flow_index_set_logical_dp_group(lf_row, ldpg); SBREC_LOGICAL_FLOW_FOR_EACH_EQUAL ( lflow, lf_row, l_ctx_in->sbrec_logical_flow_by_logical_dp_group) { + if (lflows_processed_find(l_ctx_out->lflows_processed, + &lflow->header_.uuid)) { + continue; + } + /* Don't call lflows_processed_add() because here we process the + * lflow only for one of the DPs in the DP group, which may be + * incomplete. */ consider_logical_flow__(lflow, dp, &dhcp_opts, &dhcpv6_opts, &nd_ra_opts, &controller_event_opts, l_ctx_in, l_ctx_out); diff --git a/controller/lflow.h b/controller/lflow.h index 489dd70fb..d6dad17ec 100644 --- a/controller/lflow.h +++ b/controller/lflow.h @@ -159,10 +159,17 @@ struct lflow_ctx_out { struct lflow_resource_ref *lfrr; struct lflow_cache *lflow_cache; struct conj_ids *conj_ids; + struct hmap *lflows_processed; struct simap *hairpin_lb_ids; struct id_pool *hairpin_id_pool; }; +struct lflow_processed_node { + struct hmap_node hmap_node; /* In ed_type_lflow_output.lflows_processed. */ + struct uuid lflow_uuid; +}; +void lflows_processed_destroy(struct hmap *lflows_processed); + void lflow_init(void); void lflow_run(struct lflow_ctx_in *, struct lflow_ctx_out *); void lflow_handle_cached_flows(struct lflow_cache *, diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index 5069aedfc..8631bccbc 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -2176,6 +2176,11 @@ struct ed_type_lflow_output { /* conjunciton ID usage information of lflows */ struct conj_ids conj_ids; + /* lflows processed in the current engine execution. + * Cleared by en_lflow_output_clear_tracked_data before each engine + * execution. */ + struct hmap lflows_processed; + /* Data which is persistent and not cleared during * full recompute. */ struct lflow_output_persistent_data pd; @@ -2307,6 +2312,7 @@ init_lflow_ctx(struct engine_node *node, l_ctx_out->meter_table = &fo->meter_table; l_ctx_out->lfrr = &fo->lflow_resource_ref; l_ctx_out->conj_ids = &fo->conj_ids; + l_ctx_out->lflows_processed = &fo->lflows_processed; l_ctx_out->lflow_cache = fo->pd.lflow_cache; l_ctx_out->hairpin_id_pool = fo->hd.pool; l_ctx_out->hairpin_lb_ids = &fo->hd.ids; @@ -2322,11 +2328,20 @@ en_lflow_output_init(struct engine_node *node OVS_UNUSED, ovn_extend_table_init(&data->meter_table); lflow_resource_init(&data->lflow_resource_ref); lflow_conj_ids_init(&data->conj_ids); + hmap_init(&data->lflows_processed); simap_init(&data->hd.ids); data->hd.pool = id_pool_create(1, UINT32_MAX - 1); return data; } +static void +en_lflow_output_clear_tracked_data(void *data) +{ + struct ed_type_lflow_output *flow_output_data = data; + lflows_processed_destroy(&flow_output_data->lflows_processed); + hmap_init(&flow_output_data->lflows_processed); +} + static void en_lflow_output_cleanup(void *data) { @@ -2336,6 +2351,7 @@ en_lflow_output_cleanup(void *data) ovn_extend_table_destroy(&flow_output_data->meter_table); lflow_resource_destroy(&flow_output_data->lflow_resource_ref); lflow_conj_ids_destroy(&flow_output_data->conj_ids); + lflows_processed_destroy(&flow_output_data->lflows_processed); lflow_cache_destroy(flow_output_data->pd.lflow_cache); simap_destroy(&flow_output_data->hd.ids); id_pool_destroy(flow_output_data->hd.pool); @@ -3213,7 +3229,7 @@ main(int argc, char *argv[]) ENGINE_NODE(mff_ovn_geneve, "mff_ovn_geneve"); ENGINE_NODE(ofctrl_is_connected, "ofctrl_is_connected"); ENGINE_NODE(pflow_output, "physical_flow_output"); - ENGINE_NODE(lflow_output, "logical_flow_output"); + ENGINE_NODE_WITH_CLEAR_TRACK_DATA(lflow_output, "logical_flow_output"); ENGINE_NODE(flow_output, "flow_output"); ENGINE_NODE(addr_sets, "addr_sets"); ENGINE_NODE_WITH_CLEAR_TRACK_DATA(port_groups, "port_groups");