On 2/24/2021 12:37 PM, Sriharsha Basavapatna wrote:
On Wed, Feb 10, 2021 at 8:57 PM Eli Britstein <[email protected]> wrote:
Vports are virtual, OVS only logical devices, so rte_flows cannot be
applied as is on them. Instead, apply the rules the physical port from
which the packet has arrived, provided by orig_in_port field.

Signed-off-by: Eli Britstein <[email protected]>
Reviewed-by: Gaetan Rivet <[email protected]>
---
  lib/netdev-offload-dpdk.c | 169 ++++++++++++++++++++++++++++++--------
  1 file changed, 137 insertions(+), 32 deletions(-)

diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
index f6e668bff..ad47d717f 100644
--- a/lib/netdev-offload-dpdk.c
+++ b/lib/netdev-offload-dpdk.c
@@ -62,6 +62,7 @@ struct ufid_to_rte_flow_data {
      struct rte_flow *rte_flow;
      bool actions_offloaded;
      struct dpif_flow_stats stats;
+    struct netdev *physdev;
  };

  /* Find rte_flow with @ufid. */
@@ -87,7 +88,8 @@ ufid_to_rte_flow_data_find(const ovs_u128 *ufid, bool warn)

  static inline struct ufid_to_rte_flow_data *
  ufid_to_rte_flow_associate(const ovs_u128 *ufid, struct netdev *netdev,
-                           struct rte_flow *rte_flow, bool actions_offloaded)
+                           struct netdev *vport, struct rte_flow *rte_flow,
+                           bool actions_offloaded)
  {
      size_t hash = hash_bytes(ufid, sizeof *ufid, 0);
      struct ufid_to_rte_flow_data *data = xzalloc(sizeof *data);
@@ -105,7 +107,8 @@ ufid_to_rte_flow_associate(const ovs_u128 *ufid, struct 
netdev *netdev,
      }

      data->ufid = *ufid;
-    data->netdev = netdev_ref(netdev);
+    data->netdev = vport ? netdev_ref(vport) : netdev_ref(netdev);
+    data->physdev = netdev_ref(netdev);
For non-tunnel offloads, we end up getting two references to the same
'netdev'; can we avoid this ? That is, get a reference to physdev only
for the vport case.
I know. This is on purpose. It simplifies other places, for example query, to always use physdev, and always close both without any additional logic there.
      data->rte_flow = rte_flow;
      data->actions_offloaded = actions_offloaded;

@@ -122,6 +125,7 @@ ufid_to_rte_flow_disassociate(struct ufid_to_rte_flow_data 
*data)
      cmap_remove(&ufid_to_rte_flow,
                  CONST_CAST(struct cmap_node *, &data->node), hash);
      netdev_close(data->netdev);
+    netdev_close(data->physdev);
Similar comment, release reference to physdev only if we got a
reference earlier (i.e., physdev should be non-null only when netdev
is a vport).
Right. As it is written this way, no need for any additional logic here.
      ovsrcu_postpone(free, data);
  }

