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

[Ferruh Yigit <ferruh.yigit@intel.com>]

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?


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