net/i40e: fix incorrect byte counters

PATCH Superseded Junyu Jiang 6 participants 20 messages Sep 10, 2020 – Sep 22, 2020 Network drivers

lore

Share & tools

lore

Junyu Jiang

author

2020-09-10 02:03 UTC #0 lore raw

PATCH 1/1 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 [email protected]

Signed-off-by Junyu Jiang <[email protected]>

---
 drivers/net/i40e/i40e_ethdev.c | 47 ++++++++++++++++++++++++++++++++++
 drivers/net/i40e/i40e_ethdev.h |  9 +++++++
 2 files changed, 56 insertions(+)

drivers/net/i40e/i40e_ethdev.c +47 -0

index 11c02b188..e3d4b7f4f 100644
@@ -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;

drivers/net/i40e/i40e_ethdev.h +9 -0

index 19f821829..5d17be1f0 100644
@@ -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 {

Signature · 1 line

2.17.1

Han, YingyaX

Tested-by on v1 1/1

2020-09-10 02:18 UTC #1 lore raw

Tested-by Yingya Han <[email protected]>

-----Original Message-----
From: stable <[email protected]> On Behalf Of Junyu Jiang
Sent: Thursday, September 10, 2020 9:54 AM
To: [email protected]
Cc: Guo, Jia <[email protected]>; Xing, Beilei <[email protected]>; Jiang, JunyuX <[email protected]>; [email protected]
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 [email protected]

Signed-off-by Junyu Jiang <[email protected]>

---
 drivers/net/i40e/i40e_ethdev.c | 47 ++++++++++++++++++++++++++++++++++
 drivers/net/i40e/i40e_ethdev.h |  9 +++++++
 2 files changed, 56 insertions(+)

drivers/net/i40e/i40e_ethdev.c +7 -0

@@ -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;

drivers/net/i40e/i40e_ethdev.h +9 -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)
+
 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

Guo, Jia

2020-09-10 05:58 UTC #2 lore raw

Hi, junyu

Quoted 53 lines

-----Original Message-----
From: Han, YingyaX <[email protected]>
Sent: Thursday, September 10, 2020 10:18 AM
To: Jiang, JunyuX <[email protected]>; [email protected]
Cc: Guo, Jia <[email protected]>; Xing, Beilei <[email protected]>; Jiang, JunyuX <[email protected]>; [email protected]
Subject: RE: [dpdk-stable] [PATCH] net/i40e: fix incorrect byte counters
Tested-by Yingya Han <[email protected]>
-----Original Message-----
From: stable <[email protected]> On Behalf Of Junyu Jiang
Sent: Thursday, September 10, 2020 9:54 AM
To: [email protected]
Cc: Guo, Jia <[email protected]>; Xing, Beilei <[email protected]>; Jiang, JunyuX <[email protected]>; [email protected]
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 [email protected]
Signed-off-by Junyu Jiang <[email protected]>
---
 drivers/net/i40e/i40e_ethdev.c | 47
++++++++++++++++++++++++++++++++++
 drivers/net/i40e/i40e_ethdev.h |  9 +++++++
 2 files changed, 56 insertions(+)
drivers/net/i40e/i40e_ethdev.c +7 -0
index 11c02b188..e3d4b7f4f 100644
@@ -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);

drivers/net/i40e/i40e_ethdev.c

Quoted 7 lines

drivers/net/i40e/i40e_ethdev.c +7 -0

