[net PATCH] i40e/i40evf: Limit TSO to 7 descriptors for payload instead of 8 per packet

This patch addresses a bug introduced based on my interpretation of the
XL710 datasheet.  Specifically section 8.4.1 states that "A single transmit
packet may span up to 8 buffers (up to 8 data descriptors per packet
including both the header and payload buffers)."  It then later goes on to
say that each segment for a TSO obeys the previous rule, however it then
refers to TSO header and the segment payload buffers.

I believe the actual limit for fragments with TSO and a skbuff that has
payload data in the header portion of the buffer is actually only 7
fragments as the skb->data portion counts as 2 buffers, one for the TSO
header, and one for a segment payload buffer.

Fixes: 2d37490b82af ("i40e/i40evf: Rewrite logic for 8 descriptor per packet check")
Signed-off-by: Alexander Duyck <redacted>
---

This patch has been sanity checked only.  I cannot yet guarantee it
resolves the original issue that was reported.  I'll try to get a
reproduction environment setup tomorrow but I don't know how long that
should take.

 drivers/net/ethernet/intel/i40e/i40e_txrx.c   |   40 ++++++++++++++-----------
 drivers/net/ethernet/intel/i40evf/i40e_txrx.c |   40 ++++++++++++++-----------
 2 files changed, 44 insertions(+), 36 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
index 5d5fa5359a1d..97437f04d99d 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_txrx.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.c

