>-----Original Message-----
>From: Ilya Maximets <i.maxim...@ovn.org>
>Sent: Tuesday, August 11, 2020 4:19 PM
>To: Finn, Emma <emma.f...@intel.com>; Ilya Maximets <i.maxim...@ovn.org>;
>Xing, Beilei <beilei.x...@intel.com>; Stokes, Ian <ian.sto...@intel.com>; Eli
>Britstein <el...@mellanox.com>; d...@openvswitch.org; Guo, Jia
><jia....@intel.com>
>Subject: Re: [PATCH] netdev-offload-dpdk: Fix for broken ethernet matching
>HWOL for XL710 NIC
>
>On 8/11/20 3:12 PM, Finn, Emma wrote:
>>
>>
>>> -----Original Message-----
>>> From: Ilya Maximets <i.maxim...@ovn.org>
>>> Sent: Tuesday 11 August 2020 11:02
>>> To: Xing, Beilei <beilei.x...@intel.com>; Stokes, Ian
>>> <ian.sto...@intel.com>; Eli Britstein <el...@mellanox.com>; Finn,
>>> Emma <emma.f...@intel.com>; d...@openvswitch.org; Guo, Jia
>>> <jia....@intel.com>
>>> Cc: i.maxim...@ovn.org
>>> Subject: Re: [PATCH] netdev-offload-dpdk: Fix for broken ethernet
>>> matching HWOL for XL710 NIC
>>>
>>> On 8/11/20 5:35 AM, Xing, Beilei wrote:
>>>>
>>>>
>>>>> -----Original Message-----
>>>>> From: Stokes, Ian <ian.sto...@intel.com>
>>>>> Sent: Tuesday, August 11, 2020 4:52 AM
>>>>> To: Eli Britstein <el...@mellanox.com>; Finn, Emma
>>>>> <emma.f...@intel.com>; d...@openvswitch.org; Xing, Beilei
>>>>> <beilei.x...@intel.com>; Guo, Jia <jia....@intel.com>
>>>>> Cc: i.maxim...@ovn.org
>>>>> Subject: RE: [PATCH] netdev-offload-dpdk: Fix for broken ethernet
>>>>> matching HWOL for XL710 NIC
>>>>>
>>>>>> On 8/7/2020 7:55 AM, Eli Britstein wrote:
>>>>>>>
>>>>>>> On 8/6/2020 8:28 PM, Stokes, Ian wrote:
>>>>>>>>> On 8/6/2020 6:17 PM, Emma Finn wrote:
>>>>>>>>>> The following 2 commits introduced changes which caused a
>>>>>>>>>> regression for XL710 devices and functionality ceases for
>>>>>>>>>> partial offload as a result.
>>>>>>>>>> 864852a0624a ("netdev-offload-dpdk: Fix Ethernet matching for
>>>>>>>>>> type
>>>>>>>>>> only.")
>>>>>>>>>> a79eae87abe4 ("netdev-offload-dpdk: Remove pre-validate of
>>>>>> patterns
>>>>>>>>>> function.")
>>>>>>>>> OVS is vendor agnostic. That kind of workaround belongs in
>>>>>>>>> intel PMD in dpdk, not in OVS.
>>>>>>>> Hi Eli,
>>>>>>>>
>>>>>>>> Yes OVS looks to be vendor agnostic, but this code I believe was
>>>>>>>> already in place and working for this usecase. As such it's
>>>>>>>> removal introduced a regression from an OVS point of view
>>>>>>>> between
>>> the releases.
>>>>>>>>
>>>>>>>> We have had examples in the past where workarounds are
>>> permissible
>>>>>>>> if there is a clear path to fixing this in the future on the
>>>>>>>> DPDK side (which is what I suggest here) (for example scatter
>>>>>>>> gather support for MTUs in the past raised similar issue where
>>>>>>>> we hand to handle specific NIC until the next DPDK LTS release).
>>>>>>>>
>>>>>>>> So my suggestion is to re-instate the original workaround and
>>>>>>>> remove its when fixed in the next DPDK LTS which supports the
>>>>>>>> change for i40e at the PMD layer or if it's backported to the
>>>>>>>> next
>>>>>>>> 19.11 stable release which would be validated for use with OVS.
>>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> There was a bug with this WA.
>>>>>>>
>>>>>>> Please see
>>>>>>> https://nam03.safelinks.protection.outlook.com/?url=https%3A%2F%2
>>>>>>>
>Fpatchwork.ozlabs.org%2Fproject%2Fopenvswitch%2Fpatch%2F158718026
>>>>>>> 6-
>&data=02%7C01%7Celibr%40nvidia.com%7C2eb7f5d9553c44e4980a08
>>>>>>>
>d83df912c9%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637327
>487
>>>>>>>
>263661244&sdata=fgnwXLy3xt%2B8H8h9BJwDzlPmp3dtWv6AMTQ69%2
>B9kb
>>>>>>> NM%3D&reserved=0
>>>>>> 28632-1-git-send-email-jackerd...@gmail.com/.
>>>
>>> I'm worried about this bug. Current version of a workaround is not
>>> correct from the OVS point of view since it wildcards dl_type during
>>> offloading that is not expected by higher layers, causing HW flow
>>> being much more wide than requested. In case we going to have this
>>> workaround for xl710, we should have another workaround for this issue too.
>>>
>>>
>>>>>>>
>>>>>>> Is it possible to address it in DPDK instead of reverting in OVS
>>>>>>> and later re-reverting?
>>>>>>>
>>>>>>> Thanks,
>>>>>>>
>>>>>>> Eli
>>>>>>>
>>>>>>>>
>>>>>>>> I've included the i40e DPDK maintainers here for their thoughts also.
>>>>>>>>
>>>>>>>> @Beilei/Jia Is this something we can look at to introduce in the
>>>>>>>> i40e PMD?
>>>>>>
>>>>>> Hello,
>>>>>>
>>>>>> Please take a look at:
>>>>>>
>>>>>> https://nam03.safelinks.protection.outlook.com/?url=http%3A%2F%2Fm
>>>>>> ails.dpdk.org%2Farchives%2Fdev%2F2020-
>May%2F166272.html&data=0
>>>>>>
>2%7C01%7Celibr%40nvidia.com%7C2eb7f5d9553c44e4980a08d83df912c9%7
>C4
>>>>>>
>3083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637327487263661244&a
>mp;s
>>>>>>
>data=g2Yiy07cL4yGIQYsJzNkZqimUibNMXsDQLNdtJ8Evmw%3D&reserved=
>0
>>>>>>
>>>>>> For MLX it was only an optimization. For i40e something similar
>>>>>> may be a workaround for this issue.
>>>>>
>>>>> Thanks for this Eli, let me sync with Beilei on this.
>>>>>
>>>>> If it's something we can resolve in the PMD then I think we can add
>>>>> an errata or known issue for the 2.14 release (and possibly the
>>>>> 2.13 release as I think the same issue is present there).
>>>>>
>>>>> If it was fixed in the future we could then remove the issue
>>>>> notice.>
>>>>
>>>> Hi,
>>>>
>>>> According to the OVS patch and mlx DPDK patch, is the requirement to
>>>> support rte flow pattern like 'pattern IPv4 / UDP src is 32 / end',
>>>> no need to use ''pattern ETH / IPv4 / UDP src is 32 / end '?
>>>> please correct me if I'm wrong.
>>>>
>>>> If yes, could you please tell me how OVS adds a flow which doesn’t
>>>> include ETH item? I'm not very familiar with OVS, I run some simple
>>>> commands before, and find eth will always exist. E.g:
>>>> flow_add: ufid:fced09a8-9b8a-420d-9cb6-454ab9bed224
>>>> skb_priority(0),skb_mark(0),\
>>>> ct_state(-new-est-rel-rpl-inv-trk-snat-dnat),ct_zone(0),ct_mark(0),c
>>>> t_
>>>> label(0),recirc_id(0),\
>>>> dp_hash(0),in_port(2),packet_type(ns=0,id=0),eth(src=00:e8:ca:11:ba:
>>>> 80
>>>> ,dst=ff:ff:ff:ff:ff:ff),
>>>> eth_type(0x0800),ipv4(src=0.0.0.0,dst=255.255.255.255,proto=17,tos=0
>>>> x1
>>>> 0,ttl=128,frag=no),
>>>> udp(src=68,dst=67), actions:drop
>>>
>>> Emma, Ian, could you please provide the pattern that fails to offload
>>> on current OVS master so it will be easier for everyone to understand the
>issue.
>>> It's not obvious how to construct the bad pattern by only looking at
>>> the i40e pmd code. Also, please, enable debug logs and provide the
>>> testpmd-style arguments constructed by OVS.
>>>
>>> It might be also good to have all this information in the commit message.
>>>
>>
>> Following flow pattern is failing from logs:
>>
>> Attributes: ingress=1, egress=0, prio=0, group=0, transfer=1 rte
>> flow eth pattern:
>> Spec: src=00:00:04:00:0b:00, dst=00:00:04:00:0a:00, type=0x0800
>> Mask: src=00:00:00:00:00:00, dst=00:00:00:00:00:00, type=0xffff rte
>> flow ipv4 pattern:
>> Spec: tos=0x0, ttl=40, proto=0x11, src=1.1.0.0, dst=0.0.0.0
>> Mask: tos=0x0, ttl=0, proto=0x0, src=255.255.255.255, dst=0.0.0.0
>> rte flow count action:
>> Count: shared=0, id=0
>> rte flow port-id action:
>> Port-id: original=0, id=1
>> 2020-08-
>11T10:49:28.002Z|00003|netdev_offload_dpdk(dp_netdev_flow_18)|WARN|
>dpdk0: rte_flow creation failed: 13 (Unsupported ether_type.).
Hi
Maybe another workaround until fixed in i40e PMD is to mask out the ether_type
match if there is IPv4 or IPv6.
It means that patterns of:
eth type is 0x0800 / ipv4 / ...
will be
eth / ipv4 / ...
For IPv6 the same, but for other ether types no removal of the match.
eth type is 0x1234 /
kept the same.
I referred to MLX5 PMD patch. As I mentioned, for MLX5 it's only an
optimization, for XL710 it can be used as a workaround.
http://mails.dpdk.org/archives/dev/2020-May/166272.html
A similar masking out of protocol type is done today for TCP/UDP/SCTP/ICMP. For
example:
/* proto == TCP and ITEM_TYPE_TCP, thus no need for proto match. */
if (next_proto_mask) {
*next_proto_mask = 0;
}
Please take a look at
https://patchwork.ozlabs.org/project/openvswitch/patch/20200710120718.38633-3-sriharsha.basavapa...@broadcom.com/
That posted commit suggest not to mask out the protocol type and let the PMDs
do it if they wish.
The proposed workaround (not tested):
diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
index de6101e4d..1af7bebcd 100644
--- a/lib/netdev-offload-dpdk.c
+++ b/lib/netdev-offload-dpdk.c
@@ -676,6 +676,7 @@ static int
parse_flow_match(struct flow_patterns *patterns,
struct match *match)
{
+ uint8_t *next_eth_type_mask = NULL;
uint8_t *next_proto_mask = NULL;
struct flow *consumed_masks;
uint8_t proto = 0;
@@ -712,6 +713,7 @@ parse_flow_match(struct flow_patterns *patterns,
consumed_masks->dl_type = 0;
add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_ETH, spec, mask);
+ next_eth_type_mask = &mask->type;
}
/* VLAN */
@@ -739,6 +741,11 @@ parse_flow_match(struct flow_patterns *patterns,
if (match->flow.dl_type == htons(ETH_TYPE_IP)) {
struct rte_flow_item_ipv4 *spec, *mask;
+ /* IPv4. No need to match ether type. */
+ if (next_eth_type_mask) {
+ *next_eth_type_mask = 0;
+ }
+
spec = xzalloc(sizeof *spec);
mask = xzalloc(sizeof *mask);
@@ -777,6 +784,11 @@ parse_flow_match(struct flow_patterns *patterns,
if (match->flow.dl_type == htons(ETH_TYPE_IPV6)) {
struct rte_flow_item_ipv6 *spec, *mask;
+ /* IPv6. No need to match ether type. */
+ if (next_eth_type_mask) {
+ *next_eth_type_mask = 0;
+ }
+
spec = xzalloc(sizeof *spec);
mask = xzalloc(sizeof *mask);
>>
>> Or from dump-flows :
>>
>> flow-dump from pmd on cpu core: 23
>> ufid:b4ba667d-8f4d-4f45-8896-c541d9fcecc0,
>> skb_priority(0/0),skb_mark(0/0),ct_state(0/0),ct_zone(0/0),ct_mark(0/0
>> ),ct_label(0/0),recirc_id(0),dp_hash(0/0),in_port(dpdk0),packet_type(n
>> s=0,id=0),eth(src=00:00:04:00:0b:00/00:00:00:00:00:00,dst=00:00:04:00:
>> 0a:00/00:00:00:00:00:00),eth_type(0x0800),ipv4(src=1.1.0.0,dst=0.0.0.0
>> /0.0.0.0,proto=17/0,tos=0/0,ttl=64/0,frag=no),udp(src=63/0,dst=63/0),
>> packets:42575487, bytes:2554529220, used:0.000s, offloaded:partial,
>> dp:ovs, actions:dpdk1, dp-extra-info:miniflow_bits(4,2)
>>
>>
>> with regards to " provide the testpmd-style arguments constructed by OVS."
>> Can you confirm what you mean?
>
>We changed the way of logging in a following commit:
>d8ad173fb9c1 ("netdev-offload-dpdk: Log testpmd format for flow
>create/destroy.")
>
>It's on master and branch-2.14.
>
>>
>> Thanks,
>> Emma
>>
>>> Thanks.
>>>
>>> Best regards, Ilya Maximets.
>>>
>>>>
>>>> Thanks,
>>>> Beilei
>>>>
>>>>>
>>>>> Regards
>>>>> Ian
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> Eli
>>>>>>
>>>>>>>>
>>>>>>>> Regards
>>>>>>>> Ian
>>>>>>>>
>>>>>>>>>> Fixed by partial reversion of these changes.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Emma Finn<emma.f...@intel.com>
>>>>
>>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
