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 <el...@mellanox.com> >>> Reviewed-by: Roni Bar Yanai <ron...@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); >>> } >>> _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev