On 1/30/2022 4:48 PM, Raja Zidane wrote: > 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. We can take help/input from i40 maintainers for it. Hi Beilei Xing, For setting packet_type as Tunnel, what criteria is used by i40 driver. Is it only udp_dst port or any other parameters also. > > -----Original Message----- > From: Singh, Aman Deep > Sent: Thursday, January 20, 2022 12:47 PM > To: Matan Azrad; Ferruh Yigit; Raja Zidane;dev@dpdk.org > Cc:stable@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: > 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) { >>> -----Original Message----- >>> From: Ferruh Yigit >>> Sent: Tuesday, January 18, 2022 3:03 PM >>> To: Matan Azrad; Raja Zidane; >>> dev@dpdk.org >>> Cc:stable@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 >>>>> Sent: Tuesday, January 18, 2022 2:28 PM >>>>> To: Matan Azrad; Raja Zidane >>>>> ;dev@dpdk.org >>>>> Cc:stable@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 >>>>>>> Sent: Tuesday, January 18, 2022 11:52 AM >>>>>>> To: Raja Zidane;dev@dpdk.org >>>>>>> Cc: Matan Azrad;stable@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.matz@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:stable@dpdk.org >>>>>>>> >>>>>>>> Signed-off-by: Raja Zidane >>>>>>>> --- >>>>>>>> Acked-by: Matan Azrad >>>>>>> Ack should be before '---' to be part of the commit log, >>>>>>> otherwise it is dropped when applied as comment. >>>>>>>