DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] net/i40e: fix incorrect byte counters
@ 2020-09-10  1:54 Junyu Jiang
  2020-09-10  2:18 ` [dpdk-dev] [dpdk-stable] " Han, YingyaX
                   ` (4 more replies)
  0 siblings, 5 replies; 20+ messages in thread
From: Junyu Jiang @ 2020-09-10  1:54 UTC (permalink / raw)
  To: dev; +Cc: Jeff Guo, Beilei Xing, Junyu Jiang, stable

This patch fixed the issue that rx/tx bytes overflowed
on 48 bit limitation by enlarging the limitation.

Fixes: 4861cde46116 ("i40e: new poll mode driver")
Cc: stable@dpdk.org

Signed-off-by: Junyu Jiang <junyux.jiang@intel.com>
---
 drivers/net/i40e/i40e_ethdev.c | 47 ++++++++++++++++++++++++++++++++++
 drivers/net/i40e/i40e_ethdev.h |  9 +++++++
 2 files changed, 56 insertions(+)

diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 11c02b188..e3d4b7f4f 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -3070,6 +3070,13 @@ i40e_update_vsi_stats(struct i40e_vsi *vsi)
 	i40e_stat_update_48(hw, I40E_GLV_BPRCH(idx), I40E_GLV_BPRCL(idx),
 			    vsi->offset_loaded, &oes->rx_broadcast,
 			    &nes->rx_broadcast);
+	/* enlarge the limitation when rx_bytes overflowed */
+	if (vsi->offset_loaded) {
+		if (I40E_RXTX_BYTES_LOW(vsi->old_rx_bytes) > nes->rx_bytes)
+			nes->rx_bytes += (uint64_t)1 << I40E_48_BIT_WIDTH;
+		nes->rx_bytes += I40E_RXTX_BYTES_HIGH(vsi->old_rx_bytes);
+	}
+	vsi->old_rx_bytes = nes->rx_bytes;
 	/* exclude CRC bytes */
 	nes->rx_bytes -= (nes->rx_unicast + nes->rx_multicast +
 		nes->rx_broadcast) * RTE_ETHER_CRC_LEN;
@@ -3096,6 +3103,13 @@ i40e_update_vsi_stats(struct i40e_vsi *vsi)
 	/* GLV_TDPC not supported */
 	i40e_stat_update_32(hw, I40E_GLV_TEPC(idx), vsi->offset_loaded,
 			    &oes->tx_errors, &nes->tx_errors);
+	/* enlarge the limitation when tx_bytes overflowed */
+	if (vsi->offset_loaded) {
+		if (I40E_RXTX_BYTES_LOW(vsi->old_tx_bytes) > nes->tx_bytes)
+			nes->tx_bytes += (uint64_t)1 << I40E_48_BIT_WIDTH;
+		nes->tx_bytes += I40E_RXTX_BYTES_HIGH(vsi->old_tx_bytes);
+	}
+	vsi->old_rx_bytes = nes->rx_bytes;
 	vsi->offset_loaded = true;
 
 	PMD_DRV_LOG(DEBUG, "***************** VSI[%u] stats start *******************",
@@ -3168,6 +3182,24 @@ i40e_read_stats_registers(struct i40e_pf *pf, struct i40e_hw *hw)
 			    pf->offset_loaded,
 			    &pf->internal_stats_offset.tx_broadcast,
 			    &pf->internal_stats.tx_broadcast);
+	/* enlarge the limitation when internal rx/tx bytes overflowed */
+	if (pf->offset_loaded) {
+		if (I40E_RXTX_BYTES_LOW(pf->internal_old_rx_bytes) >
+		    pf->internal_stats.rx_bytes)
+			pf->internal_stats.rx_bytes +=
+				(uint64_t)1 << I40E_48_BIT_WIDTH;
+		pf->internal_stats.rx_bytes +=
+			I40E_RXTX_BYTES_HIGH(pf->internal_old_rx_bytes);
+
+		if (I40E_RXTX_BYTES_LOW(pf->internal_old_tx_bytes) >
+		    pf->internal_stats.tx_bytes)
+			pf->internal_stats.tx_bytes +=
+				(uint64_t)1 << I40E_48_BIT_WIDTH;
+		pf->internal_stats.tx_bytes +=
+			I40E_RXTX_BYTES_HIGH(pf->internal_old_tx_bytes);
+	}
+	pf->internal_old_rx_bytes = pf->internal_stats.rx_bytes;
+	pf->internal_old_tx_bytes = pf->internal_stats.tx_bytes;
 
 	/* exclude CRC size */
 	pf->internal_stats.rx_bytes -= (pf->internal_stats.rx_unicast +
@@ -3191,6 +3223,14 @@ 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);
+	/* enlarge the limitation when rx_bytes overflowed */
+	if (pf->offset_loaded) {
+		if (I40E_RXTX_BYTES_LOW(pf->old_rx_bytes) > ns->eth.rx_bytes)
+			ns->eth.rx_bytes += (uint64_t)1 << I40E_48_BIT_WIDTH;
+		ns->eth.rx_bytes += I40E_RXTX_BYTES_HIGH(pf->old_rx_bytes);
+	}
+	pf->old_rx_bytes = ns->eth.rx_bytes;
+
 	/* Workaround: CRC size should not be included in byte statistics,
 	 * so subtract RTE_ETHER_CRC_LEN from the byte counter for each rx
 	 * packet.
@@ -3249,6 +3289,13 @@ 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);
+	/* enlarge the limitation when tx_bytes overflowed */
+	if (pf->offset_loaded) {
+		if (I40E_RXTX_BYTES_LOW(pf->old_tx_bytes) > ns->eth.tx_bytes)
+			ns->eth.tx_bytes += (uint64_t)1 << I40E_48_BIT_WIDTH;
+		ns->eth.tx_bytes += I40E_RXTX_BYTES_HIGH(pf->old_tx_bytes);
+	}
+	pf->old_tx_bytes = ns->eth.tx_bytes;
 	ns->eth.tx_bytes -= (ns->eth.tx_unicast + ns->eth.tx_multicast +
 		ns->eth.tx_broadcast) * RTE_ETHER_CRC_LEN;
 
