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

Sriharsha Basavapatna sriharsha.basavapatna at broadcom.com
Mon Jun 29 09:07:17 UTC 2020


On Mon, Jun 29, 2020 at 2:27 PM Ilya Maximets <i.maximets at 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 at mellanox.com>
> >>> Reviewed-by: Roni Bar Yanai <roniba at 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);
> >>>       }
> >>>
>


More information about the dev mailing list