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

[Matan Azrad <matan@nvidia.com>]

> -----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.

> 
> >>> 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) {
> >