@@ -134,6 +138,8 @@ struct flow_patterns {
      struct rte_flow_item *items;
      int cnt;
      int current_max;
+    uint32_t num_of_tnl_items;
change to --> num_pmd_tnl_items
OK.
+    struct ds s_tnl;
  };

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

  /* Adds one pattern item 'field' with the 'mask' to dynamic string 's' using
@@ -177,9 +187,18 @@ dump_flow_attr(struct ds *s, struct ds *s_extra,
      }

  static void
-dump_flow_pattern(struct ds *s, const struct rte_flow_item *item)
+dump_flow_pattern(struct ds *s,
+                  struct flow_patterns *flow_patterns,
+                  int pattern_index)
  {
-    if (item->type == RTE_FLOW_ITEM_TYPE_ETH) {
+    const struct rte_flow_item *item = &flow_patterns->items[pattern_index];
+
+    if (item->type == RTE_FLOW_ITEM_TYPE_END) {
+        ds_put_cstr(s, "end ");
+    } else if (flow_patterns->num_of_tnl_items &&
+               pattern_index < flow_patterns->num_of_tnl_items) {
+        return;
+    } else if (item->type == RTE_FLOW_ITEM_TYPE_ETH) {
          const struct rte_flow_item_eth *eth_spec = item->spec;
          const struct rte_flow_item_eth *eth_mask = item->mask;

@@ -569,19 +588,19 @@ dump_flow_action(struct ds *s, struct ds *s_extra,
  static struct ds *
  dump_flow(struct ds *s, struct ds *s_extra,
            const struct rte_flow_attr *attr,
-          const struct rte_flow_item *items,
+          struct flow_patterns *flow_patterns,
            struct flow_actions *flow_actions)
  {
      int i;

      if (attr) {
-        dump_flow_attr(s, s_extra, attr, flow_actions);
+        dump_flow_attr(s, s_extra, attr, flow_patterns, flow_actions);
      }
      ds_put_cstr(s, "pattern ");
-    while (items && items->type != RTE_FLOW_ITEM_TYPE_END) {
-        dump_flow_pattern(s, items++);
+    for (i = 0; i < flow_patterns->cnt; i++) {
+        dump_flow_pattern(s, flow_patterns, i);
      }
-    ds_put_cstr(s, "end actions ");
+    ds_put_cstr(s, "actions ");
      for (i = 0; i < flow_actions->cnt; i++) {
          dump_flow_action(s, s_extra, flow_actions, i);
      }
@@ -591,11 +610,12 @@ dump_flow(struct ds *s, struct ds *s_extra,
  static struct rte_flow *
  netdev_offload_dpdk_flow_create(struct netdev *netdev,
                                  const struct rte_flow_attr *attr,
-                                const struct rte_flow_item *items,
+                                struct flow_patterns *flow_patterns,
                                  struct flow_actions *flow_actions,
                                  struct rte_flow_error *error)
  {
      const struct rte_flow_action *actions = flow_actions->actions;
+    const struct rte_flow_item *items = flow_patterns->items;
      struct ds s_extra = DS_EMPTY_INITIALIZER;
      struct ds s = DS_EMPTY_INITIALIZER;
      struct rte_flow *flow;
@@ -604,7 +624,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, flow_actions);
+            dump_flow(&s, &s_extra, attr, flow_patterns, 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,
@@ -619,7 +639,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, flow_actions);
+            dump_flow(&s, &s_extra, attr, flow_patterns, 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,
@@ -654,6 +674,28 @@ add_flow_pattern(struct flow_patterns *patterns, enum 
rte_flow_item_type type,
      patterns->cnt++;
  }

+static void
+add_flow_tnl_patterns(struct flow_patterns *all_patterns,
+                      struct rte_flow_item *tnl_items,
--> pmd_tnl_items
+                      uint32_t num_of_tnl_items,
--> num_pmd_tnl_items
+                      struct flow_patterns *flow_patterns)
+{
+    int i;
+
+    all_patterns->num_of_tnl_items = num_of_tnl_items;
+
+    for (i = 0; i < num_of_tnl_items; i++) {
+        add_flow_pattern(all_patterns, tnl_items[i].type, tnl_items[i].spec,
+                         tnl_items[i].mask);
+    }
+
+    for (i = 0; i < flow_patterns->cnt; i++) {
+        add_flow_pattern(all_patterns, flow_patterns->items[i].type,
+                         flow_patterns->items[i].spec,
+                         flow_patterns->items[i].mask);
+    }
+}
+
  static void
  add_flow_action(struct flow_actions *actions, enum rte_flow_action_type type,
                  const void *conf)
@@ -1487,6 +1529,7 @@ parse_flow_actions(struct netdev *netdev,

  static struct ufid_to_rte_flow_data *
  create_netdev_offload(struct netdev *netdev,
+                      struct netdev *vport,
                        const ovs_u128 *ufid,
                        struct flow_patterns *flow_patterns,
                        struct flow_actions *flow_actions,
@@ -1495,32 +1538,34 @@ create_netdev_offload(struct netdev *netdev,
  {
      struct rte_flow_attr flow_attr = { .ingress = 1, .transfer = 1, };
      struct flow_actions rss_actions = { .actions = NULL, .cnt = 0 };
-    struct rte_flow_item *items = flow_patterns->items;
      struct ufid_to_rte_flow_data *flow_data = NULL;
      bool actions_offloaded = true;
      struct rte_flow *flow = NULL;
      struct rte_flow_error error;

      if (enable_full) {
-        flow = netdev_offload_dpdk_flow_create(netdev, &flow_attr, items,
-                                               flow_actions, &error);
+        flow = netdev_offload_dpdk_flow_create(netdev, &flow_attr,
+                                               flow_patterns, flow_actions,
+                                               &error);
      }

-    if (!flow) {
+    if (!vport && !flow) {
          /* If we failed to offload the rule actions fallback to MARK+RSS
           * actions.
           */
          actions_offloaded = false;
          flow_attr.transfer = 0;
          add_flow_mark_rss_actions(&rss_actions, flow_mark, netdev);
