[ovs-dev,2/2] xlate: normalize the actions after translation

Message ID 1512557728-31050-3-git-send-email-zoltan.balogh@ericsson.com
State New
Headers show
Series
  • xlate: optimize dp flow action in case of error in multi-bridge setup
Related show

Commit Message

Zoltán Balogh Dec. 6, 2017, 10:55 a.m.
When all OF actions have been translated, there could be actions at
the end of list of odp actions which are not needed to be executed.
So, the list can be normalized at the end of xlate_actions().

Signed-off-by: Zoltan Balogh <zoltan.balogh@ericsson.com>
Signed-off-by: Sugesh Chandran <sugesh.chandran@intel.com>
Co-authored-by: Sugesh Chandran <sugesh.chandran@intel.com>
CC: Ben Pfaff <blp@ovn.org>
---
 ofproto/ofproto-dpif-xlate.c | 67 ++++++++++++++++++++++++++++++++++++++++++++
 tests/ofproto-dpif.at        | 48 +++++++++++++++++++++++++++++++
 2 files changed, 115 insertions(+)

Comments

Zoltán Balogh Dec. 6, 2017, 11 a.m. | #1
Subject is incorrect, missing v2. Please use this instead:
https://mail.openvswitch.org/pipermail/ovs-dev/2017-December/341658.html


