patches for DPDK stable branches
 help / color / mirror / Atom feed
* [dpdk-stable] [PATCH] net/i40e: fix Rx packet statistics
@ 2021-09-26  7:57 Alvin Zhang
  2021-09-27 16:00 ` [dpdk-stable] [dpdk-dev] " Kevin Traynor
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Alvin Zhang @ 2021-09-26  7:57 UTC (permalink / raw)
  To: beilei.xing, junfeng.guo; +Cc: dev, Alvin Zhang, stable

Some packets are discarded by the NIC because they are larger than
the MTU, these packets should be counted as "RX error" instead of
"RX packet".

The register 'GL_RXERR1' can count above discarded packets.
This patch adds reading and calculation of the 'GL_RXERR1' counter
when reporting DPDK statistics.

Fixes: f4a91c38b4ad ("i40e: add extended stats")
Cc: stable@dpdk.org

Signed-off-by: Alvin Zhang <alvinx.zhang@intel.com>
---
 drivers/net/i40e/i40e_ethdev.c | 16 +++++++++++++---
 drivers/net/i40e/i40e_ethdev.h | 10 ++++++++++
 2 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 7a2a828..30a2cdf 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -532,7 +532,7 @@ static int i40e_sw_tunnel_filter_insert(struct i40e_pf *pf,
 /* store statistics names and its offset in stats structure */
 struct rte_i40e_xstats_name_off {
 	char name[RTE_ETH_XSTATS_NAME_SIZE];
-	unsigned offset;
+	int offset;
 };
 
 static const struct rte_i40e_xstats_name_off rte_i40e_stats_strings[] = {
@@ -542,6 +542,8 @@ struct rte_i40e_xstats_name_off {
 	{"rx_dropped_packets", offsetof(struct i40e_eth_stats, rx_discards)},
 	{"rx_unknown_protocol_packets", offsetof(struct i40e_eth_stats,
 		rx_unknown_protocol)},
+	{"rx_err1", offsetof(struct i40e_pf, rx_err1) -
+		    offsetof(struct i40e_pf, stats)},
 	{"tx_unicast_packets", offsetof(struct i40e_eth_stats, tx_unicast)},
 	{"tx_multicast_packets", offsetof(struct i40e_eth_stats, tx_multicast)},
 	{"tx_broadcast_packets", offsetof(struct i40e_eth_stats, tx_broadcast)},
@@ -3238,6 +3240,10 @@ void i40e_flex_payload_reg_set_default(struct i40e_hw *hw)
 			    pf->offset_loaded,
 			    &os->eth.rx_unknown_protocol,
 			    &ns->eth.rx_unknown_protocol);
+	i40e_stat_update_48(hw, I40E_GL_RXERR1_H(hw->pf_id + I40E_MAX_VF),
+			    I40E_GL_RXERR1_L(hw->pf_id + I40E_MAX_VF),
+			    pf->offset_loaded, &pf->rx_err1_offset,
+			    &pf->rx_err1);
 	i40e_stat_update_48_in_64(hw, I40E_GLPRT_GOTCH(hw->port),
 				  I40E_GLPRT_GOTCL(hw->port),
 				  pf->offset_loaded, &os->eth.tx_bytes,
@@ -3437,7 +3443,8 @@ void i40e_flex_payload_reg_set_default(struct i40e_hw *hw)
 	stats->ipackets = pf->main_vsi->eth_stats.rx_unicast +
 			pf->main_vsi->eth_stats.rx_multicast +
 			pf->main_vsi->eth_stats.rx_broadcast -
-			pf->main_vsi->eth_stats.rx_discards;
+			pf->main_vsi->eth_stats.rx_discards -
+			pf->rx_err1;
 	stats->opackets = ns->eth.tx_unicast +
 			ns->eth.tx_multicast +
 			ns->eth.tx_broadcast;
@@ -3451,7 +3458,8 @@ void i40e_flex_payload_reg_set_default(struct i40e_hw *hw)
 			pf->main_vsi->eth_stats.rx_discards;
 	stats->ierrors  = ns->crc_errors +
 			ns->rx_length_errors + ns->rx_undersize +
-			ns->rx_oversize + ns->rx_fragments + ns->rx_jabber;
+			ns->rx_oversize + ns->rx_fragments + ns->rx_jabber +
+			pf->rx_err1;
 
 	if (pf->vfs) {
 		for (i = 0; i < pf->vf_num; i++) {
@@ -6232,6 +6240,8 @@ struct i40e_vsi *
 	memset(&pf->stats_offset, 0, sizeof(struct i40e_hw_port_stats));
 	memset(&pf->internal_stats, 0, sizeof(struct i40e_eth_stats));
 	memset(&pf->internal_stats_offset, 0, sizeof(struct i40e_eth_stats));
+	pf->rx_err1 = 0;
+	pf->rx_err1_offset = 0;
 
 	ret = i40e_pf_get_switch_config(pf);
 	if (ret != I40E_SUCCESS) {
diff --git a/drivers/net/i40e/i40e_ethdev.h b/drivers/net/i40e/i40e_ethdev.h
index cd6deab..846c8d4 100644
--- a/drivers/net/i40e/i40e_ethdev.h
+++ b/drivers/net/i40e/i40e_ethdev.h
@@ -19,6 +19,13 @@
 #include "base/i40e_type.h"
 #include "base/virtchnl.h"
 
+#define I40E_GL_RXERR1_H(_i)	(0x00318004 + ((_i) * 8))
+/**
+ * _i=0...143,
+ * counters 0-127 are for the 128 VFs,
+ * counters 128-143 are for the 16 PFs
+ */
+
 #define I40E_VLAN_TAG_SIZE        4
 
 #define I40E_AQ_LEN               32
@@ -1134,6 +1141,9 @@ struct i40e_pf {
 
 	struct i40e_hw_port_stats stats_offset;
 	struct i40e_hw_port_stats stats;
+	u64 rx_err1;	/* rxerr1 */
+	u64 rx_err1_offset;
+
 	/* internal packet statistics, it should be excluded from the total */
 	struct i40e_eth_stats internal_stats_offset;
 	struct i40e_eth_stats internal_stats;
-- 
1.8.3.1


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

* Re: [dpdk-stable] [dpdk-dev] [PATCH] net/i40e: fix Rx packet statistics
  2021-09-26  7:57 [dpdk-stable] [PATCH] net/i40e: fix Rx packet statistics Alvin Zhang
@ 2021-09-27 16:00 ` Kevin Traynor
  2021-09-28  2:12   ` Zhang, AlvinX
  2021-09-28  3:21 ` [dpdk-stable] [PATCH v2] " Alvin Zhang
  2021-09-28  3:22 ` Alvin Zhang
  2 siblings, 1 reply; 10+ messages in thread
From: Kevin Traynor @ 2021-09-27 16:00 UTC (permalink / raw)
  To: Alvin Zhang, beilei.xing, junfeng.guo
  Cc: dev, stable, Zhang, Qi Z, Ferruh Yigit

On 26/09/2021 08:57, Alvin Zhang wrote:
> Some packets are discarded by the NIC because they are larger than
> the MTU, these packets should be counted as "RX error" instead of
> "RX packet".
> 
> The register 'GL_RXERR1' can count above discarded packets.
> This patch adds reading and calculation of the 'GL_RXERR1' counter
> when reporting DPDK statistics.
> 
> Fixes: f4a91c38b4ad ("i40e: add extended stats")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Alvin Zhang <alvinx.zhang@intel.com>
> ---
>   drivers/net/i40e/i40e_ethdev.c | 16 +++++++++++++---
>   drivers/net/i40e/i40e_ethdev.h | 10 ++++++++++
>   2 files changed, 23 insertions(+), 3 deletions(-)
> 

It's a bit hard to understand the code for someone not familiar with the 
i40e stats. I think it needs careful review from i40e maintainers. A few 
questions below,

Did you test this with testpmd? Can you show an example of a test where 
these packets are now correctly accounted for?

I see there is also an RXERR2 register that catches other errors, does 
it need to be considered as well?

> diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
> index 7a2a828..30a2cdf 100644
> --- a/drivers/net/i40e/i40e_ethdev.c
> +++ b/drivers/net/i40e/i40e_ethdev.c
> @@ -532,7 +532,7 @@ static int i40e_sw_tunnel_filter_insert(struct i40e_pf *pf,
>   /* store statistics names and its offset in stats structure */
>   struct rte_i40e_xstats_name_off {
>   	char name[RTE_ETH_XSTATS_NAME_SIZE];
> -	unsigned offset;
> +	int offset;

It is unusual to see you changing an offset to an int. You are expecting 
negative offsets?

>   };
>   
>   static const struct rte_i40e_xstats_name_off rte_i40e_stats_strings[] = {
> @@ -542,6 +542,8 @@ struct rte_i40e_xstats_name_off {
>   	{"rx_dropped_packets", offsetof(struct i40e_eth_stats, rx_discards)},
>   	{"rx_unknown_protocol_packets", offsetof(struct i40e_eth_stats,
>   		rx_unknown_protocol)},
> +	{"rx_err1", offsetof(struct i40e_pf, rx_err1) -
> +		    offsetof(struct i40e_pf, stats)},

rx_err1 is correct by datasheet but meaningless to a user. Suggest to 
find a more descriptive name, or document what it is, or tell the user 
to reference the datasheet.

>   	{"tx_unicast_packets", offsetof(struct i40e_eth_stats, tx_unicast)},
>   	{"tx_multicast_packets", offsetof(struct i40e_eth_stats, tx_multicast)},
>   	{"tx_broadcast_packets", offsetof(struct i40e_eth_stats, tx_broadcast)},
> @@ -3238,6 +3240,10 @@ void i40e_flex_payload_reg_set_default(struct i40e_hw *hw)
>   			    pf->offset_loaded,
>   			    &os->eth.rx_unknown_protocol,
>   			    &ns->eth.rx_unknown_protocol);
> +	i40e_stat_update_48(hw, I40E_GL_RXERR1_H(hw->pf_id + I40E_MAX_VF),
> +			    I40E_GL_RXERR1_L(hw->pf_id + I40E_MAX_VF),
> +			    pf->offset_loaded, &pf->rx_err1_offset,
> +			    &pf->rx_err1);
>   	i40e_stat_update_48_in_64(hw, I40E_GLPRT_GOTCH(hw->port),
>   				  I40E_GLPRT_GOTCL(hw->port),
>   				  pf->offset_loaded, &os->eth.tx_bytes,
> @@ -3437,7 +3443,8 @@ void i40e_flex_payload_reg_set_default(struct i40e_hw *hw)
>   	stats->ipackets = pf->main_vsi->eth_stats.rx_unicast +
>   			pf->main_vsi->eth_stats.rx_multicast +
>   			pf->main_vsi->eth_stats.rx_broadcast -
> -			pf->main_vsi->eth_stats.rx_discards;
> +			pf->main_vsi->eth_stats.rx_discards -
> +			pf->rx_err1;
>   	stats->opackets = ns->eth.tx_unicast +
>   			ns->eth.tx_multicast +
>   			ns->eth.tx_broadcast;
> @@ -3451,7 +3458,8 @@ void i40e_flex_payload_reg_set_default(struct i40e_hw *hw)
>   			pf->main_vsi->eth_stats.rx_discards;
>   	stats->ierrors  = ns->crc_errors +
>   			ns->rx_length_errors + ns->rx_undersize +
> -			ns->rx_oversize + ns->rx_fragments + ns->rx_jabber;
> +			ns->rx_oversize + ns->rx_fragments + ns->rx_jabber +
> +			pf->rx_err1;
>   
>   	if (pf->vfs) {
>   		for (i = 0; i < pf->vf_num; i++) {
> @@ -6232,6 +6240,8 @@ struct i40e_vsi *
>   	memset(&pf->stats_offset, 0, sizeof(struct i40e_hw_port_stats));
>   	memset(&pf->internal_stats, 0, sizeof(struct i40e_eth_stats));
>   	memset(&pf->internal_stats_offset, 0, sizeof(struct i40e_eth_stats));
> +	pf->rx_err1 = 0;
> +	pf->rx_err1_offset = 0;
>   
>   	ret = i40e_pf_get_switch_config(pf);
>   	if (ret != I40E_SUCCESS) {
> diff --git a/drivers/net/i40e/i40e_ethdev.h b/drivers/net/i40e/i40e_ethdev.h
> index cd6deab..846c8d4 100644
> --- a/drivers/net/i40e/i40e_ethdev.h
> +++ b/drivers/net/i40e/i40e_ethdev.h
> @@ -19,6 +19,13 @@
>   #include "base/i40e_type.h"
>   #include "base/virtchnl.h"
>   
> +#define I40E_GL_RXERR1_H(_i)	(0x00318004 + ((_i) * 8))
> +/**
> + * _i=0...143,
> + * counters 0-127 are for the 128 VFs,
> + * counters 128-143 are for the 16 PFs
> + */
> +
>   #define I40E_VLAN_TAG_SIZE        4
>   
>   #define I40E_AQ_LEN               32
> @@ -1134,6 +1141,9 @@ struct i40e_pf {
>   
>   	struct i40e_hw_port_stats stats_offset;
>   	struct i40e_hw_port_stats stats;
> +	u64 rx_err1;	/* rxerr1 */
> +	u64 rx_err1_offset;
> +
>   	/* internal packet statistics, it should be excluded from the total */
>   	struct i40e_eth_stats internal_stats_offset;
>   	struct i40e_eth_stats internal_stats;
> 


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

* Re: [dpdk-stable] [dpdk-dev] [PATCH] net/i40e: fix Rx packet statistics
  2021-09-27 16:00 ` [dpdk-stable] [dpdk-dev] " Kevin Traynor
