[PATCH net] e1000e: Change wthresh to 1 to avoid possible Tx stalls.

[PATCH net] e1000e: Change wthresh to 1 to avoid possible Tx stalls.

[Hiroaki SHIMODA]

Denys Fedoryshchenko reported Tx stalls on e1000e with BQL enabled.

e1000e has WTHRESH which determines when Tx descripters are written
back and successive Tx interrupts are generated, and setting WTHRESH
to 5 gives efficient bus utilization but this cause possible Tx stalls,
especially on BQL enabled system.

To avoid possible Tx stalls, change WTHRESH to 1.

Reported-by: Denys Fedoryshchenko <denys@visp.net.lb>
Tested-by: Denys Fedoryshchenko <denys@visp.net.lb>
Signed-off-by: Hiroaki SHIMODA <shimoda.hiroaki@gmail.com>
---
 drivers/net/ethernet/intel/e1000e/e1000.h  |    6 +++---
 drivers/net/ethernet/intel/e1000e/netdev.c |    2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000e/e1000.h b/drivers/net/ethernet/intel/e1000e/e1000.h
index 6e6fffb..dc28078 100644
--- a/drivers/net/ethernet/intel/e1000e/e1000.h
+++ b/drivers/net/ethernet/intel/e1000e/e1000.h
@@ -175,13 +175,13 @@ struct e1000_info;
 /*
  * in the case of WTHRESH, it appears at least the 82571/2 hardware
  * writes back 4 descriptors when WTHRESH=5, and 3 descriptors when
- * WTHRESH=4, and since we want 64 bytes at a time written back, set
- * it to 5
+ * WTHRESH=4, so setting 5 gives most efficient bus utilization but
+ * to avoid possible Tx hangs, set it to 1
  */
 #define E1000_TXDCTL_DMA_BURST_ENABLE                          \
 	(E1000_TXDCTL_GRAN | /* set descriptor granularity */  \
 	 E1000_TXDCTL_COUNT_DESC |                             \
-	 (5 << 16) | /* wthresh must be +1 more than desired */\
+	 (1 << 16) | /* wthresh must be +1 more than desired */\
 	 (1 << 8)  | /* hthresh */                             \
 	 0x1f)       /* pthresh */
 
diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index a4b0435..b031312 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -2806,7 +2806,7 @@ static void e1000_configure_tx(struct e1000_adapter *adapter)
 		 * set up some performance related parameters to encourage the
 		 * hardware to use the bus more efficiently in bursts, depends
 		 * on the tx_int_delay to be enabled,
-		 * wthresh = 5 ==> burst write a cacheline (64 bytes) at a time
+		 * wthresh = 1 ==> burst write is disabled to consider Tx hangs
 		 * hthresh = 1 ==> prefetch when one or more available
 		 * pthresh = 0x1f ==> prefetch if internal cache 31 or less
 		 * BEWARE: this seems to work but should be considered first if
-- 
1.7.8.6

Re: [PATCH net] e1000e: Change wthresh to 1 to avoid possible Tx stalls.

[Jeff Kirsher]

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

On Wed, 2012-06-06 at 17:43 +0900, Hiroaki SHIMODA wrote:
> Denys Fedoryshchenko reported Tx stalls on e1000e with BQL enabled.
> 
> e1000e has WTHRESH which determines when Tx descripters are written
> back and successive Tx interrupts are generated, and setting WTHRESH
> to 5 gives efficient bus utilization but this cause possible Tx
> stalls,
> especially on BQL enabled system.
> 
> To avoid possible Tx stalls, change WTHRESH to 1.
> 
> Reported-by: Denys Fedoryshchenko <denys@visp.net.lb>
> Tested-by: Denys Fedoryshchenko <denys@visp.net.lb>
> Signed-off-by: Hiroaki SHIMODA <shimoda.hiroaki@gmail.com>
> ---
>  drivers/net/ethernet/intel/e1000e/e1000.h  |    6 +++---
>  drivers/net/ethernet/intel/e1000e/netdev.c |    2 +-
>  2 files changed, 4 insertions(+), 4 deletions(-) 

Thanks! I will add this to my queue of e1000e patches.

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

Re: [PATCH net] e1000e: Change wthresh to 1 to avoid possible Tx stalls.

[Eric Dumazet]

