diff mbox series

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

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

Commit Message

Eli Britstein June 21, 2020, 11:19 a.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 | 122 ++++++++++++++++------------------------------
 1 file changed, 43 insertions(+), 79 deletions(-)

Comments

Ilya Maximets June 22, 2020, 7:04 a.m. UTC | #1 
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.

I think that validation function here is to speed up the process in case
of unsupported offloading patterns.  It saves time for all the not needed
memory allocations and copies.  With this change we will construct all the
patterns and only in the end will realize that we don't need them as the
flow can not be offloaded.

Best regards, Ilya Maximets.
Eli Britstein June 22, 2020, 7:33 a.m. UTC | #2 
On 6/22/2020 10:04 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.
> I think that validation function here is to speed up the process in case
> of unsupported offloading patterns.  It saves time for all the not needed
> memory allocations and copies.  With this change we will construct all the
> patterns and only in the end will realize that we don't need them as the
> flow can not be offloaded.
I know, and agree. However, the "save time" is negligible, comparing to 
more complicated code.
>
> Best regards, Ilya Maximets.
Eli Britstein June 23, 2020, 12:06 p.m. UTC | #3 
On 6/22/2020 10:33 AM, Eli Britstein wrote:
>
> On 6/22/2020 10:04 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.
>> I think that validation function here is to speed up the process in case
>> of unsupported offloading patterns.  It saves time for all the not 
>> needed
>> memory allocations and copies.  With this change we will construct 
>> all the
>> patterns and only in the end will realize that we don't need them as the
>> flow can not be offloaded.
> I know, and agree. However, the "save time" is negligible, comparing 
> to more complicated code.
Another note: looking from the point of view of the "good" flows, this 
validation, as fast as it is, is redundant.
>
>>
>> Best regards, Ilya Maximets.
Ilya Maximets June 23, 2020, 12:15 p.m. UTC | #4 
On 6/23/20 2:06 PM, Eli Britstein wrote:
> 
> On 6/22/2020 10:33 AM, Eli Britstein wrote:
>>
>> On 6/22/2020 10:04 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.
>>> I think that validation function here is to speed up the process in case
>>> of unsupported offloading patterns.  It saves time for all the not needed
>>> memory allocations and copies.  With this change we will construct all the
>>> patterns and only in the end will realize that we don't need them as the
>>> flow can not be offloaded.
>> I know, and agree. However, the "save time" is negligible, comparing to more complicated code.
> Another note: looking from the point of view of the "good" flows, this validation, as fast as it is, is redundant.

OK.  I understand what you're trying to achieve here.
Looking closer to the rest of the series now.

Best regards, Ilya Maximets.
Ilya Maximets June 28, 2020, 10:01 p.m. UTC | #5 
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 <elibr@mellanox.com>
> Reviewed-by: Roni Bar Yanai <roniba@mellanox.com>
> ---
>  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.

> +        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.

>  
>      /* 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.

>  
>      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);
>      }
>
Eli Britstein June 29, 2020, 7:11 a.m. UTC | #6 
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 <elibr@mellanox.com>
>> Reviewed-by: Roni Bar Yanai <roniba@mellanox.com>
>> ---
>>   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);
>>       }
>>
Ilya Maximets June 29, 2020, 8:57 a.m. UTC | #7 
On 6/29/20 9:11 AM, Eli Britstein wrote:
> 
> 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 <elibr@mellanox.com>
>>> Reviewed-by: Roni Bar Yanai <roniba@mellanox.com>
>>> ---
>>>   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.

OK.  Sorry, missed that it's a union.

> 
> 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.

OK.

> 
>>
>>>         /* 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);
>>>       }
>>>
Sriharsha Basavapatna June 29, 2020, 9:07 a.m. UTC | #8 
On Mon, Jun 29, 2020 at 2:27 PM Ilya Maximets <i.maximets@ovn.org> wrote:
>
> On 6/29/20 9:11 AM, Eli Britstein wrote:
> >
> > 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 <elibr@mellanox.com>
> >>> Reviewed-by: Roni Bar Yanai <roniba@mellanox.com>
> >>> ---
> >>>   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.
>
> OK.  Sorry, missed that it's a union.
>
> >
> > 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.
>
> OK.
Eli, Can you please add a comment above that line (that it's being
cleared outside to handle native vlan case)

Thanks,
-Harsha
>
> >
> >>
> >>>         /* 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);
> >>>       }
> >>>
>
diff mbox series

Patch

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) {
+        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]);
 
     /* 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;
 
     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);
     }