diff mbox series

[ovs-dev,01/11] netdev-offload-dpdk: Remove pre-validate of patterns function

Message ID 20200518154026.18059-2-elibr@mellanox.com
State Changes Requested
Headers show
Series netdev datapath offload: Support IPv6 and VXLAN encap | expand

Commit Message

Eli Britstein May 18, 2020, 3:40 p.m. UTC 
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 <elibr@mellanox.com>
Reviewed-by: Roni Bar Yanai <roniba@mellanox.com>
---
 lib/netdev-offload-dpdk.c | 133 ++++++++++++++++++----------------------------
 1 file changed, 51 insertions(+), 82 deletions(-)

Comments

Sriharsha Basavapatna May 19, 2020, 1:41 p.m. UTC | #1 
On Mon, May 18, 2020 at 9:10 PM Eli Britstein <elibr@mellanox.com> 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 <elibr@mellanox.com>
> Reviewed-by: Roni Bar Yanai <roniba@mellanox.com>
> ---
>  lib/netdev-offload-dpdk.c | 133 ++++++++++++++++++----------------------------
>  1 file changed, 51 insertions(+), 82 deletions(-)
>
> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
> index f8c46bbaa..84bbe29e7 100644
> --- a/lib/netdev-offload-dpdk.c
> +++ b/lib/netdev-offload-dpdk.c
> @@ -559,10 +559,22 @@ 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;
> +    int ret = 0;
> +
> +    consumed_masks = &match->wc.masks;
> +
> +    memset(&consumed_masks->in_port, 0, sizeof consumed_masks->in_port);
> +    if (match->flow.recirc_id != 0) {
> +        ret = -1;
> +        goto out;
> +    }
> +    consumed_masks->recirc_id = 0;
> +    consumed_masks->packet_type = 0;
>
>      /* Eth */
>      if (!eth_addr_is_zero(match->wc.masks.dl_src) ||
> @@ -580,6 +592,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 +606,7 @@ parse_flow_match(struct flow_patterns *patterns,
>           */
>          add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_ETH, NULL, NULL);
>      }
> +    consumed_masks->dl_type = 0;

Why not set the dl_type also under the if() condition above, where
dl_src an dl_dst consumed masks are being set ? Similarly, vlan
consumed mask below.

