From patchwork Mon Oct 28 21:24:17 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mark Michelson X-Patchwork-Id: 1185708 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.169.12; helo=mail.linuxfoundation.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Authentication-Results: ozlabs.org; dmarc=fail (p=none dis=none) header.from=redhat.com Authentication-Results: ozlabs.org; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.b="PgT+cpBl"; dkim-atps=neutral Received: from mail.linuxfoundation.org (mail.linuxfoundation.org [140.211.169.12]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 47275Q5Zthz9sPL for ; Tue, 29 Oct 2019 08:24:26 +1100 (AEDT) Received: from mail.linux-foundation.org (localhost [127.0.0.1]) by mail.linuxfoundation.org (Postfix) with ESMTP id 599C0CC3; Mon, 28 Oct 2019 21:24:25 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@mail.linuxfoundation.org Received: from smtp1.linuxfoundation.org (smtp1.linux-foundation.org [172.17.192.35]) by mail.linuxfoundation.org (Postfix) with ESMTPS id 22E37CB6 for ; Mon, 28 Oct 2019 21:24:24 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from us-smtp-1.mimecast.com (us-smtp-delivery-1.mimecast.com [207.211.31.120]) by smtp1.linuxfoundation.org (Postfix) with ESMTP id D309C8A9 for ; Mon, 28 Oct 2019 21:24:22 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1572297861; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=yWkeuUlfDcb65ByRO4uBE7Cm9riHws2dxUZa5G5X40A=; b=PgT+cpBl4Acq8fy5sw6ZyuXDPLfQ4Vtx8FW5vEvfmgmdd7bssCqso9g58XyIZFBqP/GdPG czryn31qjCRiWz+KwEqiMH6gglnbO6o5V6lL48iUxJkJtv82/uHyGpaDPwwlI7uxBl632E hkr94LAuQ0Q0ZVQthJWR3znQ/EKHYMs= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-7-OBI1mZvCN7iemd7yAe9C4A-1; Mon, 28 Oct 2019 17:24:19 -0400 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 29D1785B6EE for ; Mon, 28 Oct 2019 21:24:19 +0000 (UTC) Received: from monae.redhat.com (ovpn-122-41.rdu2.redhat.com [10.10.122.41]) by smtp.corp.redhat.com (Postfix) with ESMTP id 939DE6085E for ; Mon, 28 Oct 2019 21:24:18 +0000 (UTC) From: Mark Michelson To: dev@openvswitch.org Date: Mon, 28 Oct 2019 17:24:17 -0400 Message-Id: <20191028212417.2426-1-mmichels@redhat.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-MC-Unique: OBI1mZvCN7iemd7yAe9C4A-1 X-Mimecast-Spam-Score: 0 X-Spam-Status: No, score=-4.3 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, RCVD_IN_DNSWL_MED autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on smtp1.linux-foundation.org Subject: [ovs-dev] [PATCH v2.12] OVN: Combine conjunctions with identical matches into one flow. X-BeenThere: ovs-dev@openvswitch.org X-Mailman-Version: 2.1.12 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Sender: ovs-dev-bounces@openvswitch.org Errors-To: ovs-dev-bounces@openvswitch.org This is a backport of commit e659bab31a916d540411c93ca7125011b2e82b5c from OVN master. Conjunctive matches have an issue where it is possible to install multiple flows that have identical matches. This results in ambiguity, and can lead to features (such as ACLs) not functioning properly. This change fixes the problem by combining conjunctions with identical matches into a single flow. As an example, in the past we may have had something like: nw_dst=10.0.0.1 actions=conjunction(2,1/2) nw_dst=10.0.0.1 actions=conjunction(3,1/2) This commit changes this into nw_dst=10.0.0.1 actions=conjunction(2,1/2),conjunction(3,1/2) This way, there is only a single flow with the proscribed match, and there is no ambiguity. Signed-off-by: Mark Michelson Acked-by: Numan Siddique --- ovn/controller/lflow.c | 5 ++-- ovn/controller/ofctrl.c | 74 ++++++++++++++++++++++++++++++++++++++++++------- ovn/controller/ofctrl.h | 6 ++++ tests/ovn.at | 17 +++++------- 4 files changed, 80 insertions(+), 22 deletions(-) diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c index 1aafafb33..09c2c78f1 100644 --- a/ovn/controller/lflow.c +++ b/ovn/controller/lflow.c @@ -735,8 +735,9 @@ consider_logical_flow( dst->clause = src->clause; dst->n_clauses = src->n_clauses; } - ofctrl_add_flow(flow_table, ptable, lflow->priority, 0, &m->match, - &conj, &lflow->header_.uuid); + + ofctrl_add_or_append_flow(flow_table, ptable, lflow->priority, 0, + &m->match, &conj, &lflow->header_.uuid); ofpbuf_uninit(&conj); } } diff --git a/ovn/controller/ofctrl.c b/ovn/controller/ofctrl.c index 0fcaa72c3..f7211c5e0 100644 --- a/ovn/controller/ofctrl.c +++ b/ovn/controller/ofctrl.c @@ -69,6 +69,11 @@ struct ovn_flow { uint64_t cookie; }; +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); 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, @@ -657,16 +662,8 @@ 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 = xmalloc(sizeof *f); - 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; + struct ovn_flow *f = ovn_flow_alloc(table_id, priority, cookie, match, + actions, sb_uuid); ovn_flow_log(f, "ofctrl_add_flow"); @@ -721,9 +718,66 @@ ofctrl_add_flow(struct ovn_desired_flow_table *desired_flows, ofctrl_check_and_add_flow(desired_flows, table_id, priority, cookie, match, actions, sb_uuid, true); } + +void +ofctrl_add_or_append_flow(struct ovn_desired_flow_table *desired_flows, + uint8_t table_id, uint16_t priority, uint64_t cookie, + const struct match *match, + const struct ofpbuf *actions, + 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"); + + struct ovn_flow *existing; + existing = ovn_flow_lookup(&desired_flows->match_flow_table, f, false); + if (existing) { + /* There's already a flow with this particular match. Append the + * action to that flow rather than adding a new flow + */ + 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); + + free(existing->ofpacts); + existing->ofpacts = xmemdup(compound.data, compound.size); + existing->ofpacts_len = compound.size; + + ofpbuf_uninit(&compound); + ovn_flow_destroy(f); + } 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); + } +} /* ovn_flow. */ +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) +{ + struct ovn_flow *f = xmalloc(sizeof *f); + 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; +} + /* Returns a hash of the match key in 'f'. */ static uint32_t ovn_flow_match_hash(const struct ovn_flow *f) diff --git a/ovn/controller/ofctrl.h b/ovn/controller/ofctrl.h index 2b21c1133..54b80d48f 100644 --- a/ovn/controller/ofctrl.h +++ b/ovn/controller/ofctrl.h @@ -70,6 +70,12 @@ void ofctrl_add_flow(struct ovn_desired_flow_table *, uint8_t table_id, const struct match *, const struct ofpbuf *ofpacts, const struct uuid *); +void ofctrl_add_or_append_flow(struct ovn_desired_flow_table *desired_flows, + uint8_t table_id, uint16_t priority, + uint64_t cookie, const struct match *match, + const struct ofpbuf *actions, + const struct uuid *sb_uuid); + void ofctrl_remove_flows(struct ovn_desired_flow_table *, const struct uuid *); void ovn_desired_flow_table_init(struct ovn_desired_flow_table *); diff --git a/tests/ovn.at b/tests/ovn.at index 54aa19bb2..e339b8158 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -12197,7 +12197,7 @@ ovn-nbctl create Address_Set name=set1 \ addresses=\"10.0.0.4\",\"10.0.0.5\",\"10.0.0.6\" ovn-nbctl create Address_Set name=set2 \ addresses=\"10.0.0.7\",\"10.0.0.8\",\"10.0.0.9\" -ovn-nbctl acl-add ls1 to-lport 1002 \ +ovn-nbctl acl-add ls1 to-lport 1001 \ 'ip4 && ip4.src == $set1 && ip4.dst == $set1' allow ovn-nbctl acl-add ls1 to-lport 1001 \ 'ip4 && ip4.src == $set1 && ip4.dst == $set2' drop @@ -12246,7 +12246,7 @@ cat 2.expected > expout $PYTHON "$top_srcdir/utilities/ovs-pcap.in" hv1/vif2-tx.pcap > 2.packets AT_CHECK([cat 2.packets], [0], [expout]) -# There should be total of 12 flows present with conjunction action and 2 flows +# 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=3,metadata=0x1 actions=drop @@ -12256,14 +12256,11 @@ AT_CHECK([cat 2.packets], [0], [expout]) # 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) -# priority=2002,ip,metadata=0x1,nw_src=10.0.0.4 actions=conjunction(2,1/2) -# priority=2002,ip,metadata=0x1,nw_src=10.0.0.5 actions=conjunction(2,1/2) -# priority=2001,ip,metadata=0x1,nw_src=10.0.0.6 actions=conjunction(3,1/2) -# priority=2001,ip,metadata=0x1,nw_src=10.0.0.4 actions=conjunction(3,1/2) -# priority=2001,ip,metadata=0x1,nw_src=10.0.0.5 actions=conjunction(3,1/2) - -OVS_WAIT_UNTIL([test 12 = `as hv1 ovs-ofctl dump-flows br-int | \ +# 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) + +OVS_WAIT_UNTIL([test 9 = `as hv1 ovs-ofctl dump-flows br-int | \ grep conjunction | wc -l`]) OVS_WAIT_UNTIL([test 2 = `as hv1 ovs-ofctl dump-flows br-int | \ grep conj_id | wc -l`])