* [net PATCH] i40e/i40evf: Limit TSO to 7 descriptors for payload instead of 8 per packet
@ 2016-03-30 6:44 Alexander Duyck
2016-03-30 17:00 ` Sowmini Varadhan
0 siblings, 1 reply; 12+ messages in thread
From: Alexander Duyck @ 2016-03-30 6:44 UTC (permalink / raw)
To: netdev, intel-wired-lan, jesse.brandeburg, alexander.duyck,
jeffrey.t.kirsher
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 <aduyck@mirantis.com>
---
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;
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [net PATCH] i40e/i40evf: Limit TSO to 7 descriptors for payload instead of 8 per packet
2016-03-30 6:44 [net PATCH] i40e/i40evf: Limit TSO to 7 descriptors for payload instead of 8 per packet Alexander Duyck
@ 2016-03-30 17:00 ` Sowmini Varadhan
2016-03-30 17:12 ` Alexander Duyck
0 siblings, 1 reply; 12+ messages in thread
From: Sowmini Varadhan @ 2016-03-30 17:00 UTC (permalink / raw)
To: Alexander Duyck
Cc: netdev, intel-wired-lan, jesse.brandeburg, alexander.duyck,
jeffrey.t.kirsher
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [net PATCH] i40e/i40evf: Limit TSO to 7 descriptors for payload instead of 8 per packet
2016-03-30 17:00 ` Sowmini Varadhan
@ 2016-03-30 17:12 ` Alexander Duyck
2016-03-30 17:20 ` Sowmini Varadhan
2016-03-30 18:38 ` Jesse Brandeburg
0 siblings, 2 replies; 12+ messages in thread
From: Alexander Duyck @ 2016-03-30 17:12 UTC (permalink / raw)
To: Sowmini Varadhan
Cc: Alexander Duyck, Netdev, intel-wired-lan, Brandeburg, Jesse,
Jeff Kirsher
On Wed, Mar 30, 2016 at 10:00 AM, Sowmini Varadhan
<sowmini.varadhan@oracle.com> wrote:
> 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/
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [net PATCH] i40e/i40evf: Limit TSO to 7 descriptors for payload instead of 8 per packet
2016-03-30 17:12 ` Alexander Duyck
@ 2016-03-30 17:20 ` Sowmini Varadhan
2016-03-30 17:35 ` Alexander Duyck
2016-03-30 18:38 ` Jesse Brandeburg
1 sibling, 1 reply; 12+ messages in thread
From: Sowmini Varadhan @ 2016-03-30 17:20 UTC (permalink / raw)
To: Alexander Duyck
Cc: Alexander Duyck, Netdev, intel-wired-lan, Brandeburg, Jesse,
Jeff Kirsher
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [net PATCH] i40e/i40evf: Limit TSO to 7 descriptors for payload instead of 8 per packet
2016-03-30 17:20 ` Sowmini Varadhan
@ 2016-03-30 17:35 ` Alexander Duyck
2016-03-30 19:41 ` Jesse Brandeburg
0 siblings, 1 reply; 12+ messages in thread
From: Alexander Duyck @ 2016-03-30 17:35 UTC (permalink / raw)
To: Sowmini Varadhan
Cc: Alexander Duyck, Netdev, intel-wired-lan, Brandeburg, Jesse,
Jeff Kirsher
On Wed, Mar 30, 2016 at 10:20 AM, Sowmini Varadhan
<sowmini.varadhan@oracle.com> wrote:
> 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?
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [net PATCH] i40e/i40evf: Limit TSO to 7 descriptors for payload instead of 8 per packet
2016-03-30 17:35 ` Alexander Duyck
@ 2016-03-30 19:41 ` Jesse Brandeburg
2016-03-30 20:09 ` Sowmini Varadhan
2016-03-30 21:20 ` Alexander Duyck
0 siblings, 2 replies; 12+ messages in thread
From: Jesse Brandeburg @ 2016-03-30 19:41 UTC (permalink / raw)
To: Alexander Duyck
Cc: Sowmini Varadhan, Alexander Duyck, Netdev, intel-wired-lan,
Jeff Kirsher, jesse.brandeburg
On Wed, 30 Mar 2016 10:35:55 -0700
Alexander Duyck <alexander.duyck@gmail.com> wrote:
> On Wed, Mar 30, 2016 at 10:20 AM, Sowmini Varadhan
> <sowmini.varadhan@oracle.com> wrote:
> > 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?
>
> 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......
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [net PATCH] i40e/i40evf: Limit TSO to 7 descriptors for payload instead of 8 per packet
2016-03-30 19:41 ` Jesse Brandeburg
@ 2016-03-30 20:09 ` Sowmini Varadhan
2016-03-30 20:15 ` Eric Dumazet
2016-03-30 21:20 ` Alexander Duyck
1 sibling, 1 reply; 12+ messages in thread
From: Sowmini Varadhan @ 2016-03-30 20:09 UTC (permalink / raw)
To: Jesse Brandeburg
Cc: Alexander Duyck, Alexander Duyck, Netdev, intel-wired-lan,
Jeff Kirsher
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [net PATCH] i40e/i40evf: Limit TSO to 7 descriptors for payload instead of 8 per packet
2016-03-30 20:09 ` Sowmini Varadhan
@ 2016-03-30 20:15 ` Eric Dumazet
2016-03-30 20:40 ` Sowmini Varadhan
0 siblings, 1 reply; 12+ messages in thread
From: Eric Dumazet @ 2016-03-30 20:15 UTC (permalink / raw)
To: Sowmini Varadhan
Cc: Jesse Brandeburg, Alexander Duyck, Alexander Duyck, Netdev,
intel-wired-lan, Jeff Kirsher
On Wed, 2016-03-30 at 16:09 -0400, Sowmini Varadhan wrote:
> 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.
You might try netperf -t TCP_SENDFILE -- -m 150
to let netperf use sendfile() on small frags.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [net PATCH] i40e/i40evf: Limit TSO to 7 descriptors for payload instead of 8 per packet
2016-03-30 20:15 ` Eric Dumazet
@ 2016-03-30 20:40 ` Sowmini Varadhan
0 siblings, 0 replies; 12+ messages in thread
From: Sowmini Varadhan @ 2016-03-30 20:40 UTC (permalink / raw)
To: Eric Dumazet
Cc: Jesse Brandeburg, Alexander Duyck, Alexander Duyck, Netdev,
intel-wired-lan, Jeff Kirsher
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [net PATCH] i40e/i40evf: Limit TSO to 7 descriptors for payload instead of 8 per packet
2016-03-30 19:41 ` Jesse Brandeburg
2016-03-30 20:09 ` Sowmini Varadhan
@ 2016-03-30 21:20 ` Alexander Duyck
2016-03-30 23:06 ` Alexander Duyck
1 sibling, 1 reply; 12+ messages in thread
From: Alexander Duyck @ 2016-03-30 21:20 UTC (permalink / raw)
To: Jesse Brandeburg
Cc: Sowmini Varadhan, Alexander Duyck, Netdev, intel-wired-lan,
Jeff Kirsher
On Wed, Mar 30, 2016 at 12:41 PM, Jesse Brandeburg
<jesse.brandeburg@intel.com> wrote:
> On Wed, 30 Mar 2016 10:35:55 -0700
> Alexander Duyck <alexander.duyck@gmail.com> wrote:
>
>> On Wed, Mar 30, 2016 at 10:20 AM, Sowmini Varadhan
>> <sowmini.varadhan@oracle.com> wrote:
>> > 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?
>>
>> 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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [net PATCH] i40e/i40evf: Limit TSO to 7 descriptors for payload instead of 8 per packet
2016-03-30 21:20 ` Alexander Duyck
@ 2016-03-30 23:06 ` Alexander Duyck
0 siblings, 0 replies; 12+ messages in thread
From: Alexander Duyck @ 2016-03-30 23:06 UTC (permalink / raw)
To: Jesse Brandeburg
Cc: Sowmini Varadhan, Alexander Duyck, Netdev, intel-wired-lan,
Jeff Kirsher
On Wed, Mar 30, 2016 at 2:20 PM, Alexander Duyck
<alexander.duyck@gmail.com> wrote:
> On Wed, Mar 30, 2016 at 12:41 PM, Jesse Brandeburg
> <jesse.brandeburg@intel.com> wrote:
>> On Wed, 30 Mar 2016 10:35:55 -0700
>> Alexander Duyck <alexander.duyck@gmail.com> wrote:
>>
>>> On Wed, Mar 30, 2016 at 10:20 AM, Sowmini Varadhan
>>> <sowmini.varadhan@oracle.com> wrote:
>>> > 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?
>>>
>>> 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
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [net PATCH] i40e/i40evf: Limit TSO to 7 descriptors for payload instead of 8 per packet
2016-03-30 17:12 ` Alexander Duyck
2016-03-30 17:20 ` Sowmini Varadhan
@ 2016-03-30 18:38 ` Jesse Brandeburg
1 sibling, 0 replies; 12+ messages in thread
From: Jesse Brandeburg @ 2016-03-30 18:38 UTC (permalink / raw)
To: Alexander Duyck
Cc: Sowmini Varadhan, Alexander Duyck, Netdev, intel-wired-lan,
Jeff Kirsher, jesse.brandeburg
On Wed, 30 Mar 2016 10:12:51 -0700
Alexander Duyck <alexander.duyck@gmail.com> wrote:
> On Wed, Mar 30, 2016 at 10:00 AM, Sowmini Varadhan
> <sowmini.varadhan@oracle.com> wrote:
> > 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/
>
> 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.
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2016-03-30 23:06 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-03-30 6:44 [net PATCH] i40e/i40evf: Limit TSO to 7 descriptors for payload instead of 8 per packet Alexander Duyck
2016-03-30 17:00 ` Sowmini Varadhan
2016-03-30 17:12 ` Alexander Duyck
2016-03-30 17:20 ` Sowmini Varadhan
2016-03-30 17:35 ` Alexander Duyck
2016-03-30 19:41 ` Jesse Brandeburg
2016-03-30 20:09 ` Sowmini Varadhan
2016-03-30 20:15 ` Eric Dumazet
2016-03-30 20:40 ` Sowmini Varadhan
2016-03-30 21:20 ` Alexander Duyck
2016-03-30 23:06 ` Alexander Duyck
2016-03-30 18:38 ` Jesse Brandeburg
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).