On Wed, 2012-06-06 at 17:43 +0900, Hiroaki SHIMODA wrote:
> Denys Fedoryshchenko reported Tx stalls on e1000e with BQL enabled.
> 
> e1000e has WTHRESH which determines when Tx descripters are written
> back and successive Tx interrupts are generated, and setting WTHRESH
> to 5 gives efficient bus utilization but this cause possible Tx stalls,
> especially on BQL enabled system.
> 
> To avoid possible Tx stalls, change WTHRESH to 1.
> 
> Reported-by: Denys Fedoryshchenko <denys@visp.net.lb>
> Tested-by: Denys Fedoryshchenko <denys@visp.net.lb>
> Signed-off-by: Hiroaki SHIMODA <shimoda.hiroaki@gmail.com>
> ---
>  drivers/net/ethernet/intel/e1000e/e1000.h  |    6 +++---
>  drivers/net/ethernet/intel/e1000e/netdev.c |    2 +-
>  2 files changed, 4 insertions(+), 4 deletions(-)

Thanks a lot !

Acked-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH net] e1000e: Change wthresh to 1 to avoid possible Tx stalls.

[Jeff Kirsher]

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

On Wed, 2012-06-06 at 01:43 -0700, Hiroaki SHIMODA wrote:
> Denys Fedoryshchenko reported Tx stalls on e1000e with BQL enabled.
> 
> e1000e has WTHRESH which determines when Tx descripters are written
> back and successive Tx interrupts are generated, and setting WTHRESH
> to 5 gives efficient bus utilization but this cause possible Tx
> stalls,
> especially on BQL enabled system.
> 
> To avoid possible Tx stalls, change WTHRESH to 1.
> 
> Reported-by: Denys Fedoryshchenko <denys@visp.net.lb>
> Tested-by: Denys Fedoryshchenko <denys@visp.net.lb>
> Signed-off-by: Hiroaki SHIMODA <shimoda.hiroaki@gmail.com>
> ---
>  drivers/net/ethernet/intel/e1000e/e1000.h  |    6 +++---
>  drivers/net/ethernet/intel/e1000e/netdev.c |    2 +-
>  2 files changed, 4 insertions(+), 4 deletions(-) 

After further internal review, NACK.

This patch will cause unacceptable performance issues with non-ESB2
parts.

I am dropping this patch from my queue.


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

Re: [PATCH net] e1000e: Change wthresh to 1 to avoid possible Tx stalls.

[David Miller]

From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Date: Wed, 06 Jun 2012 17:59:12 -0700

> This patch will cause unacceptable performance issues with non-ESB2
> parts.

You have to fix the regression a non-1 setting causes, performance
is secondary.

Re: [PATCH net] e1000e: Change wthresh to 1 to avoid possible Tx stalls.

[Eric Dumazet]

On Wed, 2012-06-06 at 17:59 -0700, Jeff Kirsher wrote:

> After further internal review, NACK.
> 
> This patch will cause unacceptable performance issues with non-ESB2
> parts.
> 
> I am dropping this patch from my queue.
> 

I'd like you share your performance numbers before NACKing this patch.

What is the alternative patch you guys have ?

Re: [PATCH net] e1000e: Change wthresh to 1 to avoid possible Tx stalls.

[Jeff Kirsher]

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

On Thu, 2012-06-07 at 06:24 +0200, Eric Dumazet wrote:
> On Wed, 2012-06-06 at 17:59 -0700, Jeff Kirsher wrote:
> 
> > After further internal review, NACK.
> > 
> > This patch will cause unacceptable performance issues with non-ESB2
> > parts.
> > 
> > I am dropping this patch from my queue.
> > 
> 
> I'd like you share your performance numbers before NACKing this patch.
> 
> What is the alternative patch you guys have ?
> 

Jesse did not share any performance numbers with me, I am sure he can
give some background tomorrow when he is back online.

I am working on an alternative patch now and should have something to
share tomorrow.

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

Re: [PATCH net] e1000e: Change wthresh to 1 to avoid possible Tx stalls.

[Eric Dumazet]

On Wed, 2012-06-06 at 21:52 -0700, Jeff Kirsher wrote:

> Jesse did not share any performance numbers with me, I am sure he can
> give some background tomorrow when he is back online.
> 
> I am working on an alternative patch now and should have something to
> share tomorrow.

Thanks !

Re: [PATCH net] e1000e: Change wthresh to 1 to avoid possible Tx stalls.

[Frank Reppin]

