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: feee58b9587f ("ofproto-dpif-xlate: Keep track of the last action")
Fixes: dadd8357f224 ("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>

1 files changed, 37 insertions, 0 deletions

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index d53d1d5d8..9bba55710 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/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. */