[PATCH 0/2] igb: drop stats due to OS cannot keep up

[PATCH 0/2] igb: drop stats due to OS cannot keep up

[Jesper Dangaard Brouer]

The following patchset address "drop" stats counters in the igb
driver (chips 82576 and 82575).  The drop stats addressed here
are due to the OS cannot keep up.

I have only tested it for the 82576 chip, as my (12 port) 82575
chip based NIC from Hotlava Systems recently died in my testlab :-(((

[First patch]
Implements reading of the per queue drop stats.
These stats only gets incremented if the DROP_EN bit it set (in
the SRRCTL register for that queue, or QDE reg is set).

[Second patch]
Address the case where the DROP_EN it NOT set.  In
this case a global register records the equivalent drop type. And
aggregation of these two types of drops into the device net_stats.

I strongly believe that these drops counters should be made
available to sysadm's through the normal device stats.  These
stats are essential when diagnosing server performance issues, as
these stats are due to the OS cannot keep up. I strongly believe
that it should be stored in the net_stats rx_fifo_errors counter,
to indicate performance issues.

-- 
Med venlig hilsen / Best regards
  Jesper Brouer
  ComX Networks A/S
  Linux Network developer
  Cand. Scient Datalog / MSc.
  Author of http://adsl-optimizer.dk
  LinkedIn: http://www.linkedin.com/in/brouer

[PATCH 1/2] igb: Implement reading of reg RQDPC (Receive Queue Drop Packet Count)

[Jesper Dangaard Brouer]

Implement reading the per queue drop stats register
RQDPC (Receive Queue Drop Packet Count).  It counts the number of
packets dropped by a queue due to lack of descriptors available.

Notice RQDPC (Receive Queue Drop Packet Count) stats only gets
incremented, if the DROP_EN bit it set (in the SRRCTL register
for that queue). If DROP_EN bit is NOT set, then the some what
equivalent count is stored in RNBC (not per queue basis).

The RQDPC register is only 12 bit, thus the precision might
suffer due to overrun in-between the watchdog polling interval.

Signed-off-by: Jesper Dangaard Brouer <hawk@comx.dk>
---

 drivers/net/igb/e1000_regs.h  |    2 ++
 drivers/net/igb/igb.h         |   13 ++++++++++---
 drivers/net/igb/igb_ethtool.c |   16 ++++++++++------
 drivers/net/igb/igb_main.c    |   14 ++++++++++++++
 4 files changed, 36 insertions(+), 9 deletions(-)


diff --git a/drivers/net/igb/e1000_regs.h b/drivers/net/igb/e1000_regs.h
index 0bd7728..85683e2 100644
--- a/drivers/net/igb/e1000_regs.h
+++ b/drivers/net/igb/e1000_regs.h
@@ -165,6 +165,8 @@ enum {
 				    : (0x0C018 + ((_n) * 0x40)))
 #define E1000_RXDCTL(_n)  ((_n) < 4 ? (0x02828 + ((_n) * 0x100)) \
 				    : (0x0C028 + ((_n) * 0x40)))
+#define E1000_RQDPC(_n)   ((_n) < 4 ? (0x02830 + ((_n) * 0x100)) \
+				    : (0x0C030 + ((_n) * 0x40)))
 #define E1000_TDBAL(_n)   ((_n) < 4 ? (0x03800 + ((_n) * 0x100)) \
 				    : (0x0E000 + ((_n) * 0x40)))
 #define E1000_TDBAH(_n)   ((_n) < 4 ? (0x03804 + ((_n) * 0x100)) \
diff --git a/drivers/net/igb/igb.h b/drivers/net/igb/igb.h
index 154c5ac..b2c98de 100644
--- a/drivers/net/igb/igb.h
+++ b/drivers/net/igb/igb.h
@@ -137,11 +137,17 @@ struct igb_buffer {
 	};
 };
 
