ixgbe: Replace LRO with GRO

On Wed, Jan 14, 2009 at 7:46 PM, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> On Wed, Jan 14, 2009 at 07:44:12PM +1100, Herbert Xu wrote:
>>
>> ixgbe: Replace LRO with GRO
>
> I forgot to delete the Kconfig dependency, here is an updated
> version.
>
> ixgbe: Replace LRO with GRO
>
> This patch makes igb invoke the GRO hooks instead of LRO.  As
> GRO has a compatible external interface to LRO this is a very
> straightforward replacement.
>
> As GRO uses the napi structure to track the held packets, I've
> modified the code paths involved to pass that along.
>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
>

Adding Emil to provide further testing details...

We are seeing data corruption with this patch applied.  When we
disable GRO the data corruption goes away.

We have this simple ftp test that does put/get and then compares
md5sum of the file. The file we get back is different then the one we
sent with GRO enabled.  The file is around 34MB. Note that we don't
always see the corruption with 30meg file, but it becomes more
apparent as we increase the size of the file.

If this helps - Emil did a diff off of a hexdump output between the
sent and received file. Looks like the receive was cut off at the end:

On Fri, Jan 16, 2009 at 03:32:56PM -0800, Jeff Kirsher wrote:
> 
> Adding Emil to provide further testing details...
> 
> We are seeing data corruption with this patch applied.  When we
> disable GRO the data corruption goes away.
> 
> We have this simple ftp test that does put/get and then compares
> md5sum of the file. The file we get back is different then the one we
> sent with GRO enabled.  The file is around 34MB. Note that we don't
> always see the corruption with 30meg file, but it becomes more
> apparent as we increase the size of the file.
> 
> If this helps - Emil did a diff off of a hexdump output between the
> sent and received file. Looks like the receive was cut off at the end:

Thanks for the info Jeff!

As the file isn't too big, could you do two packet dumps, one
at the sender and one at the receiver? Capturing just the headers
should be sufficient for now.

Cheers,

See attached.

Thanks,
Emil 

-----Original Message-----
From: Herbert Xu [mailto:herbert@gondor.apana.org.au] 
Sent: Friday, January 16, 2009 4:36 PM
To: Kirsher, Jeffrey T
Cc: David S. Miller; netdev@vger.kernel.org; Tantilov, Emil S
Subject: Re: ixgbe: Replace LRO with GRO

On Fri, Jan 16, 2009 at 03:32:56PM -0800, Jeff Kirsher wrote:
> 
> Adding Emil to provide further testing details...
> 
> We are seeing data corruption with this patch applied.  When we
> disable GRO the data corruption goes away.
> 
> We have this simple ftp test that does put/get and then compares
> md5sum of the file. The file we get back is different then the one we
> sent with GRO enabled.  The file is around 34MB. Note that we don't
> always see the corruption with 30meg file, but it becomes more
> apparent as we increase the size of the file.
> 
> If this helps - Emil did a diff off of a hexdump output between the
> sent and received file. Looks like the receive was cut off at the end:

Thanks for the info Jeff!

As the file isn't too big, could you do two packet dumps, one
at the sender and one at the receiver? Capturing just the headers
should be sufficient for now.

Cheers,

On Fri, Jan 16, 2009 at 06:06:18PM -0700, Tantilov, Emil S wrote:
> See attached.

Thanks Emil!

However, it's not very clear who is the sender in this case :)

FTP has a separate data connection, so the sender of the control
connection is not necessarily the sender of the data connection.

In fact, looking at the dump of the data connection it'd appear
that the one you marked as the sender dump is in fact the receiver
for the data connection:

12:07:17.045244 IP (tos 0x8, ttl 64, id 19084, offset 0, flags [DF], proto TCP (6), length 1500) 190.0.15.1.9504 > 190.0.15.3.41206: . 1:1449(1448) ack 1 win 46 <nop,nop,timestamp 199022661 4897363>
12:07:17.045260 IP (tos 0x8, ttl 64, id 15863, offset 0, flags [DF], proto TCP (6), length 52) 190.0.15.3.41206 > 190.0.15.1.9504: ., cksum 0x72a1 (correct), ack 1449 win 69 <nop,nop,timestamp 4897363 199022661>

We can see that because the delta between the data and the ack
is much smaller on this side.

It's also peculiar that there seems to be no GRO aggregation
at all on either side.  Were these dumps taken with it enabled
on both sides? You'll need a patched ethtool to check.

The transfer itself appears to be encountering a lot of packet loss,
which presumably caused a premature end which is why you saw a
missing chunk in the file.

However, it is not very clear what is causing this loss.  BTW,
did you apply the fix b0059b50b70dc3a908bea4ece2f9494a22200018
(gro: Fix page ref count for skbs freed normally) on both sides?
That one is required to make igb or ixgbe work properly.

Another thing to try is to disable GSO/TSO on both sides as that
is complicating the interpretation of the dumps.

Cheers,

Herbert Xu wrote:
> On Fri, Jan 16, 2009 at 06:06:18PM -0700, Tantilov, Emil S wrote:
>> See attached.
> 
> Thanks Emil!
> 
> However, it's not very clear who is the sender in this case :)
> 
> FTP has a separate data connection, so the sender of the control
> connection is not necessarily the sender of the data connection.
> 
> In fact, looking at the dump of the data connection it'd appear
> that the one you marked as the sender dump is in fact the receiver
> for the data connection:

The test is done by a script that:

1. checks the connection to the secondary system (ping)
2. copies the test file to the secondary test system (put)
3. copies the same file again from the secondary to the test system (get)
4. Compares the checksum of the sent vs. the received file

