RE: [PATCH] app/testpmd: fix GENEVE parsing in csum forward mode

[Raja Zidane <rzidane@nvidia.com>]

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.singh@intel.com> 
Sent: Thursday, January 20, 2022 12:47 PM
To: Matan Azrad <matan@nvidia.com>; Ferruh Yigit <ferruh.yigit@intel.com>; Raja Zidane <rzidane@nvidia.com>; 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:
>
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>> Sent: Tuesday, January 18, 2022 3:03 PM
>> To: Matan Azrad <matan@nvidia.com>; Raja Zidane <rzidane@nvidia.com>; 
>> 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 <ferruh.yigit@intel.com>
>>>> Sent: Tuesday, January 18, 2022 2:28 PM
>>>> To: Matan Azrad <matan@nvidia.com>; Raja Zidane 
>>>> <rzidane@nvidia.com>; 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 <ferruh.yigit@intel.com>
>>>>>> Sent: Tuesday, January 18, 2022 11:52 AM
>>>>>> To: Raja Zidane <rzidane@nvidia.com>; dev@dpdk.org
>>>>>> Cc: Matan Azrad <matan@nvidia.com>; 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 <rzidane@nvidia.com>
>>>>>>> ---
>>>>>>> Acked-by: Matan Azrad <matan@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) {