From patchwork Tue May 24 03:43:59 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Simon Horman X-Patchwork-Id: 625467 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from archives.nicira.com (archives.nicira.com [96.126.127.54]) by ozlabs.org (Postfix) with ESMTP id 3rDLr04x62z9t0r for ; Tue, 24 May 2016 13:44:14 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=netronome-com.20150623.gappssmtp.com header.i=@netronome-com.20150623.gappssmtp.com header.b=smVqrfsZ; dkim-atps=neutral Received: from archives.nicira.com (localhost [127.0.0.1]) by archives.nicira.com (Postfix) with ESMTP id B1BF6108EC; Mon, 23 May 2016 20:44:13 -0700 (PDT) X-Original-To: dev@openvswitch.org Delivered-To: dev@openvswitch.org Received: from mx1e4.cudamail.com (mx1.cudamail.com [69.90.118.67]) by archives.nicira.com (Postfix) with ESMTPS id D5205108B6 for ; Mon, 23 May 2016 20:44:11 -0700 (PDT) Received: from bar5.cudamail.com (unknown [192.168.21.12]) by mx1e4.cudamail.com (Postfix) with ESMTPS id 1F47A1E02CD for ; Mon, 23 May 2016 21:44:11 -0600 (MDT) X-ASG-Debug-ID: 1464061448-09eadd02ef59c770001-byXFYA Received: from mx3-pf2.cudamail.com ([192.168.14.1]) by bar5.cudamail.com with ESMTP id r2MY59WYBfG8jofW (version=TLSv1 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO) for ; Mon, 23 May 2016 21:44:08 -0600 (MDT) X-Barracuda-Envelope-From: simon.horman@netronome.com X-Barracuda-RBL-Trusted-Forwarder: 192.168.14.1 Received: from unknown (HELO mail-pf0-f182.google.com) (209.85.192.182) by mx3-pf2.cudamail.com with ESMTPS (AES128-SHA encrypted); 24 May 2016 03:44:08 -0000 Received-SPF: neutral (mx3-pf2.cudamail.com: 209.85.192.182 is neither permitted nor denied by SPF record at spf.mandrillapp.com) X-Barracuda-RBL-Trusted-Forwarder: 209.85.192.182 Received: by mail-pf0-f182.google.com with SMTP id g64so2238248pfb.2 for ; Mon, 23 May 2016 20:44:07 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=netronome-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=GzsHm8n/1UWSNA1whmpTttBtbCdySrF8DzEtPBlA5hs=; b=smVqrfsZxxlqc84V6Ss15Tp248azPV8RypH8GwunORo9QZIWHmck+VTYM/JC+6SDYa RpaRYEp2mI+eShsUJ0etl7J5tziViS5DZnQn6DI6vA6TbdsVWlO0hLQP0s3hkrD0RNSZ 150yWZefQYdjB2cvBfvTQfkQSZBYvRs49FGgt7JSYv/Ilf6rhxJCnYxBFN+ZEzi0LN+2 iMSedwA0wl2DByEnptywZhM9vgVL2mliPkaq+WwQiW9JVtR+t84ka+RXyQpfX1nlNxcT tiy3Gfg90/t2jLaZfk47MOvQox2C0NJC0mn7Gy103FoyFdcuy7SY8578O4iGYBtHWNZb OiaQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=GzsHm8n/1UWSNA1whmpTttBtbCdySrF8DzEtPBlA5hs=; b=ZRTalrSonCOAw7qv0owocjy1hlllNCj7px/m/5UqpFYUq+ctHnTdFVUNaV+dHu5nSB BMJFjT14DhDQBnYj5Av3ISqkMcMZ34ZrHfubwOto1TBxc+bU/sqbDJssrG1MBshY54MB aG3oUAA0tYRcY4gpcaEOggTaUksmMrvj5VeVxrwxzDzvY15wmOCntDTVREXYV/eYulY9 avsWUwEl5fmyQUpZHQ8vFvF2Xco4PJ/QTEPdFQ+JdaOktz5sJkDsbzCuWqakAT7UPPEb cBsVzUg2Epu2sWATe+ufHynnvAgfZ208jgLauECX7yoBYpI04lw9PdZDHK3nT7ypVDpG 94Jw== X-Gm-Message-State: ALyK8tJZAaVXql+hX0XmnMzOoGV9Kf6GRQlJXo7Uyx6baWoUT8oED0ysdB04wSVyzULP+UgK X-Received: by 10.98.89.71 with SMTP id n68mr3434068pfb.82.1464061447178; Mon, 23 May 2016 20:44:07 -0700 (PDT) Received: from vergenet.net (reginn.isobedori.kobe.vergenet.net. [2001:470:4832:303:d63d:7eff:fe99:ac9d]) by smtp.gmail.com with ESMTPSA id b67sm10689570pfc.74.2016.05.23.20.44.03 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 23 May 2016 20:44:05 -0700 (PDT) Date: Tue, 24 May 2016 12:43:59 +0900 X-Barracuda-Apparent-Source-IP: 2001:470:4832:303:d63d:7eff:fe99:ac9d X-CudaMail-Envelope-Sender: simon.horman@netronome.com From: Simon Horman To: Ben Pfaff X-CudaMail-MID: CM-V2-522050617 X-CudaMail-DTE: 052316 X-CudaMail-Originating-IP: 209.85.192.182 Message-ID: <20160524034356.GA31389@vergenet.net> X-ASG-Orig-Subj: [##CM-V2-522050617##]Re: [ovs-dev] [PATCH] xlate: Skip recirculation for output and set actions References: <1459757795-21310-1-git-send-email-simon.horman@netronome.com> <20160412193951.GI17989@ovn.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20160412193951.GI17989@ovn.org> User-Agent: Mutt/1.5.23 (2014-03-12) X-GBUdb-Analysis: 0, 209.85.192.182, Ugly c=0.377531 p=-0.282051 Source Normal X-MessageSniffer-Rules: 0-0-0-21202-c X-Barracuda-Connect: UNKNOWN[192.168.14.1] X-Barracuda-Start-Time: 1464061448 X-Barracuda-Encrypted: DHE-RSA-AES256-SHA X-Barracuda-URL: https://web.cudamail.com:443/cgi-mod/mark.cgi X-Virus-Scanned: by bsmtpd at cudamail.com X-Barracuda-BRTS-Status: 1 X-Barracuda-Spam-Score: 0.10 X-Barracuda-Spam-Status: No, SCORE=0.10 using global scores of TAG_LEVEL=3.5 QUARANTINE_LEVEL=1000.0 KILL_LEVEL=4.0 tests=BSF_SC0_MISMATCH_TO, DKIM_SIGNED, RDNS_NONE X-Barracuda-Spam-Report: Code version 3.2, rules version 3.2.3.29852 Rule breakdown below pts rule name description ---- ---------------------- -------------------------------------------------- 0.00 BSF_SC0_MISMATCH_TO Envelope rcpt doesn't match header 0.00 DKIM_SIGNED Domain Keys Identified Mail: message has a signature 0.10 RDNS_NONE Delivered to trusted network by a host with no rDNS Cc: dev@openvswitch.org Subject: Re: [ovs-dev] [PATCH] xlate: Skip recirculation for output and set actions X-BeenThere: dev@openvswitch.org X-Mailman-Version: 2.1.16 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@openvswitch.org Sender: "dev" Hi Ben, On Tue, Apr 12, 2016 at 12:39:51PM -0700, Ben Pfaff wrote: > On Mon, Apr 04, 2016 at 05:16:35PM +0900, Simon Horman wrote: > > Until 8bf009bf8ab4 ("xlate: Always recirculate after an MPLS POP to a > > non-MPLS ethertype.") the translation code took some care to only > > recirculate as a result of a pop_mpls action if necessary. This was > > implemented using per-action checks and resulted in some maintenance > > burden. > > > > Unfortunately recirculation is a relatively expensive operation and a > > performance degradation of up to 35% has been observed with the above > > mentioned patch applied for the arguably common case of: > > > > pop_mpls,set(l2 field),output > > > > This patch attempts to strike a balance between performance and > > maintainability by special casing set and output actions such > > that recirculation may be avoided. > > > > This partially reverts the above mentioned commit. In particular most > > of the C code changes outside of do_xlate_actions(). > > > > Signed-off-by: Simon Horman > > --- > > * Lightly tested using test-suite portion of this patch > > I think that recirculation is necessary for output to patch ports. I believe this is already handled in my patch as recirculation is triggered in xlate_table_action(). As part of the incremental patch below I have included a test to exercise this. > I think that recirculation is necessary for output to a group that > chooses a bucket based on L3+ fields, even if the actions in the group > do not otherwise require recirculation. Thanks, I missed that one. I think that the best thing to do at this time is to trigger recirculation for select groups - other groups don't access fields as you describe and should already be handled correctly. The incremental patch below updates the code to do this as well as providing a test to exercise this case. diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index 1d60f32384d2..cc0ad54f42c2 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -3504,6 +3504,13 @@ xlate_select_group(struct xlate_ctx *ctx, struct group_dpif *group) { const char *selection_method = group_dpif_get_selection_method(group); + /* Select groups may access flow keys beyond L2 in order to + * select a bucket. Recirculate as appropriate to make this possible. + */ + if (ctx->was_mpls) { + ctx_trigger_freeze(ctx); + } + if (selection_method[0] == '\0') { xlate_default_select_group(ctx, group); } else if (!strcasecmp("hash", selection_method)) { diff --git a/tests/mpls-xlate.at b/tests/mpls-xlate.at index a32dc34e7a38..a9a3cf53bed3 100644 --- a/tests/mpls-xlate.at +++ b/tests/mpls-xlate.at @@ -2,23 +2,43 @@ AT_BANNER([mpls-xlate]) AT_SETUP([MPLS xlate action]) -OVS_VSWITCHD_START([add-port br0 p0 -- set Interface p0 type=dummy ofport_request=1]) +OVS_VSWITCHD_START( + [add-port br0 p0 -- set Interface p0 type=dummy ofport_request=1 -- \ + add-port br0 p1 -- set Interface p1 type=patch \ + options:peer=p2 ofport_request=2 -- \ + add-br br1 -- \ + set bridge br1 other-config:hwaddr=aa:66:aa:66:00:00 -- \ + set bridge br1 datapath-type=dummy other-config:datapath-id=1234 \ + fail-mode=secure -- \ + add-port br1 p2 -- set Interface p2 type=patch \ + options:peer=p1]) AT_CHECK([ovs-appctl dpif/show], [0], [dnl dummy@ovs-dummy: hit:0 missed:0 br0: br0 65534/100: (dummy) p0 1/1: (dummy) + p1 2/none: (patch: peer=p2) + br1: + br1 65534/101: (dummy) + p2 1/none: (patch: peer=p1) ]) dnl Setup single MPLS tags. AT_CHECK([ovs-ofctl -O OpenFlow13 add-group br0 group_id=1232,type=select,bucket=output:LOCAL]) -AT_CHECK([ovs-ofctl -O OpenFlow13 add-group br0 group_id=1233,type=select,bucket=dec_ttl,output:LOCAL]) +AT_CHECK([ovs-ofctl -O OpenFlow13 add-group br0 group_id=1233,type=all,bucket=output:LOCAL]) +AT_CHECK([ovs-ofctl -O OpenFlow13 add-group br0 group_id=1234,type=all,bucket=dec_ttl,output:LOCAL]) AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br0 in_port=local,dl_type=0x0800,action=push_mpls:0x8847,set_field:10-\>mpls_label,output:1]) AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br0 dl_type=0x8847,in_port=1,mpls_label=20,action=pop_mpls:0x0800,output:LOCAL]) AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br0 dl_type=0x8847,in_port=1,mpls_label=21,action=pop_mpls:0x0800,dec_ttl,output:LOCAL]) AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br0 dl_type=0x8847,in_port=1,mpls_label=22,action=pop_mpls:0x0800,group:1232]) AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br0 dl_type=0x8847,in_port=1,mpls_label=23,action=pop_mpls:0x0800,group:1233]) +AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br0 dl_type=0x8847,in_port=1,mpls_label=24,action=pop_mpls:0x0800,group:1234]) +AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br0 dl_type=0x8847,in_port=1,mpls_label=25,action=pop_mpls:0x0800,output:2]) + +AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br1 in_port=1,action=output:LOCAL]) + + dnl Test MPLS push AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(100),eth(src=f8:bc:12:44:34:b6,dst=f8:bc:12:46:58:e0),eth_type(0x0800),ipv4(src=1.1.2.92,dst=1.1.2.88,proto=17,tos=0,ttl=64,frag=no),udp(src=7777,dst=80)'], [0], [stdout]) @@ -43,23 +63,45 @@ AT_CHECK([tail -1 stdout], [0], [Datapath actions: set(ipv4(ttl=63)),100 ]) -dnl Test MPLS pop then group output (bucket actions do not trigger recirculation) +dnl Test MPLS pop then select group output (group type triggers recirculation) AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(1),eth(src=f8:bc:12:44:34:b6,dst=f8:bc:12:46:58:e0),eth_type(0x8847),mpls(label=22,tc=0,ttl=64,bos=1)'], [0], [stdout]) AT_CHECK([tail -1 stdout], [0], - [Datapath actions: pop_mpls(eth_type=0x800),100 + [Datapath actions: pop_mpls(eth_type=0x800),recirc(0x2) +]) + +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'recirc_id(2),in_port(1),eth(src=f8:bc:12:44:34:b6,dst=f8:bc:12:46:58:e0),eth_type(0x0800),ipv4(src=1.1.2.92,dst=1.1.2.88,proto=47,tos=0,ttl=64,frag=no)'], [0], [stdout]) +AT_CHECK([tail -1 stdout], [0], + [Datapath actions: 100 ]) -dnl Test MPLS pop then group output (bucket actions trigger recirculation) +dnl Test MPLS pop then all group output (bucket actions do not trigger recirculation) AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(1),eth(src=f8:bc:12:44:34:b6,dst=f8:bc:12:46:58:e0),eth_type(0x8847),mpls(label=23,tc=0,ttl=64,bos=1)'], [0], [stdout]) AT_CHECK([tail -1 stdout], [0], - [Datapath actions: pop_mpls(eth_type=0x800),recirc(0x2) + [Datapath actions: pop_mpls(eth_type=0x800),100 ]) -AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'recirc_id(2),in_port(1),eth(src=f8:bc:12:44:34:b6,dst=f8:bc:12:46:58:e0),eth_type(0x0800),ipv4(src=1.1.2.92,dst=1.1.2.88,proto=47,tos=0,ttl=64,frag=no)'], [0], [stdout]) +dnl Test MPLS pop then all group output (bucket actions trigger recirculation) +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(1),eth(src=f8:bc:12:44:34:b6,dst=f8:bc:12:46:58:e0),eth_type(0x8847),mpls(label=24,tc=0,ttl=64,bos=1)'], [0], [stdout]) +AT_CHECK([tail -1 stdout], [0], + [Datapath actions: pop_mpls(eth_type=0x800),recirc(0x3) +]) + +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'recirc_id(3),in_port(1),eth(src=f8:bc:12:44:34:b6,dst=f8:bc:12:46:58:e0),eth_type(0x0800),ipv4(src=1.1.2.92,dst=1.1.2.88,proto=47,tos=0,ttl=64,frag=no)'], [0], [stdout]) AT_CHECK([tail -1 stdout], [0], [Datapath actions: set(ipv4(ttl=63)),100 ]) +dnl Test MPLS pop then all output to patch port +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(1),eth(src=f8:bc:12:44:34:b6,dst=f8:bc:12:46:58:e0),eth_type(0x8847),mpls(label=25,tc=0,ttl=64,bos=1)'], [0], [stdout]) +AT_CHECK([tail -1 stdout], [0], + [Datapath actions: pop_mpls(eth_type=0x800),recirc(0x4) +]) + +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'recirc_id(4),in_port(1),eth(src=f8:bc:12:44:34:b6,dst=f8:bc:12:46:58:e0),eth_type(0x0800),ipv4(src=1.1.2.92,dst=1.1.2.88,proto=47,tos=0,ttl=64,frag=no)'], [0], [stdout]) +AT_CHECK([tail -1 stdout], [0], + [Datapath actions: 101 +]) + dnl Setup multiple MPLS tags. AT_CHECK([ovs-ofctl del-flows br0]) @@ -80,10 +122,10 @@ AT_CHECK([tail -1 stdout], [0], dnl Double MPLS pop AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(1),eth(src=f8:bc:12:44:34:b6,dst=f8:bc:12:46:58:e0),eth_type(0x8847),mpls(label=60,tc=0,ttl=64,bos=0,label=50,tc=0,ttl=64,bos=1)'], [0], [stdout]) AT_CHECK([tail -1 stdout], [0], - [Datapath actions: pop_mpls(eth_type=0x8847),pop_mpls(eth_type=0x800),recirc(0x3) + [Datapath actions: pop_mpls(eth_type=0x8847),pop_mpls(eth_type=0x800),recirc(0x5) ]) -AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'recirc_id(3),in_port(1),eth(src=f8:bc:12:44:34:b6,dst=f8:bc:12:46:58:e0),eth_type(0x0800),ipv4(src=1.1.2.92,dst=1.1.2.88,proto=47,tos=0,ttl=64,frag=no)'], [0], [stdout]) +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'recirc_id(5),in_port(1),eth(src=f8:bc:12:44:34:b6,dst=f8:bc:12:46:58:e0),eth_type(0x0800),ipv4(src=1.1.2.92,dst=1.1.2.88,proto=47,tos=0,ttl=64,frag=no)'], [0], [stdout]) AT_CHECK([tail -1 stdout], [0], [Datapath actions: set(ipv4(ttl=10)),100 ])