Jeff Kirsher <jeffrey.t.kirsher <at> intel.com> writes:
> 
> On Thu, 2012-06-07 at 06:24 +0200, Eric Dumazet wrote:
> > On Wed, 2012-06-06 at 17:59 -0700, Jeff Kirsher wrote:
> > 
> > > After further internal review, NACK.
> > > 
> > > This patch will cause unacceptable performance issues with non-ESB2
> > > parts.
> > > 
> > > I am dropping this patch from my queue.
> > > 
> > 
> > I'd like you share your performance numbers before NACKing this patch.
> > 
> > What is the alternative patch you guys have ?
> > 
> 
> Jesse did not share any performance numbers with me, I am sure he can
> give some background tomorrow when he is back online.
> 
> I am working on an alternative patch now and should have something to
> share tomorrow.
Please allow me to ask if there's any progess here?

I've tried 3.5.4 a couple of days ago on a SuperMicro X8SIE-LN4 (82574L)
and could still observe severe latency (up to 3000ms) spikes.

Applying Hiroakis suggested patch did fix this for me as well.
[please note as well that I didn't had this issue in any 3.4.x kernel
before - so +1 for fixing the regression]

Thankyou!
Frank Reppin

-- 

Re: [PATCH net] e1000e: Change wthresh to 1 to avoid possible Tx stalls.

[Eric Dumazet]

On Tue, 2012-10-09 at 00:25 +0000, Frank Reppin wrote:
> Jeff Kirsher <jeffrey.t.kirsher <at> intel.com> writes:
> > 
> > On Thu, 2012-06-07 at 06:24 +0200, Eric Dumazet wrote:
> > > On Wed, 2012-06-06 at 17:59 -0700, Jeff Kirsher wrote:
> > > 
> > > > After further internal review, NACK.
> > > > 
> > > > This patch will cause unacceptable performance issues with non-ESB2
> > > > parts.
> > > > 
> > > > I am dropping this patch from my queue.
> > > > 
> > > 
> > > I'd like you share your performance numbers before NACKing this patch.
> > > 
> > > What is the alternative patch you guys have ?
> > > 
> > 
> > Jesse did not share any performance numbers with me, I am sure he can
> > give some background tomorrow when he is back online.
> > 
> > I am working on an alternative patch now and should have something to
> > share tomorrow.
> Please allow me to ask if there's any progess here?
> 
> I've tried 3.5.4 a couple of days ago on a SuperMicro X8SIE-LN4 (82574L)
> and could still observe severe latency (up to 3000ms) spikes.
> 
> Applying Hiroakis suggested patch did fix this for me as well.
> [please note as well that I didn't had this issue in any 3.4.x kernel
> before - so +1 for fixing the regression]

Its a shame this bug is not yet fixed.

I dont know what to say.

Re: [PATCH net] e1000e: Change wthresh to 1 to avoid possible Tx stalls.

[Jesse Brandeburg]

> > > Jesse did not share any performance numbers with me, I am sure he can
> > > give some background tomorrow when he is back online.
> > > 
> > > I am working on an alternative patch now and should have something to
> > > share tomorrow.
> > Please allow me to ask if there's any progess here?
> > 
> > I've tried 3.5.4 a couple of days ago on a SuperMicro X8SIE-LN4 (82574L)
> > and could still observe severe latency (up to 3000ms) spikes.
> > 
> > Applying Hiroakis suggested patch did fix this for me as well.
> > [please note as well that I didn't had this issue in any 3.4.x kernel
> > before - so +1 for fixing the regression]

I'm not sure what went wrong internally here that this hasn't been
fixed, and I'm personally embarrassed.  I am working on it until I have
a patch/solution.

currently am trying to reproduce the issue, am in some weird how to
use BQL limbo, the lack of documentation on user usage of BQL is slowing
me down.

Hints or clues (I'm trying to follow the repro steps mentioned in
some related threads) are appreciated.

Re: [PATCH net] e1000e: Change wthresh to 1 to avoid possible Tx stalls.

[Eric Dumazet]

On Tue, 2012-10-09 at 10:36 -0700, Jesse Brandeburg wrote:
> > > > Jesse did not share any performance numbers with me, I am sure he can
> > > > give some background tomorrow when he is back online.
> > > > 
> > > > I am working on an alternative patch now and should have something to
> > > > share tomorrow.
> > > Please allow me to ask if there's any progess here?
> > > 
> > > I've tried 3.5.4 a couple of days ago on a SuperMicro X8SIE-LN4 (82574L)
> > > and could still observe severe latency (up to 3000ms) spikes.
> > > 
> > > Applying Hiroakis suggested patch did fix this for me as well.
> > > [please note as well that I didn't had this issue in any 3.4.x kernel
> > > before - so +1 for fixing the regression]
> 
> I'm not sure what went wrong internally here that this hasn't been
> fixed, and I'm personally embarrassed.  I am working on it until I have
> a patch/solution.
> 
> currently am trying to reproduce the issue, am in some weird how to
> use BQL limbo, the lack of documentation on user usage of BQL is slowing
> me down.
> 

