On 2/24/2021 9:52 AM, Sriharsha Basavapatna wrote:
On Wed, Feb 10, 2021 at 8:57 PM Eli Britstein <[email protected]> wrote:
Support tunnel pop action.

Signed-off-by: Eli Britstein <[email protected]>
Reviewed-by: Gaetan Rivet <[email protected]>
---
  Documentation/howto/dpdk.rst |   1 +
  NEWS                         |   1 +
  lib/netdev-offload-dpdk.c    | 173 ++++++++++++++++++++++++++++++++---
  3 files changed, 160 insertions(+), 15 deletions(-)

diff --git a/Documentation/howto/dpdk.rst b/Documentation/howto/dpdk.rst
index f0d45e47b..4918d80f3 100644
--- a/Documentation/howto/dpdk.rst
+++ b/Documentation/howto/dpdk.rst
@@ -398,6 +398,7 @@ Supported actions for hardware offload are:
  - VLAN Push/Pop (push_vlan/pop_vlan).
  - Modification of IPv6 (set_field:<ADDR>->ipv6_src/ipv6_dst/mod_nw_ttl).
  - Clone/output (tnl_push and output) for encapsulating over a tunnel.
+- Tunnel pop, for changing from a physical port to a vport.

  Further Reading
  ---------------
diff --git a/NEWS b/NEWS
index a7bffce97..6850d5621 100644
--- a/NEWS
+++ b/NEWS
@@ -26,6 +26,7 @@ v2.15.0 - xx xxx xxxx
     - DPDK:
       * Removed support for vhost-user dequeue zero-copy.
       * Add support for DPDK 20.11.
+     * Add hardware offload support for tunnel pop action (experimental).
     - Userspace datapath:
       * Add the 'pmd' option to "ovs-appctl dpctl/dump-flows", which
         restricts a flow dump to a single PMD thread if set.
diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
index 78f866080..493cc9159 100644
--- a/lib/netdev-offload-dpdk.c
+++ b/lib/netdev-offload-dpdk.c
@@ -140,15 +140,30 @@ struct flow_actions {
      struct rte_flow_action *actions;
      int cnt;
      int current_max;
+    struct netdev *tnl_netdev;
+    /* tnl_actions is the opaque array of actions returned by the PMD. */
+    struct rte_flow_action *tnl_actions;
Why is this an opaque array ? Since it is struct rte_flow_action, OVS
knows the type and members. Is it opaque because the value of
rte_flow_action_type member is unknown to OVS ? Is it a private action
type and if so how does the PMD ensure that it doesn't conflict with
standard action types ?

True it is not used by OVS, but that's not why it opaque. Although it is struct rte_flow_action array, the PMD may use its own private actions, not defined in rte_flow.h, thus not known to the application.

The details of this array is under the PMD's responsibility.


+    uint32_t num_of_tnl_actions;
+    /* tnl_actions_pos is where the tunnel actions starts within the 'actions'
+     * field.
+     */
+    int tnl_actions_pos;
Names should indicate that they are private or pmd specific ?

tnl_actions --> tnl_private_actions or tnl_pmd_actions
num_of_tnl_actions --> num_private_tnl_actions or num_pmd_tnl_actions
tnl_actions_pos --> tnl_private_actions_pos or tnl_pmd_actions_pos
OK. _pmd_

+    struct ds s_tnl;
  };

  static void
