diff mbox series

[v2] net: prepare the outer ipv4 hdr for checksum

Message ID 20210630110404.21209-1-mohsin.kazmi14@gmail.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series [v2] net: prepare the outer ipv4 hdr for checksum |

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/github-robot success github build: passed
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-testing fail Testing issues
ci/iol-mellanox-Functional fail Functional Testing issues
ci/iol-intel-Performance fail Performance Testing issues
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/iol-abi-testing success Testing PASS

Commit Message

Mohsin Kazmi June 30, 2021, 11:04 a.m. UTC 
  Preparation the headers for the hardware offload
misses the outer ipv4 checksum offload.
It results in bad checksum computed by hardware NIC.

This patch fixes the issue by setting the outer ipv4
checksum field to 0.

Fixes: 4fb7e803eb1a ("ethdev: add Tx preparation")
Cc: stable@dpdk.org

Signed-off-by: Mohsin Kazmi <mohsin.kazmi14@gmail.com>
Acked-by: Qi Zhang <qi.z.zhang@intel.com>
---

v2:
* Update the commit message with Fixes.
---
 lib/net/rte_net.h | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

Comments

Olivier Matz June 30, 2021, 2:09 p.m. UTC | #1 
Hi Mohsin,

Hope you are fine!
Please see my comments below.

On Wed, Jun 30, 2021 at 01:04:04PM +0200, Mohsin Kazmi wrote:
> Re: [PATCH v2] net: prepare the outer ipv4 hdr for checksum

I suggest to highlight that it this is the Intel-specific tx-prepare
function in the commit title. What about:

  net: fix Intel-specific Tx preparation for outer checksums

> Preparation the headers for the hardware offload
> misses the outer ipv4 checksum offload.
> It results in bad checksum computed by hardware NIC.
> 
> This patch fixes the issue by setting the outer ipv4
> checksum field to 0.
> 
> Fixes: 4fb7e803eb1a ("ethdev: add Tx preparation")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Mohsin Kazmi <mohsin.kazmi14@gmail.com>
> Acked-by: Qi Zhang <qi.z.zhang@intel.com>
> ---
> 
> v2:
> * Update the commit message with Fixes.
> ---
>  lib/net/rte_net.h | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/net/rte_net.h b/lib/net/rte_net.h
> index 434435ffa2..e47365099e 100644
> --- a/lib/net/rte_net.h
> +++ b/lib/net/rte_net.h
> @@ -128,8 +128,18 @@ rte_net_intel_cksum_flags_prepare(struct rte_mbuf *m, uint64_t ol_flags)
>  	if (!(ol_flags & (PKT_TX_IP_CKSUM | PKT_TX_L4_MASK | PKT_TX_TCP_SEG)))
>  		return 0;

I think this test should be updated too with PKT_TX_OUTER_IP_CKSUM.

>  
> -	if (ol_flags & (PKT_TX_OUTER_IPV4 | PKT_TX_OUTER_IPV6))
> +	if (ol_flags & (PKT_TX_OUTER_IPV4 | PKT_TX_OUTER_IPV6)) {
>  		inner_l3_offset += m->outer_l2_len + m->outer_l3_len;
> +		/*
> +		 * prepare outer ipv4 header checksum by setting it to 0,
> +		 * in order to be computed by hardware NICs.
> +		 */
> +		if (ol_flags & PKT_TX_OUTER_IP_CKSUM) {
> +			ipv4_hdr = rte_pktmbuf_mtod_offset(m,
> +					struct rte_ipv4_hdr *, m->outer_l2_len);
> +			ipv4_hdr->hdr_checksum = 0;
> +		}
> +	}

What about outer L4 checksum? Does it requires the same than inner?