>
>      /* VLAN */
>      if (match->wc.masks.vlans[0].tci && match->flow.vlans[0].tci) {
> @@ -607,6 +623,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]);
>
>      /* IP v4 */
>      if (match->flow.dl_type == htons(ETH_TYPE_IP)) {
> @@ -627,6 +644,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 +657,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;
>
>      if (proto != IPPROTO_ICMP && proto != IPPROTO_UDP  &&
>          proto != IPPROTO_SCTP && proto != IPPROTO_TCP  &&
> @@ -641,12 +666,14 @@ parse_flow_match(struct flow_patterns *patterns,
>           match->wc.masks.tp_dst ||
>           match->wc.masks.tcp_flags)) {
>          VLOG_DBG("L4 Protocol (%u) not supported", proto);
> -        return -1;
> +        ret = -1;
> +        goto out;
>      }
>
>      if ((match->wc.masks.tp_src && match->wc.masks.tp_src != OVS_BE16_MAX) ||
>          (match->wc.masks.tp_dst && match->wc.masks.tp_dst != OVS_BE16_MAX)) {
> -        return -1;
> +        ret = -1;
> +        goto out;
>      }
>
>      if (proto == IPPROTO_TCP) {
> @@ -665,6 +692,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 +714,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 +735,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 +756,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,7 +769,11 @@ parse_flow_match(struct flow_patterns *patterns,
>
>      add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_END, NULL, NULL);
>
> -    return 0;
> +    if (!is_all_zeros(consumed_masks, sizeof *consumed_masks)) {
> +        ret = -1;
> +    }
> +out:
> +    return ret;

There's really no cleanup/error handling code under the label 'out'.
So, this label could be removed and all the above goto statements
replaced by just return -1. This also improves readability (avoid
scrolling down just to see a return statement).

Thanks,
-Harsha



>  }
>
>  static void
> @@ -1039,7 +1083,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 +1096,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 +1125,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 +1168,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);
>      }
> --
> 2.14.5
>
Eli Britstein May 19, 2020, 2:26 p.m. UTC | #2 
On 5/19/2020 4:41 PM, Sriharsha Basavapatna wrote:
> On Mon, May 18, 2020 at 9:10 PM Eli Britstein <elibr@mellanox.com> 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 <elibr@mellanox.com>
>> Reviewed-by: Roni Bar Yanai <roniba@mellanox.com>
>> ---
>>   lib/netdev-offload-dpdk.c | 133 ++++++++++++++++++----------------------------
>>   1 file changed, 51 insertions(+), 82 deletions(-)
>>
>> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
>> index f8c46bbaa..84bbe29e7 100644
>> --- a/lib/netdev-offload-dpdk.c
>> +++ b/lib/netdev-offload-dpdk.c
>> @@ -559,10 +559,22 @@ 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;
>> +    int ret = 0;
>> +
>> +    consumed_masks = &match->wc.masks;
>> +
>> +    memset(&consumed_masks->in_port, 0, sizeof consumed_masks->in_port);
>> +    if (match->flow.recirc_id != 0) {
>> +        ret = -1;
>> +        goto out;
>> +    }
>> +    consumed_masks->recirc_id = 0;
>> +    consumed_masks->packet_type = 0;
>>
>>       /* Eth */
>>       if (!eth_addr_is_zero(match->wc.masks.dl_src) ||
>> @@ -580,6 +592,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 +606,7 @@ parse_flow_match(struct flow_patterns *patterns,
>>            */
>>           add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_ETH, NULL, NULL);
>>       }
>> +    consumed_masks->dl_type = 0;
> Why not set the dl_type also under the if() condition above, where
> dl_src an dl_dst consumed masks are being set ? Similarly, vlan
> consumed mask below.
OVS always matches on dl_type (same for the below of VLAN). We must 
clean it in any case in order not to fail.
>
>>       /* VLAN */
>>       if (match->wc.masks.vlans[0].tci && match->flow.vlans[0].tci) {
>> @@ -607,6 +623,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]);
>>
>>       /* IP v4 */
>>       if (match->flow.dl_type == htons(ETH_TYPE_IP)) {
>> @@ -627,6 +644,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 +657,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;
>>
>>       if (proto != IPPROTO_ICMP && proto != IPPROTO_UDP  &&
>>           proto != IPPROTO_SCTP && proto != IPPROTO_TCP  &&
>> @@ -641,12 +666,14 @@ parse_flow_match(struct flow_patterns *patterns,
>>            match->wc.masks.tp_dst ||
>>            match->wc.masks.tcp_flags)) {
>>           VLOG_DBG("L4 Protocol (%u) not supported", proto);
>> -        return -1;
>> +        ret = -1;
>> +        goto out;
>>       }
>>
>>       if ((match->wc.masks.tp_src && match->wc.masks.tp_src != OVS_BE16_MAX) ||
>>           (match->wc.masks.tp_dst && match->wc.masks.tp_dst != OVS_BE16_MAX)) {
>> -        return -1;
>> +        ret = -1;
>> +        goto out;
>>       }
>>
>>       if (proto == IPPROTO_TCP) {
>> @@ -665,6 +692,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 +714,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 +735,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 +756,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,7 +769,11 @@ parse_flow_match(struct flow_patterns *patterns,
>>
>>       add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_END, NULL, NULL);
>>
>> -    return 0;
>> +    if (!is_all_zeros(consumed_masks, sizeof *consumed_masks)) {
>> +        ret = -1;
>> +    }
>> +out:
>> +    return ret;
> There's really no cleanup/error handling code under the label 'out'.
> So, this label could be removed and all the above goto statements
> replaced by just return -1. This also improves readability (avoid
> scrolling down just to see a return statement).
OK. I'll change.
>
> Thanks,
> -Harsha
>
>
>
>>   }
>>
>>   static void
>> @@ -1039,7 +1083,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 +1096,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 +1125,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 +1168,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);
>>       }
>> --
>> 2.14.5
>>
Eli Britstein May 26, 2020, 1:46 p.m. UTC | #3 
On 5/19/2020 5:26 PM, Eli Britstein wrote:
>
> On 5/19/2020 4:41 PM, Sriharsha Basavapatna wrote:
>> On Mon, May 18, 2020 at 9:10 PM Eli Britstein <elibr@mellanox.com> 
>> 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 <elibr@mellanox.com>
>>> Reviewed-by: Roni Bar Yanai <roniba@mellanox.com>
>>> ---
>>>   lib/netdev-offload-dpdk.c | 133 
>>> ++++++++++++++++++----------------------------
>>>   1 file changed, 51 insertions(+), 82 deletions(-)
>>>
>>> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
>>> index f8c46bbaa..84bbe29e7 100644
>>> --- a/lib/netdev-offload-dpdk.c
>>> +++ b/lib/netdev-offload-dpdk.c
>>> @@ -559,10 +559,22 @@ 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;
>>> +    int ret = 0;
>>> +
>>> +    consumed_masks = &match->wc.masks;
>>> +
>>> +    memset(&consumed_masks->in_port, 0, sizeof 
>>> consumed_masks->in_port);
>>> +    if (match->flow.recirc_id != 0) {
>>> +        ret = -1;
>>> +        goto out;
>>> +    }
>>> +    consumed_masks->recirc_id = 0;
>>> +    consumed_masks->packet_type = 0;
>>>
>>>       /* Eth */
>>>       if (!eth_addr_is_zero(match->wc.masks.dl_src) ||
>>> @@ -580,6 +592,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 +606,7 @@ parse_flow_match(struct flow_patterns *patterns,
>>>            */
>>>           add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_ETH, NULL, 
>>> NULL);
>>>       }
>>> +    consumed_masks->dl_type = 0;
>> Why not set the dl_type also under the if() condition above, where
>> dl_src an dl_dst consumed masks are being set ? Similarly, vlan
>> consumed mask below.
> OVS always matches on dl_type (same for the below of VLAN). We must 
> clean it in any case in order not to fail.
That's right. It's a bug here from upstream. dl_type should be handled 
even if no matches on src/dst. I'll add a commit to fix it in v2.
>>
>>>       /* VLAN */
>>>       if (match->wc.masks.vlans[0].tci && match->flow.vlans[0].tci) {
>>> @@ -607,6 +623,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]);
>>>
>>>       /* IP v4 */
>>>       if (match->flow.dl_type == htons(ETH_TYPE_IP)) {
>>> @@ -627,6 +644,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 +657,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;
>>>
>>>       if (proto != IPPROTO_ICMP && proto != IPPROTO_UDP  &&
>>>           proto != IPPROTO_SCTP && proto != IPPROTO_TCP  &&
>>> @@ -641,12 +666,14 @@ parse_flow_match(struct flow_patterns *patterns,
>>>            match->wc.masks.tp_dst ||
>>>            match->wc.masks.tcp_flags)) {
>>>           VLOG_DBG("L4 Protocol (%u) not supported", proto);
>>> -        return -1;
>>> +        ret = -1;
>>> +        goto out;
>>>       }
>>>
>>>       if ((match->wc.masks.tp_src && match->wc.masks.tp_src != 
>>> OVS_BE16_MAX) ||
>>>           (match->wc.masks.tp_dst && match->wc.masks.tp_dst != 
>>> OVS_BE16_MAX)) {
>>> -        return -1;
>>> +        ret = -1;
>>> +        goto out;
>>>       }
>>>
>>>       if (proto == IPPROTO_TCP) {
>>> @@ -665,6 +692,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 +714,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 +735,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 +756,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,7 +769,11 @@ parse_flow_match(struct flow_patterns *patterns,
>>>
>>>       add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_END, NULL, NULL);
>>>
>>> -    return 0;
>>> +    if (!is_all_zeros(consumed_masks, sizeof *consumed_masks)) {
>>> +        ret = -1;
>>> +    }
>>> +out:
>>> +    return ret;
>> There's really no cleanup/error handling code under the label 'out'.
>> So, this label could be removed and all the above goto statements
>> replaced by just return -1. This also improves readability (avoid
>> scrolling down just to see a return statement).
> OK. I'll change.
>>
>> Thanks,
>> -Harsha
>>
>>
>>
>>>   }
>>>
>>>   static void
>>> @@ -1039,7 +1083,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 +1096,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 +1125,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 +1168,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);
>>>       }
>>> -- 
>>> 2.14.5
>>>
Ilya Maximets May 26, 2020, 8:30 p.m. UTC | #4 
On 5/26/20 3:46 PM, Eli Britstein wrote:
> 
> On 5/19/2020 5:26 PM, Eli Britstein wrote:
>>
>> On 5/19/2020 4:41 PM, Sriharsha Basavapatna wrote:
>>> On Mon, May 18, 2020 at 9:10 PM Eli Britstein <elibr@mellanox.com> 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 <elibr@mellanox.com>
>>>> Reviewed-by: Roni Bar Yanai <roniba@mellanox.com>
>>>> ---
>>>>   lib/netdev-offload-dpdk.c | 133 ++++++++++++++++++----------------------------
>>>>   1 file changed, 51 insertions(+), 82 deletions(-)
>>>>
>>>> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
>>>> index f8c46bbaa..84bbe29e7 100644
>>>> --- a/lib/netdev-offload-dpdk.c
>>>> +++ b/lib/netdev-offload-dpdk.c
>>>> @@ -559,10 +559,22 @@ 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;
>>>> +    int ret = 0;
>>>> +
>>>> +    consumed_masks = &match->wc.masks;
>>>> +
>>>> +    memset(&consumed_masks->in_port, 0, sizeof consumed_masks->in_port);
>>>> +    if (match->flow.recirc_id != 0) {
>>>> +        ret = -1;
>>>> +        goto out;
>>>> +    }
>>>> +    consumed_masks->recirc_id = 0;
>>>> +    consumed_masks->packet_type = 0;
>>>>
>>>>       /* Eth */
>>>>       if (!eth_addr_is_zero(match->wc.masks.dl_src) ||
>>>> @@ -580,6 +592,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 +606,7 @@ parse_flow_match(struct flow_patterns *patterns,
>>>>            */
>>>>           add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_ETH, NULL, NULL);
>>>>       }
>>>> +    consumed_masks->dl_type = 0;
>>> Why not set the dl_type also under the if() condition above, where
>>> dl_src an dl_dst consumed masks are being set ? Similarly, vlan
>>> consumed mask below.
>> OVS always matches on dl_type (same for the below of VLAN). We must clean it in any case in order not to fail.
> That's right. It's a bug here from upstream. dl_type should be handled even if no matches on src/dst. I'll add a commit to fix it in v2.

