[net PATCH 0/2] Don't use lco_csum to compute IPv4 checksum

[net PATCH 0/2] Don't use lco_csum to compute IPv4 checksum

[Alexander Duyck]

When I implemented the GSO partial support in the Intel drivers I was using
lco_csum to compute the checksum that we needed to plug into the IPv4
checksum field in order to cancel out the data that was not a part of the
IPv4 header.  However this didn't take into account that the transport
offset might be pointing to the inner transport header.

Instead of using lco_csum I have just coded around it so that we can use
the outer IP header plus the IP header length to determine where we need to
start our checksum and then just call csum_partial ourselves.

This should fix the SIT issue reported on igb interfaces as well as simliar
issues that would pop up on other Intel NICs.

---

Alexander Duyck (2):
      igb/igbvf: Don't use lco_csum to compute IPv4 checksum
      ixgbe/ixgbevf: Don't use lco_csum to compute IPv4 checksum


 drivers/net/ethernet/intel/igb/igb_main.c         |    8 ++++++--
 drivers/net/ethernet/intel/igbvf/netdev.c         |    8 ++++++--
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c     |    8 ++++++--
 drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c |    8 ++++++--
 4 files changed, 24 insertions(+), 8 deletions(-)

[net PATCH 1/2] igb/igbvf: Don't use lco_csum to compute IPv4 checksum

[Alexander Duyck]

In the case of IPIP and SIT tunnel frames the outer transport header
offset is actually set to the same offset as the inner transport header.
This results in the lco_csum call not doing any checksum computation over
the inner IPv4/v6 header data.

In order to account for that I am updating the code so that we determine
the location to start the checksum ourselves based on the location of the
IPv4 header and the length.

