ofproto-dpif-xlate: Optimize datapath action set by removing last clone action.

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: feee58b958 ("ofproto-dpif-xlate: Keep track of the last action")
Fixes: dadd8357f2 ("ofproto-dpif: Fix issue with non-reversible actions on a patch ports.")
Acked-by: Ales Musil <amusil@redhat.com>
Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>

ofproto/ofproto-dpif-xlate.c

@@ -7671,6 +7671,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
@@ -8096,6 +8129,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. */

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 -- dnl
+   add-port br0 p1 -- set Interface p1 type=patch dnl
+                                       options:peer=p2 ofport_request=2 -- dnl
+   add-br br1 -- dnl
+   set bridge br1 other-config:hwaddr=aa:66:aa:66:00:00 -- dnl
+   set bridge br1 datapath-type=dummy other-config:datapath-id=1234 dnl
+                  fail-mode=secure -- dnl
+   add-port br1 p2 -- set Interface p2 type=patch dnl
+                                       options:peer=p1 -- dnl
+   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' | dnl
+          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])
 

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

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