[PATCH] s2io: read rx_packets count from the hardware stats

[PATCH] s2io: read rx_packets count from the hardware stats

[Michal Schmidt]

Most of the statistics the s2io driver provides in /proc/net/dev
it reads directly from the hardware counters. For some reason it does
not do that for rx_packets. It counts rx_packets purely in software.

A customer reported a bug where in /proc/net/dev the 'multicast' counter
was increasing faster than 'packets' ( = rx_packets in the source code).
This confuses userspace, especially snmpd.

The hardware provides a counter for the total number of received
frames (RMAC_VLD_FRMS) which the driver can use for the rx_packets
statistic. By reading both statistics from the hardware it makes sure
that all multicast frames are included in the total.

The customer tested a patch like this (only modified for RHEL5) with
S2io Inc. Xframe II 10Gbps Ethernet (rev 02)
and it fixed the problem.

Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
---

 drivers/net/s2io.c |   11 ++++++-----
 drivers/net/s2io.h |    1 -
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/s2io.c b/drivers/net/s2io.c
index 668327c..eefc4b2 100644
--- a/drivers/net/s2io.c
+++ b/drivers/net/s2io.c
@@ -4919,6 +4919,10 @@ static struct net_device_stats *s2io_get_stats(struct net_device *dev)
 		sp->stats.tx_packets;
 	sp->stats.tx_packets = le32_to_cpu(stats->tmac_frms);
 
+	dev->stats.rx_packets += le32_to_cpu(stats->rmac_vld_frms) -
+		sp->stats.rx_packets;
+	sp->stats.rx_packets = le32_to_cpu(stats->rmac_vld_frms);
+
 	dev->stats.tx_errors += le32_to_cpu(stats->tmac_any_err_frms) -
 		sp->stats.tx_errors;
 	sp->stats.tx_errors = le32_to_cpu(stats->tmac_any_err_frms);
@@ -4935,12 +4939,11 @@ static struct net_device_stats *s2io_get_stats(struct net_device *dev)
 		sp->stats.rx_length_errors;
 	sp->stats.rx_length_errors = le64_to_cpu(stats->rmac_long_frms);
 
-	/* collect per-ring rx_packets and rx_bytes */
-	dev->stats.rx_packets = dev->stats.rx_bytes = 0;
+	/* collect per-ring rx_bytes */
+	dev->stats.rx_bytes = 0;
 	for (i = 0; i < config->rx_ring_num; i++) {
 		struct ring_info *ring = &mac_control->rings[i];
 
-		dev->stats.rx_packets += ring->rx_packets;
 		dev->stats.rx_bytes += ring->rx_bytes;
 	}
 
@@ -7455,8 +7458,6 @@ static int rx_osm_handler(struct ring_info *ring_data, struct RxD_t * rxdp)
 		}
 	}
 
