[ovs-dev] [PATCH 01/11] netdev-offload-dpdk: Remove pre-validate of patterns function
Eli Britstein
elibr at mellanox.com
Tue May 26 13:46:43 UTC 2020
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 at 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 at mellanox.com>
>>> Reviewed-by: Roni Bar Yanai <roniba at 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
>>>
More information about the dev
mailing list