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

["Singh, Aman Deep" <aman.deep.singh@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 11613 bytes --]

Hi Raja Zidane,

On 2/15/2022 8:01 PM, Raja Zidane wrote:
>
> Hi all,
> reviving the discussion.
>
> @Beilei Xing <mailto:beilei.xing@intel.com> could you please provide 
> info on what UDP destination ports are used for VxLan by i40 driver? 
> if its just the default then we can remove "RTE_ETH_IS_TUNNEL_PKT(pkt_type) == 0"

Right, we would like to remove the check "RTE_ETH_IS_TUNNEL_PKT(pkt_type) == 0" from parse_vxlan function.

And rather, add a default case with this check "RTE_ETH_IS_TUNNEL_PKT(pkt_type) == 0" at the end.
This default case can print an error stating "unknown tunnel pkt" with its udp_port parameters.

> *From:* Singh, Aman Deep <aman.deep.singh@intel.com>
> *Sent:* Monday, January 31, 2022 6:48 PM
> *To:* Raja Zidane <rzidane@nvidia.com>; Matan Azrad 
> <matan@nvidia.com>; Ferruh Yigit <ferruh.yigit@intel.com>; 
> dev@dpdk.org; Beilei Xing <beilei.xing@intel.com>; Qi Zhang 
> <qi.z.zhang@intel.com>
> *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/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@intel.com>  <mailto:aman.deep.singh@intel.com>  
>
>     Sent: Thursday, January 20, 2022 12:47 PM
>
>     To: Matan Azrad<matan@nvidia.com>  <mailto:matan@nvidia.com>; Ferruh Yigit<ferruh.yigit@intel.com>  <mailto:ferruh.yigit@intel.com>; Raja Zidane<rzidane@nvidia.com>  <mailto: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:
>
>           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@intel.com>  <mailto:ferruh.yigit@intel.com>
>
>             Sent: Tuesday, January 18, 2022 3:03 PM
>
>             To: Matan Azrad<matan@nvidia.com>  <mailto:matan@nvidia.com>; Raja Zidane<rzidane@nvidia.com>  <mailto: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>  <mailto:ferruh.yigit@intel.com>
>
>                     Sent: Tuesday, January 18, 2022 2:28 PM
>
>                     To: Matan Azrad<matan@nvidia.com>  <mailto:matan@nvidia.com>; Raja Zidane
>
>                     <rzidane@nvidia.com>  <mailto: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>  <mailto:ferruh.yigit@intel.com>
>
>                             Sent: Tuesday, January 18, 2022 11:52 AM
>
>                             To: Raja Zidane<rzidane@nvidia.com>  <mailto:rzidane@nvidia.com>;dev@dpdk.org
>
>                             Cc: Matan Azrad<matan@nvidia.com>  <mailto: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>  <mailto:rzidane@nvidia.com>
>
>                                 ---
>
>                                 Acked-by: Matan Azrad<matan@nvidia.com>  <mailto:matan@nvidia.com>
>
>                             Ack should be before '---' to be part of the commit log,
>
>                             otherwise it is dropped when applied as comment.
>

[-- Attachment #2: Type: text/html, Size: 27270 bytes --]