-        flow = netdev_offload_dpdk_flow_create(netdev, &flow_attr, items,
-                                               &rss_actions, &error);
+        flow = netdev_offload_dpdk_flow_create(netdev, &flow_attr,
+                                               flow_patterns, &rss_actions,
+                                               &error);
      }

      if (flow) {
-        flow_data = ufid_to_rte_flow_associate(ufid, netdev, flow,
+        flow_data = ufid_to_rte_flow_associate(ufid, netdev, vport, flow,
                                                 actions_offloaded);
-        VLOG_DBG("%s: installed flow %p by ufid "UUID_FMT,
+        VLOG_DBG("%s/%s: installed flow %p by ufid "UUID_FMT,
+                 vport ? netdev_get_name(vport) : netdev_get_name(netdev),
                   netdev_get_name(netdev), flow,
                   UUID_ARGS((struct uuid *) ufid));
      }
@@ -1529,6 +1574,55 @@ create_netdev_offload(struct netdev *netdev,
      return flow_data;
  }

+static struct ufid_to_rte_flow_data *
+create_vport_offload(struct netdev *vport,
+                     odp_port_t orig_in_port,
+                     const ovs_u128 *ufid,
+                     struct flow_patterns *flow_patterns,
+                     struct flow_actions *flow_actions)
+{
+    struct flow_patterns all_patterns = { .items = NULL, .cnt = 0 };
+    struct ufid_to_rte_flow_data *flows_data = NULL;
+    struct rte_flow_item *tnl_items;
+    struct rte_flow_tunnel tunnel;
+    struct rte_flow_error error;
+    uint32_t num_of_tnl_items;
+    struct netdev *physdev;
+
+    physdev = netdev_ports_get(orig_in_port, vport->dpif_type);
+    if (physdev == NULL) {
+        return NULL;
+    }
+
+    if (vport_to_rte_tunnel(vport, &tunnel, physdev,
+                            &all_patterns.s_tnl)) {
+        goto out;
+    }
+    if (netdev_dpdk_rte_flow_tunnel_match(physdev, &tunnel, &tnl_items,
+                                          &num_of_tnl_items, &error)) {
+        VLOG_DBG_RL(&rl, "%s: netdev_dpdk_rte_flow_tunnel_match failed: "
+                    "%d (%s).", netdev_get_name(physdev), error.type,
+                    error.message);
+        goto out;
+    }
+    add_flow_tnl_patterns(&all_patterns, tnl_items, num_of_tnl_items,
+                          flow_patterns);
+    flows_data = create_netdev_offload(physdev, vport, ufid, &all_patterns,
+                                       flow_actions, true, 0);
+    if (netdev_dpdk_rte_flow_tunnel_item_release(physdev, tnl_items,
+                                                 num_of_tnl_items,
+                                                 &error)) {
+        VLOG_DBG_RL(&rl, "%s: netdev_dpdk_rte_flow_tunnel_item_release "
+                    "failed: %d (%s).", netdev_get_name(physdev),
+                    error.type, error.message);
+    }
+out:
+    all_patterns.cnt = 0;
+    free_flow_patterns(&all_patterns);
+    netdev_close(physdev);
+    return flows_data;
+}
+
  static struct ufid_to_rte_flow_data *
  netdev_offload_dpdk_add_flow(struct netdev *netdev,
                               struct match *match,
@@ -1550,8 +1644,14 @@ netdev_offload_dpdk_add_flow(struct netdev *netdev,

      err = parse_flow_actions(netdev, &actions, nl_actions, actions_len);
For the vport offload (flow-F2), we should add a VXLAN_DECAP action to
the action list. Otherwise, there is no action in either F1 or F2 that
tells the PMD to pop the tunnel header.

     if (!strcmp(netdev_get_type(netdev), "vxlan")) {
         add_flow_action(actions, RTE_FLOW_ACTION_TYPE_VXLAN_DECAP, NULL);
     }

Thanks,
-Harsha
-    flow_data = create_netdev_offload(netdev, ufid, &patterns, &actions, !err,
-                                      info->flow_mark);
+    if (netdev_vport_is_vport_class(netdev->netdev_class)) {
+        flow_data = err ? NULL :
+            create_vport_offload(netdev, info->orig_in_port, ufid, &patterns,
+                                 &actions);
+    } else {
+        flow_data = create_netdev_offload(netdev, NULL, ufid, &patterns,
+                                           &actions, !err, info->flow_mark);
+    }

  out:
      free_flow_patterns(&patterns);
@@ -1564,26 +1664,30 @@ netdev_offload_dpdk_flow_destroy(struct 
ufid_to_rte_flow_data *rte_flow_data)
  {
      struct rte_flow_error error;
      struct rte_flow *rte_flow;
+    struct netdev *physdev;
      struct netdev *netdev;
      ovs_u128 *ufid;
      int ret;

      rte_flow = rte_flow_data->rte_flow;
+    physdev = rte_flow_data->physdev;
      netdev = rte_flow_data->netdev;
      ufid = &rte_flow_data->ufid;

-    ret = netdev_dpdk_rte_flow_destroy(netdev, rte_flow, &error);
+    ret = netdev_dpdk_rte_flow_destroy(physdev, rte_flow, &error);

      if (ret == 0) {
          ufid_to_rte_flow_disassociate(rte_flow_data);
-        VLOG_DBG_RL(&rl, "%s: rte_flow 0x%"PRIxPTR
+        VLOG_DBG_RL(&rl, "%s/%s: rte_flow 0x%"PRIxPTR
                      " flow destroy %d ufid " UUID_FMT,
-                    netdev_get_name(netdev), (intptr_t) rte_flow,
+                    netdev_get_name(netdev), netdev_get_name(physdev),
+                    (intptr_t) rte_flow,
                      netdev_dpdk_get_port_id(netdev),
                      UUID_ARGS((struct uuid *) ufid));
      } else {
-        VLOG_ERR("Failed flow: %s: flow destroy %d ufid " UUID_FMT,
-                 netdev_get_name(netdev), netdev_dpdk_get_port_id(netdev),
+        VLOG_ERR("Failed flow: %s/%s: flow destroy %d ufid " UUID_FMT,
+                 netdev_get_name(netdev), netdev_get_name(physdev),
+                 netdev_dpdk_get_port_id(netdev),
                   UUID_ARGS((struct uuid *) ufid));
      }

@@ -1688,8 +1792,9 @@ netdev_offload_dpdk_flow_get(struct netdev *netdev,
          goto out;
      }
      attrs->dp_layer = "dpdk";
-    ret = netdev_dpdk_rte_flow_query_count(netdev, rte_flow_data->rte_flow,
-                                           &query, &error);
+    ret = netdev_dpdk_rte_flow_query_count(rte_flow_data->physdev,
+                                           rte_flow_data->rte_flow, &query,
+                                           &error);
      if (ret) {
          VLOG_DBG_RL(&rl, "%s: Failed to query ufid "UUID_FMT" flow: %p",
                      netdev_get_name(netdev), UUID_ARGS((struct uuid *) ufid),
@@ -1713,7 +1818,7 @@ netdev_offload_dpdk_flow_flush(struct netdev *netdev)
      struct ufid_to_rte_flow_data *data;

      CMAP_FOR_EACH (data, node, &ufid_to_rte_flow) {
-        if (data->netdev != netdev) {
+        if (data->netdev != netdev && data->physdev != netdev) {
              continue;
          }

--
2.28.0.546.g385c171

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

Reply via email to