[ovs-dev,V4,12/17] netdev-offload-dpdk: Framework for actions offload
diff mbox series

Message ID 20191216151047.5967-13-elibr@mellanox.com
State Changes Requested
Delegated to: Ilya Maximets
Headers show
Series
  • netdev datapath actions offload
Related show

Commit Message

Eli Britstein Dec. 16, 2019, 3:10 p.m. UTC
Currently HW offload is accelerating only the rule matching sequence.
Introduce a framework for offloading rule actions as a pre-step for
processing the rule actions in HW. In case of a failure, fallback to the
legacy partial offload scheme.

Note: a flow will be fully offloaded only if it can process all its
actions in HW.

Signed-off-by: Eli Britstein <elibr@mellanox.com>
Reviewed-by: Oz Shlomo <ozsh@mellanox.com>
---
 lib/netdev-offload-dpdk.c | 104 ++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 91 insertions(+), 13 deletions(-)

Comments

0-day Robot Dec. 16, 2019, 4:24 p.m. UTC | #1
Bleep bloop.  Greetings Eli Britstein, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
WARNING: Line is 80 characters long (recommended limit is 79)
#145 FILE: lib/netdev-offload-dpdk.c:801:
        flow = netdev_offload_dpdk_mark_rss(&patterns, netdev, info->flow_mark);

Lines checked: 160, Warnings: 1, Errors: 0


Please check this out.  If you feel there has been an error, please email aconole@redhat.com