-	/* Updating statistics */
-	ring_data->rx_packets++;
 	rxdp->Host_Control = 0;
 	if (sp->rxd_mode == RXD_MODE_1) {
 		int len = RXD_GET_BUFFER0_SIZE_1(rxdp->Control_2);
diff --git a/drivers/net/s2io.h b/drivers/net/s2io.h
index 47c36e0..4da9ab8 100644
--- a/drivers/net/s2io.h
+++ b/drivers/net/s2io.h
@@ -747,7 +747,6 @@ struct ring_info {
 	struct buffAdd **ba;
 
 	/* per-Ring statistics */
-	unsigned long rx_packets;
 	unsigned long rx_bytes;
 } ____cacheline_aligned;
 

Re: [PATCH] s2io: read rx_packets count from the hardware stats

[David Miller]

From: Michal Schmidt <mschmidt@redhat.com>
Date: Fri, 25 Jun 2010 01:32:32 +0200

> Most of the statistics the s2io driver provides in /proc/net/dev
> it reads directly from the hardware counters. For some reason it does
> not do that for rx_packets. It counts rx_packets purely in software.
> 
> A customer reported a bug where in /proc/net/dev the 'multicast' counter
> was increasing faster than 'packets' ( = rx_packets in the source code).
> This confuses userspace, especially snmpd.
> 
> The hardware provides a counter for the total number of received
> frames (RMAC_VLD_FRMS) which the driver can use for the rx_packets
> statistic. By reading both statistics from the hardware it makes sure
> that all multicast frames are included in the total.
> 
> The customer tested a patch like this (only modified for RHEL5) with
> S2io Inc. Xframe II 10Gbps Ethernet (rev 02)
> and it fixed the problem.
> 
> Signed-off-by: Michal Schmidt <mschmidt@redhat.com>

Please also use the rmac_data_octets HW statistic for rx_bytes
otherwise rx_bytes will be out of sync with the other stats too.

Re: [PATCH] s2io: read rx_packets count from the hardware stats

[Jon Mason]

On Thu, Jun 24, 2010 at 04:32:32PM -0700, Michal Schmidt wrote:
> Most of the statistics the s2io driver provides in /proc/net/dev
> it reads directly from the hardware counters. For some reason it does
> not do that for rx_packets. It counts rx_packets purely in software.
>
> A customer reported a bug where in /proc/net/dev the 'multicast' counter
> was increasing faster than 'packets' ( = rx_packets in the source code).
> This confuses userspace, especially snmpd.
>
> The hardware provides a counter for the total number of received
> frames (RMAC_VLD_FRMS) which the driver can use for the rx_packets
> statistic. By reading both statistics from the hardware it makes sure
> that all multicast frames are included in the total.

On the Xframe adapter, there is a issue with the multicast statistics
counter.  It includes broadcast and pause frames in its count.  This
is most likely the cause of the issue you are seeing.

While, looking over the patch you submitted, I noticed other issues
with the s2io_get_stats function.  I have a patch that I will push
soon that addresses the multicast issue, the issues I discovered, and
the octet issues Dave suggested.

Thanks,
Jon

>
> The customer tested a patch like this (only modified for RHEL5) with
> S2io Inc. Xframe II 10Gbps Ethernet (rev 02)
> and it fixed the problem.
>
> Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
> ---
>
>  drivers/net/s2io.c |   11 ++++++-----
>  drivers/net/s2io.h |    1 -
>  2 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/s2io.c b/drivers/net/s2io.c
> index 668327c..eefc4b2 100644
> --- a/drivers/net/s2io.c
> +++ b/drivers/net/s2io.c
> @@ -4919,6 +4919,10 @@ static struct net_device_stats *s2io_get_stats(struct net_device *dev)
>               sp->stats.tx_packets;
>       sp->stats.tx_packets = le32_to_cpu(stats->tmac_frms);
>
> +     dev->stats.rx_packets += le32_to_cpu(stats->rmac_vld_frms) -
> +             sp->stats.rx_packets;
> +     sp->stats.rx_packets = le32_to_cpu(stats->rmac_vld_frms);
> +
>       dev->stats.tx_errors += le32_to_cpu(stats->tmac_any_err_frms) -
>               sp->stats.tx_errors;
>       sp->stats.tx_errors = le32_to_cpu(stats->tmac_any_err_frms);
> @@ -4935,12 +4939,11 @@ static struct net_device_stats *s2io_get_stats(struct net_device *dev)
>               sp->stats.rx_length_errors;
>       sp->stats.rx_length_errors = le64_to_cpu(stats->rmac_long_frms);
>
> -     /* collect per-ring rx_packets and rx_bytes */
> -     dev->stats.rx_packets = dev->stats.rx_bytes = 0;
> +     /* collect per-ring rx_bytes */
> +     dev->stats.rx_bytes = 0;
>       for (i = 0; i < config->rx_ring_num; i++) {
>               struct ring_info *ring = &mac_control->rings[i];
>
> -             dev->stats.rx_packets += ring->rx_packets;
>               dev->stats.rx_bytes += ring->rx_bytes;
>       }
>
> @@ -7455,8 +7458,6 @@ static int rx_osm_handler(struct ring_info *ring_data, struct RxD_t * rxdp)
>               }
>       }
>
> -     /* Updating statistics */
> -     ring_data->rx_packets++;
>       rxdp->Host_Control = 0;
>       if (sp->rxd_mode == RXD_MODE_1) {
>               int len = RXD_GET_BUFFER0_SIZE_1(rxdp->Control_2);
> diff --git a/drivers/net/s2io.h b/drivers/net/s2io.h
> index 47c36e0..4da9ab8 100644
> --- a/drivers/net/s2io.h
> +++ b/drivers/net/s2io.h
> @@ -747,7 +747,6 @@ struct ring_info {
>       struct buffAdd **ba;
>
>       /* per-Ring statistics */
> -     unsigned long rx_packets;
>       unsigned long rx_bytes;
>  } ____cacheline_aligned;
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

The information and any attached documents contained in this message
may be confidential and/or legally privileged.  The message is
intended solely for the addressee(s).  If you are not the intended
recipient, you are hereby notified that any use, dissemination, or
reproduction is strictly prohibited and may be unlawful.  If you are
not the intended recipient, please contact the sender immediately by
return e-mail and destroy all copies of the original message.

[PATCH] s2io: resolve statistics issues

[Jon Mason]

This patch resolves a number of issues in the statistics gathering of
the s2io driver.

On Xframe adapters, the received multicast statistics counter includes
pause frames which are not indicated to the driver.  This can cause
issues where the multicast packet count is higher than what has actually
been received, possibly higher than the number of packets received.
    
The driver software counters are replaced with the adapter hardware
statistics for rx_packets, rx_bytes, and tx_bytes.  It also uses the
overflow registers to determine if the statistics wrapped the 32bit
register (removing the window of having a statistic value less than the
previous call).  rx_length_errors statistic now includes undersized
packets in addition to oversized packets in its counting.  Finally,
rx_crc_errors are now being counted.
    
Signed-off-by: Jon Mason <jon.mason@exar.com>

diff --git a/drivers/net/s2io.c b/drivers/net/s2io.c
index 22371f1..d0af924 100644
--- a/drivers/net/s2io.c
+++ b/drivers/net/s2io.c
@@ -3129,7 +3129,6 @@ static void tx_intr_handler(struct fifo_info *fifo_data)
 		pkt_cnt++;
 
 		/* Updating the statistics block */
-		nic->dev->stats.tx_bytes += skb->len;
 		swstats->mem_freed += skb->truesize;
 		dev_kfree_skb_irq(skb);
 
@@ -4900,48 +4899,81 @@ static void s2io_updt_stats(struct s2io_nic *sp)
  *  Return value:
  *  pointer to the updated net_device_stats structure.
  */
-
 static struct net_device_stats *s2io_get_stats(struct net_device *dev)
 {
 	struct s2io_nic *sp = netdev_priv(dev);
-	struct config_param *config = &sp->config;
 	struct mac_info *mac_control = &sp->mac_control;
 	struct stat_block *stats = mac_control->stats_info;
-	int i;
+	u64 delta;
 
 	/* Configure Stats for immediate updt */
 	s2io_updt_stats(sp);
 
-	/* Using sp->stats as a staging area, because reset (due to mtu
-	   change, for example) will clear some hardware counters */
-	dev->stats.tx_packets += le32_to_cpu(stats->tmac_frms) -
-		sp->stats.tx_packets;
-	sp->stats.tx_packets = le32_to_cpu(stats->tmac_frms);
-
-	dev->stats.tx_errors += le32_to_cpu(stats->tmac_any_err_frms) -
-		sp->stats.tx_errors;
-	sp->stats.tx_errors = le32_to_cpu(stats->tmac_any_err_frms);
-
-	dev->stats.rx_errors += le64_to_cpu(stats->rmac_drop_frms) -
-		sp->stats.rx_errors;
-	sp->stats.rx_errors = le64_to_cpu(stats->rmac_drop_frms);
-
-	dev->stats.multicast = le32_to_cpu(stats->rmac_vld_mcst_frms) -
-		sp->stats.multicast;
-	sp->stats.multicast = le32_to_cpu(stats->rmac_vld_mcst_frms);
-
-	dev->stats.rx_length_errors = le64_to_cpu(stats->rmac_long_frms) -
-		sp->stats.rx_length_errors;
-	sp->stats.rx_length_errors = le64_to_cpu(stats->rmac_long_frms);
+	/* A device reset will cause the on-adapter statistics to be zero'ed.
+	 * This can be done while running by changing the MTU.  To prevent the
+	 * system from having the stats zero'ed, the driver keeps a copy of the
+	 * last update to the system (which is also zero'ed on reset).  This
+	 * enables the driver to accurately know the delta between the last
+	 * update and the current update.
+	 */
+	delta = ((u64) le32_to_cpu(stats->rmac_vld_frms_oflow) << 32 |
+		le32_to_cpu(stats->rmac_vld_frms)) - sp->stats.rx_packets;
+	sp->stats.rx_packets += delta;
+	dev->stats.rx_packets += delta;
+
+	delta = ((u64) le32_to_cpu(stats->tmac_frms_oflow) << 32 |
+		le32_to_cpu(stats->tmac_frms)) - sp->stats.tx_packets;
+	sp->stats.tx_packets += delta;
+	dev->stats.tx_packets += delta;
+
+	delta = ((u64) le32_to_cpu(stats->rmac_data_octets_oflow) << 32 |
+		le32_to_cpu(stats->rmac_data_octets)) - sp->stats.rx_bytes;
+	sp->stats.rx_bytes += delta;
+	dev->stats.rx_bytes += delta;
+
+	delta = ((u64) le32_to_cpu(stats->tmac_data_octets_oflow) << 32 |
+		le32_to_cpu(stats->tmac_data_octets)) - sp->stats.tx_bytes;
+	sp->stats.tx_bytes += delta;
+	dev->stats.tx_bytes += delta;
+
+	delta = le64_to_cpu(stats->rmac_drop_frms) - sp->stats.rx_errors;
+	sp->stats.rx_errors += delta;
+	dev->stats.rx_errors += delta;
+
+	delta = ((u64) le32_to_cpu(stats->tmac_any_err_frms_oflow) << 32 |
+		le32_to_cpu(stats->tmac_any_err_frms)) - sp->stats.tx_errors;
+	sp->stats.tx_errors += delta;
+	dev->stats.tx_errors += delta;
+
+	delta = le64_to_cpu(stats->rmac_drop_frms) - sp->stats.rx_dropped;
+	sp->stats.rx_dropped += delta;
+	dev->stats.rx_dropped += delta;
+
+	delta = le64_to_cpu(stats->tmac_drop_frms) - sp->stats.tx_dropped;
+	sp->stats.tx_dropped += delta;
+	dev->stats.tx_dropped += delta;
+
+	/* The adapter MAC interprets pause frames as multicast packets, but
+	 * does not pass them up.  This erroneously increases the multicast
+	 * packet count and needs to be deducted when the multicast frame count
+	 * is queried.
+	 */
+	delta = (u64) le32_to_cpu(stats->rmac_vld_mcst_frms_oflow) << 32 |
+		le32_to_cpu(stats->rmac_vld_mcst_frms);
+	delta -= le64_to_cpu(stats->rmac_pause_ctrl_frms);
+	delta -= sp->stats.multicast;
+	sp->stats.multicast += delta;
+	dev->stats.multicast += delta;
 
-	/* collect per-ring rx_packets and rx_bytes */
-	dev->stats.rx_packets = dev->stats.rx_bytes = 0;
-	for (i = 0; i < config->rx_ring_num; i++) {
-		struct ring_info *ring = &mac_control->rings[i];
+	delta = ((u64) le32_to_cpu(stats->rmac_usized_frms_oflow) << 32 |
+		le32_to_cpu(stats->rmac_usized_frms)) +
+		le64_to_cpu(stats->rmac_long_frms) - sp->stats.rx_length_errors;
+	sp->stats.rx_length_errors += delta;
+	dev->stats.rx_length_errors += delta;
 
-		dev->stats.rx_packets += ring->rx_packets;
-		dev->stats.rx_bytes += ring->rx_bytes;
-	}
+	delta = le64_to_cpu(stats->rmac_fcs_err_frms) - sp->stats.rx_crc_errors;
+	sp->stats.rx_crc_errors += delta;
+	dev->stats.rx_crc_errors += delta;
 
 	return &dev->stats;
 }
@@ -7494,15 +7526,11 @@ static int rx_osm_handler(struct ring_info *ring_data, struct RxD_t * rxdp)
 		}
 	}
 
