xlate: Refactor compose_clone() API

Create a new function that hides the details of netlink encoding for the translated clone action. Signed-off-by: Andy Zhou <[email protected]> Acked-by: Ben Pfaff <[email protected]>
lhh · Aug 3, 2017 · 9c2a44d · 9c2a44d
1 parent 80dc5c8
commit 9c2a44d
Show file tree

Hide file tree

Showing 2 changed files with 39 additions and 44 deletions.
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
@@ -5249,35 +5249,39 @@ xlate_sample_action(struct xlate_ctx *ctx,
                           tunnel_out_port, false);
 }
 
-/* Use datapath 'clone' or sample to enclose the translation of 'oc'.   */
+/* 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_action(struct xlate_ctx *ctx, const struct ofpact_nest *oc)
+compose_clone(struct xlate_ctx *ctx, struct flow *old_base_flow,
+              const struct ofpact_nest *oc)
 {
-    size_t 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, clone_offset);
-}
-
-/* Use datapath 'sample' action to translate clone.  */
-static void
-compose_clone_action_using_sample(struct xlate_ctx *ctx,
-                                  const struct ofpact_nest *oc)
-{
-    size_t offset = nl_msg_start_nested(ctx->odp_actions,
-                                        OVS_ACTION_ATTR_SAMPLE);
-
-    size_t 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);
+    size_t offset, ac_offset;
 
-    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);
+    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);
+        }
+        ctx->base_flow = *old_base_flow;
+    } else {  /* Use actions. */
+        do_xlate_actions(oc->actions, ofpact_nest_get_action_len(oc), ctx);
     }
 }
 
@@ -5298,27 +5302,11 @@ 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);
 
-    /* Datapath clone action will make sure the pre clone packets
-     * are used for actions after clone. Save and restore
-     * ctx->base_flow to reflect this for the openflow pipeline. */
-    if (ctx->xbridge->support.clone) {
-        struct flow old_base_flow = ctx->base_flow;
-        compose_clone_action(ctx, oc);
-        ctx->base_flow = old_base_flow;
-    } else if (ctx->xbridge->support.sample_nesting > 3) {
-        /* Avoid generate sample action if datapath
-         * only allow small number of nesting. Deeper nesting
-         * can cause the datapath to reject the generated flow.  */
-        struct flow old_base_flow = ctx->base_flow;
-        compose_clone_action_using_sample(ctx, oc);
-        ctx->base_flow = old_base_flow;
-    } else {
-        do_xlate_actions(oc->actions, ofpact_nest_get_action_len(oc), ctx);
-    }
+    struct flow old_base = ctx->base_flow;
+    compose_clone(ctx, &old_base, oc);
 
     ofpbuf_uninit(&ctx->action_set);
     ctx->action_set = old_action_set;
-
     ofpbuf_uninit(&ctx->stack);
     ctx->stack = old_stack;
 

diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
@@ -6821,6 +6821,13 @@ 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
 ])
 
+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(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
+])
+
 OVS_VSWITCHD_STOP
 AT_CLEANUP