netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* RE: [PATCH] abysmal e1000 performance (DITR)
@ 2004-06-04 15:44 Venkatesan, Ganesh
  2004-08-26 17:55 ` jamal
  0 siblings, 1 reply; 12+ messages in thread
From: Venkatesan, Ganesh @ 2004-06-04 15:44 UTC (permalink / raw)
  To: tharbaugh, netdev; +Cc: Feldman, Scott, Brandeburg, Jesse

Thayne:

We are studying the patch and will get back to you once we have a plan
to integrate it into the driver.

Thanks,
ganesh 
 
-------------------------------------------------
Ganesh Venkatesan
Network/Storage Division, Hillsboro, OR

-----Original Message-----
From: Thayne Harbaugh [mailto:tharbaugh@lnxi.com] 
Sent: Friday, June 04, 2004 7:20 AM
To: netdev@oss.sgi.com
Cc: Venkatesan, Ganesh; Feldman, Scott; Brandeburg, Jesse
Subject: Re: [PATCH] abysmal e1000 performance (DITR)

I have documented a serious performance problem with DITR in the e1000
driver.  I have sent a patch to the netdev list as well as CC'ed various
people at Intel.  There has been very little response.  I am wondering
who has reviewed the DITR performance problem and what the plans are for
fixing it.

-- 
Thayne Harbaugh
Linux Networx

^ permalink raw reply	[flat|nested] 12+ messages in thread
* RE: [PATCH] abysmal e1000 performance (DITR)
@ 2004-08-27 22:41 Ronciak, John
  2004-08-30 17:06 ` Thayne Harbaugh
  0 siblings, 1 reply; 12+ messages in thread
From: Ronciak, John @ 2004-08-27 22:41 UTC (permalink / raw)
  To: tharbaugh
  Cc: Jeff Garzik, hadi, Venkatesan, Ganesh, netdev, Feldman, Scott,
	Brandeburg, Jesse

Thayne,

I can't speak to what happened previously, only what's going on now.
You need to be careful about basing any tuning on any one single test.
For each one of these test you find that have performance problems I can
show you where it works just fine and most cases works better.  So let
be careful about making judgements like this.

We are going to look at this and make some changes based on what we
find.  I already said that we probably won't be ripping out the DITR as
some people really make a lot of use out of it.  It may be off by
default or have some default setting which doesn't hurt test cases.
Since Netpipe is a relatively unused test for performance (not really
it's purpose, it was developed to find "holes" in internet packets
lengths), it's not part of our normal testing.

Like I said, we are looking at it and will come up with a more robust
solution.

Cheers,
John


