From patchwork Mon Sep 7 06:45:34 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Han Zhou X-Patchwork-Id: 1358614 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 4BlJht3ps8z9sSP for ; Mon, 7 Sep 2020 16:46:22 +1000 (AEST) Received: from localhost (localhost [127.0.0.1]) by hemlock.osuosl.org (Postfix) with ESMTP id 9E08A87089; Mon, 7 Sep 2020 06:46:17 +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 4ASieV9UaFCV; Mon, 7 Sep 2020 06:46:14 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by hemlock.osuosl.org (Postfix) with ESMTP id 272618708D; Mon, 7 Sep 2020 06:46:14 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id 018B8C0893; Mon, 7 Sep 2020 06:46:14 +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 19AB3C0859 for ; Mon, 7 Sep 2020 06:46:10 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by whitealder.osuosl.org (Postfix) with ESMTP id 0BB5B86225 for ; Mon, 7 Sep 2020 06:46:10 +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 qnshjICUyp2z for ; Mon, 7 Sep 2020 06:46:08 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from relay11.mail.gandi.net (relay11.mail.gandi.net [217.70.178.231]) by whitealder.osuosl.org (Postfix) with ESMTPS id 7A6C886156 for ; Mon, 7 Sep 2020 06:46:08 +0000 (UTC) Received: from localhost.localdomain.localdomain (unknown [73.241.94.255]) (Authenticated sender: hzhou@ovn.org) by relay11.mail.gandi.net (Postfix) with ESMTPSA id 19ABA10000A; Mon, 7 Sep 2020 06:46:04 +0000 (UTC) From: Han Zhou To: dev@openvswitch.org Date: Sun, 6 Sep 2020 23:45:34 -0700 Message-Id: <1599461142-84752-2-git-send-email-hzhou@ovn.org> X-Mailer: git-send-email 2.1.0 In-Reply-To: <1599461142-84752-1-git-send-email-hzhou@ovn.org> References: <1599461142-84752-1-git-send-email-hzhou@ovn.org> Cc: Han Zhou Subject: [ovs-dev] [PATCH ovn v2 1/9] ofctrl: change ofctrl_dup_flow to module internal function 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" Signed-off-by: Han Zhou --- controller/ofctrl.c | 8 +++++--- controller/ofctrl.h | 2 -- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/controller/ofctrl.c b/controller/ofctrl.c index b8a9c2d..919db6d 100644 --- a/controller/ofctrl.c +++ b/controller/ofctrl.c @@ -169,6 +169,8 @@ static struct ofpbuf *encode_meter_mod(const struct ofputil_meter_mod *); static void ovn_installed_flow_table_clear(void); static void ovn_installed_flow_table_destroy(void); +static struct ovn_flow *ovn_flow_dup(struct ovn_flow *source); + static void ofctrl_recv(const struct ofp_header *, enum ofptype); @@ -787,8 +789,8 @@ ovn_flow_match_hash(const struct ovn_flow *f) } /* Duplicate an ovn_flow structure. */ -struct ovn_flow * -ofctrl_dup_flow(struct ovn_flow *src) +static struct ovn_flow * +ovn_flow_dup(struct ovn_flow *src) { struct ovn_flow *dst = xmalloc(sizeof *dst); dst->table_id = src->table_id; @@ -1291,7 +1293,7 @@ ofctrl_put(struct ovn_desired_flow_table *flow_table, ovn_flow_log(d, "adding installed"); /* Copy 'd' from 'flow_table' to installed_flows. */ - struct ovn_flow *new_node = ofctrl_dup_flow(d); + struct ovn_flow *new_node = ovn_flow_dup(d); hmap_insert(&installed_flows, &new_node->match_hmap_node, new_node->match_hmap_node.hash); } diff --git a/controller/ofctrl.h b/controller/ofctrl.h index 21d2ce6..37f06db 100644 --- a/controller/ofctrl.h +++ b/controller/ofctrl.h @@ -56,8 +56,6 @@ void ofctrl_wait(void); void ofctrl_destroy(void); int64_t ofctrl_get_cur_cfg(void); -struct ovn_flow *ofctrl_dup_flow(struct ovn_flow *source); - void ofctrl_ct_flush_zone(uint16_t zone_id); char *ofctrl_inject_pkt(const struct ovsrec_bridge *br_int, From patchwork Mon Sep 7 06:45:35 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Han Zhou X-Patchwork-Id: 1358613 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 4BlJhr1TMHz9sR4 for ; Mon, 7 Sep 2020 16:46:19 +1000 (AEST) Received: from localhost (localhost [127.0.0.1]) by hemlock.osuosl.org (Postfix) with ESMTP id DCCD887082; Mon, 7 Sep 2020 06:46:13 +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 CXcAr8kFMhI5; Mon, 7 Sep 2020 06:46:13 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by hemlock.osuosl.org (Postfix) with ESMTP id 0B36A87074; Mon, 7 Sep 2020 06:46:13 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id E0800C0893; Mon, 7 Sep 2020 06:46:12 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@lists.linuxfoundation.org Received: from hemlock.osuosl.org (smtp2.osuosl.org [140.211.166.133]) by lists.linuxfoundation.org (Postfix) with ESMTP id 17D17C0051 for ; Mon, 7 Sep 2020 06:46:10 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by hemlock.osuosl.org (Postfix) with ESMTP id 0632086F71 for ; Mon, 7 Sep 2020 06:46:10 +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 xrred+G05nHr for ; Mon, 7 Sep 2020 06:46:09 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from relay11.mail.gandi.net (relay11.mail.gandi.net [217.70.178.231]) by hemlock.osuosl.org (Postfix) with ESMTPS id AB30686F6B for ; Mon, 7 Sep 2020 06:46:08 +0000 (UTC) Received: from localhost.localdomain.localdomain (unknown [73.241.94.255]) (Authenticated sender: hzhou@ovn.org) by relay11.mail.gandi.net (Postfix) with ESMTPSA id 7765E10000C; Mon, 7 Sep 2020 06:46:06 +0000 (UTC) From: Han Zhou To: dev@openvswitch.org Date: Sun, 6 Sep 2020 23:45:35 -0700 Message-Id: <1599461142-84752-3-git-send-email-hzhou@ovn.org> X-Mailer: git-send-email 2.1.0 In-Reply-To: <1599461142-84752-1-git-send-email-hzhou@ovn.org> References: <1599461142-84752-1-git-send-email-hzhou@ovn.org> Cc: Han Zhou Subject: [ovs-dev] [PATCH ovn v2 2/9] ovn.at: Fix AT for conjunction case. 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" 1. Fix flows priorities in the comments. 2. Clean up hv1. Signed-off-by: Han Zhou --- tests/ovn.at | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/tests/ovn.at b/tests/ovn.at index 2ee3862..6c71614 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -13582,17 +13582,17 @@ AT_CHECK([cat 2.packets], [0], [expout]) # There should be total of 9 flows present with conjunction action and 2 flows # with conj match. Eg. -# table=44, priority=2002,conj_id=2,metadata=0x1 actions=resubmit(,45) +# table=44, priority=2001,conj_id=2,metadata=0x1 actions=resubmit(,45) # table=44, priority=2001,conj_id=3,metadata=0x1 actions=drop -# priority=2002,ip,metadata=0x1,nw_dst=10.0.0.6 actions=conjunction(2,2/2) -# priority=2002,ip,metadata=0x1,nw_dst=10.0.0.4 actions=conjunction(2,2/2) -# priority=2002,ip,metadata=0x1,nw_dst=10.0.0.5 actions=conjunction(2,2/2) +# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.6 actions=conjunction(2,2/2) +# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.4 actions=conjunction(2,2/2) +# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.5 actions=conjunction(2,2/2) # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.7 actions=conjunction(3,2/2) # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.9 actions=conjunction(3,2/2) # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.8 actions=conjunction(3,2/2) -# priority=2002,ip,metadata=0x1,nw_src=10.0.0.6 actions=conjunction(2,1/2),conjunction(3,1/2) -# priority=2002,ip,metadata=0x1,nw_src=10.0.0.4 actions=conjunction(2,1/2),conjunction(3,1/2) -# priority=2002,ip,metadata=0x1,nw_src=10.0.0.5 actions=conjunction(2,1/2),conjunction(3,1/2) +# priority=2001,ip,metadata=0x1,nw_src=10.0.0.6 actions=conjunction(2,1/2),conjunction(3,1/2) +# priority=2001,ip,metadata=0x1,nw_src=10.0.0.4 actions=conjunction(2,1/2),conjunction(3,1/2) +# priority=2001,ip,metadata=0x1,nw_src=10.0.0.5 actions=conjunction(2,1/2),conjunction(3,1/2) OVS_WAIT_UNTIL([test 9 = `as hv1 ovs-ofctl dump-flows br-int | \ grep conjunction | wc -l`]) @@ -13616,6 +13616,7 @@ test_ip 1 f00000000001 f00000000002 $sip $dip $PYTHON "$ovs_srcdir/utilities/ovs-pcap.in" hv1/vif2-tx.pcap > 2.packets AT_CHECK([cat 2.packets], [0], []) +OVN_CLEANUP([hv1]) AT_CLEANUP # 3 hypervisors, one logical switch, 3 logical ports per hypervisor From patchwork Mon Sep 7 06:45:36 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Han Zhou X-Patchwork-Id: 1358615 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 4BlJhv4NXXz9sR4 for ; Mon, 7 Sep 2020 16:46:23 +1000 (AEST) Received: from localhost (localhost [127.0.0.1]) by hemlock.osuosl.org (Postfix) with ESMTP id 61E478706D; Mon, 7 Sep 2020 06:46:16 +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 EEUc4LgcheVg; Mon, 7 Sep 2020 06:46:14 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by hemlock.osuosl.org (Postfix) with ESMTP id B960E8708C; Mon, 7 Sep 2020 06:46:14 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id 9B0B9C0893; Mon, 7 Sep 2020 06:46:14 +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 BFC31C0051 for ; Mon, 7 Sep 2020 06:46:10 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by fraxinus.osuosl.org (Postfix) with ESMTP id AF0A1858B8 for ; Mon, 7 Sep 2020 06:46:10 +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 UOfmVne1He7l for ; Mon, 7 Sep 2020 06:46:10 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from relay11.mail.gandi.net (relay11.mail.gandi.net [217.70.178.231]) by fraxinus.osuosl.org (Postfix) with ESMTPS id 096B58543A for ; Mon, 7 Sep 2020 06:46:09 +0000 (UTC) Received: from localhost.localdomain.localdomain (unknown [73.241.94.255]) (Authenticated sender: hzhou@ovn.org) by relay11.mail.gandi.net (Postfix) with ESMTPSA id CD8B4100014; Mon, 7 Sep 2020 06:46:07 +0000 (UTC) From: Han Zhou To: dev@openvswitch.org Date: Sun, 6 Sep 2020 23:45:36 -0700 Message-Id: <1599461142-84752-4-git-send-email-hzhou@ovn.org> X-Mailer: git-send-email 2.1.0 In-Reply-To: <1599461142-84752-1-git-send-email-hzhou@ovn.org> References: <1599461142-84752-1-git-send-email-hzhou@ovn.org> Cc: Han Zhou Subject: [ovs-dev] [PATCH ovn v2 3/9] lflow.c: No need to remove flows for adding new datapath. 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 handling a new datapath, the flows should have never existed, so there is no need to remove them before adding. Although it seems not harmful to do it, the implementation was not complete. To remove existing flows, it also need to remove the flow references. Because the flows never existed, so this wasn't a problem. So, instead of fixing the incomplete flow removing, this patch simply avoid the unnecessary operation. Signed-off-by: Han Zhou --- controller/lflow.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/controller/lflow.c b/controller/lflow.c index 1515612..0c35b7d 100644 --- a/controller/lflow.c +++ b/controller/lflow.c @@ -907,9 +907,6 @@ lflow_add_flows_for_datapath(const struct sbrec_datapath_binding *dp, const struct sbrec_logical_flow *lflow; SBREC_LOGICAL_FLOW_FOR_EACH_EQUAL ( lflow, lf_row, l_ctx_in->sbrec_logical_flow_by_logical_datapath) { - /* Remove the lflow from flow_table if present before processing it. */ - ofctrl_remove_flows(l_ctx_out->flow_table, &lflow->header_.uuid); - if (!consider_logical_flow(lflow, &dhcp_opts, &dhcpv6_opts, &nd_ra_opts, &controller_event_opts, l_ctx_in, l_ctx_out)) { From patchwork Mon Sep 7 06:45:37 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Han Zhou X-Patchwork-Id: 1358616 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 4BlJjJ3Pmtz9sR4 for ; Mon, 7 Sep 2020 16:46:44 +1000 (AEST) Received: from localhost (localhost [127.0.0.1]) by whitealder.osuosl.org (Postfix) with ESMTP id B05EF86700; Mon, 7 Sep 2020 06:46:42 +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 aMYLywyl1xPX; Mon, 7 Sep 2020 06:46:26 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by whitealder.osuosl.org (Postfix) with ESMTP id 97FA286239; Mon, 7 Sep 2020 06:46:26 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id 496CEC0859; Mon, 7 Sep 2020 06:46:26 +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 D07AAC0894 for ; Mon, 7 Sep 2020 06:46:24 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by fraxinus.osuosl.org (Postfix) with ESMTP id AD42B8594B for ; Mon, 7 Sep 2020 06:46:24 +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 NbejdR377_Xz for ; Mon, 7 Sep 2020 06:46:22 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from relay11.mail.gandi.net (relay11.mail.gandi.net [217.70.178.231]) by fraxinus.osuosl.org (Postfix) with ESMTPS id 212D8858DA for ; Mon, 7 Sep 2020 06:46:21 +0000 (UTC) Received: from localhost.localdomain.localdomain (unknown [73.241.94.255]) (Authenticated sender: hzhou@ovn.org) by relay11.mail.gandi.net (Postfix) with ESMTPSA id 2AB09100007; Mon, 7 Sep 2020 06:46:08 +0000 (UTC) From: Han Zhou To: dev@openvswitch.org Date: Sun, 6 Sep 2020 23:45:37 -0700 Message-Id: <1599461142-84752-5-git-send-email-hzhou@ovn.org> X-Mailer: git-send-email 2.1.0 In-Reply-To: <1599461142-84752-1-git-send-email-hzhou@ovn.org> References: <1599461142-84752-1-git-send-email-hzhou@ovn.org> Cc: Han Zhou Subject: [ovs-dev] [PATCH ovn v2 4/9] ovn-controller: Fix conjunction handling with incremental processing. 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 translating lflows to OVS flows, different lflows can refer to same OVS flow as a result of calling ofctrl_add_or_append_flow(), particularly for conjunction combinding. However, the implementation doesn't work with incremental processing, because when any of the lflows are removed, we rely on the lflow's uuid to remove the OVS flow in the desired flow table. Currently only one single lflow uuid is maintained in the desired flow, so removing one of the lflows that references to the same desired flow resulted in wrong behavior: either removing flows that are used by other lflows, or the existing flows are not updated (part of the conjunction actions should be removed from the flow). To solve the problem, this patch maintains the cross reference (M:N) between lflows (and other sb objects) and desired flows, and handles the flow removal and updates with a flood-removal and re-add approach. Fixes: e659bab31a9 ("Combine conjunctions with identical matches into one flow.") Cc: Mark Michelson Signed-off-by: Han Zhou --- controller/lflow.c | 103 +++++++++------ controller/ofctrl.c | 351 ++++++++++++++++++++++++++++++++++++++++++---------- controller/ofctrl.h | 31 ++++- tests/ovn.at | 90 ++++++++++++++ 4 files changed, 469 insertions(+), 106 deletions(-) diff --git a/controller/lflow.c b/controller/lflow.c index 0c35b7d..6fbd36f 100644 --- a/controller/lflow.c +++ b/controller/lflow.c @@ -342,41 +342,56 @@ lflow_handle_changed_flows(struct lflow_ctx_in *l_ctx_in, struct hmap nd_ra_opts = HMAP_INITIALIZER(&nd_ra_opts); nd_ra_opts_init(&nd_ra_opts); - /* Handle removed flows first, and then other flows, so that when - * the flows being added and removed have same match conditions - * can be processed in the proper order */ + struct controller_event_options controller_event_opts; + controller_event_opts_init(&controller_event_opts); + + /* Handle flow removing first (for deleted or updated lflows), and then + * handle reprocessing or adding flows, so that when the flows being + * removed and added with same match conditions can be processed in the + * proper order */ + + struct hmap flood_remove_nodes = HMAP_INITIALIZER(&flood_remove_nodes); + struct ofctrl_flood_remove_node *ofrn, *next; SBREC_LOGICAL_FLOW_TABLE_FOR_EACH_TRACKED (lflow, l_ctx_in->logical_flow_table) { - /* Remove any flows that should be removed. */ - if (sbrec_logical_flow_is_deleted(lflow)) { - VLOG_DBG("handle deleted lflow "UUID_FMT, + if (!sbrec_logical_flow_is_new(lflow)) { + VLOG_DBG("delete lflow "UUID_FMT, UUID_ARGS(&lflow->header_.uuid)); - ofctrl_remove_flows(l_ctx_out->flow_table, &lflow->header_.uuid); - /* Delete entries from lflow resource reference. */ - lflow_resource_destroy_lflow(l_ctx_out->lfrr, - &lflow->header_.uuid); + ofrn = xmalloc(sizeof *ofrn); + ofrn->sb_uuid = lflow->header_.uuid; + hmap_insert(&flood_remove_nodes, &ofrn->hmap_node, + uuid_hash(&ofrn->sb_uuid)); } } + ofctrl_flood_remove_flows(l_ctx_out->flow_table, &flood_remove_nodes); + HMAP_FOR_EACH (ofrn, hmap_node, &flood_remove_nodes) { + /* Delete entries from lflow resource reference. */ + lflow_resource_destroy_lflow(l_ctx_out->lfrr, &ofrn->sb_uuid); + /* Reprocessing the lflow if the sb record is not deleted. */ + lflow = sbrec_logical_flow_table_get_for_uuid( + l_ctx_in->logical_flow_table, &ofrn->sb_uuid); + if (lflow) { + VLOG_DBG("re-add lflow "UUID_FMT, + UUID_ARGS(&lflow->header_.uuid)); + if (!consider_logical_flow(lflow, &dhcp_opts, &dhcpv6_opts, + &nd_ra_opts, &controller_event_opts, + l_ctx_in, l_ctx_out)) { + ret = false; + break; + } + } + } + HMAP_FOR_EACH_SAFE (ofrn, next, hmap_node, &flood_remove_nodes) { + hmap_remove(&flood_remove_nodes, &ofrn->hmap_node); + free(ofrn); + } + hmap_destroy(&flood_remove_nodes); - struct controller_event_options controller_event_opts; - controller_event_opts_init(&controller_event_opts); - + /* Now handle new lflows only. */ SBREC_LOGICAL_FLOW_TABLE_FOR_EACH_TRACKED (lflow, l_ctx_in->logical_flow_table) { - if (!sbrec_logical_flow_is_deleted(lflow)) { - /* Now, add/modify existing flows. If the logical - * flow is a modification, just remove the flows - * for this row, and then add new flows. */ - if (!sbrec_logical_flow_is_new(lflow)) { - VLOG_DBG("handle updated lflow "UUID_FMT, - UUID_ARGS(&lflow->header_.uuid)); - ofctrl_remove_flows(l_ctx_out->flow_table, - &lflow->header_.uuid); - /* Delete entries from lflow resource reference. */ - lflow_resource_destroy_lflow(l_ctx_out->lfrr, - &lflow->header_.uuid); - } - VLOG_DBG("handle new lflow "UUID_FMT, + if (sbrec_logical_flow_is_new(lflow)) { + VLOG_DBG("add lflow "UUID_FMT, UUID_ARGS(&lflow->header_.uuid)); if (!consider_logical_flow(lflow, &dhcp_opts, &dhcpv6_opts, &nd_ra_opts, &controller_event_opts, @@ -420,7 +435,6 @@ lflow_handle_changed_ref(enum ref_type ref_type, const char *ref_name, * when reparsing the lflows. */ LIST_FOR_EACH (lrln, ref_list, &rlfn->ref_lflow_head) { ovs_list_remove(&lrln->lflow_list); - lflow_resource_destroy_lflow(l_ctx_out->lfrr, &lrln->lflow_uuid); } struct hmap dhcp_opts = HMAP_INITIALIZER(&dhcp_opts); @@ -446,22 +460,32 @@ lflow_handle_changed_ref(enum ref_type ref_type, const char *ref_name, controller_event_opts_init(&controller_event_opts); /* Re-parse the related lflows. */ + /* Firstly, flood remove the flows from desired flow table. */ + struct hmap flood_remove_nodes = HMAP_INITIALIZER(&flood_remove_nodes); + struct ofctrl_flood_remove_node *ofrn, *ofrn_next; LIST_FOR_EACH (lrln, ref_list, &rlfn->ref_lflow_head) { + VLOG_DBG("Reprocess lflow "UUID_FMT" for resource type: %d," + " name: %s.", + UUID_ARGS(&lrln->lflow_uuid), + ref_type, ref_name); + ofctrl_flood_remove_add_node(&flood_remove_nodes, &lrln->lflow_uuid); + } + ofctrl_flood_remove_flows(l_ctx_out->flow_table, &flood_remove_nodes); + + /* Secondly, for each lflow that is actually removed, reprocessing it. */ + HMAP_FOR_EACH (ofrn, hmap_node, &flood_remove_nodes) { + lflow_resource_destroy_lflow(l_ctx_out->lfrr, &ofrn->sb_uuid); + const struct sbrec_logical_flow *lflow = sbrec_logical_flow_table_get_for_uuid(l_ctx_in->logical_flow_table, - &lrln->lflow_uuid); + &ofrn->sb_uuid); if (!lflow) { - VLOG_DBG("Reprocess lflow "UUID_FMT" for resource type: %d," - " name: %s - not found.", - UUID_ARGS(&lrln->lflow_uuid), + VLOG_DBG("lflow "UUID_FMT" not found while reprocessing for" + " resource type: %d, name: %s.", + UUID_ARGS(&ofrn->sb_uuid), ref_type, ref_name); continue; } - VLOG_DBG("Reprocess lflow "UUID_FMT" for resource type: %d," - " name: %s.", - UUID_ARGS(&lrln->lflow_uuid), - ref_type, ref_name); - ofctrl_remove_flows(l_ctx_out->flow_table, &lrln->lflow_uuid); if (!consider_logical_flow(lflow, &dhcp_opts, &dhcpv6_opts, &nd_ra_opts, &controller_event_opts, @@ -471,6 +495,11 @@ lflow_handle_changed_ref(enum ref_type ref_type, const char *ref_name, } *changed = true; } + HMAP_FOR_EACH_SAFE (ofrn, ofrn_next, hmap_node, &flood_remove_nodes) { + hmap_remove(&flood_remove_nodes, &ofrn->hmap_node); + free(ofrn); + } + hmap_destroy(&flood_remove_nodes); LIST_FOR_EACH_SAFE (lrln, next, ref_list, &rlfn->ref_lflow_head) { ovs_list_remove(&lrln->ref_list); diff --git a/controller/ofctrl.c b/controller/ofctrl.c index 919db6d..0fc3e69 100644 --- a/controller/ofctrl.c +++ b/controller/ofctrl.c @@ -51,11 +51,45 @@ VLOG_DEFINE_THIS_MODULE(ofctrl); -/* An OpenFlow flow. */ +/* An OpenFlow flow. + * + * Links are maintained between desired flows and SB data. The relationship + * is M to N. The struct sb_flow_ref is used to link a pair of desired flow + * and SB UUID. The below diagram depicts the data structure. + * + * SB UUIDs + * +-----+-----+-----+-----+-----+-----+-----+ + * | | | | | | | | + * +--+--+--+--+--+--+-----+--+--+--+--+--+--+ + * | | | | | | + * Desired Flows | | | | | | + * +----+ +-+-+ | +-+-+ | +-+-+ | + * | +-------+ +-------+ +-------------+ | | + * +----+ +---+ | +-+-+ | +---+ | + * | | | | | | + * +----+ | | +-+-+ | + * | +-------------------------------+ | | + * +----+ +---+ | +---+ | + * | +-------------+ | | | + * +----+ +---+ | | + * | | | | + * +----+ +-+-+ +-+-+ + * | +-------------------+ +-------------------+ | + * +----+ +---+ +---+ + * | | + * +----+ + * + * The links are updated whenever there is a change in desired flows, which is + * usually triggered by a SB data change in I-P engine. + */ struct ovn_flow { struct hmap_node match_hmap_node; /* For match based hashing. */ - struct hindex_node uuid_hindex_node; /* For uuid based hashing. */ struct ovs_list list_node; /* For handling lists of flows. */ + struct ovs_list references; /* A list of struct sb_flow_ref nodes, which + references this flow. (There are cases + that multiple SB entities share the same + desired OpenFlow flow, e.g. when + conjunction is used.) */ /* Key. */ uint8_t table_id; @@ -63,21 +97,34 @@ struct ovn_flow { struct minimatch match; /* Data. */ - struct uuid sb_uuid; struct ofpact *ofpacts; size_t ofpacts_len; uint64_t cookie; }; +struct sb_to_flow { + struct hmap_node hmap_node; /* Node in + ovn_desired_flow_table.uuid_flow_table. */ + struct uuid sb_uuid; + struct ovs_list flows; /* A list of struct sb_flow_ref nodes that are + referenced by the sb_uuid. */ +}; + +struct sb_flow_ref { + struct ovs_list sb_list; /* List node in ovn_flow.references. */ + struct ovs_list flow_list; /* List node in sb_to_flow.ovn_flows. */ + struct ovn_flow *flow; + struct uuid sb_uuid; +}; + static struct ovn_flow *ovn_flow_alloc(uint8_t table_id, uint16_t priority, uint64_t cookie, const struct match *match, - const struct ofpbuf *actions, - const struct uuid *sb_uuid); + const struct ofpbuf *actions); static uint32_t ovn_flow_match_hash(const struct ovn_flow *); static struct ovn_flow *ovn_flow_lookup(struct hmap *flow_table, const struct ovn_flow *target, - bool cmp_sb_uuid); + const struct uuid *sb_uuid); static char *ovn_flow_to_string(const struct ovn_flow *); static void ovn_flow_log(const struct ovn_flow *, const char *action); static void ovn_flow_destroy(struct ovn_flow *); @@ -644,13 +691,46 @@ ofctrl_recv(const struct ofp_header *oh, enum ofptype type) } } +static struct sb_to_flow * +sb_to_flow_find(struct hmap *uuid_flow_table, const struct uuid *sb_uuid) +{ + struct sb_to_flow *stf; + HMAP_FOR_EACH_WITH_HASH (stf, hmap_node, uuid_hash(sb_uuid), + uuid_flow_table) { + if (uuid_equals(sb_uuid, &stf->sb_uuid)) { + return stf; + } + } + return NULL; +} + +static void +link_flow_to_sb(struct ovn_desired_flow_table *flow_table, + struct ovn_flow *f, const struct uuid *sb_uuid) +{ + struct sb_flow_ref *sfr = xmalloc(sizeof *sfr); + sfr->flow = f; + sfr->sb_uuid = *sb_uuid; + ovs_list_insert(&f->references, &sfr->sb_list); + struct sb_to_flow *stf = sb_to_flow_find(&flow_table->uuid_flow_table, + sb_uuid); + if (!stf) { + stf = xmalloc(sizeof *stf); + stf->sb_uuid = *sb_uuid; + ovs_list_init(&stf->flows); + hmap_insert(&flow_table->uuid_flow_table, &stf->hmap_node, + uuid_hash(sb_uuid)); + } + ovs_list_insert(&stf->flows, &sfr->flow_list); +} + /* Flow table interfaces to the rest of ovn-controller. */ /* Adds a flow to 'desired_flows' with the specified 'match' and 'actions' to * the OpenFlow table numbered 'table_id' with the given 'priority' and * OpenFlow 'cookie'. The caller retains ownership of 'match' and 'actions'. * - * The flow is also added to a hash index based on sb_uuid. + * The flow is also linked to the sb_uuid that generates it. * * This just assembles the desired flow table in memory. Nothing is actually * sent to the switch until a later call to ofctrl_put(). @@ -665,11 +745,9 @@ ofctrl_check_and_add_flow(struct ovn_desired_flow_table *flow_table, bool log_duplicate_flow) { struct ovn_flow *f = ovn_flow_alloc(table_id, priority, cookie, match, - actions, sb_uuid); + actions); - ovn_flow_log(f, "ofctrl_add_flow"); - - if (ovn_flow_lookup(&flow_table->match_flow_table, f, true)) { + if (ovn_flow_lookup(&flow_table->match_flow_table, f, sb_uuid)) { if (log_duplicate_flow) { static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5); if (!VLOG_DROP_DBG(&rl)) { @@ -684,31 +762,8 @@ ofctrl_check_and_add_flow(struct ovn_desired_flow_table *flow_table, hmap_insert(&flow_table->match_flow_table, &f->match_hmap_node, f->match_hmap_node.hash); - hindex_insert(&flow_table->uuid_flow_table, &f->uuid_hindex_node, - f->uuid_hindex_node.hash); -} - -/* Removes a bundles of flows from the flow table. */ -void -ofctrl_remove_flows(struct ovn_desired_flow_table *flow_table, - const struct uuid *sb_uuid) -{ - struct ovn_flow *f, *next; - HINDEX_FOR_EACH_WITH_HASH_SAFE (f, next, uuid_hindex_node, - uuid_hash(sb_uuid), - &flow_table->uuid_flow_table) { - if (uuid_equals(&f->sb_uuid, sb_uuid)) { - ovn_flow_log(f, "ofctrl_remove_flow"); - hmap_remove(&flow_table->match_flow_table, - &f->match_hmap_node); - hindex_remove(&flow_table->uuid_flow_table, &f->uuid_hindex_node); - ovn_flow_destroy(f); - } - } - - /* remove any related group and meter info */ - ovn_extend_table_remove_desired(groups, sb_uuid); - ovn_extend_table_remove_desired(meters, sb_uuid); + link_flow_to_sb(flow_table, f, sb_uuid); + ovn_flow_log(f, "ofctrl_add_flow"); } void @@ -721,6 +776,9 @@ ofctrl_add_flow(struct ovn_desired_flow_table *desired_flows, match, actions, sb_uuid, true); } +/* Either add a new flow, or append actions on an existing flow. If the + * flow existed, a new link will also be created between the new sb_uuid + * and the existing flow. */ void ofctrl_add_or_append_flow(struct ovn_desired_flow_table *desired_flows, uint8_t table_id, uint16_t priority, uint64_t cookie, @@ -729,12 +787,10 @@ ofctrl_add_or_append_flow(struct ovn_desired_flow_table *desired_flows, const struct uuid *sb_uuid) { struct ovn_flow *f = ovn_flow_alloc(table_id, priority, cookie, match, - actions, sb_uuid); - - ovn_flow_log(f, "ofctrl_add_or_append_flow"); + actions); struct ovn_flow *existing; - existing = ovn_flow_lookup(&desired_flows->match_flow_table, f, false); + existing = ovn_flow_lookup(&desired_flows->match_flow_table, f, NULL); if (existing) { /* There's already a flow with this particular match. Append the * action to that flow rather than adding a new flow @@ -751,11 +807,169 @@ ofctrl_add_or_append_flow(struct ovn_desired_flow_table *desired_flows, ofpbuf_uninit(&compound); ovn_flow_destroy(f); + f = existing; } else { hmap_insert(&desired_flows->match_flow_table, &f->match_hmap_node, f->match_hmap_node.hash); - hindex_insert(&desired_flows->uuid_flow_table, &f->uuid_hindex_node, - f->uuid_hindex_node.hash); + } + link_flow_to_sb(desired_flows, f, sb_uuid); + + if (existing) { + ovn_flow_log(f, "ofctrl_add_or_append_flow (append)"); + } else { + ovn_flow_log(f, "ofctrl_add_or_append_flow (add)"); + } +} + +/* Remove ovn_flows for the given "sb_to_flow" node in the uuid_flow_table. + * Optionally log the message for each flow that is acturally removed, if + * log_msg is not NULL. */ +static void +remove_flows_from_sb_to_flow(struct ovn_desired_flow_table *flow_table, + struct sb_to_flow *stf, + const char *log_msg) +{ + struct sb_flow_ref *sfr, *next; + LIST_FOR_EACH_SAFE (sfr, next, flow_list, &stf->flows) { + ovs_list_remove(&sfr->sb_list); + ovs_list_remove(&sfr->flow_list); + struct ovn_flow *f = sfr->flow; + free(sfr); + + if (ovs_list_is_empty(&f->references)) { + if (log_msg) { + ovn_flow_log(f, log_msg); + } + hmap_remove(&flow_table->match_flow_table, + &f->match_hmap_node); + ovn_flow_destroy(f); + } + } + hmap_remove(&flow_table->uuid_flow_table, &stf->hmap_node); + free(stf); +} + +void +ofctrl_remove_flows(struct ovn_desired_flow_table *flow_table, + const struct uuid *sb_uuid) +{ + struct sb_to_flow *stf = sb_to_flow_find(&flow_table->uuid_flow_table, + sb_uuid); + if (stf) { + remove_flows_from_sb_to_flow(flow_table, stf, "ofctrl_remove_flow"); + } + + /* remove any related group and meter info */ + ovn_extend_table_remove_desired(groups, sb_uuid); + ovn_extend_table_remove_desired(meters, sb_uuid); +} + +static struct ofctrl_flood_remove_node * +flood_remove_find_node(struct hmap *flood_remove_nodes, struct uuid *sb_uuid) +{ + struct ofctrl_flood_remove_node *ofrn; + HMAP_FOR_EACH_WITH_HASH (ofrn, hmap_node, uuid_hash(sb_uuid), + flood_remove_nodes) { + if (uuid_equals(&ofrn->sb_uuid, sb_uuid)) { + return ofrn; + } + } + return NULL; +} + +void +ofctrl_flood_remove_add_node(struct hmap *flood_remove_nodes, + const struct uuid *sb_uuid) +{ + struct ofctrl_flood_remove_node *ofrn = xmalloc(sizeof *ofrn); + ofrn->sb_uuid = *sb_uuid; + hmap_insert(flood_remove_nodes, &ofrn->hmap_node, uuid_hash(sb_uuid)); +} + +static void +flood_remove_flows_for_sb_uuid(struct ovn_desired_flow_table *flow_table, + const struct uuid *sb_uuid, + struct hmap *flood_remove_nodes) +{ + struct sb_to_flow *stf = sb_to_flow_find(&flow_table->uuid_flow_table, + sb_uuid); + if (!stf) { + return; + } + + /* ovn_flows that have other references and waiting to be removed. */ + struct ovs_list to_be_removed = OVS_LIST_INITIALIZER(&to_be_removed); + + /* Traverse all flows for the given sb_uuid. */ + struct sb_flow_ref *sfr, *next; + LIST_FOR_EACH_SAFE (sfr, next, flow_list, &stf->flows) { + struct ovn_flow *f = sfr->flow; + ovn_flow_log(f, "flood remove"); + + ovs_list_remove(&sfr->sb_list); + ovs_list_remove(&sfr->flow_list); + free(sfr); + + ovs_assert(ovs_list_is_empty(&f->list_node)); + if (ovs_list_is_empty(&f->references)) { + /* This is to optimize the case when most flows have only + * one referencing sb_uuid, so to_be_removed list should + * be empty in most cases. */ + hmap_remove(&flow_table->match_flow_table, + &f->match_hmap_node); + ovn_flow_destroy(f); + } else { + ovs_list_insert(&to_be_removed, &f->list_node); + } + } + hmap_remove(&flow_table->uuid_flow_table, &stf->hmap_node); + free(stf); + + /* Traverse other referencing sb_uuids for the flows in the to_be_removed + * list. */ + + /* Detach the items in f->references from the sfr.flow_list lists, + * so that recursive calls will not mess up the sfr.sb_list list. */ + struct ovn_flow *f, *f_next; + LIST_FOR_EACH (f, list_node, &to_be_removed) { + ovs_assert(!ovs_list_is_empty(&f->references)); + LIST_FOR_EACH (sfr, sb_list, &f->references) { + ovs_list_remove(&sfr->flow_list); + } + } + LIST_FOR_EACH_SAFE (f, f_next, list_node, &to_be_removed) { + LIST_FOR_EACH_SAFE (sfr, next, sb_list, &f->references) { + if (!flood_remove_find_node(flood_remove_nodes, &sfr->sb_uuid)) { + ofctrl_flood_remove_add_node(flood_remove_nodes, + &sfr->sb_uuid); + flood_remove_flows_for_sb_uuid(flow_table, &sfr->sb_uuid, + flood_remove_nodes); + } + ovs_list_remove(&sfr->sb_list); + free(sfr); + } + ovs_list_remove(&f->list_node); + hmap_remove(&flow_table->match_flow_table, + &f->match_hmap_node); + ovn_flow_destroy(f); + } + +} + +void +ofctrl_flood_remove_flows(struct ovn_desired_flow_table *flow_table, + struct hmap *flood_remove_nodes) +{ + struct ofctrl_flood_remove_node *ofrn; + HMAP_FOR_EACH (ofrn, hmap_node, flood_remove_nodes) { + flood_remove_flows_for_sb_uuid(flow_table, &ofrn->sb_uuid, + flood_remove_nodes); + } + + /* remove any related group and meter info */ + HMAP_FOR_EACH (ofrn, hmap_node, flood_remove_nodes) { + ovn_extend_table_remove_desired(groups, &ofrn->sb_uuid); + ovn_extend_table_remove_desired(meters, &ofrn->sb_uuid); } } @@ -763,18 +977,17 @@ ofctrl_add_or_append_flow(struct ovn_desired_flow_table *desired_flows, static struct ovn_flow * ovn_flow_alloc(uint8_t table_id, uint16_t priority, uint64_t cookie, - const struct match *match, const struct ofpbuf *actions, - const struct uuid *sb_uuid) + const struct match *match, const struct ofpbuf *actions) { struct ovn_flow *f = xmalloc(sizeof *f); + ovs_list_init(&f->references); + ovs_list_init(&f->list_node); f->table_id = table_id; f->priority = priority; minimatch_init(&f->match, match); f->ofpacts = xmemdup(actions->data, actions->size); f->ofpacts_len = actions->size; - f->sb_uuid = *sb_uuid; f->match_hmap_node.hash = ovn_flow_match_hash(f); - f->uuid_hindex_node.hash = uuid_hash(&f->sb_uuid); f->cookie = cookie; return f; @@ -793,23 +1006,27 @@ static struct ovn_flow * ovn_flow_dup(struct ovn_flow *src) { struct ovn_flow *dst = xmalloc(sizeof *dst); + ovs_list_init(&dst->references); dst->table_id = src->table_id; dst->priority = src->priority; minimatch_clone(&dst->match, &src->match); dst->ofpacts = xmemdup(src->ofpacts, src->ofpacts_len); dst->ofpacts_len = src->ofpacts_len; - dst->sb_uuid = src->sb_uuid; dst->match_hmap_node.hash = src->match_hmap_node.hash; - dst->uuid_hindex_node.hash = uuid_hash(&src->sb_uuid); dst->cookie = src->cookie; return dst; } /* Finds and returns an ovn_flow in 'flow_table' whose key is identical to - * 'target''s key, or NULL if there is none. */ + * 'target''s key, or NULL if there is none. + * + * If sb_uuid is not NULL, the function will also check if the found flow is + * referenced by the sb_uuid. + * + * NOTE: sb_uuid can only be used for ovn_desired_flow_table lookup. */ static struct ovn_flow * ovn_flow_lookup(struct hmap *flow_table, const struct ovn_flow *target, - bool cmp_sb_uuid) + const struct uuid *sb_uuid) { struct ovn_flow *f; @@ -818,9 +1035,16 @@ ovn_flow_lookup(struct hmap *flow_table, const struct ovn_flow *target, if (f->table_id == target->table_id && f->priority == target->priority && minimatch_equal(&f->match, &target->match)) { - if (!cmp_sb_uuid || uuid_equals(&target->sb_uuid, &f->sb_uuid)) { + if (!sb_uuid) { return f; } + ovs_assert(flow_table != &installed_flows); + struct sb_flow_ref *sfr; + LIST_FOR_EACH (sfr, sb_list, &f->references) { + if (uuid_equals(sb_uuid, &sfr->sb_uuid)) { + return f; + } + } } } return NULL; @@ -830,7 +1054,7 @@ static char * ovn_flow_to_string(const struct ovn_flow *f) { struct ds s = DS_EMPTY_INITIALIZER; - ds_put_format(&s, "sb_uuid="UUID_FMT", ", UUID_ARGS(&f->sb_uuid)); + ds_put_format(&s, "cookie=%"PRIx64", ", f->cookie); ds_put_format(&s, "table_id=%"PRIu8", ", f->table_id); ds_put_format(&s, "priority=%"PRIu16", ", f->priority); @@ -855,6 +1079,7 @@ static void ovn_flow_destroy(struct ovn_flow *f) { if (f) { + ovs_assert(ovs_list_is_empty(&f->references)); minimatch_destroy(&f->match); free(f->ofpacts); free(f); @@ -866,18 +1091,16 @@ void ovn_desired_flow_table_init(struct ovn_desired_flow_table *flow_table) { hmap_init(&flow_table->match_flow_table); - hindex_init(&flow_table->uuid_flow_table); + hmap_init(&flow_table->uuid_flow_table); } void ovn_desired_flow_table_clear(struct ovn_desired_flow_table *flow_table) { - struct ovn_flow *f, *next; - HMAP_FOR_EACH_SAFE (f, next, match_hmap_node, - &flow_table->match_flow_table) { - hmap_remove(&flow_table->match_flow_table, &f->match_hmap_node); - hindex_remove(&flow_table->uuid_flow_table, &f->uuid_hindex_node); - ovn_flow_destroy(f); + struct sb_to_flow *stf, *next; + HMAP_FOR_EACH_SAFE (stf, next, hmap_node, + &flow_table->uuid_flow_table) { + remove_flows_from_sb_to_flow(flow_table, stf, NULL); } } @@ -886,7 +1109,7 @@ ovn_desired_flow_table_destroy(struct ovn_desired_flow_table *flow_table) { ovn_desired_flow_table_clear(flow_table); hmap_destroy(&flow_table->match_flow_table); - hindex_destroy(&flow_table->uuid_flow_table); + hmap_destroy(&flow_table->uuid_flow_table); } static void @@ -1221,7 +1444,7 @@ ofctrl_put(struct ovn_desired_flow_table *flow_table, struct ovn_flow *i, *next; HMAP_FOR_EACH_SAFE (i, next, match_hmap_node, &installed_flows) { struct ovn_flow *d = ovn_flow_lookup(&flow_table->match_flow_table, - i, false); + i, NULL); if (!d) { /* Installed flow is no longer desirable. Delete it from the * switch and from installed_flows. */ @@ -1237,10 +1460,6 @@ ofctrl_put(struct ovn_desired_flow_table *flow_table, hmap_remove(&installed_flows, &i->match_hmap_node); ovn_flow_destroy(i); } else { - if (!uuid_equals(&i->sb_uuid, &d->sb_uuid)) { - /* Update installed flow's UUID. */ - i->sb_uuid = d->sb_uuid; - } if (!ofpacts_equal(i->ofpacts, i->ofpacts_len, d->ofpacts, d->ofpacts_len) || i->cookie != d->cookie) { @@ -1277,7 +1496,7 @@ ofctrl_put(struct ovn_desired_flow_table *flow_table, * in the installed flow table. */ struct ovn_flow *d; HMAP_FOR_EACH (d, match_hmap_node, &flow_table->match_flow_table) { - i = ovn_flow_lookup(&installed_flows, d, false); + i = ovn_flow_lookup(&installed_flows, d, NULL); if (!i) { /* Send flow_mod to add flow. */ struct ofputil_flow_mod fm = { diff --git a/controller/ofctrl.h b/controller/ofctrl.h index 37f06db..8789ce4 100644 --- a/controller/ofctrl.h +++ b/controller/ofctrl.h @@ -35,8 +35,9 @@ struct ovn_desired_flow_table { /* Hash map flow table using flow match conditions as hash key.*/ struct hmap match_flow_table; - /* SB uuid index for the nodes in match_flow_table.*/ - struct hindex uuid_flow_table; + /* SB uuid index for the cross reference nodes that link to the nodes in + * match_flow_table.*/ + struct hmap uuid_flow_table; }; /* Interface for OVN main loop. */ @@ -74,7 +75,30 @@ void ofctrl_add_or_append_flow(struct ovn_desired_flow_table *desired_flows, const struct ofpbuf *actions, const struct uuid *sb_uuid); -void ofctrl_remove_flows(struct ovn_desired_flow_table *, const struct uuid *); +/* Removes a bundles of flows from the flow table for a specific sb_uuid. The + * flows are removed only if they are not referenced by any other sb_uuid(s). + * For flood-removing all related flows referenced by other sb_uuid(s), use + * ofctrl_flood_remove_flows(). */ +void ofctrl_remove_flows(struct ovn_desired_flow_table *, + const struct uuid *sb_uuid); + +/* The function ofctrl_flood_remove_flows flood-removes flows from the desired + * flow table for the sb_uuids provided in the flood_remove_nodes argument. + * For each given sb_uuid in flood_remove_nodes, it removes all the flows + * generated by the sb_uuid, and if any of the flows are referenced by another + * sb_uuid, it continues removing all the flows used by that sb_uuid as well, + * and so on, recursively. + * + * It adds all the sb_uuids that are actually removed in the + * flood_remove_nodes. */ +struct ofctrl_flood_remove_node { + struct hmap_node hmap_node; + struct uuid sb_uuid; +}; +void ofctrl_flood_remove_flows(struct ovn_desired_flow_table *, + struct hmap *flood_remove_nodes); +void ofctrl_flood_remove_add_node(struct hmap *flood_remove_nodes, + const struct uuid *sb_uuid); void ovn_desired_flow_table_init(struct ovn_desired_flow_table *); void ovn_desired_flow_table_clear(struct ovn_desired_flow_table *); @@ -86,6 +110,7 @@ void ofctrl_check_and_add_flow(struct ovn_desired_flow_table *, const struct ofpbuf *ofpacts, const struct uuid *, bool log_duplicate_flow); + bool ofctrl_is_connected(void); void ofctrl_set_probe_interval(int probe_interval); diff --git a/tests/ovn.at b/tests/ovn.at index 6c71614..31cfddc 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -13596,6 +13596,8 @@ AT_CHECK([cat 2.packets], [0], [expout]) OVS_WAIT_UNTIL([test 9 = `as hv1 ovs-ofctl dump-flows br-int | \ grep conjunction | wc -l`]) +OVS_WAIT_UNTIL([test 3 = `as hv1 ovs-ofctl dump-flows br-int | \ +grep conjunction.*conjunction | wc -l`]) OVS_WAIT_UNTIL([test 2 = `as hv1 ovs-ofctl dump-flows br-int | \ grep conj_id | wc -l`]) @@ -13613,9 +13615,97 @@ sip=`ip_to_hex 10 0 0 4` dip=`ip_to_hex 10 0 0 7` test_ip 1 f00000000001 f00000000002 $sip $dip + $PYTHON "$ovs_srcdir/utilities/ovs-pcap.in" hv1/vif2-tx.pcap > 2.packets AT_CHECK([cat 2.packets], [0], []) +# Remove the first ACL, and verify that the conjunction flows are updated +# properly. +# There should be total of 6 flows present with conjunction action and 1 flow +# with conj match. Eg. +# table=44, priority=2001,conj_id=3,metadata=0x1 actions=drop +# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.7 actions=conjunction(4,2/2) +# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.9 actions=conjunction(4,2/2) +# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.8 actions=conjunction(4,2/2) +# priority=2001,ip,metadata=0x1,nw_src=10.0.0.6 actions=conjunction(4,1/2) +# priority=2001,ip,metadata=0x1,nw_src=10.0.0.4 actions=conjunction(4,1/2) +# priority=2001,ip,metadata=0x1,nw_src=10.0.0.5 actions=conjunction(4,1/2) + +ovn-nbctl acl-del ls1 to-lport 1001 \ +'ip4 && ip4.src == $set1 && ip4.dst == $set1' + +OVS_WAIT_UNTIL([test 6 = `as hv1 ovs-ofctl dump-flows br-int | \ +grep conjunction | wc -l`]) +OVS_WAIT_UNTIL([test 0 = `as hv1 ovs-ofctl dump-flows br-int | \ +grep conjunction.*conjunction | wc -l`]) +OVS_WAIT_UNTIL([test 1 = `as hv1 ovs-ofctl dump-flows br-int | \ +grep conj_id | wc -l`]) + +# Add the ACL back +ovn-nbctl acl-add ls1 to-lport 1001 \ +'ip4 && ip4.src == $set1 && ip4.dst == $set1' allow +# Add one more ACL with more overlapping +ovn-nbctl acl-add ls1 to-lport 1001 \ +'ip4 && ip4.src == $set1 && ip4.dst == {10.0.0.9, 10.0.0.10}' drop + +# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.8 actions=conjunction(4,1/2) +# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.7 actions=conjunction(4,1/2) +# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.4 actions=conjunction(5,1/2) +# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.5 actions=conjunction(5,1/2) +# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.6 actions=conjunction(5,1/2) +# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.9 actions=conjunction(4,1/2),conjunction(6,1/2) +# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.10 actions=conjunction(6,1/2) +# priority=2001,ip,metadata=0x1,nw_src=10.0.0.5 actions=conjunction(4,2/2),conjunction(5,2/2),conjunction(6,2/2) +# priority=2001,ip,metadata=0x1,nw_src=10.0.0.4 actions=conjunction(4,2/2),conjunction(5,2/2),conjunction(6,2/2) +# priority=2001,ip,metadata=0x1,nw_src=10.0.0.6 actions=conjunction(4,2/2),conjunction(5,2/2),conjunction(6,2/2) + +OVS_WAIT_UNTIL([test 10 = `as hv1 ovs-ofctl dump-flows br-int | \ +grep conjunction | wc -l`]) +OVS_WAIT_UNTIL([test 4 = `as hv1 ovs-ofctl dump-flows br-int | \ +grep conjunction.*conjunction | wc -l`]) +OVS_WAIT_UNTIL([test 3 = `as hv1 ovs-ofctl dump-flows br-int | \ +grep conjunction.*conjunction.*conjunction | wc -l`]) + +# Remove 10.0.0.7 from address set2. All flows should be updated properly. +ovn-nbctl set Address_Set set2 \ +addresses=\"10.0.0.8\",\"10.0.0.9\" + +# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.4 actions=conjunction(9,1/2) +# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.10 actions=conjunction(7,1/2) +# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.8 actions=conjunction(8,1/2) +# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.5 actions=conjunction(9,1/2) +# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.9 actions=conjunction(7,1/2),conjunction(8,1/2) +# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.6 actions=conjunction(9,1/2) +# priority=2001,ip,metadata=0x1,nw_src=10.0.0.5 actions=conjunction(7,2/2),conjunction(8,2/2),conjunction(9,2/2) +# priority=2001,ip,metadata=0x1,nw_src=10.0.0.6 actions=conjunction(7,2/2),conjunction(8,2/2),conjunction(9,2/2) +# priority=2001,ip,metadata=0x1,nw_src=10.0.0.4 actions=conjunction(7,2/2),conjunction(8,2/2),conjunction(9,2/2) + +OVS_WAIT_UNTIL([test 9 = `as hv1 ovs-ofctl dump-flows br-int | \ +grep conjunction | wc -l`]) +OVS_WAIT_UNTIL([test 4 = `as hv1 ovs-ofctl dump-flows br-int | \ +grep conjunction.*conjunction | wc -l`]) +OVS_WAIT_UNTIL([test 3 = `as hv1 ovs-ofctl dump-flows br-int | \ +grep conjunction.*conjunction.*conjunction | wc -l`]) + +# Remove an ACL again +ovn-nbctl acl-del ls1 to-lport 1001 \ +'ip4 && ip4.src == $set1 && ip4.dst == $set1' + +ovn-nbctl --wait=hv sync +# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.10 actions=conjunction(10,1/2) +# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.8 actions=conjunction(11,1/2) +# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.9 actions=conjunction(10,1/2),conjunction(11,1/2) +# priority=2001,ip,metadata=0x1,nw_src=10.0.0.5 actions=conjunction(10,2/2),conjunction(11,2/2) +# priority=2001,ip,metadata=0x1,nw_src=10.0.0.6 actions=conjunction(10,2/2),conjunction(11,2/2) +# priority=2001,ip,metadata=0x1,nw_src=10.0.0.4 actions=conjunction(10,2/2),conjunction(11,2/2) + +OVS_WAIT_UNTIL([test 6 = `as hv1 ovs-ofctl dump-flows br-int | \ +grep conjunction | wc -l`]) +OVS_WAIT_UNTIL([test 4 = `as hv1 ovs-ofctl dump-flows br-int | \ +grep conjunction.*conjunction | wc -l`]) +OVS_WAIT_UNTIL([test 0 = `as hv1 ovs-ofctl dump-flows br-int | \ +grep conjunction.*conjunction.*conjunction | wc -l`]) + OVN_CLEANUP([hv1]) AT_CLEANUP From patchwork Mon Sep 7 06:45:38 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Han Zhou X-Patchwork-Id: 1358618 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 4BlJkc2d6Zz9sR4 for ; Mon, 7 Sep 2020 16:47:51 +1000 (AEST) Received: from localhost (localhost [127.0.0.1]) by silver.osuosl.org (Postfix) with ESMTP id 17B6C207A6; Mon, 7 Sep 2020 06:47:49 +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 gSaNR-cGjUwO; Mon, 7 Sep 2020 06:47:40 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by silver.osuosl.org (Postfix) with ESMTP id C621F20500; Mon, 7 Sep 2020 06:46:51 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id B7CE8C0891; Mon, 7 Sep 2020 06:46:51 +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 29DF0C0051 for ; Mon, 7 Sep 2020 06:46:50 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by whitealder.osuosl.org (Postfix) with ESMTP id 26D0F865CF for ; Mon, 7 Sep 2020 06:46:50 +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 x9f3QQIBFcJ8 for ; Mon, 7 Sep 2020 06:46:43 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from relay11.mail.gandi.net (relay11.mail.gandi.net [217.70.178.231]) by whitealder.osuosl.org (Postfix) with ESMTPS id 0CC93866AF for ; Mon, 7 Sep 2020 06:46:28 +0000 (UTC) Received: from localhost.localdomain.localdomain (unknown [73.241.94.255]) (Authenticated sender: hzhou@ovn.org) by relay11.mail.gandi.net (Postfix) with ESMTPSA id 47E31100005; Mon, 7 Sep 2020 06:46:20 +0000 (UTC) From: Han Zhou To: dev@openvswitch.org Date: Sun, 6 Sep 2020 23:45:38 -0700 Message-Id: <1599461142-84752-6-git-send-email-hzhou@ovn.org> X-Mailer: git-send-email 2.1.0 In-Reply-To: <1599461142-84752-1-git-send-email-hzhou@ovn.org> References: <1599461142-84752-1-git-send-email-hzhou@ovn.org> Cc: Han Zhou Subject: [ovs-dev] [PATCH ovn v2 5/9] ovn.at: Add test case for duplicated flow handling. 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" In ofctrl_put() of controller/ofctrl.c, some special considerations have been made to handle duplicated match conditions from difference desired flows because OVS doesn't allow multiple flows with same priority and same match condition. This patch adds a test to cover such scenarios make sure it works as expected. Signed-off-by: Han Zhou --- tests/ovn.at | 120 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 120 insertions(+) diff --git a/tests/ovn.at b/tests/ovn.at index 31cfddc..3414de9 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -21537,3 +21537,123 @@ OVS_WAIT_UNTIL([test x$(as hv1 ovn-appctl -t ovn-controller debug/status) = "xru OVN_CLEANUP([hv1]) AT_CLEANUP + +# Duplicate ACLs (same match with same action) should work as expected. +# Conflict ACLs (same match with different actions) behavior is unpredictable +# (only one of them would work). +# This test covers both situation and also makes sure adding/deleting in +# different order is handled properly (duplicated flow handling in ofctrl_put() +# of ovn-controller) +AT_SETUP([ovn -- conflict or duplicate ACLs resulting in same OVS match]) +ovn_start + +ovn-nbctl ls-add ls1 + +ovn-nbctl lsp-add ls1 lsp1 \ +-- lsp-set-addresses lsp1 "f0:00:00:00:00:01 10.0.0.1" + +ovn-nbctl lsp-add ls1 lsp2 \ +-- lsp-set-addresses lsp2 "f0:00:00:00:00:02 10.0.0.2" + +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 + +ovs-vsctl -- add-port br-int hv1-vif2 -- \ + set interface hv1-vif2 external-ids:iface-id=lsp2 \ + options:tx_pcap=hv1/vif2-tx.pcap \ + options:rxq_pcap=hv1/vif2-rx.pcap \ + ofport-request=2 + +# Default drop +ovn-nbctl acl-add ls1 to-lport 1000 \ +'outport == "lsp1" && ip4' drop + +# test_ip INPORT SRC_MAC DST_MAC SRC_IP DST_IP OUTPORT... +# +# This shell function causes an ip packet to be received on INPORT. +# The packet's content has Ethernet destination DST and source SRC +# (each exactly 12 hex digits) and Ethernet type ETHTYPE (4 hex digits). +# The OUTPORTs (zero or more) list the VIFs on which the packet should +# be received. INPORT and the OUTPORTs are specified as logical switch +# port numbers, e.g. 11 for vif11. +test_ip() { + # This packet has bad checksums but logical L3 routing doesn't check. + local inport=$1 src_mac=$2 dst_mac=$3 src_ip=$4 dst_ip=$5 + local packet=${dst_mac}${src_mac}08004500001c0000000040110000${src_ip}\ +${dst_ip}0035111100080000 + shift; shift; shift; shift; shift + as hv1 ovs-appctl netdev-dummy/receive hv1-vif$inport $packet + for outport; do + echo $packet >> $outport.expected + done +} + +ip_to_hex() { + printf "%02x%02x%02x%02x" "$@" +} + +reset_pcap_file() { + local iface=$1 + local pcap_file=$2 + ovs-vsctl -- set Interface $iface options:tx_pcap=dummy-tx.pcap \ +options:rxq_pcap=dummy-rx.pcap + rm -f ${pcap_file}*.pcap + ovs-vsctl -- set Interface $iface options:tx_pcap=${pcap_file}-tx.pcap \ +options:rxq_pcap=${pcap_file}-rx.pcap +} + + +# Create overlapping ACLs resulting in duplicated desired OVS flows +ovn-nbctl acl-add ls1 to-lport 1001 \ +'outport == "lsp1" && ip4 && ip4.src == 10.0.0.2' allow +ovn-nbctl acl-add ls1 to-lport 1001 \ +'outport == "lsp1" && ip4 && ip4.src == {10.0.0.2, 10.0.0.3}' allow + +ovn-nbctl --wait=hv sync + +sip=`ip_to_hex 10 0 0 2` +dip=`ip_to_hex 10 0 0 1` +test_ip 2 f00000000002 f00000000001 $sip $dip 1 +OVN_CHECK_PACKETS([hv1/vif1-tx.pcap], [1.expected]) + +# Delete one of the ACLs. +ovn-nbctl acl-del ls1 to-lport 1001 \ +'outport == "lsp1" && ip4 && ip4.src == {10.0.0.2, 10.0.0.3}' + +test_ip 2 f00000000002 f00000000001 $sip $dip 1 +OVN_CHECK_PACKETS([hv1/vif1-tx.pcap], [1.expected]) + +# Add a conflict ACL with drop action. +ovn-nbctl acl-add ls1 to-lport 1001 \ +'outport == "lsp1" && ip4 && ip4.src == {10.0.0.2, 10.0.0.3}' drop +# Don't test because it is unpredicatable which rule will take effect. + +# Delete the ACL that has "allow" action +ovn-nbctl acl-del ls1 to-lport 1001 \ +'outport == "lsp1" && ip4 && ip4.src == 10.0.0.2' + +# Packet should be dropped +test_ip 2 f00000000002 f00000000001 $sip $dip +OVN_CHECK_PACKETS([hv1/vif1-tx.pcap], [1.expected]) + +# Add the ACL back and delete the "drop" ACL +ovn-nbctl acl-add ls1 to-lport 1001 \ +'outport == "lsp1" && ip4 && ip4.src == 10.0.0.2' allow +ovn-nbctl acl-del ls1 to-lport 1001 \ +'outport == "lsp1" && ip4 && ip4.src == {10.0.0.2, 10.0.0.3}' + +# Packet should be received +test_ip 2 f00000000002 f00000000001 $sip $dip 1 +OVN_CHECK_PACKETS([hv1/vif1-tx.pcap], [1.expected]) + +OVN_CLEANUP([hv1]) +AT_CLEANUP From patchwork Mon Sep 7 06:45:39 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Han Zhou X-Patchwork-Id: 1358619 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 4BlJkl5h1wz9sSP for ; Mon, 7 Sep 2020 16:47:59 +1000 (AEST) Received: from localhost (localhost [127.0.0.1]) by whitealder.osuosl.org (Postfix) with ESMTP id B79EC86679; Mon, 7 Sep 2020 06:47:56 +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 3c7KeUH80k1d; Mon, 7 Sep 2020 06:47:42 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by whitealder.osuosl.org (Postfix) with ESMTP id A4C85866D9; Mon, 7 Sep 2020 06:47:40 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id 8AE06C0859; Mon, 7 Sep 2020 06:47:40 +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 D383DC0051 for ; Mon, 7 Sep 2020 06:47:39 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by silver.osuosl.org (Postfix) with ESMTP id CF73F22D10 for ; Mon, 7 Sep 2020 06:47:39 +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 KsHP8gAB78Q4 for ; Mon, 7 Sep 2020 06:47:14 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from relay11.mail.gandi.net (relay11.mail.gandi.net [217.70.178.231]) by silver.osuosl.org (Postfix) with ESMTPS id 2AB6820529 for ; Mon, 7 Sep 2020 06:46:36 +0000 (UTC) Received: from localhost.localdomain.localdomain (unknown [73.241.94.255]) (Authenticated sender: hzhou@ovn.org) by relay11.mail.gandi.net (Postfix) with ESMTPSA id 2F5DE100002; Mon, 7 Sep 2020 06:46:27 +0000 (UTC) From: Han Zhou To: dev@openvswitch.org Date: Sun, 6 Sep 2020 23:45:39 -0700 Message-Id: <1599461142-84752-7-git-send-email-hzhou@ovn.org> X-Mailer: git-send-email 2.1.0 In-Reply-To: <1599461142-84752-1-git-send-email-hzhou@ovn.org> References: <1599461142-84752-1-git-send-email-hzhou@ovn.org> Cc: Han Zhou Subject: [ovs-dev] [PATCH ovn v2 6/9] ofctrl.c: Maintain references between installed flows and desired flows. 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" Currently there is no link maintained between installed flows and desired flows. This patch maintains the mapping between them, which will be useful for a future patch that incrementally processes the flow installation without having to do the full comparison between them. This patch also refactors the struct ovn_flow with two different wrapper types: desired_flow and installed_flow, and the related static functions, to make the code easier to read and avoid misuses of the struct. Signed-off-by: Han Zhou --- controller/ofctrl.c | 428 ++++++++++++++++++++++++++++++++++++---------------- 1 file changed, 294 insertions(+), 134 deletions(-) diff --git a/controller/ofctrl.c b/controller/ofctrl.c index 0fc3e69..ecea081 100644 --- a/controller/ofctrl.c +++ b/controller/ofctrl.c @@ -51,7 +51,27 @@ VLOG_DEFINE_THIS_MODULE(ofctrl); -/* An OpenFlow flow. +/* An OpenFlow flow. */ +struct ovn_flow { + /* Key. */ + uint8_t table_id; + uint16_t priority; + struct minimatch match; + + /* Hash. */ + uint32_t hash; + + /* Data. */ + struct ofpact *ofpacts; + size_t ofpacts_len; + uint64_t cookie; +}; + +/* A desired flow, in struct ovn_desired_flow_table, calculated by the + * incremental processing engine. + * - They are added/removed incrementally when I-P engine is able to process + * the changes incrementally, or + * - Completely cleared and recomputed by I-P engine when recompute happens. * * Links are maintained between desired flows and SB data. The relationship * is M to N. The struct sb_flow_ref is used to link a pair of desired flow @@ -82,52 +102,92 @@ VLOG_DEFINE_THIS_MODULE(ofctrl); * The links are updated whenever there is a change in desired flows, which is * usually triggered by a SB data change in I-P engine. */ -struct ovn_flow { +struct desired_flow { + struct ovn_flow flow; struct hmap_node match_hmap_node; /* For match based hashing. */ struct ovs_list list_node; /* For handling lists of flows. */ - struct ovs_list references; /* A list of struct sb_flow_ref nodes, which - references this flow. (There are cases - that multiple SB entities share the same - desired OpenFlow flow, e.g. when - conjunction is used.) */ - /* Key. */ - uint8_t table_id; - uint16_t priority; - struct minimatch match; + /* A list of struct sb_flow_ref nodes, which references this flow. (There + * are cases that multiple SB entities share the same desired OpenFlow + * flow, e.g. when conjunction is used.) */ + struct ovs_list references; - /* Data. */ - struct ofpact *ofpacts; - size_t ofpacts_len; - uint64_t cookie; + /* The corresponding flow in installed table. */ + struct installed_flow *installed_flow; + + /* Node in installed_flow.desired_refs list. */ + struct ovs_list installed_ref_list_node; }; struct sb_to_flow { struct hmap_node hmap_node; /* Node in ovn_desired_flow_table.uuid_flow_table. */ struct uuid sb_uuid; - struct ovs_list flows; /* A list of struct sb_flow_ref nodes that are - referenced by the sb_uuid. */ + struct ovs_list flows; /* A list of struct sb_flow_ref nodes that + are referenced by the sb_uuid. */ }; struct sb_flow_ref { - struct ovs_list sb_list; /* List node in ovn_flow.references. */ - struct ovs_list flow_list; /* List node in sb_to_flow.ovn_flows. */ - struct ovn_flow *flow; + struct ovs_list sb_list; /* List node in desired_flow.references. */ + struct ovs_list flow_list; /* List node in sb_to_flow.desired_flows. */ + struct desired_flow *flow; struct uuid sb_uuid; }; -static struct ovn_flow *ovn_flow_alloc(uint8_t table_id, uint16_t priority, - uint64_t cookie, - const struct match *match, - const struct ofpbuf *actions); +/* A installed flow, in static variable installed_flows. + * + * Installed flows are updated in ofctrl_put for maintaining the flow + * installation to OVS. They are updated according to desired flows: either by + * processing the tracked desired flow changes, or by comparing desired flows + * with currently installed flows when tracked desired flows changes are not + * available. + * + * In addition, when ofctrl state machine enters S_CLEAR, the installed flows + * will be cleared. (This happens in initialization phase and also when + * ovs-vswitchd is disconnected/reconnected). + * + * Links are maintained between installed flows and desired flows. The + * relationship is 1 to N. A link is added when a flow addition is processed. + * A link is removed when a flow deletion is processed, the desired flow + * table is cleared, or the installed flow table is cleared. + */ +struct installed_flow { + struct ovn_flow flow; + struct hmap_node match_hmap_node; /* For match based hashing. */ + + /* A list of desired ovn_flow nodes (linked by + * desired_flow.installed_ref_list_node), which reference this installed + * flow. (There are cases that multiple desired flows reference the same + * installed flow, e.g. when there are conflict/duplicated ACLs that + * generates same match conditions). */ + struct ovs_list desired_refs; + + /* The corresponding flow in desired table. It must be one of the flows in + * desired_refs list. If there are more than one flows in references list, + * this is the one that is actually installed. */ + struct desired_flow *desired_flow; +}; + +static struct desired_flow *desired_flow_alloc( + uint8_t table_id, + uint16_t priority, + uint64_t cookie, + const struct match *match, + const struct ofpbuf *actions); +static struct desired_flow *desired_flow_lookup( + struct ovn_desired_flow_table *, + const struct ovn_flow *target, + const struct uuid *sb_uuid); +static void desired_flow_destroy(struct desired_flow *); + +static struct installed_flow *installed_flow_lookup( + const struct ovn_flow *target); +static void installed_flow_destroy(struct installed_flow *); +static struct installed_flow *installed_flow_dup(struct desired_flow *); + static uint32_t ovn_flow_match_hash(const struct ovn_flow *); -static struct ovn_flow *ovn_flow_lookup(struct hmap *flow_table, - const struct ovn_flow *target, - const struct uuid *sb_uuid); static char *ovn_flow_to_string(const struct ovn_flow *); static void ovn_flow_log(const struct ovn_flow *, const char *action); -static void ovn_flow_destroy(struct ovn_flow *); /* OpenFlow connection to the switch. */ static struct rconn *swconn; @@ -216,7 +276,6 @@ static struct ofpbuf *encode_meter_mod(const struct ofputil_meter_mod *); static void ovn_installed_flow_table_clear(void); static void ovn_installed_flow_table_destroy(void); -static struct ovn_flow *ovn_flow_dup(struct ovn_flow *source); static void ofctrl_recv(const struct ofp_header *, enum ofptype); @@ -691,6 +750,45 @@ ofctrl_recv(const struct ofp_header *oh, enum ofptype type) } } +static void +link_installed_to_desired(struct installed_flow *i, struct desired_flow *d) +{ + if (i->desired_flow == d) { + return; + } + + if (ovs_list_is_empty(&i->desired_refs)) { + ovs_assert(!i->desired_flow); + i->desired_flow = d; + } + ovs_list_insert(&i->desired_refs, &d->installed_ref_list_node); + d->installed_flow = i; +} + +static void +unlink_installed_to_desired(struct installed_flow *i, struct desired_flow *d) +{ + ovs_assert(i && i->desired_flow && !ovs_list_is_empty(&i->desired_refs)); + ovs_assert(d && d->installed_flow == i); + ovs_list_remove(&d->installed_ref_list_node); + d->installed_flow = NULL; + if (i->desired_flow == d) { + i->desired_flow = ovs_list_is_empty(&i->desired_refs) ? NULL : + CONTAINER_OF(ovs_list_front(&i->desired_refs), + struct desired_flow, + installed_ref_list_node); + } +} + +static void +unlink_all_refs_for_installed_flow(struct installed_flow *i) +{ + struct desired_flow *d, *next; + LIST_FOR_EACH_SAFE (d, next, installed_ref_list_node, &i->desired_refs) { + unlink_installed_to_desired(i, d); + } +} + static struct sb_to_flow * sb_to_flow_find(struct hmap *uuid_flow_table, const struct uuid *sb_uuid) { @@ -706,7 +804,7 @@ sb_to_flow_find(struct hmap *uuid_flow_table, const struct uuid *sb_uuid) static void link_flow_to_sb(struct ovn_desired_flow_table *flow_table, - struct ovn_flow *f, const struct uuid *sb_uuid) + struct desired_flow *f, const struct uuid *sb_uuid) { struct sb_flow_ref *sfr = xmalloc(sizeof *sfr); sfr->flow = f; @@ -744,26 +842,26 @@ ofctrl_check_and_add_flow(struct ovn_desired_flow_table *flow_table, const struct uuid *sb_uuid, bool log_duplicate_flow) { - struct ovn_flow *f = ovn_flow_alloc(table_id, priority, cookie, match, - actions); + struct desired_flow *f = desired_flow_alloc(table_id, priority, cookie, + match, actions); - if (ovn_flow_lookup(&flow_table->match_flow_table, f, sb_uuid)) { + if (desired_flow_lookup(flow_table, &f->flow, sb_uuid)) { if (log_duplicate_flow) { static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5); if (!VLOG_DROP_DBG(&rl)) { - char *s = ovn_flow_to_string(f); + char *s = ovn_flow_to_string(&f->flow); VLOG_DBG("dropping duplicate flow: %s", s); free(s); } } - ovn_flow_destroy(f); + desired_flow_destroy(f); return; } hmap_insert(&flow_table->match_flow_table, &f->match_hmap_node, - f->match_hmap_node.hash); + f->flow.hash); link_flow_to_sb(flow_table, f, sb_uuid); - ovn_flow_log(f, "ofctrl_add_flow"); + ovn_flow_log(&f->flow, "ofctrl_add_flow"); } void @@ -786,11 +884,11 @@ ofctrl_add_or_append_flow(struct ovn_desired_flow_table *desired_flows, const struct ofpbuf *actions, const struct uuid *sb_uuid) { - struct ovn_flow *f = ovn_flow_alloc(table_id, priority, cookie, match, - actions); + struct desired_flow *f = desired_flow_alloc(table_id, priority, cookie, + match, actions); - struct ovn_flow *existing; - existing = ovn_flow_lookup(&desired_flows->match_flow_table, f, NULL); + struct desired_flow *existing; + existing = desired_flow_lookup(desired_flows, &f->flow, NULL); if (existing) { /* There's already a flow with this particular match. Append the * action to that flow rather than adding a new flow @@ -798,26 +896,27 @@ ofctrl_add_or_append_flow(struct ovn_desired_flow_table *desired_flows, uint64_t compound_stub[64 / 8]; struct ofpbuf compound; ofpbuf_use_stub(&compound, compound_stub, sizeof(compound_stub)); - ofpbuf_put(&compound, existing->ofpacts, existing->ofpacts_len); - ofpbuf_put(&compound, f->ofpacts, f->ofpacts_len); + ofpbuf_put(&compound, existing->flow.ofpacts, + existing->flow.ofpacts_len); + ofpbuf_put(&compound, f->flow.ofpacts, f->flow.ofpacts_len); - free(existing->ofpacts); - existing->ofpacts = xmemdup(compound.data, compound.size); - existing->ofpacts_len = compound.size; + free(existing->flow.ofpacts); + existing->flow.ofpacts = xmemdup(compound.data, compound.size); + existing->flow.ofpacts_len = compound.size; ofpbuf_uninit(&compound); - ovn_flow_destroy(f); + desired_flow_destroy(f); f = existing; } else { hmap_insert(&desired_flows->match_flow_table, &f->match_hmap_node, - f->match_hmap_node.hash); + f->flow.hash); } link_flow_to_sb(desired_flows, f, sb_uuid); if (existing) { - ovn_flow_log(f, "ofctrl_add_or_append_flow (append)"); + ovn_flow_log(&f->flow, "ofctrl_add_or_append_flow (append)"); } else { - ovn_flow_log(f, "ofctrl_add_or_append_flow (add)"); + ovn_flow_log(&f->flow, "ofctrl_add_or_append_flow (add)"); } } @@ -833,16 +932,19 @@ remove_flows_from_sb_to_flow(struct ovn_desired_flow_table *flow_table, LIST_FOR_EACH_SAFE (sfr, next, flow_list, &stf->flows) { ovs_list_remove(&sfr->sb_list); ovs_list_remove(&sfr->flow_list); - struct ovn_flow *f = sfr->flow; + struct desired_flow *f = sfr->flow; free(sfr); if (ovs_list_is_empty(&f->references)) { if (log_msg) { - ovn_flow_log(f, log_msg); + ovn_flow_log(&f->flow, log_msg); } hmap_remove(&flow_table->match_flow_table, &f->match_hmap_node); - ovn_flow_destroy(f); + if (f->installed_flow) { + unlink_installed_to_desired(f->installed_flow, f); + } + desired_flow_destroy(f); } } hmap_remove(&flow_table->uuid_flow_table, &stf->hmap_node); @@ -903,8 +1005,8 @@ flood_remove_flows_for_sb_uuid(struct ovn_desired_flow_table *flow_table, /* Traverse all flows for the given sb_uuid. */ struct sb_flow_ref *sfr, *next; LIST_FOR_EACH_SAFE (sfr, next, flow_list, &stf->flows) { - struct ovn_flow *f = sfr->flow; - ovn_flow_log(f, "flood remove"); + struct desired_flow *f = sfr->flow; + ovn_flow_log(&f->flow, "flood remove"); ovs_list_remove(&sfr->sb_list); ovs_list_remove(&sfr->flow_list); @@ -917,7 +1019,10 @@ flood_remove_flows_for_sb_uuid(struct ovn_desired_flow_table *flow_table, * be empty in most cases. */ hmap_remove(&flow_table->match_flow_table, &f->match_hmap_node); - ovn_flow_destroy(f); + if (f->installed_flow) { + unlink_installed_to_desired(f->installed_flow, f); + } + desired_flow_destroy(f); } else { ovs_list_insert(&to_be_removed, &f->list_node); } @@ -930,7 +1035,7 @@ flood_remove_flows_for_sb_uuid(struct ovn_desired_flow_table *flow_table, /* Detach the items in f->references from the sfr.flow_list lists, * so that recursive calls will not mess up the sfr.sb_list list. */ - struct ovn_flow *f, *f_next; + struct desired_flow *f, *f_next; LIST_FOR_EACH (f, list_node, &to_be_removed) { ovs_assert(!ovs_list_is_empty(&f->references)); LIST_FOR_EACH (sfr, sb_list, &f->references) { @@ -951,7 +1056,10 @@ flood_remove_flows_for_sb_uuid(struct ovn_desired_flow_table *flow_table, ovs_list_remove(&f->list_node); hmap_remove(&flow_table->match_flow_table, &f->match_hmap_node); - ovn_flow_destroy(f); + if (f->installed_flow) { + unlink_installed_to_desired(f->installed_flow, f); + } + desired_flow_destroy(f); } } @@ -973,22 +1081,32 @@ ofctrl_flood_remove_flows(struct ovn_desired_flow_table *flow_table, } } -/* ovn_flow. */ +/* flow operations. */ -static struct ovn_flow * -ovn_flow_alloc(uint8_t table_id, uint16_t priority, uint64_t cookie, - const struct match *match, const struct ofpbuf *actions) +static void +ovn_flow_init(struct ovn_flow *f, uint8_t table_id, uint16_t priority, + uint64_t cookie, const struct match *match, + const struct ofpbuf *actions) { - struct ovn_flow *f = xmalloc(sizeof *f); - ovs_list_init(&f->references); - ovs_list_init(&f->list_node); f->table_id = table_id; f->priority = priority; minimatch_init(&f->match, match); f->ofpacts = xmemdup(actions->data, actions->size); f->ofpacts_len = actions->size; - f->match_hmap_node.hash = ovn_flow_match_hash(f); + f->hash = ovn_flow_match_hash(f); f->cookie = cookie; +} + +static struct desired_flow * +desired_flow_alloc(uint8_t table_id, uint16_t priority, uint64_t cookie, + const struct match *match, const struct ofpbuf *actions) +{ + struct desired_flow *f = xmalloc(sizeof *f); + ovs_list_init(&f->references); + ovs_list_init(&f->list_node); + ovs_list_init(&f->installed_ref_list_node); + f->installed_flow = NULL; + ovn_flow_init(&f->flow, table_id, priority, cookie, match, actions); return f; } @@ -1001,48 +1119,47 @@ ovn_flow_match_hash(const struct ovn_flow *f) minimatch_hash(&f->match, 0)); } -/* Duplicate an ovn_flow structure. */ -static struct ovn_flow * -ovn_flow_dup(struct ovn_flow *src) +/* Duplicate a desired flow to an installed flow. */ +static struct installed_flow * +installed_flow_dup(struct desired_flow *src) { - struct ovn_flow *dst = xmalloc(sizeof *dst); - ovs_list_init(&dst->references); - dst->table_id = src->table_id; - dst->priority = src->priority; - minimatch_clone(&dst->match, &src->match); - dst->ofpacts = xmemdup(src->ofpacts, src->ofpacts_len); - dst->ofpacts_len = src->ofpacts_len; - dst->match_hmap_node.hash = src->match_hmap_node.hash; - dst->cookie = src->cookie; + struct installed_flow *dst = xmalloc(sizeof *dst); + ovs_list_init(&dst->desired_refs); + dst->desired_flow = NULL; + dst->flow.table_id = src->flow.table_id; + dst->flow.priority = src->flow.priority; + minimatch_clone(&dst->flow.match, &src->flow.match); + dst->flow.ofpacts = xmemdup(src->flow.ofpacts, src->flow.ofpacts_len); + dst->flow.ofpacts_len = src->flow.ofpacts_len; + dst->flow.hash = src->flow.hash; + dst->flow.cookie = src->flow.cookie; return dst; } -/* Finds and returns an ovn_flow in 'flow_table' whose key is identical to +/* Finds and returns a desired_flow in 'flow_table' whose key is identical to * 'target''s key, or NULL if there is none. * * If sb_uuid is not NULL, the function will also check if the found flow is - * referenced by the sb_uuid. - * - * NOTE: sb_uuid can only be used for ovn_desired_flow_table lookup. */ -static struct ovn_flow * -ovn_flow_lookup(struct hmap *flow_table, const struct ovn_flow *target, - const struct uuid *sb_uuid) + * referenced by the sb_uuid. */ +static struct desired_flow * +desired_flow_lookup(struct ovn_desired_flow_table *flow_table, + const struct ovn_flow *target, + const struct uuid *sb_uuid) { - struct ovn_flow *f; - - HMAP_FOR_EACH_WITH_HASH (f, match_hmap_node, target->match_hmap_node.hash, - flow_table) { + struct desired_flow *d; + HMAP_FOR_EACH_WITH_HASH (d, match_hmap_node, target->hash, + &flow_table->match_flow_table) { + struct ovn_flow *f = &d->flow; if (f->table_id == target->table_id && f->priority == target->priority && minimatch_equal(&f->match, &target->match)) { if (!sb_uuid) { - return f; + return d; } - ovs_assert(flow_table != &installed_flows); struct sb_flow_ref *sfr; - LIST_FOR_EACH (sfr, sb_list, &f->references) { + LIST_FOR_EACH (sfr, sb_list, &d->references) { if (uuid_equals(sb_uuid, &sfr->sb_uuid)) { - return f; + return d; } } } @@ -1050,6 +1167,24 @@ ovn_flow_lookup(struct hmap *flow_table, const struct ovn_flow *target, return NULL; } +/* Finds and returns an installed_flow in installed_flows whose key is + * identical to 'target''s key, or NULL if there is none. */ +static struct installed_flow * +installed_flow_lookup(const struct ovn_flow *target) +{ + struct installed_flow *i; + HMAP_FOR_EACH_WITH_HASH (i, match_hmap_node, target->hash, + &installed_flows) { + struct ovn_flow *f = &i->flow; + if (f->table_id == target->table_id + && f->priority == target->priority + && minimatch_equal(&f->match, &target->match)) { + return i; + } + } + return NULL; +} + static char * ovn_flow_to_string(const struct ovn_flow *f) { @@ -1076,17 +1211,35 @@ ovn_flow_log(const struct ovn_flow *f, const char *action) } static void -ovn_flow_destroy(struct ovn_flow *f) +ovn_flow_uninit(struct ovn_flow *f) +{ + minimatch_destroy(&f->match); + free(f->ofpacts); +} + +static void +desired_flow_destroy(struct desired_flow *f) { if (f) { ovs_assert(ovs_list_is_empty(&f->references)); - minimatch_destroy(&f->match); - free(f->ofpacts); + ovs_assert(!f->installed_flow); + ovn_flow_uninit(&f->flow); + free(f); + } +} + +static void +installed_flow_destroy(struct installed_flow *f) +{ + if (f) { + ovs_assert(ovs_list_is_empty(&f->desired_refs)); + ovs_assert(!f->desired_flow); + ovn_flow_uninit(&f->flow); free(f); } } -/* Flow tables of struct ovn_flow. */ +/* Desired flow table operations. */ void ovn_desired_flow_table_init(struct ovn_desired_flow_table *flow_table) { @@ -1112,13 +1265,16 @@ ovn_desired_flow_table_destroy(struct ovn_desired_flow_table *flow_table) hmap_destroy(&flow_table->uuid_flow_table); } + +/* Installed flow table operations. */ static void ovn_installed_flow_table_clear(void) { - struct ovn_flow *f, *next; + struct installed_flow *f, *next; HMAP_FOR_EACH_SAFE (f, next, match_hmap_node, &installed_flows) { hmap_remove(&installed_flows, &f->match_hmap_node); - ovn_flow_destroy(f); + unlink_all_refs_for_installed_flow(f); + installed_flow_destroy(f); } } @@ -1441,81 +1597,85 @@ ofctrl_put(struct ovn_desired_flow_table *flow_table, /* Iterate through all of the installed flows. If any of them are no * longer desired, delete them; if any of them should have different * actions, update them. */ - struct ovn_flow *i, *next; + struct installed_flow *i, *next; HMAP_FOR_EACH_SAFE (i, next, match_hmap_node, &installed_flows) { - struct ovn_flow *d = ovn_flow_lookup(&flow_table->match_flow_table, - i, NULL); + unlink_all_refs_for_installed_flow(i); + struct desired_flow *d = desired_flow_lookup(flow_table, &i->flow, + NULL); if (!d) { /* Installed flow is no longer desirable. Delete it from the * switch and from installed_flows. */ struct ofputil_flow_mod fm = { - .match = i->match, - .priority = i->priority, - .table_id = i->table_id, + .match = i->flow.match, + .priority = i->flow.priority, + .table_id = i->flow.table_id, .command = OFPFC_DELETE_STRICT, }; add_flow_mod(&fm, &msgs); - ovn_flow_log(i, "removing installed"); + ovn_flow_log(&i->flow, "removing installed"); hmap_remove(&installed_flows, &i->match_hmap_node); - ovn_flow_destroy(i); + installed_flow_destroy(i); } else { - if (!ofpacts_equal(i->ofpacts, i->ofpacts_len, - d->ofpacts, d->ofpacts_len) || - i->cookie != d->cookie) { + if (!ofpacts_equal(i->flow.ofpacts, i->flow.ofpacts_len, + d->flow.ofpacts, d->flow.ofpacts_len) || + i->flow.cookie != d->flow.cookie) { /* Update actions in installed flow. */ struct ofputil_flow_mod fm = { - .match = i->match, - .priority = i->priority, - .table_id = i->table_id, - .ofpacts = d->ofpacts, - .ofpacts_len = d->ofpacts_len, + .match = i->flow.match, + .priority = i->flow.priority, + .table_id = i->flow.table_id, + .ofpacts = d->flow.ofpacts, + .ofpacts_len = d->flow.ofpacts_len, .command = OFPFC_MODIFY_STRICT, }; /* Update cookie if it is changed. */ - if (i->cookie != d->cookie) { + if (i->flow.cookie != d->flow.cookie) { fm.modify_cookie = true; - fm.new_cookie = htonll(d->cookie); + fm.new_cookie = htonll(d->flow.cookie); /* Use OFPFC_ADD so that cookie can be updated. */ fm.command = OFPFC_ADD, - i->cookie = d->cookie; + i->flow.cookie = d->flow.cookie; } add_flow_mod(&fm, &msgs); - ovn_flow_log(i, "updating installed"); + ovn_flow_log(&i->flow, "updating installed"); /* Replace 'i''s actions by 'd''s. */ - free(i->ofpacts); - i->ofpacts = xmemdup(d->ofpacts, d->ofpacts_len); - i->ofpacts_len = d->ofpacts_len; + free(i->flow.ofpacts); + i->flow.ofpacts = xmemdup(d->flow.ofpacts, + d->flow.ofpacts_len); + i->flow.ofpacts_len = d->flow.ofpacts_len; } + link_installed_to_desired(i, d); } } /* Iterate through the desired flows and add those that aren't found * in the installed flow table. */ - struct ovn_flow *d; + struct desired_flow *d; HMAP_FOR_EACH (d, match_hmap_node, &flow_table->match_flow_table) { - i = ovn_flow_lookup(&installed_flows, d, NULL); + i = installed_flow_lookup(&d->flow); if (!i) { /* Send flow_mod to add flow. */ struct ofputil_flow_mod fm = { - .match = d->match, - .priority = d->priority, - .table_id = d->table_id, - .ofpacts = d->ofpacts, - .ofpacts_len = d->ofpacts_len, - .new_cookie = htonll(d->cookie), + .match = d->flow.match, + .priority = d->flow.priority, + .table_id = d->flow.table_id, + .ofpacts = d->flow.ofpacts, + .ofpacts_len = d->flow.ofpacts_len, + .new_cookie = htonll(d->flow.cookie), .command = OFPFC_ADD, }; add_flow_mod(&fm, &msgs); - ovn_flow_log(d, "adding installed"); + ovn_flow_log(&d->flow, "adding installed"); /* Copy 'd' from 'flow_table' to installed_flows. */ - struct ovn_flow *new_node = ovn_flow_dup(d); - hmap_insert(&installed_flows, &new_node->match_hmap_node, - new_node->match_hmap_node.hash); + i = installed_flow_dup(d); + hmap_insert(&installed_flows, &i->match_hmap_node, + i->flow.hash); } + link_installed_to_desired(i, d); } /* Iterate through the installed groups from previous runs. If they From patchwork Mon Sep 7 06:45:40 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Han Zhou X-Patchwork-Id: 1358621 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 4BlJm90jjvz9sR4 for ; Mon, 7 Sep 2020 16:49:13 +1000 (AEST) Received: from localhost (localhost [127.0.0.1]) by silver.osuosl.org (Postfix) with ESMTP id 718612051E; Mon, 7 Sep 2020 06:49:11 +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 km9-M00pmj32; Mon, 7 Sep 2020 06:48:56 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by silver.osuosl.org (Postfix) with ESMTP id D35FE204E0; Mon, 7 Sep 2020 06:47:25 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id ABE94C0893; Mon, 7 Sep 2020 06:47:25 +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 A0525C0051 for ; Mon, 7 Sep 2020 06:47:24 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by silver.osuosl.org (Postfix) with ESMTP id 723FF20533 for ; Mon, 7 Sep 2020 06:47:24 +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 SF+jV8h6e8Ky for ; Mon, 7 Sep 2020 06:47:17 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from relay11.mail.gandi.net (relay11.mail.gandi.net [217.70.178.231]) by silver.osuosl.org (Postfix) with ESMTPS id 948BE20763 for ; Mon, 7 Sep 2020 06:46:38 +0000 (UTC) Received: from localhost.localdomain.localdomain (unknown [73.241.94.255]) (Authenticated sender: hzhou@ovn.org) by relay11.mail.gandi.net (Postfix) with ESMTPSA id 01F6F10000A; Mon, 7 Sep 2020 06:46:34 +0000 (UTC) From: Han Zhou To: dev@openvswitch.org Date: Sun, 6 Sep 2020 23:45:40 -0700 Message-Id: <1599461142-84752-8-git-send-email-hzhou@ovn.org> X-Mailer: git-send-email 2.1.0 In-Reply-To: <1599461142-84752-1-git-send-email-hzhou@ovn.org> References: <1599461142-84752-1-git-send-email-hzhou@ovn.org> Cc: Han Zhou Subject: [ovs-dev] [PATCH ovn v2 7/9] ofctrl.c: Refactor - move openflow msg construction to functions. 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" These functions will be reused in multiple places in a future patch. Signed-off-by: Han Zhou --- controller/ofctrl.c | 103 ++++++++++++++++++++++++++++++---------------------- 1 file changed, 60 insertions(+), 43 deletions(-) diff --git a/controller/ofctrl.c b/controller/ofctrl.c index ecea081..bc6a115 100644 --- a/controller/ofctrl.c +++ b/controller/ofctrl.c @@ -1480,6 +1480,63 @@ add_meter(struct ovn_extend_table_info *m_desired, free(mm.meter.bands); } +static void +installed_flow_add(struct ovn_flow *d, struct ovs_list *msgs) +{ + /* Send flow_mod to add flow. */ + struct ofputil_flow_mod fm = { + .match = d->match, + .priority = d->priority, + .table_id = d->table_id, + .ofpacts = d->ofpacts, + .ofpacts_len = d->ofpacts_len, + .new_cookie = htonll(d->cookie), + .command = OFPFC_ADD, + }; + add_flow_mod(&fm, msgs); +} + +static void +installed_flow_mod(struct ovn_flow *i, struct ovn_flow *d, + struct ovs_list *msgs) +{ + /* Update actions in installed flow. */ + struct ofputil_flow_mod fm = { + .match = i->match, + .priority = i->priority, + .table_id = i->table_id, + .ofpacts = d->ofpacts, + .ofpacts_len = d->ofpacts_len, + .command = OFPFC_MODIFY_STRICT, + }; + /* Update cookie if it is changed. */ + if (i->cookie != d->cookie) { + fm.modify_cookie = true; + fm.new_cookie = htonll(d->cookie); + /* Use OFPFC_ADD so that cookie can be updated. */ + fm.command = OFPFC_ADD; + } + add_flow_mod(&fm, msgs); + + /* Replace 'i''s actions and cookie by 'd''s. */ + free(i->ofpacts); + i->ofpacts = xmemdup(d->ofpacts, d->ofpacts_len); + i->ofpacts_len = d->ofpacts_len; + i->cookie = d->cookie; +} + +static void +installed_flow_del(struct ovn_flow *i, struct ovs_list *msgs) +{ + struct ofputil_flow_mod fm = { + .match = i->match, + .priority = i->priority, + .table_id = i->table_id, + .command = OFPFC_DELETE_STRICT, + }; + add_flow_mod(&fm, msgs); +} + /* The flow table can be updated if the connection to the switch is up and * in the correct state and not backlogged with existing flow_mods. (Our * criteria for being backlogged appear very conservative, but the socket @@ -1605,46 +1662,16 @@ ofctrl_put(struct ovn_desired_flow_table *flow_table, if (!d) { /* Installed flow is no longer desirable. Delete it from the * switch and from installed_flows. */ - struct ofputil_flow_mod fm = { - .match = i->flow.match, - .priority = i->flow.priority, - .table_id = i->flow.table_id, - .command = OFPFC_DELETE_STRICT, - }; - add_flow_mod(&fm, &msgs); + installed_flow_del(&i->flow, &msgs); ovn_flow_log(&i->flow, "removing installed"); - hmap_remove(&installed_flows, &i->match_hmap_node); installed_flow_destroy(i); } else { if (!ofpacts_equal(i->flow.ofpacts, i->flow.ofpacts_len, d->flow.ofpacts, d->flow.ofpacts_len) || i->flow.cookie != d->flow.cookie) { - /* Update actions in installed flow. */ - struct ofputil_flow_mod fm = { - .match = i->flow.match, - .priority = i->flow.priority, - .table_id = i->flow.table_id, - .ofpacts = d->flow.ofpacts, - .ofpacts_len = d->flow.ofpacts_len, - .command = OFPFC_MODIFY_STRICT, - }; - /* Update cookie if it is changed. */ - if (i->flow.cookie != d->flow.cookie) { - fm.modify_cookie = true; - fm.new_cookie = htonll(d->flow.cookie); - /* Use OFPFC_ADD so that cookie can be updated. */ - fm.command = OFPFC_ADD, - i->flow.cookie = d->flow.cookie; - } - add_flow_mod(&fm, &msgs); ovn_flow_log(&i->flow, "updating installed"); - - /* Replace 'i''s actions by 'd''s. */ - free(i->flow.ofpacts); - i->flow.ofpacts = xmemdup(d->flow.ofpacts, - d->flow.ofpacts_len); - i->flow.ofpacts_len = d->flow.ofpacts_len; + installed_flow_mod(&i->flow, &d->flow, &msgs); } link_installed_to_desired(i, d); @@ -1657,17 +1684,7 @@ ofctrl_put(struct ovn_desired_flow_table *flow_table, HMAP_FOR_EACH (d, match_hmap_node, &flow_table->match_flow_table) { i = installed_flow_lookup(&d->flow); if (!i) { - /* Send flow_mod to add flow. */ - struct ofputil_flow_mod fm = { - .match = d->flow.match, - .priority = d->flow.priority, - .table_id = d->flow.table_id, - .ofpacts = d->flow.ofpacts, - .ofpacts_len = d->flow.ofpacts_len, - .new_cookie = htonll(d->flow.cookie), - .command = OFPFC_ADD, - }; - add_flow_mod(&fm, &msgs); + installed_flow_add(&d->flow, &msgs); ovn_flow_log(&d->flow, "adding installed"); /* Copy 'd' from 'flow_table' to installed_flows. */ From patchwork Mon Sep 7 06:45:41 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Han Zhou X-Patchwork-Id: 1358620 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 4BlJls1bZfz9sR4 for ; Mon, 7 Sep 2020 16:48:57 +1000 (AEST) Received: from localhost (localhost [127.0.0.1]) by silver.osuosl.org (Postfix) with ESMTP id 6EF37204AA; Mon, 7 Sep 2020 06:48:55 +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 EkFTrRefNEmU; Mon, 7 Sep 2020 06:48:33 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by silver.osuosl.org (Postfix) with ESMTP id 7D18620BF9; Mon, 7 Sep 2020 06:47:21 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id 641B6C0891; Mon, 7 Sep 2020 06:47:21 +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 AE515C0051 for ; Mon, 7 Sep 2020 06:47:20 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by whitealder.osuosl.org (Postfix) with ESMTP id 5E18D86272 for ; Mon, 7 Sep 2020 06:47:20 +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 F+th8odUnMHE for ; Mon, 7 Sep 2020 06:47:07 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from relay11.mail.gandi.net (relay11.mail.gandi.net [217.70.178.231]) by whitealder.osuosl.org (Postfix) with ESMTPS id 77765865CF for ; Mon, 7 Sep 2020 06:46:56 +0000 (UTC) Received: from localhost.localdomain.localdomain (unknown [73.241.94.255]) (Authenticated sender: hzhou@ovn.org) by relay11.mail.gandi.net (Postfix) with ESMTPSA id B83B310000E; Mon, 7 Sep 2020 06:46:37 +0000 (UTC) From: Han Zhou To: dev@openvswitch.org Date: Sun, 6 Sep 2020 23:45:41 -0700 Message-Id: <1599461142-84752-9-git-send-email-hzhou@ovn.org> X-Mailer: git-send-email 2.1.0 In-Reply-To: <1599461142-84752-1-git-send-email-hzhou@ovn.org> References: <1599461142-84752-1-git-send-email-hzhou@ovn.org> MIME-Version: 1.0 Cc: Han Zhou Subject: [ovs-dev] [PATCH ovn v2 8/9] ofctrl: Incremental processing for flow installation by tracking. 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" With incremental processing for flow computation, the bottle neck of ovn-controller in large scale environment is in the flow installation (ofctrl_put()), which does full comparison between the two big flow tables: the installed flows and desired flows. This patch implements tracking desired flow changes when flows are incrementally computed by I-P engine, and then incrementally processing the flow installation using the tracked information in ofctrl_put(). It falls back to the full comparison whenever tracking information is unavailable, e.g. when I-P engine triggers full recompute. In ovn-scale-test with 3000 HVs and 30k lports, the end-to-end latency between the moment a lflow is updated in SB DB and the moment when all the 3K HVs complete OVS flow updating has reduced around 60% (from 1s to 400ms). Below is the perf result for a ovn-controller processing a new port-binding: Beore: + 96.76% 0.00% ovn-controller [unknown] [k] 0xffffffffffffffff + 90.21% 0.00% ovn-controller ovn-controller [.] main + 39.93% 1.19% ovn-controller ovn-controller [.] ofctrl_put + 31.27% 12.47% ovn-controller ovn-controller [.] ovn_flow_lookup + 22.12% 3.12% ovn-controller ovn-controller [.] encaps_run + 18.69% 2.77% ovn-controller ovn-controller [.] minimatch_equal + 17.63% 4.11% ovn-controller ovn-controller [.] patch_run + 15.91% 0.00% ovn-controller ovn-controller [.] add_bridge_mappings (inlined) + 14.03% 12.08% ovn-controller ovn-controller [.] minimask_equal + 12.41% 0.00% ovn-controller ovn-controller [.] chassis_tunnel_add (inlined) + 11.40% 0.00% ovn-controller ovn-controller [.] tunnel_add (inlined) After: + 94.59% 0.00% ovn-controller [unknown] [k] 0xffffffffffffffff + 83.56% 0.09% ovn-controller ovn-controller [.] main + 35.37% 3.13% ovn-controller ovn-controller [.] encaps_run + 27.54% 7.53% ovn-controller ovn-controller [.] patch_run + 24.86% 0.00% ovn-controller ovn-controller [.] add_bridge_mappings (inlined) + 20.01% 0.00% ovn-controller ovn-controller [.] chassis_tunnel_add (inlined) + 18.51% 0.00% ovn-controller ovn-controller [.] tunnel_add (inlined) + 14.08% 0.17% ovn-controller ovn-controller [.] physical_run + 11.37% 11.28% ovn-controller ovn-controller [.] next_real_row + 10.50% 2.59% ovn-controller ovn-controller [.] consider_port_binding ... + 2.14% 0.32% ovn-controller ovn-controller [.] ofctrl_put ▒ Before the optimization, ofctrl_put took 40% of CPU, and now it disappears from the hot spots. Signed-off-by: Han Zhou --- controller/ofctrl.c | 289 ++++++++++++++++++++++++++++++++++++++++++---------- controller/ofctrl.h | 6 +- 2 files changed, 240 insertions(+), 55 deletions(-) diff --git a/controller/ofctrl.c b/controller/ofctrl.c index bc6a115..a2aa45d 100644 --- a/controller/ofctrl.c +++ b/controller/ofctrl.c @@ -101,6 +101,39 @@ struct ovn_flow { * * The links are updated whenever there is a change in desired flows, which is * usually triggered by a SB data change in I-P engine. + * + * ** Tracking ** + * + * A desired flow can be tracked - listed in ovn_desired_flow_table's + * tracked_flows. + * + * Tracked flows is initially empty, and stays empty after the first run of I-P + * engine when installed flows are initially populated. After that, flow + * changes are tracked when I-P engine incrementally computes flow changes. + * Tracked flows are then processed and removed completely in ofctrl_put. + * ("processed" means OpenFlow change messages are composed and sent/queued to + * OVS, which ensures flows in OVS is always in sync (eventually) with the + * installed flows table). + * + * In case of full recompute of I-P engine, tracked flows are not + * added/removed, and ofctrl_put will not rely on tracked flows. (It is I-P + * engine's responsibility to ensure the tracked flows are cleared before + * recompute). + * + * Tracked flows can be preserved across multiple I-P engine runs - if in some + * iterations ofctrl_put() is skipped. Tracked flows are cleared only when it + * is consumed or when flow recompute happens. + * + * The "change_tracked" member of desired flow table maintains the status of + * whether flow changes are tracked or not. It is always set to true when + * ofctrl_put is completed, and transition to false whenever + * ovn_desired_flow_table_clear is called. + * + * NOTE: A tracked flow is just a reference to a desired flow, instead of a new + * copy. When a desired flow is removed and tracked, it is removed from the + * match_flow_table and uuid_flow_table indexes, and added to the tracked_flows + * list, marking is_deleted = true, but not immediately destroyed. It is + * destroyed when the tracking is processed for installed flow updates. */ struct desired_flow { struct ovn_flow flow; @@ -117,6 +150,11 @@ struct desired_flow { /* Node in installed_flow.desired_refs list. */ struct ovs_list installed_ref_list_node; + + /* For tracking. */ + struct ovs_list track_list_node; /* node in ovn_desired_flow_table's + * tracked_flows list. */ + bool is_deleted; /* If the tracked flow is deleted. */ }; struct sb_to_flow { @@ -789,6 +827,62 @@ unlink_all_refs_for_installed_flow(struct installed_flow *i) } } +static void +track_flow_add_or_modify(struct ovn_desired_flow_table *flow_table, + struct desired_flow *f) +{ + if (!flow_table->change_tracked) { + return; + } + + /* If same node (flow adding/modifying) was tracked, remove it from + * tracking first. */ + if (!ovs_list_is_empty(&f->track_list_node)) { + ovs_list_remove(&f->track_list_node); + } + f->is_deleted = false; + ovs_list_push_back(&flow_table->tracked_flows, &f->track_list_node); + +} + +static void +track_flow_del(struct ovn_desired_flow_table *flow_table, + struct desired_flow *f) +{ + if (!flow_table->change_tracked) { + return; + } + /* If same node (flow adding/modifying) was tracked, remove it from + * tracking first. */ + if (!ovs_list_is_empty(&f->track_list_node)) { + ovs_list_remove(&f->track_list_node); + if (!f->installed_flow) { + /* If it is not installed yet, simply destroy it. */ + desired_flow_destroy(f); + return; + } + } + f->is_deleted = true; + ovs_list_push_back(&flow_table->tracked_flows, &f->track_list_node); +} + +/* When a desired flow is being removed, depending on "change_tracked", this + * function either unlinks a desired flow from installed flow and destroy it, + * or do nothing but track it. */ +static void +track_or_destroy_for_flow_del(struct ovn_desired_flow_table *flow_table, + struct desired_flow *f) +{ + if (flow_table->change_tracked) { + track_flow_del(flow_table, f); + } else { + if (f->installed_flow) { + unlink_installed_to_desired(f->installed_flow, f); + } + desired_flow_destroy(f); + } +} + static struct sb_to_flow * sb_to_flow_find(struct hmap *uuid_flow_table, const struct uuid *sb_uuid) { @@ -861,6 +955,7 @@ ofctrl_check_and_add_flow(struct ovn_desired_flow_table *flow_table, hmap_insert(&flow_table->match_flow_table, &f->match_hmap_node, f->flow.hash); link_flow_to_sb(flow_table, f, sb_uuid); + track_flow_add_or_modify(flow_table, f); ovn_flow_log(&f->flow, "ofctrl_add_flow"); } @@ -912,6 +1007,7 @@ ofctrl_add_or_append_flow(struct ovn_desired_flow_table *desired_flows, f->flow.hash); } link_flow_to_sb(desired_flows, f, sb_uuid); + track_flow_add_or_modify(desired_flows, f); if (existing) { ovn_flow_log(&f->flow, "ofctrl_add_or_append_flow (append)"); @@ -941,10 +1037,7 @@ remove_flows_from_sb_to_flow(struct ovn_desired_flow_table *flow_table, } hmap_remove(&flow_table->match_flow_table, &f->match_hmap_node); - if (f->installed_flow) { - unlink_installed_to_desired(f->installed_flow, f); - } - desired_flow_destroy(f); + track_or_destroy_for_flow_del(flow_table, f); } } hmap_remove(&flow_table->uuid_flow_table, &stf->hmap_node); @@ -1019,10 +1112,7 @@ flood_remove_flows_for_sb_uuid(struct ovn_desired_flow_table *flow_table, * be empty in most cases. */ hmap_remove(&flow_table->match_flow_table, &f->match_hmap_node); - if (f->installed_flow) { - unlink_installed_to_desired(f->installed_flow, f); - } - desired_flow_destroy(f); + track_or_destroy_for_flow_del(flow_table, f); } else { ovs_list_insert(&to_be_removed, &f->list_node); } @@ -1056,10 +1146,7 @@ flood_remove_flows_for_sb_uuid(struct ovn_desired_flow_table *flow_table, ovs_list_remove(&f->list_node); hmap_remove(&flow_table->match_flow_table, &f->match_hmap_node); - if (f->installed_flow) { - unlink_installed_to_desired(f->installed_flow, f); - } - desired_flow_destroy(f); + track_or_destroy_for_flow_del(flow_table, f); } } @@ -1105,7 +1192,9 @@ desired_flow_alloc(uint8_t table_id, uint16_t priority, uint64_t cookie, ovs_list_init(&f->references); ovs_list_init(&f->list_node); ovs_list_init(&f->installed_ref_list_node); + ovs_list_init(&f->track_list_node); f->installed_flow = NULL; + f->is_deleted = false; ovn_flow_init(&f->flow, table_id, priority, cookie, match, actions); return f; @@ -1245,11 +1334,27 @@ ovn_desired_flow_table_init(struct ovn_desired_flow_table *flow_table) { hmap_init(&flow_table->match_flow_table); hmap_init(&flow_table->uuid_flow_table); + ovs_list_init(&flow_table->tracked_flows); + flow_table->change_tracked = false; } void ovn_desired_flow_table_clear(struct ovn_desired_flow_table *flow_table) { + flow_table->change_tracked = false; + + struct desired_flow *f, *f_next; + LIST_FOR_EACH_SAFE (f, f_next, track_list_node, + &flow_table->tracked_flows) { + ovs_list_remove(&f->track_list_node); + if (f->is_deleted) { + if (f->installed_flow) { + unlink_installed_to_desired(f->installed_flow, f); + } + desired_flow_destroy(f); + } + } + struct sb_to_flow *stf, *next; HMAP_FOR_EACH_SAFE (stf, next, hmap_node, &flow_table->uuid_flow_table) { @@ -1537,6 +1642,117 @@ installed_flow_del(struct ovn_flow *i, struct ovs_list *msgs) add_flow_mod(&fm, msgs); } +static void +update_installed_flows_by_compare(struct ovn_desired_flow_table *flow_table, + struct ovs_list *msgs) +{ + ovs_assert(ovs_list_is_empty(&flow_table->tracked_flows)); + /* Iterate through all of the installed flows. If any of them are no + * longer desired, delete them; if any of them should have different + * actions, update them. */ + struct installed_flow *i, *next; + HMAP_FOR_EACH_SAFE (i, next, match_hmap_node, &installed_flows) { + unlink_all_refs_for_installed_flow(i); + struct desired_flow *d = + desired_flow_lookup(flow_table, &i->flow, NULL); + if (!d) { + /* Installed flow is no longer desirable. Delete it from the + * switch and from installed_flows. */ + installed_flow_del(&i->flow, msgs); + ovn_flow_log(&i->flow, "removing installed"); + + hmap_remove(&installed_flows, &i->match_hmap_node); + installed_flow_destroy(i); + } else { + if (!ofpacts_equal(i->flow.ofpacts, i->flow.ofpacts_len, + d->flow.ofpacts, d->flow.ofpacts_len) || + i->flow.cookie != d->flow.cookie) { + installed_flow_mod(&i->flow, &d->flow, msgs); + ovn_flow_log(&i->flow, "updating installed"); + } + link_installed_to_desired(i, d); + + } + } + + /* Iterate through the desired flows and add those that aren't found + * in the installed flow table. */ + struct desired_flow *d; + HMAP_FOR_EACH (d, match_hmap_node, &flow_table->match_flow_table) { + i = installed_flow_lookup(&d->flow); + if (!i) { + ovn_flow_log(&d->flow, "adding installed"); + installed_flow_add(&d->flow, msgs); + + /* Copy 'd' from 'flow_table' to installed_flows. */ + i = installed_flow_dup(d); + hmap_insert(&installed_flows, &i->match_hmap_node, i->flow.hash); + } + link_installed_to_desired(i, d); + } +} + +static void +update_installed_flows_by_track(struct ovn_desired_flow_table *flow_table, + struct ovs_list *msgs) +{ + struct desired_flow *f, *f_next; + LIST_FOR_EACH_SAFE (f, f_next, track_list_node, + &flow_table->tracked_flows) { + ovs_list_remove(&f->track_list_node); + if (f->is_deleted) { + /* The desired flow was deleted */ + if (f->installed_flow) { + struct installed_flow *i = f->installed_flow; + unlink_installed_to_desired(i, f); + + if (!i->desired_flow) { + installed_flow_del(&i->flow, msgs); + ovn_flow_log(&i->flow, "removing installed (tracked)"); + + hmap_remove(&installed_flows, &i->match_hmap_node); + installed_flow_destroy(i); + } else { + /* There are other desired flow(s) referencing this + * installed flow, so update the OVS flow for the new + * active flow (at least the cookie will be different, + * even if the actions are the same). */ + struct desired_flow *d = i->desired_flow; + ovn_flow_log(&i->flow, "updating installed (tracked)"); + installed_flow_mod(&i->flow, &d->flow, msgs); + } + } + desired_flow_destroy(f); + } else { + /* The desired flow was added or modified. */ + struct installed_flow *i = installed_flow_lookup(&f->flow); + if (!i) { + /* Adding a new flow. */ + installed_flow_add(&f->flow, msgs); + ovn_flow_log(&f->flow, "adding installed (tracked)"); + + /* Copy 'f' from 'flow_table' to installed_flows. */ + struct installed_flow *new_node = installed_flow_dup(f); + hmap_insert(&installed_flows, &new_node->match_hmap_node, + new_node->flow.hash); + link_installed_to_desired(new_node, f); + } else if (i->desired_flow == f) { + /* The installed flow is installed for f, but f has change + * tracked, so it must have been modified. */ + ovn_flow_log(&i->flow, "updating installed (tracked)"); + installed_flow_mod(&i->flow, &f->flow, msgs); + } else { + /* Adding a new flow that conflicts with an existing installed + * flow, so just add it to the link. */ + link_installed_to_desired(i, f); + } + /* The track_list_node emptyness is used to check if the node is + * already added to track list, so initialize it again here. */ + ovs_list_init(&f->track_list_node); + } + } +} + /* The flow table can be updated if the connection to the switch is up and * in the correct state and not backlogged with existing flow_mods. (Our * criteria for being backlogged appear very conservative, but the socket @@ -1651,48 +1867,10 @@ ofctrl_put(struct ovn_desired_flow_table *flow_table, } } - /* Iterate through all of the installed flows. If any of them are no - * longer desired, delete them; if any of them should have different - * actions, update them. */ - struct installed_flow *i, *next; - HMAP_FOR_EACH_SAFE (i, next, match_hmap_node, &installed_flows) { - unlink_all_refs_for_installed_flow(i); - struct desired_flow *d = desired_flow_lookup(flow_table, &i->flow, - NULL); - if (!d) { - /* Installed flow is no longer desirable. Delete it from the - * switch and from installed_flows. */ - installed_flow_del(&i->flow, &msgs); - ovn_flow_log(&i->flow, "removing installed"); - hmap_remove(&installed_flows, &i->match_hmap_node); - installed_flow_destroy(i); - } else { - if (!ofpacts_equal(i->flow.ofpacts, i->flow.ofpacts_len, - d->flow.ofpacts, d->flow.ofpacts_len) || - i->flow.cookie != d->flow.cookie) { - ovn_flow_log(&i->flow, "updating installed"); - installed_flow_mod(&i->flow, &d->flow, &msgs); - } - link_installed_to_desired(i, d); - - } - } - - /* Iterate through the desired flows and add those that aren't found - * in the installed flow table. */ - struct desired_flow *d; - HMAP_FOR_EACH (d, match_hmap_node, &flow_table->match_flow_table) { - i = installed_flow_lookup(&d->flow); - if (!i) { - installed_flow_add(&d->flow, &msgs); - ovn_flow_log(&d->flow, "adding installed"); - - /* Copy 'd' from 'flow_table' to installed_flows. */ - i = installed_flow_dup(d); - hmap_insert(&installed_flows, &i->match_hmap_node, - i->flow.hash); - } - link_installed_to_desired(i, d); + if (flow_table->change_tracked) { + update_installed_flows_by_track(flow_table, &msgs); + } else { + update_installed_flows_by_compare(flow_table, &msgs); } /* Iterate through the installed groups from previous runs. If they @@ -1806,6 +1984,9 @@ ofctrl_put(struct ovn_desired_flow_table *flow_table, /* We were completely up-to-date before and still are. */ cur_cfg = nb_cfg; } + + flow_table->change_tracked = true; + ovs_assert(ovs_list_is_empty(&flow_table->tracked_flows)); } /* Looks up the logical port with the name 'port_name' in 'br_int_'. If diff --git a/controller/ofctrl.h b/controller/ofctrl.h index 8789ce4..64b0ea5 100644 --- a/controller/ofctrl.h +++ b/controller/ofctrl.h @@ -38,6 +38,11 @@ struct ovn_desired_flow_table { /* SB uuid index for the cross reference nodes that link to the nodes in * match_flow_table.*/ struct hmap uuid_flow_table; + + /* Is flow changes tracked. */ + bool change_tracked; + /* Tracked flow changes. */ + struct ovs_list tracked_flows; }; /* Interface for OVN main loop. */ @@ -99,7 +104,6 @@ void ofctrl_flood_remove_flows(struct ovn_desired_flow_table *, struct hmap *flood_remove_nodes); void ofctrl_flood_remove_add_node(struct hmap *flood_remove_nodes, const struct uuid *sb_uuid); - void ovn_desired_flow_table_init(struct ovn_desired_flow_table *); void ovn_desired_flow_table_clear(struct ovn_desired_flow_table *); void ovn_desired_flow_table_destroy(struct ovn_desired_flow_table *); From patchwork Mon Sep 7 06:45:42 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Han Zhou X-Patchwork-Id: 1358617 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 4BlJk02r9xz9sR4 for ; Mon, 7 Sep 2020 16:47:20 +1000 (AEST) Received: from localhost (localhost [127.0.0.1]) by whitealder.osuosl.org (Postfix) with ESMTP id A6FB386661; Mon, 7 Sep 2020 06:47:17 +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 XEy9LdLiSTqw; Mon, 7 Sep 2020 06:47:14 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by whitealder.osuosl.org (Postfix) with ESMTP id 9A7BC86672; Mon, 7 Sep 2020 06:47:14 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id 779B4C0891; Mon, 7 Sep 2020 06:47:14 +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 96A00C0051 for ; Mon, 7 Sep 2020 06:47:13 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by whitealder.osuosl.org (Postfix) with ESMTP id 8F82786156 for ; Mon, 7 Sep 2020 06:47:13 +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 ix-zfs5ORf7Y for ; Mon, 7 Sep 2020 06:47:10 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from relay11.mail.gandi.net (relay11.mail.gandi.net [217.70.178.231]) by whitealder.osuosl.org (Postfix) with ESMTPS id D205E866D9 for ; Mon, 7 Sep 2020 06:47:04 +0000 (UTC) Received: from localhost.localdomain.localdomain (unknown [73.241.94.255]) (Authenticated sender: hzhou@ovn.org) by relay11.mail.gandi.net (Postfix) with ESMTPSA id 0FA2F100004; Mon, 7 Sep 2020 06:46:56 +0000 (UTC) From: Han Zhou To: dev@openvswitch.org Date: Sun, 6 Sep 2020 23:45:42 -0700 Message-Id: <1599461142-84752-10-git-send-email-hzhou@ovn.org> X-Mailer: git-send-email 2.1.0 In-Reply-To: <1599461142-84752-1-git-send-email-hzhou@ovn.org> References: <1599461142-84752-1-git-send-email-hzhou@ovn.org> Cc: Han Zhou Subject: [ovs-dev] [PATCH ovn v2 9/9] ofctrl.c: Merge opposite changes of tracked flows before installing. 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" This patch optimizes the previous patch that incrementally processes flow installation by merging the "add-after-delete" flow changes as much as possible to avoid unnecessary OpenFlow updates. Signed-off-by: Han Zhou --- controller/ofctrl.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 74 insertions(+) diff --git a/controller/ofctrl.c b/controller/ofctrl.c index a2aa45d..81a00c8 100644 --- a/controller/ofctrl.c +++ b/controller/ofctrl.c @@ -1692,10 +1692,84 @@ update_installed_flows_by_compare(struct ovn_desired_flow_table *flow_table, } } +/* Finds and returns a desired_flow in 'deleted_flows' that is exactly the + * same as 'target', including cookie and actions. + */ +static struct desired_flow * +deleted_flow_lookup(struct hmap *deleted_flows, struct ovn_flow *target) +{ + struct desired_flow *d; + HMAP_FOR_EACH_WITH_HASH (d, match_hmap_node, target->hash, + deleted_flows) { + struct ovn_flow *f = &d->flow; + if (f->table_id == target->table_id + && f->priority == target->priority + && minimatch_equal(&f->match, &target->match) + && f->cookie == target->cookie + && ofpacts_equal(f->ofpacts, f->ofpacts_len, target->ofpacts, + target->ofpacts_len)) { + return d; + } + } + return NULL; +} + +/* This function scans the tracked flow changes in the order and merges "add" + * or "update" after "deleted" operations for exactly same flow (priority, + * table, match, action and cookie), to avoid unnecessary OF messages being + * sent to OVS. */ +static void +merge_tracked_flows(struct ovn_desired_flow_table *flow_table) +{ + struct hmap deleted_flows = HMAP_INITIALIZER(&deleted_flows); + struct desired_flow *f, *next; + LIST_FOR_EACH_SAFE (f, next, track_list_node, + &flow_table->tracked_flows) { + if (f->is_deleted) { + /* reuse f->match_hmap_node field since it is already removed from + * the desired flow table's match index. */ + hmap_insert(&deleted_flows, &f->match_hmap_node, + f->flow.hash); + } else { + struct desired_flow *del_f = deleted_flow_lookup(&deleted_flows, + &f->flow); + if (!del_f) { + continue; + } + + /* del_f must have been installed, otherwise it should have been + * removed during track_flow_add_or_modify. */ + ovs_assert(del_f->installed_flow); + if (!f->installed_flow) { + /* f is not installed yet. */ + struct installed_flow *i = del_f->installed_flow; + unlink_installed_to_desired(i, del_f); + link_installed_to_desired(i, f); + } else { + /* f has been installed before, and now was updated to exact + * the same flow as del_f. */ + ovs_assert(f->installed_flow == del_f->installed_flow); + unlink_installed_to_desired(del_f->installed_flow, del_f); + } + hmap_remove(&deleted_flows, &del_f->match_hmap_node); + ovs_list_remove(&del_f->track_list_node); + desired_flow_destroy(del_f); + + ovs_list_remove(&f->track_list_node); + ovs_list_init(&f->track_list_node); + } + } + HMAP_FOR_EACH_SAFE (f, next, match_hmap_node, &deleted_flows) { + hmap_remove(&deleted_flows, &f->match_hmap_node); + } + hmap_destroy(&deleted_flows); +} + static void update_installed_flows_by_track(struct ovn_desired_flow_table *flow_table, struct ovs_list *msgs) { + merge_tracked_flows(flow_table); struct desired_flow *f, *f_next; LIST_FOR_EACH_SAFE (f, f_next, track_list_node, &flow_table->tracked_flows) {