On 28/09/2021 03:12, Zhang, AlvinX wrote:

quoted

-----Original Message-----
From: Kevin Traynor <redacted>
Sent: Tuesday, September 28, 2021 12:00 AM
To: Zhang, AlvinX <redacted>; Xing, Beilei
[off-list ref]; Guo, Junfeng [off-list ref]
Cc: dev@dpdk.org; stable@dpdk.org; Zhang, Qi Z <redacted>;
Yigit, Ferruh [off-list ref]
Subject: Re: [dpdk-dev] [PATCH] net/i40e: fix Rx packet statistics

On 26/09/2021 08:57, Alvin Zhang wrote:

quoted

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

quoted

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.

quoted

quoted

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,

quoted

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

quoted

   };

   static const struct rte_i40e_xstats_name_off
rte_i40e_stats_strings[] = { @@ -542,6 +542,8 @@ struct

rte_i40e_xstats_name_off {

quoted

   	{"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.

quoted

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.

quoted

quoted

   	{"tx_unicast_packets", offsetof(struct i40e_eth_stats, tx_unicast)},
   	{"tx_multicast_packets", offsetof(struct i40e_eth_stats,

tx_multicast)},

quoted

   	{"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)

quoted

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

quoted

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

quoted

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)

quoted

   			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;