Re: [ovs-dev] [PATCH V3 01/12] netdev-offload-dpdk: Remove pre-validate of patterns function

Mon, 29 Jun 2020 00:45:46 -0700 


On 6/29/2020 1:01 AM, Ilya Maximets wrote:

On 6/21/20 1:19 PM, Eli Britstein wrote:

The function of adding patterns by requested matches checks that it
consumed all the required matches, and err if not. This nullify the
purpose of the validation function. Future supported matches will only
change the pattern parsing code.

Signed-off-by: Eli Britstein <[email protected]>
Reviewed-by: Roni Bar Yanai <[email protected]>
---
  lib/netdev-offload-dpdk.c | 122 ++++++++++++++++------------------------------
  1 file changed, 43 insertions(+), 79 deletions(-)

diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
index f8c46bbaa..2f1b42bf7 100644
--- a/lib/netdev-offload-dpdk.c
+++ b/lib/netdev-offload-dpdk.c
@@ -559,11 +559,21 @@ free_flow_actions(struct flow_actions *actions)

  
  static int

  parse_flow_match(struct flow_patterns *patterns,
-                 const struct match *match)
+                 struct match *match)
  {
      uint8_t *next_proto_mask = NULL;
+    struct flow *consumed_masks;
      uint8_t proto = 0;

  
+    consumed_masks = &match->wc.masks;

+
+    memset(&consumed_masks->in_port, 0, sizeof consumed_masks->in_port);
+    if (match->flow.recirc_id != 0) {

This is not fully correct since we may not be requested to match on it.
Original validation function checks against match_zero_wc which has
all wildcarded fields zeroed.

OK.



+        return -1;
+    }
+    consumed_masks->recirc_id = 0;
+    consumed_masks->packet_type = 0;
+
      /* Eth */
      if (!eth_addr_is_zero(match->wc.masks.dl_src) ||
          !eth_addr_is_zero(match->wc.masks.dl_dst)) {
@@ -580,6 +590,9 @@ parse_flow_match(struct flow_patterns *patterns,
          memcpy(&mask->src, &match->wc.masks.dl_src, sizeof mask->src);
          mask->type = match->wc.masks.dl_type;

  
+        memset(&consumed_masks->dl_dst, 0, sizeof consumed_masks->dl_dst);

+        memset(&consumed_masks->dl_src, 0, sizeof consumed_masks->dl_src);
+
          add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_ETH, spec, mask);
      } else {
          /*
@@ -591,6 +604,7 @@ parse_flow_match(struct flow_patterns *patterns,
           */
          add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_ETH, NULL, NULL);
      }
+    consumed_masks->dl_type = 0;

  
      /* VLAN */

      if (match->wc.masks.vlans[0].tci && match->flow.vlans[0].tci) {
@@ -607,6 +621,7 @@ parse_flow_match(struct flow_patterns *patterns,

  
          add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_VLAN, spec, mask);

      }
+    memset(&consumed_masks->vlans[0], 0, sizeof consumed_masks->vlans[0]);

I don't think 'qtag' is consumed here.  Also this clearing should be done
inside the if condition.



If you refer to 'qtag' field in union flow_vlan_hdr, so it is consumed 
as this is a union. If you refer to qinq, it was not consumed either 
before. I didn't change that.



