[dpdk-dev] [PATCH 0/4] Remove CRC from byte counters

[dpdk-dev] [PATCH 0/4] Remove CRC from byte counters

[Harry van Haaren]

This patchset removes CRC bytes from byte statistics in the following
PMDs: igb, ixgbe, i40e, fm10k.

This brings the reported byte statistics in-line with other NICs,
providing consistency.

Removing the CRC from byte counters in i40e resolves a bug, see
i40e commit message for details.


Harry van Haaren (4):
  e1000: remove crc size from all byte counters
  ixgbe: remove crc size from all byte counters
  i40e: fix rx/tx size mismatch, remove crc bytes
  fm10k: remove crc size from all byte counters

 drivers/net/e1000/igb_ethdev.c   | 19 ++++++++++++++--
 drivers/net/fm10k/fm10k_ethdev.c |  8 ++++---
 drivers/net/i40e/i40e_ethdev.c   | 33 ++++++++++++++++++----------
 drivers/net/ixgbe/ixgbe_ethdev.c | 47 ++++++++++++++++++++++++++++++----------
 4 files changed, 79 insertions(+), 28 deletions(-)

-- 
1.9.1

[dpdk-dev] [PATCH 1/4] e1000: remove crc size from all byte counters

[Harry van Haaren]

This patch removes the crc bytes from byte counter statistics.

Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
---
 drivers/net/e1000/igb_ethdev.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/net/e1000/igb_ethdev.c b/drivers/net/e1000/igb_ethdev.c
