I didn't want to remove the default parsing of tunnel as VxLan because I thought it might be used, Instead I moved it to the end, which makes it detect all supported tunnel through udp_dst_port, And only if no tunnel was matched it would default to VxLan. That was the reason geneve weren't detected and parsed as vxlan instead, which is the bug I was trying to solve.
-----Original Message----- From: Singh, Aman Deep <aman.deep.si...@intel.com> Sent: Thursday, January 20, 2022 12:47 PM To: Matan Azrad <ma...@nvidia.com>; Ferruh Yigit <ferruh.yi...@intel.com>; Raja Zidane <rzid...@nvidia.com>; dev@dpdk.org Cc: sta...@dpdk.org Subject: Re: [PATCH] app/testpmd: fix GENEVE parsing in csum forward mode External email: Use caution opening links or attachments On 1/18/2022 6:49 PM, Matan Azrad wrote: > >> -----Original Message----- >> From: Ferruh Yigit <ferruh.yi...@intel.com> >> Sent: Tuesday, January 18, 2022 3:03 PM >> To: Matan Azrad <ma...@nvidia.com>; Raja Zidane <rzid...@nvidia.com>; >> dev@dpdk.org >> Cc: sta...@dpdk.org >> Subject: Re: [PATCH] app/testpmd: fix GENEVE parsing in csum forward >> mode >> >> External email: Use caution opening links or attachments >> >> >> On 1/18/2022 12:55 PM, Matan Azrad wrote: >>> >>>> -----Original Message----- >>>> From: Ferruh Yigit <ferruh.yi...@intel.com> >>>> Sent: Tuesday, January 18, 2022 2:28 PM >>>> To: Matan Azrad <ma...@nvidia.com>; Raja Zidane >>>> <rzid...@nvidia.com>; dev@dpdk.org >>>> Cc: sta...@dpdk.org >>>> Subject: Re: [PATCH] app/testpmd: fix GENEVE parsing in csum >>>> forward mode >>>> >>>> External email: Use caution opening links or attachments >>>> >>>> >>>> On 1/18/2022 11:27 AM, Matan Azrad wrote: >>>>> >>>>>> -----Original Message----- >>>>>> From: Ferruh Yigit <ferruh.yi...@intel.com> >>>>>> Sent: Tuesday, January 18, 2022 11:52 AM >>>>>> To: Raja Zidane <rzid...@nvidia.com>; dev@dpdk.org >>>>>> Cc: Matan Azrad <ma...@nvidia.com>; sta...@dpdk.org >>>>>> Subject: Re: [PATCH] app/testpmd: fix GENEVE parsing in csum >>>>>> forward mode >>>>>> >>>>>> External email: Use caution opening links or attachments >>>>>> >>>>>> >>>>>> On 12/5/2021 3:44 AM, Raja Zidane wrote: >>>>>>> The csum FWD mode parses any received packet to set mbuf >>>>>>> offloads for the transmitting burst, mainly in the checksum/TSO areas. >>>>>>> In the case of a tunnel header, the csum FWD tries to detect >>>>>>> known tunnels by the standard definition using the header'sdata >>>>>>> and fallback to check the packet type in the mbuf to see if the >>>>>>> Rx port driver already sign the packet as a tunnel. >>>>>>> In the fallback case, the csum assumes the tunnel is VXLAN and >>>>>>> parses the tunnel as VXLAN. >>>>>> As far as I can see there is a VXLAN port check in >>>>>> 'parse_vxlan()', why it is not helping? >>>>>> >>>>> The problem is not the vxlan check but the tunnel type in mbuf >>>>> that caused the >>>> packet to be detected as vxlan(default) before checking GENEVE tunnel case. >>>> Check is as following: >>>> >>>> if (udp_hdr->dst_port != _htons(RTE_VXLAN_DEFAULT_PORT) && >>>> RTE_ETH_IS_TUNNEL_PKT(pkt_type) == 0) >>>> return; >>>> >>>> Do you what is the intention for the >>>> "RTE_ETH_IS_TUNNEL_PKT(pkt_type) == >> 0" >>>> check? >>>> Why vxlan parsing doesn't stop when it is not default port? >>> Maybe some drivers set the tunnel type for vxlan packets coming >>> after non- >> standard vxlan port. >> But checking the tunnel flag to say that it is vxlan is too broad, isn't it? >> And this is the problem you are having. >> >> Can there be any way to detect and check non-standard vxlan port? > Maybe yes, but it is probably more complex solusion. > > See this patch: > https://patches.dpdk.org/project/dpdk/patch/1423819371-24222-13-git-se > nd-email-olivier.m...@6wind.com/ > > comments there: > 1. check udp destination port, 4789 is the default vxlan port (rfc7348). > 2. currently, this flag is set by i40e only if the packet is vxlan > > And maybe another driver assumes more ports here. > > Collecting all the non-standard ports can start from i40 maintainers😊 I think, here we should check for supported tunnel types only. The i40 driver setting a Tunnel flag, does not means that it VxLan type only. As in your case those were GENEVE packets getting treated as VxLan. Making parse_vxlan() udp_dst_port specific can avoid this issue. >>>>>>> When the GENEVE tunnel was added to the known tunnels in csum, >>>>>>> its parsing trial was wrongly located after the pkt type >>>>>>> detection, causing the csum to parse the GENEVE header as VXLAN >>>>>>> when the Rx port set the tunnel packet type. >>>>>>> >>>>>>> Locate the GENEVE parsing trial before the packet type detection. >>>>>>> >>>>>>> Fixes: ea0e711b8ae0 ("app/testpmd: add GENEVE parsing") >>>>>>> Cc: sta...@dpdk.org >>>>>>> >>>>>>> Signed-off-by: Raja Zidane <rzid...@nvidia.com> >>>>>>> --- >>>>>>> Acked-by: Matan Azrad <ma...@nvidia.com> >>>>>> Ack should be before '---' to be part of the commit log, >>>>>> otherwise it is dropped when applied as comment. >>>>>> >>>>>>> app/test-pmd/csumonly.c | 16 ++++++++++------ >>>>>>> 1 file changed, 10 insertions(+), 6 deletions(-) >>>>>>> >>>>>>> diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c >>>>>>> index 2aeea243b6..fe810fecdd 100644 >>>>>>> --- a/app/test-pmd/csumonly.c >>>>>>> +++ b/app/test-pmd/csumonly.c >>>>>>> @@ -254,7 +254,10 @@ parse_gtp(struct rte_udp_hdr *udp_hdr, >>>>>>> info->l2_len += RTE_ETHER_GTP_HLEN; >>>>>>> } >>>>>>> >>>>>>> -/* Parse a vxlan header */ >>>>>>> +/* >>>>>>> + * Parse a vxlan header. >>>>>>> + * If a tunnel is detected in 'pkt_type' it will be parsed by default >>>>>>> as vxlan. >>>>>>> + */ >>>>>>> static void >>>>>>> parse_vxlan(struct rte_udp_hdr *udp_hdr, >>>>>>> struct testpmd_offload_info *info, @@ -912,17 >>>>>>> +915,18 @@ pkt_burst_checksum_forward(struct fwd_stream *fs) >>>>>>> >>>>>>> RTE_MBUF_F_TX_TUNNEL_VXLAN_GPE; >>>>>>> goto tunnel_update; >>>>>>> } >>>>>>> - parse_vxlan(udp_hdr, &info, >>>>>>> - m->packet_type); >>>>>>> + parse_geneve(udp_hdr, &info); >>>>>>> if (info.is_tunnel) { >>>>>>> tx_ol_flags |= >>>>>>> - >>>>>>> RTE_MBUF_F_TX_TUNNEL_VXLAN; >>>>>>> + >>>>>>> + RTE_MBUF_F_TX_TUNNEL_GENEVE; >>>>>>> goto tunnel_update; >>>>>>> } >>>>>>> - parse_geneve(udp_hdr, &info); >>>>>>> + /* Always keep last. */ >>>>>>> + parse_vxlan(udp_hdr, &info, >>>>>>> + m->packet_type); >>>>>>> if (info.is_tunnel) { >>>>>>> tx_ol_flags |= >>>>>>> - >>>>>>> RTE_MBUF_F_TX_TUNNEL_GENEVE; >>>>>>> + >>>>>>> + RTE_MBUF_F_TX_TUNNEL_VXLAN; >>>>>>> goto tunnel_update; >>>>>>> } >>>>>>> } else if (info.l4_proto == >>>>>>> IPPROTO_GRE) {
