From patchwork Tue Feb 18 19:53:51 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Numan Siddique X-Patchwork-Id: 1240304 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 48MWl918YRz9sRJ for ; Wed, 19 Feb 2020 06:54:12 +1100 (AEDT) Received: from localhost (localhost [127.0.0.1]) by fraxinus.osuosl.org (Postfix) with ESMTP id 062FB844E7; Tue, 18 Feb 2020 19:54: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 SDJDCrKzpW9C; Tue, 18 Feb 2020 19:54:09 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by fraxinus.osuosl.org (Postfix) with ESMTP id D7D7E849FA; Tue, 18 Feb 2020 19:54:09 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id BD046C08A0; Tue, 18 Feb 2020 19:54:09 +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 60F52C013E for ; Tue, 18 Feb 2020 19:54:07 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by fraxinus.osuosl.org (Postfix) with ESMTP id 50387844E7 for ; Tue, 18 Feb 2020 19:54:07 +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 M4N7qlF56MTO for ; Tue, 18 Feb 2020 19:54:06 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net [217.70.183.199]) by fraxinus.osuosl.org (Postfix) with ESMTPS id 9F215845D5 for ; Tue, 18 Feb 2020 19:54:05 +0000 (UTC) X-Originating-IP: 115.99.223.178 Received: from nummac.local (unknown [115.99.223.178]) (Authenticated sender: numans@ovn.org) by relay9-d.mail.gandi.net (Postfix) with ESMTPSA id 2FEC1FF803; Tue, 18 Feb 2020 19:54:01 +0000 (UTC) From: numans@ovn.org To: dev@openvswitch.org Date: Wed, 19 Feb 2020 01:23:51 +0530 Message-Id: <20200218195351.1223687-1-numans@ovn.org> X-Mailer: git-send-email 2.24.1 MIME-Version: 1.0 Subject: [ovs-dev] [PATCH v2 ovn] Save the addr set and port groups in lflow expr cache 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 After the patch [1], which added caching of lflow expr, the lflow_resource_ref is not rebuilt properly when lflow_run() is called. If a lflow is already cached in lflow expr cache, then the lflow_resource_ref is not updated. But flow_output_run() clears the lflow_resource_ref cache before calling lflow_run(). This results in incorrect OF flows present in the switch even if the address set/port group references have changed. This patch fixes this issue by saving the addr set and port group sets in the lfow expr cache and updating the lflow_resource_ref. [1] - 8795bec737b9("ovn-controller: Cache logical flow expr tree for each lflow.") Fixes: 8795bec737b9("ovn-controller: Cache logical flow expr tree for each lflow.") Signed-off-by: Numan Siddique Acked-by: Han Zhou --- controller/lflow.c | 63 ++++++++++++++++++++++++++++++++++------------ tests/ovn.at | 4 +++ 2 files changed, 51 insertions(+), 16 deletions(-) diff --git a/controller/lflow.c b/controller/lflow.c index 79d89131a..110809df1 100644 --- a/controller/lflow.c +++ b/controller/lflow.c @@ -268,16 +268,21 @@ 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 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)); } @@ -301,6 +306,8 @@ 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); } @@ -310,6 +317,8 @@ 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); } @@ -548,6 +557,25 @@ 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, @@ -615,33 +643,27 @@ consider_logical_flow(const struct sbrec_logical_flow *lflow, struct hmap matches; struct expr *expr = NULL; - struct sset addr_sets_ref = SSET_INITIALIZER(&addr_sets_ref); - struct sset port_groups_ref = SSET_INITIALIZER(&port_groups_ref); 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; } } 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); - 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); + /* 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) { @@ -658,15 +680,24 @@ consider_logical_flow(const struct sbrec_logical_flow *lflow, 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); + 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_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); } struct condition_aux cond_aux = { diff --git a/tests/ovn.at b/tests/ovn.at index ea6760e1f..254645a3a 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -13778,6 +13778,10 @@ for i in 1 2 3; do n_flows_after=`ovs-ofctl dump-flows br-int | grep "10.1.2.10" | wc -l` AT_CHECK([test $(expr $n_flows_before \* 2) = $n_flows_after], [0], [ignore]) + # Trigger full recompute. Creating a chassis would trigger full recompute. + ovn-sbctl chassis-add tst geneve 127.0.0.4 + ovn-sbctl chassis-del tst + # Remove an ACL ovn-nbctl --wait=hv acl-del ls1 to-lport 200 \ 'outport=="lp2" && ip4 && ip4.src == $as1'