index 88995b0..0ad7341 100644
--- a/drivers/net/e1000/igb_ethdev.c
+++ b/drivers/net/e1000/igb_ethdev.c
@@ -1480,6 +1480,13 @@ igb_read_stats_registers(struct e1000_hw *hw, struct e1000_hw_stats *stats)
 {
 	int pause_frames;
 
+	uint64_t old_gprc  = stats->gprc;
+	uint64_t old_gptc  = stats->gptc;
+	uint64_t old_tpr   = stats->tpr;
+	uint64_t old_tpt   = stats->tpt;
+	uint64_t old_rpthc = stats->rpthc;
+	uint64_t old_hgptc = stats->hgptc;
+
 	if(hw->phy.media_type == e1000_media_type_copper ||
 	    (E1000_READ_REG(hw, E1000_STATUS) & E1000_STATUS_LU)) {
 		stats->symerrs +=
@@ -1521,10 +1528,13 @@ igb_read_stats_registers(struct e1000_hw *hw, struct e1000_hw_stats *stats)
 	/* For the 64-bit byte counters the low dword must be read first. */
 	/* Both registers clear on the read of the high dword */
 
+	/* Workaround CRC bytes included in size, take away 4 bytes/packet */
 	stats->gorc += E1000_READ_REG(hw, E1000_GORCL);
 	stats->gorc += ((uint64_t)E1000_READ_REG(hw, E1000_GORCH) << 32);
+	stats->gorc -= (stats->gprc - old_gprc) * 4;
 	stats->gotc += E1000_READ_REG(hw, E1000_GOTCL);
 	stats->gotc += ((uint64_t)E1000_READ_REG(hw, E1000_GOTCH) << 32);
+	stats->gotc -= (stats->gptc - old_gptc) * 4;
 
 	stats->rnbc += E1000_READ_REG(hw, E1000_RNBC);
 	stats->ruc += E1000_READ_REG(hw, E1000_RUC);
@@ -1532,13 +1542,16 @@ igb_read_stats_registers(struct e1000_hw *hw, struct e1000_hw_stats *stats)
 	stats->roc += E1000_READ_REG(hw, E1000_ROC);
 	stats->rjc += E1000_READ_REG(hw, E1000_RJC);
 
+	stats->tpr += E1000_READ_REG(hw, E1000_TPR);
+	stats->tpt += E1000_READ_REG(hw, E1000_TPT);
+
 	stats->tor += E1000_READ_REG(hw, E1000_TORL);
 	stats->tor += ((uint64_t)E1000_READ_REG(hw, E1000_TORH) << 32);
+	stats->tor -= (stats->tpr - old_tpr) * 4;
 	stats->tot += E1000_READ_REG(hw, E1000_TOTL);
 	stats->tot += ((uint64_t)E1000_READ_REG(hw, E1000_TOTH) << 32);
+	stats->tot -= (stats->tpt - old_tpt) * 4;
 
-	stats->tpr += E1000_READ_REG(hw, E1000_TPR);
-	stats->tpt += E1000_READ_REG(hw, E1000_TPT);
 	stats->ptc64 += E1000_READ_REG(hw, E1000_PTC64);
 	stats->ptc127 += E1000_READ_REG(hw, E1000_PTC127);
 	stats->ptc255 += E1000_READ_REG(hw, E1000_PTC255);
@@ -1571,8 +1584,10 @@ igb_read_stats_registers(struct e1000_hw *hw, struct e1000_hw_stats *stats)
 	stats->htcbdpc += E1000_READ_REG(hw, E1000_HTCBDPC);
 	stats->hgorc += E1000_READ_REG(hw, E1000_HGORCL);
 	stats->hgorc += ((uint64_t)E1000_READ_REG(hw, E1000_HGORCH) << 32);
+	stats->hgorc -= (stats->rpthc - old_rpthc) * 4;
 	stats->hgotc += E1000_READ_REG(hw, E1000_HGOTCL);
 	stats->hgotc += ((uint64_t)E1000_READ_REG(hw, E1000_HGOTCH) << 32);
+	stats->hgotc -= (stats->hgptc - old_hgptc) * 4;
 	stats->lenerrs += E1000_READ_REG(hw, E1000_LENERRS);
 	stats->scvpc += E1000_READ_REG(hw, E1000_SCVPC);
 	stats->hrmpc += E1000_READ_REG(hw, E1000_HRMPC);
-- 
1.9.1

Re: [dpdk-dev] [PATCH 1/4] e1000: remove crc size from all byte counters

[Stephen Hemminger]

On Mon, 16 Nov 2015 10:35:14 +0000
Harry van Haaren <harry.van.haaren@intel.com> wrote:

> This patch removes the crc bytes from byte counter statistics.
> 
> Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
> ---
>  drivers/net/e1000/igb_ethdev.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/e1000/igb_ethdev.c b/drivers/net/e1000/igb_ethdev.c
> index 88995b0..0ad7341 100644
> --- a/drivers/net/e1000/igb_ethdev.c
> +++ b/drivers/net/e1000/igb_ethdev.c
> @@ -1480,6 +1480,13 @@ igb_read_stats_registers(struct e1000_hw *hw, struct e1000_hw_stats *stats)
>  {
>  	int pause_frames;
>  
> +	uint64_t old_gprc  = stats->gprc;
> +	uint64_t old_gptc  = stats->gptc;
> +	uint64_t old_tpr   = stats->tpr;
> +	uint64_t old_tpt   = stats->tpt;
> +	uint64_t old_rpthc = stats->rpthc;
> +	uint64_t old_hgptc = stats->hgptc;
> +
>  	if(hw->phy.media_type == e1000_media_type_copper ||
>  	    (E1000_READ_REG(hw, E1000_STATUS) & E1000_STATUS_LU)) {
>  		stats->symerrs +=
> @@ -1521,10 +1528,13 @@ igb_read_stats_registers(struct e1000_hw *hw, struct e1000_hw_stats *stats)
>  	/* For the 64-bit byte counters the low dword must be read first. */
>  	/* Both registers clear on the read of the high dword */
>  
> +	/* Workaround CRC bytes included in size, take away 4 bytes/packet */
>  	stats->gorc += E1000_READ_REG(hw, E1000_GORCL);
>  	stats->gorc += ((uint64_t)E1000_READ_REG(hw, E1000_GORCH) << 32);
> +	stats->gorc -= (stats->gprc - old_gprc) * 4;
>  	stats->gotc += E1000_READ_REG(hw, E1000_GOTCL);
>  	stats->gotc += ((uint64_t)E1000_READ_REG(hw, E1000_GOTCH) << 32);
> +	stats->gotc -= (stats->gptc - old_gptc) * 4;
>  
>  	stats->rnbc += E1000_READ_REG(hw, E1000_RNBC);
>  	stats->ruc += E1000_READ_REG(hw, E1000_RUC);
> @@ -1532,13 +1542,16 @@ igb_read_stats_registers(struct e1000_hw *hw, struct e1000_hw_stats *stats)
>  	stats->roc += E1000_READ_REG(hw, E1000_ROC);
>  	stats->rjc += E1000_READ_REG(hw, E1000_RJC);
>  
> +	stats->tpr += E1000_READ_REG(hw, E1000_TPR);
> +	stats->tpt += E1000_READ_REG(hw, E1000_TPT);
> +
>  	stats->tor += E1000_READ_REG(hw, E1000_TORL);
>  	stats->tor += ((uint64_t)E1000_READ_REG(hw, E1000_TORH) << 32);
> +	stats->tor -= (stats->tpr - old_tpr) * 4;

Why not use ETHER_CRC_LEN rather than magic # 4?

Re: [dpdk-dev] [PATCH 1/4] e1000: remove crc size from all byte counters

[Van Haaren, Harry]

> From: Stephen Hemminger [mailto:shemming@brocade.com]
> Harry van Haaren <harry.van.haaren@intel.com> wrote:
> <snip>
> > +	stats->tor -= (stats->tpr - old_tpr) * 4;
> 
> Why not use ETHER_CRC_LEN rather than magic # 4?

That would work too. Will respin tomorrow to fix and to give time
for other comments.

Thanks, -Harry

[dpdk-dev] [PATCH 2/4] ixgbe: remove crc size from all byte counters

[Harry van Haaren]

This patch removes the crc bytes from byte counter statistics.

Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
---
 drivers/net/ixgbe/ixgbe_ethdev.c | 47 ++++++++++++++++++++++++++++++----------
 1 file changed, 35 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
index 80801f0..3ccfd89 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -2331,12 +2331,14 @@ ixgbe_dev_close(struct rte_eth_dev *dev)
 }
 
 static void
-ixgbe_read_stats_registers(struct ixgbe_hw *hw, struct ixgbe_hw_stats
-						   *hw_stats, uint64_t *total_missed_rx,
-						   uint64_t *total_qbrc, uint64_t *total_qprc,
-						   uint64_t *total_qprdc)
+ixgbe_read_stats_registers(struct ixgbe_hw *hw,
+			   struct ixgbe_hw_stats *hw_stats,
+			   uint64_t *total_missed_rx, uint64_t *total_qbrc,
+			   uint64_t *total_qprc, uint64_t *total_qprdc)
 {
 	uint32_t bprc, lxon, lxoff, total;
+	uint32_t delta_gprc = 0;
+	uint32_t delta_gptc = 0;
 	unsigned i;
 
 	hw_stats->crcerrs += IXGBE_READ_REG(hw, IXGBE_CRCERRS);
@@ -2372,26 +2374,41 @@ ixgbe_read_stats_registers(struct ixgbe_hw *hw, struct ixgbe_hw_stats
 		    IXGBE_READ_REG(hw, IXGBE_PXOFFTXC(i));
 	}
 	for (i = 0; i < IXGBE_QUEUE_STAT_COUNTERS; i++) {
-		hw_stats->qprc[i] += IXGBE_READ_REG(hw, IXGBE_QPRC(i));
-		hw_stats->qptc[i] += IXGBE_READ_REG(hw, IXGBE_QPTC(i));
+		uint32_t delta_qprc = IXGBE_READ_REG(hw, IXGBE_QPRC(i));
+		uint32_t delta_qptc = IXGBE_READ_REG(hw, IXGBE_QPTC(i));
+		uint32_t delta_qprdc = IXGBE_READ_REG(hw, IXGBE_QPRDC(i));
+
+		delta_gprc += delta_qprc;
+		delta_gptc += delta_qptc;
+
+		hw_stats->qprc[i] += delta_qprc;
+		hw_stats->qptc[i] += delta_qptc;
+
 		hw_stats->qbrc[i] += IXGBE_READ_REG(hw, IXGBE_QBRC_L(i));
 		hw_stats->qbrc[i] +=
 		    ((uint64_t)IXGBE_READ_REG(hw, IXGBE_QBRC_H(i)) << 32);
+		hw_stats->qbrc[i] -= delta_qprc * 4;
+
 		hw_stats->qbtc[i] += IXGBE_READ_REG(hw, IXGBE_QBTC_L(i));
 		hw_stats->qbtc[i] +=
 		    ((uint64_t)IXGBE_READ_REG(hw, IXGBE_QBTC_H(i)) << 32);
-		*total_qprdc += hw_stats->qprdc[i] +=
-				IXGBE_READ_REG(hw, IXGBE_QPRDC(i));
+
+		hw_stats->qprdc[i] += delta_qprdc;
+		*total_qprdc += hw_stats->qprdc[i];
 
 		*total_qprc += hw_stats->qprc[i];
 		*total_qbrc += hw_stats->qbrc[i];
 	}
+
 	hw_stats->mlfc += IXGBE_READ_REG(hw, IXGBE_MLFC);
 	hw_stats->mrfc += IXGBE_READ_REG(hw, IXGBE_MRFC);
 	hw_stats->rlec += IXGBE_READ_REG(hw, IXGBE_RLEC);
 
-	/* Note that gprc counts missed packets */
-	hw_stats->gprc += IXGBE_READ_REG(hw, IXGBE_GPRC);
+	/*
+	 * An errata states that gprc actually counts good + missed packets:
+	 * Workaround to set gprc to summated queue packet recieves
+	 */
+	hw_stats->gprc = *total_qprc;
 
 	if (hw->mac.type != ixgbe_mac_82598EB) {
 		hw_stats->gorc += IXGBE_READ_REG(hw, IXGBE_GORCL);
@@ -2410,6 +2427,14 @@ ixgbe_read_stats_registers(struct ixgbe_hw *hw, struct ixgbe_hw_stats
 		hw_stats->gotc += IXGBE_READ_REG(hw, IXGBE_GOTCH);
 		hw_stats->tor += IXGBE_READ_REG(hw, IXGBE_TORH);
 	}
+	uint64_t old_tpr = hw_stats->tpr;
+
+	hw_stats->tpr += IXGBE_READ_REG(hw, IXGBE_TPR);
+	hw_stats->tpt += IXGBE_READ_REG(hw, IXGBE_TPT);
+
+	hw_stats->gorc -= delta_gprc * 4;
+	hw_stats->gotc -= delta_gptc * 4;
+	hw_stats->tor -= (hw_stats->tpr - old_tpr) * 4;
 
 	/*
 	 * Workaround: mprc hardware is incorrectly counting
@@ -2449,8 +2474,6 @@ ixgbe_read_stats_registers(struct ixgbe_hw *hw, struct ixgbe_hw_stats
 	hw_stats->mngprc += IXGBE_READ_REG(hw, IXGBE_MNGPRC);
 	hw_stats->mngpdc += IXGBE_READ_REG(hw, IXGBE_MNGPDC);
 	hw_stats->mngptc += IXGBE_READ_REG(hw, IXGBE_MNGPTC);
-	hw_stats->tpr += IXGBE_READ_REG(hw, IXGBE_TPR);
-	hw_stats->tpt += IXGBE_READ_REG(hw, IXGBE_TPT);
 	hw_stats->ptc127 += IXGBE_READ_REG(hw, IXGBE_PTC127);
 	hw_stats->ptc255 += IXGBE_READ_REG(hw, IXGBE_PTC255);
 	hw_stats->ptc511 += IXGBE_READ_REG(hw, IXGBE_PTC511);
-- 
1.9.1

[dpdk-dev] [PATCH 3/4] i40e: fix rx/tx size mismatch, remove crc bytes

[Harry van Haaren]

This patch removes the crc bytes from byte counter statistics.

Doing so fixes a bug that CRC bytes were included on TX but not
on RX, causing mismatch of bytes recieved / sent.

Fixes: 9aace75fc82e ("i40e: fix statistics")

Reported-by: Weichun Chen <weichunx.chen@intel.com>
Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
---
 drivers/net/i40e/i40e_ethdev.c | 33 ++++++++++++++++++++++-----------
 1 file changed, 22 insertions(+), 11 deletions(-)

diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 2c51a0b..c0c268d 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -1870,11 +1870,11 @@ i40e_read_stats_registers(struct i40e_pf *pf, struct i40e_hw *hw)
 	unsigned int i;
 	struct i40e_hw_port_stats *ns = &pf->stats; /* new stats */
 	struct i40e_hw_port_stats *os = &pf->stats_offset; /* old stats */
-	/* Get statistics of struct i40e_eth_stats */
-	i40e_stat_update_48(hw, I40E_GLPRT_GORCH(hw->port),
-			    I40E_GLPRT_GORCL(hw->port),
-			    pf->offset_loaded, &os->eth.rx_bytes,
-			    &ns->eth.rx_bytes);
+
+	/* Workaround: CRC size should not be included in byte statistics,
+	 * so we add 4 bytes to the offset, causing the resulting byte
+	 * counters to be 4 bytes smaller.
+	 */
 	i40e_stat_update_48(hw, I40E_GLPRT_UPRCH(hw->port),
 			    I40E_GLPRT_UPRCL(hw->port),
 			    pf->offset_loaded, &os->eth.rx_unicast,
@@ -1887,6 +1887,15 @@ i40e_read_stats_registers(struct i40e_pf *pf, struct i40e_hw *hw)
 			    I40E_GLPRT_BPRCL(hw->port),
 			    pf->offset_loaded, &os->eth.rx_broadcast,
 			    &ns->eth.rx_broadcast);
+
+	/* Get statistics of struct i40e_eth_stats */
+	i40e_stat_update_48(hw, I40E_GLPRT_GORCH(hw->port),
+			    I40E_GLPRT_GORCL(hw->port),
+			    pf->offset_loaded, &os->eth.rx_bytes,
+			    &ns->eth.rx_bytes);
+	ns->eth.rx_bytes -= (ns->eth.rx_unicast + ns->eth.rx_multicast +
+		ns->eth.rx_broadcast) * 4;
+
 	i40e_stat_update_32(hw, I40E_GLPRT_RDPC(hw->port),
 			    pf->offset_loaded, &os->eth.rx_discards,
 			    &ns->eth.rx_discards);
@@ -1896,10 +1905,6 @@ i40e_read_stats_registers(struct i40e_pf *pf, struct i40e_hw *hw)
 			    pf->offset_loaded,
 			    &os->eth.rx_unknown_protocol,
 			    &ns->eth.rx_unknown_protocol);
-	i40e_stat_update_48(hw, I40E_GLPRT_GOTCH(hw->port),
-			    I40E_GLPRT_GOTCL(hw->port),
-			    pf->offset_loaded, &os->eth.tx_bytes,
-			    &ns->eth.tx_bytes);
 	i40e_stat_update_48(hw, I40E_GLPRT_UPTCH(hw->port),
 			    I40E_GLPRT_UPTCL(hw->port),
 			    pf->offset_loaded, &os->eth.tx_unicast,
@@ -1912,6 +1917,12 @@ i40e_read_stats_registers(struct i40e_pf *pf, struct i40e_hw *hw)
 			    I40E_GLPRT_BPTCL(hw->port),
 			    pf->offset_loaded, &os->eth.tx_broadcast,
 			    &ns->eth.tx_broadcast);
+	i40e_stat_update_48(hw, I40E_GLPRT_GOTCH(hw->port),
+			    I40E_GLPRT_GOTCL(hw->port),
+			    pf->offset_loaded, &os->eth.tx_bytes,
+			    &ns->eth.tx_bytes);
+	ns->eth.tx_bytes -= (ns->eth.tx_unicast + ns->eth.tx_multicast +
+		ns->eth.tx_broadcast) * 4;
 	/* GLPRT_TEPC not supported */
 
 	/* additional port specific stats */
@@ -2069,8 +2080,8 @@ i40e_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
 	stats->opackets = pf->main_vsi->eth_stats.tx_unicast +
 			pf->main_vsi->eth_stats.tx_multicast +
 			pf->main_vsi->eth_stats.tx_broadcast;
-	stats->ibytes   = pf->main_vsi->eth_stats.rx_bytes;
-	stats->obytes   = pf->main_vsi->eth_stats.tx_bytes;
+	stats->ibytes   = ns->eth.rx_bytes;
+	stats->obytes   = ns->eth.tx_bytes;
 	stats->oerrors  = ns->eth.tx_errors +
 			pf->main_vsi->eth_stats.tx_errors;
 	stats->imcasts  = pf->main_vsi->eth_stats.rx_multicast;
-- 
1.9.1

[dpdk-dev] [PATCH 4/4] fm10k: remove crc size from all byte counters

[Harry van Haaren]

This patch removes the crc bytes from byte counter statistics.

Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
---
 drivers/net/fm10k/fm10k_ethdev.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/fm10k/fm10k_ethdev.c b/drivers/net/fm10k/fm10k_ethdev.c
index 441f713..fdb2e81 100644
--- a/drivers/net/fm10k/fm10k_ethdev.c
+++ b/drivers/net/fm10k/fm10k_ethdev.c
@@ -1183,11 +1183,13 @@ fm10k_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
 
 	ipackets = opackets = ibytes = obytes = 0;
 	for (i = 0; (i < RTE_ETHDEV_QUEUE_STAT_CNTRS) &&
-		(i < hw->mac.max_queues); ++i) {
+			(i < hw->mac.max_queues); ++i) {
 		stats->q_ipackets[i] = hw_stats->q[i].rx_packets.count;
 		stats->q_opackets[i] = hw_stats->q[i].tx_packets.count;
-		stats->q_ibytes[i]   = hw_stats->q[i].rx_bytes.count;
-		stats->q_obytes[i]   = hw_stats->q[i].tx_bytes.count;
+		stats->q_ibytes[i]   = hw_stats->q[i].rx_bytes.count -
+			(stats->q_ipackets[i] * 4);
+		stats->q_obytes[i]   = hw_stats->q[i].tx_bytes.count -
+			(stats->q_opackets[i] * 4);
 		ipackets += stats->q_ipackets[i];
 		opackets += stats->q_opackets[i];
 		ibytes   += stats->q_ibytes[i];
-- 
1.9.1

Re: [dpdk-dev] [PATCH 4/4] fm10k: remove crc size from all byte counters

[Qiu, Michael]

Hi, Harry

Have you ever tested this patch by yourself?

fm10k's stats should already remove the crc bytes by default.

After your patch applied, if send a packet without vlan(64 bytes),
we expect receive 60 bytes, but it will disappoint you, that only
56 bytes shows in system.

Thanks,
Michael

On 2015/11/16 18:36, Harry van Haaren wrote:
> This patch removes the crc bytes from byte counter statistics.
>
> Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
> ---
>  drivers/net/fm10k/fm10k_ethdev.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/fm10k/fm10k_ethdev.c b/drivers/net/fm10k/fm10k_ethdev.c
> index 441f713..fdb2e81 100644
> --- a/drivers/net/fm10k/fm10k_ethdev.c
> +++ b/drivers/net/fm10k/fm10k_ethdev.c
> @@ -1183,11 +1183,13 @@ fm10k_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
>  
>  	ipackets = opackets = ibytes = obytes = 0;
>  	for (i = 0; (i < RTE_ETHDEV_QUEUE_STAT_CNTRS) &&
> -		(i < hw->mac.max_queues); ++i) {
> +			(i < hw->mac.max_queues); ++i) {
>  		stats->q_ipackets[i] = hw_stats->q[i].rx_packets.count;
>  		stats->q_opackets[i] = hw_stats->q[i].tx_packets.count;
> -		stats->q_ibytes[i]   = hw_stats->q[i].rx_bytes.count;
> -		stats->q_obytes[i]   = hw_stats->q[i].tx_bytes.count;
> +		stats->q_ibytes[i]   = hw_stats->q[i].rx_bytes.count -
> +			(stats->q_ipackets[i] * 4);
> +		stats->q_obytes[i]   = hw_stats->q[i].tx_bytes.count -
> +			(stats->q_opackets[i] * 4);
>  		ipackets += stats->q_ipackets[i];
>  		opackets += stats->q_opackets[i];
>  		ibytes   += stats->q_ibytes[i];

[dpdk-dev] [PATCH v2 0/3] Remove CRC from byte counters

[Harry van Haaren]

This patchset removes CRC bytes from byte statistics in the following
PMDs: igb, ixgbe, i40e.

This brings the reported byte statistics in-line with other NICs,
providing consistency.

Removing the CRC from byte counters in i40e resolves a bug, see
i40e commit message for details.

v2:
-Done Stephen Hemmingers suggestion of ETHER_CRC_LEN replacing magic #4
-Removed fm10k patch, thanks Michael Qiu for reporting - my testpmd usage
 for packet sizes on fm10k was creating an offset of 4 bytes in testing.
-Reworded some comments


Harry van Haaren (3):
  e1000: remove crc size from all byte counters
  ixgbe: remove crc size from all byte counters
  i40e: fix rx/tx size mismatch, remove crc bytes

 drivers/net/e1000/igb_ethdev.c   | 19 ++++++++++++--
 drivers/net/i40e/i40e_ethdev.c   | 13 ++++++++--
 drivers/net/ixgbe/ixgbe_ethdev.c | 55 +++++++++++++++++++++++++++++++---------
 3 files changed, 71 insertions(+), 16 deletions(-)

-- 
1.9.1

[dpdk-dev] [PATCH v2 1/3] e1000: remove crc size from all byte counters

[Harry van Haaren]

This patch removes the crc bytes from byte counter statistics.

Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
---
 drivers/net/e1000/igb_ethdev.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/net/e1000/igb_ethdev.c b/drivers/net/e1000/igb_ethdev.c
index 88995b0..bf4c2a5 100644
--- a/drivers/net/e1000/igb_ethdev.c
+++ b/drivers/net/e1000/igb_ethdev.c
@@ -1480,6 +1480,13 @@ igb_read_stats_registers(struct e1000_hw *hw, struct e1000_hw_stats *stats)
 {
 	int pause_frames;
 
+	uint64_t old_gprc  = stats->gprc;
+	uint64_t old_gptc  = stats->gptc;
+	uint64_t old_tpr   = stats->tpr;
+	uint64_t old_tpt   = stats->tpt;
+	uint64_t old_rpthc = stats->rpthc;
+	uint64_t old_hgptc = stats->hgptc;
+
 	if(hw->phy.media_type == e1000_media_type_copper ||
 	    (E1000_READ_REG(hw, E1000_STATUS) & E1000_STATUS_LU)) {
 		stats->symerrs +=
@@ -1521,10 +1528,13 @@ igb_read_stats_registers(struct e1000_hw *hw, struct e1000_hw_stats *stats)
 	/* For the 64-bit byte counters the low dword must be read first. */
 	/* Both registers clear on the read of the high dword */
 
+	/* Workaround CRC bytes included in size, take away 4 bytes/packet */
 	stats->gorc += E1000_READ_REG(hw, E1000_GORCL);
 	stats->gorc += ((uint64_t)E1000_READ_REG(hw, E1000_GORCH) << 32);
+	stats->gorc -= (stats->gprc - old_gprc) * ETHER_CRC_LEN;
 	stats->gotc += E1000_READ_REG(hw, E1000_GOTCL);
 	stats->gotc += ((uint64_t)E1000_READ_REG(hw, E1000_GOTCH) << 32);
+	stats->gotc -= (stats->gptc - old_gptc) * ETHER_CRC_LEN;
 
 	stats->rnbc += E1000_READ_REG(hw, E1000_RNBC);
 	stats->ruc += E1000_READ_REG(hw, E1000_RUC);
@@ -1532,13 +1542,16 @@ igb_read_stats_registers(struct e1000_hw *hw, struct e1000_hw_stats *stats)
 	stats->roc += E1000_READ_REG(hw, E1000_ROC);
 	stats->rjc += E1000_READ_REG(hw, E1000_RJC);
 
+	stats->tpr += E1000_READ_REG(hw, E1000_TPR);
+	stats->tpt += E1000_READ_REG(hw, E1000_TPT);
+
 	stats->tor += E1000_READ_REG(hw, E1000_TORL);
 	stats->tor += ((uint64_t)E1000_READ_REG(hw, E1000_TORH) << 32);
+	stats->tor -= (stats->tpr - old_tpr) * ETHER_CRC_LEN;
 	stats->tot += E1000_READ_REG(hw, E1000_TOTL);
 	stats->tot += ((uint64_t)E1000_READ_REG(hw, E1000_TOTH) << 32);
+	stats->tot -= (stats->tpt - old_tpt) * ETHER_CRC_LEN;
 
-	stats->tpr += E1000_READ_REG(hw, E1000_TPR);
-	stats->tpt += E1000_READ_REG(hw, E1000_TPT);
 	stats->ptc64 += E1000_READ_REG(hw, E1000_PTC64);
 	stats->ptc127 += E1000_READ_REG(hw, E1000_PTC127);
 	stats->ptc255 += E1000_READ_REG(hw, E1000_PTC255);
@@ -1571,8 +1584,10 @@ igb_read_stats_registers(struct e1000_hw *hw, struct e1000_hw_stats *stats)
 	stats->htcbdpc += E1000_READ_REG(hw, E1000_HTCBDPC);
 	stats->hgorc += E1000_READ_REG(hw, E1000_HGORCL);
 	stats->hgorc += ((uint64_t)E1000_READ_REG(hw, E1000_HGORCH) << 32);
+	stats->hgorc -= (stats->rpthc - old_rpthc) * ETHER_CRC_LEN;
 	stats->hgotc += E1000_READ_REG(hw, E1000_HGOTCL);
 	stats->hgotc += ((uint64_t)E1000_READ_REG(hw, E1000_HGOTCH) << 32);
+	stats->hgotc -= (stats->hgptc - old_hgptc) * ETHER_CRC_LEN;
 	stats->lenerrs += E1000_READ_REG(hw, E1000_LENERRS);
 	stats->scvpc += E1000_READ_REG(hw, E1000_SCVPC);
 	stats->hrmpc += E1000_READ_REG(hw, E1000_HRMPC);
-- 
1.9.1

[dpdk-dev] [PATCH v2 2/3] ixgbe: remove crc size from all byte counters

[Harry van Haaren]

This patch removes the crc bytes from byte counter statistics.

Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
---
 drivers/net/ixgbe/ixgbe_ethdev.c | 55 +++++++++++++++++++++++++++++++---------
 1 file changed, 43 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
index 80801f0..0124b1c 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -2331,13 +2331,21 @@ ixgbe_dev_close(struct rte_eth_dev *dev)
 }
 
 static void
-ixgbe_read_stats_registers(struct ixgbe_hw *hw, struct ixgbe_hw_stats
-						   *hw_stats, uint64_t *total_missed_rx,
-						   uint64_t *total_qbrc, uint64_t *total_qprc,
-						   uint64_t *total_qprdc)
+ixgbe_read_stats_registers(struct ixgbe_hw *hw,
+			   struct ixgbe_hw_stats *hw_stats,
+			   uint64_t *total_missed_rx, uint64_t *total_qbrc,
+			   uint64_t *total_qprc, uint64_t *total_qprdc)
 {
 	uint32_t bprc, lxon, lxoff, total;
+	uint32_t delta_gprc = 0;
+	uint32_t delta_gptc = 0;
 	unsigned i;
+	/* Workaround for RX byte count not including CRC bytes when CRC
++	 * strip is enabled. CRC bytes are removed from counters when crc_strip
+	 * is disabled.
++	 */
+	int crc_strip = (IXGBE_READ_REG(hw, IXGBE_HLREG0) &
+			IXGBE_HLREG0_RXCRCSTRP);
 
 	hw_stats->crcerrs += IXGBE_READ_REG(hw, IXGBE_CRCERRS);
 	hw_stats->illerrc += IXGBE_READ_REG(hw, IXGBE_ILLERRC);
@@ -2372,16 +2380,28 @@ ixgbe_read_stats_registers(struct ixgbe_hw *hw, struct ixgbe_hw_stats
 		    IXGBE_READ_REG(hw, IXGBE_PXOFFTXC(i));
 	}
 	for (i = 0; i < IXGBE_QUEUE_STAT_COUNTERS; i++) {
-		hw_stats->qprc[i] += IXGBE_READ_REG(hw, IXGBE_QPRC(i));
-		hw_stats->qptc[i] += IXGBE_READ_REG(hw, IXGBE_QPTC(i));
+		uint32_t delta_qprc = IXGBE_READ_REG(hw, IXGBE_QPRC(i));
+		uint32_t delta_qptc = IXGBE_READ_REG(hw, IXGBE_QPTC(i));
+		uint32_t delta_qprdc = IXGBE_READ_REG(hw, IXGBE_QPRDC(i));
+
+		delta_gprc += delta_qprc;
+		delta_gptc += delta_qptc;
+
+		hw_stats->qprc[i] += delta_qprc;
+		hw_stats->qptc[i] += delta_qptc;
+
 		hw_stats->qbrc[i] += IXGBE_READ_REG(hw, IXGBE_QBRC_L(i));
 		hw_stats->qbrc[i] +=
 		    ((uint64_t)IXGBE_READ_REG(hw, IXGBE_QBRC_H(i)) << 32);
+		if (crc_strip == 0)
+			hw_stats->qbrc[i] -= delta_qprc * ETHER_CRC_LEN;
+
 		hw_stats->qbtc[i] += IXGBE_READ_REG(hw, IXGBE_QBTC_L(i));
 		hw_stats->qbtc[i] +=
 		    ((uint64_t)IXGBE_READ_REG(hw, IXGBE_QBTC_H(i)) << 32);
-		*total_qprdc += hw_stats->qprdc[i] +=
-				IXGBE_READ_REG(hw, IXGBE_QPRDC(i));
+
+		hw_stats->qprdc[i] += delta_qprdc;
+		*total_qprdc += hw_stats->qprdc[i];
 
 		*total_qprc += hw_stats->qprc[i];
 		*total_qbrc += hw_stats->qbrc[i];
@@ -2390,8 +2410,11 @@ ixgbe_read_stats_registers(struct ixgbe_hw *hw, struct ixgbe_hw_stats
 	hw_stats->mrfc += IXGBE_READ_REG(hw, IXGBE_MRFC);
 	hw_stats->rlec += IXGBE_READ_REG(hw, IXGBE_RLEC);
 
-	/* Note that gprc counts missed packets */
-	hw_stats->gprc += IXGBE_READ_REG(hw, IXGBE_GPRC);
+	/*
+	 * An errata states that gprc actually counts good + missed packets:
+	 * Workaround to set gprc to summated queue packet recieves
+	 */
+	hw_stats->gprc = *total_qprc;
 
 	if (hw->mac.type != ixgbe_mac_82598EB) {
 		hw_stats->gorc += IXGBE_READ_REG(hw, IXGBE_GORCL);
@@ -2410,6 +2433,16 @@ ixgbe_read_stats_registers(struct ixgbe_hw *hw, struct ixgbe_hw_stats
 		hw_stats->gotc += IXGBE_READ_REG(hw, IXGBE_GOTCH);
 		hw_stats->tor += IXGBE_READ_REG(hw, IXGBE_TORH);
 	}
+	uint64_t old_tpr = hw_stats->tpr;
+
+	hw_stats->tpr += IXGBE_READ_REG(hw, IXGBE_TPR);
+	hw_stats->tpt += IXGBE_READ_REG(hw, IXGBE_TPT);
+
+	if (crc_strip == 0)
+		hw_stats->gorc -= delta_gprc * ETHER_CRC_LEN;
+
+	hw_stats->gotc -= delta_gptc * ETHER_CRC_LEN;
+	hw_stats->tor -= (hw_stats->tpr - old_tpr) * ETHER_CRC_LEN;
 
 	/*
 	 * Workaround: mprc hardware is incorrectly counting
@@ -2449,8 +2482,6 @@ ixgbe_read_stats_registers(struct ixgbe_hw *hw, struct ixgbe_hw_stats
 	hw_stats->mngprc += IXGBE_READ_REG(hw, IXGBE_MNGPRC);
 	hw_stats->mngpdc += IXGBE_READ_REG(hw, IXGBE_MNGPDC);
 	hw_stats->mngptc += IXGBE_READ_REG(hw, IXGBE_MNGPTC);
-	hw_stats->tpr += IXGBE_READ_REG(hw, IXGBE_TPR);
-	hw_stats->tpt += IXGBE_READ_REG(hw, IXGBE_TPT);
 	hw_stats->ptc127 += IXGBE_READ_REG(hw, IXGBE_PTC127);
 	hw_stats->ptc255 += IXGBE_READ_REG(hw, IXGBE_PTC255);
 	hw_stats->ptc511 += IXGBE_READ_REG(hw, IXGBE_PTC511);
-- 
1.9.1

[dpdk-dev] [PATCH v2 3/3] i40e: fix rx/tx size mismatch, remove crc bytes

[Harry van Haaren]

This patch removes the crc bytes from byte counter statistics.

Doing so fixes a bug that CRC bytes were included on TX but not
on RX, causing mismatch of bytes recieved / sent.

Fixes: 9aace75fc82e ("i40e: fix statistics")

Reported-by: Weichun Chen <weichunx.chen@intel.com>
Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
---
 drivers/net/i40e/i40e_ethdev.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 2c51a0b..6cf99dc 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -1870,6 +1870,7 @@ i40e_read_stats_registers(struct i40e_pf *pf, struct i40e_hw *hw)
 	unsigned int i;
 	struct i40e_hw_port_stats *ns = &pf->stats; /* new stats */
 	struct i40e_hw_port_stats *os = &pf->stats_offset; /* old stats */
+
 	/* Get statistics of struct i40e_eth_stats */
 	i40e_stat_update_48(hw, I40E_GLPRT_GORCH(hw->port),
 			    I40E_GLPRT_GORCL(hw->port),
@@ -1887,6 +1888,12 @@ i40e_read_stats_registers(struct i40e_pf *pf, struct i40e_hw *hw)
 			    I40E_GLPRT_BPRCL(hw->port),
 			    pf->offset_loaded, &os->eth.rx_broadcast,
 			    &ns->eth.rx_broadcast);
+	/* Workaround: CRC size should not be included in byte statistics,
+	 * so subtract ETHER_CRC_LEN from the byte counter for each rx packet.
+	 */
+	ns->eth.rx_bytes -= (ns->eth.rx_unicast + ns->eth.rx_multicast +
+		ns->eth.rx_broadcast) * ETHER_CRC_LEN;
+
 	i40e_stat_update_32(hw, I40E_GLPRT_RDPC(hw->port),
 			    pf->offset_loaded, &os->eth.rx_discards,
 			    &ns->eth.rx_discards);
@@ -1912,6 +1919,8 @@ i40e_read_stats_registers(struct i40e_pf *pf, struct i40e_hw *hw)
 			    I40E_GLPRT_BPTCL(hw->port),
 			    pf->offset_loaded, &os->eth.tx_broadcast,
 			    &ns->eth.tx_broadcast);
+	ns->eth.tx_bytes -= (ns->eth.tx_unicast + ns->eth.tx_multicast +
+		ns->eth.tx_broadcast) * ETHER_CRC_LEN;
 	/* GLPRT_TEPC not supported */
 
 	/* additional port specific stats */
@@ -2069,8 +2078,8 @@ i40e_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
 	stats->opackets = pf->main_vsi->eth_stats.tx_unicast +
 			pf->main_vsi->eth_stats.tx_multicast +
 			pf->main_vsi->eth_stats.tx_broadcast;
-	stats->ibytes   = pf->main_vsi->eth_stats.rx_bytes;
-	stats->obytes   = pf->main_vsi->eth_stats.tx_bytes;
+	stats->ibytes   = ns->eth.rx_bytes;
+	stats->obytes   = ns->eth.tx_bytes;
 	stats->oerrors  = ns->eth.tx_errors +
 			pf->main_vsi->eth_stats.tx_errors;
 	stats->imcasts  = pf->main_vsi->eth_stats.rx_multicast;
-- 
1.9.1

Re: [dpdk-dev] [PATCH v2 0/3] Remove CRC from byte counters

[Ananyev, Konstantin]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Harry van Haaren
> Sent: Wednesday, November 18, 2015 10:48 AM
> To: dev@dpdk.org
> Cc: shemming@brocade.com
> Subject: [dpdk-dev] [PATCH v2 0/3] Remove CRC from byte counters
> 
> This patchset removes CRC bytes from byte statistics in the following
> PMDs: igb, ixgbe, i40e.
> 
> This brings the reported byte statistics in-line with other NICs,
> providing consistency.
> 
> Removing the CRC from byte counters in i40e resolves a bug, see
> i40e commit message for details.
> 
> v2:
> -Done Stephen Hemmingers suggestion of ETHER_CRC_LEN replacing magic #4
> -Removed fm10k patch, thanks Michael Qiu for reporting - my testpmd usage
>  for packet sizes on fm10k was creating an offset of 4 bytes in testing.
> -Reworded some comments
> 
> 
> Harry van Haaren (3):
>   e1000: remove crc size from all byte counters
>   ixgbe: remove crc size from all byte counters
>   i40e: fix rx/tx size mismatch, remove crc bytes
> 
>  drivers/net/e1000/igb_ethdev.c   | 19 ++++++++++++--
>  drivers/net/i40e/i40e_ethdev.c   | 13 ++++++++--
>  drivers/net/ixgbe/ixgbe_ethdev.c | 55 +++++++++++++++++++++++++++++++---------
>  3 files changed, 71 insertions(+), 16 deletions(-)
> 

Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

> --
> 1.9.1

Re: [dpdk-dev] [PATCH v2 0/3] Remove CRC from byte counters

[Thomas Monjalon]

> > Harry van Haaren (3):
> >   e1000: remove crc size from all byte counters
> >   ixgbe: remove crc size from all byte counters
> >   i40e: fix rx/tx size mismatch, remove crc bytes
> 
> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

Applied, thanks