fix some csum errors

PATCH In review Xiaoyun wang 2 participants 8 messages Oct 31, 2020 – Nov 4, 2020 Network drivers

Share & tools

Xiaoyun wang

author

2020-10-31 03:35 UTC #0 lore raw

cover fix some csum errors

This patch fixes outer_l3_len parse error when PKT_TX_OUTER_IP_CKSUM is not set, which does not affect checksum function, just be consistent with mbuf meta information description, and fixes SCTP checksum errors because driver doesn't pass payload offset info to hardware, which may cause SCTP checksum error.

v1:
- fix outer_l3_len parse error
- fix SCTP checksum error

Xiaoyun wang (2):
net/hinic: fix outer_l3_len parse error
net/hinic: fix SCTP checksum error

 drivers/net/hinic/hinic_pmd_tx.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

Signature · 1 line

1.8.3.1

Xiaoyun wang

author

2020-10-31 03:36 UTC #1 lore raw

PATCH 1/2 net/hinic: fix outer_l3_len parse error

This patch fixes outer_l3_len parse error when PKT_TX_OUTER_IP_CKSUM is not set, which does not affect checksum function, just be consistent with mbuf meta information description.

Fixes 8c8b61234ffd ("net/hinic: refactor checksum functions")

Cc [email protected]

Signed-off-by Xiaoyun wang <[email protected]>

---
 drivers/net/hinic/hinic_pmd_tx.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

drivers/net/hinic/hinic_pmd_tx.c +12 ~~-13~~

