From patchwork Tue Feb 25 15:50:01 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Numan Siddique X-Patchwork-Id: 1244375 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 48Rk0l3ZJQz9sPk for ; Wed, 26 Feb 2020 02:50:31 +1100 (AEDT) Received: from localhost (localhost [127.0.0.1]) by fraxinus.osuosl.org (Postfix) with ESMTP id A469E85F63; Tue, 25 Feb 2020 15:50:29 +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 rEvqEF6L2s97; Tue, 25 Feb 2020 15:50:25 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by fraxinus.osuosl.org (Postfix) with ESMTP id 6106D8502B; Tue, 25 Feb 2020 15:50:25 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id 4E7D3C18DA; Tue, 25 Feb 2020 15:50:25 +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 D2EDDC0177 for ; Tue, 25 Feb 2020 15:50:23 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by fraxinus.osuosl.org (Postfix) with ESMTP id BD1698502B for ; Tue, 25 Feb 2020 15:50:23 +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 mBoSAAjwOtvE for ; Tue, 25 Feb 2020 15:50:19 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net [217.70.183.195]) by fraxinus.osuosl.org (Postfix) with ESMTPS id 148CD85F7D for ; Tue, 25 Feb 2020 15:50:18 +0000 (UTC) X-Originating-IP: 116.75.77.250 Received: from nummac.local (unknown [116.75.77.250]) (Authenticated sender: numans@ovn.org) by relay3-d.mail.gandi.net (Postfix) with ESMTPSA id E1AB860016; Tue, 25 Feb 2020 15:50:14 +0000 (UTC) From: numans@ovn.org To: dev@openvswitch.org Date: Tue, 25 Feb 2020 21:20:01 +0530 Message-Id: <20200225155001.393465-1-numans@ovn.org> X-Mailer: git-send-email 2.24.1 MIME-Version: 1.0 Cc: Jakub Libosvar Subject: [ovs-dev] [PATCH ovn] ovn-controller: Revert lflow expr caching 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 reverts the below patches which added lflow expr caching supported and follow up patches which fixed few issues. With the present lflow expr caching we still have issues with logical flows referencing port groups/address sets. If a port group/address set change happens along with the port binding change in the same transaction, ovn-controller may trigger full recompute and the lflow expr cache storing the address sets and port groups would be invalid resulting in wrong OF flows. This patch reverts the below patches [1] which added lflow expr caching supported and follow up patches which fixed few issues for now. These patches will be submitted again addressing all the issues and the support to periodically clear the expr cache which is missing now. Reverts 99e3a145927("expr: Evaluate the condition expression in a separate step.") 8795bec737b("ovn-controller: Cache logical flow expr tree for each lflow.) 672508f6368("ovn-controller: Fix memory issues due to lflow expr caching.) 06ccb8d1dff("Save the addr set and port groups in lflow expr cache") Reported-by: Jakub Libosvar Signed-off-by: Numan Siddique Acked-by: Mark Michelson --- controller/lflow.c | 216 +++++++----------------------------- controller/ovn-controller.c | 6 - include/ovn/expr.h | 10 +- lib/expr.c | 50 ++------- tests/test-ovn.c | 11 +- utilities/ovn-trace.c | 6 +- 6 files changed, 63 insertions(+), 236 deletions(-) diff --git a/controller/lflow.c b/controller/lflow.c index 55e6b8b0a..ee11fc617 100644 --- a/controller/lflow.c +++ b/controller/lflow.c @@ -66,7 +66,6 @@ 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 delete_expr_from_cache, struct lflow_ctx_in *l_ctx_in, struct lflow_ctx_out *l_ctx_out); @@ -256,75 +255,6 @@ lflow_resource_destroy_lflow(struct lflow_resource_ref *lfrr, free(lfrn); } -/* Represents expr tree for the lflow. We don't store the - * match in this structure because - - * - Whenever a flow match or action needs to be modified, - * ovn-northd deletes the existing flow in the logical_flow - * table and adds a new one. - * We may want to revisit this and store match incase the behavior - * of ovn-northd changes. - */ -struct lflow_expr { - struct hmap_node node; - struct uuid lflow_uuid; /* key */ - struct expr *expr; - struct sset addr_sets_ref; - struct sset port_groups_ref; -}; - -static void -lflow_expr_add(struct hmap *lflow_expr_cache, - const struct sbrec_logical_flow *lflow, - struct expr *lflow_expr, struct sset *addr_sets_ref, - struct sset *port_groups_ref) -{ - struct lflow_expr *le = xmalloc(sizeof *le); - le->lflow_uuid = lflow->header_.uuid; - le->expr = lflow_expr; - sset_clone(&le->addr_sets_ref, addr_sets_ref); - sset_clone(&le->port_groups_ref, port_groups_ref); - hmap_insert(lflow_expr_cache, &le->node, uuid_hash(&le->lflow_uuid)); -} - -static struct lflow_expr * -lflow_expr_get(struct hmap *lflow_expr_cache, - const struct sbrec_logical_flow *lflow) -{ - struct lflow_expr *le; - size_t hash = uuid_hash(&lflow->header_.uuid); - HMAP_FOR_EACH_WITH_HASH (le, node, hash, lflow_expr_cache) { - if (uuid_equals(&le->lflow_uuid, &lflow->header_.uuid)) { - return le; - } - } - - return NULL; -} - -static void -lflow_expr_delete(struct hmap *lflow_expr_cache, struct lflow_expr *le) -{ - hmap_remove(lflow_expr_cache, &le->node); - expr_destroy(le->expr); - sset_destroy(&le->addr_sets_ref); - sset_destroy(&le->port_groups_ref); - free(le); -} - -void -lflow_expr_destroy(struct hmap *lflow_expr_cache) -{ - struct lflow_expr *le; - HMAP_FOR_EACH_POP (le, node, lflow_expr_cache) { - expr_destroy(le->expr); - sset_destroy(&le->addr_sets_ref); - sset_destroy(&le->port_groups_ref); - free(le); - } - - hmap_destroy(lflow_expr_cache); -} - /* Adds the logical flows from the Logical_Flow table to flow tables. */ static void add_logical_flows(struct lflow_ctx_in *l_ctx_in, @@ -357,7 +287,7 @@ add_logical_flows(struct lflow_ctx_in *l_ctx_in, SBREC_LOGICAL_FLOW_TABLE_FOR_EACH (lflow, l_ctx_in->logical_flow_table) { if (!consider_logical_flow(lflow, &dhcp_opts, &dhcpv6_opts, - &nd_ra_opts, &controller_event_opts, false, + &nd_ra_opts, &controller_event_opts, l_ctx_in, l_ctx_out)) { static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5); VLOG_ERR_RL(&rl, "Conjunction id overflow when processing lflow " @@ -411,11 +341,6 @@ 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); - struct lflow_expr *le = - lflow_expr_get(l_ctx_out->lflow_expr_cache, lflow); - if (le) { - lflow_expr_delete(l_ctx_out->lflow_expr_cache, le); - } } } @@ -441,7 +366,7 @@ lflow_handle_changed_flows(struct lflow_ctx_in *l_ctx_in, UUID_ARGS(&lflow->header_.uuid)); if (!consider_logical_flow(lflow, &dhcp_opts, &dhcpv6_opts, &nd_ra_opts, &controller_event_opts, - true, l_ctx_in, l_ctx_out)) { + l_ctx_in, l_ctx_out)) { ret = false; break; } @@ -526,7 +451,7 @@ lflow_handle_changed_ref(enum ref_type ref_type, const char *ref_name, if (!consider_logical_flow(lflow, &dhcp_opts, &dhcpv6_opts, &nd_ra_opts, &controller_event_opts, - true, l_ctx_in, l_ctx_out)) { + l_ctx_in, l_ctx_out)) { ret = false; break; } @@ -557,31 +482,11 @@ update_conj_id_ofs(uint32_t *conj_id_ofs, uint32_t n_conjs) return true; } -static void -lflow_update_resource_refs(const struct sbrec_logical_flow *lflow, - struct sset *addr_sets_ref, - struct sset *port_groups_ref, - struct lflow_resource_ref *lfrr) -{ - const char *addr_set_name; - SSET_FOR_EACH (addr_set_name, addr_sets_ref) { - lflow_resource_add(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(lfrr, REF_TYPE_PORTGROUP, port_group_name, - &lflow->header_.uuid); - } -} - static bool 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 delete_expr_from_cache, struct lflow_ctx_in *l_ctx_in, struct lflow_ctx_out *l_ctx_out) { @@ -641,83 +546,59 @@ consider_logical_flow(const struct sbrec_logical_flow *lflow, /* Translate OVN match into table of OpenFlow matches. */ struct hmap matches; - struct expr *expr = NULL; + struct expr *expr; - struct lflow_expr *le = lflow_expr_get(l_ctx_out->lflow_expr_cache, lflow); - if (le) { - if (delete_expr_from_cache) { - lflow_expr_delete(l_ctx_out->lflow_expr_cache, le); - le = NULL; - } else { - expr = le->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, &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 (!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, &error); - /* Add the address set and port groups (if any) to the lflow - * references. */ - lflow_update_resource_refs(lflow, &addr_sets_ref, &port_groups_ref, - l_ctx_out->lfrr); - - if (!error) { - if (prereqs) { - expr = expr_combine(EXPR_T_AND, expr, prereqs); - prereqs = NULL; - } - expr = expr_annotate(expr, &symtab, &error); + if (!error) { + if (prereqs) { + expr = expr_combine(EXPR_T_AND, expr, prereqs); + prereqs = NULL; } - 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); - sset_destroy(&addr_sets_ref); - sset_destroy(&port_groups_ref); - return true; - } - - expr = expr_simplify(expr); - expr = expr_normalize(expr); - - lflow_expr_add(l_ctx_out->lflow_expr_cache, lflow, expr, - &addr_sets_ref, &port_groups_ref); - sset_destroy(&addr_sets_ref); - sset_destroy(&port_groups_ref); - } else { + 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); - /* Add the cached address set and port groups (if any) to the lflow - * references. */ - lflow_update_resource_refs(lflow, &le->addr_sets_ref, - &le->port_groups_ref, l_ctx_out->lfrr); + free(error); + ovnacts_free(ovnacts.data, ovnacts.size); + ofpbuf_uninit(&ovnacts); + return true; } - struct condition_aux cond_aux = { - .sbrec_port_binding_by_name = l_ctx_in->sbrec_port_binding_by_name, - .chassis = l_ctx_in->chassis, - .active_tunnels = l_ctx_in->active_tunnels, - }; - - expr = expr_clone(expr); - expr = expr_evaluate_condition(expr, is_chassis_resident_cb, &cond_aux); - 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 }; + struct condition_aux cond_aux = { + .sbrec_port_binding_by_name = l_ctx_in->sbrec_port_binding_by_name, + .chassis = l_ctx_in->chassis, + .active_tunnels = l_ctx_in->active_tunnels, + }; + expr = expr_simplify(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)) { @@ -954,21 +835,6 @@ 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 expr cache for these lflows, - * otherwise, they will not be deleted at all. */ - 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)) { - struct lflow_expr *le = - lflow_expr_get(l_ctx_out->lflow_expr_cache, lflow); - if (le) { - lflow_expr_delete(l_ctx_out->lflow_expr_cache, le); - } - } - } - 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, diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index cacaaa578..303e5708d 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -1240,9 +1240,6 @@ struct ed_type_flow_output { uint32_t conj_id_ofs; /* lflow resource cross reference */ struct lflow_resource_ref lflow_resource_ref; - - /* Cache of lflow expr tree. */ - struct hmap lflow_expr_cache; }; static void init_physical_ctx(struct engine_node *node, @@ -1380,7 +1377,6 @@ 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->lflow_expr_cache = &fo->lflow_expr_cache; l_ctx_out->conj_id_ofs = &fo->conj_id_ofs; } @@ -1395,7 +1391,6 @@ en_flow_output_init(struct engine_node *node OVS_UNUSED, ovn_extend_table_init(&data->meter_table); data->conj_id_ofs = 1; lflow_resource_init(&data->lflow_resource_ref); - hmap_init(&data->lflow_expr_cache); return data; } @@ -1407,7 +1402,6 @@ 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_expr_destroy(&flow_output_data->lflow_expr_cache); } static void diff --git a/include/ovn/expr.h b/include/ovn/expr.h index 00a021236..21bf51c22 100644 --- a/include/ovn/expr.h +++ b/include/ovn/expr.h @@ -404,12 +404,10 @@ void expr_destroy(struct expr *); struct expr *expr_annotate(struct expr *, const struct shash *symtab, char **errorp); -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_simplify(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 12557e3ca..78646a1af 100644 --- a/lib/expr.c +++ b/lib/expr.c @@ -2033,10 +2033,10 @@ expr_simplify_relational(struct expr *expr) /* Resolves condition and replaces the expression with a boolean. */ static struct expr * -expr_evaluate_condition__(struct expr *expr, - bool (*is_chassis_resident)(const void *c_aux, +expr_simplify_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; @@ -2054,41 +2054,13 @@ expr_evaluate_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) +expr_simplify(struct expr *expr, + bool (*is_chassis_resident)(const void *c_aux, + const char *port_name), + const void *c_aux) { struct expr *sub, *next; @@ -2103,7 +2075,8 @@ 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)); + expr_insert_andor(expr, next, + expr_simplify(sub, is_chassis_resident, c_aux)); } return expr_fix(expr); @@ -2111,7 +2084,7 @@ expr_simplify(struct expr *expr) return expr; case EXPR_T_CONDITION: - return expr; + return expr_simplify_condition(expr, is_chassis_resident, c_aux); } OVS_NOT_REACHED(); } @@ -3393,8 +3366,7 @@ expr_parse_microflow__(struct lexer *lexer, struct ds annotated = DS_EMPTY_INITIALIZER; expr_format(e, &annotated); - e = expr_simplify(e); - e = expr_evaluate_condition(e, microflow_is_chassis_resident_cb, NULL); + e = expr_simplify(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 c607a8f89..11697ebd0 100644 --- a/tests/test-ovn.c +++ b/tests/test-ovn.c @@ -308,9 +308,8 @@ test_parse_expr__(int steps) } if (!error) { if (steps > 1) { - expr = expr_simplify(expr); - expr = expr_evaluate_condition(expr, is_chassis_resident_cb, - &ports); + expr = expr_simplify(expr, is_chassis_resident_cb, + &ports); } if (steps > 2) { expr = expr_normalize(expr); @@ -911,9 +910,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)); - modified = expr_evaluate_condition( - modified, tree_shape_is_chassis_resident_cb, NULL); + modified = expr_simplify(expr_clone(expr), + 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 7279452ee..84e5f2b5c 100644 --- a/utilities/ovn-trace.c +++ b/utilities/ovn-trace.c @@ -926,10 +926,8 @@ read_flows(void) continue; } if (match) { - match = expr_simplify(match); - match = expr_evaluate_condition(match, - ovntrace_is_chassis_resident, - NULL); + match = expr_simplify(match, ovntrace_is_chassis_resident, + NULL); } struct ovntrace_flow *flow = xzalloc(sizeof *flow);