From patchwork Wed Sep 9 10:09:41 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Numan Siddique X-Patchwork-Id: 1360532 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.136; helo=silver.osuosl.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Authentication-Results: ozlabs.org; dmarc=none (p=none dis=none) header.from=ovn.org Received: from silver.osuosl.org (smtp3.osuosl.org [140.211.166.136]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 4Bmd6q3tk3z9sTN for ; Wed, 9 Sep 2020 20:09:55 +1000 (AEST) Received: from localhost (localhost [127.0.0.1]) by silver.osuosl.org (Postfix) with ESMTP id E9BED2E17D; Wed, 9 Sep 2020 10:09:53 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from silver.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id Cs9WKG1eBgSx; Wed, 9 Sep 2020 10:09:52 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by silver.osuosl.org (Postfix) with ESMTP id 74A9422E96; Wed, 9 Sep 2020 10:09:52 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id 51FDEC0891; Wed, 9 Sep 2020 10:09:52 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@lists.linuxfoundation.org Received: from silver.osuosl.org (smtp3.osuosl.org [140.211.166.136]) by lists.linuxfoundation.org (Postfix) with ESMTP id AEA7CC0051 for ; Wed, 9 Sep 2020 10:09:50 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by silver.osuosl.org (Postfix) with ESMTP id 8FEB322E96 for ; Wed, 9 Sep 2020 10:09:50 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from silver.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id hwl3VkQIw5mh for ; Wed, 9 Sep 2020 10:09:49 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net [217.70.183.200]) by silver.osuosl.org (Postfix) with ESMTPS id 8218E203F0 for ; Wed, 9 Sep 2020 10:09:49 +0000 (UTC) X-Originating-IP: 27.7.129.187 Received: from nusiddiq.home.org.com (unknown [27.7.129.187]) (Authenticated sender: numans@ovn.org) by relay7-d.mail.gandi.net (Postfix) with ESMTPSA id B56DB20002; Wed, 9 Sep 2020 10:09:45 +0000 (UTC) From: numans@ovn.org To: dev@openvswitch.org Date: Wed, 9 Sep 2020 15:39:41 +0530 Message-Id: <20200909100941.649009-1-numans@ovn.org> X-Mailer: git-send-email 2.26.2 In-Reply-To: <20200909100903.648778-1-numans@ovn.org> References: <20200909100903.648778-1-numans@ovn.org> MIME-Version: 1.0 Subject: [ovs-dev] [PATCH ovn v2 1/4] I-P engine: Provide the option to store client data in engine ctx. 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" From: Numan Siddique There can be some client specific data which could change from one engine run to another. Adding a 'void *' data in 'struct engine_ctx' will be useful. One such usecase is to provide a config option in ovn-controller to turn on or off logical flow expr caching. And this config knob can be stored in the engine_ctx. An upcoming patch will make use of this. Signed-off-by: Numan Siddique --- lib/inc-proc-eng.h | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/inc-proc-eng.h b/lib/inc-proc-eng.h index e25bcb29c6..80de75029e 100644 --- a/lib/inc-proc-eng.h +++ b/lib/inc-proc-eng.h @@ -66,6 +66,7 @@ struct engine_context { struct ovsdb_idl_txn *ovs_idl_txn; struct ovsdb_idl_txn *ovnsb_idl_txn; + void *client_ctx; }; /* Arguments to be passed to the engine at engine_init(). */ From patchwork Wed Sep 9 10:09:46 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Numan Siddique X-Patchwork-Id: 1360535 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.136; helo=silver.osuosl.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Authentication-Results: ozlabs.org; dmarc=none (p=none dis=none) header.from=ovn.org Received: from silver.osuosl.org (smtp3.osuosl.org [140.211.166.136]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 4Bmd7X5FFWz9sTg for ; Wed, 9 Sep 2020 20:10:32 +1000 (AEST) Received: from localhost (localhost [127.0.0.1]) by silver.osuosl.org (Postfix) with ESMTP id CAB492E224; Wed, 9 Sep 2020 10:10:30 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from silver.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id Hj7CFt8uhYj0; Wed, 9 Sep 2020 10:09:59 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by silver.osuosl.org (Postfix) with ESMTP id 3AE082E18D; Wed, 9 Sep 2020 10:09:59 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id 22480C0859; Wed, 9 Sep 2020 10:09:59 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@lists.linuxfoundation.org Received: from fraxinus.osuosl.org (smtp4.osuosl.org [140.211.166.137]) by lists.linuxfoundation.org (Postfix) with ESMTP id 566F4C0051 for ; Wed, 9 Sep 2020 10:09:58 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by fraxinus.osuosl.org (Postfix) with ESMTP id 462BA87060 for ; Wed, 9 Sep 2020 10:09:58 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from fraxinus.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id txgwaVfVkZAF for ; Wed, 9 Sep 2020 10:09:56 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net [217.70.183.201]) by fraxinus.osuosl.org (Postfix) with ESMTPS id 9C0A387051 for ; Wed, 9 Sep 2020 10:09:55 +0000 (UTC) X-Originating-IP: 27.7.129.187 Received: from nusiddiq.home.org.com (unknown [27.7.129.187]) (Authenticated sender: numans@ovn.org) by relay8-d.mail.gandi.net (Postfix) with ESMTPSA id 8479F1BF20A; Wed, 9 Sep 2020 10:09:51 +0000 (UTC) From: numans@ovn.org To: dev@openvswitch.org Date: Wed, 9 Sep 2020 15:39:46 +0530 Message-Id: <20200909100946.649063-1-numans@ovn.org> X-Mailer: git-send-email 2.26.2 In-Reply-To: <20200909100903.648778-1-numans@ovn.org> References: <20200909100903.648778-1-numans@ovn.org> MIME-Version: 1.0 Subject: [ovs-dev] [PATCH ovn v2 2/4] ovn-controller: Persist the conjunction ids allocated for conjuctive matches. 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" From: Numan Siddique For a logical flow which results in conjuctive OF matches, we are not persisting the allocated conjunction ids for it. There are few side effects because of this. - When a port group or address set gets modified, the logical flows which references these port groups/address sets gets reprocessed again and the resulting OpenvSwitch flows with conjunctive matches gets modified in the vswitchd if the conjunction id changes. - And because of this there is small probability of a packet getting dropped when the OF flows gets updated with different conjunction ids. This patch fixes this issue by persisting the conjunction ids. Earlier, logical flow caching support was added [1] to ovn-controller and then reverted [2] later due to some issues. This patch takes the lflow caching approach to persist the conjunction ids. But it only creates the cache for logical flows which result in conjunctive matches. And it doesn't cache the expr tree. The lflow caching is made configurable and enabled by default. Any user can disable caching by setting 'ovn-enable-lflow-cache' to 'false' in the OVS db. - ovs-vsctl set open . external_ids:ovn-enable-lflow-cache=false Note: Changing the option 'ovn-enable-lflow-cache' doesn't result in full recompute of I-P engine. But ovn-controller updates the chassis.other_config column. And when it receives the update2/update3 message, this results in full recompute (as any chassis changes results in full recompute). This is the case with all the config options in OVS db. An upcoming patch series, will attempt to add back the expr caching (addressing all the issues.) [1] - 8795bec737b("ovn-controller: Cache logical flow expr tree for each lflow.) [2] - 065bcf46218("ovn-controller: Revert lflow expr caching") Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1858878 Signed-off-by: Numan Siddique --- controller/chassis.c | 22 +++++- controller/lflow.c | 118 ++++++++++++++++++++++++++++++- controller/lflow.h | 8 ++- controller/ovn-controller.c | 97 +++++++++++++++++++++++--- tests/ovn.at | 135 ++++++++++++++++++++++++++++++++++++ 5 files changed, 363 insertions(+), 17 deletions(-) diff --git a/controller/chassis.c b/controller/chassis.c index 45e018e385..8e6ad2d459 100644 --- a/controller/chassis.c +++ b/controller/chassis.c @@ -86,6 +86,7 @@ struct ovs_chassis_cfg { const char *cms_options; const char *monitor_all; const char *chassis_macs; + const char *enable_lflow_cache; /* Set of encap types parsed from the 'ovn-encap-type' external-id. */ struct sset encap_type_set; @@ -165,6 +166,12 @@ get_monitor_all(const struct smap *ext_ids) return smap_get_def(ext_ids, "ovn-monitor-all", "false"); } +static const char * +get_enable_lflow_cache(const struct smap *ext_ids) +{ + return smap_get_def(ext_ids, "ovn-enable-lflow-cache", "true"); +} + static const char * get_encap_csum(const struct smap *ext_ids) { @@ -286,6 +293,7 @@ chassis_parse_ovs_config(const struct ovsrec_open_vswitch_table *ovs_table, ovs_cfg->cms_options = get_cms_options(&cfg->external_ids); ovs_cfg->monitor_all = get_monitor_all(&cfg->external_ids); ovs_cfg->chassis_macs = get_chassis_mac_mappings(&cfg->external_ids); + ovs_cfg->enable_lflow_cache = get_enable_lflow_cache(&cfg->external_ids); if (!chassis_parse_ovs_encap_type(encap_type, &ovs_cfg->encap_type_set)) { return false; @@ -312,12 +320,14 @@ static void chassis_build_other_config(struct smap *config, const char *bridge_mappings, const char *datapath_type, const char *cms_options, const char *monitor_all, const char *chassis_macs, - const char *iface_types, bool is_interconn) + const char *iface_types, + const char *enable_lflow_cache, bool is_interconn) { smap_replace(config, "ovn-bridge-mappings", bridge_mappings); smap_replace(config, "datapath-type", datapath_type); smap_replace(config, "ovn-cms-options", cms_options); smap_replace(config, "ovn-monitor-all", monitor_all); + smap_replace(config, "ovn-enable-lflow-cache", enable_lflow_cache); smap_replace(config, "iface-types", iface_types); smap_replace(config, "ovn-chassis-mac-mappings", chassis_macs); smap_replace(config, "is-interconn", is_interconn ? "true" : "false"); @@ -332,6 +342,7 @@ chassis_other_config_changed(const char *bridge_mappings, const char *cms_options, const char *monitor_all, const char *chassis_macs, + const char *enable_lflow_cache, const struct ds *iface_types, bool is_interconn, const struct sbrec_chassis *chassis_rec) @@ -364,6 +375,13 @@ chassis_other_config_changed(const char *bridge_mappings, return true; } + const char *chassis_enable_lflow_cache = + get_enable_lflow_cache(&chassis_rec->other_config); + + if (strcmp(enable_lflow_cache, chassis_enable_lflow_cache)) { + return true; + } + const char *chassis_mac_mappings = get_chassis_mac_mappings(&chassis_rec->other_config); if (strcmp(chassis_macs, chassis_mac_mappings)) { @@ -588,6 +606,7 @@ chassis_update(const struct sbrec_chassis *chassis_rec, ovs_cfg->cms_options, ovs_cfg->monitor_all, ovs_cfg->chassis_macs, + ovs_cfg->enable_lflow_cache, &ovs_cfg->iface_types, ovs_cfg->is_interconn, chassis_rec)) { @@ -600,6 +619,7 @@ chassis_update(const struct sbrec_chassis *chassis_rec, ovs_cfg->monitor_all, ovs_cfg->chassis_macs, ds_cstr_ro(&ovs_cfg->iface_types), + ovs_cfg->enable_lflow_cache, ovs_cfg->is_interconn); sbrec_chassis_verify_other_config(chassis_rec); sbrec_chassis_set_other_config(chassis_rec, &other_config); diff --git a/controller/lflow.c b/controller/lflow.c index 151561210a..c2f939f90f 100644 --- a/controller/lflow.c +++ b/controller/lflow.c @@ -269,6 +269,70 @@ lflow_resource_destroy_lflow(struct lflow_resource_ref *lfrr, free(lfrn); } +/* Represents an lflow cache which + * - stores the conjunction id offset if the lflow matches + * results in conjunctive OpenvSwitch flows. + */ +struct lflow_cache { + struct hmap_node node; + struct uuid lflow_uuid; /* key */ + uint32_t conj_id_ofs; +}; + +static struct lflow_cache * +lflow_cache_add(struct hmap *lflow_cache_map, + const struct sbrec_logical_flow *lflow) +{ + struct lflow_cache *lc = xmalloc(sizeof *lc); + lc->lflow_uuid = lflow->header_.uuid; + lc->conj_id_ofs = 0; + hmap_insert(lflow_cache_map, &lc->node, uuid_hash(&lc->lflow_uuid)); + return lc; +} + +static struct lflow_cache * +lflow_cache_get(struct hmap *lflow_cache_map, + const struct sbrec_logical_flow *lflow) +{ + struct lflow_cache *lc; + size_t hash = uuid_hash(&lflow->header_.uuid); + HMAP_FOR_EACH_WITH_HASH (lc, node, hash, lflow_cache_map) { + if (uuid_equals(&lc->lflow_uuid, &lflow->header_.uuid)) { + return lc; + } + } + + return NULL; +} + +static void +lflow_cache_delete(struct hmap *lflow_cache_map, + const struct sbrec_logical_flow *lflow) +{ + struct lflow_cache *lc = lflow_cache_get(lflow_cache_map, lflow); + if (lc) { + hmap_remove(lflow_cache_map, &lc->node); + free(lc); + } +} + +void +lflow_cache_init(struct hmap *lflow_cache_map) +{ + hmap_init(lflow_cache_map); +} + +void +lflow_cache_destroy(struct hmap *lflow_cache_map) +{ + struct lflow_cache *lc; + HMAP_FOR_EACH_POP (lc, node, lflow_cache_map) { + free(lc); + } + + hmap_destroy(lflow_cache_map); +} + /* Adds the logical flows from the Logical_Flow table to flow tables. */ static void add_logical_flows(struct lflow_ctx_in *l_ctx_in, @@ -306,6 +370,7 @@ add_logical_flows(struct lflow_ctx_in *l_ctx_in, static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5); VLOG_ERR_RL(&rl, "Conjunction id overflow when processing lflow " UUID_FMT, UUID_ARGS(&lflow->header_.uuid)); + l_ctx_out->conj_id_overflow = true; } } @@ -355,6 +420,9 @@ lflow_handle_changed_flows(struct lflow_ctx_in *l_ctx_in, /* Delete entries from lflow resource reference. */ lflow_resource_destroy_lflow(l_ctx_out->lfrr, &lflow->header_.uuid); + if (l_ctx_out->lflow_cache_map) { + lflow_cache_delete(l_ctx_out->lflow_cache_map, lflow); + } } } @@ -382,6 +450,7 @@ lflow_handle_changed_flows(struct lflow_ctx_in *l_ctx_in, &nd_ra_opts, &controller_event_opts, l_ctx_in, l_ctx_out)) { ret = false; + l_ctx_out->conj_id_overflow = true; break; } } @@ -467,6 +536,7 @@ lflow_handle_changed_ref(enum ref_type ref_type, const char *ref_name, &nd_ra_opts, &controller_event_opts, l_ctx_in, l_ctx_out)) { ret = false; + l_ctx_out->conj_id_overflow = true; break; } *changed = true; @@ -652,13 +722,41 @@ consider_logical_flow(const struct sbrec_logical_flow *lflow, ovnacts_free(ovnacts.data, ovnacts.size); ofpbuf_uninit(&ovnacts); + uint32_t conj_id_ofs = *l_ctx_out->conj_id_ofs; + if (n_conjs) { + if (l_ctx_out->lflow_cache_map) { + struct lflow_cache *lc = + lflow_cache_get(l_ctx_out->lflow_cache_map, lflow); + if (!lc) { + lc = lflow_cache_add(l_ctx_out->lflow_cache_map, lflow); + } + + if (!lc->conj_id_ofs) { + lc->conj_id_ofs = *l_ctx_out->conj_id_ofs; + if (!update_conj_id_ofs(l_ctx_out->conj_id_ofs, n_conjs)) { + lc->conj_id_ofs = 0; + expr_matches_destroy(&matches); + return false; + } + } + + conj_id_ofs = lc->conj_id_ofs; + } else { + /* lflow caching is disabled. */ + if (!update_conj_id_ofs(l_ctx_out->conj_id_ofs, n_conjs)) { + expr_matches_destroy(&matches); + return false; + } + } + } + /* Prepare the OpenFlow matches for adding to the flow table. */ struct expr_match *m; HMAP_FOR_EACH (m, hmap_node, &matches) { match_set_metadata(&m->match, htonll(lflow->logical_datapath->tunnel_key)); if (m->match.wc.masks.conj_id) { - m->match.flow.conj_id += *l_ctx_out->conj_id_ofs; + m->match.flow.conj_id += conj_id_ofs; } if (datapath_is_switch(ldp)) { unsigned int reg_index @@ -693,7 +791,7 @@ consider_logical_flow(const struct sbrec_logical_flow *lflow, struct ofpact_conjunction *dst; dst = ofpact_put_CONJUNCTION(&conj); - dst->id = src->id + *l_ctx_out->conj_id_ofs; + dst->id = src->id + conj_id_ofs; dst->clause = src->clause; dst->n_clauses = src->n_clauses; } @@ -708,7 +806,7 @@ consider_logical_flow(const struct sbrec_logical_flow *lflow, /* Clean up. */ expr_matches_destroy(&matches); ofpbuf_uninit(&ofpacts); - return update_conj_id_ofs(l_ctx_out->conj_id_ofs, n_conjs); + return true; } static void @@ -858,6 +956,19 @@ lflow_run(struct lflow_ctx_in *l_ctx_in, struct lflow_ctx_out *l_ctx_out) { COVERAGE_INC(lflow_run); + /* when lflow_run is called, it's possible that some of the logical flows + * are deleted. We need to delete the lflow cache for these + * lflows (if present), otherwise, they will not be deleted at all. */ + if (l_ctx_out->lflow_cache_map) { + const struct sbrec_logical_flow *lflow; + SBREC_LOGICAL_FLOW_TABLE_FOR_EACH_TRACKED (lflow, + l_ctx_in->logical_flow_table) { + if (sbrec_logical_flow_is_deleted(lflow)) { + lflow_cache_delete(l_ctx_out->lflow_cache_map, lflow); + } + } + } + add_logical_flows(l_ctx_in, l_ctx_out); add_neighbor_flows(l_ctx_in->sbrec_port_binding_by_name, l_ctx_in->mac_binding_table, l_ctx_in->local_datapaths, @@ -914,6 +1025,7 @@ lflow_add_flows_for_datapath(const struct sbrec_datapath_binding *dp, &nd_ra_opts, &controller_event_opts, l_ctx_in, l_ctx_out)) { handled = false; + l_ctx_out->conj_id_overflow = true; break; } } diff --git a/controller/lflow.h b/controller/lflow.h index ae02eaf5e8..c66b318e9c 100644 --- a/controller/lflow.h +++ b/controller/lflow.h @@ -141,12 +141,13 @@ struct lflow_ctx_out { struct ovn_extend_table *group_table; struct ovn_extend_table *meter_table; struct lflow_resource_ref *lfrr; - struct hmap *lflow_expr_cache; + struct hmap *lflow_cache_map; uint32_t *conj_id_ofs; + bool conj_id_overflow; }; void lflow_init(void); -void lflow_run(struct lflow_ctx_in *, struct lflow_ctx_out *); +void lflow_run(struct lflow_ctx_in *, struct lflow_ctx_out *); bool lflow_handle_changed_flows(struct lflow_ctx_in *, struct lflow_ctx_out *); bool lflow_handle_changed_ref(enum ref_type, const char *ref_name, struct lflow_ctx_in *, struct lflow_ctx_out *, @@ -159,7 +160,8 @@ void lflow_handle_changed_neighbors( void lflow_destroy(void); -void lflow_expr_destroy(struct hmap *lflow_expr_cache); +void lflow_cache_init(struct hmap *); +void lflow_cache_destroy(struct hmap *); bool lflow_add_flows_for_datapath(const struct sbrec_datapath_binding *, struct lflow_ctx_in *, diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index 674a09489a..995805470e 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -76,6 +76,7 @@ static unixctl_cb_func cluster_state_reset_cmd; static unixctl_cb_func debug_pause_execution; static unixctl_cb_func debug_resume_execution; static unixctl_cb_func debug_status_execution; +static unixctl_cb_func flush_lflow_cache; #define DEFAULT_BRIDGE_NAME "br-int" #define DEFAULT_PROBE_INTERVAL_MSEC 5000 @@ -485,7 +486,8 @@ get_ofctrl_probe_interval(struct ovsdb_idl *ovs_idl) * updates 'sbdb_idl' with that pointer. */ static void update_sb_db(struct ovsdb_idl *ovs_idl, struct ovsdb_idl *ovnsb_idl, - bool *monitor_all_p, bool *reset_ovnsb_idl_min_index) + bool *monitor_all_p, bool *reset_ovnsb_idl_min_index, + bool *enable_lflow_cache) { const struct ovsrec_open_vswitch *cfg = ovsrec_open_vswitch_first(ovs_idl); if (!cfg) { @@ -521,6 +523,11 @@ update_sb_db(struct ovsdb_idl *ovs_idl, struct ovsdb_idl *ovnsb_idl, ovsdb_idl_reset_min_index(ovnsb_idl); *reset_ovnsb_idl_min_index = false; } + + if (enable_lflow_cache != NULL) { + *enable_lflow_cache = + smap_get_bool(&cfg->external_ids, "ovn-enable-lflow-cache", true); + } } static void @@ -811,6 +818,10 @@ enum ovs_engine_node { OVS_NODES #undef OVS_NODE +struct controller_engine_ctx { + bool enable_lflow_cache; +}; + struct ed_type_ofctrl_is_connected { bool connected; }; @@ -1542,6 +1553,12 @@ physical_flow_changes_ovs_iface_handler(struct engine_node *node, void *data) return true; } +struct flow_output_persistent_data { + uint32_t conj_id_ofs; + struct hmap lflow_cache_map; + bool lflow_cache_enabled; +}; + struct ed_type_flow_output { /* desired flows */ struct ovn_desired_flow_table flow_table; @@ -1549,10 +1566,12 @@ struct ed_type_flow_output { struct ovn_extend_table group_table; /* meter ids for QoS */ struct ovn_extend_table meter_table; - /* conjunction id offset */ - uint32_t conj_id_ofs; /* lflow resource cross reference */ struct lflow_resource_ref lflow_resource_ref; + + /* Data which is persistent and not cleared during + * full recompute. */ + struct flow_output_persistent_data pd; }; static void init_physical_ctx(struct engine_node *node, @@ -1703,7 +1722,13 @@ static void init_lflow_ctx(struct engine_node *node, l_ctx_out->group_table = &fo->group_table; l_ctx_out->meter_table = &fo->meter_table; l_ctx_out->lfrr = &fo->lflow_resource_ref; - l_ctx_out->conj_id_ofs = &fo->conj_id_ofs; + l_ctx_out->conj_id_ofs = &fo->pd.conj_id_ofs; + if (fo->pd.lflow_cache_enabled) { + l_ctx_out->lflow_cache_map = &fo->pd.lflow_cache_map; + } else { + l_ctx_out->lflow_cache_map = NULL; + } + l_ctx_out->conj_id_overflow = false; } static void * @@ -1715,8 +1740,10 @@ en_flow_output_init(struct engine_node *node OVS_UNUSED, ovn_desired_flow_table_init(&data->flow_table); ovn_extend_table_init(&data->group_table); ovn_extend_table_init(&data->meter_table); - data->conj_id_ofs = 1; + data->pd.conj_id_ofs = 1; lflow_resource_init(&data->lflow_resource_ref); + lflow_cache_init(&data->pd.lflow_cache_map); + data->pd.lflow_cache_enabled = true; return data; } @@ -1728,6 +1755,7 @@ en_flow_output_cleanup(void *data) ovn_extend_table_destroy(&flow_output_data->group_table); ovn_extend_table_destroy(&flow_output_data->meter_table); lflow_resource_destroy(&flow_output_data->lflow_resource_ref); + lflow_cache_destroy(&flow_output_data->pd.lflow_cache_map); } static void @@ -1761,7 +1789,6 @@ en_flow_output_run(struct engine_node *node, void *data) struct ovn_desired_flow_table *flow_table = &fo->flow_table; struct ovn_extend_table *group_table = &fo->group_table; struct ovn_extend_table *meter_table = &fo->meter_table; - uint32_t *conj_id_ofs = &fo->conj_id_ofs; struct lflow_resource_ref *lfrr = &fo->lflow_resource_ref; static bool first_run = true; @@ -1774,12 +1801,39 @@ en_flow_output_run(struct engine_node *node, void *data) lflow_resource_clear(lfrr); } - *conj_id_ofs = 1; + struct controller_engine_ctx *ctrl_ctx = engine_get_context()->client_ctx; + if (fo->pd.lflow_cache_enabled && !ctrl_ctx->enable_lflow_cache) { + lflow_cache_destroy(&fo->pd.lflow_cache_map); + lflow_cache_init(&fo->pd.lflow_cache_map); + } + fo->pd.lflow_cache_enabled = ctrl_ctx->enable_lflow_cache; + + if (!fo->pd.lflow_cache_enabled) { + fo->pd.conj_id_ofs = 1; + } + struct lflow_ctx_in l_ctx_in; struct lflow_ctx_out l_ctx_out; init_lflow_ctx(node, rt_data, fo, &l_ctx_in, &l_ctx_out); lflow_run(&l_ctx_in, &l_ctx_out); + if (l_ctx_out.conj_id_overflow) { + /* Conjunction ids overflow. There can be many holes in between. + * Destroy lflow cache and call lflow_run() again. */ + ovn_desired_flow_table_clear(flow_table); + ovn_extend_table_clear(group_table, false /* desired */); + ovn_extend_table_clear(meter_table, false /* desired */); + lflow_resource_clear(lfrr); + fo->pd.conj_id_ofs = 1; + lflow_cache_destroy(&fo->pd.lflow_cache_map); + lflow_cache_init(&fo->pd.lflow_cache_map); + l_ctx_out.conj_id_overflow = false; + lflow_run(&l_ctx_in, &l_ctx_out); + if (l_ctx_out.conj_id_overflow) { + VLOG_WARN("Conjunction id overflow."); + } + } + struct physical_ctx p_ctx; init_physical_ctx(node, rt_data, &p_ctx); @@ -2346,6 +2400,8 @@ main(int argc, char *argv[]) unixctl_command_register("recompute", "", 0, 0, engine_recompute_cmd, NULL); + unixctl_command_register("flush-lflow-cache", "", 0, 0, flush_lflow_cache, + &flow_output_data->pd); bool reset_ovnsb_idl_min_index = false; unixctl_command_register("sb-cluster-state-reset", "", 0, 0, @@ -2363,6 +2419,10 @@ main(int argc, char *argv[]) unsigned int ovs_cond_seqno = UINT_MAX; unsigned int ovnsb_cond_seqno = UINT_MAX; + struct controller_engine_ctx ctrl_engine_ctx = { + .enable_lflow_cache = true + }; + /* Main loop. */ exiting = false; restart = false; @@ -2391,7 +2451,8 @@ main(int argc, char *argv[]) } update_sb_db(ovs_idl_loop.idl, ovnsb_idl_loop.idl, &sb_monitor_all, - &reset_ovnsb_idl_min_index); + &reset_ovnsb_idl_min_index, + &ctrl_engine_ctx.enable_lflow_cache); update_ssl_config(ovsrec_ssl_table_get(ovs_idl_loop.idl)); ofctrl_set_probe_interval(get_ofctrl_probe_interval(ovs_idl_loop.idl)); @@ -2409,7 +2470,8 @@ main(int argc, char *argv[]) struct engine_context eng_ctx = { .ovs_idl_txn = ovs_idl_txn, - .ovnsb_idl_txn = ovnsb_idl_txn + .ovnsb_idl_txn = ovnsb_idl_txn, + .client_ctx = &ctrl_engine_ctx }; engine_set_context(&eng_ctx); @@ -2651,7 +2713,8 @@ loop_done: if (!restart) { bool done = !ovsdb_idl_has_ever_connected(ovnsb_idl_loop.idl); while (!done) { - update_sb_db(ovs_idl_loop.idl, ovnsb_idl_loop.idl, NULL, NULL); + update_sb_db(ovs_idl_loop.idl, ovnsb_idl_loop.idl, + NULL, NULL, NULL); update_ssl_config(ovsrec_ssl_table_get(ovs_idl_loop.idl)); struct ovsdb_idl_txn *ovs_idl_txn @@ -2881,6 +2944,20 @@ engine_recompute_cmd(struct unixctl_conn *conn OVS_UNUSED, int argc OVS_UNUSED, unixctl_command_reply(conn, NULL); } +static void +flush_lflow_cache(struct unixctl_conn *conn OVS_UNUSED, int argc OVS_UNUSED, + const char *argv[] OVS_UNUSED, void *arg_) +{ + VLOG_INFO("User triggered lflow cache flush."); + struct flow_output_persistent_data *fo_pd = arg_; + lflow_cache_destroy(&fo_pd->lflow_cache_map); + lflow_cache_init(&fo_pd->lflow_cache_map); + fo_pd->conj_id_ofs = 1; + engine_set_force_recompute(true); + poll_immediate_wake(); + unixctl_command_reply(conn, NULL); +} + static void cluster_state_reset_cmd(struct unixctl_conn *conn, int argc OVS_UNUSED, const char *argv[] OVS_UNUSED, void *idl_reset_) diff --git a/tests/ovn.at b/tests/ovn.at index 1fe34b781e..1d8f470a36 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -21409,6 +21409,141 @@ OVN_CLEANUP([hv1],[hv2]) AT_CLEANUP +AT_SETUP([ovn -- lflow cache for conjunctions]) +ovn_start +net_add n1 +sim_add hv1 + +as hv1 +ovs-vsctl add-br br-phys +ovn_attach n1 br-phys 192.168.0.1 + +ovn-nbctl ls-add sw0 +ovn-nbctl lsp-add sw0 sw0-p1 +ovn-nbctl lsp-set-addresses sw0-p1 "10:14:00:00:00:03 10.0.0.3" +ovn-nbctl lsp-set-port-security sw0-p1 "10:14:00:00:00:03 10.0.0.3" + +ovn-nbctl lsp-add sw0 sw0-p2 +ovn-nbctl lsp-set-addresses sw0-p2 "10:14:00:00:00:04 10.0.0.4" +ovn-nbctl lsp-set-port-security sw0-p2 "10:14:00:00:00:04 10.0.0.4" + +ovn-nbctl lsp-add sw0 sw0-p3 +ovn-nbctl lsp-set-addresses sw0-p3 "10:14:00:00:00:05 10.0.0.5" +ovn-nbctl lsp-set-port-security sw0-p3 "10:14:00:00:00:05 10.0.0.5" + +ovn-nbctl lsp-add sw0 sw0-p4 +ovn-nbctl lsp-set-addresses sw0-p4 "10:14:00:00:00:06 10.0.0.6" +ovn-nbctl lsp-set-port-security sw0-p4 "10:14:00:00:00:06 10.0.0.6" + +as hv1 +ovs-vsctl -- add-port br-int hv1-vif1 -- \ + set interface hv1-vif1 external-ids:iface-id=sw0-p1 \ + options:tx_pcap=hv1/vif1-tx.pcap \ + options:rxq_pcap=hv1/vif1-rx.pcap \ + ofport-request=1 +ovs-vsctl -- add-port br-int hv1-vif2 -- \ + set interface hv1-vif2 external-ids:iface-id=sw0-p2 \ + options:tx_pcap=hv1/vif2-tx.pcap \ + options:rxq_pcap=hv1/vif2-rx.pcap \ + ofport-request=2 +ovs-vsctl -- add-port br-int hv1-vif3 -- \ + set interface hv1-vif3 external-ids:iface-id=sw0-p3 \ + options:tx_pcap=hv1/vif3-tx.pcap \ + options:rxq_pcap=hv1/vif3-rx.pcap \ + ofport-request=3 +ovs-vsctl -- add-port br-int hv1-vif4 -- \ + set interface hv1-vif4 external-ids:iface-id=sw0-p4 \ + options:tx_pcap=hv1/vif4-tx.pcap \ + options:rxq_pcap=hv1/vif4-rx.pcap \ + ofport-request=4 + +OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p1) = xup]) +OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p2) = xup]) +OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p3) = xup]) +OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p4) = xup]) + +ovn-nbctl pg-add pg0 sw0-p1 sw0-p2 +ovn-nbctl acl-add pg0 to-lport 1002 "outport == @pg0 && ip4 && tcp.dst >= 80 && tcp.dst <= 82" allow +ovn-nbctl --wait=hv sync + +OVS_WAIT_UNTIL([test 1 = $(as hv1 ovs-ofctl dump-flows br-int table=44 | grep -c "conj_id=2")]) + +# Add sw0-p3 to the port group pg0. The conj_id should be 2. +ovn-nbctl pg-set-ports pg0 sw0-p1 sw0-p2 sw0-p3 +OVS_WAIT_UNTIL([test 1 = $(as hv1 ovs-ofctl dump-flows br-int table=44 | grep -c "conj_id")]) +AT_CHECK([test 1 = $(as hv1 ovs-ofctl dump-flows br-int table=44 | grep -c "conj_id=2")]) + +# Add sw0p4 to the port group pg0. The conj_id should be 2. +ovn-nbctl pg-set-ports pg0 sw0-p1 sw0-p2 sw0-p3 sw0-p4 +OVS_WAIT_UNTIL([test 1 = $(as hv1 ovs-ofctl dump-flows br-int table=44 | grep -c "conj_id")]) +AT_CHECK([test 1 = $(as hv1 ovs-ofctl dump-flows br-int table=44 | grep -c "conj_id=2")]) + +# Add another ACL with conjunction. +ovn-nbctl acl-add pg0 to-lport 1002 "outport == @pg0 && ip4 && udp.dst >= 80 && udp.dst <= 82" allow +OVS_WAIT_UNTIL([test 2 = $(as hv1 ovs-ofctl dump-flows br-int table=44 | grep -c "conj_id")]) +AT_CHECK([test 1 = $(as hv1 ovs-ofctl dump-flows br-int table=44 | grep tcp | grep -c "conj_id=2")]) +AT_CHECK([test 1 = $(as hv1 ovs-ofctl dump-flows br-int table=44 | grep udp | grep -c "conj_id=3")]) + +# Delete tcp ACL. +ovn-nbctl acl-del pg0 to-lport 1002 "outport == @pg0 && ip4 && tcp.dst >= 80 && tcp.dst <= 82" +OVS_WAIT_UNTIL([test 1 = $(as hv1 ovs-ofctl dump-flows br-int table=44 | grep -c "conj_id")]) +AT_CHECK([test 1 = $(as hv1 ovs-ofctl dump-flows br-int table=44 | grep udp | grep -c "conj_id=3")]) + +# Add back the tcp ACL. +ovn-nbctl acl-add pg0 to-lport 1002 "outport == @pg0 && ip4 && tcp.dst >= 80 && tcp.dst <= 82" allow +OVS_WAIT_UNTIL([test 2 = $(as hv1 ovs-ofctl dump-flows br-int table=44 | grep -c "conj_id")]) +AT_CHECK([test 1 = $(as hv1 ovs-ofctl dump-flows br-int table=44 | grep udp | grep -c "conj_id=3")]) +AT_CHECK([test 1 = $(as hv1 ovs-ofctl dump-flows br-int table=44 | grep tcp | grep -c "conj_id=4")]) + +ovn-nbctl acl-add pg0 to-lport 1002 "outport == @pg0 && inport == @pg0 && ip4 && tcp.dst >= 84 && tcp.dst <= 86" allow +OVS_WAIT_UNTIL([test 3 = $(as hv1 ovs-ofctl dump-flows br-int table=44 | grep -c "conj_id")]) +AT_CHECK([test 1 = $(as hv1 ovs-ofctl dump-flows br-int table=44 | grep udp | grep -c "conj_id=3")]) +AT_CHECK([test 1 = $(as hv1 ovs-ofctl dump-flows br-int table=44 | grep tcp | grep -c "conj_id=4")]) +AT_CHECK([test 1 = $(as hv1 ovs-ofctl dump-flows br-int table=44 | grep tcp | grep -c "conj_id=5")]) + +ovn-nbctl clear port_group pg0 acls +OVS_WAIT_UNTIL([test 0 = $(as hv1 ovs-ofctl dump-flows br-int table=44 | grep -c "conj_id")]) + +ovn-nbctl --wait=hv acl-add pg0 to-lport 1002 "outport == @pg0 && ip4 && tcp.dst >= 80 && tcp.dst <= 82" allow +ovn-nbctl --wait=hv acl-add pg0 to-lport 1002 "outport == @pg0 && ip4 && udp.dst >= 80 && udp.dst <= 82" allow +OVS_WAIT_UNTIL([test 2 = $(as hv1 ovs-ofctl dump-flows br-int table=44 | grep -c "conj_id")]) +AT_CHECK([test 1 = $(as hv1 ovs-ofctl dump-flows br-int table=44 | grep tcp | grep -c "conj_id=6")]) +AT_CHECK([test 1 = $(as hv1 ovs-ofctl dump-flows br-int table=44 | grep udp | grep -c "conj_id=7")]) + +# Flush the lflow cache. +as hv1 ovn-appctl -t ovn-controller flush-lflow-cache +OVS_WAIT_UNTIL([test 2 = $(as hv1 ovs-ofctl dump-flows br-int table=44 | grep -c "conj_id")]) +AT_CHECK([test 1 = $(as hv1 ovs-ofctl dump-flows br-int table=44 | grep -c "conj_id=2")]) +AT_CHECK([test 1 = $(as hv1 ovs-ofctl dump-flows br-int table=44 | grep -c "conj_id=3")]) + +# Disable lflow caching. + +as hv1 ovs-vsctl set open . external_ids:ovn-enable-lflow-cache=false + +# Wait until ovn-enble-lflow-cache is processed by ovn-controller. +OVS_WAIT_UNTIL([ + test $(ovn-sbctl get chassis hv1 other_config:ovn-enable-lflow-cache) = '"false"' +]) + +AT_CHECK([test 1 = $(as hv1 ovs-ofctl dump-flows br-int table=44 | grep -c "conj_id=2")]) +AT_CHECK([test 1 = $(as hv1 ovs-ofctl dump-flows br-int table=44 | grep -c "conj_id=3")]) + +# Remove port sw0-p4 from port group. +ovn-nbctl pg-set-ports pg0 sw0-p1 sw0-p2 sw0-p3 +OVS_WAIT_UNTIL([test 2 = $(as hv1 ovs-ofctl dump-flows br-int table=44 | grep -c "conj_id")]) +AT_CHECK([test 1 = $(as hv1 ovs-ofctl dump-flows br-int table=44 | grep -c "conj_id=4")]) +AT_CHECK([test 1 = $(as hv1 ovs-ofctl dump-flows br-int table=44 | grep -c "conj_id=5")]) + +as hv1 ovn-appctl -t ovn-controller recompute + +OVS_WAIT_UNTIL([test 2 = $(as hv1 ovs-ofctl dump-flows br-int table=44 | grep -c "conj_id")]) +OVS_WAIT_UNTIL([test 1 = $(as hv1 ovs-ofctl dump-flows br-int table=44 | grep -c "conj_id=2")]) +AT_CHECK([test 1 = $(as hv1 ovs-ofctl dump-flows br-int table=44 | grep -c "conj_id=3")]) + +OVN_CLEANUP([hv1]) + +AT_CLEANUP + AT_SETUP([ovn -- Delete Port_Binding and OVS port Incremental Processing]) ovn_start From patchwork Wed Sep 9 10:09:53 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Numan Siddique X-Patchwork-Id: 1360533 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=fraxinus.osuosl.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Authentication-Results: ozlabs.org; dmarc=none (p=none dis=none) header.from=ovn.org Received: from fraxinus.osuosl.org (smtp4.osuosl.org [140.211.166.137]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 4Bmd794Nrzz9sTN for ; Wed, 9 Sep 2020 20:10:13 +1000 (AEST) Received: from localhost (localhost [127.0.0.1]) by fraxinus.osuosl.org (Postfix) with ESMTP id D124E852D5; Wed, 9 Sep 2020 10:10:11 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from fraxinus.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 8yZNG33pPAtp; Wed, 9 Sep 2020 10:10:08 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by fraxinus.osuosl.org (Postfix) with ESMTP id 7378E87051; Wed, 9 Sep 2020 10:10:06 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id 566FDC0859; Wed, 9 Sep 2020 10:10:06 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@lists.linuxfoundation.org Received: from whitealder.osuosl.org (smtp1.osuosl.org [140.211.166.138]) by lists.linuxfoundation.org (Postfix) with ESMTP id 45F83C0051 for ; Wed, 9 Sep 2020 10:10:05 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by whitealder.osuosl.org (Postfix) with ESMTP id 42E1587146 for ; Wed, 9 Sep 2020 10:10:05 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from whitealder.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id hyN7N3bj9M-O for ; Wed, 9 Sep 2020 10:10:02 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from relay1-d.mail.gandi.net (relay1-d.mail.gandi.net [217.70.183.193]) by whitealder.osuosl.org (Postfix) with ESMTPS id B327887136 for ; Wed, 9 Sep 2020 10:10:01 +0000 (UTC) X-Originating-IP: 27.7.129.187 Received: from nusiddiq.home.org.com (unknown [27.7.129.187]) (Authenticated sender: numans@ovn.org) by relay1-d.mail.gandi.net (Postfix) with ESMTPSA id 8FF7B240006; Wed, 9 Sep 2020 10:09:57 +0000 (UTC) From: numans@ovn.org To: dev@openvswitch.org Date: Wed, 9 Sep 2020 15:39:53 +0530 Message-Id: <20200909100953.649117-1-numans@ovn.org> X-Mailer: git-send-email 2.26.2 In-Reply-To: <20200909100903.648778-1-numans@ovn.org> References: <20200909100903.648778-1-numans@ovn.org> MIME-Version: 1.0 Subject: [ovs-dev] [PATCH ovn v2 3/4] expr: Evaluate the condition expression in a separate step. 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" From: Numan Siddique A similar patch was committed earlier [1] and then reverted [2]. This patch is similar to [1], but not exactly same. A new function is added - expr_evaluate_condition() which evaluates the condition expression - is_chassis_resident. expr_simply() will no longer evaluates this condition. expr_normalize() is still expected to be called after expr_evaluate_condition(). Otherwise it will assert if 'is_chassis_resident()' is not evaluated. An upcoming commit needs this in order to cache the logical flow expressions for logical flows having 'is_chassis_resident()' condition in their match. The expr tree after expr_simplify() for such logical flows is cached. Since we can't cache the is_chassis_resident condition, it is separated out. (For logical flows which do not have this condition in their match, the expr matches will be cached.) [1] - 99e3a145927("expr: Evaluate the condition expression in a separate step.") [2] - 065bcf46218("ovn-controller: Revert lflow expr caching") Signed-off-by: Numan Siddique Acked-by: Mark Michelson --- controller/lflow.c | 3 ++- include/ovn/expr.h | 10 +++++---- lib/expr.c | 50 +++++++++++++++++++++++++++++++++---------- tests/test-ovn.c | 11 +++++----- utilities/ovn-trace.c | 6 ++++-- 5 files changed, 57 insertions(+), 23 deletions(-) diff --git a/controller/lflow.c b/controller/lflow.c index c2f939f90f..c6e1586283 100644 --- a/controller/lflow.c +++ b/controller/lflow.c @@ -684,7 +684,8 @@ consider_logical_flow(const struct sbrec_logical_flow *lflow, .lflow = lflow, .lfrr = l_ctx_out->lfrr }; - expr = expr_simplify(expr, is_chassis_resident_cb, &cond_aux); + expr = expr_simplify(expr); + expr = expr_evaluate_condition(expr, is_chassis_resident_cb, &cond_aux); expr = expr_normalize(expr); uint32_t n_conjs = expr_to_matches(expr, lookup_port_cb, &aux, &matches); diff --git a/include/ovn/expr.h b/include/ovn/expr.h index b34fb0e813..ed7b054144 100644 --- a/include/ovn/expr.h +++ b/include/ovn/expr.h @@ -432,10 +432,12 @@ void expr_destroy(struct expr *); struct expr *expr_annotate(struct expr *, const struct shash *symtab, char **errorp); -struct expr *expr_simplify(struct expr *, - bool (*is_chassis_resident)(const void *c_aux, - const char *port_name), - const void *c_aux); +struct expr *expr_simplify(struct expr *); +struct expr *expr_evaluate_condition( + struct expr *, + bool (*is_chassis_resident)(const void *c_aux, + const char *port_name), + const void *c_aux); struct expr *expr_normalize(struct expr *); bool expr_honors_invariants(const struct expr *); diff --git a/lib/expr.c b/lib/expr.c index 6fb96757ad..bff48dfd20 100644 --- a/lib/expr.c +++ b/lib/expr.c @@ -2063,10 +2063,10 @@ expr_simplify_relational(struct expr *expr) /* Resolves condition and replaces the expression with a boolean. */ static struct expr * -expr_simplify_condition(struct expr *expr, - bool (*is_chassis_resident)(const void *c_aux, +expr_evaluate_condition__(struct expr *expr, + bool (*is_chassis_resident)(const void *c_aux, const char *port_name), - const void *c_aux) + const void *c_aux) { bool result; @@ -2084,13 +2084,41 @@ expr_simplify_condition(struct expr *expr, return expr_create_boolean(result); } +struct expr * +expr_evaluate_condition(struct expr *expr, + bool (*is_chassis_resident)(const void *c_aux, + const char *port_name), + const void *c_aux) +{ + struct expr *sub, *next; + + switch (expr->type) { + case EXPR_T_AND: + case EXPR_T_OR: + LIST_FOR_EACH_SAFE (sub, next, node, &expr->andor) { + ovs_list_remove(&sub->node); + struct expr *e = expr_evaluate_condition(sub, is_chassis_resident, + c_aux); + e = expr_fix(e); + expr_insert_andor(expr, next, e); + } + return expr_fix(expr); + + case EXPR_T_CONDITION: + return expr_evaluate_condition__(expr, is_chassis_resident, c_aux); + + case EXPR_T_CMP: + case EXPR_T_BOOLEAN: + return expr; + } + + OVS_NOT_REACHED(); +} + /* Takes ownership of 'expr' and returns an equivalent expression whose * EXPR_T_CMP nodes use only tests for equality (EXPR_R_EQ). */ struct expr * -expr_simplify(struct expr *expr, - bool (*is_chassis_resident)(const void *c_aux, - const char *port_name), - const void *c_aux) +expr_simplify(struct expr *expr) { struct expr *sub, *next; @@ -2105,8 +2133,7 @@ expr_simplify(struct expr *expr, case EXPR_T_OR: LIST_FOR_EACH_SAFE (sub, next, node, &expr->andor) { ovs_list_remove(&sub->node); - expr_insert_andor(expr, next, - expr_simplify(sub, is_chassis_resident, c_aux)); + expr_insert_andor(expr, next, expr_simplify(sub)); } return expr_fix(expr); @@ -2114,7 +2141,7 @@ expr_simplify(struct expr *expr, return expr; case EXPR_T_CONDITION: - return expr_simplify_condition(expr, is_chassis_resident, c_aux); + return expr; } OVS_NOT_REACHED(); } @@ -3397,7 +3424,8 @@ expr_parse_microflow__(struct lexer *lexer, struct ds annotated = DS_EMPTY_INITIALIZER; expr_format(e, &annotated); - e = expr_simplify(e, microflow_is_chassis_resident_cb, NULL); + e = expr_simplify(e); + e = expr_evaluate_condition(e, microflow_is_chassis_resident_cb, NULL); e = expr_normalize(e); struct match m = MATCH_CATCHALL_INITIALIZER; diff --git a/tests/test-ovn.c b/tests/test-ovn.c index ba628288bb..544fee4c87 100644 --- a/tests/test-ovn.c +++ b/tests/test-ovn.c @@ -312,8 +312,9 @@ test_parse_expr__(int steps) } if (!error) { if (steps > 1) { - expr = expr_simplify(expr, is_chassis_resident_cb, - &ports); + expr = expr_simplify(expr); + expr = expr_evaluate_condition(expr, is_chassis_resident_cb, + &ports); } if (steps > 2) { expr = expr_normalize(expr); @@ -914,9 +915,9 @@ test_tree_shape_exhaustively(struct expr *expr, struct shash *symtab, exit(EXIT_FAILURE); } } else if (operation >= OP_SIMPLIFY) { - modified = expr_simplify(expr_clone(expr), - tree_shape_is_chassis_resident_cb, - NULL); + modified = expr_simplify(expr_clone(expr)); + modified = expr_evaluate_condition( + expr_clone(modified), tree_shape_is_chassis_resident_cb, NULL); ovs_assert(expr_honors_invariants(modified)); if (operation >= OP_NORMALIZE) { diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c index 50a32b7149..39839cb709 100644 --- a/utilities/ovn-trace.c +++ b/utilities/ovn-trace.c @@ -931,8 +931,10 @@ read_flows(void) continue; } if (match) { - match = expr_simplify(match, ovntrace_is_chassis_resident, - NULL); + match = expr_simplify(match); + match = expr_evaluate_condition(match, + ovntrace_is_chassis_resident, + NULL); } struct ovntrace_flow *flow = xzalloc(sizeof *flow); From patchwork Wed Sep 9 10:09:58 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Numan Siddique X-Patchwork-Id: 1360534 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.133; helo=hemlock.osuosl.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Authentication-Results: ozlabs.org; dmarc=none (p=none dis=none) header.from=ovn.org Received: from hemlock.osuosl.org (smtp2.osuosl.org [140.211.166.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 4Bmd7K1bxvz9sTN for ; Wed, 9 Sep 2020 20:10:21 +1000 (AEST) Received: from localhost (localhost [127.0.0.1]) by hemlock.osuosl.org (Postfix) with ESMTP id 732708756F; Wed, 9 Sep 2020 10:10:19 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from hemlock.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id weVOk7gjrqTZ; Wed, 9 Sep 2020 10:10:17 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by hemlock.osuosl.org (Postfix) with ESMTP id 25053875A3; Wed, 9 Sep 2020 10:10:17 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id 0D79FC0859; Wed, 9 Sep 2020 10:10:17 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@lists.linuxfoundation.org Received: from fraxinus.osuosl.org (smtp4.osuosl.org [140.211.166.137]) by lists.linuxfoundation.org (Postfix) with ESMTP id 7A39DC0891 for ; Wed, 9 Sep 2020 10:10:16 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by fraxinus.osuosl.org (Postfix) with ESMTP id 65B768705B for ; Wed, 9 Sep 2020 10:10:16 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from fraxinus.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id YuwgAaenOT8S for ; Wed, 9 Sep 2020 10:10:11 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net [217.70.183.198]) by fraxinus.osuosl.org (Postfix) with ESMTPS id 26CD187096 for ; Wed, 9 Sep 2020 10:10:06 +0000 (UTC) X-Originating-IP: 27.7.129.187 Received: from nusiddiq.home.org.com (unknown [27.7.129.187]) (Authenticated sender: numans@ovn.org) by relay6-d.mail.gandi.net (Postfix) with ESMTPSA id EA702C000E; Wed, 9 Sep 2020 10:10:02 +0000 (UTC) From: numans@ovn.org To: dev@openvswitch.org Date: Wed, 9 Sep 2020 15:39:58 +0530 Message-Id: <20200909100958.649186-1-numans@ovn.org> X-Mailer: git-send-email 2.26.2 In-Reply-To: <20200909100903.648778-1-numans@ovn.org> References: <20200909100903.648778-1-numans@ovn.org> MIME-Version: 1.0 Subject: [ovs-dev] [PATCH ovn v2 4/4] ovn-controller: Cache logical flow expr matches. 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" From: Numan Siddique This patch is a second attempt in caching the expr tree of logical flows. Earlier attemp [1] was reverted. In this approach, we do the following - If a logical flow match has any port group/address set references, we don't cache expr match or expr tree. - If a logical flow match doesn't have a port group/address set reference and expr condition match - is_chassis_resident, we cache the expr matches (which is hmap of struct expr_match *). - If a logical flow match has expr condition match - is_chassis_resident we cache the simplified 'expr' tree. Caching is configurable and can be disabled if desired. The second patch of this series added this support. In my testing, a complete lflow_run() took around 5 seconds for around 90k logical flows, where as it took around 2 seconds with caching. Signed-off-by: Numan Siddique --- controller/lflow.c | 459 ++++++++++++++++++++++++++++-------------- include/ovn/expr.h | 2 +- lib/expr.c | 17 +- tests/test-ovn.c | 5 +- utilities/ovn-trace.c | 2 +- 5 files changed, 327 insertions(+), 158 deletions(-) diff --git a/controller/lflow.c b/controller/lflow.c index c6e1586283..59b32ab803 100644 --- a/controller/lflow.c +++ b/controller/lflow.c @@ -269,14 +269,34 @@ lflow_resource_destroy_lflow(struct lflow_resource_ref *lfrr, free(lfrn); } +enum lflow_cache_type { + LCACHE_T_NO_CACHE, + LCACHE_T_MATCHES, + LCACHE_T_EXPR, +}; + /* Represents an lflow cache which * - stores the conjunction id offset if the lflow matches * results in conjunctive OpenvSwitch flows. + * + * - Caches + * (1) Nothing if the logical flow has port group/address set references. + * (2) expr tree if the logical flow has is_chassis_resident() match. + * (3) expr matches if (1) and (2) are false. */ struct lflow_cache { struct hmap_node node; struct uuid lflow_uuid; /* key */ uint32_t conj_id_ofs; + + enum lflow_cache_type type; + union { + struct { + struct hmap *expr_matches; + size_t n_conjs; + }; + struct expr *expr; + }; }; static struct lflow_cache * @@ -286,6 +306,7 @@ lflow_cache_add(struct hmap *lflow_cache_map, struct lflow_cache *lc = xmalloc(sizeof *lc); lc->lflow_uuid = lflow->header_.uuid; lc->conj_id_ofs = 0; + lc->type = LCACHE_T_NO_CACHE; hmap_insert(lflow_cache_map, &lc->node, uuid_hash(&lc->lflow_uuid)); return lc; } @@ -312,6 +333,12 @@ lflow_cache_delete(struct hmap *lflow_cache_map, struct lflow_cache *lc = lflow_cache_get(lflow_cache_map, lflow); if (lc) { hmap_remove(lflow_cache_map, &lc->node); + if (lc->type == LCACHE_T_MATCHES) { + expr_matches_destroy(lc->expr_matches); + free(lc->expr_matches); + } else if (lc->type == LCACHE_T_EXPR) { + expr_destroy(lc->expr); + } free(lc); } } @@ -327,6 +354,12 @@ lflow_cache_destroy(struct hmap *lflow_cache_map) { struct lflow_cache *lc; HMAP_FOR_EACH_POP (lc, node, lflow_cache_map) { + if (lc->type == LCACHE_T_MATCHES) { + expr_matches_destroy(lc->expr_matches); + free(lc->expr_matches); + } else if (lc->type == LCACHE_T_EXPR) { + expr_destroy(lc->expr); + } free(lc); } @@ -567,6 +600,159 @@ update_conj_id_ofs(uint32_t *conj_id_ofs, uint32_t n_conjs) return true; } +static void +add_matches_to_flow_table(const struct sbrec_logical_flow *lflow, + struct hmap *matches, size_t conj_id_ofs, + uint8_t ptable, uint8_t output_ptable, + struct ofpbuf *ovnacts, + bool ingress, struct lflow_ctx_in *l_ctx_in, + struct lflow_ctx_out *l_ctx_out) +{ + struct lookup_port_aux aux = { + .sbrec_multicast_group_by_name_datapath + = l_ctx_in->sbrec_multicast_group_by_name_datapath, + .sbrec_port_binding_by_name = l_ctx_in->sbrec_port_binding_by_name, + .dp = lflow->logical_datapath + }; + + /* Encode OVN logical actions into OpenFlow. */ + uint64_t ofpacts_stub[1024 / 8]; + struct ofpbuf ofpacts = OFPBUF_STUB_INITIALIZER(ofpacts_stub); + struct ovnact_encode_params ep = { + .lookup_port = lookup_port_cb, + .tunnel_ofport = tunnel_ofport_cb, + .aux = &aux, + .is_switch = datapath_is_switch(lflow->logical_datapath), + .group_table = l_ctx_out->group_table, + .meter_table = l_ctx_out->meter_table, + .lflow_uuid = lflow->header_.uuid, + + .pipeline = ingress ? OVNACT_P_INGRESS : OVNACT_P_EGRESS, + .ingress_ptable = OFTABLE_LOG_INGRESS_PIPELINE, + .egress_ptable = OFTABLE_LOG_EGRESS_PIPELINE, + .output_ptable = output_ptable, + .mac_bind_ptable = OFTABLE_MAC_BINDING, + .mac_lookup_ptable = OFTABLE_MAC_LOOKUP, + }; + ovnacts_encode(ovnacts->data, ovnacts->size, &ep, &ofpacts); + + struct expr_match *m; + HMAP_FOR_EACH (m, hmap_node, matches) { + match_set_metadata(&m->match, + htonll(lflow->logical_datapath->tunnel_key)); + if (m->match.wc.masks.conj_id) { + m->match.flow.conj_id += conj_id_ofs; + } + if (datapath_is_switch(lflow->logical_datapath)) { + unsigned int reg_index + = (ingress ? MFF_LOG_INPORT : MFF_LOG_OUTPORT) - MFF_REG0; + int64_t port_id = m->match.flow.regs[reg_index]; + if (port_id) { + int64_t dp_id = lflow->logical_datapath->tunnel_key; + char buf[16]; + get_unique_lport_key(dp_id, port_id, buf, sizeof(buf)); + lflow_resource_add(l_ctx_out->lfrr, REF_TYPE_PORTBINDING, buf, + &lflow->header_.uuid); + if (!sset_contains(l_ctx_in->local_lport_ids, buf)) { + VLOG_DBG("lflow "UUID_FMT + " port %s in match is not local, skip", + UUID_ARGS(&lflow->header_.uuid), + buf); + continue; + } + } + } + if (!m->n) { + ofctrl_add_flow(l_ctx_out->flow_table, ptable, lflow->priority, + lflow->header_.uuid.parts[0], &m->match, &ofpacts, + &lflow->header_.uuid); + } else { + uint64_t conj_stubs[64 / 8]; + struct ofpbuf conj; + + ofpbuf_use_stub(&conj, conj_stubs, sizeof conj_stubs); + for (int i = 0; i < m->n; i++) { + const struct cls_conjunction *src = &m->conjunctions[i]; + struct ofpact_conjunction *dst; + + dst = ofpact_put_CONJUNCTION(&conj); + dst->id = src->id + conj_id_ofs; + dst->clause = src->clause; + dst->n_clauses = src->n_clauses; + } + + ofctrl_add_or_append_flow(l_ctx_out->flow_table, ptable, + lflow->priority, 0, + &m->match, &conj, &lflow->header_.uuid); + ofpbuf_uninit(&conj); + } + } + + ofpbuf_uninit(&ofpacts); +} + +/* Converts the actions and returns the simplified expre tree. + * The caller should evaluate the conditions and normalize the + * expr tree. */ +static struct expr * +convert_acts_to_expr(const struct sbrec_logical_flow *lflow, + struct expr *prereqs, + struct lflow_ctx_in *l_ctx_in, + struct lflow_ctx_out *l_ctx_out, + bool *pg_addr_set_ref, char **errorp) +{ + struct sset addr_sets_ref = SSET_INITIALIZER(&addr_sets_ref); + struct sset port_groups_ref = SSET_INITIALIZER(&port_groups_ref); + char *error; + + struct expr *e = expr_parse_string(lflow->match, &symtab, + l_ctx_in->addr_sets, + l_ctx_in->port_groups, &addr_sets_ref, + &port_groups_ref, + lflow->logical_datapath->tunnel_key, + &error); + const char *addr_set_name; + SSET_FOR_EACH (addr_set_name, &addr_sets_ref) { + lflow_resource_add(l_ctx_out->lfrr, REF_TYPE_ADDRSET, addr_set_name, + &lflow->header_.uuid); + } + const char *port_group_name; + SSET_FOR_EACH (port_group_name, &port_groups_ref) { + lflow_resource_add(l_ctx_out->lfrr, REF_TYPE_PORTGROUP, + port_group_name, &lflow->header_.uuid); + } + + if (!error) { + if (prereqs) { + e = expr_combine(EXPR_T_AND, e, prereqs); + prereqs = NULL; + } + e = expr_annotate(e, &symtab, &error); + } + if (error) { + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); + VLOG_WARN_RL(&rl, "error parsing match \"%s\": %s", + lflow->match, error); + sset_destroy(&addr_sets_ref); + sset_destroy(&port_groups_ref); + *errorp = error; + return NULL; + } + + e = expr_simplify(e); + + if (pg_addr_set_ref) { + *pg_addr_set_ref = (!sset_is_empty(&port_groups_ref) || + !sset_is_empty(&addr_sets_ref)); + } + + sset_destroy(&addr_sets_ref); + sset_destroy(&port_groups_ref); + + *errorp = NULL; + return e; +} + static bool consider_logical_flow(const struct sbrec_logical_flow *lflow, struct hmap *dhcp_opts, struct hmap *dhcpv6_opts, @@ -629,48 +815,6 @@ consider_logical_flow(const struct sbrec_logical_flow *lflow, return true; } - /* Translate OVN match into table of OpenFlow matches. */ - struct hmap matches; - struct expr *expr; - - struct sset addr_sets_ref = SSET_INITIALIZER(&addr_sets_ref); - struct sset port_groups_ref = SSET_INITIALIZER(&port_groups_ref); - expr = expr_parse_string(lflow->match, &symtab, l_ctx_in->addr_sets, - l_ctx_in->port_groups, - &addr_sets_ref, &port_groups_ref, - lflow->logical_datapath->tunnel_key, - &error); - const char *addr_set_name; - SSET_FOR_EACH (addr_set_name, &addr_sets_ref) { - lflow_resource_add(l_ctx_out->lfrr, REF_TYPE_ADDRSET, addr_set_name, - &lflow->header_.uuid); - } - const char *port_group_name; - SSET_FOR_EACH (port_group_name, &port_groups_ref) { - lflow_resource_add(l_ctx_out->lfrr, REF_TYPE_PORTGROUP, - port_group_name, &lflow->header_.uuid); - } - sset_destroy(&addr_sets_ref); - sset_destroy(&port_groups_ref); - - if (!error) { - if (prereqs) { - expr = expr_combine(EXPR_T_AND, expr, prereqs); - prereqs = NULL; - } - expr = expr_annotate(expr, &symtab, &error); - } - if (error) { - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); - VLOG_WARN_RL(&rl, "error parsing match \"%s\": %s", - lflow->match, error); - expr_destroy(prereqs); - free(error); - ovnacts_free(ovnacts.data, ovnacts.size); - ofpbuf_uninit(&ovnacts); - return true; - } - struct lookup_port_aux aux = { .sbrec_multicast_group_by_name_datapath = l_ctx_in->sbrec_multicast_group_by_name_datapath, @@ -682,131 +826,150 @@ consider_logical_flow(const struct sbrec_logical_flow *lflow, .chassis = l_ctx_in->chassis, .active_tunnels = l_ctx_in->active_tunnels, .lflow = lflow, - .lfrr = l_ctx_out->lfrr + .lfrr = l_ctx_out->lfrr, }; - expr = expr_simplify(expr); - expr = expr_evaluate_condition(expr, is_chassis_resident_cb, &cond_aux); - expr = expr_normalize(expr); - uint32_t n_conjs = expr_to_matches(expr, lookup_port_cb, &aux, - &matches); - expr_destroy(expr); - if (hmap_is_empty(&matches)) { - VLOG_DBG("lflow "UUID_FMT" matches are empty, skip", - UUID_ARGS(&lflow->header_.uuid)); + struct expr *expr = NULL; + if (!l_ctx_out->lflow_cache_map) { + /* Caching is disabled. */ + expr = convert_acts_to_expr(lflow, prereqs, l_ctx_in, + l_ctx_out, NULL, &error); + if (error) { + expr_destroy(prereqs); + ovnacts_free(ovnacts.data, ovnacts.size); + ofpbuf_uninit(&ovnacts); + free(error); + return true; + } + + expr = expr_evaluate_condition(expr, is_chassis_resident_cb, &cond_aux, + NULL); + expr = expr_normalize(expr); + struct hmap matches = HMAP_INITIALIZER(&matches); + uint32_t n_conjs = expr_to_matches(expr, lookup_port_cb, &aux, + &matches); + expr_destroy(expr); + if (hmap_is_empty(&matches)) { + VLOG_DBG("lflow "UUID_FMT" matches are empty, skip", + UUID_ARGS(&lflow->header_.uuid)); + ovnacts_free(ovnacts.data, ovnacts.size); + ofpbuf_uninit(&ovnacts); + expr_matches_destroy(&matches); + return true; + } + + add_matches_to_flow_table(lflow, &matches, *l_ctx_out->conj_id_ofs, + ptable, output_ptable, &ovnacts, ingress, + l_ctx_in, l_ctx_out); + ovnacts_free(ovnacts.data, ovnacts.size); ofpbuf_uninit(&ovnacts); expr_matches_destroy(&matches); - return true; + return update_conj_id_ofs(l_ctx_out->conj_id_ofs, n_conjs); } - /* Encode OVN logical actions into OpenFlow. */ - uint64_t ofpacts_stub[1024 / 8]; - struct ofpbuf ofpacts = OFPBUF_STUB_INITIALIZER(ofpacts_stub); - struct ovnact_encode_params ep = { - .lookup_port = lookup_port_cb, - .tunnel_ofport = tunnel_ofport_cb, - .aux = &aux, - .is_switch = datapath_is_switch(ldp), - .group_table = l_ctx_out->group_table, - .meter_table = l_ctx_out->meter_table, - .lflow_uuid = lflow->header_.uuid, + /* Caching is enabled. */ + struct lflow_cache *lc = + lflow_cache_get(l_ctx_out->lflow_cache_map, lflow); - .pipeline = ingress ? OVNACT_P_INGRESS : OVNACT_P_EGRESS, - .ingress_ptable = OFTABLE_LOG_INGRESS_PIPELINE, - .egress_ptable = OFTABLE_LOG_EGRESS_PIPELINE, - .output_ptable = output_ptable, - .mac_bind_ptable = OFTABLE_MAC_BINDING, - .mac_lookup_ptable = OFTABLE_MAC_LOOKUP, - }; - ovnacts_encode(ovnacts.data, ovnacts.size, &ep, &ofpacts); - ovnacts_free(ovnacts.data, ovnacts.size); - ofpbuf_uninit(&ovnacts); - - uint32_t conj_id_ofs = *l_ctx_out->conj_id_ofs; - if (n_conjs) { - if (l_ctx_out->lflow_cache_map) { - struct lflow_cache *lc = - lflow_cache_get(l_ctx_out->lflow_cache_map, lflow); - if (!lc) { - lc = lflow_cache_add(l_ctx_out->lflow_cache_map, lflow); - } + if (lc && lc->type == LCACHE_T_MATCHES) { + /* 'matches' is cached. No need to do expr parsing. + * Add matches to flow table and return. */ + add_matches_to_flow_table(lflow, lc->expr_matches, lc->conj_id_ofs, + ptable, output_ptable, &ovnacts, ingress, + l_ctx_in, l_ctx_out); + ovnacts_free(ovnacts.data, ovnacts.size); + ofpbuf_uninit(&ovnacts); + expr_destroy(prereqs); + return true; + } - if (!lc->conj_id_ofs) { - lc->conj_id_ofs = *l_ctx_out->conj_id_ofs; - if (!update_conj_id_ofs(l_ctx_out->conj_id_ofs, n_conjs)) { - lc->conj_id_ofs = 0; - expr_matches_destroy(&matches); - return false; - } - } + if (!lc) { + /* Create the lflow_cache for the lflow. */ + lc = lflow_cache_add(l_ctx_out->lflow_cache_map, lflow); + } - conj_id_ofs = lc->conj_id_ofs; - } else { - /* lflow caching is disabled. */ - if (!update_conj_id_ofs(l_ctx_out->conj_id_ofs, n_conjs)) { - expr_matches_destroy(&matches); - return false; - } - } + if (lc && lc->type == LCACHE_T_EXPR) { + expr = lc->expr; } - /* Prepare the OpenFlow matches for adding to the flow table. */ - struct expr_match *m; - HMAP_FOR_EACH (m, hmap_node, &matches) { - match_set_metadata(&m->match, - htonll(lflow->logical_datapath->tunnel_key)); - if (m->match.wc.masks.conj_id) { - m->match.flow.conj_id += conj_id_ofs; - } - if (datapath_is_switch(ldp)) { - unsigned int reg_index - = (ingress ? MFF_LOG_INPORT : MFF_LOG_OUTPORT) - MFF_REG0; - int64_t port_id = m->match.flow.regs[reg_index]; - if (port_id) { - int64_t dp_id = lflow->logical_datapath->tunnel_key; - char buf[16]; - get_unique_lport_key(dp_id, port_id, buf, sizeof(buf)); - lflow_resource_add(l_ctx_out->lfrr, REF_TYPE_PORTBINDING, buf, - &lflow->header_.uuid); - if (!sset_contains(l_ctx_in->local_lport_ids, buf)) { - VLOG_DBG("lflow "UUID_FMT - " port %s in match is not local, skip", - UUID_ARGS(&lflow->header_.uuid), - buf); - continue; - } - } + bool pg_addr_set_ref = false; + if (!expr) { + expr = convert_acts_to_expr(lflow, prereqs, l_ctx_in, l_ctx_out, + &pg_addr_set_ref, &error); + if (error) { + expr_destroy(prereqs); + ovnacts_free(ovnacts.data, ovnacts.size); + ofpbuf_uninit(&ovnacts); + free(error); + return true; } - if (!m->n) { - ofctrl_add_flow(l_ctx_out->flow_table, ptable, lflow->priority, - lflow->header_.uuid.parts[0], &m->match, &ofpacts, - &lflow->header_.uuid); - } else { - uint64_t conj_stubs[64 / 8]; - struct ofpbuf conj; + } else { + expr_destroy(prereqs); + } - ofpbuf_use_stub(&conj, conj_stubs, sizeof conj_stubs); - for (int i = 0; i < m->n; i++) { - const struct cls_conjunction *src = &m->conjunctions[i]; - struct ofpact_conjunction *dst; + ovs_assert(lc && expr); - dst = ofpact_put_CONJUNCTION(&conj); - dst->id = src->id + conj_id_ofs; - dst->clause = src->clause; - dst->n_clauses = src->n_clauses; - } + /* Cache the 'expr' only if the lflow doesn't reference a port group and + * address set. */ + if (!pg_addr_set_ref) { + /* Note: If the expr doesn't have 'is_chassis_resident, then the + * type will be set to LCACHE_T_MATCHES and 'matches' will be + * cached instead. See below. */ + lc->type = LCACHE_T_EXPR; + lc->expr = expr; + } - ofctrl_add_or_append_flow(l_ctx_out->flow_table, ptable, - lflow->priority, 0, - &m->match, &conj, &lflow->header_.uuid); - ofpbuf_uninit(&conj); + if (lc->type == LCACHE_T_EXPR) { + expr = expr_clone(lc->expr); + } + + bool is_cr_cond_present = false; + expr = expr_evaluate_condition(expr, is_chassis_resident_cb, &cond_aux, + &is_cr_cond_present); + expr = expr_normalize(expr); + struct hmap *matches = xmalloc(sizeof *matches); + uint32_t n_conjs = expr_to_matches(expr, lookup_port_cb, &aux, + matches); + expr_destroy(expr); + if (hmap_is_empty(matches)) { + VLOG_DBG("lflow "UUID_FMT" matches are empty, skip", + UUID_ARGS(&lflow->header_.uuid)); + ovnacts_free(ovnacts.data, ovnacts.size); + ofpbuf_uninit(&ovnacts); + expr_matches_destroy(matches); + free(matches); + return true; + } + + if (n_conjs && !lc->conj_id_ofs) { + lc->conj_id_ofs = *l_ctx_out->conj_id_ofs; + if (!update_conj_id_ofs(l_ctx_out->conj_id_ofs, n_conjs)) { + lc->conj_id_ofs = 0; + expr_matches_destroy(matches); + free(matches); + return false; } } - /* Clean up. */ - expr_matches_destroy(&matches); - ofpbuf_uninit(&ofpacts); + /* Encode OVN logical actions into OpenFlow. */ + add_matches_to_flow_table(lflow, matches, lc->conj_id_ofs, + ptable, output_ptable, &ovnacts, ingress, + l_ctx_in, l_ctx_out); + + if (!is_cr_cond_present && lc->type == LCACHE_T_EXPR) { + /* If 'is_chassis_resident' match is not present, then cache + * 'matches'. */ + expr_destroy(lc->expr); + lc->type = LCACHE_T_MATCHES; + lc->expr_matches = matches; + } + + if (lc->type != LCACHE_T_MATCHES) { + expr_matches_destroy(matches); + free(matches); + } + return true; } diff --git a/include/ovn/expr.h b/include/ovn/expr.h index ed7b054144..ec8e95b2d5 100644 --- a/include/ovn/expr.h +++ b/include/ovn/expr.h @@ -437,7 +437,7 @@ struct expr *expr_evaluate_condition( struct expr *, bool (*is_chassis_resident)(const void *c_aux, const char *port_name), - const void *c_aux); + const void *c_aux, bool *condition_present); struct expr *expr_normalize(struct expr *); bool expr_honors_invariants(const struct expr *); diff --git a/lib/expr.c b/lib/expr.c index bff48dfd20..3877cf9d45 100644 --- a/lib/expr.c +++ b/lib/expr.c @@ -2065,14 +2065,17 @@ expr_simplify_relational(struct expr *expr) static struct expr * expr_evaluate_condition__(struct expr *expr, bool (*is_chassis_resident)(const void *c_aux, - const char *port_name), - const void *c_aux) + const char *port_name), + const void *c_aux, bool *condition_present) { bool result; switch (expr->cond.type) { case EXPR_COND_CHASSIS_RESIDENT: result = is_chassis_resident(c_aux, expr->cond.string); + if (condition_present != NULL) { + *condition_present = true; + } break; default: @@ -2088,7 +2091,7 @@ struct expr * expr_evaluate_condition(struct expr *expr, bool (*is_chassis_resident)(const void *c_aux, const char *port_name), - const void *c_aux) + const void *c_aux, bool *condition_present) { struct expr *sub, *next; @@ -2098,14 +2101,15 @@ expr_evaluate_condition(struct expr *expr, LIST_FOR_EACH_SAFE (sub, next, node, &expr->andor) { ovs_list_remove(&sub->node); struct expr *e = expr_evaluate_condition(sub, is_chassis_resident, - c_aux); + c_aux, condition_present); e = expr_fix(e); expr_insert_andor(expr, next, e); } return expr_fix(expr); case EXPR_T_CONDITION: - return expr_evaluate_condition__(expr, is_chassis_resident, c_aux); + return expr_evaluate_condition__(expr, is_chassis_resident, c_aux, + condition_present); case EXPR_T_CMP: case EXPR_T_BOOLEAN: @@ -3425,7 +3429,8 @@ expr_parse_microflow__(struct lexer *lexer, expr_format(e, &annotated); e = expr_simplify(e); - e = expr_evaluate_condition(e, microflow_is_chassis_resident_cb, NULL); + e = expr_evaluate_condition(e, microflow_is_chassis_resident_cb, + NULL, NULL); e = expr_normalize(e); struct match m = MATCH_CATCHALL_INITIALIZER; diff --git a/tests/test-ovn.c b/tests/test-ovn.c index 544fee4c87..d94ab025d9 100644 --- a/tests/test-ovn.c +++ b/tests/test-ovn.c @@ -314,7 +314,7 @@ test_parse_expr__(int steps) if (steps > 1) { expr = expr_simplify(expr); expr = expr_evaluate_condition(expr, is_chassis_resident_cb, - &ports); + &ports, NULL); } if (steps > 2) { expr = expr_normalize(expr); @@ -917,7 +917,8 @@ test_tree_shape_exhaustively(struct expr *expr, struct shash *symtab, } else if (operation >= OP_SIMPLIFY) { modified = expr_simplify(expr_clone(expr)); modified = expr_evaluate_condition( - expr_clone(modified), tree_shape_is_chassis_resident_cb, NULL); + expr_clone(modified), tree_shape_is_chassis_resident_cb, + NULL, NULL); ovs_assert(expr_honors_invariants(modified)); if (operation >= OP_NORMALIZE) { diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c index 39839cb709..33afc4f43c 100644 --- a/utilities/ovn-trace.c +++ b/utilities/ovn-trace.c @@ -934,7 +934,7 @@ read_flows(void) match = expr_simplify(match); match = expr_evaluate_condition(match, ovntrace_is_chassis_resident, - NULL); + NULL, NULL); } struct ovntrace_flow *flow = xzalloc(sizeof *flow);