* [net PATCH 1/2] igb/igbvf: Don't use lco_csum to compute IPv4 checksum
2016-11-28 15:42 [net PATCH 0/2] Don't use lco_csum to compute IPv4 checksum Alexander Duyck
@ 2016-11-28 15:42 ` Alexander Duyck
2016-11-28 15:42 ` [net PATCH 2/2] ixgbe/ixgbevf: " Alexander Duyck
` (2 subsequent siblings)
3 siblings, 0 replies; 10+ messages in thread
From: Alexander Duyck @ 2016-11-28 15:42 UTC (permalink / raw)
To: netdev, intel-wired-lan; +Cc: sfr, davem, jeffrey.t.kirsher
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;
^ permalink raw reply related [flat|nested] 10+ messages in thread* [net PATCH 2/2] ixgbe/ixgbevf: Don't use lco_csum to compute IPv4 checksum
2016-11-28 15:42 [net PATCH 0/2] Don't use lco_csum to compute IPv4 checksum Alexander Duyck
2016-11-28 15:42 ` [net PATCH 1/2] igb/igbvf: " Alexander Duyck
@ 2016-11-28 15:42 ` Alexander Duyck
2016-11-28 22:26 ` [net PATCH 0/2] " Jeff Kirsher
2016-11-30 14:47 ` David Miller
3 siblings, 0 replies; 10+ messages in thread
From: Alexander Duyck @ 2016-11-28 15:42 UTC (permalink / raw)
To: netdev, intel-wired-lan; +Cc: sfr, davem, jeffrey.t.kirsher
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;
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [net PATCH 0/2] Don't use lco_csum to compute IPv4 checksum
2016-11-28 15:42 [net PATCH 0/2] Don't use lco_csum to compute IPv4 checksum Alexander Duyck
2016-11-28 15:42 ` [net PATCH 1/2] igb/igbvf: " Alexander Duyck
2016-11-28 15:42 ` [net PATCH 2/2] ixgbe/ixgbevf: " Alexander Duyck
@ 2016-11-28 22:26 ` Jeff Kirsher
2016-11-28 23:43 ` Stephen Rothwell
2016-11-30 14:47 ` David Miller
3 siblings, 1 reply; 10+ messages in thread
From: Jeff Kirsher @ 2016-11-28 22:26 UTC (permalink / raw)
To: sfr, netdev, intel-wired-lan; +Cc: Alexander Duyck, davem
[-- 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 --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [net PATCH 0/2] Don't use lco_csum to compute IPv4 checksum
2016-11-28 22:26 ` [net PATCH 0/2] " Jeff Kirsher
@ 2016-11-28 23:43 ` Stephen Rothwell
2016-11-29 9:07 ` Stephen Rothwell
0 siblings, 1 reply; 10+ messages in thread
From: Stephen Rothwell @ 2016-11-28 23:43 UTC (permalink / raw)
To: Jeff Kirsher; +Cc: netdev, intel-wired-lan, Alexander Duyck, davem
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [net PATCH 0/2] Don't use lco_csum to compute IPv4 checksum
2016-11-28 23:43 ` Stephen Rothwell
@ 2016-11-29 9:07 ` Stephen Rothwell
2017-01-11 23:19 ` Stephen Rothwell
0 siblings, 1 reply; 10+ messages in thread
From: Stephen Rothwell @ 2016-11-29 9:07 UTC (permalink / raw)
To: Jeff Kirsher
Cc: netdev, intel-wired-lan, Alexander Duyck, davem, Eric Dumazet,
Eli Cooper, Lance Richardson, Sven-Haegar Koch
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [net PATCH 0/2] Don't use lco_csum to compute IPv4 checksum
2016-11-29 9:07 ` Stephen Rothwell
@ 2017-01-11 23:19 ` Stephen Rothwell
0 siblings, 0 replies; 10+ messages in thread
From: Stephen Rothwell @ 2017-01-11 23:19 UTC (permalink / raw)
To: davem
Cc: Jeff Kirsher, netdev, intel-wired-lan, Alexander Duyck,
Eric Dumazet, Eli Cooper, Lance Richardson, Sven-Haegar Koch
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [net PATCH 0/2] Don't use lco_csum to compute IPv4 checksum
2016-11-28 15:42 [net PATCH 0/2] Don't use lco_csum to compute IPv4 checksum Alexander Duyck
` (2 preceding siblings ...)
2016-11-28 22:26 ` [net PATCH 0/2] " Jeff Kirsher
@ 2016-11-30 14:47 ` David Miller
2016-11-30 21:15 ` Jeff Kirsher
3 siblings, 1 reply; 10+ messages in thread
From: David Miller @ 2016-11-30 14:47 UTC (permalink / raw)
To: alexander.h.duyck; +Cc: netdev, intel-wired-lan, sfr, jeffrey.t.kirsher
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.
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [net PATCH 0/2] Don't use lco_csum to compute IPv4 checksum
2016-11-30 14:47 ` David Miller
@ 2016-11-30 21:15 ` Jeff Kirsher
2016-12-01 20:41 ` David Miller
0 siblings, 1 reply; 10+ messages in thread
From: Jeff Kirsher @ 2016-11-30 21:15 UTC (permalink / raw)
To: David Miller, alexander.h.duyck; +Cc: netdev, intel-wired-lan, sfr
[-- 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 --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [net PATCH 0/2] Don't use lco_csum to compute IPv4 checksum
2016-11-30 21:15 ` Jeff Kirsher
@ 2016-12-01 20:41 ` David Miller
0 siblings, 0 replies; 10+ messages in thread
From: David Miller @ 2016-12-01 20:41 UTC (permalink / raw)
To: jeffrey.t.kirsher; +Cc: alexander.h.duyck, netdev, intel-wired-lan, sfr
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.
^ permalink raw reply [flat|nested] 10+ messages in thread