BQL is blocking qdisc from delivering additional packet to TX as long as
previous packets were not completed. Not sure what you want to know
about BQL.

> Hints or clues (I'm trying to follow the repro steps mentioned in
> some related threads) are appreciated.


Problem is BQL depends on TX completion being done in a reasonable time.

It seems e1000e can hold an skb in TX ring for up to 3000ms (not
reasonable it seems)

Aggregation / coalescing is fine, as long as we dont hold a packet too
long, in case no other packet follows.

Re: [PATCH net] e1000e: Change wthresh to 1 to avoid possible Tx stalls.

[Andre Tomt]

On 09. okt. 2012 19:36, Jesse Brandeburg wrote:
> I'm not sure what went wrong internally here that this hasn't been
> fixed, and I'm personally embarrassed.  I am working on it until I have
> a patch/solution.
>
> currently am trying to reproduce the issue, am in some weird how to
> use BQL limbo, the lack of documentation on user usage of BQL is slowing
> me down.
>
> Hints or clues (I'm trying to follow the repro steps mentioned in
> some related threads) are appreciated.

I found it simplest to reproduce when doing forwarding, and *not* 
saturating the interface doing the TX. 100Mbps forwarding on gigabit 
link triggered it in seconds. Doing gigabit forwarding speeds (~980Mbps) 
did not trigger anything.

Setup looked somewhat like this, GE beeing gigabit link, FE 100Mbps;

reciever PC (iperf -s)
     |
     GE
     |
    eth0   <- TX lockups
router with 2*e1000e
    eth1
     |
     GE
     |
switch
     |
     FE
     |
source PC (iperf -c recieverPC)

I don't recall all the details anymore, but I'm fairly certain I didnt 
use any non-default qdiscs to reproduce - eg just pfifo_fast (usually 
doing fq_codel though).