Since I had tcpdump on both ends - I think you should see both session in each. The file I named sender (probably a bit incorrect) is just the capture taken from the primary test system (the one that started the script). This is also the system on which I have the latest git with GRO.

> 12:07:17.045244 IP (tos 0x8, ttl 64, id 19084, offset 0, flags [DF],
> proto TCP (6), length 1500) 190.0.15.1.9504 > 190.0.15.3.41206: .
> 1:1449(1448) ack 1 win 46 <nop,nop,timestamp 199022661 4897363>
> 12:07:17.045260 IP (tos 0x8, ttl 64, id 15863, offset 0, flags [DF],
> proto TCP (6), length 52) 190.0.15.3.41206 > 190.0.15.1.9504: .,
> cksum 0x72a1 (correct), ack 1449 win 69 <nop,nop,timestamp 4897363
> 199022661>     
> 
> We can see that because the delta between the data and the ack
> is much smaller on this side.
> 
> It's also peculiar that there seems to be no GRO aggregation
> at all on either side.  Were these dumps taken with it enabled
> on both sides? You'll need a patched ethtool to check.

No only one of the systems (the sender) runs the latest git with GRO. The secondary test system runs on stock RHEL5.2 kernel.

> The transfer itself appears to be encountering a lot of packet loss,
> which presumably caused a premature end which is why you saw a
> missing chunk in the file.
> 
> However, it is not very clear what is causing this loss.  BTW,
> did you apply the fix b0059b50b70dc3a908bea4ece2f9494a22200018
> (gro: Fix page ref count for skbs freed normally) on both sides?
> That one is required to make igb or ixgbe work properly.
Ah - doesn't seem like I have this fix. Jeff will have to pull this in our test tree.

 
> Another thing to try is to disable GSO/TSO on both sides as that
> is complicating the interpretation of the dumps.
Will do if the fix above doesn't help.

Thanks,
Emil
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Sat, Jan 17, 2009 at 01:07:46AM -0700, Tantilov, Emil S wrote:
>
> The test is done by a script that:
> 
> 1. checks the connection to the secondary system (ping)
> 2. copies the test file to the secondary test system (put)
> 3. copies the same file again from the secondary to the test system (get)
> 4. Compares the checksum of the sent vs. the received file
> 
> Since I had tcpdump on both ends - I think you should see both session in each. The file I named sender (probably a bit incorrect) is just the capture taken from the primary test system (the one that started the script). This is also the system on which I have the latest git with GRO.

Thanks, that makes a lot more sense.  Just to confirm, the second
part of the sender dump should be the one showing GRO, right?

> > However, it is not very clear what is causing this loss.  BTW,
> > did you apply the fix b0059b50b70dc3a908bea4ece2f9494a22200018
> > (gro: Fix page ref count for skbs freed normally) on both sides?
> > That one is required to make igb or ixgbe work properly.
> Ah - doesn't seem like I have this fix. Jeff will have to pull this in our test tree.

OK, although this bug should only manifest itself if aggregation
occurs, which apparently isn't the case according to the dump.

> > Another thing to try is to disable GSO/TSO on both sides as that
> > is complicating the interpretation of the dumps.
> Will do if the fix above doesn't help.

BTW, when you're testing without GRO are you disabling GRO with
ethtool or loading an ixgbe driver without the patch? If the latter
please try it with ethtool (even if you don't have a patched ethtool
that supports GRO you can always disable RX checksums).

Maybe I just stuffed up the ixgbe patch.

Thanks,

Herbert Xu wrote:
> On Sat, Jan 17, 2009 at 01:07:46AM -0700, Tantilov, Emil S wrote:
>> 
>> The test is done by a script that:
>> 
>> 1. checks the connection to the secondary system (ping)
>> 2. copies the test file to the secondary test system (put)
>> 3. copies the same file again from the secondary to the test system
>> (get) 
>> 4. Compares the checksum of the sent vs. the received file
>> 
>> Since I had tcpdump on both ends - I think you should see both
>> session in each. The file I named sender (probably a bit incorrect)
>> is just the capture taken from the primary test system (the one that
>> started the script). This is also the system on which I have the
>> latest git with GRO.    
> 
> Thanks, that makes a lot more sense.  Just to confirm, the second
> part of the sender dump should be the one showing GRO, right?

Yes - the second part is the one where the primary test system is receiving (hence GRO).

 
>>> However, it is not very clear what is causing this loss.  BTW,
>>> did you apply the fix b0059b50b70dc3a908bea4ece2f9494a22200018
>>> (gro: Fix page ref count for skbs freed normally) on both sides?
>>> That one is required to make igb or ixgbe work properly.
>> Ah - doesn't seem like I have this fix. Jeff will have to pull this
>> in our test tree. 
> 
> OK, although this bug should only manifest itself if aggregation
> occurs, which apparently isn't the case according to the dump.
> 
>>> Another thing to try is to disable GSO/TSO on both sides as that
>>> is complicating the interpretation of the dumps.
>> Will do if the fix above doesn't help.
> 
> BTW, when you're testing without GRO are you disabling GRO with
> ethtool or loading an ixgbe driver without the patch? If the latter
> please try it with ethtool (even if you don't have a patched ethtool
> that supports GRO you can always disable RX checksums).
To disable GRO I just recompile ixgbe with "netdev->features |= NETIF_F_GRO" commented out. Is there a patch for ethtool? 

> 
> Maybe I just stuffed up the ixgbe patch.
> 
> Thanks,

Thanks for looking into it.
Emil--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html