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 <simon.hor...@netronome.com>
> > ---
> > * 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
])
--
2.1.4
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
