diff mbox series

app/testpmd: fix IPv6 tunnel packet checksum error

Message ID 20230530153603.105257-1-shiyangx.he@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers
Series app/testpmd: fix IPv6 tunnel packet checksum error |

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/loongarch-compilation success Compilation OK
ci/loongarch-unit-testing success Unit Testing PASS
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/github-robot: build success github build: passed
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-x86_64-compile-testing success Testing PASS
ci/iol-abi-testing success Testing PASS
ci/iol-aarch64-unit-testing success Testing PASS
ci/iol-testing warning Testing issues
ci/iol-x86_64-unit-testing success Testing PASS
ci/iol-aarch64-compile-testing success Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-unit-testing success Testing PASS

Commit Message

Shiyang He May 30, 2023, 3:36 p.m. UTC 
  In checksum forwarding mode, the checksum of tunnel packet calculated
incorrectly when outer header is IPv6.

This patch fixes the issue by setting L4 checksum flag.

Fixes: daa02b5cddbb ("mbuf: add namespace to offload flags")
Cc: stable@dpdk.org

Signed-off-by: Shiyang He <shiyangx.he@intel.com>
---
 app/test-pmd/csumonly.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Ferruh Yigit June 1, 2023, 10:09 p.m. UTC | #1 
On 5/30/2023 4:36 PM, Shiyang He wrote:
> In checksum forwarding mode, the checksum of tunnel packet calculated
> incorrectly when outer header is IPv6.
> 
> This patch fixes the issue by setting L4 checksum flag.
> 
> Fixes: daa02b5cddbb ("mbuf: add namespace to offload flags")

This commit just updates the flag name, actual commit that introduce the
commit should be something else.

Probably:
Fixes: c14236f210d8 ("mbuf: replace inner fields by outer fields semantic")
Cc: jijiang.liu@intel.com


> Cc: stable@dpdk.org
> 
> Signed-off-by: Shiyang He <shiyangx.he@intel.com>
> ---
>  app/test-pmd/csumonly.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
> index fc85c22a77..bd2fccc458 100644
> --- a/app/test-pmd/csumonly.c
> +++ b/app/test-pmd/csumonly.c
> @@ -582,7 +582,7 @@ process_outer_cksums(void *outer_l3_hdr, struct testpmd_offload_info *info,
>  		else
>  			ipv4_hdr->hdr_checksum = rte_ipv4_cksum(ipv4_hdr);
>  	} else
> -		ol_flags |= RTE_MBUF_F_TX_OUTER_IPV6;
> +		ol_flags |= RTE_MBUF_F_TX_OUTER_IPV6 | RTE_MBUF_F_TX_L4_MASK;
>  

When 'or' with MASK, it is same as requesting UDP checksum.
```
#define RTE_MBUF_F_TX_UDP_CKSUM     (3ULL << 52)
#define RTE_MBUF_F_TX_L4_MASK       (3ULL << 52)
```

Can you please describe more the problem, is it UDP checksum missing
(since ipv6 itself doesn't have checksum)?
If so, shouldn't code check first that if UDP checksum offload is requested?
Can you please describe why/how above code change is fixing the issue?
Shiyang He June 6, 2023, 9:39 a.m. UTC | #2 
>-----Original Message-----
>From: Ferruh Yigit <ferruh.yigit@amd.com>
>Sent: Friday, June 2, 2023 6:10 AM
>To: He, ShiyangX <shiyangx.he@intel.com>; dev@dpdk.org
>Cc: Zhou, YidingX <yidingx.zhou@intel.com>; stable@dpdk.org; Singh, Aman
>Deep <aman.deep.singh@intel.com>; Zhang, Yuying
><yuying.zhang@intel.com>; Somnath Kotur
><somnath.kotur@broadcom.com>; Andrew Rybchenko
><andrew.rybchenko@oktetlabs.ru>; Matz, Olivier <olivier.matz@6wind.com>;
>Ajit Khaparde <ajit.khaparde@broadcom.com>
>Subject: Re: [PATCH] app/testpmd: fix IPv6 tunnel packet checksum error
>
>On 5/30/2023 4:36 PM, Shiyang He wrote:
>> In checksum forwarding mode, the checksum of tunnel packet calculated
>> incorrectly when outer header is IPv6.
>>
>> This patch fixes the issue by setting L4 checksum flag.
>>
>> Fixes: daa02b5cddbb ("mbuf: add namespace to offload flags")
>
>This commit just updates the flag name, actual commit that introduce the
>commit should be something else.
>
>Probably:
>Fixes: c14236f210d8 ("mbuf: replace inner fields by outer fields semantic")
>Cc: jijiang.liu@intel.com
>
>
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Shiyang He <shiyangx.he@intel.com>
>> ---
>>  app/test-pmd/csumonly.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c index
>> fc85c22a77..bd2fccc458 100644
>> --- a/app/test-pmd/csumonly.c
>> +++ b/app/test-pmd/csumonly.c
>> @@ -582,7 +582,7 @@ process_outer_cksums(void *outer_l3_hdr, struct
>testpmd_offload_info *info,
>>  		else
>>  			ipv4_hdr->hdr_checksum =
>rte_ipv4_cksum(ipv4_hdr);
>>  	} else
>> -		ol_flags |= RTE_MBUF_F_TX_OUTER_IPV6;
>> +		ol_flags |= RTE_MBUF_F_TX_OUTER_IPV6 |
>RTE_MBUF_F_TX_L4_MASK;
>>
>
>When 'or' with MASK, it is same as requesting UDP checksum.
>```
>#define RTE_MBUF_F_TX_UDP_CKSUM     (3ULL << 52)
>#define RTE_MBUF_F_TX_L4_MASK       (3ULL << 52)
>```
>
>Can you please describe more the problem, is it UDP checksum missing (since
>ipv6 itself doesn't have checksum)?
>If so, shouldn't code check first that if UDP checksum offload is requested?
>Can you please describe why/how above code change is fixing the issue?

Sorry for the late reply!
I re-debugged the issue again and found that it was caused by PMD.
I will resubmit the v2 patch!
diff mbox series

Patch

diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
index fc85c22a77..bd2fccc458 100644
--- a/app/test-pmd/csumonly.c
+++ b/app/test-pmd/csumonly.c
@@ -582,7 +582,7 @@  process_outer_cksums(void *outer_l3_hdr, struct testpmd_offload_info *info,
 		else
 			ipv4_hdr->hdr_checksum = rte_ipv4_cksum(ipv4_hdr);
 	} else
-		ol_flags |= RTE_MBUF_F_TX_OUTER_IPV6;
+		ol_flags |= RTE_MBUF_F_TX_OUTER_IPV6 | RTE_MBUF_F_TX_L4_MASK;
 
 	if (info->outer_l4_proto != IPPROTO_UDP)
 		return ol_flags;