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

[Singh, Aman Deep]

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<aman.deep.singh at intel.com>  
> Sent: Thursday, January 20, 2022 12:47 PM
> To: Matan Azrad<matan at nvidia.com>; Ferruh Yigit<ferruh.yigit at intel.com>; Raja Zidane<rzidane at nvidia.com>;dev at dpdk.org
> Cc:stable at 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<ferruh.yigit at intel.com>
>>> Sent: Tuesday, January 18, 2022 3:03 PM
>>> To: Matan Azrad<matan at nvidia.com>; Raja Zidane<rzidane at nvidia.com>;
>>> dev at dpdk.org
>>> Cc:stable at 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 at intel.com>
>>>>> Sent: Tuesday, January 18, 2022 2:28 PM
>>>>> To: Matan Azrad<matan at nvidia.com>; Raja Zidane
>>>>> <rzidane at nvidia.com>;dev at dpdk.org
>>>>> Cc:stable at 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 at intel.com>
>>>>>>> Sent: Tuesday, January 18, 2022 11:52 AM
>>>>>>> To: Raja Zidane<rzidane at nvidia.com>;dev at dpdk.org
>>>>>>> Cc: Matan Azrad<matan at nvidia.com>;stable at 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 at 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 at dpdk.org
>>>>>>>>
>>>>>>>> Signed-off-by: Raja Zidane<rzidane at nvidia.com>
>>>>>>>> ---
>>>>>>>> Acked-by: Matan Azrad<matan at nvidia.com>
>>>>>>> Ack should be before '---' to be part of the commit log,
>>>>>>> otherwise it is dropped when applied as comment.
>>>>>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mails.dpdk.org/archives/stable/attachments/20220131/b96a330a/attachment.htm>