-struct igb_queue_stats {
+struct igb_tx_queue_stats {
 	u64 packets;
 	u64 bytes;
 };
 
+struct igb_rx_queue_stats {
+	u64 packets;
+	u64 bytes;
+	u64 drops;
+};
+
 struct igb_ring {
 	struct igb_adapter *adapter; /* backlink */
 	void *desc;                  /* descriptor ring memory */
@@ -167,12 +173,13 @@ struct igb_ring {
 	union {
 		/* TX */
 		struct {
-			struct igb_queue_stats tx_stats;
+			struct igb_tx_queue_stats tx_stats;
 			bool detect_tx_hung;
 		};
 		/* RX */
 		struct {
-			struct igb_queue_stats rx_stats;
+			struct igb_rx_queue_stats rx_stats;
+			u64 rx_queue_drops;
 			struct napi_struct napi;
 			int set_itr;
 			struct igb_ring *buddy;
diff --git a/drivers/net/igb/igb_ethtool.c b/drivers/net/igb/igb_ethtool.c
index b8551a5..f0bf6f1 100644
--- a/drivers/net/igb/igb_ethtool.c
+++ b/drivers/net/igb/igb_ethtool.c
@@ -96,9 +96,10 @@ static const struct igb_stats igb_gstrings_stats[] = {
 };
 
 #define IGB_QUEUE_STATS_LEN \
-	((((struct igb_adapter *)netdev_priv(netdev))->num_rx_queues + \
-	 ((struct igb_adapter *)netdev_priv(netdev))->num_tx_queues) * \
-	(sizeof(struct igb_queue_stats) / sizeof(u64)))
+	(((((struct igb_adapter *)netdev_priv(netdev))->num_rx_queues) * \
+	  (sizeof(struct igb_rx_queue_stats) / sizeof(u64))) +		 \
+	 ((((struct igb_adapter *)netdev_priv(netdev))->num_tx_queues) * \
+	  (sizeof(struct igb_tx_queue_stats) / sizeof(u64))))
 #define IGB_GLOBAL_STATS_LEN	\
 	sizeof(igb_gstrings_stats) / sizeof(struct igb_stats)
 #define IGB_STATS_LEN (IGB_GLOBAL_STATS_LEN + IGB_QUEUE_STATS_LEN)
@@ -1960,7 +1961,8 @@ static void igb_get_ethtool_stats(struct net_device *netdev,
 {
 	struct igb_adapter *adapter = netdev_priv(netdev);
 	u64 *queue_stat;
-	int stat_count = sizeof(struct igb_queue_stats) / sizeof(u64);
+	int stat_count_tx = sizeof(struct igb_tx_queue_stats) / sizeof(u64);
+	int stat_count_rx = sizeof(struct igb_rx_queue_stats) / sizeof(u64);
 	int j;
 	int i;
 
@@ -1973,14 +1975,14 @@ static void igb_get_ethtool_stats(struct net_device *netdev,
 	for (j = 0; j < adapter->num_tx_queues; j++) {
 		int k;
 		queue_stat = (u64 *)&adapter->tx_ring[j].tx_stats;
-		for (k = 0; k < stat_count; k++)
+		for (k = 0; k < stat_count_tx; k++)
 			data[i + k] = queue_stat[k];
 		i += k;
 	}
 	for (j = 0; j < adapter->num_rx_queues; j++) {
 		int k;
 		queue_stat = (u64 *)&adapter->rx_ring[j].rx_stats;
-		for (k = 0; k < stat_count; k++)
+		for (k = 0; k < stat_count_rx; k++)
 			data[i + k] = queue_stat[k];
 		i += k;
 	}
@@ -2014,6 +2016,8 @@ static void igb_get_strings(struct net_device *netdev, u32 stringset, u8 *data)
 			p += ETH_GSTRING_LEN;
 			sprintf(p, "rx_queue_%u_bytes", i);
 			p += ETH_GSTRING_LEN;
+			sprintf(p, "rx_queue_%u_drops", i);
+			p += ETH_GSTRING_LEN;
 		}
 /*		BUG_ON(p - data != IGB_STATS_LEN * ETH_GSTRING_LEN); */
 		break;
diff --git a/drivers/net/igb/igb_main.c b/drivers/net/igb/igb_main.c
index 8de8629..06b01fc 100644
--- a/drivers/net/igb/igb_main.c
+++ b/drivers/net/igb/igb_main.c
@@ -3495,6 +3495,8 @@ void igb_update_stats(struct igb_adapter *adapter)
 	struct e1000_hw *hw = &adapter->hw;
 	struct pci_dev *pdev = adapter->pdev;
 	u16 phy_tmp;
+	u32 rqdpc_tmp;
+	int i;
 
 #define PHY_IDLE_ERROR_COUNT_MASK 0x00FF
 
@@ -3586,6 +3588,18 @@ void igb_update_stats(struct igb_adapter *adapter)
 
 	/* Rx Errors */
 
+	/* Read out drops stats per RX queue. Notice RQDPC (Receive
+	 * Queue Drop Packet Count) stats only gets incremented, if
+	 * the DROP_EN bit it set (in the SRRCTL register for that
+	 * queue). If DROP_EN bit is NOT set, then the some what
+	 * equivalent count is stored in RNBC (not per queue basis).
+	 * Also note the drop count is due to lack of available descriptors.
+	 */
+	for (i = 0; i < adapter->num_rx_queues; i++) {
+		rqdpc_tmp = rd32(E1000_RQDPC(i)) & 0xFFF;
+		adapter->rx_ring[i].rx_stats.drops += rqdpc_tmp;
+	}
+
 	/* RLEC on some newer hardware can be incorrect so build
 	* our own version based on RUC and ROC */
 	adapter->net_stats.rx_errors = adapter->stats.rxerrc +

RE: [PATCH 1/2] igb: Implement reading of reg RQDPC (Receive Queue Drop Packet Count)

[Williams, Mitch A]

 

>-----Original Message-----
>From: netdev-owner@vger.kernel.org 
>[mailto:netdev-owner@vger.kernel.org] On Behalf Of Jesper 
>Dangaard Brouer
>Sent: Thursday, May 07, 2009 6:37 AM
>To: Kirsher, Jeffrey T
>Cc: David S. Miller; netdev@vger.kernel.org; 
>e1000-devel@lists.sourceforge.net; Ronciak, John; Waskiewicz 
>Jr, Peter P
>Subject: [PATCH 1/2] igb: Implement reading of reg RQDPC 
>(Receive Queue Drop Packet Count)
>
>
>Implement reading the per queue drop stats register
>RQDPC (Receive Queue Drop Packet Count).  It counts the number of
>packets dropped by a queue due to lack of descriptors available.
>
>Notice RQDPC (Receive Queue Drop Packet Count) stats only gets
>incremented, if the DROP_EN bit it set (in the SRRCTL register
>for that queue). If DROP_EN bit is NOT set, then the some what
>equivalent count is stored in RNBC (not per queue basis).
>
>The RQDPC register is only 12 bit, thus the precision might
>suffer due to overrun in-between the watchdog polling interval.

NAK.  82575 doesn't have RQDPC registers.  You need to check
which part you're running on before you read those registers.

Other than that, this looks OK to me.

-Mitch

RE: [PATCH 1/2] igb: Implement reading of reg RQDPC (Receive Queue Drop Packet Count)

[Jesper Dangaard Brouer]

On Thu, 7 May 2009, Williams, Mitch A wrote:

>> Implement reading the per queue drop stats register
>> RQDPC (Receive Queue Drop Packet Count).  It counts the number of
>> packets dropped by a queue due to lack of descriptors available.
>>
>> Notice RQDPC (Receive Queue Drop Packet Count) stats only gets
>> incremented, if the DROP_EN bit it set (in the SRRCTL register
>> for that queue). If DROP_EN bit is NOT set, then the some what
>> equivalent count is stored in RNBC (not per queue basis).
>>
>> The RQDPC register is only 12 bit, thus the precision might
>> suffer due to overrun in-between the watchdog polling interval.
>
> NAK.  82575 doesn't have RQDPC registers.  You need to check
> which part you're running on before you read those registers.

Strange, I though the RQDPC register were compatible with the 82575 
registers, as the 82576 datasheet states:

Quote 82576 DS:
   Note: In order to keep compatibility with the 82575, for queues 0-3,
   these registers are aliased to addresses 0x2830, 0x2930, 0x2A30 & 0x2B30
   respectively.

And I do take care of reading the aliased adresses in the RQDPC define 
statement for queues 0-3.

Well, I unfortunatly done have a 82575 based NIC anymore (as my prototype 
NIC from Hotlava Systems Inc. just died).  And I don't have the 82575 
datasheet, it does not seem to be available!?

Any chance I could get the 82575 register datasheet?


> Other than that, this looks OK to me.

That sounds good :-)

ps. It will be monday or tuesday before I have time to repost the 
patchset.

Cheers,
   Jesper Brouer

--
-------------------------------------------------------------------
MSc. Master of Computer Science
Dept. of Computer Science, University of Copenhagen
Author of http://www.adsl-optimizer.dk
-------------------------------------------------------------------

Re: [PATCH 1/2] igb: Implement reading of reg RQDPC (Receive Queue Drop Packet Count)

[Stephen Hemminger]

On Thu, 7 May 2009 21:19:35 +0200 (CEST)
Jesper Dangaard Brouer <hawk@diku.dk> wrote:

> 
> On Thu, 7 May 2009, Williams, Mitch A wrote:
> 
> >> Implement reading the per queue drop stats register
> >> RQDPC (Receive Queue Drop Packet Count).  It counts the number of
> >> packets dropped by a queue due to lack of descriptors available.
> >>
> >> Notice RQDPC (Receive Queue Drop Packet Count) stats only gets
> >> incremented, if the DROP_EN bit it set (in the SRRCTL register
> >> for that queue). If DROP_EN bit is NOT set, then the some what
> >> equivalent count is stored in RNBC (not per queue basis).
> >>
> >> The RQDPC register is only 12 bit, thus the precision might
> >> suffer due to overrun in-between the watchdog polling interval.
> >
> > NAK.  82575 doesn't have RQDPC registers.  You need to check
> > which part you're running on before you read those registers.
> 
> Strange, I though the RQDPC register were compatible with the 82575 
> registers, as the 82576 datasheet states:
> 
> Quote 82576 DS:
>    Note: In order to keep compatibility with the 82575, for queues 0-3,
>    these registers are aliased to addresses 0x2830, 0x2930, 0x2A30 & 0x2B30
>    respectively.
> 
> And I do take care of reading the aliased adresses in the RQDPC define 
> statement for queues 0-3.
> 
> Well, I unfortunatly done have a 82575 based NIC anymore (as my prototype 
> NIC from Hotlava Systems Inc. just died).  And I don't have the 82575 
> datasheet, it does not seem to be available!?
> 
> Any chance I could get the 82575 register datasheet?
> 
> 
> > Other than that, this looks OK to me.
> 
> That sounds good :-)
> 
> ps. It will be monday or tuesday before I have time to repost the 
> patchset.
> 
> Cheers,
>    Jesper Brouer
> 


I'll test the patch, but what would happen if the registers were not
there?

RE: [PATCH 1/2] igb: Implement reading of reg RQDPC (Receive Queue Drop Packet Count)

[Jesper Dangaard Brouer]

On Thu, 2009-05-07 at 10:06 -0600, Williams, Mitch A wrote: 

> NAK.  82575 doesn't have RQDPC registers.  You need to check
> which part you're running on before you read those registers.

I have now tested it on a 82575 chip NIC. (I just got my 82575 NIC
working again (This 12 port monster from Hotlava Systems, just needed
more power on PCIe 100Watt)).

I don't see the reason for doing special checks for the 82575. Reading
the RQDPC registers on 82575 always returns 0.  I don't see any harm in
that!?  (it also returns zero in overload situations)

What do you want to redraw your NAK?

I have adjusted the commit message in the patch below...

-- 
Med venlig hilsen / Best regards
  Jesper Brouer
  ComX Networks A/S
  Linux Network developer
  Cand. Scient Datalog / MSc.
  Author of http://adsl-optimizer.dk
  LinkedIn: http://www.linkedin.com/in/brouer

[PATCH 1/2 v3] igb: Implement reading of reg RQDPC (Receive Queue Drop Packet Count)

Implement reading the per queue drop stats register
RQDPC (Receive Queue Drop Packet Count).  It counts the number of
packets dropped by a queue due to lack of descriptors available.

Notice the 82575 chip does not have the RQDPC register.  Reading the
register anyhow always results in value 0, which makes this patch
harmless on the 82575 chip.

Notice RQDPC (Receive Queue Drop Packet Count) stats only gets
incremented, if the DROP_EN bit it set (in the SRRCTL register
for that queue). If DROP_EN bit is NOT set, then the some what
equivalent count is stored in RNBC (not per queue basis).

The RQDPC register is only 12 bit, thus the precision might
suffer due to overrun in-between the watchdog polling interval.

Signed-off-by: Jesper Dangaard Brouer <hawk@comx.dk>
---

 drivers/net/igb/e1000_regs.h  |    2 ++
 drivers/net/igb/igb.h         |   13 ++++++++++---
 drivers/net/igb/igb_ethtool.c |   16 ++++++++++------
 drivers/net/igb/igb_main.c    |   14 ++++++++++++++
 4 files changed, 36 insertions(+), 9 deletions(-)


diff --git a/drivers/net/igb/e1000_regs.h b/drivers/net/igb/e1000_regs.h
index 0bd7728..85683e2 100644
--- a/drivers/net/igb/e1000_regs.h
+++ b/drivers/net/igb/e1000_regs.h
@@ -165,6 +165,8 @@ enum {
 				    : (0x0C018 + ((_n) * 0x40)))
 #define E1000_RXDCTL(_n)  ((_n) < 4 ? (0x02828 + ((_n) * 0x100)) \
 				    : (0x0C028 + ((_n) * 0x40)))
+#define E1000_RQDPC(_n)   ((_n) < 4 ? (0x02830 + ((_n) * 0x100)) \
+				    : (0x0C030 + ((_n) * 0x40)))
 #define E1000_TDBAL(_n)   ((_n) < 4 ? (0x03800 + ((_n) * 0x100)) \
 				    : (0x0E000 + ((_n) * 0x40)))
 #define E1000_TDBAH(_n)   ((_n) < 4 ? (0x03804 + ((_n) * 0x100)) \
diff --git a/drivers/net/igb/igb.h b/drivers/net/igb/igb.h
index 154c5ac..b2c98de 100644
--- a/drivers/net/igb/igb.h
+++ b/drivers/net/igb/igb.h
@@ -137,11 +137,17 @@ struct igb_buffer {
 	};
 };
 
-struct igb_queue_stats {
+struct igb_tx_queue_stats {
 	u64 packets;
 	u64 bytes;
 };
 
+struct igb_rx_queue_stats {
+	u64 packets;
+	u64 bytes;
+	u64 drops;
+};
+
 struct igb_ring {
 	struct igb_adapter *adapter; /* backlink */
 	void *desc;                  /* descriptor ring memory */
@@ -167,12 +173,13 @@ struct igb_ring {
 	union {
 		/* TX */
 		struct {
-			struct igb_queue_stats tx_stats;
+			struct igb_tx_queue_stats tx_stats;
 			bool detect_tx_hung;
 		};
 		/* RX */
 		struct {
-			struct igb_queue_stats rx_stats;
+			struct igb_rx_queue_stats rx_stats;
+			u64 rx_queue_drops;
 			struct napi_struct napi;
 			int set_itr;
 			struct igb_ring *buddy;
diff --git a/drivers/net/igb/igb_ethtool.c b/drivers/net/igb/igb_ethtool.c
index b8551a5..f0bf6f1 100644
--- a/drivers/net/igb/igb_ethtool.c
+++ b/drivers/net/igb/igb_ethtool.c
@@ -96,9 +96,10 @@ static const struct igb_stats igb_gstrings_stats[] = {
 };
 
 #define IGB_QUEUE_STATS_LEN \
-	((((struct igb_adapter *)netdev_priv(netdev))->num_rx_queues + \
-	 ((struct igb_adapter *)netdev_priv(netdev))->num_tx_queues) * \
-	(sizeof(struct igb_queue_stats) / sizeof(u64)))
+	(((((struct igb_adapter *)netdev_priv(netdev))->num_rx_queues) * \
+	  (sizeof(struct igb_rx_queue_stats) / sizeof(u64))) +		 \
+	 ((((struct igb_adapter *)netdev_priv(netdev))->num_tx_queues) * \
+	  (sizeof(struct igb_tx_queue_stats) / sizeof(u64))))
 #define IGB_GLOBAL_STATS_LEN	\
 	sizeof(igb_gstrings_stats) / sizeof(struct igb_stats)
 #define IGB_STATS_LEN (IGB_GLOBAL_STATS_LEN + IGB_QUEUE_STATS_LEN)
@@ -1960,7 +1961,8 @@ static void igb_get_ethtool_stats(struct net_device *netdev,
 {
 	struct igb_adapter *adapter = netdev_priv(netdev);
 	u64 *queue_stat;
-	int stat_count = sizeof(struct igb_queue_stats) / sizeof(u64);
+	int stat_count_tx = sizeof(struct igb_tx_queue_stats) / sizeof(u64);
+	int stat_count_rx = sizeof(struct igb_rx_queue_stats) / sizeof(u64);
 	int j;
 	int i;
 
@@ -1973,14 +1975,14 @@ static void igb_get_ethtool_stats(struct net_device *netdev,
 	for (j = 0; j < adapter->num_tx_queues; j++) {
 		int k;
 		queue_stat = (u64 *)&adapter->tx_ring[j].tx_stats;
-		for (k = 0; k < stat_count; k++)
+		for (k = 0; k < stat_count_tx; k++)
 			data[i + k] = queue_stat[k];
 		i += k;
 	}
 	for (j = 0; j < adapter->num_rx_queues; j++) {
 		int k;
 		queue_stat = (u64 *)&adapter->rx_ring[j].rx_stats;
-		for (k = 0; k < stat_count; k++)
+		for (k = 0; k < stat_count_rx; k++)
 			data[i + k] = queue_stat[k];
 		i += k;
 	}
@@ -2014,6 +2016,8 @@ static void igb_get_strings(struct net_device *netdev, u32 stringset, u8 *data)
 			p += ETH_GSTRING_LEN;
 			sprintf(p, "rx_queue_%u_bytes", i);
 			p += ETH_GSTRING_LEN;
+			sprintf(p, "rx_queue_%u_drops", i);
+			p += ETH_GSTRING_LEN;
 		}
 /*		BUG_ON(p - data != IGB_STATS_LEN * ETH_GSTRING_LEN); */
 		break;
diff --git a/drivers/net/igb/igb_main.c b/drivers/net/igb/igb_main.c
index 8de8629..06b01fc 100644
--- a/drivers/net/igb/igb_main.c
+++ b/drivers/net/igb/igb_main.c
@@ -3495,6 +3495,8 @@ void igb_update_stats(struct igb_adapter *adapter)
 	struct e1000_hw *hw = &adapter->hw;
 	struct pci_dev *pdev = adapter->pdev;
 	u16 phy_tmp;
+	u32 rqdpc_tmp;
+	int i;
 
 #define PHY_IDLE_ERROR_COUNT_MASK 0x00FF
 
@@ -3586,6 +3588,18 @@ void igb_update_stats(struct igb_adapter *adapter)
 
 	/* Rx Errors */
 
+	/* Read out drops stats per RX queue. Notice RQDPC (Receive
+	 * Queue Drop Packet Count) stats only gets incremented, if
+	 * the DROP_EN bit it set (in the SRRCTL register for that
+	 * queue). If DROP_EN bit is NOT set, then the some what
+	 * equivalent count is stored in RNBC (not per queue basis).
+	 * Also note the drop count is due to lack of available descriptors.
+	 */
+	for (i = 0; i < adapter->num_rx_queues; i++) {
+		rqdpc_tmp = rd32(E1000_RQDPC(i)) & 0xFFF;
+		adapter->rx_ring[i].rx_stats.drops += rqdpc_tmp;
+	}
+
 	/* RLEC on some newer hardware can be incorrect so build
 	* our own version based on RUC and ROC */
 	adapter->net_stats.rx_errors = adapter->stats.rxerrc +

RE: [PATCH 1/2] igb: Implement reading of reg RQDPC (Receive Queue Drop Packet Count)

[Williams, Mitch A]

 

>-----Original Message-----
>From: Jesper Dangaard Brouer [mailto:hawk@comx.dk] 
>Sent: Monday, May 11, 2009 2:50 AM
>
>I have now tested it on a 82575 chip NIC. (I just got my 82575 NIC
>working again (This 12 port monster from Hotlava Systems, just needed
>more power on PCIe 100Watt)).
>
>I don't see the reason for doing special checks for the 82575. Reading
>the RQDPC registers on 82575 always returns 0.  I don't see any harm in
>that!?  (it also returns zero in overload situations)
>
>What do you want to redraw your NAK?
>

Jesper, I still stand by my NAK.  It's never ever a good idea to read
non-existent hardware registers.  You don't know what effect those
reads will have on the hardware.  These addresses may be aliased to other
registers without being documented.  Hardware designers do this all the
time to save a few gates in the address decode logic, and these aliases
may or may not be documented.  If this is the case, reading these
registers will have unintended consequences.  You might be clearing some
other statistic, or worse.

Since we just don't know, it's better to be safe than sorry.  Wrap
those register reads so they only happen on 82576, and I'll happily
ack your patch.

-Mitch

Re: [PATCH 1/2] igb: Implement reading of reg RQDPC (Receive Queue Drop Packet Count)

[Jesper Dangaard Brouer]

On Wed, 2009-05-13 at 15:07 -0600, Williams, Mitch A wrote:
>  
> >-----Original Message-----
> >From: Jesper Dangaard Brouer [mailto:hawk@comx.dk] 
> >Sent: Monday, May 11, 2009 2:50 AM
> >
> >I have now tested it on a 82575 chip NIC. (I just got my 82575 NIC
> >working again (This 12 port monster from Hotlava Systems, just needed
> >more power on PCIe 100Watt)).
> >
> >I don't see the reason for doing special checks for the 82575. Reading
> >the RQDPC registers on 82575 always returns 0.  I don't see any harm in
> >that!?  (it also returns zero in overload situations)
> >
> >What do you want to redraw your NAK?
> >
> 
> Jesper, I still stand by my NAK.  It's never ever a good idea to read
> non-existent hardware registers.  You don't know what effect those
> reads will have on the hardware.  These addresses may be aliased to other
> registers without being documented.  Hardware designers do this all the
> time to save a few gates in the address decode logic, and these aliases
> may or may not be documented.  If this is the case, reading these
> registers will have unintended consequences.  You might be clearing some
> other statistic, or worse.
> 
> Since we just don't know, it's better to be safe than sorry.  Wrap
> those register reads so they only happen on 82576, and I'll happily
> ack your patch.

Fine, you argumented well for your case.

I'll repost some patches when time permits...

-- 
Med venlig hilsen / Best regards
  Jesper Brouer
  ComX Networks A/S
  Linux Network developer
  Cand. Scient Datalog / MSc.
  Author of http://adsl-optimizer.dk
  LinkedIn: http://www.linkedin.com/in/brouer


------------------------------------------------------------------------------
The NEW KODAK i700 Series Scanners deliver under ANY circumstances! Your
production scanning environment may not be a perfect world - but thanks to
Kodak, there's a perfect scanner to get the job done! With the NEW KODAK i700
Series Scanner you'll get full speed at 300 dpi even with all image 
processing features enabled. http://p.sf.net/sfu/kodak-com

[PATCH 2/2] igb: Record host memory receive overflow in net_stats

[Jesper Dangaard Brouer]

The RNBC (Receive No Buffers Count) register for the 82576, indicate
that frames were received when there were no available buffers in host
memory to store those frames (receive descriptor head and tail
pointers were equal).  The packet is still received by the NIC if
there is space in the FIFO on the NIC.

I have shown that this situation can arise when the kernel is too
busy else where.

As the the RNBC value is not necessary a packet drop, I choose to
store the RNBC value in net_stats.rx_fifo_errors, as its potentically
not a true drop.

Saving the stats in dev->net_stats makes it visible via
/proc/net/dev as "fifo", and thus viewable to ifconfig
as "overruns" and 'netstat -i' as "RX-OVR".

The Receive No Buffers Count (RNBC) can already be queried by
ethtool -S as "rx_no_buffer_count".

Signed-off-by: Jesper Dangaard Brouer <hawk@comx.dk>
---

 drivers/net/igb/igb_main.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)


diff --git a/drivers/net/igb/igb_main.c b/drivers/net/igb/igb_main.c
index 06b01fc..539b9f8 100644
--- a/drivers/net/igb/igb_main.c
+++ b/drivers/net/igb/igb_main.c
@@ -3496,6 +3496,7 @@ void igb_update_stats(struct igb_adapter *adapter)
 	struct pci_dev *pdev = adapter->pdev;
 	u16 phy_tmp;
 	u32 rqdpc_tmp;
+	u64 rqdpc_total = 0;
 	int i;
 
 #define PHY_IDLE_ERROR_COUNT_MASK 0x00FF
@@ -3598,7 +3599,15 @@ void igb_update_stats(struct igb_adapter *adapter)
 	for (i = 0; i < adapter->num_rx_queues; i++) {
 		rqdpc_tmp = rd32(E1000_RQDPC(i)) & 0xFFF;
 		adapter->rx_ring[i].rx_stats.drops += rqdpc_tmp;
+		rqdpc_total += adapter->rx_ring[i].rx_stats.drops;
 	}
+	adapter->net_stats.rx_fifo_errors = rqdpc_total;
+
+	/* Note RNBC (Receive No Buffers Count) is an not an exact
+	 * drop count as the hardware FIFO might save the day.  Thats
+	 * one of the reason for saving it in rx_fifo_errors, as its
+	 * potentically not a true drop. */
+	adapter->net_stats.rx_fifo_errors += adapter->stats.rnbc;
 
 	/* RLEC on some newer hardware can be incorrect so build
 	* our own version based on RUC and ROC */

Re: [PATCH 0/2] igb: drop stats due to OS cannot keep up

[Jeff Kirsher]

On Thu, May 7, 2009 at 6:35 AM, Jesper Dangaard Brouer <hawk@comx.dk> wrote:
>
> The following patchset address "drop" stats counters in the igb
> driver (chips 82576 and 82575).  The drop stats addressed here
> are due to the OS cannot keep up.
>
> I have only tested it for the 82576 chip, as my (12 port) 82575
> chip based NIC from Hotlava Systems recently died in my testlab :-(((
>
> [First patch]
> Implements reading of the per queue drop stats.
> These stats only gets incremented if the DROP_EN bit it set (in
> the SRRCTL register for that queue, or QDE reg is set).
>
> [Second patch]
> Address the case where the DROP_EN it NOT set.  In
> this case a global register records the equivalent drop type. And
> aggregation of these two types of drops into the device net_stats.
>
> I strongly believe that these drops counters should be made
> available to sysadm's through the normal device stats.  These
> stats are essential when diagnosing server performance issues, as
> these stats are due to the OS cannot keep up. I strongly believe
> that it should be stored in the net_stats rx_fifo_errors counter,
> to indicate performance issues.
>
> --
> Med venlig hilsen / Best regards
>  Jesper Brouer
>  ComX Networks A/S
>  Linux Network developer
>  Cand. Scient Datalog / MSc.
>  Author of http://adsl-optimizer.dk
>  LinkedIn: http://www.linkedin.com/in/brouer
>

Jesper - I have pulled in your patches, and made the necessary
modifications based on Mitch's comments.  Right now, they are in
testing and I should be pushing them to Dave in the next couple of
days.

-- 
Cheers,
Jeff

------------------------------------------------------------------------------
Crystal Reports - New Free Runtime and 30 Day Trial
Check out the new simplified licensing option that enables 
unlimited royalty-free distribution of the report engine 
for externally facing server and web deployment. 
http://p.sf.net/sfu/businessobjects

Re: [PATCH 0/2] igb: drop stats due to OS cannot keep up

[Jesper Dangaard Brouer]

On Thu, 2009-05-14 at 15:43 -0700, Jeff Kirsher wrote:
> Jesper - I have pulled in your patches, and made the necessary
> modifications based on Mitch's comments.  Right now, they are in
> testing and I should be pushing them to Dave in the next couple of
> days.

Sounds great! :-)

Do you or e1000-devel have a public git tree?
So I can work off that next time I have some changes, thus it would be
easier for you to merge in... (this time around I rebased my changes
each time some of you igb changes got into daveM's net-next-2.6 tree)

Cheers
-- 
Med venlig hilsen / Best regards
  Jesper Brouer
  ComX Networks A/S
  Linux Network developer
  Cand. Scient Datalog / MSc.
  Author of http://adsl-optimizer.dk
  LinkedIn: http://www.linkedin.com/in/brouer


------------------------------------------------------------------------------
Crystal Reports - New Free Runtime and 30 Day Trial
Check out the new simplified licensing option that enables 
unlimited royalty-free distribution of the report engine 
for externally facing server and web deployment. 
http://p.sf.net/sfu/businessobjects

Re: [PATCH 0/2] igb: drop stats due to OS cannot keep up

[Jeff Kirsher]

On Fri, May 15, 2009 at 12:41 AM, Jesper Dangaard Brouer <hawk@comx.dk> wrote:
> On Thu, 2009-05-14 at 15:43 -0700, Jeff Kirsher wrote:
>> Jesper - I have pulled in your patches, and made the necessary
>> modifications based on Mitch's comments.  Right now, they are in
>> testing and I should be pushing them to Dave in the next couple of
>> days.
>
> Sounds great! :-)
>
> Do you or e1000-devel have a public git tree?
> So I can work off that next time I have some changes, thus it would be
> easier for you to merge in... (this time around I rebased my changes
> each time some of you igb changes got into daveM's net-next-2.6 tree)
>
> Cheers
> --
> Med venlig hilsen / Best regards
>  Jesper Brouer
>  ComX Networks A/S
>  Linux Network developer
>  Cand. Scient Datalog / MSc.
>  Author of http://adsl-optimizer.dk
>  LinkedIn: http://www.linkedin.com/in/brouer

No we do not have a public git tree, but do not worry about it.  Feel
free to send patches, and I can work through most of the issues if any
arise.  So no worries there...

Regarding your patches, during testing we saw a significant difference
in the stats reported by ifconfig versus the stats reported by
ethtool.  Reason being that ethtool stats do not take into account the
RQDPC numbers.  I have been looking at fixing that in the patches, so
while all the testing looks good (except for the difference in
numbers) the patches will be delayed a day or so, while I fix the
ethtool reporting of RQDPC.

-- 
Cheers,
Jeff