Thanks,
0-day Robot
Ilya Maximets Dec. 16, 2019, 9:27 p.m. UTC | #2
On 16.12.2019 16:10, Eli Britstein wrote:
> Currently HW offload is accelerating only the rule matching sequence.
> Introduce a framework for offloading rule actions as a pre-step for
> processing the rule actions in HW. In case of a failure, fallback to the
> legacy partial offload scheme.
> 
> Note: a flow will be fully offloaded only if it can process all its
> actions in HW.
> 
> Signed-off-by: Eli Britstein <elibr@mellanox.com>
> Reviewed-by: Oz Shlomo <ozsh@mellanox.com>
> ---
>  lib/netdev-offload-dpdk.c | 104 ++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 91 insertions(+), 13 deletions(-)
> 
> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
> index 79a7d33fb..a60d68d2c 100644
> --- a/lib/netdev-offload-dpdk.c
> +++ b/lib/netdev-offload-dpdk.c
> @@ -691,6 +691,89 @@ add_flow_mark_rss_actions(struct flow_actions *actions,
>      add_flow_action(actions, RTE_FLOW_ACTION_TYPE_END, NULL);
>  }
>  
> +static struct rte_flow *
> +netdev_offload_dpdk_mark_rss(struct flow_patterns *patterns,
> +                             struct netdev *netdev,
> +                             uint32_t flow_mark)
> +{
> +    struct flow_actions actions = { .actions = NULL, .cnt = 0 };
> +    const struct rte_flow_attr flow_attr = {
> +        .group = 0,
> +        .priority = 0,
> +        .ingress = 1,
> +        .egress = 0
> +    };
> +    struct rte_flow_error error;
> +    struct rte_flow *flow;
> +
> +    add_flow_mark_rss_actions(&actions, flow_mark, netdev);
> +
> +    flow = netdev_offload_dpdk_flow_create(netdev, &flow_attr, patterns->items,
> +                                           actions.actions, &error);
> +
> +    if (!flow) {
> +        VLOG_ERR("%s: Failed to create flow: %s (%u)\n",
> +                 netdev_get_name(netdev), error.message, error.type);

netdev_offload_dpdk_flow_create() already printed the whole flow and the
error.  Do we need a duplicated message?

> +    }
> +
> +    free_flow_actions(&actions);
> +    return flow;
> +}
> +
> +static int
> +parse_flow_actions(struct netdev *netdev OVS_UNUSED,
> +                   struct flow_actions *actions,
> +                   struct nlattr *nl_actions,
> +                   size_t nl_actions_len,
> +                   struct offload_info *info OVS_UNUSED)
> +{
> +    struct nlattr *nla;
> +    size_t left;
> +
> +    NL_ATTR_FOR_EACH_UNSAFE (nla, left, nl_actions, nl_actions_len) {
> +        VLOG_DBG_RL(&error_rl,
> +                    "Unsupported action type %d", nl_attr_type(nla));
> +        return -1;
> +    }
> +
> +    if (nl_actions_len == 0) {
> +        VLOG_DBG_RL(&error_rl,
> +                    "Unsupported action type drop");
> +        return -1;
> +    }
> +
> +    add_flow_action(actions, RTE_FLOW_ACTION_TYPE_END, NULL);
> +    return 0;
> +}
> +
> +static struct rte_flow *
> +netdev_offload_dpdk_actions(struct netdev *netdev,
> +                            struct flow_patterns *patterns,
> +                            struct nlattr *nl_actions,
> +                            size_t actions_len,
> +                            struct offload_info *info)
> +{
> +    const struct rte_flow_attr flow_attr = { .ingress = 1, .transfer = 1 };
> +    struct flow_actions actions = { .actions = NULL, .cnt = 0 };
> +    struct rte_flow *flow = NULL;
> +    struct rte_flow_error error;
> +    int ret;
> +
> +    ret = parse_flow_actions(netdev, &actions, nl_actions, actions_len, info);
> +    if (ret) {
> +        goto out;
> +    }
> +    flow = netdev_offload_dpdk_flow_create(netdev, &flow_attr, patterns->items,
> +                                           actions.actions, &error);
> +    if (!flow) {
> +        VLOG_ERR("%s: Failed to create flow: %s (%u)\n",
> +                 netdev_get_name(netdev), error.message, error.type);

Ditto.

> +    }
> +out:
> +    free_flow_actions(&actions);
> +    return flow;
> +}
> +
>  static int
>  netdev_offload_dpdk_add_flow(struct netdev *netdev,
>                               const struct match *match,
> @@ -699,16 +782,8 @@ netdev_offload_dpdk_add_flow(struct netdev *netdev,
>                               const ovs_u128 *ufid,
>                               struct offload_info *info)
>  {
> -    const struct rte_flow_attr flow_attr = {
> -        .group = 0,
> -        .priority = 0,
> -        .ingress = 1,
> -        .egress = 0
> -    };
>      struct flow_patterns patterns = { .items = NULL, .cnt = 0 };
> -    struct flow_actions actions = { .actions = NULL, .cnt = 0 };
>      struct rte_flow *flow;
> -    struct rte_flow_error error;
>      int ret = 0;
>  
>      ret = parse_flow_match(&patterns, match);
> @@ -716,10 +791,14 @@ netdev_offload_dpdk_add_flow(struct netdev *netdev,
>          goto out;
>      }
>  
> -    add_flow_mark_rss_actions(&actions, info->flow_mark, netdev);
> -
> -    flow = netdev_offload_dpdk_flow_create(netdev, &flow_attr, patterns.items,
> -                                           actions.actions, &error);
> +    flow = netdev_offload_dpdk_actions(netdev, &patterns, nl_actions,
> +                                       actions_len, info);
> +    if (!flow) {
> +        /* if we failed to offload the rule actions fallback to mark rss
> +         * actions.
> +         */

Comment style.

> +        flow = netdev_offload_dpdk_mark_rss(&patterns, netdev, info->flow_mark);
> +    }
>  
>      if (!flow) {
>          ret = -1;
> @@ -731,7 +810,6 @@ netdev_offload_dpdk_add_flow(struct netdev *netdev,
>  
>  out:
>      free_flow_patterns(&patterns);
> -    free_flow_actions(&actions);
>      return ret;
>  }
>  
>

Patch
diff mbox series

diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
index 79a7d33fb..a60d68d2c 100644
--- a/lib/netdev-offload-dpdk.c
+++ b/lib/netdev-offload-dpdk.c
@@ -691,6 +691,89 @@  add_flow_mark_rss_actions(struct flow_actions *actions,
     add_flow_action(actions, RTE_FLOW_ACTION_TYPE_END, NULL);
 }
 
+static struct rte_flow *
+netdev_offload_dpdk_mark_rss(struct flow_patterns *patterns,
+                             struct netdev *netdev,
+                             uint32_t flow_mark)
+{
+    struct flow_actions actions = { .actions = NULL, .cnt = 0 };
+    const struct rte_flow_attr flow_attr = {
+        .group = 0,
+        .priority = 0,
+        .ingress = 1,
+        .egress = 0
+    };
+    struct rte_flow_error error;
+    struct rte_flow *flow;
+
+    add_flow_mark_rss_actions(&actions, flow_mark, netdev);
+
+    flow = netdev_offload_dpdk_flow_create(netdev, &flow_attr, patterns->items,
+                                           actions.actions, &error);
+
+    if (!flow) {
+        VLOG_ERR("%s: Failed to create flow: %s (%u)\n",
+                 netdev_get_name(netdev), error.message, error.type);
+    }
+
+    free_flow_actions(&actions);
+    return flow;
+}
+
+static int
+parse_flow_actions(struct netdev *netdev OVS_UNUSED,
+                   struct flow_actions *actions,
+                   struct nlattr *nl_actions,
+                   size_t nl_actions_len,
+                   struct offload_info *info OVS_UNUSED)
+{
+    struct nlattr *nla;
+    size_t left;
+
+    NL_ATTR_FOR_EACH_UNSAFE (nla, left, nl_actions, nl_actions_len) {
+        VLOG_DBG_RL(&error_rl,
+                    "Unsupported action type %d", nl_attr_type(nla));
+        return -1;
+    }
+
+    if (nl_actions_len == 0) {
+        VLOG_DBG_RL(&error_rl,
+                    "Unsupported action type drop");
+        return -1;
+    }
+
+    add_flow_action(actions, RTE_FLOW_ACTION_TYPE_END, NULL);
+    return 0;
+}
+
+static struct rte_flow *
+netdev_offload_dpdk_actions(struct netdev *netdev,
+                            struct flow_patterns *patterns,
+                            struct nlattr *nl_actions,
+                            size_t actions_len,
+                            struct offload_info *info)
+{
+    const struct rte_flow_attr flow_attr = { .ingress = 1, .transfer = 1 };
+    struct flow_actions actions = { .actions = NULL, .cnt = 0 };
+    struct rte_flow *flow = NULL;
+    struct rte_flow_error error;
+    int ret;
+
+    ret = parse_flow_actions(netdev, &actions, nl_actions, actions_len, info);
+    if (ret) {
+        goto out;
+    }
+    flow = netdev_offload_dpdk_flow_create(netdev, &flow_attr, patterns->items,
+                                           actions.actions, &error);
+    if (!flow) {
+        VLOG_ERR("%s: Failed to create flow: %s (%u)\n",
+                 netdev_get_name(netdev), error.message, error.type);
+    }
+out:
+    free_flow_actions(&actions);
+    return flow;
+}
+
 static int
 netdev_offload_dpdk_add_flow(struct netdev *netdev,
                              const struct match *match,
@@ -699,16 +782,8 @@  netdev_offload_dpdk_add_flow(struct netdev *netdev,
                              const ovs_u128 *ufid,
                              struct offload_info *info)
 {
-    const struct rte_flow_attr flow_attr = {
-        .group = 0,
-        .priority = 0,
-        .ingress = 1,
-        .egress = 0
-    };
     struct flow_patterns patterns = { .items = NULL, .cnt = 0 };
-    struct flow_actions actions = { .actions = NULL, .cnt = 0 };
     struct rte_flow *flow;
-    struct rte_flow_error error;
     int ret = 0;
 
     ret = parse_flow_match(&patterns, match);
@@ -716,10 +791,14 @@  netdev_offload_dpdk_add_flow(struct netdev *netdev,
         goto out;
     }
 
-    add_flow_mark_rss_actions(&actions, info->flow_mark, netdev);
-
-    flow = netdev_offload_dpdk_flow_create(netdev, &flow_attr, patterns.items,
-                                           actions.actions, &error);
+    flow = netdev_offload_dpdk_actions(netdev, &patterns, nl_actions,
+                                       actions_len, info);
+    if (!flow) {
+        /* if we failed to offload the rule actions fallback to mark rss
+         * actions.
+         */
+        flow = netdev_offload_dpdk_mark_rss(&patterns, netdev, info->flow_mark);
+    }
 
     if (!flow) {
         ret = -1;
@@ -731,7 +810,6 @@  netdev_offload_dpdk_add_flow(struct netdev *netdev,
 
 out:
     free_flow_patterns(&patterns);
-    free_flow_actions(&actions);
     return ret;
 }