From patchwork Tue Sep 15 18:40:25 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Han Zhou X-Patchwork-Id: 1364586 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.138; helo=whitealder.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 whitealder.osuosl.org (smtp1.osuosl.org [140.211.166.138]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 4BrX9X34KTz9sTK for ; Wed, 16 Sep 2020 04:40:48 +1000 (AEST) Received: from localhost (localhost [127.0.0.1]) by whitealder.osuosl.org (Postfix) with ESMTP id 07C7084F57; Tue, 15 Sep 2020 18:40:47 +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 pPosnN+c3jug; Tue, 15 Sep 2020 18:40:43 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by whitealder.osuosl.org (Postfix) with ESMTP id 2DA0D84F5A; Tue, 15 Sep 2020 18:40:43 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id 08727C0859; Tue, 15 Sep 2020 18:40:43 +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 58B64C0051 for ; Tue, 15 Sep 2020 18:40:41 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by whitealder.osuosl.org (Postfix) with ESMTP id 3C2E384F27 for ; Tue, 15 Sep 2020 18:40:41 +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 Xu65bpMt79Ku for ; Tue, 15 Sep 2020 18:40:39 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net [217.70.183.194]) by whitealder.osuosl.org (Postfix) with ESMTPS id 66D4A84F12 for ; Tue, 15 Sep 2020 18:40:39 +0000 (UTC) X-Originating-IP: 73.241.94.255 Received: from localhost.localdomain.localdomain (unknown [73.241.94.255]) (Authenticated sender: hzhou@ovn.org) by relay2-d.mail.gandi.net (Postfix) with ESMTPSA id E625140005; Tue, 15 Sep 2020 18:40:34 +0000 (UTC) From: Han Zhou To: dev@openvswitch.org Date: Tue, 15 Sep 2020 11:40:25 -0700 Message-Id: <1600195226-123272-1-git-send-email-hzhou@ovn.org> X-Mailer: git-send-email 2.1.0 Cc: Han Zhou Subject: [ovs-dev] [PATCH ovn v2 1/2] lflow.c: Avoid adding redundant resource refs for port-bindings. 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: , MIME-Version: 1.0 Errors-To: ovs-dev-bounces@openvswitch.org Sender: "dev" When a lport is referenced by a logical flow where port-binding refs needs to be added, currently it can add the same reference pair multiple times in below situations (introduced in commit ade4e77): 1) In add_matches_to_flow_table(), different matches from same lflow can have same inport/outport. 2) In is_chassis_resident_cb(), a lflow can have multiple is_chassis_resident check for same lport (although not very common), and at the same time the lport used in is_chassis_resident can overlap with the inport/ outport of the same flow. Now because of the redundant entries added, it results in unexpected behavior such as same lflow being processed multiple times as a waste of processing. More severely, after commit 580aea72e it can result in orphaned pointer leading to crash, as reported in [0]. This patch fixes the problems by checking existance of same reference before adding in lflow_resource_add(). To do this check efficiently, hmap is used to replace the list struct for the resource-to-lflow index. [0] https://mail.openvswitch.org/pipermail/ovs-dev/2020-September/374991.html Reported-by: Dumitru Ceara Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2020-September/374991.html Fixes: ade4e779d3fb ("ovn-controller: Use the tracked runtime data changes for flow calculation.") Fixes: 580aea72e26f ("ovn-controller: Fix conjunction handling with incremental processing.") Signed-off-by: Han Zhou --- v1 -> v2: - Addressed Dumitru's comments. - Moved the unrelated change to a separate patch in the series: "lflow.c: Release ref_lflow_node as soon as it is not needed." controller/lflow.c | 69 +++++++++++++++++++++++++++++++++++++----------------- controller/lflow.h | 27 +++++++++++---------- tests/ovn.at | 49 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 112 insertions(+), 33 deletions(-) diff --git a/controller/lflow.c b/controller/lflow.c index e785866..86a5b76 100644 --- a/controller/lflow.c +++ b/controller/lflow.c @@ -74,6 +74,15 @@ consider_logical_flow(const struct sbrec_logical_flow *lflow, struct lflow_ctx_out *l_ctx_out); static void lflow_resource_add(struct lflow_resource_ref *, enum ref_type, const char *ref_name, const struct uuid *); +static struct ref_lflow_node * ref_lflow_lookup(struct hmap *ref_lflow_table, + enum ref_type, + const char *ref_name); +static struct lflow_ref_node * lflow_ref_lookup(struct hmap *lflow_ref_table, + const struct uuid *lflow_uuid); +static void ref_lflow_node_destroy(struct ref_lflow_node *); +static void lflow_resource_destroy_lflow(struct lflow_resource_ref *, + const struct uuid *lflow_uuid); + static bool lookup_port_cb(const void *aux_, const char *port_name, unsigned int *portp) @@ -161,15 +170,14 @@ lflow_resource_destroy(struct lflow_resource_ref *lfrr) { struct ref_lflow_node *rlfn, *rlfn_next; HMAP_FOR_EACH_SAFE (rlfn, rlfn_next, node, &lfrr->ref_lflow_table) { - free(rlfn->ref_name); struct lflow_ref_list_node *lrln, *next; - LIST_FOR_EACH_SAFE (lrln, next, ref_list, &rlfn->ref_lflow_head) { - ovs_list_remove(&lrln->ref_list); - ovs_list_remove(&lrln->lflow_list); + HMAP_FOR_EACH_SAFE (lrln, next, hmap_node, &rlfn->lflow_uuids) { + ovs_list_remove(&lrln->list_node); + hmap_remove(&rlfn->lflow_uuids, &lrln->hmap_node); free(lrln); } hmap_remove(&lfrr->ref_lflow_table, &rlfn->node); - free(rlfn); + ref_lflow_node_destroy(rlfn); } hmap_destroy(&lfrr->ref_lflow_table); @@ -224,17 +232,28 @@ lflow_resource_add(struct lflow_resource_ref *lfrr, enum ref_type type, { struct ref_lflow_node *rlfn = ref_lflow_lookup(&lfrr->ref_lflow_table, type, ref_name); + struct lflow_ref_node *lfrn = lflow_ref_lookup(&lfrr->lflow_ref_table, + lflow_uuid); + if (rlfn && lfrn) { + /* Check if the mapping already existed before adding a new one. */ + struct lflow_ref_list_node *n; + HMAP_FOR_EACH_WITH_HASH (n, hmap_node, uuid_hash(lflow_uuid), + &rlfn->lflow_uuids) { + if (uuid_equals(&n->lflow_uuid, lflow_uuid)) { + return; + } + } + } + if (!rlfn) { rlfn = xzalloc(sizeof *rlfn); rlfn->node.hash = hash_string(ref_name, type); rlfn->type = type; rlfn->ref_name = xstrdup(ref_name); - ovs_list_init(&rlfn->ref_lflow_head); + hmap_init(&rlfn->lflow_uuids); hmap_insert(&lfrr->ref_lflow_table, &rlfn->node, rlfn->node.hash); } - struct lflow_ref_node *lfrn = lflow_ref_lookup(&lfrr->lflow_ref_table, - lflow_uuid); if (!lfrn) { lfrn = xzalloc(sizeof *lfrn); lfrn->node.hash = uuid_hash(lflow_uuid); @@ -245,8 +264,17 @@ lflow_resource_add(struct lflow_resource_ref *lfrr, enum ref_type type, struct lflow_ref_list_node *lrln = xzalloc(sizeof *lrln); lrln->lflow_uuid = *lflow_uuid; - ovs_list_push_back(&rlfn->ref_lflow_head, &lrln->ref_list); - ovs_list_push_back(&lfrn->lflow_ref_head, &lrln->lflow_list); + lrln->rlfn = rlfn; + hmap_insert(&rlfn->lflow_uuids, &lrln->hmap_node, uuid_hash(lflow_uuid)); + ovs_list_push_back(&lfrn->lflow_ref_head, &lrln->list_node); +} + +static void +ref_lflow_node_destroy(struct ref_lflow_node *rlfn) +{ + free(rlfn->ref_name); + hmap_destroy(&rlfn->lflow_uuids); + free(rlfn); } static void @@ -261,9 +289,9 @@ lflow_resource_destroy_lflow(struct lflow_resource_ref *lfrr, hmap_remove(&lfrr->lflow_ref_table, &lfrn->node); struct lflow_ref_list_node *lrln, *next; - LIST_FOR_EACH_SAFE (lrln, next, lflow_list, &lfrn->lflow_ref_head) { - ovs_list_remove(&lrln->ref_list); - ovs_list_remove(&lrln->lflow_list); + LIST_FOR_EACH_SAFE (lrln, next, list_node, &lfrn->lflow_ref_head) { + ovs_list_remove(&lrln->list_node); + hmap_remove(&lrln->rlfn->lflow_uuids, &lrln->hmap_node); free(lrln); } free(lfrn); @@ -531,12 +559,12 @@ lflow_handle_changed_ref(enum ref_type ref_type, const char *ref_name, hmap_remove(&l_ctx_out->lfrr->ref_lflow_table, &rlfn->node); struct lflow_ref_list_node *lrln, *next; - /* Detach the rlfn->ref_lflow_head nodes from the lfrr table and clean + /* Detach the rlfn->lflow_uuids nodes from the lfrr table and clean * up all other nodes related to the lflows that uses the resource, * so that the old nodes won't interfere with updating the lfrr table * when reparsing the lflows. */ - LIST_FOR_EACH (lrln, ref_list, &rlfn->ref_lflow_head) { - ovs_list_remove(&lrln->lflow_list); + HMAP_FOR_EACH (lrln, hmap_node, &rlfn->lflow_uuids) { + ovs_list_remove(&lrln->list_node); } struct hmap dhcp_opts = HMAP_INITIALIZER(&dhcp_opts); @@ -565,7 +593,7 @@ lflow_handle_changed_ref(enum ref_type ref_type, const char *ref_name, /* Firstly, flood remove the flows from desired flow table. */ struct hmap flood_remove_nodes = HMAP_INITIALIZER(&flood_remove_nodes); struct ofctrl_flood_remove_node *ofrn, *ofrn_next; - LIST_FOR_EACH (lrln, ref_list, &rlfn->ref_lflow_head) { + HMAP_FOR_EACH (lrln, hmap_node, &rlfn->lflow_uuids) { VLOG_DBG("Reprocess lflow "UUID_FMT" for resource type: %d," " name: %s.", UUID_ARGS(&lrln->lflow_uuid), @@ -604,12 +632,11 @@ lflow_handle_changed_ref(enum ref_type ref_type, const char *ref_name, } hmap_destroy(&flood_remove_nodes); - LIST_FOR_EACH_SAFE (lrln, next, ref_list, &rlfn->ref_lflow_head) { - ovs_list_remove(&lrln->ref_list); + HMAP_FOR_EACH_SAFE (lrln, next, hmap_node, &rlfn->lflow_uuids) { + hmap_remove(&rlfn->lflow_uuids, &lrln->hmap_node); free(lrln); } - free(rlfn->ref_name); - free(rlfn); + ref_lflow_node_destroy(rlfn); dhcp_opts_destroy(&dhcp_opts); dhcp_opts_destroy(&dhcpv6_opts); diff --git a/controller/lflow.h b/controller/lflow.h index c66b318..1251fb0 100644 --- a/controller/lflow.h +++ b/controller/lflow.h @@ -78,25 +78,28 @@ enum ref_type { REF_TYPE_PORTBINDING }; -/* Maintains the relationship for a pair of named resource and - * a lflow, indexed by both ref_lflow_table and lflow_ref_table. */ -struct lflow_ref_list_node { - struct ovs_list lflow_list; /* list for same lflow */ - struct ovs_list ref_list; /* list for same ref */ - struct uuid lflow_uuid; -}; - struct ref_lflow_node { - struct hmap_node node; + struct hmap_node node; /* node in lflow_resource_ref.ref_lflow_table. */ enum ref_type type; /* key */ char *ref_name; /* key */ - struct ovs_list ref_lflow_head; + struct hmap lflow_uuids; /* Contains lflow_ref_list_node. Use hmap instead + of list so that lflow_resource_add() can check + and avoid adding redundant entires in O(1). */ }; struct lflow_ref_node { - struct hmap_node node; + struct hmap_node node; /* node in lflow_resource_ref.lflow_ref_table. */ struct uuid lflow_uuid; /* key */ - struct ovs_list lflow_ref_head; + struct ovs_list lflow_ref_head; /* Contains lflow_ref_list_node. */ +}; + +/* Maintains the relationship for a pair of named resource and + * a lflow, indexed by both ref_lflow_table and lflow_ref_table. */ +struct lflow_ref_list_node { + struct ovs_list list_node; /* node in lflow_ref_node.lflow_ref_head. */ + struct hmap_node hmap_node; /* node in ref_lflow_node.lflow_uuids. */ + struct uuid lflow_uuid; + struct ref_lflow_node *rlfn; }; struct lflow_resource_ref { diff --git a/tests/ovn.at b/tests/ovn.at index 41fe577..d368fb9 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -22117,3 +22117,52 @@ OVN_CHECK_PACKETS([hv1/vif1-tx.pcap], [1.expected]) OVN_CLEANUP([hv1]) AT_CLEANUP + +AT_SETUP([ovn -- port bind/unbind change handling with conj flows - IPv6]) +ovn_start + +ovn-nbctl ls-add ls1 + +ovn-nbctl lsp-add ls1 lsp1 \ + -- lsp-set-addresses lsp1 "f0:00:00:00:00:01 2001::1" \ + -- lsp-set-port-security lsp1 "f0:00:00:00:00:01 2001::1" + +net_add n1 +sim_add hv1 + +as hv1 +ovs-vsctl add-br br-phys +ovn_attach n1 br-phys 192.168.0.1 +ovs-vsctl -- add-port br-int hv1-vif1 -- \ + set interface hv1-vif1 external-ids:iface-id=lsp1 \ + options:tx_pcap=hv1/vif1-tx.pcap \ + options:rxq_pcap=hv1/vif1-rx.pcap \ + ofport-request=1 + +ovn-nbctl --wait=hv sync + +# Expected conjunction flows: +# ... nd_tll=00:00:00:00:00:00 actions=conjunction(2,2/2) +# ... nd_tll=f0:00:00:00:00:01 actions=conjunction(2,2/2) +# ... nd_target=fe80::f200:ff:fe00:1 actions=conjunction(2,1/2) +# ... nd_target=2001::1 actions=conjunction(2,1/2) +OVS_WAIT_UNTIL([test 4 = `as hv1 ovs-ofctl dump-flows br-int | \ +grep conjunction | wc -l`]) + +# unbind the port +ovs-vsctl set interface hv1-vif1 external_ids:iface-id=foo +OVS_WAIT_UNTIL([test 0 = `as hv1 ovs-ofctl dump-flows br-int | \ +grep conjunction | wc -l`]) + +# bind the port again +ovs-vsctl set interface hv1-vif1 external_ids:iface-id=lsp1 +OVS_WAIT_UNTIL([test 4 = `as hv1 ovs-ofctl dump-flows br-int | \ +grep conjunction | wc -l`]) + +# unbind the port again +ovs-vsctl set interface hv1-vif1 external_ids:iface-id=foo +OVS_WAIT_UNTIL([test 0 = `as hv1 ovs-ofctl dump-flows br-int | \ +grep conjunction | wc -l`]) + +OVN_CLEANUP([hv1]) +AT_CLEANUP From patchwork Tue Sep 15 18:40:26 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Han Zhou X-Patchwork-Id: 1364585 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 4BrX9R4wM9z9sTK for ; Wed, 16 Sep 2020 04:40:43 +1000 (AEST) Received: from localhost (localhost [127.0.0.1]) by fraxinus.osuosl.org (Postfix) with ESMTP id 3E0B58562D; Tue, 15 Sep 2020 18:40:42 +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 YOG1q2cxyT0D; Tue, 15 Sep 2020 18:40:41 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by fraxinus.osuosl.org (Postfix) with ESMTP id 38B15851FB; Tue, 15 Sep 2020 18:40:41 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id 1A497C0859; Tue, 15 Sep 2020 18:40:41 +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 21BD3C0051 for ; Tue, 15 Sep 2020 18:40:40 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by fraxinus.osuosl.org (Postfix) with ESMTP id 1D4D28561D for ; Tue, 15 Sep 2020 18:40:40 +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 El11OtOWjRoe for ; Tue, 15 Sep 2020 18:40:39 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net [217.70.183.194]) by fraxinus.osuosl.org (Postfix) with ESMTPS id 20387851FB for ; Tue, 15 Sep 2020 18:40:38 +0000 (UTC) X-Originating-IP: 73.241.94.255 Received: from localhost.localdomain.localdomain (unknown [73.241.94.255]) (Authenticated sender: hzhou@ovn.org) by relay2-d.mail.gandi.net (Postfix) with ESMTPSA id 6160C40006; Tue, 15 Sep 2020 18:40:36 +0000 (UTC) From: Han Zhou To: dev@openvswitch.org Date: Tue, 15 Sep 2020 11:40:26 -0700 Message-Id: <1600195226-123272-2-git-send-email-hzhou@ovn.org> X-Mailer: git-send-email 2.1.0 In-Reply-To: <1600195226-123272-1-git-send-email-hzhou@ovn.org> References: <1600195226-123272-1-git-send-email-hzhou@ovn.org> Cc: Han Zhou Subject: [ovs-dev] [PATCH ovn v2 2/2] lflow.c: Release ref_lflow_node as soon as it is not needed. 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: , MIME-Version: 1.0 Errors-To: ovs-dev-bounces@openvswitch.org Sender: "dev" If a resource doesn't have any lflows referencing it any more, the node ref_lflow_node in lflow_resource_ref.ref_lflow_table should be removed and released. Otherwise, the table could keep growing in some scenarios, until a recompute is triggered. Now that the chance of triggering recompute is lower and there are more resources references maintained (for type port-binding), this problem is more likely to happen than before. This patch fixes the problem by releasing the node as soon as it is not needed. Fixes: d2aa2c7cafe ("ovn-controller: Maintain resource references for logical flows.") Signed-off-by: Han Zhou --- controller/lflow.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/controller/lflow.c b/controller/lflow.c index 86a5b76..9d54c73 100644 --- a/controller/lflow.c +++ b/controller/lflow.c @@ -292,6 +292,10 @@ lflow_resource_destroy_lflow(struct lflow_resource_ref *lfrr, LIST_FOR_EACH_SAFE (lrln, next, list_node, &lfrn->lflow_ref_head) { ovs_list_remove(&lrln->list_node); hmap_remove(&lrln->rlfn->lflow_uuids, &lrln->hmap_node); + if (hmap_is_empty(&lrln->rlfn->lflow_uuids)) { + hmap_remove(&lfrr->ref_lflow_table, &lrln->rlfn->node); + ref_lflow_node_destroy(lrln->rlfn); + } free(lrln); } free(lfrn);