> -----Original Message-----
> From: Zoltán Balogh
> Sent: Wednesday, December 06, 2017 11:55 AM
> To: dev@openvswitch.org
> Cc: Zoltán Balogh <zoltan.balogh@ericsson.com>; Sugesh Chandran <sugesh.chandran@intel.com>; Ben Pfaff <blp@ovn.org>
> Subject: [PATCH 2/2] xlate: normalize the actions after translation
> 
> When all OF actions have been translated, there could be actions at
> the end of list of odp actions which are not needed to be executed.
> So, the list can be normalized at the end of xlate_actions().
> 
> Signed-off-by: Zoltan Balogh <zoltan.balogh@ericsson.com>
> Signed-off-by: Sugesh Chandran <sugesh.chandran@intel.com>
> Co-authored-by: Sugesh Chandran <sugesh.chandran@intel.com>
> CC: Ben Pfaff <blp@ovn.org>
> ---
>  ofproto/ofproto-dpif-xlate.c | 67 ++++++++++++++++++++++++++++++++++++++++++++
>  tests/ofproto-dpif.at        | 48 +++++++++++++++++++++++++++++++
>  2 files changed, 115 insertions(+)
> 
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 36d0a0e1f..2af1ec1e8 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -6954,6 +6954,70 @@ xlate_wc_finish(struct xlate_ctx *ctx)
>      }
>  }
> 
> +/* Returns true if the action stored in 'nla' can be a valid last action of a
> + * datapath flow. */
> +static bool
> +is_valid_last_action(const struct nlattr *nla)
> +{
> +    enum ovs_action_attr action_type = nl_attr_type(nla);
> +
> +    switch (action_type) {
> +    case OVS_ACTION_ATTR_USERSPACE:
> +    case OVS_ACTION_ATTR_SAMPLE:
> +    case OVS_ACTION_ATTR_TRUNC:
> +    case OVS_ACTION_ATTR_RECIRC:
> +    case OVS_ACTION_ATTR_TUNNEL_PUSH:
> +    case OVS_ACTION_ATTR_TUNNEL_POP:
> +    case OVS_ACTION_ATTR_OUTPUT:
> +    case OVS_ACTION_ATTR_CLONE:
> +    case OVS_ACTION_ATTR_CT:
> +        return true;
> +    case OVS_ACTION_ATTR_UNSPEC:
> +    case OVS_ACTION_ATTR_SET:
> +    case OVS_ACTION_ATTR_PUSH_VLAN:
> +    case OVS_ACTION_ATTR_POP_VLAN:
> +    case OVS_ACTION_ATTR_HASH:
> +    case OVS_ACTION_ATTR_PUSH_MPLS:
> +    case OVS_ACTION_ATTR_POP_MPLS:
> +    case OVS_ACTION_ATTR_SET_MASKED:
> +    case OVS_ACTION_ATTR_PUSH_ETH:
> +    case OVS_ACTION_ATTR_POP_ETH:
> +    case OVS_ACTION_ATTR_METER:
> +    case OVS_ACTION_ATTR_ENCAP_NSH:
> +    case OVS_ACTION_ATTR_DECAP_NSH:
> +    case __OVS_ACTION_ATTR_MAX:
> +    default:
> +        return false;
> +    }
> +}
> +
> +/* Returns offset of last netlink attribute storing valid action in array
> + * 'data'. Execution of actions beyond this last attribute does not make sense.
> + */
> +static size_t
> +last_action_offset(const struct nlattr *data, const size_t data_len)
> +{
> +    const struct nlattr *last = data;
> +
> +    uint16_t left;
> +    const struct nlattr *a;
> +    NL_ATTR_FOR_EACH (a, left, data, data_len) {
> +        if (is_valid_last_action(a)) {
> +            last = nl_attr_next(a);
> +        }
> +    }
> +
> +    return (char *) last - (char *) data;
> +}
> +
> +/* Get rid of any unneeded actions at the tail end. */
> +static void
> +normalize_odp_actions(struct xlate_ctx *ctx)
> +{
> +    struct ofpbuf *oa = ctx->odp_actions;
> +    oa->size = last_action_offset(oa->data, oa->size);
> +}
> +
>  /* Translates the flow, actions, or rule in 'xin' into datapath actions in
>   * 'xout'.
>   * The caller must take responsibility for eventually freeing 'xout', with
> @@ -7364,7 +7428,10 @@ exit:
>          if (xin->odp_actions) {
>              ofpbuf_clear(xin->odp_actions);
>          }
> +    } else {
> +        normalize_odp_actions(&ctx);
>      }
> +
>      return ctx.error;
>  }
> 
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index e7df1504a..5bcd8bb46 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -10133,3 +10133,51 @@ AT_CHECK([grep "Datapath actions" stdout], [0],
> 
>  OVS_VSWITCHD_STOP
>  AT_CLEANUP
> +
> +AT_SETUP([ofproto-dpif - xlate error - normalize actions])
> +
> +#      ->-+
> +#         | p1
> +#       +-o-------+
> +#       |   br0   |
> +#       +-o-----o-+
> +#   patch0|     |patch1
> +#         +-->--+
> +
> +OVS_VSWITCHD_START([dnl
> +    -- add-port br0 patch0 \
> +    -- set interface patch0 type=patch options:peer=patch1 ofport_request=10 \
> +    -- add-port br0 patch1 \
> +    -- set interface patch1 type=patch options:peer=patch0 ofport_request=20
> +])
> +
> +AT_CHECK([
> +    ovs-vsctl add-port br0 p1 -- set interface p1 type=dummy ofport_request=1
> +])
> +
> +AT_CHECK([
> +    ovs-appctl dpif/show | grep -v hit | sed "s/$(printf \\t)/    /g" | sed 's./[[0-9]]\{1,\}..'
> +], [0], [dnl
> +    br0:
> +        br0 65534: (dummy-internal)
> +        p1 1: (dummy)
> +        patch0 10/none: (patch: peer=patch1)
> +        patch1 20/none: (patch: peer=patch0)
> +])
> +
> +# Error due to pushing too many MPLS labels.
> +AT_CHECK([
> +    ovs-ofctl del-flows br0
> +    ovs-ofctl add-flow br0 "table=0,in_port=1,actions=mod_dl_src:aa:aa:aa:bb:bb:00,goto_table:1" -OOpenFlow13
> +    ovs-ofctl add-flow br0 "table=0,in_port=patch1,actions=goto_table:2" -OOpenFlow13
> +    ovs-ofctl add-flow br0 "table=1,actions=push_vlan:0x8100,set_field:4196->vlan_vid,output:patch0" -OOpenFlow13
> +    ovs-ofctl add-flow br0 "table=2,actions=push_mpls:0x8847,output:patch0"
> +], [0])
> +
> +AT_CHECK([ovs-appctl ofproto/trace br0 'in_port=1,ip'], [0], [stdout])
> +AT_CHECK([tail -1 stdout], [0],
> +  [Datapath actions: drop
> +])
> +
> +OVS_VSWITCHD_STOP
> +AT_CLEANUP
> --
> 2.14.1

Patch

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 36d0a0e1f..2af1ec1e8 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -6954,6 +6954,70 @@  xlate_wc_finish(struct xlate_ctx *ctx)
     }
 }
 
+/* Returns true if the action stored in 'nla' can be a valid last action of a
+ * datapath flow. */
+static bool
+is_valid_last_action(const struct nlattr *nla)
+{
+    enum ovs_action_attr action_type = nl_attr_type(nla);
+
+    switch (action_type) {
+    case OVS_ACTION_ATTR_USERSPACE:
+    case OVS_ACTION_ATTR_SAMPLE:
+    case OVS_ACTION_ATTR_TRUNC:
+    case OVS_ACTION_ATTR_RECIRC:
+    case OVS_ACTION_ATTR_TUNNEL_PUSH:
+    case OVS_ACTION_ATTR_TUNNEL_POP:
+    case OVS_ACTION_ATTR_OUTPUT:
+    case OVS_ACTION_ATTR_CLONE:
+    case OVS_ACTION_ATTR_CT:
+        return true;
+    case OVS_ACTION_ATTR_UNSPEC:
+    case OVS_ACTION_ATTR_SET:
+    case OVS_ACTION_ATTR_PUSH_VLAN:
+    case OVS_ACTION_ATTR_POP_VLAN:
+    case OVS_ACTION_ATTR_HASH:
+    case OVS_ACTION_ATTR_PUSH_MPLS:
+    case OVS_ACTION_ATTR_POP_MPLS:
+    case OVS_ACTION_ATTR_SET_MASKED:
+    case OVS_ACTION_ATTR_PUSH_ETH:
+    case OVS_ACTION_ATTR_POP_ETH:
+    case OVS_ACTION_ATTR_METER:
+    case OVS_ACTION_ATTR_ENCAP_NSH:
+    case OVS_ACTION_ATTR_DECAP_NSH:
+    case __OVS_ACTION_ATTR_MAX:
+    default:
+        return false;
+    }
+}
+
+/* Returns offset of last netlink attribute storing valid action in array
+ * 'data'. Execution of actions beyond this last attribute does not make sense.
+ */
+static size_t
+last_action_offset(const struct nlattr *data, const size_t data_len)
+{
+    const struct nlattr *last = data;
+
+    uint16_t left;
+    const struct nlattr *a;
+    NL_ATTR_FOR_EACH (a, left, data, data_len) {
+        if (is_valid_last_action(a)) {
+            last = nl_attr_next(a);
+        }
+    }
+
+    return (char *) last - (char *) data;
+}
+
+/* Get rid of any unneeded actions at the tail end. */
+static void
+normalize_odp_actions(struct xlate_ctx *ctx)
+{
+    struct ofpbuf *oa = ctx->odp_actions;
+    oa->size = last_action_offset(oa->data, oa->size);
+}
+
 /* Translates the flow, actions, or rule in 'xin' into datapath actions in
  * 'xout'.
  * The caller must take responsibility for eventually freeing 'xout', with
@@ -7364,7 +7428,10 @@  exit:
         if (xin->odp_actions) {
             ofpbuf_clear(xin->odp_actions);
         }
+    } else {
+        normalize_odp_actions(&ctx);
     }
+
     return ctx.error;
 }
 
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index e7df1504a..5bcd8bb46 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -10133,3 +10133,51 @@  AT_CHECK([grep "Datapath actions" stdout], [0],
 
 OVS_VSWITCHD_STOP
 AT_CLEANUP
+
+AT_SETUP([ofproto-dpif - xlate error - normalize actions])
+
+#      ->-+
+#         | p1
+#       +-o-------+
+#       |   br0   |
+#       +-o-----o-+
+#   patch0|     |patch1
+#         +-->--+
+
+OVS_VSWITCHD_START([dnl
+    -- add-port br0 patch0 \
+    -- set interface patch0 type=patch options:peer=patch1 ofport_request=10 \
+    -- add-port br0 patch1 \
+    -- set interface patch1 type=patch options:peer=patch0 ofport_request=20
+])
+
+AT_CHECK([
+    ovs-vsctl add-port br0 p1 -- set interface p1 type=dummy ofport_request=1
+])
+
+AT_CHECK([
+    ovs-appctl dpif/show | grep -v hit | sed "s/$(printf \\t)/    /g" | sed 's./[[0-9]]\{1,\}..'
+], [0], [dnl
+    br0:
+        br0 65534: (dummy-internal)
+        p1 1: (dummy)
+        patch0 10/none: (patch: peer=patch1)
+        patch1 20/none: (patch: peer=patch0)
+])
+
+# Error due to pushing too many MPLS labels.
+AT_CHECK([
+    ovs-ofctl del-flows br0
+    ovs-ofctl add-flow br0 "table=0,in_port=1,actions=mod_dl_src:aa:aa:aa:bb:bb:00,goto_table:1" -OOpenFlow13
+    ovs-ofctl add-flow br0 "table=0,in_port=patch1,actions=goto_table:2" -OOpenFlow13
+    ovs-ofctl add-flow br0 "table=1,actions=push_vlan:0x8100,set_field:4196->vlan_vid,output:patch0" -OOpenFlow13
+    ovs-ofctl add-flow br0 "table=2,actions=push_mpls:0x8847,output:patch0"
+], [0])
+
+AT_CHECK([ovs-appctl ofproto/trace br0 'in_port=1,ip'], [0], [stdout])
+AT_CHECK([tail -1 stdout], [0],
+  [Datapath actions: drop
+])
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP