minor e1000 bug

minor e1000 bug

[Ethan Weinstein]

I've noticed that the e1000 driver does not update the counters in
/proc/net/dev as quickly as several other drivers I've tried, such as
e100 (both the Becker driver, and Intel's), sk90lin, and 3c59x. These
drivers seem to update the counters in a very timely fashion while the
e1000 driver doesn't seem to update them for several seconds.  This is
apparent in 2.6.0, and 2.4.xx. Is there an update interval that might be
modified within the driver to fix this?  It screws up realtime bandwidth
measurements for these cards.


-Ethan

Re: minor e1000 bug

[Hans-Peter Jansen]

Hi Ethan,

On Friday 19 December 2003 21:40, Ethan Weinstein wrote:
> I've noticed that the e1000 driver does not update the counters in
> /proc/net/dev as quickly as several other drivers I've tried, such
> as e100 (both the Becker driver, and Intel's), sk90lin, and 3c59x.
> These drivers seem to update the counters in a very timely fashion
> while the e1000 driver doesn't seem to update them for several
> seconds.  This is apparent in 2.6.0, and 2.4.xx. Is there an update

Every 2 seconds exactly.

I would also be interested in a statement from intel fellows on the 
reasoning behind this decision, since every user of gkrellm will 
notice some strange behaviour (value oscillating between 0 and 
throughput * 2). (Poor man's real time bandwidth management ;-). 

After being tired of cognitive interpretation of these values, I 
decided to fix it, which was pretty easy:

--- linux-2.4.20/drivers/net/e1000/e1000_main.c~    2003-08-03 00:40:21.000000000 +0200
+++ linux-2.4.20/drivers/net/e1000/e1000_main.c 2003-08-08 13:20:06.000000000 +0200
@@ -1390,7 +1390,7 @@
        netif_stop_queue(netdev);
 
    /* Reset the timer */
-   mod_timer(&adapter->watchdog_timer, jiffies + 2 * HZ);
+   mod_timer(&adapter->watchdog_timer, jiffies + HZ);
 }
 
 #define E1000_TX_FLAGS_CSUM        0x00000001


> interval that might be modified within the driver to fix this?  It
> screws up realtime bandwidth measurements for these cards.
>
>
> -Ethan

Enjoy,
Pete

RE: minor e1000 bug

[Feldman, Scott]

> I would also be interested in a statement from intel fellows on the 
> reasoning behind this decision, since every user of gkrellm will 
> notice some strange behaviour (value oscillating between 0 and 
> throughput * 2). (Poor man's real time bandwidth management ;-). 
> 
> After being tired of cognitive interpretation of these values, I 
> decided to fix it, which was pretty easy:
> 
> --- linux-2.4.20/drivers/net/e1000/e1000_main.c~    
> 2003-08-03 00:40:21.000000000 +0200
> +++ linux-2.4.20/drivers/net/e1000/e1000_main.c 2003-08-08 
> +++ 13:20:06.000000000 +0200
> @@ -1390,7 +1390,7 @@
>         netif_stop_queue(netdev);
>  
>     /* Reset the timer */
> -   mod_timer(&adapter->watchdog_timer, jiffies + 2 * HZ);
> +   mod_timer(&adapter->watchdog_timer, jiffies + HZ);
>  }

That should be OK if you're not linked at half duplex or using a 82541/7
Ethernet controller.  e1000_smartspeed() and e1000_adaptive_ifs() are
sensitive to the watchdog interval, so we'll need to make sure those are
OK before adjusting the timer from 2 to 1 seconds.  This issue is
tracker here: http://bugme.osdl.org/show_bug.cgi?id=1192.

-scott

Re: minor e1000 bug

[Hans-Peter Jansen]

Hi Scott,

On Monday 22 December 2003 06:26, Feldman, Scott wrote:
> > I would also be interested in a statement from intel fellows on
> > the reasoning behind this decision, since every user of gkrellm

> >     /* Reset the timer */
> > -   mod_timer(&adapter->watchdog_timer, jiffies + 2 * HZ);
> > +   mod_timer(&adapter->watchdog_timer, jiffies + HZ);
> >  }
>
> That should be OK if you're not linked at half duplex or using a
> 82541/7 Ethernet controller.  e1000_smartspeed() and
> e1000_adaptive_ifs() are sensitive to the watchdog interval, so
> we'll need to make sure those are OK before adjusting the timer
> from 2 to 1 seconds.  This issue is tracker here:
> http://bugme.osdl.org/show_bug.cgi?id=1192.

Thanks for clarification and the pointers. Nice to know, that this 
issue is still under investigation.

Let me add, that your and your companies strong continuous linux 
commitment/support has influenced and will influence our future 
hardware decisions. 

And NICs are a crucial part of our diskless setups..

> -scott

Thanks again and merry christmas,

Pete

<dream OT>
If only some manufacturer would pick up the _existing_ pieces, and 
create a barebone like fanless Pentium M based system with a CSA 
attached 8254x NIC. Add SSD for swap/suspend, or get nbd to work for 
those, and be done with a low current consumption/low heat/zero dB 
system, which easily outperforms current local harddisk setups.

BTW: I do remember 0 dB computing back in the 80ies on my 8MHz, 4 MB
Atari ST. Oh, well..
</dream OT>

Re: minor e1000 bug

[Ethan Weinstein]

Feldman, Scott wrote:
>>--- linux-2.4.20/drivers/net/e1000/e1000_main.c~    
>>2003-08-03 00:40:21.000000000 +0200
>>+++ linux-2.4.20/drivers/net/e1000/e1000_main.c 2003-08-08 
>>+++ 13:20:06.000000000 +0200
>>@@ -1390,7 +1390,7 @@
>>        netif_stop_queue(netdev);
>> 
>>    /* Reset the timer */
>>-   mod_timer(&adapter->watchdog_timer, jiffies + 2 * HZ);
>>+   mod_timer(&adapter->watchdog_timer, jiffies + HZ);
>> }
> 
> 
> That should be OK if you're not linked at half duplex or using a 82541/7
> Ethernet controller.  e1000_smartspeed() and e1000_adaptive_ifs() are
> sensitive to the watchdog interval, so we'll need to make sure those are
> OK before adjusting the timer from 2 to 1 seconds.  This issue is
> tracker here: http://bugme.osdl.org/show_bug.cgi?id=1192.

This modification appears to somewhat remedy the problem, however, 
bandwidth measurement seems to be much more accurate with many other 
cards.  By what method does, say, the 3c59x card export its statistics 
to /proc/net/dev that makes it easier to measure?

Ethan

RE: minor e1000 bug

[Feldman, Scott]

> This modification appears to somewhat remedy the problem, however, 
> bandwidth measurement seems to be much more accurate with many other 
> cards.  By what method does, say, the 3c59x card export its 
> statistics 
> to /proc/net/dev that makes it easier to measure?

e100 and e1000 both query h/w for stats on a timer (2 seconds) and cache
the results.  A call into the driver's get_stats function just returns
these cached values.  With e100, there is a problem in that issuing the
command to dump stats doesn't return right away, so rather than blocking
in the driver by waiting for the command to complete, the driver just
reads the results of the dump command 2 seconds prior, and then reissues
a new dump command.  So e100 stats are delayed by ~2 seconds.

3c59x (and others) query the h/w for stats in the driver's get_stats
function directly.  This gives up-to-date stats.  We could do this with
e1000, but it'll take a little bit of surgery because there is some
other code in the driver that is dependent on stats collected over 2
second period.  Nothing that can't be fixed.

-scott

Re: minor e1000 bug

[Ethan Weinstein]

Feldman, Scott wrote:
> 
> e100 and e1000 both query h/w for stats on a timer (2 seconds) and cache
> the results.  A call into the driver's get_stats function just returns
> these cached values.  With e100, there is a problem in that issuing the
> command to dump stats doesn't return right away, so rather than blocking
> in the driver by waiting for the command to complete, the driver just
> reads the results of the dump command 2 seconds prior, and then reissues
> a new dump command.  So e100 stats are delayed by ~2 seconds.
> 
> 3c59x (and others) query the h/w for stats in the driver's get_stats
> function directly.  This gives up-to-date stats.  We could do this with
> e1000, but it'll take a little bit of surgery because there is some
> other code in the driver that is dependent on stats collected over 2
> second period.  Nothing that can't be fixed.
> 

It'd be fantastic if we could get this implememted. I'm very pleased 
with the quality of this driver save for this one small issue.  I'd be 
glad to help test code if necessary as I have multiple systems using 
e1000's, and several are testboxes.  It'd be nice to have some 
semi-realtime stats from these cards.


-Ethan

Re: minor e1000 bug

[Ethan Weinstein]

Feldman, Scott wrote:
> On Mon, 22 Dec 2003, Ethan Weinstein wrote:
> 
> 
>>It'd be fantastic if we could get this implememted. I'm very pleased
>>with the quality of this driver save for this one small issue.  I'd be
>>glad to help test code if necessary as I have multiple systems using
>>e1000's, and several are testboxes.  It'd be nice to have some
>>semi-realtime stats from these cards.
> 
 >
> Ok, this is against 2.6.0, so you'll need to hand adjust it for other 
> kernels.  Let me know if you see any issues.
> 
> -scott


Much thanks. That seems to do it, tested lightly so far with two 
onboards (asus p4c800e, and supermicro X5DPL-IGM-0) using both kernels 
2.6.0 and 2.4.23, e1000-5.2.22, stats updates are much faster and seem 
more accurate. Will test with some PCI-X cards in the office tommorow, 
where I can really do some pounding.


-Ethan

Re: minor e1000 bug

[Feldman, Scott]

On Mon, 22 Dec 2003, Ethan Weinstein wrote:

> It'd be fantastic if we could get this implememted. I'm very pleased
> with the quality of this driver save for this one small issue.  I'd be
> glad to help test code if necessary as I have multiple systems using
> e1000's, and several are testboxes.  It'd be nice to have some
> semi-realtime stats from these cards.

Ok, this is against 2.6.0, so you'll need to hand adjust it for other 
kernels.  Let me know if you see any issues.

-scott


--- linux-2.6.0/drivers/net/e1000/e1000_main.c.orig	2003-12-23 15:24:20.000000000 -0800
+++ linux-2.6.0/drivers/net/e1000/e1000_main.c	2003-12-23 16:12:39.000000000 -0800
@@ -119,6 +119,7 @@
 void e1000_down(struct e1000_adapter *adapter);
 void e1000_reset(struct e1000_adapter *adapter);
 int e1000_set_spd_dplx(struct e1000_adapter *adapter, uint16_t spddplx);
+void e1000_update_stats(struct e1000_adapter *adapter);
 
 static int e1000_init_module(void);
 static void e1000_exit_module(void);
@@ -144,7 +145,6 @@
 static struct net_device_stats * e1000_get_stats(struct net_device *netdev);
 static int e1000_change_mtu(struct net_device *netdev, int new_mtu);
 static int e1000_set_mac(struct net_device *netdev, void *p);
-static void e1000_update_stats(struct e1000_adapter *adapter);
 static inline void e1000_irq_disable(struct e1000_adapter *adapter);
 static inline void e1000_irq_enable(struct e1000_adapter *adapter);
 static irqreturn_t e1000_intr(int irq, void *data, struct pt_regs *regs);
@@ -1415,6 +1415,17 @@
 	}
 
 	e1000_update_stats(adapter);
+
+	adapter->hw.tx_packet_delta = adapter->stats.tpt - adapter->tpt_old;
+	adapter->tpt_old = adapter->stats.tpt;
+	adapter->hw.collision_delta = adapter->stats.colc - adapter->colc_old;
+	adapter->colc_old = adapter->stats.colc;
+	
+	adapter->gorcl = adapter->stats.gorcl - adapter->gorcl_old;
+	adapter->gorcl_old = adapter->stats.gorcl;
+	adapter->gotcl = adapter->stats.gotcl - adapter->gotcl_old;
+	adapter->gotcl_old = adapter->stats.gotcl;
+
 	e1000_update_adaptive(&adapter->hw);
 
 	if(!netif_carrier_ok(netdev)) {
@@ -1860,6 +1871,7 @@
 {
 	struct e1000_adapter *adapter = netdev->priv;
 
+	e1000_update_stats(adapter);
 	return &adapter->net_stats;
 }
 
@@ -1918,7 +1930,7 @@
  * @adapter: board private structure
  **/
 
-static void
+void
 e1000_update_stats(struct e1000_adapter *adapter)
 {
 	struct e1000_hw *hw = &adapter->hw;
@@ -1936,8 +1948,7 @@
 
 	adapter->stats.crcerrs += E1000_READ_REG(hw, CRCERRS);
 	adapter->stats.gprc += E1000_READ_REG(hw, GPRC);
-	adapter->gorcl = E1000_READ_REG(hw, GORCL);
-	adapter->stats.gorcl += adapter->gorcl;
+	adapter->stats.gorcl += E1000_READ_REG(hw, GORCL);
 	adapter->stats.gorch += E1000_READ_REG(hw, GORCH);
 	adapter->stats.bprc += E1000_READ_REG(hw, BPRC);
 	adapter->stats.mprc += E1000_READ_REG(hw, MPRC);
@@ -1949,8 +1960,6 @@
 	adapter->stats.prc1023 += E1000_READ_REG(hw, PRC1023);
 	adapter->stats.prc1522 += E1000_READ_REG(hw, PRC1522);
 
-	spin_unlock_irqrestore(&adapter->stats_lock, flags);
-
 	/* the rest of the counters are only modified here */
 
 	adapter->stats.symerrs += E1000_READ_REG(hw, SYMERRS);
@@ -1968,8 +1977,7 @@
 	adapter->stats.xofftxc += E1000_READ_REG(hw, XOFFTXC);
 	adapter->stats.fcruc += E1000_READ_REG(hw, FCRUC);
 	adapter->stats.gptc += E1000_READ_REG(hw, GPTC);
-	adapter->gotcl = E1000_READ_REG(hw, GOTCL);
-	adapter->stats.gotcl += adapter->gotcl;
+	adapter->stats.gotcl += E1000_READ_REG(hw, GOTCL);
 	adapter->stats.gotch += E1000_READ_REG(hw, GOTCH);
 	adapter->stats.rnbc += E1000_READ_REG(hw, RNBC);
 	adapter->stats.ruc += E1000_READ_REG(hw, RUC);
@@ -2051,6 +2059,8 @@
 		   !e1000_read_phy_reg(hw, M88E1000_RX_ERR_CNTR, &phy_tmp))
 			adapter->phy_stats.receive_errors += phy_tmp;
 	}
+
+	spin_unlock_irqrestore(&adapter->stats_lock, flags);
 }
 
 /**
--- linux-2.6.0/drivers/net/e1000/e1000_ethtool.c.orig	2003-12-23 15:27:54.000000000 -0800
+++ linux-2.6.0/drivers/net/e1000/e1000_ethtool.c	2003-12-23 15:31:10.000000000 -0800
@@ -39,6 +39,7 @@
 extern void e1000_down(struct e1000_adapter *adapter);
 extern void e1000_reset(struct e1000_adapter *adapter);
 extern int e1000_set_spd_dplx(struct e1000_adapter *adapter, uint16_t spddplx);
+extern void e1000_update_stats(struct e1000_adapter *adapter);
 
 struct e1000_stats {
 	char stat_string[ETH_GSTRING_LEN];
@@ -1522,6 +1523,7 @@
 		} stats = { {ETHTOOL_GSTATS, E1000_STATS_LEN} };
 		int i;
 
+		e1000_update_stats(adapter);
 		for(i = 0; i < E1000_STATS_LEN; i++)
 			stats.data[i] = (e1000_gstrings_stats[i].sizeof_stat ==
 					sizeof(uint64_t)) ?
--- linux-2.6.0/drivers/net/e1000/e1000.h.orig	2003-12-23 16:09:14.000000000 -0800
+++ linux-2.6.0/drivers/net/e1000/e1000.h	2003-12-23 16:11:30.000000000 -0800
@@ -196,6 +196,9 @@
 	uint32_t tx_int_delay;
 	uint32_t tx_abs_int_delay;
 	uint32_t gotcl;
+	uint64_t gotcl_old;
+	uint64_t tpt_old;
+	uint64_t colc_old;
 	uint32_t tx_fifo_head;
 	uint32_t tx_head_addr;
 	uint32_t tx_fifo_size;
@@ -210,6 +213,7 @@
 	uint32_t rx_abs_int_delay;
 	boolean_t rx_csum;
 	uint32_t gorcl;
+	uint64_t gorcl_old;
 
 	/* Interrupt Throttle Rate */
 	uint32_t itr;