For the bug to manifest itself was somewhat dependent on GRO and TSO in 
kernel 3.5, but with 3.6 it didnt matter anymore (at least the rc's).

Re: [PATCH net] e1000e: Change wthresh to 1 to avoid possible Tx stalls.

[Frank Reppin]

On 10.10.2012 00:48, Andre Tomt wrote:
> On 09. okt. 2012 19:36, Jesse Brandeburg wrote:
[...]
>> Hints or clues (I'm trying to follow the repro steps mentioned in
>> some related threads) are appreciated.
In our scenario - the SuperMicro X8SIE-LN4 acts
as a router.

eth0 -> uplink
eth1 -> core servers network 192.168.a.b/24
eth2 -> developer employees network 192.168.c.d/24

eth1 and eth2 are hooked into (two different) HP2510-24G
switches (they both show no errors at all).

The switch connected to eth2 connects a bunch of
developers doing day-2-day stuff (commits/checkouts/documentation/
listening to stream music/surfing/email) and the such.

Continously running a ping from ie. 192.168.c.d/24 to
192.168.a.b/24 visualizes those spikes (mostly triplets, where
the first echo reply is quite high and the two following replies
are each around 50 percent faster - but mostly still >500ms) - whenever 
these spikes happen, it feels like the whole net 'stalls'.
Just like Denys Fedoryshchenko stated in

http://www.mail-archive.com/e1000-devel@lists.sourceforge.net/msg05517.html

HTH,
Frank Reppin

-- 
43rd Law of Computing:
         Anything that can go wr
fortune: Segmentation violation -- Core dumped

Re: [PATCH NET] e1000e: Change wthresh to 1 to avoid possible Tx stalls

[David Miller]

From: Jesse Brandeburg <jesse.brandeburg@intel.com>
Date: Wed, 10 Oct 2012 18:34:20 -0700

> From: Hiroaki SHIMODA <shimoda.hiroaki@gmail.com>
> 
> This patch originated from Hiroaki SHIMODA but has been modified
> by Intel with some minor cleanups and additional commit log text.
> 
> Denys Fedoryshchenko and others reported Tx stalls on e1000e with
> BQL enabled.  Issue was root caused to hardware delays. They were
> introduced because some of the e1000e hardware with transmit
> writeback bursting enabled, waits until the driver does an
> explict flush OR there are WTHRESH descriptors to write back.
> 
> Sometimes the delays in question were on the order of seconds,
> causing visible lag for ssh sessions and unacceptable tx
> completion latency, especially for BQL enabled kernels.
> 
> To avoid possible Tx stalls, change WTHRESH back to 1.
> 
> The current plan is to investigate a method for re-enabling
> WTHRESH while not harming BQL, but those patches will be later
> for net-next if they work.
> 
> please enqueue for stable since v3.3 as this bug was introduced in
> commit 3f0cfa3bc11e7f00c9994e0f469cbc0e7da7b00c
> Author: Tom Herbert <therbert@google.com>
> Date:   Mon Nov 28 16:33:16 2011 +0000
> 
>     e1000e: Support for byte queue limits
> 
>     Changes to e1000e to use byte queue limits.
> 
> Reported-by: Denys Fedoryshchenko <denys@visp.net.lb>
> Tested-by: Denys Fedoryshchenko <denys@visp.net.lb>
> Signed-off-by: Hiroaki SHIMODA <shimoda.hiroaki@gmail.com>
> CC: eric.dumazet@gmail.com
> CC: therbert@google.com
> Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>

Applied and queued up for -stable, thanks.

[PATCH NET] e1000e: Change wthresh to 1 to avoid possible Tx stalls

[Jesse Brandeburg]

From: Hiroaki SHIMODA <shimoda.hiroaki@gmail.com>

This patch originated from Hiroaki SHIMODA but has been modified
by Intel with some minor cleanups and additional commit log text.

Denys Fedoryshchenko and others reported Tx stalls on e1000e with
BQL enabled.  Issue was root caused to hardware delays. They were
introduced because some of the e1000e hardware with transmit
writeback bursting enabled, waits until the driver does an
explict flush OR there are WTHRESH descriptors to write back.

Sometimes the delays in question were on the order of seconds,
causing visible lag for ssh sessions and unacceptable tx
completion latency, especially for BQL enabled kernels.

To avoid possible Tx stalls, change WTHRESH back to 1.

The current plan is to investigate a method for re-enabling
WTHRESH while not harming BQL, but those patches will be later
for net-next if they work.

please enqueue for stable since v3.3 as this bug was introduced in
commit 3f0cfa3bc11e7f00c9994e0f469cbc0e7da7b00c
Author: Tom Herbert <therbert@google.com>
Date:   Mon Nov 28 16:33:16 2011 +0000

    e1000e: Support for byte queue limits

    Changes to e1000e to use byte queue limits.

Reported-by: Denys Fedoryshchenko <denys@visp.net.lb>
Tested-by: Denys Fedoryshchenko <denys@visp.net.lb>
Signed-off-by: Hiroaki SHIMODA <shimoda.hiroaki@gmail.com>
CC: eric.dumazet@gmail.com
CC: therbert@google.com
Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
---

 drivers/net/ethernet/intel/e1000e/e1000.h  |    6 +++---
 drivers/net/ethernet/intel/e1000e/netdev.c |    2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000e/e1000.h b/drivers/net/ethernet/intel/e1000e/e1000.h
index cb3356c..04668b4 100644
--- a/drivers/net/ethernet/intel/e1000e/e1000.h
+++ b/drivers/net/ethernet/intel/e1000e/e1000.h
@@ -175,13 +175,13 @@ struct e1000_info;
 /*
  * in the case of WTHRESH, it appears at least the 82571/2 hardware
  * writes back 4 descriptors when WTHRESH=5, and 3 descriptors when
- * WTHRESH=4, and since we want 64 bytes at a time written back, set
- * it to 5
+ * WTHRESH=4, so a setting of 5 gives the most efficient bus
+ * utilization but to avoid possible Tx stalls, set it to 1
  */
 #define E1000_TXDCTL_DMA_BURST_ENABLE                          \
 	(E1000_TXDCTL_GRAN | /* set descriptor granularity */  \
 	 E1000_TXDCTL_COUNT_DESC |                             \
-	 (5 << 16) | /* wthresh must be +1 more than desired */\
+	 (1 << 16) | /* wthresh must be +1 more than desired */\
 	 (1 << 8)  | /* hthresh */                             \
 	 0x1f)       /* pthresh */
 
diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index de57a2b..f444eb0 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -2831,7 +2831,7 @@ static void e1000_configure_tx(struct e1000_adapter *adapter)
 		 * set up some performance related parameters to encourage the
 		 * hardware to use the bus more efficiently in bursts, depends
 		 * on the tx_int_delay to be enabled,
-		 * wthresh = 5 ==> burst write a cacheline (64 bytes) at a time
+		 * wthresh = 1 ==> burst write is disabled to avoid Tx stalls
 		 * hthresh = 1 ==> prefetch when one or more available
 		 * pthresh = 0x1f ==> prefetch if internal cache 31 or less
 		 * BEWARE: this seems to work but should be considered first if