diff --git a/drivers/net/i40e/i40e_ethdev.h b/drivers/net/i40e/i40e_ethdev.h
index 19f821829..5d17be1f0 100644
--- a/drivers/net/i40e/i40e_ethdev.h
+++ b/drivers/net/i40e/i40e_ethdev.h
@@ -282,6 +282,9 @@ struct rte_flow {
 #define I40E_ETH_OVERHEAD \
 	(RTE_ETHER_HDR_LEN + RTE_ETHER_CRC_LEN + I40E_VLAN_TAG_SIZE * 2)
 
+#define I40E_RXTX_BYTES_HIGH(bytes) ((bytes) & ~I40E_48_BIT_MASK)
+#define I40E_RXTX_BYTES_LOW(bytes) ((bytes) & I40E_48_BIT_MASK)
+
 struct i40e_adapter;
 struct rte_pci_driver;
 
@@ -399,6 +402,8 @@ struct i40e_vsi {
 	uint8_t vlan_anti_spoof_on; /* The VLAN anti-spoofing enabled */
 	uint8_t vlan_filter_on; /* The VLAN filter enabled */
 	struct i40e_bw_info bw_info; /* VSI bandwidth information */
+	uint64_t old_rx_bytes;
+	uint64_t old_tx_bytes;
 };
 
 struct pool_entry {
@@ -1156,6 +1161,10 @@ struct i40e_pf {
 	uint16_t switch_domain_id;
 
 	struct i40e_vf_msg_cfg vf_msg_cfg;
+	uint64_t old_rx_bytes;
+	uint64_t old_tx_bytes;
+	uint64_t internal_old_rx_bytes;
+	uint64_t internal_old_tx_bytes;
 };
 
 enum pending_msg {
-- 
2.17.1


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

* Re: [dpdk-dev] [dpdk-stable] [PATCH] net/i40e: fix incorrect byte counters
  2020-09-10  1:54 [dpdk-dev] [PATCH] net/i40e: fix incorrect byte counters Junyu Jiang
@ 2020-09-10  2:18 ` Han, YingyaX
  2020-09-10  5:58   ` Guo, Jia
  2020-09-16  1:51 ` [dpdk-dev] [PATCH v2] " Junyu Jiang
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 20+ messages in thread
From: Han, YingyaX @ 2020-09-10  2:18 UTC (permalink / raw)
  To: Jiang, JunyuX, dev; +Cc: Guo, Jia, Xing, Beilei, Jiang, JunyuX, stable

Tested-by: Yingya Han <yingyax.han@intel.com>

-----Original Message-----
From: stable <stable-bounces@dpdk.org> On Behalf Of Junyu Jiang
Sent: Thursday, September 10, 2020 9:54 AM
To: dev@dpdk.org
Cc: Guo, Jia <jia.guo@intel.com>; Xing, Beilei <beilei.xing@intel.com>; Jiang, JunyuX <junyux.jiang@intel.com>; stable@dpdk.org
Subject: [dpdk-stable] [PATCH] net/i40e: fix incorrect byte counters

This patch fixed the issue that rx/tx bytes overflowed on 48 bit limitation by enlarging the limitation.

Fixes: 4861cde46116 ("i40e: new poll mode driver")
Cc: stable@dpdk.org

Signed-off-by: Junyu Jiang <junyux.jiang@intel.com>
---
 drivers/net/i40e/i40e_ethdev.c | 47 ++++++++++++++++++++++++++++++++++
 drivers/net/i40e/i40e_ethdev.h |  9 +++++++
 2 files changed, 56 insertions(+)

diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c index 11c02b188..e3d4b7f4f 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -3070,6 +3070,13 @@ i40e_update_vsi_stats(struct i40e_vsi *vsi)
 	i40e_stat_update_48(hw, I40E_GLV_BPRCH(idx), I40E_GLV_BPRCL(idx),
 			    vsi->offset_loaded, &oes->rx_broadcast,
 			    &nes->rx_broadcast);
+	/* enlarge the limitation when rx_bytes overflowed */
+	if (vsi->offset_loaded) {
+		if (I40E_RXTX_BYTES_LOW(vsi->old_rx_bytes) > nes->rx_bytes)
+			nes->rx_bytes += (uint64_t)1 << I40E_48_BIT_WIDTH;
+		nes->rx_bytes += I40E_RXTX_BYTES_HIGH(vsi->old_rx_bytes);
+	}
+	vsi->old_rx_bytes = nes->rx_bytes;
 	/* exclude CRC bytes */
 	nes->rx_bytes -= (nes->rx_unicast + nes->rx_multicast +
 		nes->rx_broadcast) * RTE_ETHER_CRC_LEN; @@ -3096,6 +3103,13 @@ i40e_update_vsi_stats(struct i40e_vsi *vsi)
 	/* GLV_TDPC not supported */
 	i40e_stat_update_32(hw, I40E_GLV_TEPC(idx), vsi->offset_loaded,
 			    &oes->tx_errors, &nes->tx_errors);
+	/* enlarge the limitation when tx_bytes overflowed */
+	if (vsi->offset_loaded) {
+		if (I40E_RXTX_BYTES_LOW(vsi->old_tx_bytes) > nes->tx_bytes)
+			nes->tx_bytes += (uint64_t)1 << I40E_48_BIT_WIDTH;
+		nes->tx_bytes += I40E_RXTX_BYTES_HIGH(vsi->old_tx_bytes);
+	}
+	vsi->old_rx_bytes = nes->rx_bytes;
 	vsi->offset_loaded = true;
 
 	PMD_DRV_LOG(DEBUG, "***************** VSI[%u] stats start *******************", @@ -3168,6 +3182,24 @@ i40e_read_stats_registers(struct i40e_pf *pf, struct i40e_hw *hw)
 			    pf->offset_loaded,
 			    &pf->internal_stats_offset.tx_broadcast,
 			    &pf->internal_stats.tx_broadcast);
+	/* enlarge the limitation when internal rx/tx bytes overflowed */
+	if (pf->offset_loaded) {
+		if (I40E_RXTX_BYTES_LOW(pf->internal_old_rx_bytes) >
+		    pf->internal_stats.rx_bytes)
+			pf->internal_stats.rx_bytes +=
+				(uint64_t)1 << I40E_48_BIT_WIDTH;
+		pf->internal_stats.rx_bytes +=
+			I40E_RXTX_BYTES_HIGH(pf->internal_old_rx_bytes);
+
+		if (I40E_RXTX_BYTES_LOW(pf->internal_old_tx_bytes) >
+		    pf->internal_stats.tx_bytes)
+			pf->internal_stats.tx_bytes +=
+				(uint64_t)1 << I40E_48_BIT_WIDTH;
+		pf->internal_stats.tx_bytes +=
+			I40E_RXTX_BYTES_HIGH(pf->internal_old_tx_bytes);
+	}
+	pf->internal_old_rx_bytes = pf->internal_stats.rx_bytes;
+	pf->internal_old_tx_bytes = pf->internal_stats.tx_bytes;
 
 	/* exclude CRC size */
 	pf->internal_stats.rx_bytes -= (pf->internal_stats.rx_unicast + @@ -3191,6 +3223,14 @@ 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);
+	/* enlarge the limitation when rx_bytes overflowed */
+	if (pf->offset_loaded) {
+		if (I40E_RXTX_BYTES_LOW(pf->old_rx_bytes) > ns->eth.rx_bytes)
+			ns->eth.rx_bytes += (uint64_t)1 << I40E_48_BIT_WIDTH;
+		ns->eth.rx_bytes += I40E_RXTX_BYTES_HIGH(pf->old_rx_bytes);
+	}
+	pf->old_rx_bytes = ns->eth.rx_bytes;
+
 	/* Workaround: CRC size should not be included in byte statistics,
 	 * so subtract RTE_ETHER_CRC_LEN from the byte counter for each rx
 	 * packet.
@@ -3249,6 +3289,13 @@ 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);
+	/* enlarge the limitation when tx_bytes overflowed */
+	if (pf->offset_loaded) {
+		if (I40E_RXTX_BYTES_LOW(pf->old_tx_bytes) > ns->eth.tx_bytes)
+			ns->eth.tx_bytes += (uint64_t)1 << I40E_48_BIT_WIDTH;
+		ns->eth.tx_bytes += I40E_RXTX_BYTES_HIGH(pf->old_tx_bytes);
+	}
+	pf->old_tx_bytes = ns->eth.tx_bytes;
 	ns->eth.tx_bytes -= (ns->eth.tx_unicast + ns->eth.tx_multicast +
 		ns->eth.tx_broadcast) * RTE_ETHER_CRC_LEN;
 
diff --git a/drivers/net/i40e/i40e_ethdev.h b/drivers/net/i40e/i40e_ethdev.h index 19f821829..5d17be1f0 100644
--- a/drivers/net/i40e/i40e_ethdev.h
+++ b/drivers/net/i40e/i40e_ethdev.h
@@ -282,6 +282,9 @@ struct rte_flow {
 #define I40E_ETH_OVERHEAD \
 	(RTE_ETHER_HDR_LEN + RTE_ETHER_CRC_LEN + I40E_VLAN_TAG_SIZE * 2)
 
+#define I40E_RXTX_BYTES_HIGH(bytes) ((bytes) & ~I40E_48_BIT_MASK) 
+#define I40E_RXTX_BYTES_LOW(bytes) ((bytes) & I40E_48_BIT_MASK)
+
 struct i40e_adapter;
 struct rte_pci_driver;
 
@@ -399,6 +402,8 @@ struct i40e_vsi {
 	uint8_t vlan_anti_spoof_on; /* The VLAN anti-spoofing enabled */
 	uint8_t vlan_filter_on; /* The VLAN filter enabled */
 	struct i40e_bw_info bw_info; /* VSI bandwidth information */
+	uint64_t old_rx_bytes;
+	uint64_t old_tx_bytes;
 };
 
 struct pool_entry {
@@ -1156,6 +1161,10 @@ struct i40e_pf {
 	uint16_t switch_domain_id;
 
 	struct i40e_vf_msg_cfg vf_msg_cfg;
+	uint64_t old_rx_bytes;
+	uint64_t old_tx_bytes;
+	uint64_t internal_old_rx_bytes;
+	uint64_t internal_old_tx_bytes;
 };
 
 enum pending_msg {
--
2.17.1


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

* Re: [dpdk-dev] [dpdk-stable] [PATCH] net/i40e: fix incorrect byte counters
  2020-09-10  2:18 ` [dpdk-dev] [dpdk-stable] " Han, YingyaX
@ 2020-09-10  5:58   ` Guo, Jia
  2020-09-10  6:45     ` Jiang, JunyuX
  0 siblings, 1 reply; 20+ messages in thread
From: Guo, Jia @ 2020-09-10  5:58 UTC (permalink / raw)
  To: Han, YingyaX, Jiang, JunyuX, dev; +Cc: Xing, Beilei, Jiang, JunyuX, stable

Hi, junyu

> -----Original Message-----
> From: Han, YingyaX <yingyax.han@intel.com>
> Sent: Thursday, September 10, 2020 10:18 AM
> To: Jiang, JunyuX <junyux.jiang@intel.com>; dev@dpdk.org
> Cc: Guo, Jia <jia.guo@intel.com>; Xing, Beilei <beilei.xing@intel.com>; Jiang,
> JunyuX <junyux.jiang@intel.com>; stable@dpdk.org
> Subject: RE: [dpdk-stable] [PATCH] net/i40e: fix incorrect byte counters
> 
> Tested-by: Yingya Han <yingyax.han@intel.com>
> 
> -----Original Message-----
> From: stable <stable-bounces@dpdk.org> On Behalf Of Junyu Jiang
> Sent: Thursday, September 10, 2020 9:54 AM
> To: dev@dpdk.org
> Cc: Guo, Jia <jia.guo@intel.com>; Xing, Beilei <beilei.xing@intel.com>; Jiang,
> JunyuX <junyux.jiang@intel.com>; stable@dpdk.org
> Subject: [dpdk-stable] [PATCH] net/i40e: fix incorrect byte counters
> 
> This patch fixed the issue that rx/tx bytes overflowed on 48 bit limitation by
> enlarging the limitation.
> 
> Fixes: 4861cde46116 ("i40e: new poll mode driver")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Junyu Jiang <junyux.jiang@intel.com>
> ---
>  drivers/net/i40e/i40e_ethdev.c | 47
> ++++++++++++++++++++++++++++++++++
>  drivers/net/i40e/i40e_ethdev.h |  9 +++++++
>  2 files changed, 56 insertions(+)
> 
> diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
> index 11c02b188..e3d4b7f4f 100644
> --- a/drivers/net/i40e/i40e_ethdev.c
> +++ b/drivers/net/i40e/i40e_ethdev.c
> @@ -3070,6 +3070,13 @@ i40e_update_vsi_stats(struct i40e_vsi *vsi)
> i40e_stat_update_48(hw, I40E_GLV_BPRCH(idx), I40E_GLV_BPRCL(idx),
>      vsi->offset_loaded, &oes->rx_broadcast,
>      &nes->rx_broadcast);
> +/* enlarge the limitation when rx_bytes overflowed */ if
> +(vsi->offset_loaded) { if (I40E_RXTX_BYTES_LOW(vsi->old_rx_bytes) >
> +nes->rx_bytes)
> +nes->rx_bytes += (uint64_t)1 << I40E_48_BIT_WIDTH; rx_bytes +=
> +nes->I40E_RXTX_BYTES_HIGH(vsi->old_rx_bytes);
> +}
> +vsi->old_rx_bytes = nes->rx_bytes;
>  /* exclude CRC bytes */
>  nes->rx_bytes -= (nes->rx_unicast + nes->rx_multicast +
>  nes->rx_broadcast) * RTE_ETHER_CRC_LEN; @@ -3096,6 +3103,13 @@
> i40e_update_vsi_stats(struct i40e_vsi *vsi)
>  /* GLV_TDPC not supported */
>  i40e_stat_update_32(hw, I40E_GLV_TEPC(idx), vsi->offset_loaded,
>      &oes->tx_errors, &nes->tx_errors);
> +/* enlarge the limitation when tx_bytes overflowed */ if
> +(vsi->offset_loaded) { if (I40E_RXTX_BYTES_LOW(vsi->old_tx_bytes) >
> +nes->tx_bytes)
> +nes->tx_bytes += (uint64_t)1 << I40E_48_BIT_WIDTH; tx_bytes +=
> +nes->I40E_RXTX_BYTES_HIGH(vsi->old_tx_bytes);
> +}
> +vsi->old_rx_bytes = nes->rx_bytes;

It should be tx, right?

>  vsi->offset_loaded = true;
> 
>  PMD_DRV_LOG(DEBUG, "***************** VSI[%u] stats start
> *******************", @@ -3168,6 +3182,24 @@
> i40e_read_stats_registers(struct i40e_pf *pf, struct i40e_hw *hw)
>      pf->offset_loaded,
>      &pf->internal_stats_offset.tx_broadcast,
>      &pf->internal_stats.tx_broadcast);
> +/* enlarge the limitation when internal rx/tx bytes overflowed */ if
> +(pf->offset_loaded) { if
> +(I40E_RXTX_BYTES_LOW(pf->internal_old_rx_bytes) >
> +    pf->internal_stats.rx_bytes)
> +pf->internal_stats.rx_bytes +=
> +(uint64_t)1 << I40E_48_BIT_WIDTH;
> +pf->internal_stats.rx_bytes +=
> +I40E_RXTX_BYTES_HIGH(pf->internal_old_rx_bytes);
> +
> +if (I40E_RXTX_BYTES_LOW(pf->internal_old_tx_bytes) >
> +    pf->internal_stats.tx_bytes)
> +pf->internal_stats.tx_bytes +=
> +(uint64_t)1 << I40E_48_BIT_WIDTH;
> +pf->internal_stats.tx_bytes +=
> +I40E_RXTX_BYTES_HIGH(pf->internal_old_tx_bytes);
> +}
> +pf->internal_old_rx_bytes = pf->internal_stats.rx_bytes;
> +pf->internal_old_tx_bytes = pf->internal_stats.tx_bytes;
> 
>  /* exclude CRC size */
>  pf->internal_stats.rx_bytes -= (pf->internal_stats.rx_unicast + @@ -3191,6
> +3223,14 @@ 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);
> +/* enlarge the limitation when rx_bytes overflowed */ if
> +(pf->offset_loaded) { if (I40E_RXTX_BYTES_LOW(pf->old_rx_bytes) >
> +ns->eth.rx_bytes)
> +ns->eth.rx_bytes += (uint64_t)1 << I40E_48_BIT_WIDTH; eth.rx_bytes +=
> +ns->I40E_RXTX_BYTES_HIGH(pf->old_rx_bytes);
> +}
> +pf->old_rx_bytes = ns->eth.rx_bytes;
> +
>  /* Workaround: CRC size should not be included in byte statistics,
>   * so subtract RTE_ETHER_CRC_LEN from the byte counter for each rx
>   * packet.
> @@ -3249,6 +3289,13 @@ 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);
> +/* enlarge the limitation when tx_bytes overflowed */ if
> +(pf->offset_loaded) { if (I40E_RXTX_BYTES_LOW(pf->old_tx_bytes) >
> +ns->eth.tx_bytes)
> +ns->eth.tx_bytes += (uint64_t)1 << I40E_48_BIT_WIDTH; eth.tx_bytes +=
> +ns->I40E_RXTX_BYTES_HIGH(pf->old_tx_bytes);
> +}
> +pf->old_tx_bytes = ns->eth.tx_bytes;
>  ns->eth.tx_bytes -= (ns->eth.tx_unicast + ns->eth.tx_multicast +
>  ns->eth.tx_broadcast) * RTE_ETHER_CRC_LEN;
> 
> diff --git a/drivers/net/i40e/i40e_ethdev.h
> b/drivers/net/i40e/i40e_ethdev.h index 19f821829..5d17be1f0 100644
> --- a/drivers/net/i40e/i40e_ethdev.h
> +++ b/drivers/net/i40e/i40e_ethdev.h
> @@ -282,6 +282,9 @@ struct rte_flow {
>  #define I40E_ETH_OVERHEAD \
>  (RTE_ETHER_HDR_LEN + RTE_ETHER_CRC_LEN + I40E_VLAN_TAG_SIZE * 2)
> 
> +#define I40E_RXTX_BYTES_HIGH(bytes) ((bytes) & ~I40E_48_BIT_MASK)
> +#define I40E_RXTX_BYTES_LOW(bytes) ((bytes) & I40E_48_BIT_MASK)
> +
>  struct i40e_adapter;
>  struct rte_pci_driver;
> 
> @@ -399,6 +402,8 @@ struct i40e_vsi {
>  uint8_t vlan_anti_spoof_on; /* The VLAN anti-spoofing enabled */  uint8_t
> vlan_filter_on; /* The VLAN filter enabled */  struct i40e_bw_info bw_info;
> /* VSI bandwidth information */
> +uint64_t old_rx_bytes;
> +uint64_t old_tx_bytes;
>  };
> 
>  struct pool_entry {
> @@ -1156,6 +1161,10 @@ struct i40e_pf {
>  uint16_t switch_domain_id;
> 
>  struct i40e_vf_msg_cfg vf_msg_cfg;
> +uint64_t old_rx_bytes;
> +uint64_t old_tx_bytes;
> +uint64_t internal_old_rx_bytes;
> +uint64_t internal_old_tx_bytes;
>  };
> 
>  enum pending_msg {
> --
> 2.17.1
> 


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

* Re: [dpdk-dev] [dpdk-stable] [PATCH] net/i40e: fix incorrect byte counters
  2020-09-10  5:58   ` Guo, Jia
@ 2020-09-10  6:45     ` Jiang, JunyuX
  0 siblings, 0 replies; 20+ messages in thread
From: Jiang, JunyuX @ 2020-09-10  6:45 UTC (permalink / raw)
  To: Guo, Jia, Han, YingyaX, dev; +Cc: Xing, Beilei, stable

HI guojia,

> -----Original Message-----
> From: Guo, Jia <jia.guo@intel.com>
> Sent: Thursday, September 10, 2020 1:59 PM
> To: Han, YingyaX <yingyax.han@intel.com>; Jiang, JunyuX
> <junyux.jiang@intel.com>; dev@dpdk.org
> Cc: Xing, Beilei <beilei.xing@intel.com>; Jiang, JunyuX
> <junyux.jiang@intel.com>; stable@dpdk.org
> Subject: RE: [dpdk-stable] [PATCH] net/i40e: fix incorrect byte counters
> 
> Hi, junyu
> 
> > -----Original Message-----
> > From: Han, YingyaX <yingyax.han@intel.com>
> > Sent: Thursday, September 10, 2020 10:18 AM
> > To: Jiang, JunyuX <junyux.jiang@intel.com>; dev@dpdk.org
> > Cc: Guo, Jia <jia.guo@intel.com>; Xing, Beilei
> > <beilei.xing@intel.com>; Jiang, JunyuX <junyux.jiang@intel.com>;
> > stable@dpdk.org
> > Subject: RE: [dpdk-stable] [PATCH] net/i40e: fix incorrect byte
> > counters
> >
> > Tested-by: Yingya Han <yingyax.han@intel.com>
> >
> > -----Original Message-----
> > From: stable <stable-bounces@dpdk.org> On Behalf Of Junyu Jiang
> > Sent: Thursday, September 10, 2020 9:54 AM
> > To: dev@dpdk.org
> > Cc: Guo, Jia <jia.guo@intel.com>; Xing, Beilei
> > <beilei.xing@intel.com>; Jiang, JunyuX <junyux.jiang@intel.com>;
> > stable@dpdk.org
> > Subject: [dpdk-stable] [PATCH] net/i40e: fix incorrect byte counters
> >
> > This patch fixed the issue that rx/tx bytes overflowed on 48 bit
> > limitation by enlarging the limitation.
> >
> > Fixes: 4861cde46116 ("i40e: new poll mode driver")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Junyu Jiang <junyux.jiang@intel.com>
> > ---
> >  drivers/net/i40e/i40e_ethdev.c | 47
> > ++++++++++++++++++++++++++++++++++
> >  drivers/net/i40e/i40e_ethdev.h |  9 +++++++
> >  2 files changed, 56 insertions(+)
> >
> > diff --git a/drivers/net/i40e/i40e_ethdev.c
> > b/drivers/net/i40e/i40e_ethdev.c index 11c02b188..e3d4b7f4f 100644
> > --- a/drivers/net/i40e/i40e_ethdev.c
> > +++ b/drivers/net/i40e/i40e_ethdev.c
> > @@ -3070,6 +3070,13 @@ i40e_update_vsi_stats(struct i40e_vsi *vsi)
> > i40e_stat_update_48(hw, I40E_GLV_BPRCH(idx), I40E_GLV_BPRCL(idx),
> >      vsi->offset_loaded, &oes->rx_broadcast,
> >      &nes->rx_broadcast);
> > +/* enlarge the limitation when rx_bytes overflowed */ if
> > +(vsi->offset_loaded) { if (I40E_RXTX_BYTES_LOW(vsi->old_rx_bytes) >
> > +nes->rx_bytes)
> > +nes->rx_bytes += (uint64_t)1 << I40E_48_BIT_WIDTH; rx_bytes +=
> > +nes->I40E_RXTX_BYTES_HIGH(vsi->old_rx_bytes);
> > +}
> > +vsi->old_rx_bytes = nes->rx_bytes;
> >  /* exclude CRC bytes */
> >  nes->rx_bytes -= (nes->rx_unicast + nes->rx_multicast +
> >  nes->rx_broadcast) * RTE_ETHER_CRC_LEN; @@ -3096,6 +3103,13 @@
> > i40e_update_vsi_stats(struct i40e_vsi *vsi)
> >  /* GLV_TDPC not supported */
> >  i40e_stat_update_32(hw, I40E_GLV_TEPC(idx), vsi->offset_loaded,
> >      &oes->tx_errors, &nes->tx_errors);
> > +/* enlarge the limitation when tx_bytes overflowed */ if
> > +(vsi->offset_loaded) { if (I40E_RXTX_BYTES_LOW(vsi->old_tx_bytes) >
> > +nes->tx_bytes)
> > +nes->tx_bytes += (uint64_t)1 << I40E_48_BIT_WIDTH; tx_bytes +=
> > +nes->I40E_RXTX_BYTES_HIGH(vsi->old_tx_bytes);
> > +}
> > +vsi->old_rx_bytes = nes->rx_bytes;
> 
> It should be tx, right?
> 
You are right, it will be fixed in V2.
> >  vsi->offset_loaded = true;
> >


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

* [dpdk-dev] [PATCH v2] net/i40e: fix incorrect byte counters
  2020-09-10  1:54 [dpdk-dev] [PATCH] net/i40e: fix incorrect byte counters Junyu Jiang
  2020-09-10  2:18 ` [dpdk-dev] [dpdk-stable] " Han, YingyaX
@ 2020-09-16  1:51 ` Junyu Jiang
  2020-09-16  2:39   ` [dpdk-dev] [dpdk-stable] " Han, YingyaX
                     ` (2 more replies)
  2020-09-21  9:59 ` [dpdk-dev] [PATCH v3] " Junyu Jiang
                   ` (2 subsequent siblings)
  4 siblings, 3 replies; 20+ messages in thread
From: Junyu Jiang @ 2020-09-16  1:51 UTC (permalink / raw)
  To: dev; +Cc: Jeff Guo, Beilei Xing, Junyu Jiang, stable

This patch fixed the issue that rx/tx bytes overflowed
on 48 bit limitation by enlarging the limitation.

Fixes: 4861cde46116 ("i40e: new poll mode driver")
Cc: stable@dpdk.org

Signed-off-by: Junyu Jiang <junyux.jiang@intel.com>
---
 drivers/net/i40e/i40e_ethdev.c | 47 ++++++++++++++++++++++++++++++++++
 drivers/net/i40e/i40e_ethdev.h |  9 +++++++
 2 files changed, 56 insertions(+)

diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 563f21d9d..4d4ea9861 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -3073,6 +3073,13 @@ i40e_update_vsi_stats(struct i40e_vsi *vsi)
 	i40e_stat_update_48(hw, I40E_GLV_BPRCH(idx), I40E_GLV_BPRCL(idx),
 			    vsi->offset_loaded, &oes->rx_broadcast,
 			    &nes->rx_broadcast);
+	/* enlarge the limitation when rx_bytes overflowed */
+	if (vsi->offset_loaded) {
+		if (I40E_RXTX_BYTES_LOW(vsi->old_rx_bytes) > nes->rx_bytes)
+			nes->rx_bytes += (uint64_t)1 << I40E_48_BIT_WIDTH;
+		nes->rx_bytes += I40E_RXTX_BYTES_HIGH(vsi->old_rx_bytes);
+	}
+	vsi->old_rx_bytes = nes->rx_bytes;
 	/* exclude CRC bytes */
 	nes->rx_bytes -= (nes->rx_unicast + nes->rx_multicast +
 		nes->rx_broadcast) * RTE_ETHER_CRC_LEN;
@@ -3099,6 +3106,13 @@ i40e_update_vsi_stats(struct i40e_vsi *vsi)
 	/* GLV_TDPC not supported */
 	i40e_stat_update_32(hw, I40E_GLV_TEPC(idx), vsi->offset_loaded,
 			    &oes->tx_errors, &nes->tx_errors);
+	/* enlarge the limitation when tx_bytes overflowed */
+	if (vsi->offset_loaded) {
+		if (I40E_RXTX_BYTES_LOW(vsi->old_tx_bytes) > nes->tx_bytes)
+			nes->tx_bytes += (uint64_t)1 << I40E_48_BIT_WIDTH;
+		nes->tx_bytes += I40E_RXTX_BYTES_HIGH(vsi->old_tx_bytes);
+	}
+	vsi->old_tx_bytes = nes->tx_bytes;
 	vsi->offset_loaded = true;
 
 	PMD_DRV_LOG(DEBUG, "***************** VSI[%u] stats start *******************",
@@ -3171,6 +3185,24 @@ i40e_read_stats_registers(struct i40e_pf *pf, struct i40e_hw *hw)
 			    pf->offset_loaded,
 			    &pf->internal_stats_offset.tx_broadcast,
 			    &pf->internal_stats.tx_broadcast);
+	/* enlarge the limitation when internal rx/tx bytes overflowed */
+	if (pf->offset_loaded) {
+		if (I40E_RXTX_BYTES_LOW(pf->internal_old_rx_bytes) >
+		    pf->internal_stats.rx_bytes)
+			pf->internal_stats.rx_bytes +=
+				(uint64_t)1 << I40E_48_BIT_WIDTH;
+		pf->internal_stats.rx_bytes +=
+			I40E_RXTX_BYTES_HIGH(pf->internal_old_rx_bytes);
+
+		if (I40E_RXTX_BYTES_LOW(pf->internal_old_tx_bytes) >
+		    pf->internal_stats.tx_bytes)
+			pf->internal_stats.tx_bytes +=
+				(uint64_t)1 << I40E_48_BIT_WIDTH;
+		pf->internal_stats.tx_bytes +=
+			I40E_RXTX_BYTES_HIGH(pf->internal_old_tx_bytes);
+	}
+	pf->internal_old_rx_bytes = pf->internal_stats.rx_bytes;
+	pf->internal_old_tx_bytes = pf->internal_stats.tx_bytes;
 
 	/* exclude CRC size */
 	pf->internal_stats.rx_bytes -= (pf->internal_stats.rx_unicast +
@@ -3194,6 +3226,14 @@ 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);
+	/* enlarge the limitation when rx_bytes overflowed */
+	if (pf->offset_loaded) {
+		if (I40E_RXTX_BYTES_LOW(pf->old_rx_bytes) > ns->eth.rx_bytes)
+			ns->eth.rx_bytes += (uint64_t)1 << I40E_48_BIT_WIDTH;
+		ns->eth.rx_bytes += I40E_RXTX_BYTES_HIGH(pf->old_rx_bytes);
+	}
+	pf->old_rx_bytes = ns->eth.rx_bytes;
+
 	/* Workaround: CRC size should not be included in byte statistics,
 	 * so subtract RTE_ETHER_CRC_LEN from the byte counter for each rx
 	 * packet.
@@ -3252,6 +3292,13 @@ 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);
+	/* enlarge the limitation when tx_bytes overflowed */
+	if (pf->offset_loaded) {
+		if (I40E_RXTX_BYTES_LOW(pf->old_tx_bytes) > ns->eth.tx_bytes)
+			ns->eth.tx_bytes += (uint64_t)1 << I40E_48_BIT_WIDTH;
+		ns->eth.tx_bytes += I40E_RXTX_BYTES_HIGH(pf->old_tx_bytes);
+	}
+	pf->old_tx_bytes = ns->eth.tx_bytes;
 	ns->eth.tx_bytes -= (ns->eth.tx_unicast + ns->eth.tx_multicast +
 		ns->eth.tx_broadcast) * RTE_ETHER_CRC_LEN;
 
diff --git a/drivers/net/i40e/i40e_ethdev.h b/drivers/net/i40e/i40e_ethdev.h
index 19f821829..5d17be1f0 100644
--- a/drivers/net/i40e/i40e_ethdev.h
+++ b/drivers/net/i40e/i40e_ethdev.h
@@ -282,6 +282,9 @@ struct rte_flow {
 #define I40E_ETH_OVERHEAD \
 	(RTE_ETHER_HDR_LEN + RTE_ETHER_CRC_LEN + I40E_VLAN_TAG_SIZE * 2)
 
+#define I40E_RXTX_BYTES_HIGH(bytes) ((bytes) & ~I40E_48_BIT_MASK)
+#define I40E_RXTX_BYTES_LOW(bytes) ((bytes) & I40E_48_BIT_MASK)
+
 struct i40e_adapter;
 struct rte_pci_driver;
 
@@ -399,6 +402,8 @@ struct i40e_vsi {
 	uint8_t vlan_anti_spoof_on; /* The VLAN anti-spoofing enabled */
 	uint8_t vlan_filter_on; /* The VLAN filter enabled */
 	struct i40e_bw_info bw_info; /* VSI bandwidth information */
+	uint64_t old_rx_bytes;
+	uint64_t old_tx_bytes;
 };
 
 struct pool_entry {
@@ -1156,6 +1161,10 @@ struct i40e_pf {
 	uint16_t switch_domain_id;
 
 	struct i40e_vf_msg_cfg vf_msg_cfg;
+	uint64_t old_rx_bytes;
+	uint64_t old_tx_bytes;
+	uint64_t internal_old_rx_bytes;
+	uint64_t internal_old_tx_bytes;
 };
 
 enum pending_msg {
-- 
2.17.1


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

* Re: [dpdk-dev] [dpdk-stable] [PATCH v2] net/i40e: fix incorrect byte counters
  2020-09-16  1:51 ` [dpdk-dev] [PATCH v2] " Junyu Jiang
@ 2020-09-16  2:39   ` Han, YingyaX
  2020-09-16  5:21   ` [dpdk-dev] " Guo, Jia
  2020-09-16 12:30   ` [dpdk-dev] [dpdk-stable] " Ferruh Yigit
  2 siblings, 0 replies; 20+ messages in thread
From: Han, YingyaX @ 2020-09-16  2:39 UTC (permalink / raw)
  To: Jiang, JunyuX, dev; +Cc: Guo, Jia, Xing, Beilei, Jiang, JunyuX, stable

Tested-by: Yingya Han <yingyax.han@intel.com>

-----Original Message-----
From: stable <stable-bounces@dpdk.org> On Behalf Of Junyu Jiang
Sent: Wednesday, September 16, 2020 9:51 AM
To: dev@dpdk.org
Cc: Guo, Jia <jia.guo@intel.com>; Xing, Beilei <beilei.xing@intel.com>; Jiang, JunyuX <junyux.jiang@intel.com>; stable@dpdk.org
Subject: [dpdk-stable] [PATCH v2] net/i40e: fix incorrect byte counters

This patch fixed the issue that rx/tx bytes overflowed on 48 bit limitation by enlarging the limitation.

Fixes: 4861cde46116 ("i40e: new poll mode driver")
Cc: stable@dpdk.org

Signed-off-by: Junyu Jiang <junyux.jiang@intel.com>
---
 drivers/net/i40e/i40e_ethdev.c | 47 ++++++++++++++++++++++++++++++++++
 drivers/net/i40e/i40e_ethdev.h |  9 +++++++
 2 files changed, 56 insertions(+)

diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c index 563f21d9d..4d4ea9861 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -3073,6 +3073,13 @@ i40e_update_vsi_stats(struct i40e_vsi *vsi)
 	i40e_stat_update_48(hw, I40E_GLV_BPRCH(idx), I40E_GLV_BPRCL(idx),
 			    vsi->offset_loaded, &oes->rx_broadcast,
 			    &nes->rx_broadcast);
+	/* enlarge the limitation when rx_bytes overflowed */
+	if (vsi->offset_loaded) {
+		if (I40E_RXTX_BYTES_LOW(vsi->old_rx_bytes) > nes->rx_bytes)
+			nes->rx_bytes += (uint64_t)1 << I40E_48_BIT_WIDTH;
+		nes->rx_bytes += I40E_RXTX_BYTES_HIGH(vsi->old_rx_bytes);
+	}
+	vsi->old_rx_bytes = nes->rx_bytes;
 	/* exclude CRC bytes */
 	nes->rx_bytes -= (nes->rx_unicast + nes->rx_multicast +
 		nes->rx_broadcast) * RTE_ETHER_CRC_LEN; @@ -3099,6 +3106,13 @@ i40e_update_vsi_stats(struct i40e_vsi *vsi)
 	/* GLV_TDPC not supported */
 	i40e_stat_update_32(hw, I40E_GLV_TEPC(idx), vsi->offset_loaded,
 			    &oes->tx_errors, &nes->tx_errors);
+	/* enlarge the limitation when tx_bytes overflowed */
+	if (vsi->offset_loaded) {
+		if (I40E_RXTX_BYTES_LOW(vsi->old_tx_bytes) > nes->tx_bytes)
+			nes->tx_bytes += (uint64_t)1 << I40E_48_BIT_WIDTH;
+		nes->tx_bytes += I40E_RXTX_BYTES_HIGH(vsi->old_tx_bytes);
+	}
+	vsi->old_tx_bytes = nes->tx_bytes;
 	vsi->offset_loaded = true;
 
 	PMD_DRV_LOG(DEBUG, "***************** VSI[%u] stats start *******************", @@ -3171,6 +3185,24 @@ i40e_read_stats_registers(struct i40e_pf *pf, struct i40e_hw *hw)
 			    pf->offset_loaded,
 			    &pf->internal_stats_offset.tx_broadcast,
 			    &pf->internal_stats.tx_broadcast);
+	/* enlarge the limitation when internal rx/tx bytes overflowed */
+	if (pf->offset_loaded) {
+		if (I40E_RXTX_BYTES_LOW(pf->internal_old_rx_bytes) >
+		    pf->internal_stats.rx_bytes)
+			pf->internal_stats.rx_bytes +=
+				(uint64_t)1 << I40E_48_BIT_WIDTH;
+		pf->internal_stats.rx_bytes +=
+			I40E_RXTX_BYTES_HIGH(pf->internal_old_rx_bytes);
+
+		if (I40E_RXTX_BYTES_LOW(pf->internal_old_tx_bytes) >
+		    pf->internal_stats.tx_bytes)
+			pf->internal_stats.tx_bytes +=
+				(uint64_t)1 << I40E_48_BIT_WIDTH;
+		pf->internal_stats.tx_bytes +=
+			I40E_RXTX_BYTES_HIGH(pf->internal_old_tx_bytes);
+	}
+	pf->internal_old_rx_bytes = pf->internal_stats.rx_bytes;
+	pf->internal_old_tx_bytes = pf->internal_stats.tx_bytes;
 
 	/* exclude CRC size */
 	pf->internal_stats.rx_bytes -= (pf->internal_stats.rx_unicast + @@ -3194,6 +3226,14 @@ 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);
+	/* enlarge the limitation when rx_bytes overflowed */
+	if (pf->offset_loaded) {
+		if (I40E_RXTX_BYTES_LOW(pf->old_rx_bytes) > ns->eth.rx_bytes)
+			ns->eth.rx_bytes += (uint64_t)1 << I40E_48_BIT_WIDTH;
+		ns->eth.rx_bytes += I40E_RXTX_BYTES_HIGH(pf->old_rx_bytes);
+	}
+	pf->old_rx_bytes = ns->eth.rx_bytes;
+
 	/* Workaround: CRC size should not be included in byte statistics,
 	 * so subtract RTE_ETHER_CRC_LEN from the byte counter for each rx
 	 * packet.
@@ -3252,6 +3292,13 @@ 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);
+	/* enlarge the limitation when tx_bytes overflowed */
+	if (pf->offset_loaded) {
+		if (I40E_RXTX_BYTES_LOW(pf->old_tx_bytes) > ns->eth.tx_bytes)
+			ns->eth.tx_bytes += (uint64_t)1 << I40E_48_BIT_WIDTH;
+		ns->eth.tx_bytes += I40E_RXTX_BYTES_HIGH(pf->old_tx_bytes);
+	}
+	pf->old_tx_bytes = ns->eth.tx_bytes;
 	ns->eth.tx_bytes -= (ns->eth.tx_unicast + ns->eth.tx_multicast +
 		ns->eth.tx_broadcast) * RTE_ETHER_CRC_LEN;
 
diff --git a/drivers/net/i40e/i40e_ethdev.h b/drivers/net/i40e/i40e_ethdev.h index 19f821829..5d17be1f0 100644
--- a/drivers/net/i40e/i40e_ethdev.h
+++ b/drivers/net/i40e/i40e_ethdev.h
@@ -282,6 +282,9 @@ struct rte_flow {
 #define I40E_ETH_OVERHEAD \
 	(RTE_ETHER_HDR_LEN + RTE_ETHER_CRC_LEN + I40E_VLAN_TAG_SIZE * 2)
 
+#define I40E_RXTX_BYTES_HIGH(bytes) ((bytes) & ~I40E_48_BIT_MASK) 
+#define I40E_RXTX_BYTES_LOW(bytes) ((bytes) & I40E_48_BIT_MASK)
+
 struct i40e_adapter;
 struct rte_pci_driver;
 
@@ -399,6 +402,8 @@ struct i40e_vsi {
 	uint8_t vlan_anti_spoof_on; /* The VLAN anti-spoofing enabled */
 	uint8_t vlan_filter_on; /* The VLAN filter enabled */
 	struct i40e_bw_info bw_info; /* VSI bandwidth information */
+	uint64_t old_rx_bytes;
+	uint64_t old_tx_bytes;
 };
 
 struct pool_entry {
@@ -1156,6 +1161,10 @@ struct i40e_pf {
 	uint16_t switch_domain_id;
 
 	struct i40e_vf_msg_cfg vf_msg_cfg;
+	uint64_t old_rx_bytes;
+	uint64_t old_tx_bytes;
+	uint64_t internal_old_rx_bytes;
+	uint64_t internal_old_tx_bytes;
 };
 
 enum pending_msg {
--
2.17.1


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

* Re: [dpdk-dev] [PATCH v2] net/i40e: fix incorrect byte counters
  2020-09-16  1:51 ` [dpdk-dev] [PATCH v2] " Junyu Jiang
  2020-09-16  2:39   ` [dpdk-dev] [dpdk-stable] " Han, YingyaX
@ 2020-09-16  5:21   ` Guo, Jia
  2020-09-16  5:50     ` Zhang, Qi Z
  2020-09-16 12:30   ` [dpdk-dev] [dpdk-stable] " Ferruh Yigit
  2 siblings, 1 reply; 20+ messages in thread
From: Guo, Jia @ 2020-09-16  5:21 UTC (permalink / raw)
  To: Jiang, JunyuX, dev; +Cc: Xing, Beilei, stable

Acked-by: Jeff Guo <jia.guo@intel.com>

> -----Original Message-----
> From: Jiang, JunyuX <junyux.jiang@intel.com>
> Sent: Wednesday, September 16, 2020 9:51 AM
> To: dev@dpdk.org
> Cc: Guo, Jia <jia.guo@intel.com>; Xing, Beilei <beilei.xing@intel.com>; Jiang,
> JunyuX <junyux.jiang@intel.com>; stable@dpdk.org
> Subject: [PATCH v2] net/i40e: fix incorrect byte counters
> 
> This patch fixed the issue that rx/tx bytes overflowed on 48 bit limitation by
> enlarging the limitation.
> 
> Fixes: 4861cde46116 ("i40e: new poll mode driver")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Junyu Jiang <junyux.jiang@intel.com>
> ---
>  drivers/net/i40e/i40e_ethdev.c | 47
> ++++++++++++++++++++++++++++++++++
>  drivers/net/i40e/i40e_ethdev.h |  9 +++++++
>  2 files changed, 56 insertions(+)
> 
> diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
> index 563f21d9d..4d4ea9861 100644
> --- a/drivers/net/i40e/i40e_ethdev.c
> +++ b/drivers/net/i40e/i40e_ethdev.c
> @@ -3073,6 +3073,13 @@ i40e_update_vsi_stats(struct i40e_vsi *vsi)
>  	i40e_stat_update_48(hw, I40E_GLV_BPRCH(idx),
> I40E_GLV_BPRCL(idx),
>  			    vsi->offset_loaded, &oes->rx_broadcast,
>  			    &nes->rx_broadcast);
> +	/* enlarge the limitation when rx_bytes overflowed */
> +	if (vsi->offset_loaded) {
> +		if (I40E_RXTX_BYTES_LOW(vsi->old_rx_bytes) > nes-
> >rx_bytes)
> +			nes->rx_bytes += (uint64_t)1 << I40E_48_BIT_WIDTH;
> +		nes->rx_bytes += I40E_RXTX_BYTES_HIGH(vsi-
> >old_rx_bytes);
> +	}
> +	vsi->old_rx_bytes = nes->rx_bytes;
>  	/* exclude CRC bytes */
>  	nes->rx_bytes -= (nes->rx_unicast + nes->rx_multicast +
>  		nes->rx_broadcast) * RTE_ETHER_CRC_LEN; @@ -3099,6
> +3106,13 @@ i40e_update_vsi_stats(struct i40e_vsi *vsi)
>  	/* GLV_TDPC not supported */
>  	i40e_stat_update_32(hw, I40E_GLV_TEPC(idx), vsi->offset_loaded,
>  			    &oes->tx_errors, &nes->tx_errors);
> +	/* enlarge the limitation when tx_bytes overflowed */
> +	if (vsi->offset_loaded) {
> +		if (I40E_RXTX_BYTES_LOW(vsi->old_tx_bytes) > nes-
> >tx_bytes)
> +			nes->tx_bytes += (uint64_t)1 << I40E_48_BIT_WIDTH;
> +		nes->tx_bytes += I40E_RXTX_BYTES_HIGH(vsi-
> >old_tx_bytes);
> +	}
> +	vsi->old_tx_bytes = nes->tx_bytes;
>  	vsi->offset_loaded = true;
> 
>  	PMD_DRV_LOG(DEBUG, "***************** VSI[%u] stats start
> *******************", @@ -3171,6 +3185,24 @@
> i40e_read_stats_registers(struct i40e_pf *pf, struct i40e_hw *hw)
>  			    pf->offset_loaded,
>  			    &pf->internal_stats_offset.tx_broadcast,
>  			    &pf->internal_stats.tx_broadcast);
> +	/* enlarge the limitation when internal rx/tx bytes overflowed */
> +	if (pf->offset_loaded) {
> +		if (I40E_RXTX_BYTES_LOW(pf->internal_old_rx_bytes) >
> +		    pf->internal_stats.rx_bytes)
> +			pf->internal_stats.rx_bytes +=
> +				(uint64_t)1 << I40E_48_BIT_WIDTH;
> +		pf->internal_stats.rx_bytes +=
> +			I40E_RXTX_BYTES_HIGH(pf->internal_old_rx_bytes);
> +
> +		if (I40E_RXTX_BYTES_LOW(pf->internal_old_tx_bytes) >
> +		    pf->internal_stats.tx_bytes)
> +			pf->internal_stats.tx_bytes +=
> +				(uint64_t)1 << I40E_48_BIT_WIDTH;
> +		pf->internal_stats.tx_bytes +=
> +			I40E_RXTX_BYTES_HIGH(pf->internal_old_tx_bytes);
> +	}
> +	pf->internal_old_rx_bytes = pf->internal_stats.rx_bytes;
> +	pf->internal_old_tx_bytes = pf->internal_stats.tx_bytes;
> 
>  	/* exclude CRC size */
>  	pf->internal_stats.rx_bytes -= (pf->internal_stats.rx_unicast + @@ -
> 3194,6 +3226,14 @@ 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);
> +	/* enlarge the limitation when rx_bytes overflowed */
> +	if (pf->offset_loaded) {
> +		if (I40E_RXTX_BYTES_LOW(pf->old_rx_bytes) > ns-
> >eth.rx_bytes)
> +			ns->eth.rx_bytes += (uint64_t)1 <<
> I40E_48_BIT_WIDTH;
> +		ns->eth.rx_bytes += I40E_RXTX_BYTES_HIGH(pf-
> >old_rx_bytes);
> +	}
> +	pf->old_rx_bytes = ns->eth.rx_bytes;
> +
>  	/* Workaround: CRC size should not be included in byte statistics,
>  	 * so subtract RTE_ETHER_CRC_LEN from the byte counter for each rx
>  	 * packet.
> @@ -3252,6 +3292,13 @@ 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);
> +	/* enlarge the limitation when tx_bytes overflowed */
> +	if (pf->offset_loaded) {
> +		if (I40E_RXTX_BYTES_LOW(pf->old_tx_bytes) > ns-
> >eth.tx_bytes)
> +			ns->eth.tx_bytes += (uint64_t)1 <<
> I40E_48_BIT_WIDTH;
> +		ns->eth.tx_bytes += I40E_RXTX_BYTES_HIGH(pf-
> >old_tx_bytes);
> +	}
> +	pf->old_tx_bytes = ns->eth.tx_bytes;
>  	ns->eth.tx_bytes -= (ns->eth.tx_unicast + ns->eth.tx_multicast +
>  		ns->eth.tx_broadcast) * RTE_ETHER_CRC_LEN;
> 
> diff --git a/drivers/net/i40e/i40e_ethdev.h
> b/drivers/net/i40e/i40e_ethdev.h index 19f821829..5d17be1f0 100644
> --- a/drivers/net/i40e/i40e_ethdev.h
> +++ b/drivers/net/i40e/i40e_ethdev.h
> @@ -282,6 +282,9 @@ struct rte_flow {
>  #define I40E_ETH_OVERHEAD \
>  	(RTE_ETHER_HDR_LEN + RTE_ETHER_CRC_LEN +
> I40E_VLAN_TAG_SIZE * 2)
> 
> +#define I40E_RXTX_BYTES_HIGH(bytes) ((bytes) & ~I40E_48_BIT_MASK)
> +#define I40E_RXTX_BYTES_LOW(bytes) ((bytes) & I40E_48_BIT_MASK)
> +
>  struct i40e_adapter;
>  struct rte_pci_driver;
> 
> @@ -399,6 +402,8 @@ struct i40e_vsi {
>  	uint8_t vlan_anti_spoof_on; /* The VLAN anti-spoofing enabled */
>  	uint8_t vlan_filter_on; /* The VLAN filter enabled */
>  	struct i40e_bw_info bw_info; /* VSI bandwidth information */
> +	uint64_t old_rx_bytes;
> +	uint64_t old_tx_bytes;
>  };
> 
>  struct pool_entry {
> @@ -1156,6 +1161,10 @@ struct i40e_pf {
>  	uint16_t switch_domain_id;
> 
>  	struct i40e_vf_msg_cfg vf_msg_cfg;
> +	uint64_t old_rx_bytes;
> +	uint64_t old_tx_bytes;
> +	uint64_t internal_old_rx_bytes;
> +	uint64_t internal_old_tx_bytes;
>  };
> 
>  enum pending_msg {
> --
> 2.17.1


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

* Re: [dpdk-dev] [PATCH v2] net/i40e: fix incorrect byte counters
  2020-09-16  5:21   ` [dpdk-dev] " Guo, Jia
@ 2020-09-16  5:50     ` Zhang, Qi Z
  0 siblings, 0 replies; 20+ messages in thread
From: Zhang, Qi Z @ 2020-09-16  5:50 UTC (permalink / raw)
  To: Guo, Jia, Jiang, JunyuX, dev; +Cc: Xing, Beilei, stable



> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Guo, Jia
> Sent: Wednesday, September 16, 2020 1:22 PM
> To: Jiang, JunyuX <junyux.jiang@intel.com>; dev@dpdk.org
> Cc: Xing, Beilei <beilei.xing@intel.com>; stable@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2] net/i40e: fix incorrect byte counters
> 
> Acked-by: Jeff Guo <jia.guo@intel.com>
> 
> > -----Original Message-----
> > From: Jiang, JunyuX <junyux.jiang@intel.com>
> > Sent: Wednesday, September 16, 2020 9:51 AM
> > To: dev@dpdk.org
> > Cc: Guo, Jia <jia.guo@intel.com>; Xing, Beilei
> > <beilei.xing@intel.com>; Jiang, JunyuX <junyux.jiang@intel.com>;
> > stable@dpdk.org
> > Subject: [PATCH v2] net/i40e: fix incorrect byte counters
> >
> > This patch fixed the issue that rx/tx bytes overflowed on 48 bit
> > limitation by enlarging the limitation.
> >
> > Fixes: 4861cde46116 ("i40e: new poll mode driver")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Junyu Jiang <junyux.jiang@intel.com>
> > ---

Applied to dpdk-next-net-intel.

Thanks
Qi

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

* Re: [dpdk-dev] [dpdk-stable] [PATCH v2] net/i40e: fix incorrect byte counters
  2020-09-16  1:51 ` [dpdk-dev] [PATCH v2] " Junyu Jiang
  2020-09-16  2:39   ` [dpdk-dev] [dpdk-stable] " Han, YingyaX
  2020-09-16  5:21   ` [dpdk-dev] " Guo, Jia
@ 2020-09-16 12:30   ` Ferruh Yigit
  2020-09-18  3:44     ` Jiang, JunyuX
  2 siblings, 1 reply; 20+ messages in thread
From: Ferruh Yigit @ 2020-09-16 12:30 UTC (permalink / raw)
  To: Junyu Jiang, dev; +Cc: Jeff Guo, Beilei Xing, stable

On 9/16/2020 2:51 AM, Junyu Jiang wrote:
> This patch fixed the issue that rx/tx bytes overflowed

"Rx/Tx statistics counters overflowed"?

> on 48 bit limitation by enlarging the limitation.
> 
> Fixes: 4861cde46116 ("i40e: new poll mode driver")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Junyu Jiang <junyux.jiang@intel.com>
> ---
>   drivers/net/i40e/i40e_ethdev.c | 47 ++++++++++++++++++++++++++++++++++
>   drivers/net/i40e/i40e_ethdev.h |  9 +++++++
>   2 files changed, 56 insertions(+)
> 
> diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
> index 563f21d9d..4d4ea9861 100644
> --- a/drivers/net/i40e/i40e_ethdev.c
> +++ b/drivers/net/i40e/i40e_ethdev.c
> @@ -3073,6 +3073,13 @@ i40e_update_vsi_stats(struct i40e_vsi *vsi)
>   	i40e_stat_update_48(hw, I40E_GLV_BPRCH(idx), I40E_GLV_BPRCL(idx),
>   			    vsi->offset_loaded, &oes->rx_broadcast,
>   			    &nes->rx_broadcast);
> +	/* enlarge the limitation when rx_bytes overflowed */
> +	if (vsi->offset_loaded) {
> +		if (I40E_RXTX_BYTES_LOW(vsi->old_rx_bytes) > nes->rx_bytes)
> +			nes->rx_bytes += (uint64_t)1 << I40E_48_BIT_WIDTH;
> +		nes->rx_bytes += I40E_RXTX_BYTES_HIGH(vsi->old_rx_bytes);
> +	}
> +	vsi->old_rx_bytes = nes->rx_bytes;


Can you please describe this logic? (indeed better to describe it in the 
commit log)

'nes->rx_bytes' is diff in the stats register since last read.
'old_rx_bytes' is the previous stats diff.

Why/how "I40E_RXTX_BYTES_LOW(vsi->old_rx_bytes) > nes->rx_bytes" has a 
meaning? Isn't this very depends on the read frequency?

I guess I am missing something but please help me understand.

Also can you please confirm the initial value of the 
"vsi->offset_loaded" is correct.

<....>

> @@ -282,6 +282,9 @@ struct rte_flow {
>   #define I40E_ETH_OVERHEAD \
>   	(RTE_ETHER_HDR_LEN + RTE_ETHER_CRC_LEN + I40E_VLAN_TAG_SIZE * 2)
>   
> +#define I40E_RXTX_BYTES_HIGH(bytes) ((bytes) & ~I40E_48_BIT_MASK)
> +#define I40E_RXTX_BYTES_LOW(bytes) ((bytes) & I40E_48_BIT_MASK)
> +

HIGH/LOW is a little misleading, for 64Bits it sounds like it is getting 
low 32 bits and high 32 bits, can you please clarify macro masks out 
48/16 bits.


>   struct i40e_adapter;
>   struct rte_pci_driver;
>   
> @@ -399,6 +402,8 @@ struct i40e_vsi {
>   	uint8_t vlan_anti_spoof_on; /* The VLAN anti-spoofing enabled */
>   	uint8_t vlan_filter_on; /* The VLAN filter enabled */
>   	struct i40e_bw_info bw_info; /* VSI bandwidth information */
> +	uint64_t old_rx_bytes;
> +	uint64_t old_tx_bytes;

'prev' seems better naming than 'old', what do you think renaming 
'old_rx_bytes' -> 'prev_rx_bytes' (for all variables)?

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

* Re: [dpdk-dev] [dpdk-stable] [PATCH v2] net/i40e: fix incorrect byte counters
  2020-09-16 12:30   ` [dpdk-dev] [dpdk-stable] " Ferruh Yigit
@ 2020-09-18  3:44     ` Jiang, JunyuX
  2020-09-18  9:23       ` Igor Ryzhov
  2020-09-18 13:37       ` Ferruh Yigit
  0 siblings, 2 replies; 20+ messages in thread
From: Jiang, JunyuX @ 2020-09-18  3:44 UTC (permalink / raw)
  To: Yigit, Ferruh, dev; +Cc: Guo, Jia, Xing, Beilei, stable

Hi Ferruh,

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Wednesday, September 16, 2020 8:31 PM
> To: Jiang, JunyuX <junyux.jiang@intel.com>; dev@dpdk.org
> Cc: Guo, Jia <jia.guo@intel.com>; Xing, Beilei <beilei.xing@intel.com>;
> stable@dpdk.org
> Subject: Re: [dpdk-stable] [PATCH v2] net/i40e: fix incorrect byte counters
> 
> On 9/16/2020 2:51 AM, Junyu Jiang wrote:
> > This patch fixed the issue that rx/tx bytes overflowed
> 
> "Rx/Tx statistics counters overflowed"?
> 
Yes, the rx_bytes and tx_bytes counter in X710 cards is 48-bit long, if keep sending packets for a log time, the register will overflow.

> > on 48 bit limitation by enlarging the limitation.
> >
> > Fixes: 4861cde46116 ("i40e: new poll mode driver")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Junyu Jiang <junyux.jiang@intel.com>
> > ---
> >   drivers/net/i40e/i40e_ethdev.c | 47
> ++++++++++++++++++++++++++++++++++
> >   drivers/net/i40e/i40e_ethdev.h |  9 +++++++
> >   2 files changed, 56 insertions(+)
> >
> > diff --git a/drivers/net/i40e/i40e_ethdev.c
> > b/drivers/net/i40e/i40e_ethdev.c index 563f21d9d..4d4ea9861 100644
> > --- a/drivers/net/i40e/i40e_ethdev.c
> > +++ b/drivers/net/i40e/i40e_ethdev.c
> > @@ -3073,6 +3073,13 @@ i40e_update_vsi_stats(struct i40e_vsi *vsi)
> >   	i40e_stat_update_48(hw, I40E_GLV_BPRCH(idx),
> I40E_GLV_BPRCL(idx),
> >   			    vsi->offset_loaded, &oes->rx_broadcast,
> >   			    &nes->rx_broadcast);
> > +	/* enlarge the limitation when rx_bytes overflowed */
> > +	if (vsi->offset_loaded) {
> > +		if (I40E_RXTX_BYTES_LOW(vsi->old_rx_bytes) > nes-
> >rx_bytes)
> > +			nes->rx_bytes += (uint64_t)1 << I40E_48_BIT_WIDTH;
> > +		nes->rx_bytes += I40E_RXTX_BYTES_HIGH(vsi-
> >old_rx_bytes);
> > +	}
> > +	vsi->old_rx_bytes = nes->rx_bytes;
> 
> 
> Can you please describe this logic? (indeed better to describe it in the
> commit log)
> 
> 'nes->rx_bytes' is diff in the stats register since last read.
> 'old_rx_bytes' is the previous stats diff.
> 
> Why/how "I40E_RXTX_BYTES_LOW(vsi->old_rx_bytes) > nes->rx_bytes" has
> a meaning? Isn't this very depends on the read frequency?
> 
> I guess I am missing something but please help me understand.
> 
This patch fixes the issue of rx/tx bytes counter register overflow:
The counter register in i40e is 48-bit long, when overflow, nes->rx_bytes becomes less than old_rx_bytes, the correct value of nes->rx_bytes should be plused 1 << 48.
Use I40E_RXTX_BYTES_HIGH() to remember the MSB, nes->rx_bytes plus the MSB is the correct value, So that using uint64_t to enlarge the 48 bit  limitation of register .

> Also can you please confirm the initial value of the "vsi->offset_loaded" is
> correct.
> 
offset_loaded will be true when get statistics of  port and offset_loaded will be false when reset or clear the statistics,
so if  offset_loaded is false, shouldn't to calculate the value of nes->rx_bytes, it will be 0.

> <....>
> 
> > @@ -282,6 +282,9 @@ struct rte_flow {
> >   #define I40E_ETH_OVERHEAD \
> >   	(RTE_ETHER_HDR_LEN + RTE_ETHER_CRC_LEN +
> I40E_VLAN_TAG_SIZE * 2)
> >
> > +#define I40E_RXTX_BYTES_HIGH(bytes) ((bytes) & ~I40E_48_BIT_MASK)
> > +#define I40E_RXTX_BYTES_LOW(bytes) ((bytes) & I40E_48_BIT_MASK)
> > +
> 
> HIGH/LOW is a little misleading, for 64Bits it sounds like it is getting low 32 bits
> and high 32 bits, can you please clarify macro masks out
> 48/16 bits.
> 
Yes, I will change the macro name in V3.
> 
> >   struct i40e_adapter;
> >   struct rte_pci_driver;
> >
> > @@ -399,6 +402,8 @@ struct i40e_vsi {
> >   	uint8_t vlan_anti_spoof_on; /* The VLAN anti-spoofing enabled */
> >   	uint8_t vlan_filter_on; /* The VLAN filter enabled */
> >   	struct i40e_bw_info bw_info; /* VSI bandwidth information */
> > +	uint64_t old_rx_bytes;
> > +	uint64_t old_tx_bytes;
> 
> 'prev' seems better naming than 'old', what do you think renaming
> 'old_rx_bytes' -> 'prev_rx_bytes' (for all variables)?
Yes, it's better, I will fix it in V3.

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

* Re: [dpdk-dev] [dpdk-stable] [PATCH v2] net/i40e: fix incorrect byte counters
  2020-09-18  3:44     ` Jiang, JunyuX
@ 2020-09-18  9:23       ` Igor Ryzhov
  2020-09-18 13:42         ` Ferruh Yigit
  2020-09-18 13:37       ` Ferruh Yigit
  1 sibling, 1 reply; 20+ messages in thread
From: Igor Ryzhov @ 2020-09-18  9:23 UTC (permalink / raw)
  To: Jiang, JunyuX; +Cc: Yigit, Ferruh, dev, Guo, Jia, Xing, Beilei, stable

Hi,

Your code will work only if stats are updated at least once between two
overflows.
So it's still up to the application to handle this properly. I think it
should be mentioned in the docs.

Igor

On Fri, Sep 18, 2020 at 6:45 AM Jiang, JunyuX <junyux.jiang@intel.com>
wrote:

> Hi Ferruh,
>
> > -----Original Message-----
> > From: Ferruh Yigit <ferruh.yigit@intel.com>
> > Sent: Wednesday, September 16, 2020 8:31 PM
> > To: Jiang, JunyuX <junyux.jiang@intel.com>; dev@dpdk.org
> > Cc: Guo, Jia <jia.guo@intel.com>; Xing, Beilei <beilei.xing@intel.com>;
> > stable@dpdk.org
> > Subject: Re: [dpdk-stable] [PATCH v2] net/i40e: fix incorrect byte
> counters
> >
> > On 9/16/2020 2:51 AM, Junyu Jiang wrote:
> > > This patch fixed the issue that rx/tx bytes overflowed
> >
> > "Rx/Tx statistics counters overflowed"?
> >
> Yes, the rx_bytes and tx_bytes counter in X710 cards is 48-bit long, if
> keep sending packets for a log time, the register will overflow.
>
> > > on 48 bit limitation by enlarging the limitation.
> > >
> > > Fixes: 4861cde46116 ("i40e: new poll mode driver")
> > > Cc: stable@dpdk.org
> > >
> > > Signed-off-by: Junyu Jiang <junyux.jiang@intel.com>
> > > ---
> > >   drivers/net/i40e/i40e_ethdev.c | 47
> > ++++++++++++++++++++++++++++++++++
> > >   drivers/net/i40e/i40e_ethdev.h |  9 +++++++
> > >   2 files changed, 56 insertions(+)
> > >
> > > diff --git a/drivers/net/i40e/i40e_ethdev.c
> > > b/drivers/net/i40e/i40e_ethdev.c index 563f21d9d..4d4ea9861 100644
> > > --- a/drivers/net/i40e/i40e_ethdev.c
> > > +++ b/drivers/net/i40e/i40e_ethdev.c
> > > @@ -3073,6 +3073,13 @@ i40e_update_vsi_stats(struct i40e_vsi *vsi)
> > >     i40e_stat_update_48(hw, I40E_GLV_BPRCH(idx),
> > I40E_GLV_BPRCL(idx),
> > >                         vsi->offset_loaded, &oes->rx_broadcast,
> > >                         &nes->rx_broadcast);
> > > +   /* enlarge the limitation when rx_bytes overflowed */
> > > +   if (vsi->offset_loaded) {
> > > +           if (I40E_RXTX_BYTES_LOW(vsi->old_rx_bytes) > nes-
> > >rx_bytes)
> > > +                   nes->rx_bytes += (uint64_t)1 << I40E_48_BIT_WIDTH;
> > > +           nes->rx_bytes += I40E_RXTX_BYTES_HIGH(vsi-
> > >old_rx_bytes);
> > > +   }
> > > +   vsi->old_rx_bytes = nes->rx_bytes;
> >
> >
> > Can you please describe this logic? (indeed better to describe it in the
> > commit log)
> >
> > 'nes->rx_bytes' is diff in the stats register since last read.
> > 'old_rx_bytes' is the previous stats diff.
> >
> > Why/how "I40E_RXTX_BYTES_LOW(vsi->old_rx_bytes) > nes->rx_bytes" has
> > a meaning? Isn't this very depends on the read frequency?
> >
> > I guess I am missing something but please help me understand.
> >
> This patch fixes the issue of rx/tx bytes counter register overflow:
> The counter register in i40e is 48-bit long, when overflow, nes->rx_bytes
> becomes less than old_rx_bytes, the correct value of nes->rx_bytes should
> be plused 1 << 48.
> Use I40E_RXTX_BYTES_HIGH() to remember the MSB, nes->rx_bytes plus the MSB
> is the correct value, So that using uint64_t to enlarge the 48 bit
> limitation of register .
>
> > Also can you please confirm the initial value of the
> "vsi->offset_loaded" is
> > correct.
> >
> offset_loaded will be true when get statistics of  port and offset_loaded
> will be false when reset or clear the statistics,
> so if  offset_loaded is false, shouldn't to calculate the value of
> nes->rx_bytes, it will be 0.
>
> > <....>
> >
> > > @@ -282,6 +282,9 @@ struct rte_flow {
> > >   #define I40E_ETH_OVERHEAD \
> > >     (RTE_ETHER_HDR_LEN + RTE_ETHER_CRC_LEN +
> > I40E_VLAN_TAG_SIZE * 2)
> > >
> > > +#define I40E_RXTX_BYTES_HIGH(bytes) ((bytes) & ~I40E_48_BIT_MASK)
> > > +#define I40E_RXTX_BYTES_LOW(bytes) ((bytes) & I40E_48_BIT_MASK)
> > > +
> >
> > HIGH/LOW is a little misleading, for 64Bits it sounds like it is getting
> low 32 bits
> > and high 32 bits, can you please clarify macro masks out
> > 48/16 bits.
> >
> Yes, I will change the macro name in V3.
> >
> > >   struct i40e_adapter;
> > >   struct rte_pci_driver;
> > >
> > > @@ -399,6 +402,8 @@ struct i40e_vsi {
> > >     uint8_t vlan_anti_spoof_on; /* The VLAN anti-spoofing enabled */
> > >     uint8_t vlan_filter_on; /* The VLAN filter enabled */
> > >     struct i40e_bw_info bw_info; /* VSI bandwidth information */
> > > +   uint64_t old_rx_bytes;
> > > +   uint64_t old_tx_bytes;
> >
> > 'prev' seems better naming than 'old', what do you think renaming
> > 'old_rx_bytes' -> 'prev_rx_bytes' (for all variables)?
> Yes, it's better, I will fix it in V3.
>

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

* Re: [dpdk-dev] [dpdk-stable] [PATCH v2] net/i40e: fix incorrect byte counters
  2020-09-18  3:44     ` Jiang, JunyuX
  2020-09-18  9:23       ` Igor Ryzhov
@ 2020-09-18 13:37       ` Ferruh Yigit
  1 sibling, 0 replies; 20+ messages in thread
From: Ferruh Yigit @ 2020-09-18 13:37 UTC (permalink / raw)
  To: Jiang, JunyuX, dev; +Cc: Guo, Jia, Xing, Beilei, stable

On 9/18/2020 4:44 AM, Jiang, JunyuX wrote:
> Hi Ferruh,
> 
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>> Sent: Wednesday, September 16, 2020 8:31 PM
>> To: Jiang, JunyuX <junyux.jiang@intel.com>; dev@dpdk.org
>> Cc: Guo, Jia <jia.guo@intel.com>; Xing, Beilei <beilei.xing@intel.com>;
>> stable@dpdk.org
>> Subject: Re: [dpdk-stable] [PATCH v2] net/i40e: fix incorrect byte counters
>>
>> On 9/16/2020 2:51 AM, Junyu Jiang wrote:
>>> This patch fixed the issue that rx/tx bytes overflowed
>>
>> "Rx/Tx statistics counters overflowed"?
>>
> Yes, the rx_bytes and tx_bytes counter in X710 cards is 48-bit long, if keep sending packets for a log time, the register will overflow.
> 
>>> on 48 bit limitation by enlarging the limitation.
>>>
>>> Fixes: 4861cde46116 ("i40e: new poll mode driver")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Junyu Jiang <junyux.jiang@intel.com>
>>> ---
>>>    drivers/net/i40e/i40e_ethdev.c | 47
>> ++++++++++++++++++++++++++++++++++
>>>    drivers/net/i40e/i40e_ethdev.h |  9 +++++++
>>>    2 files changed, 56 insertions(+)
>>>
>>> diff --git a/drivers/net/i40e/i40e_ethdev.c
>>> b/drivers/net/i40e/i40e_ethdev.c index 563f21d9d..4d4ea9861 100644
>>> --- a/drivers/net/i40e/i40e_ethdev.c
>>> +++ b/drivers/net/i40e/i40e_ethdev.c
>>> @@ -3073,6 +3073,13 @@ i40e_update_vsi_stats(struct i40e_vsi *vsi)
>>>    	i40e_stat_update_48(hw, I40E_GLV_BPRCH(idx),
>> I40E_GLV_BPRCL(idx),
>>>    			    vsi->offset_loaded, &oes->rx_broadcast,
>>>    			    &nes->rx_broadcast);
>>> +	/* enlarge the limitation when rx_bytes overflowed */
>>> +	if (vsi->offset_loaded) {
>>> +		if (I40E_RXTX_BYTES_LOW(vsi->old_rx_bytes) > nes-
>>> rx_bytes)
>>> +			nes->rx_bytes += (uint64_t)1 << I40E_48_BIT_WIDTH;
>>> +		nes->rx_bytes += I40E_RXTX_BYTES_HIGH(vsi-
>>> old_rx_bytes);
>>> +	}
>>> +	vsi->old_rx_bytes = nes->rx_bytes;
>>
>>
>> Can you please describe this logic? (indeed better to describe it in the
>> commit log)
>>
>> 'nes->rx_bytes' is diff in the stats register since last read.
>> 'old_rx_bytes' is the previous stats diff.
>>
>> Why/how "I40E_RXTX_BYTES_LOW(vsi->old_rx_bytes) > nes->rx_bytes" has
>> a meaning? Isn't this very depends on the read frequency?
>>
>> I guess I am missing something but please help me understand.
>>
> This patch fixes the issue of rx/tx bytes counter register overflow:
> The counter register in i40e is 48-bit long, when overflow, nes->rx_bytes becomes less than old_rx_bytes, the correct value of nes->rx_bytes should be plused 1 << 48.
> Use I40E_RXTX_BYTES_HIGH() to remember the MSB, nes->rx_bytes plus the MSB is the correct value, So that using uint64_t to enlarge the 48 bit  limitation of register .
 >

My bad, 'nes->rx_bytes' is NOT diff in the stats register since last 
read, it is accumulated stats value since last reset. Above logic make 
sense now.

What do you think creating a function something like 
'i40e_stat_update_48_in_64()' and hide all the extension inside it?
I think it reduces the clutter.

> 
>> Also can you please confirm the initial value of the "vsi->offset_loaded" is
>> correct.
>>
> offset_loaded will be true when get statistics of  port and offset_loaded will be false when reset or clear the statistics,
> so if  offset_loaded is false, shouldn't to calculate the value of nes->rx_bytes, it will be 0.
> 
>> <....>
>>
>>> @@ -282,6 +282,9 @@ struct rte_flow {
>>>    #define I40E_ETH_OVERHEAD \
>>>    	(RTE_ETHER_HDR_LEN + RTE_ETHER_CRC_LEN +
>> I40E_VLAN_TAG_SIZE * 2)
>>>
>>> +#define I40E_RXTX_BYTES_HIGH(bytes) ((bytes) & ~I40E_48_BIT_MASK)
>>> +#define I40E_RXTX_BYTES_LOW(bytes) ((bytes) & I40E_48_BIT_MASK)
>>> +
>>
>> HIGH/LOW is a little misleading, for 64Bits it sounds like it is getting low 32 bits
>> and high 32 bits, can you please clarify macro masks out
>> 48/16 bits.
>>
> Yes, I will change the macro name in V3.
>>
>>>    struct i40e_adapter;
>>>    struct rte_pci_driver;
>>>
>>> @@ -399,6 +402,8 @@ struct i40e_vsi {
>>>    	uint8_t vlan_anti_spoof_on; /* The VLAN anti-spoofing enabled */
>>>    	uint8_t vlan_filter_on; /* The VLAN filter enabled */
>>>    	struct i40e_bw_info bw_info; /* VSI bandwidth information */
>>> +	uint64_t old_rx_bytes;
>>> +	uint64_t old_tx_bytes;
>>
>> 'prev' seems better naming than 'old', what do you think renaming
>> 'old_rx_bytes' -> 'prev_rx_bytes' (for all variables)?
> Yes, it's better, I will fix it in V3.
> 


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

* Re: [dpdk-dev] [dpdk-stable] [PATCH v2] net/i40e: fix incorrect byte counters
  2020-09-18  9:23       ` Igor Ryzhov
@ 2020-09-18 13:42         ` Ferruh Yigit
  2020-09-21  1:55           ` Jiang, JunyuX
  0 siblings, 1 reply; 20+ messages in thread
From: Ferruh Yigit @ 2020-09-18 13:42 UTC (permalink / raw)
  To: Igor Ryzhov, Jiang, JunyuX; +Cc: dev, Guo, Jia, Xing, Beilei, stable

On 9/18/2020 10:23 AM, Igor Ryzhov wrote:
> Hi,
> 
> Your code will work only if stats are updated at least once between two 
> overflows.
 >

In this case it will have problems in 'i40e_stat_update_48()' too.
It seems there is no way to detect if the increase in stats is N or 
MAX_48+N by the software.
And obviously there is no way to detect if the overflow occurred more 
than once.

> So it's still up to the application to handle this properly. I think it 
> should be mentioned in the docs.
> 

+1 to document.

> Igor
> 
> On Fri, Sep 18, 2020 at 6:45 AM Jiang, JunyuX <junyux.jiang@intel.com 
> <mailto:junyux.jiang@intel.com>> wrote:
> 
>     Hi Ferruh,
> 
>      > -----Original Message-----
>      > From: Ferruh Yigit <ferruh.yigit@intel.com
>     <mailto:ferruh.yigit@intel.com>>
>      > Sent: Wednesday, September 16, 2020 8:31 PM
>      > To: Jiang, JunyuX <junyux.jiang@intel.com
>     <mailto:junyux.jiang@intel.com>>; dev@dpdk.org <mailto:dev@dpdk.org>
>      > Cc: Guo, Jia <jia.guo@intel.com <mailto:jia.guo@intel.com>>;
>     Xing, Beilei <beilei.xing@intel.com <mailto:beilei.xing@intel.com>>;
>      > stable@dpdk.org <mailto:stable@dpdk.org>
>      > Subject: Re: [dpdk-stable] [PATCH v2] net/i40e: fix incorrect
>     byte counters
>      >
>      > On 9/16/2020 2:51 AM, Junyu Jiang wrote:
>      > > This patch fixed the issue that rx/tx bytes overflowed
>      >
>      > "Rx/Tx statistics counters overflowed"?
>      >
>     Yes, the rx_bytes and tx_bytes counter in X710 cards is 48-bit long,
>     if keep sending packets for a log time, the register will overflow.
> 
>      > > on 48 bit limitation by enlarging the limitation.
>      > >
>      > > Fixes: 4861cde46116 ("i40e: new poll mode driver")
>      > > Cc: stable@dpdk.org <mailto:stable@dpdk.org>
>      > >
>      > > Signed-off-by: Junyu Jiang <junyux.jiang@intel.com
>     <mailto:junyux.jiang@intel.com>>
>      > > ---
>      > >   drivers/net/i40e/i40e_ethdev.c | 47
>      > ++++++++++++++++++++++++++++++++++
>      > >   drivers/net/i40e/i40e_ethdev.h |  9 +++++++
>      > >   2 files changed, 56 insertions(+)
>      > >
>      > > diff --git a/drivers/net/i40e/i40e_ethdev.c
>      > > b/drivers/net/i40e/i40e_ethdev.c index 563f21d9d..4d4ea9861 100644
>      > > --- a/drivers/net/i40e/i40e_ethdev.c
>      > > +++ b/drivers/net/i40e/i40e_ethdev.c
>      > > @@ -3073,6 +3073,13 @@ i40e_update_vsi_stats(struct i40e_vsi *vsi)
>      > >     i40e_stat_update_48(hw, I40E_GLV_BPRCH(idx),
>      > I40E_GLV_BPRCL(idx),
>      > >                         vsi->offset_loaded, &oes->rx_broadcast,
>      > >                         &nes->rx_broadcast);
>      > > +   /* enlarge the limitation when rx_bytes overflowed */
>      > > +   if (vsi->offset_loaded) {
>      > > +           if (I40E_RXTX_BYTES_LOW(vsi->old_rx_bytes) > nes-
>      > >rx_bytes)
>      > > +                   nes->rx_bytes += (uint64_t)1 <<
>     I40E_48_BIT_WIDTH;
>      > > +           nes->rx_bytes += I40E_RXTX_BYTES_HIGH(vsi-
>      > >old_rx_bytes);
>      > > +   }
>      > > +   vsi->old_rx_bytes = nes->rx_bytes;
>      >
>      >
>      > Can you please describe this logic? (indeed better to describe it
>     in the
>      > commit log)
>      >
>      > 'nes->rx_bytes' is diff in the stats register since last read.
>      > 'old_rx_bytes' is the previous stats diff.
>      >
>      > Why/how "I40E_RXTX_BYTES_LOW(vsi->old_rx_bytes) > nes->rx_bytes" has
>      > a meaning? Isn't this very depends on the read frequency?
>      >
>      > I guess I am missing something but please help me understand.
>      >
>     This patch fixes the issue of rx/tx bytes counter register overflow:
>     The counter register in i40e is 48-bit long, when overflow,
>     nes->rx_bytes becomes less than old_rx_bytes, the correct value of
>     nes->rx_bytes should be plused 1 << 48.
>     Use I40E_RXTX_BYTES_HIGH() to remember the MSB, nes->rx_bytes plus
>     the MSB is the correct value, So that using uint64_t to enlarge the
>     48 bit  limitation of register .
> 
>      > Also can you please confirm the initial value of the
>     "vsi->offset_loaded" is
>      > correct.
>      >
>     offset_loaded will be true when get statistics of  port and
>     offset_loaded will be false when reset or clear the statistics,
>     so if  offset_loaded is false, shouldn't to calculate the value of
>     nes->rx_bytes, it will be 0.
> 
>      > <....>
>      >
>      > > @@ -282,6 +282,9 @@ struct rte_flow {
>      > >   #define I40E_ETH_OVERHEAD \
>      > >     (RTE_ETHER_HDR_LEN + RTE_ETHER_CRC_LEN +
>      > I40E_VLAN_TAG_SIZE * 2)
>      > >
>      > > +#define I40E_RXTX_BYTES_HIGH(bytes) ((bytes) & ~I40E_48_BIT_MASK)
>      > > +#define I40E_RXTX_BYTES_LOW(bytes) ((bytes) & I40E_48_BIT_MASK)
>      > > +
>      >
>      > HIGH/LOW is a little misleading, for 64Bits it sounds like it is
>     getting low 32 bits
>      > and high 32 bits, can you please clarify macro masks out
>      > 48/16 bits.
>      >
>     Yes, I will change the macro name in V3.
>      >
>      > >   struct i40e_adapter;
>      > >   struct rte_pci_driver;
>      > >
>      > > @@ -399,6 +402,8 @@ struct i40e_vsi {
>      > >     uint8_t vlan_anti_spoof_on; /* The VLAN anti-spoofing
>     enabled */
>      > >     uint8_t vlan_filter_on; /* The VLAN filter enabled */
>      > >     struct i40e_bw_info bw_info; /* VSI bandwidth information */
>      > > +   uint64_t old_rx_bytes;
>      > > +   uint64_t old_tx_bytes;
>      >
>      > 'prev' seems better naming than 'old', what do you think renaming
>      > 'old_rx_bytes' -> 'prev_rx_bytes' (for all variables)?
>     Yes, it's better, I will fix it in V3.
> 


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

* Re: [dpdk-dev] [dpdk-stable] [PATCH v2] net/i40e: fix incorrect byte counters
  2020-09-18 13:42         ` Ferruh Yigit
@ 2020-09-21  1:55           ` Jiang, JunyuX
  0 siblings, 0 replies; 20+ messages in thread
From: Jiang, JunyuX @ 2020-09-21  1:55 UTC (permalink / raw)
  To: Yigit, Ferruh, Igor Ryzhov; +Cc: dev, Guo, Jia, Xing, Beilei, stable

Hi Ferruh,

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Friday, September 18, 2020 9:42 PM
> To: Igor Ryzhov <iryzhov@nfware.com>; Jiang, JunyuX
> <junyux.jiang@intel.com>
> Cc: dev@dpdk.org; Guo, Jia <jia.guo@intel.com>; Xing, Beilei
> <beilei.xing@intel.com>; stable@dpdk.org
> Subject: Re: [dpdk-dev] [dpdk-stable] [PATCH v2] net/i40e: fix incorrect byte
> counters
> 
> On 9/18/2020 10:23 AM, Igor Ryzhov wrote:
> > Hi,
> >
> > Your code will work only if stats are updated at least once between
> > two overflows.
>  >
> 
> In this case it will have problems in 'i40e_stat_update_48()' too.
> It seems there is no way to detect if the increase in stats is N or MAX_48+N
> by the software.
> And obviously there is no way to detect if the overflow occurred more than
> once.
> 
> > So it's still up to the application to handle this properly. I think
> > it should be mentioned in the docs.
> >
> 
> +1 to document.
> 
I will fix in V3.
> > Igor
> >
> > On Fri, Sep 18, 2020 at 6:45 AM Jiang, JunyuX <junyux.jiang@intel.com
> > <mailto:junyux.jiang@intel.com>> wrote:
> >
> >     Hi Ferruh,
> >
> >      > -----Original Message-----
> >      > From: Ferruh Yigit <ferruh.yigit@intel.com
> >     <mailto:ferruh.yigit@intel.com>>
> >      > Sent: Wednesday, September 16, 2020 8:31 PM
> >      > To: Jiang, JunyuX <junyux.jiang@intel.com
> >     <mailto:junyux.jiang@intel.com>>; dev@dpdk.org
> <mailto:dev@dpdk.org>
> >      > Cc: Guo, Jia <jia.guo@intel.com <mailto:jia.guo@intel.com>>;
> >     Xing, Beilei <beilei.xing@intel.com <mailto:beilei.xing@intel.com>>;
> >      > stable@dpdk.org <mailto:stable@dpdk.org>
> >      > Subject: Re: [dpdk-stable] [PATCH v2] net/i40e: fix incorrect
> >     byte counters
> >      >
> >      > On 9/16/2020 2:51 AM, Junyu Jiang wrote:
> >      > > This patch fixed the issue that rx/tx bytes overflowed
> >      >
> >      > "Rx/Tx statistics counters overflowed"?
> >      >
> >     Yes, the rx_bytes and tx_bytes counter in X710 cards is 48-bit long,
> >     if keep sending packets for a log time, the register will overflow.
> >
> >      > > on 48 bit limitation by enlarging the limitation.
> >      > >
> >      > > Fixes: 4861cde46116 ("i40e: new poll mode driver")
> >      > > Cc: stable@dpdk.org <mailto:stable@dpdk.org>
> >      > >
> >      > > Signed-off-by: Junyu Jiang <junyux.jiang@intel.com
> >     <mailto:junyux.jiang@intel.com>>
> >      > > ---
> >      > >   drivers/net/i40e/i40e_ethdev.c | 47
> >      > ++++++++++++++++++++++++++++++++++
> >      > >   drivers/net/i40e/i40e_ethdev.h |  9 +++++++
> >      > >   2 files changed, 56 insertions(+)
> >      > >
> >      > > diff --git a/drivers/net/i40e/i40e_ethdev.c
> >      > > b/drivers/net/i40e/i40e_ethdev.c index 563f21d9d..4d4ea9861
> 100644
> >      > > --- a/drivers/net/i40e/i40e_ethdev.c
> >      > > +++ b/drivers/net/i40e/i40e_ethdev.c
> >      > > @@ -3073,6 +3073,13 @@ i40e_update_vsi_stats(struct i40e_vsi *vsi)
> >      > >     i40e_stat_update_48(hw, I40E_GLV_BPRCH(idx),
> >      > I40E_GLV_BPRCL(idx),
> >      > >                         vsi->offset_loaded, &oes->rx_broadcast,
> >      > >                         &nes->rx_broadcast);
> >      > > +   /* enlarge the limitation when rx_bytes overflowed */
> >      > > +   if (vsi->offset_loaded) {
> >      > > +           if (I40E_RXTX_BYTES_LOW(vsi->old_rx_bytes) > nes-
> >      > >rx_bytes)
> >      > > +                   nes->rx_bytes += (uint64_t)1 <<
> >     I40E_48_BIT_WIDTH;
> >      > > +           nes->rx_bytes += I40E_RXTX_BYTES_HIGH(vsi-
> >      > >old_rx_bytes);
> >      > > +   }
> >      > > +   vsi->old_rx_bytes = nes->rx_bytes;
> >      >
> >      >
> >      > Can you please describe this logic? (indeed better to describe it
> >     in the
> >      > commit log)
> >      >
> >      > 'nes->rx_bytes' is diff in the stats register since last read.
> >      > 'old_rx_bytes' is the previous stats diff.
> >      >
> >      > Why/how "I40E_RXTX_BYTES_LOW(vsi->old_rx_bytes) > nes-
> >rx_bytes" has
> >      > a meaning? Isn't this very depends on the read frequency?
> >      >
> >      > I guess I am missing something but please help me understand.
> >      >
> >     This patch fixes the issue of rx/tx bytes counter register overflow:
> >     The counter register in i40e is 48-bit long, when overflow,
> >     nes->rx_bytes becomes less than old_rx_bytes, the correct value of
> >     nes->rx_bytes should be plused 1 << 48.
> >     Use I40E_RXTX_BYTES_HIGH() to remember the MSB, nes->rx_bytes
> plus
> >     the MSB is the correct value, So that using uint64_t to enlarge the
> >     48 bit  limitation of register .
> >
> >      > Also can you please confirm the initial value of the
> >     "vsi->offset_loaded" is
> >      > correct.
> >      >
> >     offset_loaded will be true when get statistics of  port and
> >     offset_loaded will be false when reset or clear the statistics,
> >     so if  offset_loaded is false, shouldn't to calculate the value of
> >     nes->rx_bytes, it will be 0.
> >
> >      > <....>
> >      >
> >      > > @@ -282,6 +282,9 @@ struct rte_flow {
> >      > >   #define I40E_ETH_OVERHEAD \
> >      > >     (RTE_ETHER_HDR_LEN + RTE_ETHER_CRC_LEN +
> >      > I40E_VLAN_TAG_SIZE * 2)
> >      > >
> >      > > +#define I40E_RXTX_BYTES_HIGH(bytes) ((bytes) &
> ~I40E_48_BIT_MASK)
> >      > > +#define I40E_RXTX_BYTES_LOW(bytes) ((bytes) &
> I40E_48_BIT_MASK)
> >      > > +
> >      >
> >      > HIGH/LOW is a little misleading, for 64Bits it sounds like it is
> >     getting low 32 bits
> >      > and high 32 bits, can you please clarify macro masks out
> >      > 48/16 bits.
> >      >
> >     Yes, I will change the macro name in V3.
> >      >
> >      > >   struct i40e_adapter;
> >      > >   struct rte_pci_driver;
> >      > >
> >      > > @@ -399,6 +402,8 @@ struct i40e_vsi {
> >      > >     uint8_t vlan_anti_spoof_on; /* The VLAN anti-spoofing
> >     enabled */
> >      > >     uint8_t vlan_filter_on; /* The VLAN filter enabled */
> >      > >     struct i40e_bw_info bw_info; /* VSI bandwidth information */
> >      > > +   uint64_t old_rx_bytes;
> >      > > +   uint64_t old_tx_bytes;
> >      >
> >      > 'prev' seems better naming than 'old', what do you think renaming
> >      > 'old_rx_bytes' -> 'prev_rx_bytes' (for all variables)?
> >     Yes, it's better, I will fix it in V3.
> >


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

* [dpdk-dev] [PATCH v3] net/i40e: fix incorrect byte counters
  2020-09-10  1:54 [dpdk-dev] [PATCH] net/i40e: fix incorrect byte counters Junyu Jiang
  2020-09-10  2:18 ` [dpdk-dev] [dpdk-stable] " Han, YingyaX
  2020-09-16  1:51 ` [dpdk-dev] [PATCH v2] " Junyu Jiang
@ 2020-09-21  9:59 ` Junyu Jiang
  2020-09-21 11:41   ` Ferruh Yigit
  2020-09-22  7:37 ` [dpdk-dev] [PATCH v4] " Junyu Jiang
  2020-09-22  9:19 ` [dpdk-dev] [PATCH v5] " Junyu Jiang
  4 siblings, 1 reply; 20+ messages in thread
From: Junyu Jiang @ 2020-09-21  9:59 UTC (permalink / raw)
  To: dev; +Cc: Jeff Guo, Beilei Xing, Ferruh Yigit, Junyu Jiang, stable

This patch fixed the issue that rx/tx bytes statistics counters
overflowed on 48 bit limitation by enlarging the limitation.

Fixes: 4861cde46116 ("i40e: new poll mode driver")
Cc: stable@dpdk.org

Signed-off-by: Junyu Jiang <junyux.jiang@intel.com>
---
 doc/guides/nics/i40e.rst       |  7 +++++++
 drivers/net/i40e/i40e_ethdev.c | 32 ++++++++++++++++++++++++++++++++
 drivers/net/i40e/i40e_ethdev.h |  9 +++++++++
 3 files changed, 48 insertions(+)

diff --git a/doc/guides/nics/i40e.rst b/doc/guides/nics/i40e.rst
index b7430f6c4..4baa58be6 100644
--- a/doc/guides/nics/i40e.rst
+++ b/doc/guides/nics/i40e.rst
@@ -830,3 +830,10 @@ Tx bytes affected by the link status change
 
 For firmware versions prior to 6.01 for X710 series and 3.33 for X722 series, the tx_bytes statistics data is affected by
 the link down event. Each time the link status changes to down, the tx_bytes decreases 110 bytes.
+
+RX/TX statistics may be incorrect when register overflowed
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+The rx_bytes/tx_bytes statistics register is 48 bit length. Although this limitation is enlarged to 64 bit length
+on the software side, but there is no way to detect if the overflow occurred more than once. So rx_bytes/tx_bytes
+statistics data is correct when statistics are updated at least once between two overflows.
diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 563f21d9d..212338ef0 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -3052,6 +3052,19 @@ i40e_dev_link_update(struct rte_eth_dev *dev,
 	return ret;
 }
 
+static void
+i40e_stat_update_48_in_64(uint64_t *new_bytes,
+			  uint64_t *prev_bytes,
+			  bool offset_loaded)
+{
+	if (offset_loaded) {
+		if (I40E_RXTX_BYTES_L_48_BIT(*prev_bytes) > *new_bytes)
+			*new_bytes += (uint64_t)1 << I40E_48_BIT_WIDTH;
+		*new_bytes += I40E_RXTX_BYTES_H_16_BIT(*prev_bytes);
+	}
+	*prev_bytes = *new_bytes;
+}
+
 /* Get all the statistics of a VSI */
 void
 i40e_update_vsi_stats(struct i40e_vsi *vsi)
@@ -3073,6 +3086,9 @@ i40e_update_vsi_stats(struct i40e_vsi *vsi)
 	i40e_stat_update_48(hw, I40E_GLV_BPRCH(idx), I40E_GLV_BPRCL(idx),
 			    vsi->offset_loaded, &oes->rx_broadcast,
 			    &nes->rx_broadcast);
+	/* enlarge the limitation when rx_bytes overflowed */
+	i40e_stat_update_48_in_64(&nes->rx_bytes, &vsi->prev_rx_bytes,
+				  vsi->offset_loaded);
 	/* exclude CRC bytes */
 	nes->rx_bytes -= (nes->rx_unicast + nes->rx_multicast +
 		nes->rx_broadcast) * RTE_ETHER_CRC_LEN;
@@ -3099,6 +3115,9 @@ i40e_update_vsi_stats(struct i40e_vsi *vsi)
 	/* GLV_TDPC not supported */
 	i40e_stat_update_32(hw, I40E_GLV_TEPC(idx), vsi->offset_loaded,
 			    &oes->tx_errors, &nes->tx_errors);
+	/* enlarge the limitation when tx_bytes overflowed */
+	i40e_stat_update_48_in_64(&nes->tx_bytes, &vsi->prev_tx_bytes,
+				  vsi->offset_loaded);
 	vsi->offset_loaded = true;
 
 	PMD_DRV_LOG(DEBUG, "***************** VSI[%u] stats start *******************",
@@ -3171,6 +3190,13 @@ i40e_read_stats_registers(struct i40e_pf *pf, struct i40e_hw *hw)
 			    pf->offset_loaded,
 			    &pf->internal_stats_offset.tx_broadcast,
 			    &pf->internal_stats.tx_broadcast);
+	/* enlarge the limitation when internal rx/tx bytes overflowed */
+	i40e_stat_update_48_in_64(&pf->internal_stats.rx_bytes,
+				  &pf->internal_prev_rx_bytes,
+				  pf->offset_loaded);
+	i40e_stat_update_48_in_64(&pf->internal_stats.tx_bytes,
+				  &pf->internal_prev_tx_bytes,
+				  pf->offset_loaded);
 
 	/* exclude CRC size */
 	pf->internal_stats.rx_bytes -= (pf->internal_stats.rx_unicast +
@@ -3194,6 +3220,9 @@ 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);
+	/* enlarge the limitation when rx_bytes overflowed */
+	i40e_stat_update_48_in_64(&ns->eth.rx_bytes, &pf->prev_rx_bytes,
+				  pf->offset_loaded);
 	/* Workaround: CRC size should not be included in byte statistics,
 	 * so subtract RTE_ETHER_CRC_LEN from the byte counter for each rx
 	 * packet.
@@ -3252,6 +3281,9 @@ 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);
+	/* enlarge the limitation when tx_bytes overflowed */
+	i40e_stat_update_48_in_64(&ns->eth.tx_bytes, &pf->prev_tx_bytes,
+				  pf->offset_loaded);
 	ns->eth.tx_bytes -= (ns->eth.tx_unicast + ns->eth.tx_multicast +
 		ns->eth.tx_broadcast) * RTE_ETHER_CRC_LEN;
 
diff --git a/drivers/net/i40e/i40e_ethdev.h b/drivers/net/i40e/i40e_ethdev.h
index 19f821829..1466998aa 100644
--- a/drivers/net/i40e/i40e_ethdev.h
+++ b/drivers/net/i40e/i40e_ethdev.h
@@ -282,6 +282,9 @@ struct rte_flow {
 #define I40E_ETH_OVERHEAD \
 	(RTE_ETHER_HDR_LEN + RTE_ETHER_CRC_LEN + I40E_VLAN_TAG_SIZE * 2)
 
+#define I40E_RXTX_BYTES_H_16_BIT(bytes) ((bytes) & ~I40E_48_BIT_MASK)
+#define I40E_RXTX_BYTES_L_48_BIT(bytes) ((bytes) & I40E_48_BIT_MASK)
+
 struct i40e_adapter;
 struct rte_pci_driver;
 
@@ -399,6 +402,8 @@ struct i40e_vsi {
 	uint8_t vlan_anti_spoof_on; /* The VLAN anti-spoofing enabled */
 	uint8_t vlan_filter_on; /* The VLAN filter enabled */
 	struct i40e_bw_info bw_info; /* VSI bandwidth information */
+	uint64_t prev_rx_bytes;
+	uint64_t prev_tx_bytes;
 };
 
 struct pool_entry {
@@ -1156,6 +1161,10 @@ struct i40e_pf {
 	uint16_t switch_domain_id;
 
 	struct i40e_vf_msg_cfg vf_msg_cfg;
+	uint64_t prev_rx_bytes;
+	uint64_t prev_tx_bytes;
+	uint64_t internal_prev_rx_bytes;
+	uint64_t internal_prev_tx_bytes;
 };
 
 enum pending_msg {
-- 
2.17.1


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

* Re: [dpdk-dev] [PATCH v3] net/i40e: fix incorrect byte counters
  2020-09-21  9:59 ` [dpdk-dev] [PATCH v3] " Junyu Jiang
@ 2020-09-21 11:41   ` Ferruh Yigit
  0 siblings, 0 replies; 20+ messages in thread
From: Ferruh Yigit @ 2020-09-21 11:41 UTC (permalink / raw)
  To: Junyu Jiang, dev; +Cc: Jeff Guo, Beilei Xing, stable

On 9/21/2020 10:59 AM, Junyu Jiang wrote:
> This patch fixed the issue that rx/tx bytes statistics counters
> overflowed on 48 bit limitation by enlarging the limitation.
> 
> Fixes: 4861cde46116 ("i40e: new poll mode driver")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Junyu Jiang <junyux.jiang@intel.com>
> ---
>   doc/guides/nics/i40e.rst       |  7 +++++++
>   drivers/net/i40e/i40e_ethdev.c | 32 ++++++++++++++++++++++++++++++++
>   drivers/net/i40e/i40e_ethdev.h |  9 +++++++++
>   3 files changed, 48 insertions(+)
> 
> diff --git a/doc/guides/nics/i40e.rst b/doc/guides/nics/i40e.rst
> index b7430f6c4..4baa58be6 100644
> --- a/doc/guides/nics/i40e.rst
> +++ b/doc/guides/nics/i40e.rst
> @@ -830,3 +830,10 @@ Tx bytes affected by the link status change
>   
>   For firmware versions prior to 6.01 for X710 series and 3.33 for X722 series, the tx_bytes statistics data is affected by
>   the link down event. Each time the link status changes to down, the tx_bytes decreases 110 bytes.
> +
> +RX/TX statistics may be incorrect when register overflowed
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +The rx_bytes/tx_bytes statistics register is 48 bit length. Although this limitation is enlarged to 64 bit length
> +on the software side, but there is no way to detect if the overflow occurred more than once. So rx_bytes/tx_bytes
> +statistics data is correct when statistics are updated at least once between two overflows.
> diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
> index 563f21d9d..212338ef0 100644
> --- a/drivers/net/i40e/i40e_ethdev.c
> +++ b/drivers/net/i40e/i40e_ethdev.c
> @@ -3052,6 +3052,19 @@ i40e_dev_link_update(struct rte_eth_dev *dev,
>   	return ret;
>   }
>   
> +static void
> +i40e_stat_update_48_in_64(uint64_t *new_bytes,
> +			  uint64_t *prev_bytes,
> +			  bool offset_loaded)
> +{
> +	if (offset_loaded) {
> +		if (I40E_RXTX_BYTES_L_48_BIT(*prev_bytes) > *new_bytes)
> +			*new_bytes += (uint64_t)1 << I40E_48_BIT_WIDTH;
> +		*new_bytes += I40E_RXTX_BYTES_H_16_BIT(*prev_bytes);
> +	}
> +	*prev_bytes = *new_bytes;
> +}
> +

I was more thinking reading stats and extending in same function, 
instead of extracting the extending part into its own function, like:

static void
i40e_stat_update_48_in_64(struct i40e_hw *hw,
		uint32_t hireg,
		uint32_t loreg,
		bool offset_loaded,
		uint64_t *offset,
		uint64_t *stat,
		uint64_t *prev_bytes) {

	i40e_stat_update_48(...)

	/* logic to convert 'stat' to 64 bits */
}

Does it make sense?

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

* [dpdk-dev] [PATCH v4] net/i40e: fix incorrect byte counters
  2020-09-10  1:54 [dpdk-dev] [PATCH] net/i40e: fix incorrect byte counters Junyu Jiang
                   ` (2 preceding siblings ...)
  2020-09-21  9:59 ` [dpdk-dev] [PATCH v3] " Junyu Jiang
@ 2020-09-22  7:37 ` Junyu Jiang
  2020-09-22  9:06   ` Ferruh Yigit
  2020-09-22  9:19 ` [dpdk-dev] [PATCH v5] " Junyu Jiang
  4 siblings, 1 reply; 20+ messages in thread
From: Junyu Jiang @ 2020-09-22  7:37 UTC (permalink / raw)
  To: dev; +Cc: Jeff Guo, Beilei Xing, Ferruh Yigit, Junyu Jiang, stable

This patch fixed the issue that rx/tx bytes statistics counters
overflowed on 48 bit limitation by enlarging the limitation.

Fixes: 4861cde46116 ("i40e: new poll mode driver")
Cc: stable@dpdk.org

Signed-off-by: Junyu Jiang <junyux.jiang@intel.com>
---
v4: put reading stats and extending in same function.
v3: create a function to hide the extension inside it.
v2: modify the error code
---
---
 doc/guides/nics/i40e.rst       |  7 ++++
 drivers/net/i40e/i40e_ethdev.c | 66 +++++++++++++++++++++-------------
 drivers/net/i40e/i40e_ethdev.h |  9 +++++
 3 files changed, 57 insertions(+), 25 deletions(-)

diff --git a/doc/guides/nics/i40e.rst b/doc/guides/nics/i40e.rst
index b7430f6c4..4baa58be6 100644
--- a/doc/guides/nics/i40e.rst
+++ b/doc/guides/nics/i40e.rst
@@ -830,3 +830,10 @@ Tx bytes affected by the link status change
 
 For firmware versions prior to 6.01 for X710 series and 3.33 for X722 series, the tx_bytes statistics data is affected by
 the link down event. Each time the link status changes to down, the tx_bytes decreases 110 bytes.
+
+RX/TX statistics may be incorrect when register overflowed
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+The rx_bytes/tx_bytes statistics register is 48 bit length. Although this limitation is enlarged to 64 bit length
+on the software side, but there is no way to detect if the overflow occurred more than once. So rx_bytes/tx_bytes
+statistics data is correct when statistics are updated at least once between two overflows.
diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 563f21d9d..6439baf2f 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -3052,6 +3052,21 @@ i40e_dev_link_update(struct rte_eth_dev *dev,
 	return ret;
 }
 
+static void
+i40e_stat_update_48_in_64(struct i40e_hw *hw, uint32_t hireg,
+			  uint32_t loreg, bool offset_loaded, uint64_t *offset,
+			  uint64_t *stat, uint64_t *prev_stat)
+{
+	i40e_stat_update_48(hw, hireg, loreg, offset_loaded, offset, stat);
+	/* enlarge the limitation when statistics counters overflowed */
+	if (offset_loaded) {
+		if (I40E_RXTX_BYTES_L_48_BIT(*prev_stat) > *stat)
+			*stat += (uint64_t)1 << I40E_48_BIT_WIDTH;
+		*stat += I40E_RXTX_BYTES_H_16_BIT(*prev_stat);
+	}
+	*prev_stat = *stat;
+}
+
 /* Get all the statistics of a VSI */
 void
 i40e_update_vsi_stats(struct i40e_vsi *vsi)
@@ -3061,9 +3076,9 @@ i40e_update_vsi_stats(struct i40e_vsi *vsi)
 	struct i40e_hw *hw = I40E_VSI_TO_HW(vsi);
 	int idx = rte_le_to_cpu_16(vsi->info.stat_counter_idx);
 
-	i40e_stat_update_48(hw, I40E_GLV_GORCH(idx), I40E_GLV_GORCL(idx),
-			    vsi->offset_loaded, &oes->rx_bytes,
-			    &nes->rx_bytes);
+	i40e_stat_update_48_in_64(hw, I40E_GLV_GORCH(idx), I40E_GLV_GORCL(idx),
+				  vsi->offset_loaded, &oes->rx_bytes,
+				  &nes->rx_bytes, &vsi->prev_rx_bytes);
 	i40e_stat_update_48(hw, I40E_GLV_UPRCH(idx), I40E_GLV_UPRCL(idx),
 			    vsi->offset_loaded, &oes->rx_unicast,
 			    &nes->rx_unicast);
@@ -3084,9 +3099,9 @@ i40e_update_vsi_stats(struct i40e_vsi *vsi)
 	i40e_stat_update_32(hw, I40E_GLV_RUPP(idx), vsi->offset_loaded,
 			    &oes->rx_unknown_protocol,
 			    &nes->rx_unknown_protocol);
-	i40e_stat_update_48(hw, I40E_GLV_GOTCH(idx), I40E_GLV_GOTCL(idx),
-			    vsi->offset_loaded, &oes->tx_bytes,
-			    &nes->tx_bytes);
+	i40e_stat_update_48_in_64(hw, I40E_GLV_GOTCH(idx), I40E_GLV_GOTCL(idx),
+				  vsi->offset_loaded, &oes->tx_bytes,
+				  &nes->tx_bytes, &vsi->prev_tx_bytes);
 	i40e_stat_update_48(hw, I40E_GLV_UPTCH(idx), I40E_GLV_UPTCL(idx),
 			    vsi->offset_loaded, &oes->tx_unicast,
 			    &nes->tx_unicast);
@@ -3128,17 +3143,18 @@ i40e_read_stats_registers(struct i40e_pf *pf, struct i40e_hw *hw)
 	struct i40e_hw_port_stats *os = &pf->stats_offset; /* old stats */
 
 	/* Get rx/tx bytes of internal transfer packets */
-	i40e_stat_update_48(hw, I40E_GLV_GORCH(hw->port),
-			I40E_GLV_GORCL(hw->port),
-			pf->offset_loaded,
-			&pf->internal_stats_offset.rx_bytes,
-			&pf->internal_stats.rx_bytes);
-
-	i40e_stat_update_48(hw, I40E_GLV_GOTCH(hw->port),
-			I40E_GLV_GOTCL(hw->port),
-			pf->offset_loaded,
-			&pf->internal_stats_offset.tx_bytes,
-			&pf->internal_stats.tx_bytes);
+	i40e_stat_update_48_in_64(hw, I40E_GLV_GORCH(hw->port),
+				  I40E_GLV_GORCL(hw->port),
+				  pf->offset_loaded,
+				  &pf->internal_stats_offset.rx_bytes,
+				  &pf->internal_stats.rx_bytes,
+				  &pf->internal_prev_rx_bytes);
+	i40e_stat_update_48_in_64(hw, I40E_GLV_GOTCH(hw->port),
+				  I40E_GLV_GOTCL(hw->port),
+				  pf->offset_loaded,
+				  &pf->internal_stats_offset.tx_bytes,
+				  &pf->internal_stats.tx_bytes,
+				  &pf->internal_prev_tx_bytes);
 	/* Get total internal rx packet count */
 	i40e_stat_update_48(hw, I40E_GLV_UPRCH(hw->port),
 			    I40E_GLV_UPRCL(hw->port),
@@ -3178,10 +3194,10 @@ i40e_read_stats_registers(struct i40e_pf *pf, struct i40e_hw *hw)
 		pf->internal_stats.rx_broadcast) * RTE_ETHER_CRC_LEN;
 
 	/* 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);
+	i40e_stat_update_48_in_64(hw, I40E_GLPRT_GORCH(hw->port),
+				  I40E_GLPRT_GORCL(hw->port),
+				  pf->offset_loaded, &os->eth.rx_bytes,
+				  &ns->eth.rx_bytes, &pf->prev_rx_bytes);
 	i40e_stat_update_48(hw, I40E_GLPRT_UPRCH(hw->port),
 			    I40E_GLPRT_UPRCL(hw->port),
 			    pf->offset_loaded, &os->eth.rx_unicast,
@@ -3236,10 +3252,10 @@ 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_in_64(hw, I40E_GLPRT_GOTCH(hw->port),
+				  I40E_GLPRT_GOTCL(hw->port),
+				  pf->offset_loaded, &os->eth.tx_bytes,
+				  &ns->eth.tx_bytes, &pf->prev_tx_bytes);
 	i40e_stat_update_48(hw, I40E_GLPRT_UPTCH(hw->port),
 			    I40E_GLPRT_UPTCL(hw->port),
 			    pf->offset_loaded, &os->eth.tx_unicast,
diff --git a/drivers/net/i40e/i40e_ethdev.h b/drivers/net/i40e/i40e_ethdev.h
index 19f821829..1466998aa 100644
--- a/drivers/net/i40e/i40e_ethdev.h
+++ b/drivers/net/i40e/i40e_ethdev.h
@@ -282,6 +282,9 @@ struct rte_flow {
 #define I40E_ETH_OVERHEAD \
 	(RTE_ETHER_HDR_LEN + RTE_ETHER_CRC_LEN + I40E_VLAN_TAG_SIZE * 2)
 
+#define I40E_RXTX_BYTES_H_16_BIT(bytes) ((bytes) & ~I40E_48_BIT_MASK)
+#define I40E_RXTX_BYTES_L_48_BIT(bytes) ((bytes) & I40E_48_BIT_MASK)
+
 struct i40e_adapter;
 struct rte_pci_driver;
 
@@ -399,6 +402,8 @@ struct i40e_vsi {
 	uint8_t vlan_anti_spoof_on; /* The VLAN anti-spoofing enabled */
 	uint8_t vlan_filter_on; /* The VLAN filter enabled */
 	struct i40e_bw_info bw_info; /* VSI bandwidth information */
+	uint64_t prev_rx_bytes;
+	uint64_t prev_tx_bytes;
 };
 
 struct pool_entry {
@@ -1156,6 +1161,10 @@ struct i40e_pf {
 	uint16_t switch_domain_id;
 
 	struct i40e_vf_msg_cfg vf_msg_cfg;
+	uint64_t prev_rx_bytes;
+	uint64_t prev_tx_bytes;
+	uint64_t internal_prev_rx_bytes;
+	uint64_t internal_prev_tx_bytes;
 };
 
 enum pending_msg {
-- 
2.17.1


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

* Re: [dpdk-dev] [PATCH v4] net/i40e: fix incorrect byte counters
  2020-09-22  7:37 ` [dpdk-dev] [PATCH v4] " Junyu Jiang
@ 2020-09-22  9:06   ` Ferruh Yigit
  0 siblings, 0 replies; 20+ messages in thread
From: Ferruh Yigit @ 2020-09-22  9:06 UTC (permalink / raw)
  To: Junyu Jiang, dev; +Cc: Jeff Guo, Beilei Xing, stable

On 9/22/2020 8:37 AM, Junyu Jiang wrote:
> This patch fixed the issue that rx/tx bytes statistics counters
> overflowed on 48 bit limitation by enlarging the limitation.
> 
> Fixes: 4861cde46116 ("i40e: new poll mode driver")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Junyu Jiang <junyux.jiang@intel.com>
> ---
> v4: put reading stats and extending in same function.
> v3: create a function to hide the extension inside it.
> v2: modify the error code
> ---
> ---
>   doc/guides/nics/i40e.rst       |  7 ++++
>   drivers/net/i40e/i40e_ethdev.c | 66 +++++++++++++++++++++-------------
>   drivers/net/i40e/i40e_ethdev.h |  9 +++++
>   3 files changed, 57 insertions(+), 25 deletions(-)
> 
> diff --git a/doc/guides/nics/i40e.rst b/doc/guides/nics/i40e.rst
> index b7430f6c4..4baa58be6 100644
> --- a/doc/guides/nics/i40e.rst
> +++ b/doc/guides/nics/i40e.rst
> @@ -830,3 +830,10 @@ Tx bytes affected by the link status change
>   
>   For firmware versions prior to 6.01 for X710 series and 3.33 for X722 series, the tx_bytes statistics data is affected by
>   the link down event. Each time the link status changes to down, the tx_bytes decreases 110 bytes.
> +
> +RX/TX statistics may be incorrect when register overflowed
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +The rx_bytes/tx_bytes statistics register is 48 bit length. Although this limitation is enlarged to 64 bit length
> +on the software side, but there is no way to detect if the overflow occurred more than once. So rx_bytes/tx_bytes
> +statistics data is correct when statistics are updated at least once between two overflows.

It can be better to move this block next to other know issue related to 
the stats (Incorrect Rx statistics when packet is oversize), apart from 
that:

Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

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

* [dpdk-dev] [PATCH v5] net/i40e: fix incorrect byte counters
  2020-09-10  1:54 [dpdk-dev] [PATCH] net/i40e: fix incorrect byte counters Junyu Jiang
                   ` (3 preceding siblings ...)
  2020-09-22  7:37 ` [dpdk-dev] [PATCH v4] " Junyu Jiang
@ 2020-09-22  9:19 ` Junyu Jiang
  2020-09-22 12:59   ` Zhang, Qi Z
  4 siblings, 1 reply; 20+ messages in thread
From: Junyu Jiang @ 2020-09-22  9:19 UTC (permalink / raw)
  To: dev; +Cc: Jeff Guo, Beilei Xing, Ferruh Yigit, Junyu Jiang, stable

This patch fixed the issue that rx/tx bytes statistics counters
overflowed on 48 bit limitation by enlarging the limitation.

Fixes: 4861cde46116 ("i40e: new poll mode driver")
Cc: stable@dpdk.org

Signed-off-by: Junyu Jiang <junyux.jiang@intel.com>
Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
V5: move this known issue next to related to the stats
v4: put reading stats and extending in same function.
v3: create a function to hide the extension inside it.
v2: modify the error code
---
---
 doc/guides/nics/i40e.rst       |  9 +++++
 drivers/net/i40e/i40e_ethdev.c | 66 +++++++++++++++++++++-------------
 drivers/net/i40e/i40e_ethdev.h |  9 +++++
 3 files changed, 59 insertions(+), 25 deletions(-)

diff --git a/doc/guides/nics/i40e.rst b/doc/guides/nics/i40e.rst
index b7430f6c4..a0b81e669 100644
--- a/doc/guides/nics/i40e.rst
+++ b/doc/guides/nics/i40e.rst
@@ -670,6 +670,15 @@ When a packet is over maximum frame size, the packet is dropped.
 However, the Rx statistics, when calling `rte_eth_stats_get` incorrectly
 shows it as received.
 
+RX/TX statistics may be incorrect when register overflowed
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+The rx_bytes/tx_bytes statistics register is 48 bit length.
+Although this limitation is enlarged to 64 bit length on the software side,
+but there is no way to detect if the overflow occurred more than once.
+So rx_bytes/tx_bytes statistics data is correct when statistics are
+updated at least once between two overflows.
+
 VF & TC max bandwidth setting
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 563f21d9d..6439baf2f 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -3052,6 +3052,21 @@ i40e_dev_link_update(struct rte_eth_dev *dev,
 	return ret;
 }
 
+static void
+i40e_stat_update_48_in_64(struct i40e_hw *hw, uint32_t hireg,
+			  uint32_t loreg, bool offset_loaded, uint64_t *offset,
+			  uint64_t *stat, uint64_t *prev_stat)
+{
+	i40e_stat_update_48(hw, hireg, loreg, offset_loaded, offset, stat);
+	/* enlarge the limitation when statistics counters overflowed */
+	if (offset_loaded) {
+		if (I40E_RXTX_BYTES_L_48_BIT(*prev_stat) > *stat)
+			*stat += (uint64_t)1 << I40E_48_BIT_WIDTH;
+		*stat += I40E_RXTX_BYTES_H_16_BIT(*prev_stat);
+	}
+	*prev_stat = *stat;
+}
+
 /* Get all the statistics of a VSI */
 void
 i40e_update_vsi_stats(struct i40e_vsi *vsi)
@@ -3061,9 +3076,9 @@ i40e_update_vsi_stats(struct i40e_vsi *vsi)
 	struct i40e_hw *hw = I40E_VSI_TO_HW(vsi);
 	int idx = rte_le_to_cpu_16(vsi->info.stat_counter_idx);
 
-	i40e_stat_update_48(hw, I40E_GLV_GORCH(idx), I40E_GLV_GORCL(idx),
-			    vsi->offset_loaded, &oes->rx_bytes,
-			    &nes->rx_bytes);
+	i40e_stat_update_48_in_64(hw, I40E_GLV_GORCH(idx), I40E_GLV_GORCL(idx),
+				  vsi->offset_loaded, &oes->rx_bytes,
+				  &nes->rx_bytes, &vsi->prev_rx_bytes);
 	i40e_stat_update_48(hw, I40E_GLV_UPRCH(idx), I40E_GLV_UPRCL(idx),
 			    vsi->offset_loaded, &oes->rx_unicast,
 			    &nes->rx_unicast);
@@ -3084,9 +3099,9 @@ i40e_update_vsi_stats(struct i40e_vsi *vsi)
 	i40e_stat_update_32(hw, I40E_GLV_RUPP(idx), vsi->offset_loaded,
 			    &oes->rx_unknown_protocol,
 			    &nes->rx_unknown_protocol);
-	i40e_stat_update_48(hw, I40E_GLV_GOTCH(idx), I40E_GLV_GOTCL(idx),
-			    vsi->offset_loaded, &oes->tx_bytes,
-			    &nes->tx_bytes);
+	i40e_stat_update_48_in_64(hw, I40E_GLV_GOTCH(idx), I40E_GLV_GOTCL(idx),
+				  vsi->offset_loaded, &oes->tx_bytes,
+				  &nes->tx_bytes, &vsi->prev_tx_bytes);
 	i40e_stat_update_48(hw, I40E_GLV_UPTCH(idx), I40E_GLV_UPTCL(idx),
 			    vsi->offset_loaded, &oes->tx_unicast,
 			    &nes->tx_unicast);
@@ -3128,17 +3143,18 @@ i40e_read_stats_registers(struct i40e_pf *pf, struct i40e_hw *hw)
 	struct i40e_hw_port_stats *os = &pf->stats_offset; /* old stats */
 
 	/* Get rx/tx bytes of internal transfer packets */
-	i40e_stat_update_48(hw, I40E_GLV_GORCH(hw->port),
-			I40E_GLV_GORCL(hw->port),
-			pf->offset_loaded,
-			&pf->internal_stats_offset.rx_bytes,
-			&pf->internal_stats.rx_bytes);
-
-	i40e_stat_update_48(hw, I40E_GLV_GOTCH(hw->port),
-			I40E_GLV_GOTCL(hw->port),
-			pf->offset_loaded,
-			&pf->internal_stats_offset.tx_bytes,
-			&pf->internal_stats.tx_bytes);
+	i40e_stat_update_48_in_64(hw, I40E_GLV_GORCH(hw->port),
+				  I40E_GLV_GORCL(hw->port),
+				  pf->offset_loaded,
+				  &pf->internal_stats_offset.rx_bytes,
+				  &pf->internal_stats.rx_bytes,
+				  &pf->internal_prev_rx_bytes);
+	i40e_stat_update_48_in_64(hw, I40E_GLV_GOTCH(hw->port),
+				  I40E_GLV_GOTCL(hw->port),
+				  pf->offset_loaded,
+				  &pf->internal_stats_offset.tx_bytes,
+				  &pf->internal_stats.tx_bytes,
+				  &pf->internal_prev_tx_bytes);
 	/* Get total internal rx packet count */
 	i40e_stat_update_48(hw, I40E_GLV_UPRCH(hw->port),
 			    I40E_GLV_UPRCL(hw->port),
@@ -3178,10 +3194,10 @@ i40e_read_stats_registers(struct i40e_pf *pf, struct i40e_hw *hw)
 		pf->internal_stats.rx_broadcast) * RTE_ETHER_CRC_LEN;
 
 	/* 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);
+	i40e_stat_update_48_in_64(hw, I40E_GLPRT_GORCH(hw->port),
+				  I40E_GLPRT_GORCL(hw->port),
+				  pf->offset_loaded, &os->eth.rx_bytes,
+				  &ns->eth.rx_bytes, &pf->prev_rx_bytes);
 	i40e_stat_update_48(hw, I40E_GLPRT_UPRCH(hw->port),
 			    I40E_GLPRT_UPRCL(hw->port),
 			    pf->offset_loaded, &os->eth.rx_unicast,
@@ -3236,10 +3252,10 @@ 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_in_64(hw, I40E_GLPRT_GOTCH(hw->port),
+				  I40E_GLPRT_GOTCL(hw->port),
+				  pf->offset_loaded, &os->eth.tx_bytes,
+				  &ns->eth.tx_bytes, &pf->prev_tx_bytes);
 	i40e_stat_update_48(hw, I40E_GLPRT_UPTCH(hw->port),
 			    I40E_GLPRT_UPTCL(hw->port),
 			    pf->offset_loaded, &os->eth.tx_unicast,
diff --git a/drivers/net/i40e/i40e_ethdev.h b/drivers/net/i40e/i40e_ethdev.h
index 19f821829..1466998aa 100644
--- a/drivers/net/i40e/i40e_ethdev.h
+++ b/drivers/net/i40e/i40e_ethdev.h
@@ -282,6 +282,9 @@ struct rte_flow {
 #define I40E_ETH_OVERHEAD \
 	(RTE_ETHER_HDR_LEN + RTE_ETHER_CRC_LEN + I40E_VLAN_TAG_SIZE * 2)
 
+#define I40E_RXTX_BYTES_H_16_BIT(bytes) ((bytes) & ~I40E_48_BIT_MASK)
+#define I40E_RXTX_BYTES_L_48_BIT(bytes) ((bytes) & I40E_48_BIT_MASK)
+
 struct i40e_adapter;
 struct rte_pci_driver;
 
@@ -399,6 +402,8 @@ struct i40e_vsi {
 	uint8_t vlan_anti_spoof_on; /* The VLAN anti-spoofing enabled */
 	uint8_t vlan_filter_on; /* The VLAN filter enabled */
 	struct i40e_bw_info bw_info; /* VSI bandwidth information */
+	uint64_t prev_rx_bytes;
+	uint64_t prev_tx_bytes;
 };
 
 struct pool_entry {
@@ -1156,6 +1161,10 @@ struct i40e_pf {
 	uint16_t switch_domain_id;
 
 	struct i40e_vf_msg_cfg vf_msg_cfg;
+	uint64_t prev_rx_bytes;
+	uint64_t prev_tx_bytes;
+	uint64_t internal_prev_rx_bytes;
+	uint64_t internal_prev_tx_bytes;
 };
 
 enum pending_msg {
-- 
2.17.1


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

* Re: [dpdk-dev] [PATCH v5] net/i40e: fix incorrect byte counters
  2020-09-22  9:19 ` [dpdk-dev] [PATCH v5] " Junyu Jiang
@ 2020-09-22 12:59   ` Zhang, Qi Z
  0 siblings, 0 replies; 20+ messages in thread
From: Zhang, Qi Z @ 2020-09-22 12:59 UTC (permalink / raw)
  To: Jiang, JunyuX, dev
  Cc: Guo, Jia, Xing, Beilei, Yigit, Ferruh, Jiang, JunyuX, stable



> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Junyu Jiang
> Sent: Tuesday, September 22, 2020 5:20 PM
> To: dev@dpdk.org
> Cc: Guo, Jia <jia.guo@intel.com>; Xing, Beilei <beilei.xing@intel.com>; Yigit,
> Ferruh <ferruh.yigit@intel.com>; Jiang, JunyuX <junyux.jiang@intel.com>;
> stable@dpdk.org
> Subject: [dpdk-dev] [PATCH v5] net/i40e: fix incorrect byte counters
> 
> This patch fixed the issue that rx/tx bytes statistics counters overflowed on 48
> bit limitation by enlarging the limitation.
> 
> Fixes: 4861cde46116 ("i40e: new poll mode driver")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Junyu Jiang <junyux.jiang@intel.com>
> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

Applied to dpdk-next-net-intel.

Thanks
Qi

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

end of thread, other threads:[~2020-09-22 12:59 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-10  1:54 [dpdk-dev] [PATCH] net/i40e: fix incorrect byte counters Junyu Jiang
2020-09-10  2:18 ` [dpdk-dev] [dpdk-stable] " Han, YingyaX
2020-09-10  5:58   ` Guo, Jia
2020-09-10  6:45     ` Jiang, JunyuX
2020-09-16  1:51 ` [dpdk-dev] [PATCH v2] " Junyu Jiang
2020-09-16  2:39   ` [dpdk-dev] [dpdk-stable] " Han, YingyaX
2020-09-16  5:21   ` [dpdk-dev] " Guo, Jia
2020-09-16  5:50     ` Zhang, Qi Z
2020-09-16 12:30   ` [dpdk-dev] [dpdk-stable] " Ferruh Yigit
2020-09-18  3:44     ` Jiang, JunyuX
2020-09-18  9:23       ` Igor Ryzhov
2020-09-18 13:42         ` Ferruh Yigit
2020-09-21  1:55           ` Jiang, JunyuX
2020-09-18 13:37       ` Ferruh Yigit
2020-09-21  9:59 ` [dpdk-dev] [PATCH v3] " Junyu Jiang
2020-09-21 11:41   ` Ferruh Yigit
2020-09-22  7:37 ` [dpdk-dev] [PATCH v4] " Junyu Jiang
2020-09-22  9:06   ` Ferruh Yigit
2020-09-22  9:19 ` [dpdk-dev] [PATCH v5] " Junyu Jiang
2020-09-22 12:59   ` Zhang, Qi Z

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