[ovs-dev] [RFC PATCHv2] ofp-actions: Add clone action.

[Ben Pfaff]

On Wed, Nov 30, 2016 at 01:35:49PM -0800, William Tu wrote:
> This patch adds OpenFlow clone action with syntax as below:
> "clone([action][,action...])".  The clone() action makes a copy of the
> current packet and executes the list of actions against the packet,
> without affecting the packet after the "clone(...)" action.  In other
> word, the packet before the clone() and after the clone() is the same,
> no matter what actions executed inside the clone().
> 
> The patch reuses the dapatah SAMPLE action, with probability of 100%.
> The kernel datapath 'sample()' clones the skb and its flow key under this
> circumstance, and actions specified in the clone() are executed against
> the cloned skb.  Note that there is no performance overhead of clone, since
> if eventually the packet is output, it will also clone the packet.  Here we
> simply move the copy at the beginning.
> 
> The complexity of adding clone might outweight its use cases.  I'm looking
> for comments as well as listing some cases below:
> Use case 1:
> Set different fields and output to different ports without unset
> actions=
>   clone(mod_dl_src:<mac1>, output:1), clone(mod_dl_dst:<mac2>, output:2), output:3
> Since each clone() has independent packet, output:1 has only dl_src modified,
> output:2 has only dl_dst modified, output:3 has original packet.
> 
> Similar to case1
> actions=
>   push_vlan(...), output:2, pop_vlan, push_vlan(...), output:3
> can be changed to
> actions=
>   clone(push_vlan(...), output:2),clone(push_vlan(...), output:3)
> without having to add pop_vlan.
> 
> case 2: resubmit to another table without worrying packet being modified
>   actions=clone(resubmit(1,2)), ...

Using "sample" for the above use cases is unneeded, and it's undesirable
because it makes the translations bigger and slower.

> case 3: truncate in the clone action
> Currently OVS can truncate packet by 'output(port=1,max_len=100)', which
> ties truncate and output together.  However, sometimes the layer decides
> to truncate is separate from the layer to output.  One proposal is to
> introduce actions s.t like truncate_set() and truncate_unset(), where
> only the action in between sees truncated packet.  Another approach is
> to use clone() as below:
> actions=
>   clone(truncate(100), push_vlan, resubmit, ...)
> where we don't need to worry about missing the truncate_unset() because
> truncated packet is not visible outside the clone().

I see how "clone" helps with this conceptually, but I'm not sure why the
"sample" is necessary.  I think that the proposed value here is that
"sample" allows the truncate to be canceled if no output occurs after
"truncate" and before the end of the "sample" action.  But it's also
possible for the translation code to see whether there's an output
action within the clone and, if there is none, then to refrain from
emitting the datapath "truncate" action entirely.  That seems like a
more efficient way to implement it.  Will that work?

> We definitely should put some limit on the action types available inside
> clone(). For this patch, there is no restriction.

Why should we limit the actions available inside clone?

> Signed-off-by: William Tu <u9012063 at gmail.com>

This incremental is needed to avoid putting anything emitted by
xlate_commit_actions() into the middle of the sample action, otherwise
the OVN test cases segfault (after my patches are applied):

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 9bcefcd..c30f93b 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -4296,8 +4296,6 @@ compose_clone_action(struct xlate_ctx *ctx,
 {
     struct flow old_flow, old_base_flow;
     size_t actions_offset;
-    size_t sample_offset = nl_msg_start_nested(ctx->odp_actions,
-                                               OVS_ACTION_ATTR_SAMPLE);
 
     /* Ensure that any prior actions are applied. */
     xlate_commit_actions(ctx);
@@ -4307,6 +4305,8 @@ compose_clone_action(struct xlate_ctx *ctx,
     old_base_flow = ctx->base_flow;
 
     /* Sample with 100% Probability */
+    size_t sample_offset = nl_msg_start_nested(ctx->odp_actions,
+                                               OVS_ACTION_ATTR_SAMPLE);
     nl_msg_put_u32(ctx->odp_actions, OVS_SAMPLE_ATTR_PROBABILITY, UINT32_MAX);
     actions_offset = nl_msg_start_nested(ctx->odp_actions,
                                          OVS_SAMPLE_ATTR_ACTIONS);