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

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

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


On 2/16/2022 6:07 PM, 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.
> 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.
>
> Remove the fall back case to VxLan.
> Log error of unrecognized tunnel if no tunnel was parsed successfully.
>
> Fixes: ea0e711b8ae0 ("app/testpmd: add GENEVE parsing")
> Cc:stable@dpdk.org
>
> Signed-off-by: Raja Zidane<rzidane@nvidia.com>
> ---
> V2: Log error when an unrecognized tunnel is found (unknown UDP dst port), instead of parsing it as VxLan by default.
>
>   app/test-pmd/csumonly.c | 23 +++++++++++++----------
>   1 file changed, 13 insertions(+), 10 deletions(-)
>
> diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
> index 02bc3929c7..210f4263f8 100644
> --- a/app/test-pmd/csumonly.c
> +++ b/app/test-pmd/csumonly.c
> @@ -255,11 +255,10 @@ parse_gtp(struct rte_udp_hdr *udp_hdr,
>   	info->l2_len += RTE_ETHER_GTP_HLEN;
>   }
>   
> -/* Parse a vxlan header */
> +/* Parse a vxlan header. */
>   static void
>   parse_vxlan(struct rte_udp_hdr *udp_hdr,
> -	    struct testpmd_offload_info *info,
> -	    uint32_t pkt_type)
> +	    struct testpmd_offload_info *info)
>   {
>   	struct rte_ether_hdr *eth_hdr;
>   
> @@ -267,8 +266,7 @@ parse_vxlan(struct rte_udp_hdr *udp_hdr,
>   	 * default vxlan port (rfc7348) or that the rx offload flag is set
>   	 * (i40e only currently)
>   	 */
> -	if (udp_hdr->dst_port != _htons(RTE_VXLAN_DEFAULT_PORT) &&
> -		RTE_ETH_IS_TUNNEL_PKT(pkt_type) == 0)
> +	if (udp_hdr->dst_port != _htons(RTE_VXLAN_DEFAULT_PORT))
>   		return;
>   
>   	update_tunnel_outer(info);
> @@ -922,19 +920,24 @@ 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);
> +				parse_vxlan(udp_hdr, &info);
>   				if (info.is_tunnel) {
>   					tx_ol_flags |=
> -						RTE_MBUF_F_TX_TUNNEL_GENEVE;
> +						RTE_MBUF_F_TX_TUNNEL_VXLAN;

After adding default case, i suppose swapping of parse_vxlan & parse_geneve is not needed.
Can we revert these above changes.

>   					goto tunnel_update;
>   				}
> +				/* Always keep last. */
> +				if (unlikely(RTE_ETH_IS_TUNNEL_PKT(
> +							m->packet_type) != 0)) {
> +					TESTPMD_LOG(ERR, "Unknown tunnel packet. UDP dst port: %hu",
> +						udp_hdr->dst_port);
> +				}
>   			} else if (info.l4_proto == IPPROTO_GRE) {
>   				struct simple_gre_hdr *gre_hdr;
>   

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