>  
>  	/*
>  	 * Check if headers are fragmented.
> -- 
> 2.17.1
>
Mohsin Kazmi July 7, 2021, 9:14 a.m. UTC | #2 
Hi Olivier,

Thanks for the review.
Please find the comments inline below:

On Wed, Jun 30, 2021 at 3:09 PM Olivier Matz <olivier.matz@6wind.com> wrote:

> Hi Mohsin,
>
> Hope you are fine!
> Please see my comments below.
>
> On Wed, Jun 30, 2021 at 01:04:04PM +0200, Mohsin Kazmi wrote:
> > Re: [PATCH v2] net: prepare the outer ipv4 hdr for checksum
>
> I suggest to highlight that it this is the Intel-specific tx-prepare
> function in the commit title. What about:
>
  net: fix Intel-specific Tx preparation for outer checksums
>
> I'll update the commit title as suggested.

> Preparation the headers for the hardware offload
> > misses the outer ipv4 checksum offload.
> > It results in bad checksum computed by hardware NIC.
> >
> > This patch fixes the issue by setting the outer ipv4
> > checksum field to 0.
> >
> > Fixes: 4fb7e803eb1a ("ethdev: add Tx preparation")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Mohsin Kazmi <mohsin.kazmi14@gmail.com>
> > Acked-by: Qi Zhang <qi.z.zhang@intel.com>
> > ---
> >
> > v2:
> > * Update the commit message with Fixes.
> > ---
> >  lib/net/rte_net.h | 12 +++++++++++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/net/rte_net.h b/lib/net/rte_net.h
> > index 434435ffa2..e47365099e 100644
> > --- a/lib/net/rte_net.h
> > +++ b/lib/net/rte_net.h
> > @@ -128,8 +128,18 @@ rte_net_intel_cksum_flags_prepare(struct rte_mbuf
> *m, uint64_t ol_flags)
> >       if (!(ol_flags & (PKT_TX_IP_CKSUM | PKT_TX_L4_MASK |
> PKT_TX_TCP_SEG)))
> >               return 0;
>
> I think this test should be updated too with PKT_TX_OUTER_IP_CKSUM.
>
Thanks. Yes, I'll update it too.

>
> >
> > -     if (ol_flags & (PKT_TX_OUTER_IPV4 | PKT_TX_OUTER_IPV6))
> > +     if (ol_flags & (PKT_TX_OUTER_IPV4 | PKT_TX_OUTER_IPV6)) {
> >               inner_l3_offset += m->outer_l2_len + m->outer_l3_len;
> > +             /*
> > +              * prepare outer ipv4 header checksum by setting it to 0,
> > +              * in order to be computed by hardware NICs.
> > +              */
> > +             if (ol_flags & PKT_TX_OUTER_IP_CKSUM) {
> > +                     ipv4_hdr = rte_pktmbuf_mtod_offset(m,
> > +                                     struct rte_ipv4_hdr *,
> m->outer_l2_len);
> > +                     ipv4_hdr->hdr_checksum = 0;
> > +             }
> > +     }
>
> What about outer L4 checksum? Does it requires the same than inner?
>
I am using XL710 for my testing with i40e dpdk driver. AFAIK, It doesn't
support outer l4 checksum. I am not sure if other Intel NICs support it.


> >
> >       /*
> >        * Check if headers are fragmented.
> > --
> > 2.17.1
> >
>
Thomas Monjalon July 22, 2021, 7:53 p.m. UTC | #3 
07/07/2021 11:14, Mohsin Kazmi:
> On Wed, Jun 30, 2021 at 3:09 PM Olivier Matz <olivier.matz@6wind.com> wrote:
> > > +     if (ol_flags & (PKT_TX_OUTER_IPV4 | PKT_TX_OUTER_IPV6)) {
> > >               inner_l3_offset += m->outer_l2_len + m->outer_l3_len;
> > > +             /*
> > > +              * prepare outer ipv4 header checksum by setting it to 0,
> > > +              * in order to be computed by hardware NICs.
> > > +              */
> > > +             if (ol_flags & PKT_TX_OUTER_IP_CKSUM) {
> > > +                     ipv4_hdr = rte_pktmbuf_mtod_offset(m,
> > > +                                     struct rte_ipv4_hdr *,
> > m->outer_l2_len);
> > > +                     ipv4_hdr->hdr_checksum = 0;
> > > +             }
> > > +     }
> >
> > What about outer L4 checksum? Does it requires the same than inner?
> >
> I am using XL710 for my testing with i40e dpdk driver. AFAIK, It doesn't
> support outer l4 checksum. I am not sure if other Intel NICs support it.

