[v2] net/i40e: fix incorrect byte counters
Checks
Commit Message
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(+)
Comments
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
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
> -----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
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)?
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.
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.
>
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.
>
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.
>
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.
> >
@@ -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;
@@ -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 {