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
---
ofproto/ofproto-dpif-xlate.c | 110 ++++++++++++++++++++++++++++++++++++++++++-
tests/mpls-xlate.at | 36 ++++++++++++--
tests/ofproto-dpif.at | 3 +-
3 files changed, 142 insertions(+), 7 deletions(-)
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index a02dc24f6acd..6f98bdfb17b9 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -320,6 +320,12 @@ struct xlate_ctx {
struct ofpbuf frozen_actions;
const struct ofpact_controller *pause;
+ /* True if a packet was but is no longer MPLS (due to an MPLS pop action).
+ * This is a trigger for recirculation in cases where translating an action
+ * or looking up a flow requires access to the fields of the packet after
+ * the MPLS label stack that was originally present. */
+ bool was_mpls;
+
/* True if conntrack has been performed on this packet during processing
* on the current bridge. This is used to determine whether conntrack
* state from the datapath should be honored after thawing. */
@@ -2986,6 +2992,7 @@ compose_output_action__(struct xlate_ctx *ctx, ofp_port_t
ofp_port,
const struct xport *peer = xport->peer;
struct flow old_flow = ctx->xin->flow;
bool old_conntrack = ctx->conntracked;
+ bool old_was_mpls = ctx->was_mpls;
cls_version_t old_version = ctx->tables_version;
struct ofpbuf old_stack = ctx->stack;
union mf_subvalue new_stack[1024 / sizeof(union mf_subvalue)];
@@ -3043,6 +3050,10 @@ compose_output_action__(struct xlate_ctx *ctx,
ofp_port_t ofp_port,
/* Restore calling bridge's lookup version. */
ctx->tables_version = old_version;
+ /* The peer bridge popping MPLS should have no effect on the original
+ * bridge. */
+ ctx->was_mpls = old_was_mpls;
+
/* The peer bridge's conntrack execution should have no effect on the
* original bridge. */
ctx->conntracked = old_conntrack;
@@ -3260,6 +3271,11 @@ static void
xlate_table_action(struct xlate_ctx *ctx, ofp_port_t in_port, uint8_t table_id,
bool may_packet_in, bool honor_table_miss)
{
+ /* Check if we need to recirculate before matching in a table. */
+ if (ctx->was_mpls) {
+ ctx_trigger_freeze(ctx);
+ return;
+ }
if (xlate_resubmit_resource_check(ctx)) {
uint8_t old_table_id = ctx->table_id;
struct rule_dpif *rule;
@@ -3321,6 +3337,7 @@ xlate_group_bucket(struct xlate_ctx *ctx, struct
ofputil_bucket *bucket)
struct ofpbuf action_set = ofpbuf_const_initializer(bucket->ofpacts,
bucket->ofpacts_len);
struct flow old_flow = ctx->xin->flow;
+ bool old_was_mpls = ctx->was_mpls;
ofpacts_execute_action_set(&action_list, &action_set);
ctx->recurse++;
@@ -3350,6 +3367,10 @@ xlate_group_bucket(struct xlate_ctx *ctx, struct
ofputil_bucket *bucket)
* group buckets. */
ctx->xin->flow = old_flow;
+ /* The group bucket popping MPLS should have no effect after bucket
+ * execution. */
+ ctx->was_mpls = old_was_mpls;
+
/* The fact that the group bucket exits (for any reason) does not mean that
* the translation after the group action should exit. Specifically, if
* the group bucket freezes translation, the actions after the group action
@@ -3765,7 +3786,7 @@ compose_mpls_pop_action(struct xlate_ctx *ctx, ovs_be16
eth_type)
if (flow_pop_mpls(flow, n, eth_type, ctx->wc)) {
if (!eth_type_mpls(eth_type) && ctx->xbridge->support.odp.recirc) {
- ctx_trigger_freeze(ctx);
+ ctx->was_mpls = true;
}
} else if (n >= FLOW_MAX_MPLS_LABELS) {
if (ctx->xin->packet != NULL) {
@@ -4437,6 +4458,90 @@ compose_conntrack_action(struct xlate_ctx *ctx, struct
ofpact_conntrack *ofc)
}
static void
+recirc_for_mpls(const struct ofpact *a, struct xlate_ctx *ctx)
+{
+ /* Do not consider recirculating unless the packet was previously MPLS. */
+ if (!ctx->was_mpls) {
+ return;
+ }
+
+ /* Special case these actions, only recirculating if necessary.
+ * This avoids the overhead of recirculation in common use-cases.
+ */
+ switch (a->type) {
+
+ /* Output actions do not require recirculation. */
+ case OFPACT_OUTPUT:
+ case OFPACT_ENQUEUE:
+ case OFPACT_OUTPUT_REG:
+ /* Set actions that don't touch L3+ fields do not require recirculation. */
+ case OFPACT_SET_VLAN_VID:
+ case OFPACT_SET_VLAN_PCP:
+ case OFPACT_SET_ETH_SRC:
+ case OFPACT_SET_ETH_DST:
+ case OFPACT_SET_TUNNEL:
+ case OFPACT_SET_QUEUE:
+ /* If actions of a group require recirculation that can be detected
+ * when translating them. */
+ case OFPACT_GROUP:
+ return;
+
+ /* Set field that don't touch L3+ fields don't require recirculation. */
+ case OFPACT_SET_FIELD:
+ if (mf_is_l3_or_higher(ofpact_get_SET_FIELD(a)->field)) {
+ break;
+ }
+ return;
+
+ /* For simplicity, recirculate in all other cases. */
+ case OFPACT_CONTROLLER:
+ case OFPACT_BUNDLE:
+ case OFPACT_STRIP_VLAN:
+ case OFPACT_PUSH_VLAN:
+ case OFPACT_SET_IPV4_SRC:
+ case OFPACT_SET_IPV4_DST:
+ case OFPACT_SET_IP_DSCP:
+ case OFPACT_SET_IP_ECN:
+ case OFPACT_SET_IP_TTL:
+ case OFPACT_SET_L4_SRC_PORT:
+ case OFPACT_SET_L4_DST_PORT:
+ case OFPACT_REG_MOVE:
+ case OFPACT_STACK_PUSH:
+ case OFPACT_STACK_POP:
+ case OFPACT_DEC_TTL:
+ case OFPACT_SET_MPLS_LABEL:
+ case OFPACT_SET_MPLS_TC:
+ case OFPACT_SET_MPLS_TTL:
+ case OFPACT_DEC_MPLS_TTL:
+ case OFPACT_PUSH_MPLS:
+ case OFPACT_POP_MPLS:
+ case OFPACT_POP_QUEUE:
+ case OFPACT_FIN_TIMEOUT:
+ case OFPACT_RESUBMIT:
+ case OFPACT_LEARN:
+ case OFPACT_CONJUNCTION:
+ case OFPACT_MULTIPATH:
+ case OFPACT_NOTE:
+ case OFPACT_EXIT:
+ case OFPACT_SAMPLE:
+ case OFPACT_UNROLL_XLATE:
+ case OFPACT_CT:
+ case OFPACT_NAT:
+ case OFPACT_DEBUG_RECIRC:
+ case OFPACT_METER:
+ case OFPACT_CLEAR_ACTIONS:
+ case OFPACT_WRITE_ACTIONS:
+ case OFPACT_WRITE_METADATA:
+ case OFPACT_GOTO_TABLE:
+ default:
+ break;
+ }
+
+ /* Recirculate */
+ ctx_trigger_freeze(ctx);
+}
+
+static void
do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
struct xlate_ctx *ctx)
{
@@ -4459,6 +4564,8 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t
ofpacts_len,
break;
}
+ recirc_for_mpls(a, ctx);
+
if (ctx->exit) {
/* Check if need to store the remaining actions for later
* execution. */
@@ -5108,6 +5215,7 @@ xlate_actions(struct xlate_in *xin, struct xlate_out
*xout)
.frozen_actions = OFPBUF_STUB_INITIALIZER(frozen_actions_stub),
.pause = NULL,
+ .was_mpls = false,
.conntracked = false,
.ct_nat_action = NULL,
diff --git a/tests/mpls-xlate.at b/tests/mpls-xlate.at
index 7df52f513e7e..a32dc34e7a38 100644
--- a/tests/mpls-xlate.at
+++ b/tests/mpls-xlate.at
@@ -12,8 +12,13 @@ dummy@ovs-dummy: hit:0 missed:0
])
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-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])
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])
@@ -21,15 +26,38 @@ AT_CHECK([tail -1 stdout], [0],
[Datapath actions: push_mpls(label=10,tc=0,ttl=64,bos=1,eth_type=0x8847),1
])
-dnl Test MPLS pop
+dnl Test MPLS pop then output (actions do not trigger reciculation)
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=20,tc=0,ttl=64,bos=1)'],
[0], [stdout])
AT_CHECK([tail -1 stdout], [0],
+ [Datapath actions: pop_mpls(eth_type=0x800),100
+])
+
+dnl Test MPLS pop, dec_ttl, output (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=21,tc=0,ttl=64,bos=1)'],
[0], [stdout])
+AT_CHECK([tail -1 stdout], [0],
[Datapath actions: pop_mpls(eth_type=0x800),recirc(0x1)
])
AT_CHECK([ovs-appctl ofproto/trace ovs-dummy
'recirc_id(1),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
+ [Datapath actions: set(ipv4(ttl=63)),100
+])
+
+dnl Test MPLS pop then 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=22,tc=0,ttl=64,bos=1)'],
[0], [stdout])
+AT_CHECK([tail -1 stdout], [0],
+ [Datapath actions: pop_mpls(eth_type=0x800),100
+])
+
+dnl Test MPLS pop then 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=23,tc=0,ttl=64,bos=1)'],
[0], [stdout])
+AT_CHECK([tail -1 stdout], [0],
+ [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: set(ipv4(ttl=63)),100
])
dnl Setup multiple MPLS tags.
@@ -52,10 +80,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(0x2)
+ [Datapath actions:
pop_mpls(eth_type=0x8847),pop_mpls(eth_type=0x800),recirc(0x3)
])
-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([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=10)),100
])
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index da29ac2f2b1f..75457f6a923d 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -6402,8 +6402,7 @@ AT_CHECK([ovs-appctl netdev-dummy/receive p1
'in_port(1),eth(src=50:54:00:00:00:
sleep 1
AT_CHECK([filter_flow_install < ovs-vswitchd.log | strip_xout_keep_actions],
[0], [dnl
recirc_id=0,mpls,in_port=1,vlan_tci=0x0000,dl_src=50:54:00:00:00:09,mpls_label=11,mpls_tc=3,mpls_ttl=64,mpls_bos=1,
actions:push_mpls(label=11,tc=3,ttl=64,bos=0,eth_type=0x8847),2
-recirc_id=0,mpls,in_port=1,vlan_tci=0x0000,dl_src=50:54:00:00:00:0b,mpls_bos=1,
actions:pop_mpls(eth_type=0x800),recirc(0x1)
-recirc_id=0x1,ip,in_port=1,vlan_tci=0x0000,nw_frag=no, actions:2
+recirc_id=0,mpls,in_port=1,vlan_tci=0x0000,dl_src=50:54:00:00:00:0b,mpls_bos=1,
actions:pop_mpls(eth_type=0x800),2
])
OVS_VSWITCHD_STOP
AT_CLEANUP
--
2.7.0.rc3.207.g0ac5344
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