@ 2021-09-28  2:12   ` Zhang, AlvinX
  2021-09-28  8:53     ` Kevin Traynor
  0 siblings, 1 reply; 10+ messages in thread
From: Zhang, AlvinX @ 2021-09-28  2:12 UTC (permalink / raw)
  To: Kevin Traynor, Xing, Beilei, Guo, Junfeng
  Cc: dev, stable, Zhang, Qi Z, Yigit, Ferruh

> -----Original Message-----
> From: Kevin Traynor <ktraynor@redhat.com>
> Sent: Tuesday, September 28, 2021 12:00 AM
> To: Zhang, AlvinX <alvinx.zhang@intel.com>; Xing, Beilei
> <beilei.xing@intel.com>; Guo, Junfeng <junfeng.guo@intel.com>
> Cc: dev@dpdk.org; stable@dpdk.org; Zhang, Qi Z <qi.z.zhang@intel.com>;
> Yigit, Ferruh <ferruh.yigit@intel.com>
> Subject: Re: [dpdk-dev] [PATCH] net/i40e: fix Rx packet statistics
> 
> On 26/09/2021 08:57, Alvin Zhang wrote:
> > Some packets are discarded by the NIC because they are larger than the
> > MTU, these packets should be counted as "RX error" instead of "RX
> > packet".
> >
> > The register 'GL_RXERR1' can count above discarded packets.
> > This patch adds reading and calculation of the 'GL_RXERR1' counter
> > when reporting DPDK statistics.
> >
> > Fixes: f4a91c38b4ad ("i40e: add extended stats")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Alvin Zhang <alvinx.zhang@intel.com>
> > ---
> >   drivers/net/i40e/i40e_ethdev.c | 16 +++++++++++++---
> >   drivers/net/i40e/i40e_ethdev.h | 10 ++++++++++
> >   2 files changed, 23 insertions(+), 3 deletions(-)
> >
> 
> It's a bit hard to understand the code for someone not familiar with the i40e
> stats. I think it needs careful review from i40e maintainers. A few questions
> below,
> 
> Did you test this with testpmd? Can you show an example of a test where these
> packets are now correctly accounted for?

The issue as below:

sendp(Ether()/IP()/Raw('x' * 1500), iface="enp24s0f1")
---------------------- Forward statistics for port 0 ----------------------
RX-packets: 1 RX-dropped: 0 RX-total: 1
TX-packets: 0 TX-dropped: 0 TX-total: 0
----------------------------------------------------------------------------

Although we didn't really got the packet, but the statistic indicates a packet has been received successfully.
We can add above example to commit log in V2.

> 
> I see there is also an RXERR2 register that catches other errors, does it need to
> be considered as well?

We are not sure whether the packets counted by RXERR2 also will be counted by "Rx-packets".
So in this patch we only consider RXERR1 and fix the issue mentioned above.

