From patchwork Fri Jul 14 06:30:51 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Justin Pettit X-Patchwork-Id: 788136 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org 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 3x82sg1k5Cz9s78 for ; Fri, 14 Jul 2017 16:32:07 +1000 (AEST) Received: from mail.linux-foundation.org (localhost [127.0.0.1]) by mail.linuxfoundation.org (Postfix) with ESMTP id 16DEBCC8; Fri, 14 Jul 2017 06:31:00 +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 AE3AECC5 for ; Fri, 14 Jul 2017 06:30:57 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net [217.70.183.194]) by smtp1.linuxfoundation.org (Postfix) with ESMTPS id F03C117C for ; Fri, 14 Jul 2017 06:30:56 +0000 (UTC) X-Originating-IP: 98.234.50.139 Received: from localhost.localdomain (unknown [98.234.50.139]) (Authenticated sender: jpettit@ovn.org) by relay2-d.mail.gandi.net (Postfix) with ESMTPSA id 41F49C5A56 for ; Fri, 14 Jul 2017 08:30:54 +0200 (CEST) From: Justin Pettit To: dev@openvswitch.org Date: Thu, 13 Jul 2017 23:30:51 -0700 Message-Id: <1500013851-89181-3-git-send-email-jpettit@ovn.org> X-Mailer: git-send-email 2.7.4 In-Reply-To: <1500013851-89181-1-git-send-email-jpettit@ovn.org> References: <1500013851-89181-1-git-send-email-jpettit@ovn.org> X-Spam-Status: No, score=-2.6 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_LOW 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 3/3] ofproto-dpif-rid: Always store tunnel metadata. 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 Tunnel metadata was only stored if the tunnel destination was set. It's possible, for example, that a flow could set the tunnel id field before recirculation and then set the destination field afterwards. The previous behavior is that the tunnel id would be lost during recirculation under such a circumstance. This changes the behavior to always copy the tunnel metadata, regardless of whether the tunnel destination is set. It also adds a unit test. Signed-off-by: Justin Pettit Acked-by: Ben Pfaff --- ofproto/ofproto-dpif-rid.c | 17 +++-------------- ofproto/ofproto-dpif-rid.h | 6 +----- tests/ofproto-dpif.at | 29 +++++++++++++++++++++++++++++ 3 files changed, 33 insertions(+), 19 deletions(-) diff --git a/ofproto/ofproto-dpif-rid.c b/ofproto/ofproto-dpif-rid.c index 10aabe5cab15..f78070a5d05f 100644 --- a/ofproto/ofproto-dpif-rid.c +++ b/ofproto/ofproto-dpif-rid.c @@ -127,16 +127,9 @@ frozen_state_hash(const struct frozen_state *state) hash = uuid_hash(&state->ofproto_uuid); hash = hash_int(state->table_id, hash); - if (flow_tnl_dst_is_set(&state->metadata.tunnel)) { - /* We may leave remainder bytes unhashed, but that is unlikely as - * the tunnel is not in the datapath format. */ - hash = hash_bytes64((const uint64_t *) &state->metadata.tunnel, - flow_tnl_size(&state->metadata.tunnel), hash); - } + hash = hash_bytes64((const uint64_t *) &state->metadata, + sizeof state->metadata, hash); hash = hash_boolean(state->conntracked, hash); - hash = hash_bytes64((const uint64_t *) &state->metadata.metadata, - sizeof state->metadata - sizeof state->metadata.tunnel, - hash); if (state->stack && state->stack_size) { hash = hash_bytes(state->stack, state->stack_size, hash); } @@ -159,9 +152,7 @@ frozen_state_equal(const struct frozen_state *a, const struct frozen_state *b) { return (a->table_id == b->table_id && uuid_equals(&a->ofproto_uuid, &b->ofproto_uuid) - && flow_tnl_equal(&a->metadata.tunnel, &b->metadata.tunnel) - && !memcmp(&a->metadata.metadata, &b->metadata.metadata, - sizeof a->metadata - sizeof a->metadata.tunnel) + && !memcmp(&a->metadata, &b->metadata, sizeof a->metadata) && a->stack_size == b->stack_size && !memcmp(a->stack, b->stack, a->stack_size) && a->mirrors == b->mirrors @@ -205,8 +196,6 @@ static void frozen_state_clone(struct frozen_state *new, const struct frozen_state *old) { *new = *old; - flow_tnl_copy__(&new->metadata.tunnel, &old->metadata.tunnel); - new->stack = (new->stack_size ? xmemdup(new->stack, new->stack_size) : NULL); diff --git a/ofproto/ofproto-dpif-rid.h b/ofproto/ofproto-dpif-rid.h index e0ea7064c406..45b23367e2a8 100644 --- a/ofproto/ofproto-dpif-rid.h +++ b/ofproto/ofproto-dpif-rid.h @@ -124,11 +124,7 @@ static inline void frozen_metadata_to_flow(const struct frozen_metadata *md, struct flow *flow) { - if (flow_tnl_dst_is_set(&md->tunnel)) { - flow->tunnel = md->tunnel; - } else { - memset(&flow->tunnel, 0, sizeof flow->tunnel); - } + flow->tunnel = md->tunnel; flow->metadata = md->metadata; memcpy(flow->regs, md->regs, sizeof flow->regs); flow->in_port.ofp_port = md->in_port; diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at index 8373f90639c2..d25d2ddc2e95 100644 --- a/tests/ofproto-dpif.at +++ b/tests/ofproto-dpif.at @@ -4763,6 +4763,35 @@ AT_CHECK_UNQUOTED([tail -1 stdout], [0], [Datapath actions: 3 OVS_VSWITCHD_STOP AT_CLEANUP + +# This test verifies that tunnel metadata is preserved across +# recirculation. At the time of recirculation, fields such as "tun_id" +# may be set before the tunnel is "valid" (ie, has a destination +# address), but the field should still be available after recirculation. +AT_SETUP([ofproto-dpif - resubmit with tun_id]) +OVS_VSWITCHD_START +add_of_ports br0 1 2 3 + +AT_DATA([flows.txt], [dnl +table=0 in_port=1 actions=2,load:6->NXM_NX_TUN_ID[[]],resubmit(,1) +table=1 in_port=1 actions=debug_recirc,resubmit:55 +table=1 in_port=55 actions=3 +]) +AT_CHECK([ovs-ofctl add-flows br0 flows.txt]) + +flow="in_port(1),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=1,tos=0,ttl=128,frag=no),icmp(type=8,code=0)" +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "$flow" -generate], [0], [stdout]) +AT_CHECK([grep "Final flow:" stdout], [0], [Final flow: icmp,tun_id=0x6,in_port=1,vlan_tci=0x0000,dl_src=50:54:00:00:00:05,dl_dst=50:54:00:00:00:07,nw_src=192.168.0.1,nw_dst=192.168.0.2,nw_tos=0,nw_ecn=0,nw_ttl=128,icmp_type=8,icmp_code=0 +]) + +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "$flow,recirc_id(1)" -generate], [0], [stdout]) +AT_CHECK([grep "Final flow:" stdout], [0], [Final flow: recirc_id=0x1,icmp,tun_id=0x6,in_port=1,vlan_tci=0x0000,dl_src=50:54:00:00:00:05,dl_dst=50:54:00:00:00:07,nw_src=192.168.0.1,nw_dst=192.168.0.2,nw_tos=0,nw_ecn=0,nw_ttl=128,icmp_type=8,icmp_code=0 +]) + +OVS_VSWITCHD_STOP +AT_CLEANUP + + # This test verifies that "resubmit", when it triggers recirculation # indirectly through the flow that it recursively invokes, is not # re-executed when execution continues later post-recirculation.