[ovs-dev] [clone optmization v2 7/7] xlate: Emit datapath clone only when necessary.
Andy Zhou
azhou at ovn.org
Fri Jul 21 22:46:33 UTC 2017
Currently the open flow 'clone' action is always translated into
datapath clone. While this is valid translation, the datapath
'clone' action is more expensive and has more restrictions than
not using them.
This patch optimizing the open flow 'clone' translation. Whenever
the open flow actions within the 'clone' is reversible, i.e.
any datapath actions that modifies a packet can be reversed
by using another datapath action. Reversible actions can be
translated without emitting datapath clone.
This patch combines xlate_clone() and compose_clone() into
a single compose_clone() API, since the layering boundary is not
obvious.
Signed-off-by: Andy Zhou <azhou at ovn.org>
---
ofproto/ofproto-dpif-xlate.c | 169 ++++++++++++++++++++++++++++++++-----------
tests/ofproto-dpif.at | 39 +++++++++-
2 files changed, 164 insertions(+), 44 deletions(-)
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 9b7a6a4f7620..66df4693dd3e 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -5245,49 +5245,83 @@ xlate_sample_action(struct xlate_ctx *ctx,
tunnel_out_port, false);
}
-/* For datapath that does not support 'clone' action, but have a suitable
- * sample action implementation for clone. The upstream Linux kernel
- * version 4.11 or greater, and kernel module from OVS version 2.8 or
- * greater version have suitable sample action implementations. */
-static void
-compose_clone(struct xlate_ctx *ctx, struct flow *old_base_flow,
- const struct ofpact_nest *oc)
+/* Determine if an datapath action translated from the openflow action
+ * can be reversed by another datapath action.
+ *
+ * Openflow actions that do not emit datapath actions are trivially
+ * reversible. Reversiblity of other actions depends on nature of
+ * action and their translation. */
+static bool
+reversible_actions(const struct ofpact *ofpacts, size_t ofpacts_len)
{
- size_t offset, ac_offset;
+ const struct ofpact *a;
- if (ctx->xbridge->support.clone) {
- /* Use clone action as datapath clone. */
- offset = nl_msg_start_nested(ctx->odp_actions, OVS_ACTION_ATTR_CLONE);
- do_xlate_actions(oc->actions, ofpact_nest_get_action_len(oc), ctx);
- nl_msg_end_non_empty_nested(ctx->odp_actions, offset);
- ctx->base_flow = *old_base_flow;
- } else if (ctx->xbridge->support.sample_nesting > 3) {
- /* Use sample action as datapath clone. */
- offset = nl_msg_start_nested(ctx->odp_actions,
- OVS_ACTION_ATTR_SAMPLE);
- ac_offset = nl_msg_start_nested(ctx->odp_actions,
- OVS_SAMPLE_ATTR_ACTIONS);
- do_xlate_actions(oc->actions, ofpact_nest_get_action_len(oc), ctx);
- if (nl_msg_end_non_empty_nested(ctx->odp_actions, ac_offset)) {
- nl_msg_cancel_nested(ctx->odp_actions, offset);
- } else {
- nl_msg_put_u32(ctx->odp_actions, OVS_SAMPLE_ATTR_PROBABILITY,
- UINT32_MAX); /* 100% probability. */
- nl_msg_end_nested(ctx->odp_actions, offset);
+ OFPACT_FOR_EACH (a, ofpacts, ofpacts_len) {
+ switch (a->type) {
+ case OFPACT_BUNDLE:
+ case OFPACT_CLEAR_ACTIONS:
+ case OFPACT_CLONE:
+ case OFPACT_CONJUNCTION:
+ case OFPACT_CONTROLLER:
+ case OFPACT_CT_CLEAR:
+ case OFPACT_DEBUG_RECIRC:
+ case OFPACT_DEC_MPLS_TTL:
+ case OFPACT_DEC_TTL:
+ case OFPACT_ENQUEUE:
+ case OFPACT_EXIT:
+ case OFPACT_FIN_TIMEOUT:
+ case OFPACT_GOTO_TABLE:
+ case OFPACT_GROUP:
+ case OFPACT_LEARN:
+ case OFPACT_MULTIPATH:
+ case OFPACT_NOTE:
+ case OFPACT_OUTPUT:
+ case OFPACT_OUTPUT_REG:
+ case OFPACT_POP_MPLS:
+ case OFPACT_POP_QUEUE:
+ case OFPACT_PUSH_MPLS:
+ case OFPACT_PUSH_VLAN:
+ case OFPACT_REG_MOVE:
+ case OFPACT_RESUBMIT:
+ case OFPACT_SAMPLE:
+ case OFPACT_SET_ETH_DST:
+ case OFPACT_SET_ETH_SRC:
+ case OFPACT_SET_FIELD:
+ case OFPACT_SET_IP_DSCP:
+ case OFPACT_SET_IP_ECN:
+ case OFPACT_SET_IP_TTL:
+ case OFPACT_SET_IPV4_DST:
+ case OFPACT_SET_IPV4_SRC:
+ case OFPACT_SET_L4_DST_PORT:
+ case OFPACT_SET_L4_SRC_PORT:
+ case OFPACT_SET_MPLS_LABEL:
+ case OFPACT_SET_MPLS_TC:
+ case OFPACT_SET_MPLS_TTL:
+ case OFPACT_SET_QUEUE:
+ case OFPACT_SET_TUNNEL:
+ case OFPACT_SET_VLAN_PCP:
+ case OFPACT_SET_VLAN_VID:
+ case OFPACT_STACK_POP:
+ case OFPACT_STACK_PUSH:
+ case OFPACT_STRIP_VLAN:
+ case OFPACT_UNROLL_XLATE:
+ case OFPACT_WRITE_ACTIONS:
+ case OFPACT_WRITE_METADATA:
+ break;
+
+ case OFPACT_CT:
+ case OFPACT_METER:
+ case OFPACT_NAT:
+ case OFPACT_OUTPUT_TRUNC:
+ return false;
}
- ctx->base_flow = *old_base_flow;
- } else { /* Use actions. */
- do_xlate_actions(oc->actions, ofpact_nest_get_action_len(oc), ctx);
}
+ return true;
}
static void
-xlate_clone(struct xlate_ctx *ctx, const struct ofpact_nest *oc)
+compose_clone(struct xlate_ctx *ctx, const struct ofpact_nest *oc)
{
- bool old_was_mpls = ctx->was_mpls;
- bool old_conntracked = ctx->conntracked;
- struct flow old_flow = ctx->xin->flow;
-
struct ofpbuf old_stack = ctx->stack;
union mf_subvalue new_stack[1024 / sizeof(union mf_subvalue)];
ofpbuf_use_stub(&ctx->stack, new_stack, sizeof new_stack);
@@ -5298,16 +5332,57 @@ xlate_clone(struct xlate_ctx *ctx, const struct ofpact_nest *oc)
ofpbuf_use_stub(&ctx->action_set, actset_stub, sizeof actset_stub);
ofpbuf_put(&ctx->action_set, old_action_set.data, old_action_set.size);
+ size_t offset, ac_offset;
+ size_t oc_actions_len = ofpact_nest_get_action_len(oc);
+ struct flow old_flow = ctx->xin->flow;
+
+ if (reversible_actions(oc->actions, oc_actions_len)) {
+ old_flow = ctx->xin->flow;
+ do_xlate_actions(oc->actions, oc_actions_len, ctx);
+ goto xlate_done;
+ }
+
+ /* Commit datapath actions before emitting the clone action to
+ * avoid emitting those actions twice. Once inside
+ * the clone, another time for the action after clone. */
+ xlate_commit_actions(ctx);
struct flow old_base = ctx->base_flow;
- compose_clone(ctx, &old_base, oc);
+ bool old_was_mpls = ctx->was_mpls;
+ bool old_conntracked = ctx->conntracked;
- ofpbuf_uninit(&ctx->action_set);
- ctx->action_set = old_action_set;
- ofpbuf_uninit(&ctx->stack);
- ctx->stack = old_stack;
+ /* The actions are not reversible, a datapath clone action is
+ * required to encode the translation. Select the clone action
+ * based on datapath capabilities. */
+ if (ctx->xbridge->support.clone) { /* Use clone action */
+ /* Use clone action as datapath clone. */
+ offset = nl_msg_start_nested(ctx->odp_actions, OVS_ACTION_ATTR_CLONE);
+ ac_offset = ctx->odp_actions->size;
+ do_xlate_actions(oc->actions, oc_actions_len, ctx);
+ nl_msg_end_non_empty_nested(ctx->odp_actions, offset);
+ goto dp_clone_done;
+ }
- ctx->xin->flow = old_flow;
+ if (ctx->xbridge->support.sample_nesting > 3) {
+ /* Use sample action as datapath clone. */
+ offset = nl_msg_start_nested(ctx->odp_actions, OVS_ACTION_ATTR_SAMPLE);
+ ac_offset = nl_msg_start_nested(ctx->odp_actions,
+ OVS_SAMPLE_ATTR_ACTIONS);
+ do_xlate_actions(oc->actions, oc_actions_len, ctx);
+ if (nl_msg_end_non_empty_nested(ctx->odp_actions, ac_offset)) {
+ nl_msg_cancel_nested(ctx->odp_actions, offset);
+ } else {
+ nl_msg_put_u32(ctx->odp_actions, OVS_SAMPLE_ATTR_PROBABILITY,
+ UINT32_MAX); /* 100% probability. */
+ nl_msg_end_nested(ctx->odp_actions, offset);
+ }
+ goto dp_clone_done;
+ }
+
+ /* Datapath does not support clone, skip xlate 'oc' and
+ * report an error */
+ xlate_report_error(ctx, "Failed to compose clone action");
+dp_clone_done:
/* The clone's conntrack execution should have no effect on the original
* packet. */
ctx->conntracked = old_conntracked;
@@ -5315,6 +5390,16 @@ xlate_clone(struct xlate_ctx *ctx, const struct ofpact_nest *oc)
/* Popping MPLS from the clone should have no effect on the original
* packet. */
ctx->was_mpls = old_was_mpls;
+
+ /* Restore the 'base_flow' for the next action. */
+ ctx->base_flow = old_base;
+
+xlate_done:
+ ofpbuf_uninit(&ctx->action_set);
+ ctx->action_set = old_action_set;
+ ofpbuf_uninit(&ctx->stack);
+ ctx->stack = old_stack;
+ ctx->xin->flow = old_flow;
}
static void
@@ -6166,7 +6251,7 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
break;
case OFPACT_CLONE:
- xlate_clone(ctx, ofpact_get_CLONE(a));
+ compose_clone(ctx, ofpact_get_CLONE(a));
break;
case OFPACT_CT:
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index ceff93014dc2..fc81e1ebfa6d 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -6772,6 +6772,7 @@ AT_SETUP([ofproto-dpif - clone action])
OVS_VSWITCHD_START
add_of_ports br0 1 2 3 4
+dnl Reversible open flow clone actions, no datapath clone action should be generated.
AT_DATA([flows.txt], [dnl
in_port=1, ip, actions=clone(set_field:192.168.3.3->ip_src),clone(set_field:192.168.4.4->ip_dst,output:2),clone(mod_dl_src:80:81:81:81:81:81,set_field:192.168.5.5->ip_dst,output:3),output:4
])
@@ -6780,7 +6781,7 @@ AT_CHECK([ovs-ofctl add-flows br0 flows.txt], [0], [ignore])
AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.10.10.2,dst=10.10.10.1,proto=1,tos=1,ttl=128,frag=no),icmp(type=8,code=0)'], [0], [stdout])
AT_CHECK([tail -1 stdout], [0], [dnl
-Datapath actions: clone(set(ipv4(src=10.10.10.2,dst=192.168.4.4)),2),clone(set(eth(src=80:81:81:81:81:81)),set(ipv4(src=10.10.10.2,dst=192.168.5.5)),3),4
+Datapath actions: set(ipv4(src=10.10.10.2,dst=192.168.4.4)),2,set(eth(src=80:81:81:81:81:81)),set(ipv4(src=10.10.10.2,dst=192.168.5.5)),3,set(eth(src=50:54:00:00:00:09)),set(ipv4(src=10.10.10.2,dst=10.10.10.1)),4
])
dnl Test flow xlate openflow clone action without using datapath clone action.
@@ -6789,7 +6790,7 @@ AT_CHECK([ovs-appctl dpif/set-dp-features br0 clone false], [0], [ignore])
AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.10.10.2,dst=10.10.10.1,proto=1,tos=1,ttl=128,frag=no),icmp(type=8,code=0)'], [0], [stdout])
AT_CHECK([tail -1 stdout], [0], [dnl
-Datapath actions: sample(sample=100.0%,actions(set(ipv4(src=10.10.10.2,dst=192.168.4.4)),2)),sample(sample=100.0%,actions(set(eth(src=80:81:81:81:81:81)),set(ipv4(src=10.10.10.2,dst=192.168.5.5)),3)),4
+Datapath actions: set(ipv4(src=10.10.10.2,dst=192.168.4.4)),2,set(eth(src=80:81:81:81:81:81)),set(ipv4(src=10.10.10.2,dst=192.168.5.5)),3,set(eth(src=50:54:00:00:00:09)),set(ipv4(src=10.10.10.2,dst=10.10.10.1)),4
])
AT_CHECK([ovs-appctl dpif/set-dp-features br0 sample_nesting 2], [0], [ignore])
@@ -6799,6 +6800,40 @@ AT_CHECK([tail -1 stdout], [0], [dnl
Datapath actions: set(ipv4(src=10.10.10.2,dst=192.168.4.4)),2,set(eth(src=80:81:81:81:81:81)),set(ipv4(src=10.10.10.2,dst=192.168.5.5)),3,set(eth(src=50:54:00:00:00:09)),set(ipv4(src=10.10.10.2,dst=10.10.10.1)),4
])
+dnl Mixing reversible and irreversible open flow clone actions. Datapath clone action
+dnl should be generated when necessary.
+
+dnl Restore the datapath support level.
+AT_CHECK([ovs-appctl dpif/set-dp-features br0 clone true], [0], [])
+AT_CHECK([ovs-appctl dpif/set-dp-features br0 sample_nesting 10], [0], [])
+
+AT_DATA([flows.txt], [dnl
+in_port=1, ip, actions=clone(set_field:192.168.3.3->ip_src),clone(set_field:192.168.4.4->ip_dst,output:2),clone(ct(commit),output:3),output:4
+])
+AT_CHECK([ovs-ofctl add-flows br0 flows.txt], [0], [ignore])
+
+AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.10.10.2,dst=10.10.10.1,proto=1,tos=1,ttl=128,frag=no),icmp(type=8,code=0)'], [0], [stdout])
+
+AT_CHECK([tail -1 stdout], [0], [dnl
+Datapath actions: set(ipv4(src=10.10.10.2,dst=192.168.4.4)),2,set(ipv4(src=10.10.10.2,dst=10.10.10.1)),clone(ct(commit),3),4
+])
+
+AT_CHECK([ovs-appctl dpif/set-dp-features br0 clone false], [0], [ignore])
+
+AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.10.10.2,dst=10.10.10.1,proto=1,tos=1,ttl=128,frag=no),icmp(type=8,code=0)'], [0], [stdout])
+
+AT_CHECK([tail -1 stdout], [0], [dnl
+Datapath actions: set(ipv4(src=10.10.10.2,dst=192.168.4.4)),2,set(ipv4(src=10.10.10.2,dst=10.10.10.1)),sample(sample=100.0%,actions(ct(commit),3)),4
+])
+
+AT_CHECK([ovs-appctl dpif/set-dp-features br0 sample_nesting 2], [0], [ignore])
+AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.10.10.2,dst=10.10.10.1,proto=1,tos=1,ttl=128,frag=no),icmp(type=8,code=0)'], [0], [stdout])
+
+AT_CHECK([tail -1 stdout], [0], [dnl
+Datapath actions: set(ipv4(src=10.10.10.2,dst=192.168.4.4)),2,set(ipv4(src=10.10.10.2,dst=10.10.10.1)),4
+])
+AT_CHECK([grep "Failed to compose clone action" stdout], [0], [ignore])
+
OVS_VSWITCHD_STOP
AT_CLEANUP
--
1.8.3.1
More information about the dev
mailing list