Fixes: e10715d3e961 ("igb/igbvf: Add support for GSO partial")
Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
---
 drivers/net/ethernet/intel/igb/igb_main.c |    8 ++++++--
 drivers/net/ethernet/intel/igbvf/netdev.c |    8 ++++++--
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index 59125b1..76ce0cc 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -4922,11 +4922,15 @@ static int igb_tso(struct igb_ring *tx_ring,
 
 	/* initialize outer IP header fields */
 	if (ip.v4->version == 4) {
+		unsigned char *csum_start = skb_checksum_start(skb);
+		unsigned char *trans_start = ip.hdr + (ip.v4->ihl * 4);
+
 		/* IP header will have to cancel out any data that
 		 * is not a part of the outer IP header
 		 */
-		ip.v4->check = csum_fold(csum_add(lco_csum(skb),
-						  csum_unfold(l4.tcp->check)));
+		ip.v4->check = csum_fold(csum_partial(trans_start,
+						      csum_start - trans_start,
+						      0));
 		type_tucmd |= E1000_ADVTXD_TUCMD_IPV4;
 
 		ip.v4->tot_len = 0;
diff --git a/drivers/net/ethernet/intel/igbvf/netdev.c b/drivers/net/ethernet/intel/igbvf/netdev.c
index b0778ba..e3a80ac 100644
--- a/drivers/net/ethernet/intel/igbvf/netdev.c
+++ b/drivers/net/ethernet/intel/igbvf/netdev.c
@@ -1965,11 +1965,15 @@ static int igbvf_tso(struct igbvf_ring *tx_ring,
 
 	/* initialize outer IP header fields */
 	if (ip.v4->version == 4) {
+		unsigned char *csum_start = skb_checksum_start(skb);
+		unsigned char *trans_start = ip.hdr + (ip.v4->ihl * 4);
+
 		/* IP header will have to cancel out any data that
 		 * is not a part of the outer IP header
 		 */
-		ip.v4->check = csum_fold(csum_add(lco_csum(skb),
-						  csum_unfold(l4.tcp->check)));
+		ip.v4->check = csum_fold(csum_partial(trans_start,
+						      csum_start - trans_start,
+						      0));
 		type_tucmd |= E1000_ADVTXD_TUCMD_IPV4;
 
 		ip.v4->tot_len = 0;

[net PATCH 2/2] ixgbe/ixgbevf: Don't use lco_csum to compute IPv4 checksum

[Alexander Duyck]

In the case of IPIP and SIT tunnel frames the outer transport header
offset is actually set to the same offset as the inner transport header.
This results in the lco_csum call not doing any checksum computation over
the inner IPv4/v6 header data.

In order to account for that I am updating the code so that we determine
the location to start the checksum ourselves based on the location of the
IPv4 header and the length.

Fixes: b83e30104bd9 ("ixgbe/ixgbevf: Add support for GSO partial")
Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c     |    8 ++++++--
 drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c |    8 ++++++--
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index b4f0374..c72f783 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -7284,11 +7284,15 @@ static int ixgbe_tso(struct ixgbe_ring *tx_ring,
 
 	/* initialize outer IP header fields */
 	if (ip.v4->version == 4) {
+		unsigned char *csum_start = skb_checksum_start(skb);
+		unsigned char *trans_start = ip.hdr + (ip.v4->ihl * 4);
+
 		/* IP header will have to cancel out any data that
 		 * is not a part of the outer IP header
 		 */
-		ip.v4->check = csum_fold(csum_add(lco_csum(skb),
-						  csum_unfold(l4.tcp->check)));
+		ip.v4->check = csum_fold(csum_partial(trans_start,
+						      csum_start - trans_start,
+						      0));
 		type_tucmd |= IXGBE_ADVTXD_TUCMD_IPV4;
 
 		ip.v4->tot_len = 0;
diff --git a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
index d9d6616..baa885e 100644
--- a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
+++ b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
@@ -3326,11 +3326,15 @@ static int ixgbevf_tso(struct ixgbevf_ring *tx_ring,
 
 	/* initialize outer IP header fields */
 	if (ip.v4->version == 4) {
+		unsigned char *csum_start = skb_checksum_start(skb);
+		unsigned char *trans_start = ip.hdr + (ip.v4->ihl * 4);
+
 		/* IP header will have to cancel out any data that
 		 * is not a part of the outer IP header
 		 */
-		ip.v4->check = csum_fold(csum_add(lco_csum(skb),
-						  csum_unfold(l4.tcp->check)));
+		ip.v4->check = csum_fold(csum_partial(trans_start,
+						      csum_start - trans_start,
+						      0));
 		type_tucmd |= IXGBE_ADVTXD_TUCMD_IPV4;
 
 		ip.v4->tot_len = 0;

Re: [net PATCH 0/2] Don't use lco_csum to compute IPv4 checksum

[Jeff Kirsher]

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

On Mon, 2016-11-28 at 10:42 -0500, Alexander Duyck wrote:
> When I implemented the GSO partial support in the Intel drivers I was
> using
> lco_csum to compute the checksum that we needed to plug into the IPv4
> checksum field in order to cancel out the data that was not a part of the
> IPv4 header.  However this didn't take into account that the transport
> offset might be pointing to the inner transport header.
> 
> Instead of using lco_csum I have just coded around it so that we can use
> the outer IP header plus the IP header length to determine where we need
> to
> start our checksum and then just call csum_partial ourselves.
> 
> This should fix the SIT issue reported on igb interfaces as well as
> simliar
> issues that would pop up on other Intel NICs.
> 
> ---
> 
> Alexander Duyck (2):
>       igb/igbvf: Don't use lco_csum to compute IPv4 checksum
>       ixgbe/ixgbevf: Don't use lco_csum to compute IPv4 checksum

Stephen, I have applied Alex's patches to my net-queue tree.  Can you
confirm they resolve the bug seen?

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [net PATCH 0/2] Don't use lco_csum to compute IPv4 checksum

[Stephen Rothwell]

Hi Jeff,

On Mon, 28 Nov 2016 14:26:02 -0800 Jeff Kirsher <jeffrey.t.kirsher@intel.com> wrote:
>
> On Mon, 2016-11-28 at 10:42 -0500, Alexander Duyck wrote:
> > When I implemented the GSO partial support in the Intel drivers I was
> > using
> > lco_csum to compute the checksum that we needed to plug into the IPv4
> > checksum field in order to cancel out the data that was not a part of the
> > IPv4 header.  However this didn't take into account that the transport
> > offset might be pointing to the inner transport header.
> > 
> > Instead of using lco_csum I have just coded around it so that we can use
> > the outer IP header plus the IP header length to determine where we need
> > to
> > start our checksum and then just call csum_partial ourselves.
> > 
> > This should fix the SIT issue reported on igb interfaces as well as
> > simliar
> > issues that would pop up on other Intel NICs.
> > 
> > ---
> > 
> > Alexander Duyck (2):
> >       igb/igbvf: Don't use lco_csum to compute IPv4 checksum
> >       ixgbe/ixgbevf: Don't use lco_csum to compute IPv4 checksum  
> 
> Stephen, I have applied Alex's patches to my net-queue tree.  Can you
> confirm they resolve the bug seen?

Its a bit tricky because the origin problem only happens on my
production server (ozlabs.org), but I will see if I can manage to just
remove and reload the driver ...  though, the server is running a 4.7.8
kernel and I am wondering how well these patches will apply?

-- 
Cheers,
Stephen Rothwell

Re: [net PATCH 0/2] Don't use lco_csum to compute IPv4 checksum

[Stephen Rothwell]

Hi Jeff,

On Tue, 29 Nov 2016 10:43:04 +1100 Stephen Rothwell <sfr@canb.auug.org.au> wrote:
>
> On Mon, 28 Nov 2016 14:26:02 -0800 Jeff Kirsher <jeffrey.t.kirsher@intel.com> wrote:
> >
> > On Mon, 2016-11-28 at 10:42 -0500, Alexander Duyck wrote:  
> > > When I implemented the GSO partial support in the Intel drivers I was
> > > using
> > > lco_csum to compute the checksum that we needed to plug into the IPv4
> > > checksum field in order to cancel out the data that was not a part of the
> > > IPv4 header.  However this didn't take into account that the transport
> > > offset might be pointing to the inner transport header.
> > > 
> > > Instead of using lco_csum I have just coded around it so that we can use
> > > the outer IP header plus the IP header length to determine where we need
> > > to
> > > start our checksum and then just call csum_partial ourselves.
> > > 
> > > This should fix the SIT issue reported on igb interfaces as well as
> > > simliar
> > > issues that would pop up on other Intel NICs.
> > > 
> > > ---
> > > 
> > > Alexander Duyck (2):
> > >       igb/igbvf: Don't use lco_csum to compute IPv4 checksum
> > >       ixgbe/ixgbevf: Don't use lco_csum to compute IPv4 checksum    
> > 
> > Stephen, I have applied Alex's patches to my net-queue tree.  Can you
> > confirm they resolve the bug seen?  
> 
> Its a bit tricky because the origin problem only happens on my
> production server (ozlabs.org), but I will see if I can manage to just
> remove and reload the driver ...  though, the server is running a 4.7.8
> kernel and I am wondering how well these patches will apply?

We have a winner!  This fixes my problem, so I can run at full speed
with gso and tso enabled in the sit interface and tx-gso-partial
enabled on the underlying ethernet.

Thanks to everyone for diagnosis and solution.

It would be nice if this fix went into the stable kernels as well so it
will turn up in the distro kernels eventually.
-- 
Cheers,
Stephen Rothwell

Re: [net PATCH 0/2] Don't use lco_csum to compute IPv4 checksum

[David Miller]

From: Alexander Duyck <alexander.h.duyck@intel.com>
Date: Mon, 28 Nov 2016 10:42:18 -0500

> When I implemented the GSO partial support in the Intel drivers I was using
> lco_csum to compute the checksum that we needed to plug into the IPv4
> checksum field in order to cancel out the data that was not a part of the
> IPv4 header.  However this didn't take into account that the transport
> offset might be pointing to the inner transport header.
> 
> Instead of using lco_csum I have just coded around it so that we can use
> the outer IP header plus the IP header length to determine where we need to
> start our checksum and then just call csum_partial ourselves.
> 
> This should fix the SIT issue reported on igb interfaces as well as simliar
> issues that would pop up on other Intel NICs.

Jeff, are you going to send me a pull request with this stuff or would
you be OK with my applying these directly to 'net'?

Thanks.

Re: [net PATCH 0/2] Don't use lco_csum to compute IPv4 checksum

[Jeff Kirsher]

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

On Wed, 2016-11-30 at 09:47 -0500, David Miller wrote:
> From: Alexander Duyck <alexander.h.duyck@intel.com>
> Date: Mon, 28 Nov 2016 10:42:18 -0500
> 
> > When I implemented the GSO partial support in the Intel drivers I was
> using
> > lco_csum to compute the checksum that we needed to plug into the IPv4
> > checksum field in order to cancel out the data that was not a part of
> the
> > IPv4 header.  However this didn't take into account that the transport
> > offset might be pointing to the inner transport header.
> > 
> > Instead of using lco_csum I have just coded around it so that we can
> use
> > the outer IP header plus the IP header length to determine where we
> need to
> > start our checksum and then just call csum_partial ourselves.
> > 
> > This should fix the SIT issue reported on igb interfaces as well as
> simliar
> > issues that would pop up on other Intel NICs.
> 
> Jeff, are you going to send me a pull request with this stuff or would
> you be OK with my applying these directly to 'net'?

Go ahead and apply those to your net tree, I do not want to hold this up.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [net PATCH 0/2] Don't use lco_csum to compute IPv4 checksum

[David Miller]

From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Date: Wed, 30 Nov 2016 13:15:22 -0800

> On Wed, 2016-11-30 at 09:47 -0500, David Miller wrote:
>> From: Alexander Duyck <alexander.h.duyck@intel.com>
>> Date: Mon, 28 Nov 2016 10:42:18 -0500
>> 
>> > When I implemented the GSO partial support in the Intel drivers I was
>> using
>> > lco_csum to compute the checksum that we needed to plug into the IPv4
>> > checksum field in order to cancel out the data that was not a part of
>> the
>> > IPv4 header.  However this didn't take into account that the transport
>> > offset might be pointing to the inner transport header.
>> > 
>> > Instead of using lco_csum I have just coded around it so that we can
>> use
>> > the outer IP header plus the IP header length to determine where we
>> need to
>> > start our checksum and then just call csum_partial ourselves.
>> > 
>> > This should fix the SIT issue reported on igb interfaces as well as
>> simliar
>> > issues that would pop up on other Intel NICs.
>> 
>> Jeff, are you going to send me a pull request with this stuff or would
>> you be OK with my applying these directly to 'net'?
> 
> Go ahead and apply those to your net tree, I do not want to hold this up.

Ok, done, thanks Jeff.

Re: [net PATCH 0/2] Don't use lco_csum to compute IPv4 checksum

[Stephen Rothwell]

Hi Dave,

On Tue, 29 Nov 2016 20:07:08 +1100 Stephen Rothwell <sfr@canb.auug.org.au> wrote:
>
> Hi Jeff,
> 
> On Tue, 29 Nov 2016 10:43:04 +1100 Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> >
> > On Mon, 28 Nov 2016 14:26:02 -0800 Jeff Kirsher <jeffrey.t.kirsher@intel.com> wrote:  
> > >
> > > On Mon, 2016-11-28 at 10:42 -0500, Alexander Duyck wrote:    
> > > > When I implemented the GSO partial support in the Intel drivers I was
> > > > using
> > > > lco_csum to compute the checksum that we needed to plug into the IPv4
> > > > checksum field in order to cancel out the data that was not a part of the
> > > > IPv4 header.  However this didn't take into account that the transport
> > > > offset might be pointing to the inner transport header.
> > > > 
> > > > Instead of using lco_csum I have just coded around it so that we can use
> > > > the outer IP header plus the IP header length to determine where we need
> > > > to
> > > > start our checksum and then just call csum_partial ourselves.
> > > > 
> > > > This should fix the SIT issue reported on igb interfaces as well as
> > > > simliar
> > > > issues that would pop up on other Intel NICs.
> > > > 
> > > > ---
> > > > 
> > > > Alexander Duyck (2):
> > > >       igb/igbvf: Don't use lco_csum to compute IPv4 checksum
> > > >       ixgbe/ixgbevf: Don't use lco_csum to compute IPv4 checksum      
> > > 
> > > Stephen, I have applied Alex's patches to my net-queue tree.  Can you
> > > confirm they resolve the bug seen?    
> > 
> > Its a bit tricky because the origin problem only happens on my
> > production server (ozlabs.org), but I will see if I can manage to just
> > remove and reload the driver ...  though, the server is running a 4.7.8
> > kernel and I am wondering how well these patches will apply?  
> 
> We have a winner!  This fixes my problem, so I can run at full speed
> with gso and tso enabled in the sit interface and tx-gso-partial
> enabled on the underlying ethernet.
> 
> Thanks to everyone for diagnosis and solution.
> 
> It would be nice if this fix went into the stable kernels as well so it
> will turn up in the distro kernels eventually.

It looks like these 2 commits:

516165a1e2f2	igb/igbvf: Don't use lco_csum to compute IPv4 checksum
c54cdc316dbd	ixgbe/ixgbevf: Don't use lco_csum to compute IPv4 checksum

have not been sent to stable.  They fix a bug introduced in v4.7-rc1
and so need to go into 4.7 and 4.8 stable trees.

-- 
Cheers,
Stephen Rothwell