When OFPROTO non-reversible actions are translated to data plane
actions, the only thing looked at is if there are more actions
pending. If this is the case, the action is encapsulated in a
clone().
This could lead to unnecessary clones if no meaningful data
plane actions are added. For example, the register pop in the
included test case.
The best solution would probably be to build the full action
path and determine if the clone is needed. However, this would
be a huge change in the existing design, so for now, we just try
to optimize the generated datapath flow. We can revisit this
later, as some of the pending CT issues might need this rework.
Fixes: feee58b9587f ("ofproto-dpif-xlate: Keep track of the last action")
Fixes: dadd8357f224 ("ofproto-dpif: Fix issue with non-reversible actions on a
patch ports.")
Signed-off-by: Eelco Chaudron <[email protected]>
---
ofproto/ofproto-dpif-xlate.c | 37 +++++++++++++++++++++++++++++++++++++
tests/ofproto-dpif.at | 34 ++++++++++++++++++++++++++++++++++
tests/system-layer3-tunnels.at | 2 +-
tests/tunnel-push-pop.at | 4 ++--
4 files changed, 74 insertions(+), 3 deletions(-)
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index fda802e83..06fe47f6a 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -7667,6 +7667,39 @@ xlate_wc_finish(struct xlate_ctx *ctx)
}
}
+/* This will optimize the odp actions generated. For now, it will remove
+ * trailing clone actions that are unnecessary. */
+static void
+xlate_optimize_odp_actions(struct xlate_in *xin)
+{
+ struct ofpbuf *actions = xin->odp_actions;
+ struct nlattr *last_action = NULL;
+ struct nlattr *a;
+ int left;
+
+ if (!actions) {
+ return;
+ }
+
+ /* Find the last action in the set. */
+ NL_ATTR_FOR_EACH (a, left, actions->data, actions->size) {
+ last_action = a;
+ }
+
+ /* Remove the trailing clone() action, by directly embedding the nested
+ * actions. */
+ if (last_action && nl_attr_type(last_action) == OVS_ACTION_ATTR_CLONE) {
+ void *dest;
+
+ nl_msg_reset_size(actions,
+ (unsigned char *) last_action -
+ (unsigned char *) actions->data);
+
+ dest = nl_msg_put_uninit(actions, nl_attr_get_size(last_action));
+ memmove(dest, nl_attr_get(last_action), nl_attr_get_size(last_action));
+ }
+}
+
/* Translates the flow, actions, or rule in 'xin' into datapath actions in
* 'xout'.
* The caller must take responsibility for eventually freeing 'xout', with
@@ -8092,6 +8125,10 @@ exit:
if (xin->odp_actions) {
ofpbuf_clear(xin->odp_actions);
}
+ } else {
+ /* In the non-error case, see if we can further optimize the datapath
+ * rules by removing redundant (clone) actions. */
+ xlate_optimize_odp_actions(xin);
}
/* Install drop action if datapath supports explicit drop action. */
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 2e91ae1a1..b91166e88 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -8923,6 +8923,40 @@ AT_CHECK([tail -1 stdout], [0],
OVS_VSWITCHD_STOP
AT_CLEANUP
+AT_SETUP([ofproto-dpif - patch ports - no additional clone])
+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 -- \
+ add-port br1 p3 -- set Interface p3 type=dummy ofport_request=3])
+
+AT_DATA([flows-br0.txt], [dnl
+priority=10,tcp,action=push:NXM_OF_IN_PORT[],resubmit(,65),pop:NXM_OF_IN_PORT[]
+table=65,priority=10,ip,in_port=p0,action=p1
+])
+
+AT_DATA([flows-br1.txt], [dnl
+priority=100,in_port=p2,tcp,ct_state=-trk,action=ct(table=0,zone=1)
+priority=100,in_port=p2,tcp,ct_state=+trk+est,ct_zone=1,action=p3
+])
+
+AT_CHECK([ovs-ofctl --bundle add-flows br0 flows-br0.txt])
+AT_CHECK([ovs-ofctl --bundle add-flows br1 flows-br1.txt])
+
+AT_CHECK([ovs-appctl ofproto/trace br0 in_port=p0,tcp --ct-next 'trk,est' | \
+ grep "Datapath actions:" | grep -q clone],
+ [1], [], [],
+ [ovs-appctl ofproto/trace br0 in_port=p0,tcp --ct-next 'trk,est'])
+
+OVS_TRAFFIC_VSWITCHD_STOP
+AT_CLEANUP
+
dnl ----------------------------------------------------------------------
AT_BANNER([ofproto-dpif -- megaflows])
diff --git a/tests/system-layer3-tunnels.at b/tests/system-layer3-tunnels.at
index d21fd777d..c37852b21 100644
--- a/tests/system-layer3-tunnels.at
+++ b/tests/system-layer3-tunnels.at
@@ -147,7 +147,7 @@ AT_CHECK([tail -1 stdout], [0],
dnl Check GRE tunnel push
AT_CHECK([ovs-appctl ofproto/trace ovs-dummy
'in_port(3),eth(dst=f9:bc:12:44:34:b6,src=af:55:aa:55:00:03),eth_type(0x0800),ipv4(src=1.1.3.88,dst=1.1.3.92,proto=1,tos=0,ttl=64,frag=no)'],
[0], [stdout])
AT_CHECK([tail -1 stdout], [0],
- [Datapath actions:
clone(tnl_push(tnl_port(4),header(size=38,type=3,eth(dst=f8:bc:12:44:34:b6,src=aa:55:aa:55:00:03,dl_type=0x0800),ipv4(src=1.1.2.88,dst=1.1.2.92,proto=47,tos=0,ttl=64,frag=0x4000),gre((flags=0x0,proto=0x6558))),out_port(2)),1)
+ [Datapath actions:
tnl_push(tnl_port(4),header(size=38,type=3,eth(dst=f8:bc:12:44:34:b6,src=aa:55:aa:55:00:03,dl_type=0x0800),ipv4(src=1.1.2.88,dst=1.1.2.92,proto=47,tos=0,ttl=64,frag=0x4000),gre((flags=0x0,proto=0x6558))),out_port(2)),1
])
OVS_VSWITCHD_STOP
diff --git a/tests/tunnel-push-pop.at b/tests/tunnel-push-pop.at
index 2c6c52eda..bc82abc1f 100644
--- a/tests/tunnel-push-pop.at
+++ b/tests/tunnel-push-pop.at
@@ -906,11 +906,11 @@ AT_CHECK([ovs-appctl ofproto/trace ovs-tun0 in_port=p7],
[0], [stdout])
AT_CHECK([tail -2 stdout], [0], [dnl
Megaflow: recirc_id=0,eth,in_port=7,dl_src=00:00:00:00:00:00,dnl
dl_dst=00:00:00:00:00:00,dl_type=0x0000
-Datapath actions: push_vlan(vid=200,pcp=0),1,clone(tnl_push(tnl_port(4789),dnl
+Datapath actions: push_vlan(vid=200,pcp=0),1,tnl_push(tnl_port(4789),dnl
header(size=50,type=4,eth(dst=aa:55:aa:66:00:00,src=aa:55:aa:55:00:00,dnl
dl_type=0x0800),ipv4(src=10.0.0.2,dst=10.0.0.11,proto=17,tos=0,ttl=64,dnl
frag=0x4000),udp(src=0,dst=4789,csum=0x0),vxlan(flags=0x8000000,vni=0x0)),dnl
-out_port(100)),8)
+out_port(100)),8
])
OVS_VSWITCHD_STOP
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