-dump_flow_attr(struct ds *s, const struct rte_flow_attr *attr)
+dump_flow_attr(struct ds *s, struct ds *s_extra,
+               const struct rte_flow_attr *attr,
+               struct flow_actions *flow_actions)
  {
-    ds_put_format(s, "%s%spriority %"PRIu32" group %"PRIu32" %s",
+    if (flow_actions->num_of_tnl_actions) {
+        ds_clone(s_extra, &flow_actions->s_tnl);
+    }
+    ds_put_format(s, "%s%spriority %"PRIu32" group %"PRIu32" %s%s",
                    attr->ingress  ? "ingress " : "",
                    attr->egress   ? "egress " : "", attr->priority, 
attr->group,
-                  attr->transfer ? "transfer " : "");
+                  attr->transfer ? "transfer " : "",
+                  flow_actions->num_of_tnl_actions ? "tunnel_set 1 " : "");
  }

  /* Adds one pattern item 'field' with the 'mask' to dynamic string 's' using
@@ -395,9 +410,19 @@ dump_vxlan_encap(struct ds *s, const struct rte_flow_item 
*items)

  static void
  dump_flow_action(struct ds *s, struct ds *s_extra,
-                 const struct rte_flow_action *actions)
+                 struct flow_actions *flow_actions, int act_index)
  {
-    if (actions->type == RTE_FLOW_ACTION_TYPE_MARK) {
+    const struct rte_flow_action *actions = &flow_actions->actions[act_index];
+
+    if (actions->type == RTE_FLOW_ACTION_TYPE_END) {
+        ds_put_cstr(s, "end");
+    } else if (flow_actions->num_of_tnl_actions &&
+               act_index >= flow_actions->tnl_actions_pos &&
+               act_index < flow_actions->tnl_actions_pos +
+                           flow_actions->num_of_tnl_actions) {
+        /* Opaque PMD tunnel actions is skipped. */
Wouldn't it be useful to at least print the value of PMD action types ?
The only way we can print them as raw numbers. I see little added value for this on one hand, but on the other hand it will defect the testpmd format print, so I think better to avoid.