+/* 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?

Quoted 97 lines

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

drivers/net/i40e/i40e_ethdev.c +19 -0

+/* 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);

drivers/net/i40e/i40e_ethdev.c +15 -0

+/* 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;

(quoted diff) +0 -0

b/drivers/net/i40e/i40e_ethdev.h index 19f821829..5d17be1f0 100644

drivers/net/i40e/i40e_ethdev.h +9 -1

@@ -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

Jiang, JunyuX

author

2020-09-10 06:45 UTC #3 lore raw

HI guojia,

Quoted 68 lines

-----Original Message-----
From: Guo, Jia <[email protected]>
Sent: Thursday, September 10, 2020 1:59 PM
To: Han, YingyaX <[email protected]>; Jiang, JunyuX <[email protected]>; [email protected]
Cc: Xing, Beilei <[email protected]>; Jiang, JunyuX <[email protected]>; [email protected]
Subject: RE: [dpdk-stable] [PATCH] net/i40e: fix incorrect byte counters
Hi, junyu
Quoted 57 lines
-----Original Message-----
From: Han, YingyaX <[email protected]>
Sent: Thursday, September 10, 2020 10:18 AM
To: Jiang, JunyuX <[email protected]>; [email protected]
Cc: Guo, Jia <[email protected]>; Xing, Beilei <[email protected]>; Jiang, JunyuX <[email protected]>; [email protected]
Subject: RE: [dpdk-stable] [PATCH] net/i40e: fix incorrect byte counters
Tested-by Yingya Han <[email protected]>
-----Original Message-----
From: stable <[email protected]> On Behalf Of Junyu Jiang
Sent: Thursday, September 10, 2020 9:54 AM
To: [email protected]
Cc: Guo, Jia <[email protected]>; Xing, Beilei <[email protected]>; Jiang, JunyuX <[email protected]>; [email protected]
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 [email protected]
Signed-off-by Junyu Jiang <[email protected]>
---
 drivers/net/i40e/i40e_ethdev.c | 47
++++++++++++++++++++++++++++++++++
 drivers/net/i40e/i40e_ethdev.h |  9 +++++++
 2 files changed, 56 insertions(+)
(quoted diff) +0 -0
b/drivers/net/i40e/i40e_ethdev.c index 11c02b188..e3d4b7f4f 100644
drivers/net/i40e/i40e_ethdev.c +7 -0
@@ -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

Quoted 9 lines

drivers/net/i40e/i40e_ethdev.c

Quoted 6 lines

drivers/net/i40e/i40e_ethdev.c +6 -0

+(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.

Quoted 2 lines

Quoted 2 lines
vsi->offset_loaded = true;

Junyu Jiang

author

2020-09-16 02:17 UTC #4 lore raw

PATCH v2 1/1 net/i40e: fix incorrect byte counters

In reply to Junyu Jiang

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 [email protected]

Signed-off-by Junyu Jiang <[email protected]>

---
 drivers/net/i40e/i40e_ethdev.c | 47 ++++++++++++++++++++++++++++++++++
 drivers/net/i40e/i40e_ethdev.h |  9 +++++++
 2 files changed, 56 insertions(+)

drivers/net/i40e/i40e_ethdev.c +47 -0

index 563f21d9d..4d4ea9861 100644
@@ -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;

drivers/net/i40e/i40e_ethdev.h +9 -0

index 19f821829..5d17be1f0 100644
@@ -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 {

Signature · 1 line

2.17.1

Han, YingyaX

Tested-by on v2 1/1

2020-09-16 02:39 UTC #5 lore raw

Tested-by Yingya Han <[email protected]>

-----Original Message-----
From: stable <[email protected]> On Behalf Of Junyu Jiang
Sent: Wednesday, September 16, 2020 9:51 AM
To: [email protected]
Cc: Guo, Jia <[email protected]>; Xing, Beilei <[email protected]>; Jiang, JunyuX <[email protected]>; [email protected]
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 [email protected]

Signed-off-by Junyu Jiang <[email protected]>

---
 drivers/net/i40e/i40e_ethdev.c | 47 ++++++++++++++++++++++++++++++++++
 drivers/net/i40e/i40e_ethdev.h |  9 +++++++
 2 files changed, 56 insertions(+)

drivers/net/i40e/i40e_ethdev.c +7 -0

@@ -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;

drivers/net/i40e/i40e_ethdev.h +9 -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)
+
 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

Guo, Jia

Acked-by on v2 1/1

2020-09-16 05:22 UTC #6 lore raw

In reply to Junyu Jiang

Acked-by Jeff Guo <[email protected]>

Quoted 158 lines

-----Original Message-----
From: Jiang, JunyuX <[email protected]>
Sent: Wednesday, September 16, 2020 9:51 AM
To: [email protected]
Cc: Guo, Jia <[email protected]>; Xing, Beilei <[email protected]>; Jiang, JunyuX <[email protected]>; [email protected]
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 [email protected]

Signed-off-by Junyu Jiang <[email protected]>

---
 drivers/net/i40e/i40e_ethdev.c | 47
++++++++++++++++++++++++++++++++++
 drivers/net/i40e/i40e_ethdev.h |  9 +++++++
 2 files changed, 56 insertions(+)

drivers/net/i40e/i40e_ethdev.c +7 -0

index 563f21d9d..4d4ea9861 100644
@@ -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;

drivers/net/i40e/i40e_ethdev.c +4 -0

 	/* 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-

Quoted 1 line

tx_bytes)

drivers/net/i40e/i40e_ethdev.c +2 -0

+			nes->tx_bytes += (uint64_t)1 << I40E_48_BIT_WIDTH;
+		nes->tx_bytes += I40E_RXTX_BYTES_HIGH(vsi-

Quoted 1 line

old_tx_bytes);

drivers/net/i40e/i40e_ethdev.c +2 -0

+	}
+	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)

drivers/net/i40e/i40e_ethdev.c +18 -0

 			    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)

drivers/net/i40e/i40e_ethdev.c +3 -0

 			    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-

Quoted 1 line

eth.rx_bytes)

drivers/net/i40e/i40e_ethdev.c +1 -0

+			ns->eth.rx_bytes += (uint64_t)1 <<

I40E_48_BIT_WIDTH;

drivers/net/i40e/i40e_ethdev.c +1 -0

+		ns->eth.rx_bytes += I40E_RXTX_BYTES_HIGH(pf-

Quoted 1 line

old_rx_bytes);

drivers/net/i40e/i40e_ethdev.c +8 -0

+	}
+	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-

Quoted 1 line

old_tx_bytes);

drivers/net/i40e/i40e_ethdev.c +2 -0

+	}
+	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;

(quoted diff) +0 -0

b/drivers/net/i40e/i40e_ethdev.h index 19f821829..5d17be1f0 100644

drivers/net/i40e/i40e_ethdev.h +9 -1

@@ -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

Zhang, Qi Z

Applied

2020-09-16 05:50 UTC #7 lore raw

Quoted 28 lines

-----Original Message-----
From: dev <[email protected]> On Behalf Of Guo, Jia
Sent: Wednesday, September 16, 2020 1:22 PM
To: Jiang, JunyuX <[email protected]>; [email protected]
Cc: Xing, Beilei <[email protected]>; [email protected]
Subject: Re: [dpdk-dev] [PATCH v2] net/i40e: fix incorrect byte counters
Acked-by Jeff Guo <[email protected]>
Quoted 17 lines
-----Original Message-----
From: Jiang, JunyuX <[email protected]>
Sent: Wednesday, September 16, 2020 9:51 AM
To: [email protected]
Cc: Guo, Jia <[email protected]>; Xing, Beilei <[email protected]>; Jiang, JunyuX <[email protected]>; [email protected]
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 [email protected]
Signed-off-by Junyu Jiang <[email protected]>
---

Applied to dpdk-next-net-intel.

Thanks
Qi

Ferruh Yigit

2020-09-16 12:30 UTC #8 lore raw

In reply to Junyu Jiang

Quoted 1 line · Junyu Jiang

This patch fixed the issue that rx/tx bytes overflowed

"Rx/Tx statistics counters overflowed"?

Quoted 16 lines

on 48 bit limitation by enlarging the limitation.
Fixes 4861cde46116 ("i40e: new poll mode driver")
Cc [email protected]
Signed-off-by Junyu Jiang <[email protected]>
---
  drivers/net/i40e/i40e_ethdev.c | 47 ++++++++++++++++++++++++++++++++++
  drivers/net/i40e/i40e_ethdev.h |  9 +++++++
  2 files changed, 56 insertions(+)
drivers/net/i40e/i40e_ethdev.c +0 -0
index 563f21d9d..4d4ea9861 100644

drivers/net/i40e/i40e_ethdev.c i40e_update_vsi_stats(struct i40e_vsi *vsi) · line 3082

Quoted 10 lines

drivers/net/i40e/i40e_ethdev.c +7 -0

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

<....>

drivers/net/i40e/i40e_ethdev.c struct rte_flow { · line 287

Quoted 7 lines

drivers/net/i40e/i40e_ethdev.c +3 -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.

drivers/net/i40e/i40e_ethdev.c struct i40e_vsi { · line 406

Quoted 9 lines

drivers/net/i40e/i40e_ethdev.c +2 -0

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

Jiang, JunyuX

author

2020-09-18 03:45 UTC #9 lore raw

Hi Ferruh,

Quoted 8 lines

-----Original Message-----
From: Ferruh Yigit <[email protected]>
Sent: Wednesday, September 16, 2020 8:31 PM
To: Jiang, JunyuX <[email protected]>; [email protected]
Cc: Guo, Jia <[email protected]>; Xing, Beilei <[email protected]>; [email protected]
Subject: Re: [dpdk-stable] [PATCH v2] net/i40e: fix incorrect byte counters

Quoted 5 lines

Quoted 1 line · Junyu Jiang
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.

Quoted 35 lines

Quoted 8 lines
on 48 bit limitation by enlarging the limitation.
Fixes 4861cde46116 ("i40e: new poll mode driver")
Cc [email protected]
Signed-off-by Junyu Jiang <[email protected]>
---
  drivers/net/i40e/i40e_ethdev.c | 47
++++++++++++++++++++++++++++++++++
drivers/net/i40e/i40e_ethdev.c i40e_update_vsi_stats(struct i40e_vsi *vsi) · line 3073
Quoted 9 lines
  drivers/net/i40e/i40e_ethdev.h |  9 +++++++
  2 files changed, 56 insertions(+)
(quoted diff) +0 -0
b/drivers/net/i40e/i40e_ethdev.c index 563f21d9d..4d4ea9861 100644
drivers/net/i40e/i40e_ethdev.c +0 -0
@@ -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),
drivers/net/i40e/i40e_ethdev.c i40e_update_vsi_stats(struct i40e_vsi *vsi) · line 3082
Quoted 11 lines
drivers/net/i40e/i40e_ethdev.c +7 -0
  			    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)

Quoted 8 lines

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

Quoted 3 lines

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.

Quoted 7 lines

<....>
drivers/net/i40e/i40e_ethdev.c struct rte_flow { · line 283
Quoted 3 lines
drivers/net/i40e/i40e_ethdev.c +0 -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)
Quoted 1 line

Quoted 8 lines

drivers/net/i40e/i40e_ethdev.c
Quoted 3 lines
drivers/net/i40e/i40e_ethdev.c +3 -0
+#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.

Quoted 4 lines

Quoted 4 lines
struct i40e_adapter;
struct rte_pci_driver;

Quoted 9 lines

drivers/net/i40e/i40e_ethdev.c struct i40e_vsi { · line 406
Quoted 6 lines
drivers/net/i40e/i40e_ethdev.c +2 -0
@@ -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.

Igor Ryzhov

2020-09-18 09:24 UTC #10 lore raw

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

Quoted 111 lines · JunyuX

Hi Ferruh,
Quoted 7 lines
-----Original Message-----
From: Ferruh Yigit <[email protected]>
Sent: Wednesday, September 16, 2020 8:31 PM
To: Jiang, JunyuX <[email protected]>; [email protected]
Cc: Guo, Jia <[email protected]>; Xing, Beilei <[email protected]>; [email protected]
Subject: Re: [dpdk-stable] [PATCH v2] net/i40e: fix incorrect byte
counters
Quoted 6 lines · Junyu Jiang
Quoted 1 line
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.
Quoted 35 lines
Quoted 8 lines
on 48 bit limitation by enlarging the limitation.
Fixes 4861cde46116 ("i40e: new poll mode driver")
Cc [email protected]
Signed-off-by Junyu Jiang <[email protected]>
---
  drivers/net/i40e/i40e_ethdev.c | 47
++++++++++++++++++++++++++++++++++
drivers/net/i40e/i40e_ethdev.c i40e_update_vsi_stats(struct i40e_vsi *vsi) · line 3073
Quoted 9 lines
  drivers/net/i40e/i40e_ethdev.h |  9 +++++++
  2 files changed, 56 insertions(+)
(quoted diff) +0 -0
b/drivers/net/i40e/i40e_ethdev.c index 563f21d9d..4d4ea9861 100644
drivers/net/i40e/i40e_ethdev.c +0 -0
@@ -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),
drivers/net/i40e/i40e_ethdev.c i40e_update_vsi_stats(struct i40e_vsi *vsi) · line 3082
Quoted 11 lines
drivers/net/i40e/i40e_ethdev.c +7 -0
                        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)
Quoted 8 lines
'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 .
Quoted 1 line
Also can you please confirm the initial value of the
"vsi->offset_loaded" is
Quoted 2 lines
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.
Quoted 7 lines
<....>
drivers/net/i40e/i40e_ethdev.c struct rte_flow { · line 283
Quoted 3 lines
drivers/net/i40e/i40e_ethdev.c +0 -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)
Quoted 1 line
Quoted 5 lines
drivers/net/i40e/i40e_ethdev.c
Quoted 3 lines
drivers/net/i40e/i40e_ethdev.c +3 -0
+#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
Quoted 3 lines
and high 32 bits, can you please clarify macro masks out 48/16 bits.
Yes, I will change the macro name in V3.
Quoted 4 lines
Quoted 4 lines
struct i40e_adapter;
struct rte_pci_driver;
Quoted 9 lines
drivers/net/i40e/i40e_ethdev.c struct i40e_vsi { · line 406
Quoted 6 lines
drivers/net/i40e/i40e_ethdev.c +2 -0
@@ -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.

Ferruh Yigit

2020-09-18 13:42 UTC #11 lore raw

Quoted 4 lines · Igor Ryzhov

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.

Quoted 2 lines

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

+1 to document.

Quoted 122 lines

Igor

On Fri, Sep 18, 2020 at 6:45 AM Jiang, JunyuX <[email protected] <mailto:[email protected]>> wrote:

    Hi Ferruh,

     > -----Original Message-----
     > From: Ferruh Yigit <[email protected]
    <mailto:[email protected]>>
     > Sent: Wednesday, September 16, 2020 8:31 PM
     > To: Jiang, JunyuX <[email protected]
    <mailto:[email protected]>>; [email protected] <mailto:[email protected]>
     > Cc: Guo, Jia <[email protected] <mailto:[email protected]>>;
    Xing, Beilei <[email protected] <mailto:[email protected]>>;
     > [email protected] <mailto:[email protected]>
     > 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: [email protected] <mailto:[email protected]>
     > >
     > > Signed-off-by: Junyu Jiang <[email protected]
    <mailto:[email protected]>>
     > > ---
     > >   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.

Jiang, JunyuX

author

2020-09-21 01:56 UTC #12 lore raw

Hi Ferruh,

Quoted 23 lines

-----Original Message-----
From: Ferruh Yigit <[email protected]>
Sent: Friday, September 18, 2020 9:42 PM
To: Igor Ryzhov <[email protected]>; Jiang, JunyuX <[email protected]>
Cc: [email protected]; Guo, Jia <[email protected]>; Xing, Beilei <[email protected]>; [email protected]
Subject: Re: [dpdk-dev] [dpdk-stable] [PATCH v2] net/i40e: fix incorrect byte counters
Quoted 5 lines · Igor Ryzhov
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.

Quoted 6 lines

Quoted 3 lines
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.

Quoted 129 lines

Quoted 5 lines

Igor
On Fri, Sep 18, 2020 at 6:45 AM Jiang, JunyuX <[email protected] <mailto:[email protected]>> wrote:

Quoted 8 lines

Hi Ferruh,
> -----Original Message-----
> From: Ferruh Yigit <[email protected]
<mailto:[email protected]>>
> Sent: Wednesday, September 16, 2020 8:31 PM > To: Jiang, JunyuX <[email protected]
<mailto:[email protected]>>; [email protected]

<mailto:[email protected]>

Quoted 21 lines

     > Cc: Guo, Jia <[email protected] <mailto:[email protected]>>;
    Xing, Beilei <[email protected] <mailto:[email protected]>>;
     > [email protected] <mailto:[email protected]>
     > 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: [email protected] <mailto:[email protected]>
     > >
     > > Signed-off-by: Junyu Jiang <[email protected]
    <mailto:[email protected]>>

Quoted 8 lines

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

Quoted 29 lines

     > > --- 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?

Quoted 8 lines

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

Quoted 12 lines

    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.

Quoted 8 lines

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

Quoted 1 line

     > > +#define I40E_RXTX_BYTES_LOW(bytes) ((bytes) &

I40E_48_BIT_MASK)

Quoted 24 lines

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

Ferruh Yigit

2020-09-18 13:37 UTC #13 lore raw

In reply to Jiang, JunyuX

Quoted 55 lines · JunyuX

Hi Ferruh,
Quoted 8 lines
-----Original Message-----
From: Ferruh Yigit <[email protected]>
Sent: Wednesday, September 16, 2020 8:31 PM
To: Jiang, JunyuX <[email protected]>; [email protected]
Cc: Guo, Jia <[email protected]>; Xing, Beilei <[email protected]>; [email protected]
Subject: Re: [dpdk-stable] [PATCH v2] net/i40e: fix incorrect byte counters
Quoted 5 lines
Quoted 1 line · Junyu Jiang
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.
Quoted 38 lines
Quoted 8 lines
on 48 bit limitation by enlarging the limitation.
Fixes 4861cde46116 ("i40e: new poll mode driver")
Cc [email protected]
Signed-off-by Junyu Jiang <[email protected]>
---
   drivers/net/i40e/i40e_ethdev.c | 47
++++++++++++++++++++++++++++++++++
drivers/net/i40e/i40e_ethdev.c i40e_update_vsi_stats(struct i40e_vsi *vsi) · line 3073
Quoted 9 lines
   drivers/net/i40e/i40e_ethdev.h |  9 +++++++
   2 files changed, 56 insertions(+)
(quoted diff) +0 -0
b/drivers/net/i40e/i40e_ethdev.c index 563f21d9d..4d4ea9861 100644
drivers/net/i40e/i40e_ethdev.c +0 -0
@@ -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),
drivers/net/i40e/i40e_ethdev.c i40e_update_vsi_stats(struct i40e_vsi *vsi) · line 3082
Quoted 11 lines
drivers/net/i40e/i40e_ethdev.c +7 -0
   			    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.

Quoted 9 lines

Quoted 5 lines
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.

Quoted 38 lines

Quoted 4 lines
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.
Quoted 7 lines
<....>
drivers/net/i40e/i40e_ethdev.c struct rte_flow { · line 283
Quoted 3 lines
drivers/net/i40e/i40e_ethdev.c +0 -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)
Quoted 1 line
Quoted 8 lines
drivers/net/i40e/i40e_ethdev.c
Quoted 3 lines
drivers/net/i40e/i40e_ethdev.c +3 -0
+#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.
Quoted 4 lines
Quoted 4 lines
   struct i40e_adapter;
   struct rte_pci_driver;
Quoted 9 lines
drivers/net/i40e/i40e_ethdev.c struct i40e_vsi { · line 406
Quoted 6 lines
drivers/net/i40e/i40e_ethdev.c +2 -0
@@ -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.

Junyu Jiang

author

2020-09-21 10:21 UTC #14 lore raw

PATCH v3 1/1 net/i40e: fix incorrect byte counters

In reply to Junyu Jiang

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 [email protected]

Signed-off-by Junyu Jiang <[email protected]>

---
 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(+)

doc/guides/nics/i40e.rst +7 -0

index b7430f6c4..4baa58be6 100644
@@ -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.

drivers/net/i40e/i40e_ethdev.c +32 -0

index 563f21d9d..212338ef0 100644
@@ -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;

drivers/net/i40e/i40e_ethdev.h +9 -0

index 19f821829..1466998aa 100644
@@ -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 {

Signature · 1 line

2.17.1

Ferruh Yigit

2020-09-21 11:41 UTC #15 lore raw

Quoted 39 lines · Junyu Jiang

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 [email protected]

Signed-off-by Junyu Jiang <[email protected]>

---
  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(+)

doc/guides/nics/i40e.rst +7 -0

index b7430f6c4..4baa58be6 100644
@@ -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.

drivers/net/i40e/i40e_ethdev.c +3 -0

index 563f21d9d..212338ef0 100644
@@ -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,

drivers/net/i40e/i40e_ethdev.c i40e_dev_link_update(struct rte_eth_dev *dev, · line 3067

Quoted 10 lines

drivers/net/i40e/i40e_ethdev.c +10 -0

+			  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?

Junyu Jiang

author

2020-09-22 08:02 UTC #16 lore raw

PATCH v4 1/1 net/i40e: fix incorrect byte counters

In reply to Junyu Jiang

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 [email protected]

Signed-off-by Junyu Jiang <[email protected]>

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

doc/guides/nics/i40e.rst +7 -0

index b7430f6c4..4baa58be6 100644
@@ -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.

drivers/net/i40e/i40e_ethdev.c +41 ~~-25~~

102 lines not on this page · raw message

drivers/net/i40e/i40e_ethdev.h +9 -0

index 19f821829..1466998aa 100644
@@ -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 {

Signature · 1 line

2.17.1

Ferruh Yigit

Reviewed-by on v4 1/1

2020-09-22 09:06 UTC #17 lore raw

Quoted 23 lines · Junyu Jiang

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 [email protected]
Signed-off-by Junyu Jiang <[email protected]>
---
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(-)
doc/guides/nics/i40e.rst +0 -0
index b7430f6c4..4baa58be6 100644

doc/guides/nics/i40e.rst Tx bytes affected by the link status change · line 839

Quoted 10 lines

doc/guides/nics/i40e.rst +7 -0

@@ -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 <[email protected]>

Junyu Jiang

author

2020-09-22 09:48 UTC #18 lore raw

PATCH v5 1/1 net/i40e: fix incorrect byte counters

In reply to Junyu Jiang

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 [email protected]

Signed-off-by Junyu Jiang <[email protected]>

Reviewed-by Ferruh Yigit <[email protected]>

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

doc/guides/nics/i40e.rst +9 -0

index b7430f6c4..a0b81e669 100644
@@ -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
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

drivers/net/i40e/i40e_ethdev.c +41 ~~-25~~

102 lines not on this page · raw message

drivers/net/i40e/i40e_ethdev.h +9 -0

index 19f821829..1466998aa 100644
@@ -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 {

Signature · 1 line

2.17.1

Zhang, Qi Z

Applied

2020-09-22 12:59 UTC #19 lore raw

Quoted 11 lines

-----Original Message-----
From: dev <[email protected]> On Behalf Of Junyu Jiang
Sent: Tuesday, September 22, 2020 5:20 PM
To: [email protected]
Cc: Guo, Jia <[email protected]>; Xing, Beilei <[email protected]>; Yigit, Ferruh <[email protected]>; Jiang, JunyuX <[email protected]>; [email protected]
Subject: [dpdk-dev] [PATCH v5] net/i40e: fix incorrect byte counters

Quoted 8 lines

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 [email protected]
Signed-off-by Junyu Jiang <[email protected]>
Reviewed-by Ferruh Yigit <[email protected]>

Applied to dpdk-next-net-intel.

Thanks
Qi