Regarding the clearing, it should not be inside, but outside. See also 
in netdev-offload-tc.c. In case of native (untagged), 
match->wc.masks.vlans[0].tci is 0xFFFF and match->flow.vlans[0].tci is 0.





  
      /* IP v4 */

      if (match->flow.dl_type == htons(ETH_TYPE_IP)) {
@@ -627,6 +642,12 @@ parse_flow_match(struct flow_patterns *patterns,
          mask->hdr.src_addr        = match->wc.masks.nw_src;
          mask->hdr.dst_addr        = match->wc.masks.nw_dst;

  
+        consumed_masks->nw_tos = 0;

+        consumed_masks->nw_ttl = 0;
+        consumed_masks->nw_proto = 0;
+        consumed_masks->nw_src = 0;
+        consumed_masks->nw_dst = 0;
+
          add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_IPV4, spec, mask);

  
          /* Save proto for L4 protocol setup. */

@@ -634,6 +655,8 @@ parse_flow_match(struct flow_patterns *patterns,
                  mask->hdr.next_proto_id;
          next_proto_mask = &mask->hdr.next_proto_id;
      }
+    /* ignore mask match for now */
+    consumed_masks->nw_frag = 0;

Why this ignored?  Original validation code failed if matching on fragmentation
flags requested.

OK.



  
      if (proto != IPPROTO_ICMP && proto != IPPROTO_UDP  &&

          proto != IPPROTO_SCTP && proto != IPPROTO_TCP  &&
@@ -665,6 +688,10 @@ parse_flow_match(struct flow_patterns *patterns,
          mask->hdr.data_off  = ntohs(match->wc.masks.tcp_flags) >> 8;
          mask->hdr.tcp_flags = ntohs(match->wc.masks.tcp_flags) & 0xff;

  
+        consumed_masks->tp_src = 0;

+        consumed_masks->tp_dst = 0;
+        consumed_masks->tcp_flags = 0;
+
          add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_TCP, spec, mask);

  
          /* proto == TCP and ITEM_TYPE_TCP, thus no need for proto match. */

@@ -683,6 +710,9 @@ parse_flow_match(struct flow_patterns *patterns,
          mask->hdr.src_port = match->wc.masks.tp_src;
          mask->hdr.dst_port = match->wc.masks.tp_dst;

  
+        consumed_masks->tp_src = 0;

+        consumed_masks->tp_dst = 0;
+
          add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_UDP, spec, mask);

  
          /* proto == UDP and ITEM_TYPE_UDP, thus no need for proto match. */

@@ -701,6 +731,9 @@ parse_flow_match(struct flow_patterns *patterns,
          mask->hdr.src_port = match->wc.masks.tp_src;
          mask->hdr.dst_port = match->wc.masks.tp_dst;

  
+        consumed_masks->tp_src = 0;

+        consumed_masks->tp_dst = 0;
+
          add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_SCTP, spec, mask);

  
          /* proto == SCTP and ITEM_TYPE_SCTP, thus no need for proto match. */

@@ -719,6 +752,9 @@ parse_flow_match(struct flow_patterns *patterns,
          mask->hdr.icmp_type = (uint8_t) ntohs(match->wc.masks.tp_src);
          mask->hdr.icmp_code = (uint8_t) ntohs(match->wc.masks.tp_dst);

  
+        consumed_masks->tp_src = 0;

+        consumed_masks->tp_dst = 0;
+
          add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_ICMP, spec, mask);

  
          /* proto == ICMP and ITEM_TYPE_ICMP, thus no need for proto match. */

@@ -729,6 +765,9 @@ parse_flow_match(struct flow_patterns *patterns,

  
      add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_END, NULL, NULL);
  
+    if (!is_all_zeros(consumed_masks, sizeof *consumed_masks)) {

+        return -1;
+    }
      return 0;
  }

  
@@ -1039,7 +1078,7 @@ out:
  
  static int

  netdev_offload_dpdk_add_flow(struct netdev *netdev,
-                             const struct match *match,
+                             struct match *match,
                               struct nlattr *nl_actions,
                               size_t actions_len,
                               const ovs_u128 *ufid,
@@ -1052,6 +1091,8 @@ netdev_offload_dpdk_add_flow(struct netdev *netdev,

  
      ret = parse_flow_match(&patterns, match);

      if (ret) {
+        VLOG_DBG_RL(&rl, "%s: matches of ufid "UUID_FMT" are not supported\n",
+                    netdev_get_name(netdev), UUID_ARGS((struct uuid *)ufid));
          goto out;
      }

  
@@ -1079,78 +1120,6 @@ out:

      return ret;
  }

  
-/*

- * Check if any unsupported flow patterns are specified.
- */
-static int
-netdev_offload_dpdk_validate_flow(const struct match *match)
-{
-    struct match match_zero_wc;
-    const struct flow *masks = &match->wc.masks;
-
-    /* Create a wc-zeroed version of flow. */
-    match_init(&match_zero_wc, &match->flow, &match->wc);
-
-    if (!is_all_zeros(&match_zero_wc.flow.tunnel,
-                      sizeof match_zero_wc.flow.tunnel)) {
-        goto err;
-    }
-
-    if (masks->metadata || masks->skb_priority ||
-        masks->pkt_mark || masks->dp_hash) {
-        goto err;
-    }
-
-    /* recirc id must be zero. */
-    if (match_zero_wc.flow.recirc_id) {
-        goto err;
-    }
-
-    if (masks->ct_state || masks->ct_nw_proto ||
-        masks->ct_zone  || masks->ct_mark     ||
-        !ovs_u128_is_zero(masks->ct_label)) {
-        goto err;
-    }
-
-    if (masks->conj_id || masks->actset_output) {
-        goto err;
-    }
-
-    /* Unsupported L2. */
-    if (!is_all_zeros(masks->mpls_lse, sizeof masks->mpls_lse)) {
-        goto err;
-    }
-
-    /* Unsupported L3. */
-    if (masks->ipv6_label || masks->ct_nw_src || masks->ct_nw_dst     ||
-        !is_all_zeros(&masks->ipv6_src,    sizeof masks->ipv6_src)    ||
-        !is_all_zeros(&masks->ipv6_dst,    sizeof masks->ipv6_dst)    ||
-        !is_all_zeros(&masks->ct_ipv6_src, sizeof masks->ct_ipv6_src) ||
-        !is_all_zeros(&masks->ct_ipv6_dst, sizeof masks->ct_ipv6_dst) ||
-        !is_all_zeros(&masks->nd_target,   sizeof masks->nd_target)   ||
-        !is_all_zeros(&masks->nsh,         sizeof masks->nsh)         ||
-        !is_all_zeros(&masks->arp_sha,     sizeof masks->arp_sha)     ||
-        !is_all_zeros(&masks->arp_tha,     sizeof masks->arp_tha)) {
-        goto err;
-    }
-
-    /* If fragmented, then don't HW accelerate - for now. */
-    if (match_zero_wc.flow.nw_frag) {
-        goto err;
-    }
-
-    /* Unsupported L4. */
-    if (masks->igmp_group_ip4 || masks->ct_tp_src || masks->ct_tp_dst) {
-        goto err;
-    }
-
-    return 0;
-
-err:
-    VLOG_ERR("cannot HW accelerate this flow due to unsupported protocols");
-    return -1;
-}
-
  static int
  netdev_offload_dpdk_destroy_flow(struct netdev *netdev,
                                   const ovs_u128 *ufid,
@@ -1194,11 +1163,6 @@ netdev_offload_dpdk_flow_put(struct netdev *netdev, 
struct match *match,
          }
      }

  
-    ret = netdev_offload_dpdk_validate_flow(match);

-    if (ret < 0) {
-        return ret;
-    }
-
      if (stats) {
          memset(stats, 0, sizeof *stats);
      }


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























					Reply via email to