Is it something related:
https://patchwork.ozlabs.org/project/openvswitch/patch/1587180266-28632-1-git-send-email-JackerDune@gmail.com/
?

Best regards, Ilya Maximets.
Eli Britstein May 27, 2020, 4:41 a.m. UTC | #5 
On 5/26/2020 11:30 PM, Ilya Maximets wrote:
> On 5/26/20 3:46 PM, Eli Britstein wrote:
>> On 5/19/2020 5:26 PM, Eli Britstein wrote:
>>> On 5/19/2020 4:41 PM, Sriharsha Basavapatna wrote:
>>>> On Mon, May 18, 2020 at 9:10 PM Eli Britstein <elibr@mellanox.com> 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 <elibr@mellanox.com>
>>>>> Reviewed-by: Roni Bar Yanai <roniba@mellanox.com>
>>>>> ---
>>>>>    lib/netdev-offload-dpdk.c | 133 ++++++++++++++++++----------------------------
>>>>>    1 file changed, 51 insertions(+), 82 deletions(-)
>>>>>
>>>>> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
>>>>> index f8c46bbaa..84bbe29e7 100644
>>>>> --- a/lib/netdev-offload-dpdk.c
>>>>> +++ b/lib/netdev-offload-dpdk.c
>>>>> @@ -559,10 +559,22 @@ 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;
>>>>> +    int ret = 0;
>>>>> +
>>>>> +    consumed_masks = &match->wc.masks;
>>>>> +
>>>>> +    memset(&consumed_masks->in_port, 0, sizeof consumed_masks->in_port);
>>>>> +    if (match->flow.recirc_id != 0) {
>>>>> +        ret = -1;
>>>>> +        goto out;
>>>>> +    }
>>>>> +    consumed_masks->recirc_id = 0;
>>>>> +    consumed_masks->packet_type = 0;
>>>>>
>>>>>        /* Eth */
>>>>>        if (!eth_addr_is_zero(match->wc.masks.dl_src) ||
>>>>> @@ -580,6 +592,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 +606,7 @@ parse_flow_match(struct flow_patterns *patterns,
>>>>>             */
>>>>>            add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_ETH, NULL, NULL);
>>>>>        }
>>>>> +    consumed_masks->dl_type = 0;
>>>> Why not set the dl_type also under the if() condition above, where
>>>> dl_src an dl_dst consumed masks are being set ? Similarly, vlan
>>>> consumed mask below.
>>> OVS always matches on dl_type (same for the below of VLAN). We must clean it in any case in order not to fail.
>> That's right. It's a bug here from upstream. dl_type should be handled even if no matches on src/dst. I'll add a commit to fix it in v2.
> Is it something related:
> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.ozlabs.org%2Fproject%2Fopenvswitch%2Fpatch%2F1587180266-28632-1-git-send-email-JackerDune%40gmail.com%2F&amp;data=02%7C01%7Celibr%40mellanox.com%7C1598511210834cb7bfeb08d801b39c95%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C637261218225476915&amp;sdata=JCsdv1rqjKljOQh%2F9%2BD53lbQMq0NYAqqzph08JO5y9Y%3D&amp;reserved=0
> ?
Yes. However, this patch causes this case not to be offloaded at all, 
with the claim that Intel XL710 doesn't support it. As OVS is vendor 
agnostic, such workarounds should not be in OVS, but rather in the 
specific PMD code. My fix will add offload support for this case, and 
not reject it.
>
> Best regards, Ilya Maximets.
diff mbox series