-	/* Updating statistics */
-	ring_data->rx_packets++;
 	rxdp->Host_Control = 0;
 	if (sp->rxd_mode == RXD_MODE_1) {
 		int len = RXD_GET_BUFFER0_SIZE_1(rxdp->Control_2);
 
-		ring_data->rx_bytes += len;
 		skb_put(skb, len);
-
 	} else if (sp->rxd_mode == RXD_MODE_3B) {
 		int get_block = ring_data->rx_curr_get_info.block_index;
 		int get_off = ring_data->rx_curr_get_info.offset;
@@ -7511,7 +7539,6 @@ static int rx_osm_handler(struct ring_info *ring_data, struct RxD_t * rxdp)
 		unsigned char *buff = skb_push(skb, buf0_len);
 
 		struct buffAdd *ba = &ring_data->ba[get_block][get_off];
-		ring_data->rx_bytes += buf0_len + buf2_len;
 		memcpy(buff, ba->ba_0, buf0_len);
 		skb_put(skb, buf2_len);
 	}
diff --git a/drivers/net/s2io.h b/drivers/net/s2io.h
index 47c36e0..5e52c75 100644
--- a/drivers/net/s2io.h
+++ b/drivers/net/s2io.h
@@ -745,10 +745,6 @@ struct ring_info {
 
 	/* Buffer Address store. */
 	struct buffAdd **ba;
-
-	/* per-Ring statistics */
-	unsigned long rx_packets;
-	unsigned long rx_bytes;
 } ____cacheline_aligned;
 
 /* Fifo specific structure */

Re: [PATCH] s2io: resolve statistics issues

[David Miller]

From: Jon Mason <jon.mason@exar.com>
Date: Fri, 2 Jul 2010 14:13:49 -0500

> This patch resolves a number of issues in the statistics gathering of
> the s2io driver.
> 
> On Xframe adapters, the received multicast statistics counter includes
> pause frames which are not indicated to the driver.  This can cause
> issues where the multicast packet count is higher than what has actually
> been received, possibly higher than the number of packets received.
>     
> The driver software counters are replaced with the adapter hardware
> statistics for rx_packets, rx_bytes, and tx_bytes.  It also uses the
> overflow registers to determine if the statistics wrapped the 32bit
> register (removing the window of having a statistic value less than the
> previous call).  rx_length_errors statistic now includes undersized
> packets in addition to oversized packets in its counting.  Finally,
> rx_crc_errors are now being counted.
>     
> Signed-off-by: Jon Mason <jon.mason@exar.com>

Looks good, applied.