+        return;
+    } else if (actions->type == RTE_FLOW_ACTION_TYPE_MARK) {
          const struct rte_flow_action_mark *mark = actions->conf;

          ds_put_cstr(s, "mark ");
@@ -528,6 +553,14 @@ dump_flow_action(struct ds *s, struct ds *s_extra,
          ds_put_cstr(s, "vxlan_encap / ");
          dump_vxlan_encap(s_extra, items);
          ds_put_cstr(s_extra, ";");
+    } else if (actions->type == RTE_FLOW_ACTION_TYPE_JUMP) {
+        const struct rte_flow_action_jump *jump = actions->conf;
+
+        ds_put_cstr(s, "jump ");
+        if (jump) {
+            ds_put_format(s, "group %"PRIu32" ", jump->group);
+        }
+        ds_put_cstr(s, "/ ");
      } else {
          ds_put_format(s, "unknown rte flow action (%d)\n", actions->type);
      }
@@ -537,20 +570,21 @@ static struct ds *
  dump_flow(struct ds *s, struct ds *s_extra,
            const struct rte_flow_attr *attr,
            const struct rte_flow_item *items,
-          const struct rte_flow_action *actions)
+          struct flow_actions *flow_actions)
  {
+    int i;
+
      if (attr) {
-        dump_flow_attr(s, attr);
+        dump_flow_attr(s, s_extra, attr, flow_actions);
      }
      ds_put_cstr(s, "pattern ");
      while (items && items->type != RTE_FLOW_ITEM_TYPE_END) {
          dump_flow_pattern(s, items++);
      }
      ds_put_cstr(s, "end actions ");
-    while (actions && actions->type != RTE_FLOW_ACTION_TYPE_END) {
-        dump_flow_action(s, s_extra, actions++);
+    for (i = 0; i < flow_actions->cnt; i++) {
+        dump_flow_action(s, s_extra, flow_actions, i);
      }
-    ds_put_cstr(s, "end");
      return s;
  }

@@ -558,9 +592,10 @@ static struct rte_flow *
  netdev_offload_dpdk_flow_create(struct netdev *netdev,
                                  const struct rte_flow_attr *attr,
                                  const struct rte_flow_item *items,
-                                const struct rte_flow_action *actions,
+                                struct flow_actions *flow_actions,
                                  struct rte_flow_error *error)
  {
+    const struct rte_flow_action *actions = flow_actions->actions;
      struct ds s_extra = DS_EMPTY_INITIALIZER;
      struct ds s = DS_EMPTY_INITIALIZER;
      struct rte_flow *flow;
@@ -569,7 +604,7 @@ netdev_offload_dpdk_flow_create(struct netdev *netdev,
      flow = netdev_dpdk_rte_flow_create(netdev, attr, items, actions, error);
      if (flow) {
          if (!VLOG_DROP_DBG(&rl)) {
-            dump_flow(&s, &s_extra, attr, items, actions);
+            dump_flow(&s, &s_extra, attr, items, flow_actions);
              extra_str = ds_cstr(&s_extra);
              VLOG_DBG_RL(&rl, "%s: rte_flow 0x%"PRIxPTR" %s  flow create %d 
%s",
                          netdev_get_name(netdev), (intptr_t) flow, extra_str,
@@ -584,7 +619,7 @@ netdev_offload_dpdk_flow_create(struct netdev *netdev,
          VLOG_RL(&rl, level, "%s: rte_flow creation failed: %d (%s).",
                  netdev_get_name(netdev), error->type, error->message);
          if (!vlog_should_drop(&this_module, level, &rl)) {
-            dump_flow(&s, &s_extra, attr, items, actions);
+            dump_flow(&s, &s_extra, attr, items, flow_actions);
              extra_str = ds_cstr(&s_extra);
              VLOG_RL(&rl, level, "%s: Failed flow: %s  flow create %d %s",
                      netdev_get_name(netdev), extra_str,
@@ -640,6 +675,23 @@ add_flow_action(struct flow_actions *actions, enum 
rte_flow_action_type type,
      actions->cnt++;
  }

+static void
+add_flow_tnl_actions(struct flow_actions *actions,
+                     struct netdev *tnl_netdev,
+                     struct rte_flow_action *tnl_actions,
+                     uint32_t num_of_tnl_actions)
+{
+    int i;
+
+    actions->tnl_netdev = tnl_netdev;
+    actions->tnl_actions_pos = actions->cnt;
+    actions->tnl_actions = tnl_actions;
+    actions->num_of_tnl_actions = num_of_tnl_actions;
+    for (i = 0; i < num_of_tnl_actions; i++) {
+        add_flow_action(actions, tnl_actions[i].type, tnl_actions[i].conf);
+    }
+}
+
  static void
  free_flow_patterns(struct flow_patterns *patterns)
  {
@@ -661,9 +713,23 @@ free_flow_patterns(struct flow_patterns *patterns)
  static void
  free_flow_actions(struct flow_actions *actions)
  {
+    struct rte_flow_error error;
      int i;

      for (i = 0; i < actions->cnt; i++) {
+        if (actions->num_of_tnl_actions && i == actions->tnl_actions_pos) {
+            if (netdev_dpdk_rte_flow_tunnel_action_decap_release
+                    (actions->tnl_netdev, actions->tnl_actions,
+                     actions->num_of_tnl_actions, &error)) {
+                VLOG_DBG_RL(&rl, "%s: "
+                            "netdev_dpdk_rte_flow_tunnel_action_decap_release "
+                            "failed: %d (%s).",
+                            netdev_get_name(actions->tnl_netdev),
+                            error.type, error.message);
+            }
+            i += actions->num_of_tnl_actions - 1;
+            continue;
+        }
          if (actions->actions[i].conf) {
              free(CONST_CAST(void *, actions->actions[i].conf));
          }
@@ -671,6 +737,7 @@ free_flow_actions(struct flow_actions *actions)
      free(actions->actions);
      actions->actions = NULL;
      actions->cnt = 0;
+    ds_destroy(&actions->s_tnl);
  }

  static int
@@ -960,7 +1027,7 @@ netdev_offload_dpdk_mark_rss(struct flow_patterns 
*patterns,
      add_flow_mark_rss_actions(&actions, flow_mark, netdev);

      flow = netdev_offload_dpdk_flow_create(netdev, &flow_attr, 
patterns->items,
-                                           actions.actions, &error);
+                                           &actions, &error);

      free_flow_actions(&actions);
      return flow;
@@ -1307,6 +1374,78 @@ parse_clone_actions(struct netdev *netdev,
      return 0;
  }

+static void
+add_jump_action(struct flow_actions *actions, uint32_t group)
+{
+    struct rte_flow_action_jump *jump = xzalloc (sizeof *jump);
+
+    jump->group = group;
+    add_flow_action(actions, RTE_FLOW_ACTION_TYPE_JUMP, jump);
+}
+
+static int
+vport_to_rte_tunnel(struct netdev *vport,
+                    struct rte_flow_tunnel *tunnel,
+                    struct netdev *netdev,
+                    struct ds *s_tnl)
+{
+    const struct netdev_tunnel_config *tnl_cfg;
+
+    memset(tunnel, 0, sizeof *tunnel);
+    if (!strcmp(netdev_get_type(vport), "vxlan")) {
+        tunnel->type = RTE_FLOW_ITEM_TYPE_VXLAN;
+        tnl_cfg = netdev_get_tunnel_config(vport);
+        if (!tnl_cfg) {
+            return -1;
+        }
+        tunnel->tp_dst = tnl_cfg->dst_port;
+        if (!VLOG_DROP_DBG(&rl)) {
+            ds_put_format(s_tnl, "flow tunnel create %d type vxlan; ",
+                          netdev_dpdk_get_port_id(netdev));
+        }
+    } else {
+        OVS_NOT_REACHED();
+    }
+
+    return 0;
+}
+
+static int
+add_tnl_pop_action(struct netdev *netdev,
+                   struct flow_actions *actions,
+                   const struct nlattr *nla)
+{
+    struct rte_flow_action *tnl_actions = NULL;
+    uint32_t num_of_tnl_actions = 0;
+    struct rte_flow_tunnel tunnel;
+    struct rte_flow_error error;
+    struct netdev *vport;
+    odp_port_t port;
+    int ret;
+
+    port = nl_attr_get_odp_port(nla);
+    vport = netdev_ports_get(port, netdev->dpif_type);
+    if (vport == NULL) {
+        return -1;
+    }
+    ret = vport_to_rte_tunnel(vport, &tunnel, netdev, &actions->s_tnl);
+    netdev_close(vport);
+    if (ret) {
+        return ret;
+    }
+    ret = netdev_dpdk_rte_flow_tunnel_decap_set(netdev, &tunnel, &tnl_actions,
+                                                &num_of_tnl_actions, &error);
+    if (ret) {
+        VLOG_DBG_RL(&rl, "%s: netdev_dpdk_rte_flow_tunnel_decap_set failed: "
+                    "%d (%s).", netdev_get_name(netdev), error.type,
+                    error.message);
+        return ret;
+    }
+    add_flow_tnl_actions(actions, netdev, tnl_actions, num_of_tnl_actions);
+    add_jump_action(actions, 0);
It is not clear why jump action is added and with a group value of 0 ?

This is equivalent to dp_netdev_recirculate, when the group represents the recirc_id that is kept 0.

I will add a comment.


Thanks,
-Harsha
+    return 0;
+}
+
  static int
  parse_flow_actions(struct netdev *netdev,
                     struct flow_actions *actions,
@@ -1351,6 +1490,10 @@ parse_flow_actions(struct netdev *netdev,
                                      clone_actions_len)) {
                  return -1;
              }
+        } else if (nl_attr_type(nla) == OVS_ACTION_ATTR_TUNNEL_POP) {
+            if (add_tnl_pop_action(netdev, actions, nla)) {
+                return -1;
+            }
          } else {
              VLOG_DBG_RL(&rl, "Unsupported action type %d", nl_attr_type(nla));
              return -1;
@@ -1383,7 +1526,7 @@ netdev_offload_dpdk_actions(struct netdev *netdev,
          goto out;
      }
      flow = netdev_offload_dpdk_flow_create(netdev, &flow_attr, 
patterns->items,
-                                           actions.actions, &error);
+                                           &actions, &error);
  out:
      free_flow_actions(&actions);
      return flow;
--
2.28.0.546.g385c171

_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to