This function is used by a lot of drivers.
Try git grep rte_net_intel_cksum_prepare

I think we need more reviews on the v3.
Given it is far from being a new bug, I suggest to wait the next release
in order to have more feedbacks.
Mohsin Kazmi Aug. 3, 2021, 12:29 p.m. UTC | #4 
Hi Thomas,

Thanks for the review.

I did the git grep rte_net_intel_cksum_prepare and git grep
PKT_TX_OUTER_UDP_CKSUM. Following are the two drivers that use the function
to prepare headers for checksum which also uses the outer_udp_checksum
offload within drivers.

1) Hisilicon hns3

2) Wangxun txgbe

1) has implemented its own version of functions to prepare for outer header
checksum. It may benefit/impact from the change.

The function "rte_net_intel_cksum_prepare" is intel specific and intel
cards do not support outer l4 checksum offload. DPDK may provide a generic
version of the same function which can be used in different drivers.

-br
Mohsin

On Thu, Jul 22, 2021 at 8:53 PM Thomas Monjalon <thomas@monjalon.net> wrote:

> 07/07/2021 11:14, Mohsin Kazmi:
> > On Wed, Jun 30, 2021 at 3:09 PM Olivier Matz <olivier.matz@6wind.com>
> wrote:
> > > > +     if (ol_flags & (PKT_TX_OUTER_IPV4 | PKT_TX_OUTER_IPV6)) {
> > > >               inner_l3_offset += m->outer_l2_len + m->outer_l3_len;
> > > > +             /*
> > > > +              * prepare outer ipv4 header checksum by setting it to
> 0,
> > > > +              * in order to be computed by hardware NICs.
> > > > +              */
> > > > +             if (ol_flags & PKT_TX_OUTER_IP_CKSUM) {
> > > > +                     ipv4_hdr = rte_pktmbuf_mtod_offset(m,
> > > > +                                     struct rte_ipv4_hdr *,
> > > m->outer_l2_len);
> > > > +                     ipv4_hdr->hdr_checksum = 0;
> > > > +             }
> > > > +     }
> > >
> > > What about outer L4 checksum? Does it requires the same than inner?
> > >
> > I am using XL710 for my testing with i40e dpdk driver. AFAIK, It doesn't
> > support outer l4 checksum. I am not sure if other Intel NICs support it.
>
> This function is used by a lot of drivers.
> Try git grep rte_net_intel_cksum_prepare
>
> I think we need more reviews on the v3.
> Given it is far from being a new bug, I suggest to wait the next release
> in order to have more feedbacks.
>
>
>
diff mbox series

Patch

diff --git a/lib/net/rte_net.h b/lib/net/rte_net.h
index 434435ffa2..e47365099e 100644
--- a/lib/net/rte_net.h
+++ b/lib/net/rte_net.h
@@ -128,8 +128,18 @@  rte_net_intel_cksum_flags_prepare(struct rte_mbuf *m, uint64_t ol_flags)
 	if (!(ol_flags & (PKT_TX_IP_CKSUM | PKT_TX_L4_MASK | PKT_TX_TCP_SEG)))
 		return 0;
 
-	if (ol_flags & (PKT_TX_OUTER_IPV4 | PKT_TX_OUTER_IPV6))
+	if (ol_flags & (PKT_TX_OUTER_IPV4 | PKT_TX_OUTER_IPV6)) {
 		inner_l3_offset += m->outer_l2_len + m->outer_l3_len;
+		/*
+		 * prepare outer ipv4 header checksum by setting it to 0,
+		 * in order to be computed by hardware NICs.
+		 */
+		if (ol_flags & PKT_TX_OUTER_IP_CKSUM) {
+			ipv4_hdr = rte_pktmbuf_mtod_offset(m,
+					struct rte_ipv4_hdr *, m->outer_l2_len);
+			ipv4_hdr->hdr_checksum = 0;
+		}
+	}
 
 	/*
 	 * Check if headers are fragmented.