> 
> > diff --git a/drivers/net/i40e/i40e_ethdev.c
> > b/drivers/net/i40e/i40e_ethdev.c index 7a2a828..30a2cdf 100644
> > --- a/drivers/net/i40e/i40e_ethdev.c
> > +++ b/drivers/net/i40e/i40e_ethdev.c
> > @@ -532,7 +532,7 @@ static int i40e_sw_tunnel_filter_insert(struct i40e_pf
> *pf,
> >   /* store statistics names and its offset in stats structure */
> >   struct rte_i40e_xstats_name_off {
> >   	char name[RTE_ETH_XSTATS_NAME_SIZE];
> > -	unsigned offset;
> > +	int offset;
> 
> It is unusual to see you changing an offset to an int. You are expecting negative
> offsets?
> 
> >   };
> >
> >   static const struct rte_i40e_xstats_name_off
> > rte_i40e_stats_strings[] = { @@ -542,6 +542,8 @@ struct
> rte_i40e_xstats_name_off {
> >   	{"rx_dropped_packets", offsetof(struct i40e_eth_stats, rx_discards)},
> >   	{"rx_unknown_protocol_packets", offsetof(struct i40e_eth_stats,
> >   		rx_unknown_protocol)},
> > +	{"rx_err1", offsetof(struct i40e_pf, rx_err1) -
> > +		    offsetof(struct i40e_pf, stats)},

Here offsetof(struct i40e_pf, rx_err1) - offsetof(struct i40e_pf, stats) may be a negative value.

> 
> rx_err1 is correct by datasheet but meaningless to a user. Suggest to find a more
> descriptive name, or document what it is, or tell the user to reference the
> datasheet.

I will update it in v2.

> 
> >   	{"tx_unicast_packets", offsetof(struct i40e_eth_stats, tx_unicast)},
> >   	{"tx_multicast_packets", offsetof(struct i40e_eth_stats,
> tx_multicast)},
> >   	{"tx_broadcast_packets", offsetof(struct i40e_eth_stats,
> > tx_broadcast)}, @@ -3238,6 +3240,10 @@ void
> i40e_flex_payload_reg_set_default(struct i40e_hw *hw)
> >   			    pf->offset_loaded,
> >   			    &os->eth.rx_unknown_protocol,
> >   			    &ns->eth.rx_unknown_protocol);
> > +	i40e_stat_update_48(hw, I40E_GL_RXERR1_H(hw->pf_id +
> I40E_MAX_VF),
> > +			    I40E_GL_RXERR1_L(hw->pf_id + I40E_MAX_VF),
> > +			    pf->offset_loaded, &pf->rx_err1_offset,
> > +			    &pf->rx_err1);
> >   	i40e_stat_update_48_in_64(hw, I40E_GLPRT_GOTCH(hw->port),
> >   				  I40E_GLPRT_GOTCL(hw->port),
> >   				  pf->offset_loaded, &os->eth.tx_bytes, @@ -3437,7
> +3443,8 @@
> > void i40e_flex_payload_reg_set_default(struct i40e_hw *hw)
> >   	stats->ipackets = pf->main_vsi->eth_stats.rx_unicast +
> >   			pf->main_vsi->eth_stats.rx_multicast +
> >   			pf->main_vsi->eth_stats.rx_broadcast -
> > -			pf->main_vsi->eth_stats.rx_discards;
> > +			pf->main_vsi->eth_stats.rx_discards -
> > +			pf->rx_err1;
> >   	stats->opackets = ns->eth.tx_unicast +
> >   			ns->eth.tx_multicast +
> >   			ns->eth.tx_broadcast;
> > @@ -3451,7 +3458,8 @@ void i40e_flex_payload_reg_set_default(struct
> i40e_hw *hw)
> >   			pf->main_vsi->eth_stats.rx_discards;
> >   	stats->ierrors  = ns->crc_errors +
> >   			ns->rx_length_errors + ns->rx_undersize +
> > -			ns->rx_oversize + ns->rx_fragments + ns->rx_jabber;
> > +			ns->rx_oversize + ns->rx_fragments + ns->rx_jabber +
> > +			pf->rx_err1;
> >
> >   	if (pf->vfs) {
> >   		for (i = 0; i < pf->vf_num; i++) { @@ -6232,6 +6240,8 @@ struct
> > i40e_vsi *
> >   	memset(&pf->stats_offset, 0, sizeof(struct i40e_hw_port_stats));
> >   	memset(&pf->internal_stats, 0, sizeof(struct i40e_eth_stats));
> >   	memset(&pf->internal_stats_offset, 0, sizeof(struct
> > i40e_eth_stats));
> > +	pf->rx_err1 = 0;
> > +	pf->rx_err1_offset = 0;
> >
> >   	ret = i40e_pf_get_switch_config(pf);
> >   	if (ret != I40E_SUCCESS) {
> > diff --git a/drivers/net/i40e/i40e_ethdev.h
> > b/drivers/net/i40e/i40e_ethdev.h index cd6deab..846c8d4 100644
> > --- a/drivers/net/i40e/i40e_ethdev.h
> > +++ b/drivers/net/i40e/i40e_ethdev.h
> > @@ -19,6 +19,13 @@
> >   #include "base/i40e_type.h"
> >   #include "base/virtchnl.h"
> >
> > +#define I40E_GL_RXERR1_H(_i)	(0x00318004 + ((_i) * 8))
> > +/**
> > + * _i=0...143,
> > + * counters 0-127 are for the 128 VFs,
> > + * counters 128-143 are for the 16 PFs  */
> > +
> >   #define I40E_VLAN_TAG_SIZE        4
> >
> >   #define I40E_AQ_LEN               32
> > @@ -1134,6 +1141,9 @@ struct i40e_pf {
> >
> >   	struct i40e_hw_port_stats stats_offset;
> >   	struct i40e_hw_port_stats stats;
> > +	u64 rx_err1;	/* rxerr1 */
> > +	u64 rx_err1_offset;
> > +
> >   	/* internal packet statistics, it should be excluded from the total */
> >   	struct i40e_eth_stats internal_stats_offset;
> >   	struct i40e_eth_stats internal_stats;
> >


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

* [dpdk-stable] [PATCH v2] net/i40e: fix Rx packet statistics
  2021-09-26  7:57 [dpdk-stable] [PATCH] net/i40e: fix Rx packet statistics Alvin Zhang
  2021-09-27 16:00 ` [dpdk-stable] [dpdk-dev] " Kevin Traynor
@ 2021-09-28  3:21 ` Alvin Zhang
  2021-09-28  3:22 ` Alvin Zhang
  2 siblings, 0 replies; 10+ messages in thread
From: Alvin Zhang @ 2021-09-28  3:21 UTC (permalink / raw)
  To: beilei.xing, junfeng.guo, ktraynor; +Cc: Alvin Zhang, stable

Some packets are discarded by the NIC because they are larger than
the MTU, these packets should be counted as "RX error" instead of
"RX packet", for example:

  pkt1 = Ether()/IP()/Raw('x' * 1400)
  pkt2 = Ether()/IP()/Raw('x' * 1500)

  ---------------- Forward statistics for port 0 -----------------
  RX-packets: 2 RX-dropped: 0 RX-total: 2
  TX-packets: 1 TX-dropped: 0 TX-total: 1
  ----------------------------------------------------------------

  Here the packet pkt2 has been discarded, but still was counted
  by "RX-packets"

The register 'GL_RXERR1' can count above discarded packets.
This patch adds reading and calculation of the 'GL_RXERR1' counter
when reporting DPDK statistics.

Fixes: f4a91c38b4ad ("i40e: add extended stats")
Cc: stable@dpdk.org

Signed-off-by: Alvin Zhang <alvinx.zhang@intel.com>
---
 drivers/net/i40e/i40e_ethdev.c | 16 +++++++++++++---
 drivers/net/i40e/i40e_ethdev.h | 10 ++++++++++
 2 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 7a2a828..7a207b2 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -532,7 +532,7 @@ static int i40e_sw_tunnel_filter_insert(struct i40e_pf *pf,
 /* store statistics names and its offset in stats structure */
 struct rte_i40e_xstats_name_off {
 	char name[RTE_ETH_XSTATS_NAME_SIZE];
-	unsigned offset;
+	int offset;
 };
 
 static const struct rte_i40e_xstats_name_off rte_i40e_stats_strings[] = {
@@ -542,6 +542,8 @@ struct rte_i40e_xstats_name_off {
 	{"rx_dropped_packets", offsetof(struct i40e_eth_stats, rx_discards)},
 	{"rx_unknown_protocol_packets", offsetof(struct i40e_eth_stats,
 		rx_unknown_protocol)},
+	{"rx_size_error_packets", offsetof(struct i40e_pf, rx_err1) -
+				  offsetof(struct i40e_pf, stats)},
 	{"tx_unicast_packets", offsetof(struct i40e_eth_stats, tx_unicast)},
 	{"tx_multicast_packets", offsetof(struct i40e_eth_stats, tx_multicast)},
 	{"tx_broadcast_packets", offsetof(struct i40e_eth_stats, tx_broadcast)},
@@ -3238,6 +3240,10 @@ void i40e_flex_payload_reg_set_default(struct i40e_hw *hw)
 			    pf->offset_loaded,
 			    &os->eth.rx_unknown_protocol,
 			    &ns->eth.rx_unknown_protocol);
+	i40e_stat_update_48(hw, I40E_GL_RXERR1_H(hw->pf_id + I40E_MAX_VF),
+			    I40E_GL_RXERR1_L(hw->pf_id + I40E_MAX_VF),
+			    pf->offset_loaded, &pf->rx_err1_offset,
+			    &pf->rx_err1);
 	i40e_stat_update_48_in_64(hw, I40E_GLPRT_GOTCH(hw->port),
 				  I40E_GLPRT_GOTCL(hw->port),
 				  pf->offset_loaded, &os->eth.tx_bytes,
@@ -3437,7 +3443,8 @@ void i40e_flex_payload_reg_set_default(struct i40e_hw *hw)
 	stats->ipackets = pf->main_vsi->eth_stats.rx_unicast +
 			pf->main_vsi->eth_stats.rx_multicast +
 			pf->main_vsi->eth_stats.rx_broadcast -
-			pf->main_vsi->eth_stats.rx_discards;
+			pf->main_vsi->eth_stats.rx_discards -
+			pf->rx_err1;
 	stats->opackets = ns->eth.tx_unicast +
 			ns->eth.tx_multicast +
 			ns->eth.tx_broadcast;
@@ -3451,7 +3458,8 @@ void i40e_flex_payload_reg_set_default(struct i40e_hw *hw)
 			pf->main_vsi->eth_stats.rx_discards;
 	stats->ierrors  = ns->crc_errors +
 			ns->rx_length_errors + ns->rx_undersize +
-			ns->rx_oversize + ns->rx_fragments + ns->rx_jabber;
+			ns->rx_oversize + ns->rx_fragments + ns->rx_jabber +
+			pf->rx_err1;
 
 	if (pf->vfs) {
 		for (i = 0; i < pf->vf_num; i++) {
@@ -6232,6 +6240,8 @@ struct i40e_vsi *
 	memset(&pf->stats_offset, 0, sizeof(struct i40e_hw_port_stats));
 	memset(&pf->internal_stats, 0, sizeof(struct i40e_eth_stats));
 	memset(&pf->internal_stats_offset, 0, sizeof(struct i40e_eth_stats));
+	pf->rx_err1 = 0;
+	pf->rx_err1_offset = 0;
 
 	ret = i40e_pf_get_switch_config(pf);
 	if (ret != I40E_SUCCESS) {
diff --git a/drivers/net/i40e/i40e_ethdev.h b/drivers/net/i40e/i40e_ethdev.h
index cd6deab..846c8d4 100644
--- a/drivers/net/i40e/i40e_ethdev.h
+++ b/drivers/net/i40e/i40e_ethdev.h
@@ -19,6 +19,13 @@
 #include "base/i40e_type.h"
 #include "base/virtchnl.h"
 
+#define I40E_GL_RXERR1_H(_i)	(0x00318004 + ((_i) * 8))
+/**
+ * _i=0...143,
+ * counters 0-127 are for the 128 VFs,
+ * counters 128-143 are for the 16 PFs
+ */
+
 #define I40E_VLAN_TAG_SIZE        4
 
 #define I40E_AQ_LEN               32
@@ -1134,6 +1141,9 @@ struct i40e_pf {
 
 	struct i40e_hw_port_stats stats_offset;
 	struct i40e_hw_port_stats stats;
+	u64 rx_err1;	/* rxerr1 */
+	u64 rx_err1_offset;
+
 	/* internal packet statistics, it should be excluded from the total */
 	struct i40e_eth_stats internal_stats_offset;
 	struct i40e_eth_stats internal_stats;
-- 
1.8.3.1


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

* [dpdk-stable] [PATCH v2] net/i40e: fix Rx packet statistics
  2021-09-26  7:57 [dpdk-stable] [PATCH] net/i40e: fix Rx packet statistics Alvin Zhang
  2021-09-27 16:00 ` [dpdk-stable] [dpdk-dev] " Kevin Traynor
  2021-09-28  3:21 ` [dpdk-stable] [PATCH v2] " Alvin Zhang
@ 2021-09-28  3:22 ` Alvin Zhang
  2021-09-28 11:16   ` [dpdk-stable] [dpdk-dev] " Zhang, Qi Z
  2021-09-30  5:19   ` [dpdk-stable] [PATCH v3] " Alvin Zhang
  2 siblings, 2 replies; 10+ messages in thread
From: Alvin Zhang @ 2021-09-28  3:22 UTC (permalink / raw)
  To: beilei.xing, junfeng.guo, ktraynor; +Cc: dev, Alvin Zhang, stable

Some packets are discarded by the NIC because they are larger than
the MTU, these packets should be counted as "RX error" instead of
"RX packet", for example:

  pkt1 = Ether()/IP()/Raw('x' * 1400)
  pkt2 = Ether()/IP()/Raw('x' * 1500)

  ---------------- Forward statistics for port 0 -----------------
  RX-packets: 2 RX-dropped: 0 RX-total: 2
  TX-packets: 1 TX-dropped: 0 TX-total: 1
  ----------------------------------------------------------------

  Here the packet pkt2 has been discarded, but still was counted
  by "RX-packets"

The register 'GL_RXERR1' can count above discarded packets.
This patch adds reading and calculation of the 'GL_RXERR1' counter
when reporting DPDK statistics.

Fixes: f4a91c38b4ad ("i40e: add extended stats")
Cc: stable@dpdk.org

Signed-off-by: Alvin Zhang <alvinx.zhang@intel.com>
---
 drivers/net/i40e/i40e_ethdev.c | 16 +++++++++++++---
 drivers/net/i40e/i40e_ethdev.h | 10 ++++++++++
 2 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 7a2a828..7a207b2 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -532,7 +532,7 @@ static int i40e_sw_tunnel_filter_insert(struct i40e_pf *pf,
 /* store statistics names and its offset in stats structure */
 struct rte_i40e_xstats_name_off {
 	char name[RTE_ETH_XSTATS_NAME_SIZE];
-	unsigned offset;
+	int offset;
 };
 
 static const struct rte_i40e_xstats_name_off rte_i40e_stats_strings[] = {
@@ -542,6 +542,8 @@ struct rte_i40e_xstats_name_off {
 	{"rx_dropped_packets", offsetof(struct i40e_eth_stats, rx_discards)},
 	{"rx_unknown_protocol_packets", offsetof(struct i40e_eth_stats,
 		rx_unknown_protocol)},
+	{"rx_size_error_packets", offsetof(struct i40e_pf, rx_err1) -
+				  offsetof(struct i40e_pf, stats)},
 	{"tx_unicast_packets", offsetof(struct i40e_eth_stats, tx_unicast)},
 	{"tx_multicast_packets", offsetof(struct i40e_eth_stats, tx_multicast)},
 	{"tx_broadcast_packets", offsetof(struct i40e_eth_stats, tx_broadcast)},
@@ -3238,6 +3240,10 @@ void i40e_flex_payload_reg_set_default(struct i40e_hw *hw)
 			    pf->offset_loaded,
 			    &os->eth.rx_unknown_protocol,
 			    &ns->eth.rx_unknown_protocol);
+	i40e_stat_update_48(hw, I40E_GL_RXERR1_H(hw->pf_id + I40E_MAX_VF),
+			    I40E_GL_RXERR1_L(hw->pf_id + I40E_MAX_VF),
+			    pf->offset_loaded, &pf->rx_err1_offset,
+			    &pf->rx_err1);
 	i40e_stat_update_48_in_64(hw, I40E_GLPRT_GOTCH(hw->port),
 				  I40E_GLPRT_GOTCL(hw->port),
 				  pf->offset_loaded, &os->eth.tx_bytes,
@@ -3437,7 +3443,8 @@ void i40e_flex_payload_reg_set_default(struct i40e_hw *hw)
 	stats->ipackets = pf->main_vsi->eth_stats.rx_unicast +
 			pf->main_vsi->eth_stats.rx_multicast +
 			pf->main_vsi->eth_stats.rx_broadcast -
-			pf->main_vsi->eth_stats.rx_discards;
+			pf->main_vsi->eth_stats.rx_discards -
+			pf->rx_err1;
 	stats->opackets = ns->eth.tx_unicast +
 			ns->eth.tx_multicast +
 			ns->eth.tx_broadcast;
@@ -3451,7 +3458,8 @@ void i40e_flex_payload_reg_set_default(struct i40e_hw *hw)
 			pf->main_vsi->eth_stats.rx_discards;
 	stats->ierrors  = ns->crc_errors +
 			ns->rx_length_errors + ns->rx_undersize +
-			ns->rx_oversize + ns->rx_fragments + ns->rx_jabber;
+			ns->rx_oversize + ns->rx_fragments + ns->rx_jabber +
+			pf->rx_err1;
 
 	if (pf->vfs) {
 		for (i = 0; i < pf->vf_num; i++) {
@@ -6232,6 +6240,8 @@ struct i40e_vsi *
 	memset(&pf->stats_offset, 0, sizeof(struct i40e_hw_port_stats));
 	memset(&pf->internal_stats, 0, sizeof(struct i40e_eth_stats));
 	memset(&pf->internal_stats_offset, 0, sizeof(struct i40e_eth_stats));
+	pf->rx_err1 = 0;
+	pf->rx_err1_offset = 0;
 
 	ret = i40e_pf_get_switch_config(pf);
 	if (ret != I40E_SUCCESS) {
diff --git a/drivers/net/i40e/i40e_ethdev.h b/drivers/net/i40e/i40e_ethdev.h
index cd6deab..846c8d4 100644
--- a/drivers/net/i40e/i40e_ethdev.h
+++ b/drivers/net/i40e/i40e_ethdev.h
@@ -19,6 +19,13 @@
 #include "base/i40e_type.h"
 #include "base/virtchnl.h"
 
+#define I40E_GL_RXERR1_H(_i)	(0x00318004 + ((_i) * 8))
+/**
+ * _i=0...143,
+ * counters 0-127 are for the 128 VFs,
+ * counters 128-143 are for the 16 PFs
+ */
+
 #define I40E_VLAN_TAG_SIZE        4
 
 #define I40E_AQ_LEN               32
@@ -1134,6 +1141,9 @@ struct i40e_pf {
 
 	struct i40e_hw_port_stats stats_offset;
 	struct i40e_hw_port_stats stats;
+	u64 rx_err1;	/* rxerr1 */
+	u64 rx_err1_offset;
+
 	/* internal packet statistics, it should be excluded from the total */
 	struct i40e_eth_stats internal_stats_offset;
 	struct i40e_eth_stats internal_stats;
-- 
1.8.3.1


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

* Re: [dpdk-stable] [dpdk-dev] [PATCH] net/i40e: fix Rx packet statistics
  2021-09-28  2:12   ` Zhang, AlvinX
@ 2021-09-28  8:53     ` Kevin Traynor
  0 siblings, 0 replies; 10+ messages in thread
From: Kevin Traynor @ 2021-09-28  8:53 UTC (permalink / raw)
  To: Zhang, AlvinX, Xing, Beilei, Guo, Junfeng
  Cc: dev, stable, Zhang, Qi Z, Yigit, Ferruh

On 28/09/2021 03:12, Zhang, AlvinX wrote:
>> -----Original Message-----
>> From: Kevin Traynor <ktraynor@redhat.com>
>> Sent: Tuesday, September 28, 2021 12:00 AM
>> To: Zhang, AlvinX <alvinx.zhang@intel.com>; Xing, Beilei
>> <beilei.xing@intel.com>; Guo, Junfeng <junfeng.guo@intel.com>
>> Cc: dev@dpdk.org; stable@dpdk.org; Zhang, Qi Z <qi.z.zhang@intel.com>;
>> Yigit, Ferruh <ferruh.yigit@intel.com>
>> Subject: Re: [dpdk-dev] [PATCH] net/i40e: fix Rx packet statistics
>>
>> On 26/09/2021 08:57, Alvin Zhang wrote:
>>> Some packets are discarded by the NIC because they are larger than the
>>> MTU, these packets should be counted as "RX error" instead of "RX
>>> packet".
>>>
>>> The register 'GL_RXERR1' can count above discarded packets.
>>> This patch adds reading and calculation of the 'GL_RXERR1' counter
>>> when reporting DPDK statistics.
>>>
>>> Fixes: f4a91c38b4ad ("i40e: add extended stats")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Alvin Zhang <alvinx.zhang@intel.com>
>>> ---
>>>    drivers/net/i40e/i40e_ethdev.c | 16 +++++++++++++---
>>>    drivers/net/i40e/i40e_ethdev.h | 10 ++++++++++
>>>    2 files changed, 23 insertions(+), 3 deletions(-)
>>>
>>
>> It's a bit hard to understand the code for someone not familiar with the i40e
>> stats. I think it needs careful review from i40e maintainers. A few questions
>> below,
>>
>> Did you test this with testpmd? Can you show an example of a test where these
>> packets are now correctly accounted for?
> 
> The issue as below:
> 
> sendp(Ether()/IP()/Raw('x' * 1500), iface="enp24s0f1")
> ---------------------- Forward statistics for port 0 ----------------------
> RX-packets: 1 RX-dropped: 0 RX-total: 1
> TX-packets: 0 TX-dropped: 0 TX-total: 0
> ----------------------------------------------------------------------------
> 
> Although we didn't really got the packet, but the statistic indicates a packet has been received successfully.
> We can add above example to commit log in V2.
> 

Hi Alvin. Thanks for answering my questions and showing how it was 
tested. I don't have any more comments. I won't ack just because I am 
not familiar enough with the i40e stats in general to give a meaningful ack.

Kevin.

>>
>> I see there is also an RXERR2 register that catches other errors, does it need to
>> be considered as well?
> 
> We are not sure whether the packets counted by RXERR2 also will be counted by "Rx-packets".
> So in this patch we only consider RXERR1 and fix the issue mentioned above.
> 
>>
>>> diff --git a/drivers/net/i40e/i40e_ethdev.c
>>> b/drivers/net/i40e/i40e_ethdev.c index 7a2a828..30a2cdf 100644
>>> --- a/drivers/net/i40e/i40e_ethdev.c
>>> +++ b/drivers/net/i40e/i40e_ethdev.c
>>> @@ -532,7 +532,7 @@ static int i40e_sw_tunnel_filter_insert(struct i40e_pf
>> *pf,
>>>    /* store statistics names and its offset in stats structure */
>>>    struct rte_i40e_xstats_name_off {
>>>    	char name[RTE_ETH_XSTATS_NAME_SIZE];
>>> -	unsigned offset;
>>> +	int offset;
>>
>> It is unusual to see you changing an offset to an int. You are expecting negative
>> offsets?
>>
>>>    };
>>>
>>>    static const struct rte_i40e_xstats_name_off
>>> rte_i40e_stats_strings[] = { @@ -542,6 +542,8 @@ struct
>> rte_i40e_xstats_name_off {
>>>    	{"rx_dropped_packets", offsetof(struct i40e_eth_stats, rx_discards)},
>>>    	{"rx_unknown_protocol_packets", offsetof(struct i40e_eth_stats,
>>>    		rx_unknown_protocol)},
>>> +	{"rx_err1", offsetof(struct i40e_pf, rx_err1) -
>>> +		    offsetof(struct i40e_pf, stats)},
> 
> Here offsetof(struct i40e_pf, rx_err1) - offsetof(struct i40e_pf, stats) may be a negative value.
> 
>>
>> rx_err1 is correct by datasheet but meaningless to a user. Suggest to find a more
>> descriptive name, or document what it is, or tell the user to reference the
>> datasheet.
> 
> I will update it in v2.
> 
>>
>>>    	{"tx_unicast_packets", offsetof(struct i40e_eth_stats, tx_unicast)},
>>>    	{"tx_multicast_packets", offsetof(struct i40e_eth_stats,
>> tx_multicast)},
>>>    	{"tx_broadcast_packets", offsetof(struct i40e_eth_stats,
>>> tx_broadcast)}, @@ -3238,6 +3240,10 @@ void
>> i40e_flex_payload_reg_set_default(struct i40e_hw *hw)
>>>    			    pf->offset_loaded,
>>>    			    &os->eth.rx_unknown_protocol,
>>>    			    &ns->eth.rx_unknown_protocol);
>>> +	i40e_stat_update_48(hw, I40E_GL_RXERR1_H(hw->pf_id +
>> I40E_MAX_VF),
>>> +			    I40E_GL_RXERR1_L(hw->pf_id + I40E_MAX_VF),
>>> +			    pf->offset_loaded, &pf->rx_err1_offset,
>>> +			    &pf->rx_err1);
>>>    	i40e_stat_update_48_in_64(hw, I40E_GLPRT_GOTCH(hw->port),
>>>    				  I40E_GLPRT_GOTCL(hw->port),
>>>    				  pf->offset_loaded, &os->eth.tx_bytes, @@ -3437,7
>> +3443,8 @@
>>> void i40e_flex_payload_reg_set_default(struct i40e_hw *hw)
>>>    	stats->ipackets = pf->main_vsi->eth_stats.rx_unicast +
>>>    			pf->main_vsi->eth_stats.rx_multicast +
>>>    			pf->main_vsi->eth_stats.rx_broadcast -
>>> -			pf->main_vsi->eth_stats.rx_discards;
>>> +			pf->main_vsi->eth_stats.rx_discards -
>>> +			pf->rx_err1;
>>>    	stats->opackets = ns->eth.tx_unicast +
>>>    			ns->eth.tx_multicast +
>>>    			ns->eth.tx_broadcast;
>>> @@ -3451,7 +3458,8 @@ void i40e_flex_payload_reg_set_default(struct
>> i40e_hw *hw)
>>>    			pf->main_vsi->eth_stats.rx_discards;
>>>    	stats->ierrors  = ns->crc_errors +
>>>    			ns->rx_length_errors + ns->rx_undersize +
>>> -			ns->rx_oversize + ns->rx_fragments + ns->rx_jabber;
>>> +			ns->rx_oversize + ns->rx_fragments + ns->rx_jabber +
>>> +			pf->rx_err1;
>>>
>>>    	if (pf->vfs) {
>>>    		for (i = 0; i < pf->vf_num; i++) { @@ -6232,6 +6240,8 @@ struct
>>> i40e_vsi *
>>>    	memset(&pf->stats_offset, 0, sizeof(struct i40e_hw_port_stats));
>>>    	memset(&pf->internal_stats, 0, sizeof(struct i40e_eth_stats));
>>>    	memset(&pf->internal_stats_offset, 0, sizeof(struct
>>> i40e_eth_stats));
>>> +	pf->rx_err1 = 0;
>>> +	pf->rx_err1_offset = 0;
>>>
>>>    	ret = i40e_pf_get_switch_config(pf);
>>>    	if (ret != I40E_SUCCESS) {
>>> diff --git a/drivers/net/i40e/i40e_ethdev.h
>>> b/drivers/net/i40e/i40e_ethdev.h index cd6deab..846c8d4 100644
>>> --- a/drivers/net/i40e/i40e_ethdev.h
>>> +++ b/drivers/net/i40e/i40e_ethdev.h
>>> @@ -19,6 +19,13 @@
>>>    #include "base/i40e_type.h"
>>>    #include "base/virtchnl.h"
>>>
>>> +#define I40E_GL_RXERR1_H(_i)	(0x00318004 + ((_i) * 8))
>>> +/**
>>> + * _i=0...143,
>>> + * counters 0-127 are for the 128 VFs,
>>> + * counters 128-143 are for the 16 PFs  */
>>> +
>>>    #define I40E_VLAN_TAG_SIZE        4
>>>
>>>    #define I40E_AQ_LEN               32
>>> @@ -1134,6 +1141,9 @@ struct i40e_pf {
>>>
>>>    	struct i40e_hw_port_stats stats_offset;
>>>    	struct i40e_hw_port_stats stats;
>>> +	u64 rx_err1;	/* rxerr1 */
>>> +	u64 rx_err1_offset;
>>> +
>>>    	/* internal packet statistics, it should be excluded from the total */
>>>    	struct i40e_eth_stats internal_stats_offset;
>>>    	struct i40e_eth_stats internal_stats;
>>>
> 


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

* Re: [dpdk-stable] [dpdk-dev] [PATCH v2] net/i40e: fix Rx packet statistics
  2021-09-28  3:22 ` Alvin Zhang
@ 2021-09-28 11:16   ` Zhang, Qi Z
  2021-09-29  1:33     ` Zhang, AlvinX
  2021-09-30  5:19   ` [dpdk-stable] [PATCH v3] " Alvin Zhang
  1 sibling, 1 reply; 10+ messages in thread
From: Zhang, Qi Z @ 2021-09-28 11:16 UTC (permalink / raw)
  To: Zhang, AlvinX, Xing, Beilei, Guo, Junfeng, ktraynor
  Cc: dev, Zhang, AlvinX, stable



> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Alvin Zhang
> Sent: Tuesday, September 28, 2021 11:23 AM
> To: Xing, Beilei <beilei.xing@intel.com>; Guo, Junfeng
> <junfeng.guo@intel.com>; ktraynor@redhat.com
> Cc: dev@dpdk.org; Zhang, AlvinX <alvinx.zhang@intel.com>; stable@dpdk.org
> Subject: [dpdk-dev] [PATCH v2] net/i40e: fix Rx packet statistics
> 
> Some packets are discarded by the NIC because they are larger than the MTU,
> these packets should be counted as "RX error" instead of "RX packet", for
> example:
> 
>   pkt1 = Ether()/IP()/Raw('x' * 1400)
>   pkt2 = Ether()/IP()/Raw('x' * 1500)
> 
>   ---------------- Forward statistics for port 0 -----------------
>   RX-packets: 2 RX-dropped: 0 RX-total: 2
>   TX-packets: 1 TX-dropped: 0 TX-total: 1
>   ----------------------------------------------------------------
> 
>   Here the packet pkt2 has been discarded, but still was counted
>   by "RX-packets"
> 
> The register 'GL_RXERR1' can count above discarded packets.
> This patch adds reading and calculation of the 'GL_RXERR1' counter when
> reporting DPDK statistics.
> 
> Fixes: f4a91c38b4ad ("i40e: add extended stats")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Alvin Zhang <alvinx.zhang@intel.com>
> ---
>  drivers/net/i40e/i40e_ethdev.c | 16 +++++++++++++---
> drivers/net/i40e/i40e_ethdev.h | 10 ++++++++++
>  2 files changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
> index 7a2a828..7a207b2 100644
> --- a/drivers/net/i40e/i40e_ethdev.c
> +++ b/drivers/net/i40e/i40e_ethdev.c
> @@ -532,7 +532,7 @@ static int i40e_sw_tunnel_filter_insert(struct i40e_pf
> *pf,
>  /* store statistics names and its offset in stats structure */  struct
> rte_i40e_xstats_name_off {
>  	char name[RTE_ETH_XSTATS_NAME_SIZE];
> -	unsigned offset;
> +	int offset;
>  };
> 
>  static const struct rte_i40e_xstats_name_off rte_i40e_stats_strings[] = { @@
> -542,6 +542,8 @@ struct rte_i40e_xstats_name_off {
>  	{"rx_dropped_packets", offsetof(struct i40e_eth_stats, rx_discards)},
>  	{"rx_unknown_protocol_packets", offsetof(struct i40e_eth_stats,
>  		rx_unknown_protocol)},
> +	{"rx_size_error_packets", offsetof(struct i40e_pf, rx_err1) -
> +				  offsetof(struct i40e_pf, stats)},
>  	{"tx_unicast_packets", offsetof(struct i40e_eth_stats, tx_unicast)},
>  	{"tx_multicast_packets", offsetof(struct i40e_eth_stats, tx_multicast)},
>  	{"tx_broadcast_packets", offsetof(struct i40e_eth_stats, tx_broadcast)},
> @@ -3238,6 +3240,10 @@ void i40e_flex_payload_reg_set_default(struct
> i40e_hw *hw)
>  			    pf->offset_loaded,
>  			    &os->eth.rx_unknown_protocol,
>  			    &ns->eth.rx_unknown_protocol);
> +	i40e_stat_update_48(hw, I40E_GL_RXERR1_H(hw->pf_id +
> I40E_MAX_VF),
> +			    I40E_GL_RXERR1_L(hw->pf_id + I40E_MAX_VF),
> +			    pf->offset_loaded, &pf->rx_err1_offset,
> +			    &pf->rx_err1);
>  	i40e_stat_update_48_in_64(hw, I40E_GLPRT_GOTCH(hw->port),
>  				  I40E_GLPRT_GOTCL(hw->port),
>  				  pf->offset_loaded, &os->eth.tx_bytes, @@ -3437,7
> +3443,8 @@ void i40e_flex_payload_reg_set_default(struct i40e_hw *hw)
>  	stats->ipackets = pf->main_vsi->eth_stats.rx_unicast +
>  			pf->main_vsi->eth_stats.rx_multicast +
>  			pf->main_vsi->eth_stats.rx_broadcast -
> -			pf->main_vsi->eth_stats.rx_discards;
> +			pf->main_vsi->eth_stats.rx_discards -
> +			pf->rx_err1;
>  	stats->opackets = ns->eth.tx_unicast +
>  			ns->eth.tx_multicast +
>  			ns->eth.tx_broadcast;
> @@ -3451,7 +3458,8 @@ void i40e_flex_payload_reg_set_default(struct
> i40e_hw *hw)
>  			pf->main_vsi->eth_stats.rx_discards;
>  	stats->ierrors  = ns->crc_errors +
>  			ns->rx_length_errors + ns->rx_undersize +
> -			ns->rx_oversize + ns->rx_fragments + ns->rx_jabber;
> +			ns->rx_oversize + ns->rx_fragments + ns->rx_jabber +
> +			pf->rx_err1;
> 
>  	if (pf->vfs) {
>  		for (i = 0; i < pf->vf_num; i++) {
> @@ -6232,6 +6240,8 @@ struct i40e_vsi *
>  	memset(&pf->stats_offset, 0, sizeof(struct i40e_hw_port_stats));
>  	memset(&pf->internal_stats, 0, sizeof(struct i40e_eth_stats));
>  	memset(&pf->internal_stats_offset, 0, sizeof(struct i40e_eth_stats));
> +	pf->rx_err1 = 0;
> +	pf->rx_err1_offset = 0;
> 
>  	ret = i40e_pf_get_switch_config(pf);
>  	if (ret != I40E_SUCCESS) {
> diff --git a/drivers/net/i40e/i40e_ethdev.h b/drivers/net/i40e/i40e_ethdev.h
> index cd6deab..846c8d4 100644
> --- a/drivers/net/i40e/i40e_ethdev.h
> +++ b/drivers/net/i40e/i40e_ethdev.h
> @@ -19,6 +19,13 @@
>  #include "base/i40e_type.h"
>  #include "base/virtchnl.h"
> 
> +#define I40E_GL_RXERR1_H(_i)	(0x00318004 + ((_i) * 8))
> +/**
> + * _i=0...143,
> + * counters 0-127 are for the 128 VFs,
> + * counters 128-143 are for the 16 PFs
> + */

I assume above comment is for I40E_GL_RXERR1_H, so it should above it? 

> +
>  #define I40E_VLAN_TAG_SIZE        4
> 
>  #define I40E_AQ_LEN               32
> @@ -1134,6 +1141,9 @@ struct i40e_pf {
> 
>  	struct i40e_hw_port_stats stats_offset;
>  	struct i40e_hw_port_stats stats;
> +	u64 rx_err1;	/* rxerr1 */
> +	u64 rx_err1_offset;
> +
>  	/* internal packet statistics, it should be excluded from the total */
>  	struct i40e_eth_stats internal_stats_offset;
>  	struct i40e_eth_stats internal_stats;
> --
> 1.8.3.1


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

* Re: [dpdk-stable] [dpdk-dev] [PATCH v2] net/i40e: fix Rx packet statistics
  2021-09-28 11:16   ` [dpdk-stable] [dpdk-dev] " Zhang, Qi Z
@ 2021-09-29  1:33     ` Zhang, AlvinX
  0 siblings, 0 replies; 10+ messages in thread
From: Zhang, AlvinX @ 2021-09-29  1:33 UTC (permalink / raw)
  To: Zhang, Qi Z, Xing, Beilei, Guo, Junfeng, ktraynor; +Cc: dev, stable

> -----Original Message-----
> From: Zhang, Qi Z <qi.z.zhang@intel.com>
> Sent: Tuesday, September 28, 2021 7:17 PM
> To: Zhang, AlvinX <alvinx.zhang@intel.com>; Xing, Beilei
> <beilei.xing@intel.com>; Guo, Junfeng <junfeng.guo@intel.com>;
> ktraynor@redhat.com
> Cc: dev@dpdk.org; Zhang, AlvinX <alvinx.zhang@intel.com>; stable@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH v2] net/i40e: fix Rx packet statistics
> 
> 
> 
> > -----Original Message-----
> > From: dev <dev-bounces@dpdk.org> On Behalf Of Alvin Zhang
> > Sent: Tuesday, September 28, 2021 11:23 AM
> > To: Xing, Beilei <beilei.xing@intel.com>; Guo, Junfeng
> > <junfeng.guo@intel.com>; ktraynor@redhat.com
> > Cc: dev@dpdk.org; Zhang, AlvinX <alvinx.zhang@intel.com>;
> > stable@dpdk.org
> > Subject: [dpdk-dev] [PATCH v2] net/i40e: fix Rx packet statistics
> >
> > Some packets are discarded by the NIC because they are larger than the
> > MTU, these packets should be counted as "RX error" instead of "RX
> > packet", for
> > example:
> >
> >   pkt1 = Ether()/IP()/Raw('x' * 1400)
> >   pkt2 = Ether()/IP()/Raw('x' * 1500)
> >
> >   ---------------- Forward statistics for port 0 -----------------
> >   RX-packets: 2 RX-dropped: 0 RX-total: 2
> >   TX-packets: 1 TX-dropped: 0 TX-total: 1
> >   ----------------------------------------------------------------
> >
> >   Here the packet pkt2 has been discarded, but still was counted
> >   by "RX-packets"
> >
> > The register 'GL_RXERR1' can count above discarded packets.
> > This patch adds reading and calculation of the 'GL_RXERR1' counter
> > when reporting DPDK statistics.
> >
> > Fixes: f4a91c38b4ad ("i40e: add extended stats")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Alvin Zhang <alvinx.zhang@intel.com>
> > ---
> >  drivers/net/i40e/i40e_ethdev.c | 16 +++++++++++++---
> > drivers/net/i40e/i40e_ethdev.h | 10 ++++++++++
> >  2 files changed, 23 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/i40e/i40e_ethdev.c
> > b/drivers/net/i40e/i40e_ethdev.c index 7a2a828..7a207b2 100644
> > --- a/drivers/net/i40e/i40e_ethdev.c
> > +++ b/drivers/net/i40e/i40e_ethdev.c
> > @@ -532,7 +532,7 @@ static int i40e_sw_tunnel_filter_insert(struct
> > i40e_pf *pf,
> >  /* store statistics names and its offset in stats structure */
> > struct rte_i40e_xstats_name_off {  char
> > name[RTE_ETH_XSTATS_NAME_SIZE]; -unsigned offset;
> > +int offset;
> >  };
> >
> >  static const struct rte_i40e_xstats_name_off rte_i40e_stats_strings[]
> > = { @@
> > -542,6 +542,8 @@ struct rte_i40e_xstats_name_off {
> > {"rx_dropped_packets", offsetof(struct i40e_eth_stats, rx_discards)},
> > {"rx_unknown_protocol_packets", offsetof(struct i40e_eth_stats,
> > rx_unknown_protocol)},
> > +{"rx_size_error_packets", offsetof(struct i40e_pf, rx_err1) -
> > +  offsetof(struct i40e_pf, stats)},
> >  {"tx_unicast_packets", offsetof(struct i40e_eth_stats, tx_unicast)},
> > {"tx_multicast_packets", offsetof(struct i40e_eth_stats,
> > tx_multicast)},  {"tx_broadcast_packets", offsetof(struct
> > i40e_eth_stats, tx_broadcast)}, @@ -3238,6 +3240,10 @@ void
> > i40e_flex_payload_reg_set_default(struct
> > i40e_hw *hw)
> >      pf->offset_loaded,
> >      &os->eth.rx_unknown_protocol,
> >      &ns->eth.rx_unknown_protocol);
> > +i40e_stat_update_48(hw, I40E_GL_RXERR1_H(hw->pf_id +
> > I40E_MAX_VF),
> > +    I40E_GL_RXERR1_L(hw->pf_id + I40E_MAX_VF),
> > +    pf->offset_loaded, &pf->rx_err1_offset,
> > +    &pf->rx_err1);
> >  i40e_stat_update_48_in_64(hw, I40E_GLPRT_GOTCH(hw->port),
> >    I40E_GLPRT_GOTCL(hw->port),
> >    pf->offset_loaded, &os->eth.tx_bytes, @@ -3437,7
> > +3443,8 @@ void i40e_flex_payload_reg_set_default(struct i40e_hw *hw)
> >  stats->ipackets = pf->main_vsi->eth_stats.rx_unicast +
> > pf->main_vsi->eth_stats.rx_multicast +
> > pf->main_vsi->eth_stats.rx_broadcast -
> > -pf->main_vsi->eth_stats.rx_discards;
> > +pf->main_vsi->eth_stats.rx_discards - rx_err1;
> >  stats->opackets = ns->eth.tx_unicast +  ns->eth.tx_multicast +
> > ns->eth.tx_broadcast; @@ -3451,7 +3458,8 @@ void
> > i40e_flex_payload_reg_set_default(struct
> > i40e_hw *hw)
> >  pf->main_vsi->eth_stats.rx_discards;
> >  stats->ierrors  = ns->crc_errors +
> >  ns->rx_length_errors + ns->rx_undersize +
> > -ns->rx_oversize + ns->rx_fragments + ns->rx_jabber;
> > +ns->rx_oversize + ns->rx_fragments + ns->rx_jabber +
> > +pf->rx_err1;
> >
> >  if (pf->vfs) {
> >  for (i = 0; i < pf->vf_num; i++) {
> > @@ -6232,6 +6240,8 @@ struct i40e_vsi *  memset(&pf->stats_offset, 0,
> > sizeof(struct i40e_hw_port_stats));  memset(&pf->internal_stats, 0,
> > sizeof(struct i40e_eth_stats));  memset(&pf->internal_stats_offset, 0,
> > sizeof(struct i40e_eth_stats));
> > +pf->rx_err1 = 0;
> > +pf->rx_err1_offset = 0;
> >
> >  ret = i40e_pf_get_switch_config(pf);
> >  if (ret != I40E_SUCCESS) {
> > diff --git a/drivers/net/i40e/i40e_ethdev.h
> > b/drivers/net/i40e/i40e_ethdev.h index cd6deab..846c8d4 100644
> > --- a/drivers/net/i40e/i40e_ethdev.h
> > +++ b/drivers/net/i40e/i40e_ethdev.h
> > @@ -19,6 +19,13 @@
> >  #include "base/i40e_type.h"
> >  #include "base/virtchnl.h"
> >
> > +#define I40E_GL_RXERR1_H(_i)(0x00318004 + ((_i) * 8))
> > +/**
> > + * _i=0...143,
> > + * counters 0-127 are for the 128 VFs,
> > + * counters 128-143 are for the 16 PFs  */
> 
> I assume above comment is for I40E_GL_RXERR1_H, so it should above it?

Ok, I will update it in v2. 
Thanks

> 
> > +
> >  #define I40E_VLAN_TAG_SIZE        4
> >
> >  #define I40E_AQ_LEN               32
> > @@ -1134,6 +1141,9 @@ struct i40e_pf {
> >
> >  struct i40e_hw_port_stats stats_offset;  struct i40e_hw_port_stats
> > stats;
> > +u64 rx_err1;/* rxerr1 */
> > +u64 rx_err1_offset;
> > +
> >  /* internal packet statistics, it should be excluded from the total
> > */  struct i40e_eth_stats internal_stats_offset;  struct
> > i40e_eth_stats internal_stats;
> > --
> > 1.8.3.1
> 


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

* [dpdk-stable] [PATCH v3] net/i40e: fix Rx packet statistics
  2021-09-28  3:22 ` Alvin Zhang
  2021-09-28 11:16   ` [dpdk-stable] [dpdk-dev] " Zhang, Qi Z
@ 2021-09-30  5:19   ` Alvin Zhang
  2021-10-08  6:17     ` Zhang, Qi Z
  1 sibling, 1 reply; 10+ messages in thread
From: Alvin Zhang @ 2021-09-30  5:19 UTC (permalink / raw)
  To: beilei.xing, junfeng.guo, ktraynor, qi.z.zhang; +Cc: dev, Alvin Zhang, stable

Some packets are discarded by the NIC because they are larger than
the MTU, these packets should be counted as "RX error" instead of
"RX packet", for example:

  pkt1 = Ether()/IP()/Raw('x' * 1400)
  pkt2 = Ether()/IP()/Raw('x' * 1500)

  ---------------- Forward statistics for port 0 -----------------
  RX-packets: 2 RX-dropped: 0 RX-total: 2
  TX-packets: 1 TX-dropped: 0 TX-total: 1
  ----------------------------------------------------------------

  Here the packet pkt2 has been discarded, but still was counted
  by "RX-packets"

The register 'GL_RXERR1' can count above discarded packets.
This patch adds reading and calculation of the 'GL_RXERR1' counter
when reporting DPDK statistics.

Fixes: f4a91c38b4ad ("i40e: add extended stats")
Cc: stable@dpdk.org

Signed-off-by: Alvin Zhang <alvinx.zhang@intel.com>
---

v3: update code notes according to comment.
---
 drivers/net/i40e/i40e_ethdev.c | 16 +++++++++++++---
 drivers/net/i40e/i40e_ethdev.h | 10 ++++++++++
 2 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 7a2a828..7a207b2 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -532,7 +532,7 @@ static int i40e_sw_tunnel_filter_insert(struct i40e_pf *pf,
 /* store statistics names and its offset in stats structure */
 struct rte_i40e_xstats_name_off {
 	char name[RTE_ETH_XSTATS_NAME_SIZE];
-	unsigned offset;
+	int offset;
 };
 
 static const struct rte_i40e_xstats_name_off rte_i40e_stats_strings[] = {
@@ -542,6 +542,8 @@ struct rte_i40e_xstats_name_off {
 	{"rx_dropped_packets", offsetof(struct i40e_eth_stats, rx_discards)},
 	{"rx_unknown_protocol_packets", offsetof(struct i40e_eth_stats,
 		rx_unknown_protocol)},
+	{"rx_size_error_packets", offsetof(struct i40e_pf, rx_err1) -
+				  offsetof(struct i40e_pf, stats)},
 	{"tx_unicast_packets", offsetof(struct i40e_eth_stats, tx_unicast)},
 	{"tx_multicast_packets", offsetof(struct i40e_eth_stats, tx_multicast)},
 	{"tx_broadcast_packets", offsetof(struct i40e_eth_stats, tx_broadcast)},
@@ -3238,6 +3240,10 @@ void i40e_flex_payload_reg_set_default(struct i40e_hw *hw)
 			    pf->offset_loaded,
 			    &os->eth.rx_unknown_protocol,
 			    &ns->eth.rx_unknown_protocol);
+	i40e_stat_update_48(hw, I40E_GL_RXERR1_H(hw->pf_id + I40E_MAX_VF),
+			    I40E_GL_RXERR1_L(hw->pf_id + I40E_MAX_VF),
+			    pf->offset_loaded, &pf->rx_err1_offset,
+			    &pf->rx_err1);
 	i40e_stat_update_48_in_64(hw, I40E_GLPRT_GOTCH(hw->port),
 				  I40E_GLPRT_GOTCL(hw->port),
 				  pf->offset_loaded, &os->eth.tx_bytes,
@@ -3437,7 +3443,8 @@ void i40e_flex_payload_reg_set_default(struct i40e_hw *hw)
 	stats->ipackets = pf->main_vsi->eth_stats.rx_unicast +
 			pf->main_vsi->eth_stats.rx_multicast +
 			pf->main_vsi->eth_stats.rx_broadcast -
-			pf->main_vsi->eth_stats.rx_discards;
+			pf->main_vsi->eth_stats.rx_discards -
+			pf->rx_err1;
 	stats->opackets = ns->eth.tx_unicast +
 			ns->eth.tx_multicast +
 			ns->eth.tx_broadcast;
@@ -3451,7 +3458,8 @@ void i40e_flex_payload_reg_set_default(struct i40e_hw *hw)
 			pf->main_vsi->eth_stats.rx_discards;
 	stats->ierrors  = ns->crc_errors +
 			ns->rx_length_errors + ns->rx_undersize +
-			ns->rx_oversize + ns->rx_fragments + ns->rx_jabber;
+			ns->rx_oversize + ns->rx_fragments + ns->rx_jabber +
+			pf->rx_err1;
 
 	if (pf->vfs) {
 		for (i = 0; i < pf->vf_num; i++) {
@@ -6232,6 +6240,8 @@ struct i40e_vsi *
 	memset(&pf->stats_offset, 0, sizeof(struct i40e_hw_port_stats));
 	memset(&pf->internal_stats, 0, sizeof(struct i40e_eth_stats));
 	memset(&pf->internal_stats_offset, 0, sizeof(struct i40e_eth_stats));
+	pf->rx_err1 = 0;
+	pf->rx_err1_offset = 0;
 
 	ret = i40e_pf_get_switch_config(pf);
 	if (ret != I40E_SUCCESS) {
diff --git a/drivers/net/i40e/i40e_ethdev.h b/drivers/net/i40e/i40e_ethdev.h
index cd6deab..8e14d9a 100644
--- a/drivers/net/i40e/i40e_ethdev.h
+++ b/drivers/net/i40e/i40e_ethdev.h
@@ -19,6 +19,13 @@
 #include "base/i40e_type.h"
 #include "base/virtchnl.h"
 
+/**
+ * _i=0...143,
+ * counters 0-127 are for the 128 VFs,
+ * counters 128-143 are for the 16 PFs
+ */
+#define I40E_GL_RXERR1_H(_i)	(0x00318004 + ((_i) * 8))
+
 #define I40E_VLAN_TAG_SIZE        4
 
 #define I40E_AQ_LEN               32
@@ -1134,6 +1141,9 @@ struct i40e_pf {
 
 	struct i40e_hw_port_stats stats_offset;
 	struct i40e_hw_port_stats stats;
+	u64 rx_err1;	/* rxerr1 */
+	u64 rx_err1_offset;
+
 	/* internal packet statistics, it should be excluded from the total */
 	struct i40e_eth_stats internal_stats_offset;
 	struct i40e_eth_stats internal_stats;
-- 
1.8.3.1


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

* Re: [dpdk-stable] [PATCH v3] net/i40e: fix Rx packet statistics
  2021-09-30  5:19   ` [dpdk-stable] [PATCH v3] " Alvin Zhang
@ 2021-10-08  6:17     ` Zhang, Qi Z
  0 siblings, 0 replies; 10+ messages in thread
From: Zhang, Qi Z @ 2021-10-08  6:17 UTC (permalink / raw)
  To: Zhang, AlvinX, Xing, Beilei, Guo, Junfeng, ktraynor; +Cc: dev, stable



> -----Original Message-----
> From: Zhang, AlvinX <alvinx.zhang@intel.com>
> Sent: Thursday, September 30, 2021 1:20 PM
> To: Xing, Beilei <beilei.xing@intel.com>; Guo, Junfeng
> <junfeng.guo@intel.com>; ktraynor@redhat.com; Zhang, Qi Z
> <qi.z.zhang@intel.com>
> Cc: dev@dpdk.org; Zhang, AlvinX <alvinx.zhang@intel.com>; stable@dpdk.org
> Subject: [PATCH v3] net/i40e: fix Rx packet statistics
> 
> Some packets are discarded by the NIC because they are larger than the MTU,
> these packets should be counted as "RX error" instead of "RX packet", for
> example:
> 
>   pkt1 = Ether()/IP()/Raw('x' * 1400)
>   pkt2 = Ether()/IP()/Raw('x' * 1500)
> 
>   ---------------- Forward statistics for port 0 -----------------
>   RX-packets: 2 RX-dropped: 0 RX-total: 2
>   TX-packets: 1 TX-dropped: 0 TX-total: 1
>   ----------------------------------------------------------------
> 
>   Here the packet pkt2 has been discarded, but still was counted
>   by "RX-packets"
> 
> The register 'GL_RXERR1' can count above discarded packets.
> This patch adds reading and calculation of the 'GL_RXERR1' counter when
> reporting DPDK statistics.
> 
> Fixes: f4a91c38b4ad ("i40e: add extended stats")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Alvin Zhang <alvinx.zhang@intel.com>

Acked-by: Qi Zhang <qi.z.zhang@intel.com>

Applied to dpdk-next-net-intel.

Thanks
Qi


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

end of thread, other threads:[~2021-10-08  6:17 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-26  7:57 [dpdk-stable] [PATCH] net/i40e: fix Rx packet statistics Alvin Zhang
2021-09-27 16:00 ` [dpdk-stable] [dpdk-dev] " Kevin Traynor
2021-09-28  2:12   ` Zhang, AlvinX
2021-09-28  8:53     ` Kevin Traynor
2021-09-28  3:21 ` [dpdk-stable] [PATCH v2] " Alvin Zhang
2021-09-28  3:22 ` Alvin Zhang
2021-09-28 11:16   ` [dpdk-stable] [dpdk-dev] " Zhang, Qi Z
2021-09-29  1:33     ` Zhang, AlvinX
2021-09-30  5:19   ` [dpdk-stable] [PATCH v3] " Alvin Zhang
2021-10-08  6:17     ` Zhang, Qi Z

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).