Patch

diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
index f8c46bbaa..84bbe29e7 100644
--- a/lib/netdev-offload-dpdk.c
+++ b/lib/netdev-offload-dpdk.c
@@ -559,10 +559,22 @@  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;
+    int ret = 0;
+
+    consumed_masks = &match->wc.masks;
+
+    memset(&consumed_masks->in_port, 0, sizeof consumed_masks->in_port);
+    if (match->flow.recirc_id != 0) {
+        ret = -1;
+        goto out;
+    }
+    consumed_masks->recirc_id = 0;
+    consumed_masks->packet_type = 0;
 
     /* Eth */
     if (!eth_addr_is_zero(match->wc.masks.dl_src) ||
@@ -580,6 +592,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 +606,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 +623,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]);
 
     /* IP v4 */
     if (match->flow.dl_type == htons(ETH_TYPE_IP)) {
@@ -627,6 +644,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 +657,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;
 
     if (proto != IPPROTO_ICMP && proto != IPPROTO_UDP  &&
         proto != IPPROTO_SCTP && proto != IPPROTO_TCP  &&
@@ -641,12 +666,14 @@  parse_flow_match(struct flow_patterns *patterns,
          match->wc.masks.tp_dst ||
          match->wc.masks.tcp_flags)) {
         VLOG_DBG("L4 Protocol (%u) not supported", proto);
-        return -1;
+        ret = -1;
+        goto out;
     }
 
     if ((match->wc.masks.tp_src && match->wc.masks.tp_src != OVS_BE16_MAX) ||
         (match->wc.masks.tp_dst && match->wc.masks.tp_dst != OVS_BE16_MAX)) {
-        return -1;
+        ret = -1;
+        goto out;
     }
 
     if (proto == IPPROTO_TCP) {
@@ -665,6 +692,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 +714,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 +735,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 +756,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,7 +769,11 @@  parse_flow_match(struct flow_patterns *patterns,
 
     add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_END, NULL, NULL);
 
-    return 0;
+    if (!is_all_zeros(consumed_masks, sizeof *consumed_masks)) {
+        ret = -1;
+    }
+out:
+    return ret;
 }
 
 static void
@@ -1039,7 +1083,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 +1096,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 +1125,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 +1168,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);
     }