@@ -2597,12 +2597,17 @@ int __i40e_maybe_stop_tx(struct i40e_ring *tx_ring, int size)
 }
 
 /**
- * __i40e_chk_linearize - Check if there are more than 8 fragments per packet
+ * __i40e_chk_linearize - Check if there are more than 8 buffers per packet
  * @skb:      send buffer
  *
- * Note: Our HW can't scatter-gather more than 8 fragments to build
- * a packet on the wire and so we need to figure out the cases where we
- * need to linearize the skb.
+ * Note: Our HW can't DMA more than 8 buffers to build a packet on the wire
+ * and so we need to figure out the cases where we need to linearize the skb.
+ *
+ * For TSO we need to count the TSO header and segment payload separately.
+ * As such we need to check cases where we have 7 fragments or more as we
+ * can potentially require 9 DMA transactions, 1 for the TSO header, 1 for
+ * the segment payload in the first descriptor, and another 7 for the
+ * fragments.
  **/
 bool __i40e_chk_linearize(struct sk_buff *skb)
 {

@@ -2614,18 +2619,17 @@ bool __i40e_chk_linearize(struct sk_buff *skb)
 	if (unlikely(!gso_size))
 		return true;
 
-	/* no need to check if number of frags is less than 8 */
+	/* no need to check if number of frags is less than 7 */
 	nr_frags = skb_shinfo(skb)->nr_frags;
-	if (nr_frags < I40E_MAX_BUFFER_TXD)
+	if (nr_frags < (I40E_MAX_BUFFER_TXD - 1))
 		return false;
 
 	/* We need to walk through the list and validate that each group
 	 * of 6 fragments totals at least gso_size.  However we don't need
-	 * to perform such validation on the first or last 6 since the first
-	 * 6 cannot inherit any data from a descriptor before them, and the
-	 * last 6 cannot inherit any data from a descriptor after them.
+	 * to perform such validation on the last 6 since the last 6 cannot
+	 * inherit any data from a descriptor after them.
 	 */
-	nr_frags -= I40E_MAX_BUFFER_TXD - 1;
+	nr_frags -= I40E_MAX_BUFFER_TXD - 2;
 	frag = &skb_shinfo(skb)->frags[0];
 
 	/* Initialize size to the negative value of gso_size minus 1.  We

@@ -2636,19 +2640,19 @@ bool __i40e_chk_linearize(struct sk_buff *skb)
 	 */
 	sum = 1 - gso_size;
 
-	/* Add size of frags 1 through 5 to create our initial sum */
-	sum += skb_frag_size(++frag);
-	sum += skb_frag_size(++frag);
-	sum += skb_frag_size(++frag);
-	sum += skb_frag_size(++frag);
-	sum += skb_frag_size(++frag);
+	/* Add size of frags 0 through 4 to create our initial sum */
+	sum += skb_frag_size(frag++);
+	sum += skb_frag_size(frag++);
+	sum += skb_frag_size(frag++);
+	sum += skb_frag_size(frag++);
+	sum += skb_frag_size(frag++);
 
 	/* Walk through fragments adding latest fragment, testing it, and
 	 * then removing stale fragments from the sum.
 	 */
 	stale = &skb_shinfo(skb)->frags[0];
 	for (;;) {
-		sum += skb_frag_size(++frag);
+		sum += skb_frag_size(frag++);
 
 		/* if sum is negative we failed to make sufficient progress */
 		if (sum < 0)

@@ -2658,7 +2662,7 @@ bool __i40e_chk_linearize(struct sk_buff *skb)
 		if (!--nr_frags)
 			break;
 
-		sum -= skb_frag_size(++stale);
+		sum -= skb_frag_size(stale++);
 	}
 
 	return false;

diff --git a/drivers/net/ethernet/intel/i40evf/i40e_txrx.c b/drivers/net/ethernet/intel/i40evf/i40e_txrx.c
index 04aabc52ba0d..240e4a1b2507 100644
--- a/drivers/net/ethernet/intel/i40evf/i40e_txrx.c
+++ b/drivers/net/ethernet/intel/i40evf/i40e_txrx.c

@@ -1799,12 +1799,17 @@ static void i40e_create_tx_ctx(struct i40e_ring *tx_ring,
 }
 
 /**
- * __i40evf_chk_linearize - Check if there are more than 8 fragments per packet
+ * __i40evf_chk_linearize - Check if there are more than 8 buffers per packet
  * @skb:      send buffer
  *
- * Note: Our HW can't scatter-gather more than 8 fragments to build
- * a packet on the wire and so we need to figure out the cases where we
- * need to linearize the skb.
+ * Note: Our HW can't DMA more than 8 buffers to build a packet on the wire
+ * and so we need to figure out the cases where we need to linearize the skb.
+ *
+ * For TSO we need to count the TSO header and segment payload separately.
+ * As such we need to check cases where we have 7 fragments or more as we
+ * can potentially require 9 DMA transactions, 1 for the TSO header, 1 for
+ * the segment payload in the first descriptor, and another 7 for the
+ * fragments.
  **/
 bool __i40evf_chk_linearize(struct sk_buff *skb)
 {

@@ -1816,18 +1821,17 @@ bool __i40evf_chk_linearize(struct sk_buff *skb)
 	if (unlikely(!gso_size))
 		return true;
 
-	/* no need to check if number of frags is less than 8 */
+	/* no need to check if number of frags is less than 7 */
 	nr_frags = skb_shinfo(skb)->nr_frags;
-	if (nr_frags < I40E_MAX_BUFFER_TXD)
+	if (nr_frags < (I40E_MAX_BUFFER_TXD - 1))
 		return false;
 
 	/* We need to walk through the list and validate that each group
 	 * of 6 fragments totals at least gso_size.  However we don't need
-	 * to perform such validation on the first or last 6 since the first
-	 * 6 cannot inherit any data from a descriptor before them, and the
-	 * last 6 cannot inherit any data from a descriptor after them.
+	 * to perform such validation on the last 6 since the last 6 cannot
+	 * inherit any data from a descriptor after them.
 	 */
-	nr_frags -= I40E_MAX_BUFFER_TXD - 1;
+	nr_frags -= I40E_MAX_BUFFER_TXD - 2;
 	frag = &skb_shinfo(skb)->frags[0];
 
 	/* Initialize size to the negative value of gso_size minus 1.  We

@@ -1838,19 +1842,19 @@ bool __i40evf_chk_linearize(struct sk_buff *skb)
 	 */
 	sum = 1 - gso_size;
 
-	/* Add size of frags 1 through 5 to create our initial sum */
-	sum += skb_frag_size(++frag);
-	sum += skb_frag_size(++frag);
-	sum += skb_frag_size(++frag);
-	sum += skb_frag_size(++frag);
-	sum += skb_frag_size(++frag);
+	/* Add size of frags 0 through 4 to create our initial sum */
+	sum += skb_frag_size(frag++);
+	sum += skb_frag_size(frag++);
+	sum += skb_frag_size(frag++);
+	sum += skb_frag_size(frag++);
+	sum += skb_frag_size(frag++);
 
 	/* Walk through fragments adding latest fragment, testing it, and
 	 * then removing stale fragments from the sum.
 	 */
 	stale = &skb_shinfo(skb)->frags[0];
 	for (;;) {
-		sum += skb_frag_size(++frag);
+		sum += skb_frag_size(frag++);
 
 		/* if sum is negative we failed to make sufficient progress */
 		if (sum < 0)

@@ -1860,7 +1864,7 @@ bool __i40evf_chk_linearize(struct sk_buff *skb)
 		if (!--nr_frags)
 			break;
 
-		sum -= skb_frag_size(++stale);
+		sum -= skb_frag_size(stale++);
 	}
 
 	return false;

On (03/29/16 23:44), Alexander Duyck wrote:

This patch has been sanity checked only.  I cannot yet guarantee it
resolves the original issue that was reported.  I'll try to get a
reproduction environment setup tomorrow but I don't know how long that
should take.

I tried this out with rds-stress on my test-pair, unfortunately, I
still see the Tx hang.

Setting up the test is quite easy- for reference, the instructions 
are here:
   https://sourceforge.net/p/e1000/mailman/message/34936766/

--Sowmini

On Wed, Mar 30, 2016 at 10:00 AM, Sowmini Varadhan
[off-list ref] wrote:

On (03/29/16 23:44), Alexander Duyck wrote:

quoted

This patch has been sanity checked only.  I cannot yet guarantee it
resolves the original issue that was reported.  I'll try to get a
reproduction environment setup tomorrow but I don't know how long that
should take.

I tried this out with rds-stress on my test-pair, unfortunately, I
still see the Tx hang.

Setting up the test is quite easy- for reference, the instructions
are here:
   https://sourceforge.net/p/e1000/mailman/message/34936766/

Yeah.  The patch was sort of a knee-jerk reaction to being told that
the patch referenced caused a regression.  From what I can tell that
is not the case as I am also seeing the Tx hangs when I run the test
with the frames being linearized.

I'll do some research this morning to see if I can find a root cause.
Unfortunately the malicious driver detection isn't very well
documented so I can't be certain what is causing it to be triggered.

- Alex

On (03/30/16 10:12), Alexander Duyck wrote:

Yeah.  The patch was sort of a knee-jerk reaction to being told that
the patch referenced caused a regression.  From what I can tell that
is not the case as I am also seeing the Tx hangs when I run the test
with the frames being linearized.

I'm not sure how important of a subtlety this is, but the actual
console log after the patch is the following:

 i40e 0000:82:00.0: TX driver issue detected, PF reset issued
 i40e 0000:82:00.0 eth2: adding 68:05:ca:30:dd:18 vid=0
 i40e 0000:82:00.0: TX driver issue detected, PF reset issued
 i40e 0000:82:00.0 eth2: adding 68:05:ca:30:dd:18 vid=0
 i40e 0000:82:00.0: TX driver issue detected, PF reset issued

Comparing with what I'd pasted in the sourceforge thread earlier,
I see that it does not say "Hung Tx queue etc."  any more, though
it still resets.

Not sure if that changed info is significant?

--Sowmini

On Wed, Mar 30, 2016 at 10:20 AM, Sowmini Varadhan
[off-list ref] wrote:

On (03/30/16 10:12), Alexander Duyck wrote:

quoted

Yeah.  The patch was sort of a knee-jerk reaction to being told that
the patch referenced caused a regression.  From what I can tell that
is not the case as I am also seeing the Tx hangs when I run the test
with the frames being linearized.

I'm not sure how important of a subtlety this is, but the actual
console log after the patch is the following:

 i40e 0000:82:00.0: TX driver issue detected, PF reset issued
 i40e 0000:82:00.0 eth2: adding 68:05:ca:30:dd:18 vid=0
 i40e 0000:82:00.0: TX driver issue detected, PF reset issued
 i40e 0000:82:00.0 eth2: adding 68:05:ca:30:dd:18 vid=0
 i40e 0000:82:00.0: TX driver issue detected, PF reset issued

Comparing with what I'd pasted in the sourceforge thread earlier,
I see that it does not say "Hung Tx queue etc."  any more, though
it still resets.

Not sure if that changed info is significant?

It might be.  Right now I am chasing down the Tx driver issue as that
I what I am reproducing in my environment as well.

From what I can tell by enabling msglvl tx_err it is reporting MDD

event 0x2.  Unfortunately the documentation doesn't say what that is
so I am checking a few different possibilities.

- Alex

On Wed, 30 Mar 2016 10:12:51 -0700
Alexander Duyck [off-list ref] wrote:

On Wed, Mar 30, 2016 at 10:00 AM, Sowmini Varadhan
[off-list ref] wrote:

quoted

On (03/29/16 23:44), Alexander Duyck wrote:

quoted

This patch has been sanity checked only.  I cannot yet guarantee it
resolves the original issue that was reported.  I'll try to get a
reproduction environment setup tomorrow but I don't know how long that
should take.

I tried this out with rds-stress on my test-pair, unfortunately, I
still see the Tx hang.

Setting up the test is quite easy- for reference, the instructions
are here:
   https://sourceforge.net/p/e1000/mailman/message/34936766/

Yeah.  The patch was sort of a knee-jerk reaction to being told that
the patch referenced caused a regression.  From what I can tell that

Thanks for working so hard on the patch Alex, I need to apologize, as
the original test appears to fail as well with 1.3.46-k (a previous
driver to your patch) and I thought we had already tested that, but I
was wrong.

This is not a regression, but likely just an undetected "bug" that we
need to work out.

is not the case as I am also seeing the Tx hangs when I run the test
with the frames being linearized.

That doesn't make much sense unless it is something about how we are
setting up the offload.  I troubleshoot by disabling the PFR from the
MDD code, then disabling tx timeout via debugfs, and using debugfs to
dump the descriptor ring after the MDD event fires.
 

I'll do some research this morning to see if I can find a root cause.
Unfortunately the malicious driver detection isn't very well
documented so I can't be certain what is causing it to be triggered.

I'm still looking at this too and appreciate the help.

On Wed, 30 Mar 2016 10:35:55 -0700
Alexander Duyck [off-list ref] wrote:

On Wed, Mar 30, 2016 at 10:20 AM, Sowmini Varadhan
[off-list ref] wrote:

quoted

On (03/30/16 10:12), Alexander Duyck wrote:

quoted

Yeah.  The patch was sort of a knee-jerk reaction to being told that
the patch referenced caused a regression.  From what I can tell that
is not the case as I am also seeing the Tx hangs when I run the test
with the frames being linearized.

I'm not sure how important of a subtlety this is, but the actual
console log after the patch is the following:

 i40e 0000:82:00.0: TX driver issue detected, PF reset issued
 i40e 0000:82:00.0 eth2: adding 68:05:ca:30:dd:18 vid=0
 i40e 0000:82:00.0: TX driver issue detected, PF reset issued
 i40e 0000:82:00.0 eth2: adding 68:05:ca:30:dd:18 vid=0
 i40e 0000:82:00.0: TX driver issue detected, PF reset issued

Comparing with what I'd pasted in the sourceforge thread earlier,
I see that it does not say "Hung Tx queue etc."  any more, though
it still resets.

Not sure if that changed info is significant?

It might be.  Right now I am chasing down the Tx driver issue as that
I what I am reproducing in my environment as well.

This gets "Even Uglier", I've turned off all offloads at my receiver,
enabled calling skb_linearize on *all* frames, which works fine for
scp, but the receiver shows > MSS sized frames on the wire for
rds-stress traffic.

This implies to me we have some issue with skb_linearize, possibly in
how the stack linearizes the data, or how the driver interprets the
linearized packets (which should always work)

Wheee......

On (03/30/16 12:41), Jesse Brandeburg wrote:

This gets "Even Uglier", I've turned off all offloads at my receiver,
enabled calling skb_linearize on *all* frames, which works fine for
scp, but the receiver shows > MSS sized frames on the wire for
rds-stress traffic.

fwiw, I was not able to reproduce this tx issue with iperf/netperf
etc either (I tried various perumtations of bidir, req-resp etc). One
difference between the rds-stress invocation and the other callers
is that this comes down via tcp_sendpage(), I dont know if there
is something in the sendpage path that i40e does not anticipate. 

Other drivers (ixgbe etc) work fine, so my hunch would be that this
is specific to i40e (and not a skb_linearize bug) but I could be
wrong.

--Sowmini

On Wed, 2016-03-30 at 16:09 -0400, Sowmini Varadhan wrote:

On (03/30/16 12:41), Jesse Brandeburg wrote:

quoted

This gets "Even Uglier", I've turned off all offloads at my receiver,
enabled calling skb_linearize on *all* frames, which works fine for
scp, but the receiver shows > MSS sized frames on the wire for
rds-stress traffic.

fwiw, I was not able to reproduce this tx issue with iperf/netperf
etc either (I tried various perumtations of bidir, req-resp etc). One
difference between the rds-stress invocation and the other callers
is that this comes down via tcp_sendpage(), I dont know if there
is something in the sendpage path that i40e does not anticipate. 

You might try netperf -t TCP_SENDFILE -- -m 150

to let netperf use sendfile() on small frags.

On (03/30/16 13:15), Eric Dumazet wrote:

You might try netperf -t TCP_SENDFILE -- -m 150

to let netperf use sendfile() on small frags.

that still did not reproduce it but let me try beating on
that approach with more permutations.

BTW, another data-point that may help debug this: even with i40e,
if you use the "-o" option to the rds-stress invocation, there are
no problems: the "-o" option  enforces uni-directional data
transfer, so one side is pure-Tx, other side is pure-Rx. 

It is only when both sides simultaneously do both Tx and Rx
on the tcp socket that you see the issue. I dont know if
that provides any clues.

--Sowmini

On Wed, Mar 30, 2016 at 12:41 PM, Jesse Brandeburg
[off-list ref] wrote:

On Wed, 30 Mar 2016 10:35:55 -0700
Alexander Duyck [off-list ref] wrote:

quoted

On Wed, Mar 30, 2016 at 10:20 AM, Sowmini Varadhan
[off-list ref] wrote:

quoted

On (03/30/16 10:12), Alexander Duyck wrote:

quoted

Yeah.  The patch was sort of a knee-jerk reaction to being told that
the patch referenced caused a regression.  From what I can tell that
is not the case as I am also seeing the Tx hangs when I run the test
with the frames being linearized.

I'm not sure how important of a subtlety this is, but the actual
console log after the patch is the following:

 i40e 0000:82:00.0: TX driver issue detected, PF reset issued
 i40e 0000:82:00.0 eth2: adding 68:05:ca:30:dd:18 vid=0
 i40e 0000:82:00.0: TX driver issue detected, PF reset issued
 i40e 0000:82:00.0 eth2: adding 68:05:ca:30:dd:18 vid=0
 i40e 0000:82:00.0: TX driver issue detected, PF reset issued

Comparing with what I'd pasted in the sourceforge thread earlier,
I see that it does not say "Hung Tx queue etc."  any more, though
it still resets.

Not sure if that changed info is significant?

It might be.  Right now I am chasing down the Tx driver issue as that
I what I am reproducing in my environment as well.

This gets "Even Uglier", I've turned off all offloads at my receiver,
enabled calling skb_linearize on *all* frames, which works fine for
scp, but the receiver shows > MSS sized frames on the wire for
rds-stress traffic.

Are you sure it isn't just GRO reassembling frames on the receive
side.  I know that one always trips me up when I am using the Rx path
to validate Tx checksums.

This implies to me we have some issue with skb_linearize, possibly in
how the stack linearizes the data, or how the driver interprets the
linearized packets (which should always work)

Wheee......

With the descriptor dump code you have you should be able to verify
what the layout is after the descriptor is linearized.  I would think
in most cases you would end up with at most something like 4 to maybe
5 descriptors for a 64K frame.

- Alex

On Wed, Mar 30, 2016 at 2:20 PM, Alexander Duyck
[off-list ref] wrote:

On Wed, Mar 30, 2016 at 12:41 PM, Jesse Brandeburg
[off-list ref] wrote:

quoted

On Wed, 30 Mar 2016 10:35:55 -0700
Alexander Duyck [off-list ref] wrote:

quoted

On Wed, Mar 30, 2016 at 10:20 AM, Sowmini Varadhan
[off-list ref] wrote:

quoted

On (03/30/16 10:12), Alexander Duyck wrote:

quoted

Yeah.  The patch was sort of a knee-jerk reaction to being told that
the patch referenced caused a regression.  From what I can tell that
is not the case as I am also seeing the Tx hangs when I run the test
with the frames being linearized.

I'm not sure how important of a subtlety this is, but the actual
console log after the patch is the following:

 i40e 0000:82:00.0: TX driver issue detected, PF reset issued
 i40e 0000:82:00.0 eth2: adding 68:05:ca:30:dd:18 vid=0
 i40e 0000:82:00.0: TX driver issue detected, PF reset issued
 i40e 0000:82:00.0 eth2: adding 68:05:ca:30:dd:18 vid=0
 i40e 0000:82:00.0: TX driver issue detected, PF reset issued

Comparing with what I'd pasted in the sourceforge thread earlier,
I see that it does not say "Hung Tx queue etc."  any more, though
it still resets.

Not sure if that changed info is significant?

It might be.  Right now I am chasing down the Tx driver issue as that
I what I am reproducing in my environment as well.

This gets "Even Uglier", I've turned off all offloads at my receiver,
enabled calling skb_linearize on *all* frames, which works fine for
scp, but the receiver shows > MSS sized frames on the wire for
rds-stress traffic.

Are you sure it isn't just GRO reassembling frames on the receive
side.  I know that one always trips me up when I am using the Rx path
to validate Tx checksums.

quoted

This implies to me we have some issue with skb_linearize, possibly in
how the stack linearizes the data, or how the driver interprets the
linearized packets (which should always work)

Wheee......

With the descriptor dump code you have you should be able to verify
what the layout is after the descriptor is linearized.  I would think
in most cases you would end up with at most something like 4 to maybe
5 descriptors for a 64K frame.

- Alex

Actually I think I just found an issue I missed in the patch.  I
didn't update the inline function that was performing the check for 8
descriptors.  As such it was allowing TSO with 8 descriptors to pass
even though the fact that the head and payload in the first descriptor
had pushed it to 9.

I should have a v2 ready in about 20 minutes or so.  In my testing it
fixes the issue.

- Alex