index 2dd4fe1..125627e 100644
@@ -779,26 +779,25 @@ static inline void hinic_analyze_tx_info(struct rte_mbuf *mbuf,
 {
 	struct rte_ether_hdr *eth_hdr;
 	struct rte_vlan_hdr *vlan_hdr;
-	struct rte_ipv4_hdr *ip4h;
-	u16 pkt_type;
-	u8 *hdr;
+	struct rte_ipv4_hdr *ipv4_hdr;
+	u16 eth_type;
 
-	hdr = (u8 *)rte_pktmbuf_mtod(mbuf, u8*);
-	eth_hdr = (struct rte_ether_hdr *)hdr;
-	pkt_type = rte_be_to_cpu_16(eth_hdr->ether_type);
+	eth_hdr = rte_pktmbuf_mtod(mbuf, struct rte_ether_hdr *);
+	eth_type = rte_be_to_cpu_16(eth_hdr->ether_type);
 
-	if (pkt_type == RTE_ETHER_TYPE_VLAN) {
+	if (eth_type == RTE_ETHER_TYPE_VLAN) {
 		off_info->outer_l2_len = ETHER_LEN_WITH_VLAN;
-		vlan_hdr = (struct rte_vlan_hdr *)(hdr + 1);
-		pkt_type = rte_be_to_cpu_16(vlan_hdr->eth_proto);
+		vlan_hdr = (struct rte_vlan_hdr *)(eth_hdr + 1);
+		eth_type = rte_be_to_cpu_16(vlan_hdr->eth_proto);
 	} else {
 		off_info->outer_l2_len = ETHER_LEN_NO_VLAN;
 	}
 
-	if (pkt_type == RTE_ETHER_TYPE_IPV4) {
-		ip4h = (struct rte_ipv4_hdr *)(hdr + off_info->outer_l2_len);
-		off_info->outer_l3_len = rte_ipv4_hdr_len(ip4h);
-	} else if (pkt_type == RTE_ETHER_TYPE_IPV6) {
+	if (eth_type == RTE_ETHER_TYPE_IPV4) {
+		ipv4_hdr = rte_pktmbuf_mtod_offset(mbuf, struct rte_ipv4_hdr *,
+						   off_info->outer_l2_len);
+		off_info->outer_l3_len = rte_ipv4_hdr_len(ipv4_hdr);
+	} else if (eth_type == RTE_ETHER_TYPE_IPV6) {
 		/* not support ipv6 extension header */
 		off_info->outer_l3_len = sizeof(struct rte_ipv6_hdr);
 	}

Signature · 1 line

1.8.3.1

Ferruh Yigit

2020-11-02 17:08 UTC #2 lore raw

Re: [dpdk-dev] [PATCH v1 1/2] net/hinic: fix outer_l3_len parse error

Quoted 46 lines · Xiaoyun wang

This patch fixes outer_l3_len parse error when PKT_TX_OUTER_IP_CKSUM is not set, which does not affect checksum function, just be consistent with mbuf meta information description.

Fixes 8c8b61234ffd ("net/hinic: refactor checksum functions")

Cc [email protected]

Signed-off-by Xiaoyun wang <[email protected]>

---
  drivers/net/hinic/hinic_pmd_tx.c | 25 ++++++++++++-------------
  1 file changed, 12 insertions(+), 13 deletions(-)

drivers/net/hinic/hinic_pmd_tx.c +7 ~~-11~~

index 2dd4fe1..125627e 100644
@@ -779,26 +779,25 @@ static inline void hinic_analyze_tx_info(struct rte_mbuf *mbuf,
  {
  	struct rte_ether_hdr *eth_hdr;
  	struct rte_vlan_hdr *vlan_hdr;
-	struct rte_ipv4_hdr *ip4h;
-	u16 pkt_type;
-	u8 *hdr;
+	struct rte_ipv4_hdr *ipv4_hdr;
+	u16 eth_type;
 
-	hdr = (u8 *)rte_pktmbuf_mtod(mbuf, u8*);
-	eth_hdr = (struct rte_ether_hdr *)hdr;
-	pkt_type = rte_be_to_cpu_16(eth_hdr->ether_type);
+	eth_hdr = rte_pktmbuf_mtod(mbuf, struct rte_ether_hdr *);
+	eth_type = rte_be_to_cpu_16(eth_hdr->ether_type);
 
-	if (pkt_type == RTE_ETHER_TYPE_VLAN) {
+	if (eth_type == RTE_ETHER_TYPE_VLAN) {
  		off_info->outer_l2_len = ETHER_LEN_WITH_VLAN;
-		vlan_hdr = (struct rte_vlan_hdr *)(hdr + 1);
-		pkt_type = rte_be_to_cpu_16(vlan_hdr->eth_proto);
+		vlan_hdr = (struct rte_vlan_hdr *)(eth_hdr + 1);
+		eth_type = rte_be_to_cpu_16(vlan_hdr->eth_proto);
  	} else {
  		off_info->outer_l2_len = ETHER_LEN_NO_VLAN;
  	}
 
-	if (pkt_type == RTE_ETHER_TYPE_IPV4) {
-		ip4h = (struct rte_ipv4_hdr *)(hdr + off_info->outer_l2_len);

drivers/net/hinic/hinic_pmd_tx.c static inline void hinic_analyze_tx_info(struct rte_mbuf *mbuf, · line 803

Quoted 10 lines

drivers/net/hinic/hinic_pmd_tx.c +5 -2

-		off_info->outer_l3_len = rte_ipv4_hdr_len(ip4h);
-	} else if (pkt_type == RTE_ETHER_TYPE_IPV6) {
+	if (eth_type == RTE_ETHER_TYPE_IPV4) {
+		ipv4_hdr = rte_pktmbuf_mtod_offset(mbuf, struct rte_ipv4_hdr *,
+						   off_info->outer_l2_len);
+		off_info->outer_l3_len = rte_ipv4_hdr_len(ipv4_hdr);
+	} else if (eth_type == RTE_ETHER_TYPE_IPV6) {
  		/* not support ipv6 extension header */
  		off_info->outer_l3_len = sizeof(struct rte_ipv6_hdr);
  	}

The actual fix is following [1] and rest is refactoring, right?
It is hard to catch the actual fix with refactoring, can you please describe the actual problem and fix in the commit log to clarify it?

[1]
@@ -789,7 +789,7 @@ static inline void hinic_analyze_tx_info(struct rte_mbuf
*mbuf,

         if (pkt_type == RTE_ETHER_TYPE_VLAN) {
                 off_info->outer_l2_len = ETHER_LEN_WITH_VLAN;
 -               vlan_hdr = (struct rte_vlan_hdr *)(hdr + 1);
 +               vlan_hdr = (struct rte_vlan_hdr *)(eth_hdr + 1);
                 pkt_type = rte_be_to_cpu_16(vlan_hdr->eth_proto);
         } else {
                 off_info->outer_l2_len = ETHER_LEN_NO_VLAN;

Ferruh Yigit

2020-11-02 17:26 UTC #3 lore raw

Re: [dpdk-dev] [PATCH v1 1/2] net/hinic: fix outer_l3_len parse error

Quoted 66 lines · Ferruh Yigit

Quoted 44 lines · Xiaoyun wang

This patch fixes outer_l3_len parse error when PKT_TX_OUTER_IP_CKSUM is not set, which does not affect checksum function, just be consistent with mbuf meta information description.

Fixes 8c8b61234ffd ("net/hinic: refactor checksum functions")

Cc [email protected]

Signed-off-by Xiaoyun wang <[email protected]>

---
drivers/net/hinic/hinic_pmd_tx.c | 25 ++++++++++++-------------

  1 file changed, 12 insertions(+), 13 deletions(-)

drivers/net/hinic/hinic_pmd_tx.c +7 ~~-12~~

index 2dd4fe1..125627e 100644
@@ -779,26 +779,25 @@ static inline void hinic_analyze_tx_info(struct rte_mbuf 
 *mbuf,
   {
       struct rte_ether_hdr *eth_hdr;
       struct rte_vlan_hdr *vlan_hdr;
-    struct rte_ipv4_hdr *ip4h;
-    u16 pkt_type;
-    u8 *hdr;
+    struct rte_ipv4_hdr *ipv4_hdr;
+    u16 eth_type;
-    hdr = (u8 *)rte_pktmbuf_mtod(mbuf, u8*);
-    eth_hdr = (struct rte_ether_hdr *)hdr;
-    pkt_type = rte_be_to_cpu_16(eth_hdr->ether_type);
+    eth_hdr = rte_pktmbuf_mtod(mbuf, struct rte_ether_hdr *);
+    eth_type = rte_be_to_cpu_16(eth_hdr->ether_type);
-    if (pkt_type == RTE_ETHER_TYPE_VLAN) {
+    if (eth_type == RTE_ETHER_TYPE_VLAN) {
           off_info->outer_l2_len = ETHER_LEN_WITH_VLAN;
-        vlan_hdr = (struct rte_vlan_hdr *)(hdr + 1);
-        pkt_type = rte_be_to_cpu_16(vlan_hdr->eth_proto);
+        vlan_hdr = (struct rte_vlan_hdr *)(eth_hdr + 1);
+        eth_type = rte_be_to_cpu_16(vlan_hdr->eth_proto);
       } else {
           off_info->outer_l2_len = ETHER_LEN_NO_VLAN;
       }
-    if (pkt_type == RTE_ETHER_TYPE_IPV4) {
-        ip4h = (struct rte_ipv4_hdr *)(hdr + off_info->outer_l2_len);
-        off_info->outer_l3_len = rte_ipv4_hdr_len(ip4h);

drivers/net/hinic/hinic_pmd_tx.c static inline void hinic_analyze_tx_info(struct rte_mbuf · line 802

Quoted 10 lines

drivers/net/hinic/hinic_pmd_tx.c +5 -1

-    } else if (pkt_type == RTE_ETHER_TYPE_IPV6) {
+    if (eth_type == RTE_ETHER_TYPE_IPV4) {
+        ipv4_hdr = rte_pktmbuf_mtod_offset(mbuf, struct rte_ipv4_hdr *,
+                           off_info->outer_l2_len);
+        off_info->outer_l3_len = rte_ipv4_hdr_len(ipv4_hdr);
+    } else if (eth_type == RTE_ETHER_TYPE_IPV6) {
           /* not support ipv6 extension header */
           off_info->outer_l3_len = sizeof(struct rte_ipv6_hdr);
       }

[1]
@@ -789,7 +789,7 @@ static inline void hinic_analyze_tx_info(struct rte_mbuf
*mbuf,

Quoted 7 lines

if (pkt_type == RTE_ETHER_TYPE_VLAN) { off_info->outer_l2_len = ETHER_LEN_WITH_VLAN; - vlan_hdr = (struct rte_vlan_hdr *)(hdr + 1); + vlan_hdr = (struct rte_vlan_hdr *)(eth_hdr + 1); pkt_type = rte_be_to_cpu_16(vlan_hdr->eth_proto); } else {
off_info->outer_l2_len = ETHER_LEN_NO_VLAN;

Added following to the commit log while merging, please correct it if it is wrong/missing:

The outer_l3_len is calculated wrong because 'vlan_hdr' is calculated wrong, 'vlan_hdr' fixed and code refactored.

Wangxiaoyun (Cloud)

author

2020-11-04 02:19 UTC #4 lore raw

Re: [dpdk-dev] [PATCH v1 1/2] net/hinic: fix outer_l3_len parse error

In reply to Ferruh Yigit

Yes,the actual fix is following as this patch:

commit 8c8b61234ffd9a283cecbf9751942cbdb87d68f6

Author: Xiaoyun Wang <[email protected]>
Date:   Sat Jul 25 16:15:33 2020 +0800

net/hinic: refactor checksum functions

Encapsulate different types of packet checksum preprocessing into functions.

Quoted 72 lines · Ferruh Yigit

Quoted 43 lines · Xiaoyun wang
This patch fixes outer_l3_len parse error when PKT_TX_OUTER_IP_CKSUM is not set, which does not affect checksum function, just be consistent with mbuf meta information description.
Fixes 8c8b61234ffd ("net/hinic: refactor checksum functions")
Cc [email protected]
Signed-off-by Xiaoyun wang <[email protected]>
---
drivers/net/hinic/hinic_pmd_tx.c | 25 ++++++++++++-------------
  1 file changed, 12 insertions(+), 13 deletions(-)
drivers/net/hinic/hinic_pmd_tx.c +7 ~~-12~~
index 2dd4fe1..125627e 100644
@@ -779,26 +779,25 @@ static inline void hinic_analyze_tx_info(struct rte_mbuf *mbuf,
   {
       struct rte_ether_hdr *eth_hdr;
       struct rte_vlan_hdr *vlan_hdr;
-    struct rte_ipv4_hdr *ip4h;
-    u16 pkt_type;
-    u8 *hdr;
+    struct rte_ipv4_hdr *ipv4_hdr;
+    u16 eth_type;
-    hdr = (u8 *)rte_pktmbuf_mtod(mbuf, u8*);
-    eth_hdr = (struct rte_ether_hdr *)hdr;
-    pkt_type = rte_be_to_cpu_16(eth_hdr->ether_type);
+    eth_hdr = rte_pktmbuf_mtod(mbuf, struct rte_ether_hdr *);
+    eth_type = rte_be_to_cpu_16(eth_hdr->ether_type);
-    if (pkt_type == RTE_ETHER_TYPE_VLAN) {
+    if (eth_type == RTE_ETHER_TYPE_VLAN) {
           off_info->outer_l2_len = ETHER_LEN_WITH_VLAN;
-        vlan_hdr = (struct rte_vlan_hdr *)(hdr + 1);
-        pkt_type = rte_be_to_cpu_16(vlan_hdr->eth_proto);
+        vlan_hdr = (struct rte_vlan_hdr *)(eth_hdr + 1);
+        eth_type = rte_be_to_cpu_16(vlan_hdr->eth_proto);
       } else {
           off_info->outer_l2_len = ETHER_LEN_NO_VLAN;
       }
-    if (pkt_type == RTE_ETHER_TYPE_IPV4) {
-        ip4h = (struct rte_ipv4_hdr *)(hdr + off_info->outer_l2_len);
-        off_info->outer_l3_len = rte_ipv4_hdr_len(ip4h);
drivers/net/hinic/hinic_pmd_tx.c static inline void hinic_analyze_tx_info(struct rte_mbuf *mbuf, · line 801
Quoted 10 lines
drivers/net/hinic/hinic_pmd_tx.c +5 -1
-    } else if (pkt_type == RTE_ETHER_TYPE_IPV6) {
+    if (eth_type == RTE_ETHER_TYPE_IPV4) {
+        ipv4_hdr = rte_pktmbuf_mtod_offset(mbuf, struct rte_ipv4_hdr *,
+                           off_info->outer_l2_len);
+        off_info->outer_l3_len = rte_ipv4_hdr_len(ipv4_hdr);
+    } else if (eth_type == RTE_ETHER_TYPE_IPV6) {
           /* not support ipv6 extension header */
           off_info->outer_l3_len = sizeof(struct rte_ipv6_hdr);
       }
 
The actual fix is following [1] and rest is refactoring, right?
It is hard to catch the actual fix with refactoring, can you please describe the actual problem and fix in the commit log to clarify it?
[1]
@@ -789,7 +789,7 @@ static inline void hinic_analyze_tx_info(struct rte_mbuf *mbuf,
if (pkt_type == RTE_ETHER_TYPE_VLAN) { off_info->outer_l2_len = ETHER_LEN_WITH_VLAN; - vlan_hdr = (struct rte_vlan_hdr *)(hdr + 1); + vlan_hdr = (struct rte_vlan_hdr *)(eth_hdr + 1); pkt_type = rte_be_to_cpu_16(vlan_hdr->eth_proto); } else {
off_info->outer_l2_len = ETHER_LEN_NO_VLAN;
.

Ferruh Yigit

2020-11-04 11:05 UTC #5 lore raw

Re: [dpdk-dev] [PATCH v1 1/2] net/hinic: fix outer_l3_len parse error

On 11/4/2020 2:19 AM, Wangxiaoyun (Cloud) wrote:

Quoted 76 lines · Ferruh Yigit

Quoted 65 lines · Xiaoyun wang
Quoted 44 lines
This patch fixes outer_l3_len parse error when PKT_TX_OUTER_IP_CKSUM is not set, which does not affect checksum function, just be consistent with mbuf meta information description.
Fixes 8c8b61234ffd ("net/hinic: refactor checksum functions")
Cc [email protected]
Signed-off-by Xiaoyun wang <[email protected]>
---
drivers/net/hinic/hinic_pmd_tx.c | 25 ++++++++++++-------------
  1 file changed, 12 insertions(+), 13 deletions(-)
drivers/net/hinic/hinic_pmd_tx.c +7 ~~-12~~
index 2dd4fe1..125627e 100644
@@ -779,26 +779,25 @@ static inline void hinic_analyze_tx_info(struct 
 rte_mbuf *mbuf,
   {
       struct rte_ether_hdr *eth_hdr;
       struct rte_vlan_hdr *vlan_hdr;
-    struct rte_ipv4_hdr *ip4h;
-    u16 pkt_type;
-    u8 *hdr;
+    struct rte_ipv4_hdr *ipv4_hdr;
+    u16 eth_type;
-    hdr = (u8 *)rte_pktmbuf_mtod(mbuf, u8*);
-    eth_hdr = (struct rte_ether_hdr *)hdr;
-    pkt_type = rte_be_to_cpu_16(eth_hdr->ether_type);
+    eth_hdr = rte_pktmbuf_mtod(mbuf, struct rte_ether_hdr *);
+    eth_type = rte_be_to_cpu_16(eth_hdr->ether_type);
-    if (pkt_type == RTE_ETHER_TYPE_VLAN) {
+    if (eth_type == RTE_ETHER_TYPE_VLAN) {
           off_info->outer_l2_len = ETHER_LEN_WITH_VLAN;
-        vlan_hdr = (struct rte_vlan_hdr *)(hdr + 1);
-        pkt_type = rte_be_to_cpu_16(vlan_hdr->eth_proto);
+        vlan_hdr = (struct rte_vlan_hdr *)(eth_hdr + 1);
+        eth_type = rte_be_to_cpu_16(vlan_hdr->eth_proto);
       } else {
           off_info->outer_l2_len = ETHER_LEN_NO_VLAN;
       }
-    if (pkt_type == RTE_ETHER_TYPE_IPV4) {
-        ip4h = (struct rte_ipv4_hdr *)(hdr + off_info->outer_l2_len);
-        off_info->outer_l3_len = rte_ipv4_hdr_len(ip4h);
drivers/net/hinic/hinic_pmd_tx.c static inline void hinic_analyze_tx_info(struct · line 802
Quoted 10 lines
drivers/net/hinic/hinic_pmd_tx.c +5 -1
-    } else if (pkt_type == RTE_ETHER_TYPE_IPV6) {
+    if (eth_type == RTE_ETHER_TYPE_IPV4) {
+        ipv4_hdr = rte_pktmbuf_mtod_offset(mbuf, struct rte_ipv4_hdr *,
+                           off_info->outer_l2_len);
+        off_info->outer_l3_len = rte_ipv4_hdr_len(ipv4_hdr);
+    } else if (eth_type == RTE_ETHER_TYPE_IPV6) {
           /* not support ipv6 extension header */
           off_info->outer_l3_len = sizeof(struct rte_ipv6_hdr);
       }
 
The actual fix is following [1] and rest is refactoring, right?
It is hard to catch the actual fix with refactoring, can you please describe the actual problem and fix in the commit log to clarify it?
[1]
@@ -789,7 +789,7 @@ static inline void hinic_analyze_tx_info(struct rte_mbuf *mbuf,
Quoted 8 lines
if (pkt_type == RTE_ETHER_TYPE_VLAN) { off_info->outer_l2_len = ETHER_LEN_WITH_VLAN; - vlan_hdr = (struct rte_vlan_hdr *)(hdr + 1); + vlan_hdr = (struct rte_vlan_hdr *)(eth_hdr + 1); pkt_type = rte_be_to_cpu_16(vlan_hdr->eth_proto); } else {
off_info->outer_l2_len = ETHER_LEN_NO_VLAN; .
Yes,the actual fix is following as this patch:

Quoted 10 lines

commit 8c8b61234ffd9a283cecbf9751942cbdb87d68f6
Author: Xiaoyun Wang <[email protected]>
Date:   Sat Jul 25 16:15:33 2020 +0800
net/hinic: refactor checksum functions
Encapsulate different types of packet checksum preprocessing into functions.

I am not asking the original patch that introduced the problem. In this patch, the single line I put above is the only required change, right?

Xiaoyun wang

author

2020-10-31 03:35 UTC #6 lore raw

PATCH 2/2 net/hinic: fix SCTP checksum error

In reply to Xiaoyun wang

For SCTP checksum offload, pmd driver does not parse payload offset info, which may cause hardware calculate SCTP checksum failed.

Fixes 8c8b61234ffd ("net/hinic: refactor checksum functions")

Cc [email protected]

Signed-off-by Xiaoyun wang <[email protected]>

---
 drivers/net/hinic/hinic_pmd_tx.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

drivers/net/hinic/hinic_pmd_tx.c +2 -1

index 125627e..9d0264e 100644
@@ -767,7 +767,8 @@ static inline void hinic_get_pld_offset(struct rte_mbuf *m,
 {
 	uint64_t ol_flags = m->ol_flags;
 
-	if ((ol_flags & PKT_TX_L4_MASK) == PKT_TX_UDP_CKSUM)
+	if (((ol_flags & PKT_TX_L4_MASK) == PKT_TX_UDP_CKSUM) ||
+	    ((ol_flags & PKT_TX_L4_MASK) == PKT_TX_SCTP_CKSUM))
 		off_info->payload_offset = m->l2_len + m->l3_len;
 	else if ((ol_flags & PKT_TX_TCP_CKSUM) || (ol_flags & PKT_TX_TCP_SEG))
 		off_info->payload_offset = m->l2_len + m->l3_len +

Signature · 1 line

1.8.3.1

Ferruh Yigit

2020-11-02 17:48 UTC #7 lore raw

In reply to Xiaoyun wang

Quoted 9 lines · Xiaoyun wang

This patch fixes outer_l3_len parse error when PKT_TX_OUTER_IP_CKSUM is not set, which does not affect checksum function, just be consistent with mbuf meta information description, and fixes SCTP checksum errors because driver doesn't pass payload offset info to hardware, which may cause SCTP checksum error.
--

Quoted 8 lines

v1:
- fix outer_l3_len parse error
- fix SCTP checksum error
Xiaoyun wang (2):
net/hinic: fix outer_l3_len parse error
net/hinic: fix SCTP checksum error

Series applied to dpdk-next-net/main, thanks.

Also can you please review a long waiting [1] hinic patch:
https://patches.dpdk.org/patch/83421/

[1]
Current patch doesn't have version information but it is continuation of the patch from early August:
https://patches.dpdk.org/project/dpdk/list/?series=&submitter=1866&state=*&q=net%2Fhinic&archive=&delegate=