> -----Original Message-----
> From: Thayne Harbaugh [mailto:tharbaugh@lnxi.com] 
> Sent: Friday, August 27, 2004 3:05 PM
> To: Ronciak, John
> Cc: Jeff Garzik; hadi@cyberus.ca; Venkatesan, Ganesh; 
> netdev@oss.sgi.com; Feldman, Scott; Brandeburg, Jesse
> Subject: RE: [PATCH] abysmal e1000 performance (DITR)
> 
> 
> On Fri, 2004-08-27 at 14:49 -0700, Ronciak, John wrote:
> > Jamal, Thayne,
> > 
> > I've asked Jeff to go ahead and apply this patch as a way 
> around this
> > for now.  We would liketo see the DITR stay but now have this
> > performacne problem so we don't want to rip it out.  We do 
> however need
> > a test case to replicate this as we have not been seeing it in our
> > testing.  Please get us those case that break things.  We'll have a
> > better solution longer term based on the test cases (as 
> well as the ones
> > we normally use of course).
> 
> Attached is an email that I sent out 2004 May 25.  The email is very
> detailed about what the problem is, how to test it, and an initial
> patch.  The email was sent to several addresses at Intel.  I 
> later sent
> it to lkml and netdev.
> 
> There was almost no response at the time.  The best reply that I
> received from Intel was that the DITR was put in and calibrated
> according to a marketing benchmark program and that it wouldn't be
> changed even though tests (and customers) showed that the performance
> was *abysmal*.  It's also a big question what role DITR plays when it
> seems deprecated by NAPI.
> 
> The response was disappointing.  I'm now wondering why this has now
> become interesting and the thread has resumed.  What's 
> changed?  How did
> this catch someone's attention?  What can I do next time 
> (hopefully that
> won't happen) so that people *do* take interest.
> 
> > 
> > > -----Original Message-----
> > > From: netdev-bounce@oss.sgi.com 
> > > [mailto:netdev-bounce@oss.sgi.com] On Behalf Of Thayne Harbaugh
> > > Sent: Thursday, August 26, 2004 2:29 PM
> > > To: Jeff Garzik
> > > Cc: hadi@cyberus.ca; Venkatesan, Ganesh; netdev@oss.sgi.com; 
> > > Feldman, Scott; Brandeburg, Jesse
> > > Subject: Re: [PATCH] abysmal e1000 performance (DITR)
> > > 
> > > 
> > > On Thu, 2004-08-26 at 16:26 -0400, Jeff Garzik wrote:
> > > > Thayne Harbaugh wrote:
> > > > > On Thu, 2004-08-26 at 13:55 -0400, jamal wrote:
> > > > > 
> > > > >>Ganesh,
> > > > >>
> > > > >>Can you please make this feature off by default and perhaps
> > > > >>accesible via ethtool for peopel who want to turn it on.
> > > > >>I just wasted a few hours and was bitten by this 
> performance-wise.
> > > > >>Please consider disabling it.
> > > > > 
> > > > > 
> > > > > This is a *horrible* problem.  Even though it's fixable 
> > > by passing a
> > > > > module parameter, the default bites those that *know* 
> > > about it.  We have
> > > > > had customers bitten by this and customers that have 
> insisted in
> > > > > swapping all the NICs in a cluster to Broadcom TG3 NICs.
> > > > > 
> > > > > It's a black eye for Intel and a loss of business - 
> > > that's the opinion
> > > > > of our customers.
> > > > 
> > > > 
> > > > If it's so bad we should disable it by default, either via 
> > > the module 
> > > > parameter or via a kernel CONFIG_xxx option.
> > > 
> > > Yes, it is so bad.  The dynamic interrupt setting should 
> be deprecated
> > > by the use of NAPI.
> > > 
> > > This is a simple way to disable it, yet still keep the 
> code so that
> > > someone can enable it if they really wanted it.  I, however, 
> > > would just
> > > as soon see all of the DITR code ripped out.
> > > 
> > > There are other ways that might be better for dealing with 
> > > it, yet still
> > > keeping the DITR code viable.
> > > 
> > > --- drivers/net/e1000/e1000_param.c.broken_ditr 2004-08-26 
> > > 15:40:34.436456736 -0600
> > > +++ drivers/net/e1000/e1000_param.c     2004-08-26 
> > > 15:49:07.186506880 -0600
> > > @@ -212,7 +212,7 @@
> > >  #define MAX_TXABSDELAY            0xFFFF
> > >  #define MIN_TXABSDELAY                 0
> > >   
> > > -#define DEFAULT_ITR                    1
> > > +#define DEFAULT_ITR                 8000
> > >  #define MAX_ITR                   100000
> > >  #define MIN_ITR                      100
> > >   
> > > 
> > > 
> > > 
> > > 
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread
* RE: [PATCH] abysmal e1000 performance (DITR)
@ 2004-08-27 21:49 Ronciak, John
  2004-08-27 22:05 ` Thayne Harbaugh
  0 siblings, 1 reply; 12+ messages in thread
From: Ronciak, John @ 2004-08-27 21:49 UTC (permalink / raw)
  To: tharbaugh, Jeff Garzik
  Cc: hadi, Venkatesan, Ganesh, netdev, Feldman, Scott,
	Brandeburg, Jesse

Jamal, Thayne,

I've asked Jeff to go ahead and apply this patch as a way around this
for now.  We would liketo see the DITR stay but now have this
performacne problem so we don't want to rip it out.  We do however need
a test case to replicate this as we have not been seeing it in our
testing.  Please get us those case that break things.  We'll have a
better solution longer term based on the test cases (as well as the ones
we normally use of course).

Thanks.

Cheers,
John


> -----Original Message-----
> From: netdev-bounce@oss.sgi.com 
> [mailto:netdev-bounce@oss.sgi.com] On Behalf Of Thayne Harbaugh
> Sent: Thursday, August 26, 2004 2:29 PM
> To: Jeff Garzik
> Cc: hadi@cyberus.ca; Venkatesan, Ganesh; netdev@oss.sgi.com; 
> Feldman, Scott; Brandeburg, Jesse
> Subject: Re: [PATCH] abysmal e1000 performance (DITR)
> 
> 
> On Thu, 2004-08-26 at 16:26 -0400, Jeff Garzik wrote:
> > Thayne Harbaugh wrote:
> > > On Thu, 2004-08-26 at 13:55 -0400, jamal wrote:
> > > 
> > >>Ganesh,
> > >>
> > >>Can you please make this feature off by default and perhaps
> > >>accesible via ethtool for peopel who want to turn it on.
> > >>I just wasted a few hours and was bitten by this performance-wise.
> > >>Please consider disabling it.
> > > 
> > > 
> > > This is a *horrible* problem.  Even though it's fixable 
> by passing a
> > > module parameter, the default bites those that *know* 
> about it.  We have
> > > had customers bitten by this and customers that have insisted in
> > > swapping all the NICs in a cluster to Broadcom TG3 NICs.
> > > 
> > > It's a black eye for Intel and a loss of business - 
> that's the opinion
> > > of our customers.
> > 
> > 
> > If it's so bad we should disable it by default, either via 
> the module 
> > parameter or via a kernel CONFIG_xxx option.
> 
> Yes, it is so bad.  The dynamic interrupt setting should be deprecated
> by the use of NAPI.
> 
> This is a simple way to disable it, yet still keep the code so that
> someone can enable it if they really wanted it.  I, however, 
> would just
> as soon see all of the DITR code ripped out.
> 
> There are other ways that might be better for dealing with 
> it, yet still
> keeping the DITR code viable.
> 
> --- drivers/net/e1000/e1000_param.c.broken_ditr 2004-08-26 
> 15:40:34.436456736 -0600
> +++ drivers/net/e1000/e1000_param.c     2004-08-26 
> 15:49:07.186506880 -0600
> @@ -212,7 +212,7 @@
>  #define MAX_TXABSDELAY            0xFFFF
>  #define MIN_TXABSDELAY                 0
>   
> -#define DEFAULT_ITR                    1
> +#define DEFAULT_ITR                 8000
>  #define MAX_ITR                   100000
>  #define MIN_ITR                      100
>   
> 
> 
> 
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread
* [PATCH] abysmal e1000 performance (DITR)
@ 2004-06-01 23:05 Thayne Harbaugh
  2004-06-04 14:20 ` Thayne Harbaugh
  0 siblings, 1 reply; 12+ messages in thread
From: Thayne Harbaugh @ 2004-06-01 23:05 UTC (permalink / raw)
  To: netdev; +Cc: ganesh.venkatesan

The introduction of Dynamic Interrupt Throttling Rate (DITR) in the 4.x
-> 5.x e1000 driver change can cause serious performance problems.  The
ITR can be reduced when the system is *not* under load which results in
gratuitous latencies for network traffic.  In other words - why is the
interrupt load reduced when the system isn't under load?

This is a patch to include system load when calculating the DITR.  It
only allows the diff/goc ratio to factor into the DITR when the 1 minute
load average is above .50.  It appears to work quite well and prevents
throttling when the load is below .50 and allows throttling when the
load is in excess of .50.

There are a few concerns.  The patch requires kernel/timer.c to
EXPORT_SYMBOL(avenrun) - otherwise the symbol isn't available to the
driver when built as a module.  This symbol "clutter" may be undesirable
to some.  Another concern is that NAPI accomplishes a similar interrupt
load reduction and is likely a better solution than ITR.


diff -ur linux-2.4.21-99/drivers/net/e1000/e1000_main.c linux-2.4.21-99-e1000/drivers/net/e1000/e1000_main.c
--- linux-2.4.21-99/drivers/net/e1000/e1000_main.c	2003-09-24 05:47:33.000000000 -0700
+++ linux-2.4.21-99-e1000/drivers/net/e1000/e1000_main.c	2004-05-27 23:38:02.000000000 -0700
@@ -27,6 +27,8 @@
 *******************************************************************************/
 
 #include "e1000.h"
+/* for avenrun[] */
+#include <linux/sched.h>
 
 /* Change Log
  *
@@ -1429,14 +1431,34 @@
 
 	/* Dynamic mode for Interrupt Throttle Rate (ITR) */
 	if(adapter->hw.mac_type >= e1000_82540 && adapter->itr == 1) {
+		/* fixed fractional part of .50 */
+#define FIXED_F50 (FIXED_1 >> 1)
+		/* This maps the range of .50-.99 -> 0-100 */
+#define FIXED_1_MAPPED (FIXED_1 - FIXED_F50)
+		unsigned long laf;	/* load average fractional part */
+		uint32_t goc;		/* good octet count */
+		uint32_t dif;		/* dif between tx and rx goc */
+		uint32_t itr;		/* inturrept throttle rate */
+		/* laf range is mapped:
+		 *     .00-.50 -> .00
+		 *     .50-.99 -> .00-.99
+		 *    1.00+    -> 1.00 */
+		if (avenrun[0] + FIXED_1/200 > FIXED_1)
+			laf = FIXED_1_MAPPED;
+		else if (avenrun[0] + FIXED_1/200 < FIXED_F50)
+			laf = 0;
+		else
+			laf = ((avenrun[0] + FIXED_1/2000) & (FIXED_1 - 1)) - FIXED_1_MAPPED;
 		/* Symmetric Tx/Rx gets a reduced ITR=2000; Total
 		 * asymmetrical Tx or Rx gets ITR=8000; everyone
 		 * else is between 2000-8000. */
-		uint32_t goc = (adapter->gotcl + adapter->gorcl) / 10000;
-		uint32_t dif = (adapter->gotcl > adapter->gorcl ? 
-			adapter->gotcl - adapter->gorcl :
-			adapter->gorcl - adapter->gotcl) / 10000;
-		uint32_t itr = goc > 0 ? (dif * 6000 / goc + 2000) : 8000;
+		goc = (adapter->gotcl + adapter->gorcl) / 10000;
+		dif = (adapter->gotcl > adapter->gorcl ? 
+		       adapter->gotcl - adapter->gorcl :
+		       adapter->gorcl - adapter->gotcl) / 10000;
+		itr = goc > 0
+			? 8000 + 6000*laf/FIXED_1_MAPPED*dif/goc - 6000*laf/FIXED_1_MAPPED
+			: 8000;
 		E1000_WRITE_REG(&adapter->hw, ITR, 1000000000 / (itr * 256));
 	}
 
diff -ur linux-2.4.21-99/kernel/Makefile linux-2.4.21-99-e1000/kernel/Makefile
--- linux-2.4.21-99/kernel/Makefile	2003-09-24 05:47:27.000000000 -0700
+++ linux-2.4.21-99-e1000/kernel/Makefile	2004-05-26 23:45:47.000000000 -0700
@@ -11,7 +11,7 @@
 
 export-objs = signal.o sys.o kmod.o context.o ksyms.o pm.o exec_domain.o \
 	      printk.o cpufreq.o rcupdate.o syscall_ksyms.o fork.o hook.o \
-	      rashooks.o module.o
+	      rashooks.o module.o timer.o
 
 obj-y     = sched.o dma.o fork.o exec_domain.o panic.o printk.o \
 	    module.o exit.o itimer.o info.o time.o softirq.o resource.o \
diff -ur linux-2.4.21-99/kernel/timer.c linux-2.4.21-99-e1000/kernel/timer.c
--- linux-2.4.21-99/kernel/timer.c	2003-09-24 05:47:27.000000000 -0700
+++ linux-2.4.21-99-e1000/kernel/timer.c	2004-05-27 00:54:29.000000000 -0700
@@ -686,6 +686,7 @@
  * all seem to differ on different machines.
  */
 unsigned long avenrun[3];
+EXPORT_SYMBOL(avenrun);
 
 static inline void calc_load(unsigned long ticks)
 {

-- 
Thayne Harbaugh
Linux Networx

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2004-08-30 17:06 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2004-06-04 15:44 [PATCH] abysmal e1000 performance (DITR) Venkatesan, Ganesh
2004-08-26 17:55 ` jamal
2004-08-26 20:04   ` Thayne Harbaugh
2004-08-26 20:26     ` Jeff Garzik
2004-08-26 21:28       ` Thayne Harbaugh
2004-08-28 22:58         ` Jeff Garzik
  -- strict thread matches above, loose matches on Subject: below --
2004-08-27 22:41 Ronciak, John
2004-08-30 17:06 ` Thayne Harbaugh
2004-08-27 21:49 Ronciak, John
2004-08-27 22:05 ` Thayne Harbaugh
2004-06-01 23:05 Thayne Harbaugh
2004-06-04 14:20 ` Thayne Harbaugh

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).