diff mbox

[dpdk-dev] ethdev: don't count missed packets in erroneous packets counter

Message ID 1457615010-87436-1-git-send-email-iryzhov@nfware.com (mailing list archive)
State Accepted, archived
Delegated to: Thomas Monjalon
Headers show

Commit Message

Igor Ryzhov March 10, 2016, 1:03 p.m. UTC
Comment for "ierrors" counter says that it counts erroneous received packets. But for some reason "imissed" counter is added to "ierrors" counter in most drivers. It is a mistake, because missed packets are obviously not received. This patch fixes it.

Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
---
 app/test-pmd/testpmd.c           | 4 ++--
 drivers/net/cxgbe/cxgbe_ethdev.c | 2 +-
 drivers/net/e1000/em_ethdev.c    | 1 -
 drivers/net/e1000/igb_ethdev.c   | 1 -
 drivers/net/i40e/i40e_ethdev.c   | 3 +--
 drivers/net/ixgbe/ixgbe_ethdev.c | 1 -
 6 files changed, 4 insertions(+), 8 deletions(-)

Comments

Igor Ryzhov March 14, 2016, 7:30 a.m. UTC | #1
Ping.

CCing to maintainers of affected drivers.

> 10 марта 2016 г., в 16:03, Igor Ryzhov <iryzhov@nfware.com> написал(а):
> 
> Comment for "ierrors" counter says that it counts erroneous received packets. But for some reason "imissed" counter is added to "ierrors" counter in most drivers. It is a mistake, because missed packets are obviously not received. This patch fixes it.
> 
> Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
> ---
> app/test-pmd/testpmd.c           | 4 ++--
> drivers/net/cxgbe/cxgbe_ethdev.c | 2 +-
> drivers/net/e1000/em_ethdev.c    | 1 -
> drivers/net/e1000/igb_ethdev.c   | 1 -
> drivers/net/i40e/i40e_ethdev.c   | 3 +--
> drivers/net/ixgbe/ixgbe_ethdev.c | 1 -
> 6 files changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index 269ef81..d3d733b 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -753,7 +753,7 @@ fwd_port_stats_display(portid_t port_id, struct rte_eth_stats *stats)
> 		if (cur_fwd_eng == &csum_fwd_engine)
> 			printf("  Bad-ipcsum: %-14"PRIu64" Bad-l4csum: %-14"PRIu64" \n",
> 			       port->rx_bad_ip_csum, port->rx_bad_l4_csum);
> -		if (((stats->ierrors - stats->imissed) + stats->rx_nombuf) > 0) {
> +		if ((stats->ierrors + stats->rx_nombuf) > 0) {
> 			printf("  RX-error: %-"PRIu64"\n",  stats->ierrors);
> 			printf("  RX-nombufs: %-14"PRIu64"\n", stats->rx_nombuf);
> 		}
> @@ -772,7 +772,7 @@ fwd_port_stats_display(portid_t port_id, struct rte_eth_stats *stats)
> 		if (cur_fwd_eng == &csum_fwd_engine)
> 			printf("  Bad-ipcsum:%14"PRIu64"    Bad-l4csum:%14"PRIu64"\n",
> 			       port->rx_bad_ip_csum, port->rx_bad_l4_csum);
> -		if (((stats->ierrors - stats->imissed) + stats->rx_nombuf) > 0) {
> +		if ((stats->ierrors + stats->rx_nombuf) > 0) {
> 			printf("  RX-error:%"PRIu64"\n", stats->ierrors);
> 			printf("  RX-nombufs:             %14"PRIu64"\n",
> 			       stats->rx_nombuf);
> diff --git a/drivers/net/cxgbe/cxgbe_ethdev.c b/drivers/net/cxgbe/cxgbe_ethdev.c
> index 97ef152..0070e2a 100644
> --- a/drivers/net/cxgbe/cxgbe_ethdev.c
> +++ b/drivers/net/cxgbe/cxgbe_ethdev.c
> @@ -662,7 +662,7 @@ static void cxgbe_dev_stats_get(struct rte_eth_dev *eth_dev,
> 			      ps.rx_trunc2 + ps.rx_trunc3;
> 	eth_stats->ierrors  = ps.rx_symbol_err + ps.rx_fcs_err +
> 			      ps.rx_jabber + ps.rx_too_long + ps.rx_runt +
> -			      ps.rx_len_err + eth_stats->imissed;
> +			      ps.rx_len_err;
> 
> 	/* TX Stats */
> 	eth_stats->opackets = ps.tx_frames;
> diff --git a/drivers/net/e1000/em_ethdev.c b/drivers/net/e1000/em_ethdev.c
> index 4a843fe..27ace6d 100644
> --- a/drivers/net/e1000/em_ethdev.c
> +++ b/drivers/net/e1000/em_ethdev.c
> @@ -914,7 +914,6 @@ eth_em_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *rte_stats)
> 	rte_stats->imissed = stats->mpc;
> 	rte_stats->ierrors = stats->crcerrs +
> 	                     stats->rlec + stats->ruc + stats->roc +
> -	                     rte_stats->imissed +
> 	                     stats->rxerrc + stats->algnerrc + stats->cexterr;
> 
> 	/* Tx Errors */
> diff --git a/drivers/net/e1000/igb_ethdev.c b/drivers/net/e1000/igb_ethdev.c
> index 4ed5e95..6e93214 100644
> --- a/drivers/net/e1000/igb_ethdev.c
> +++ b/drivers/net/e1000/igb_ethdev.c
> @@ -1640,7 +1640,6 @@ eth_igb_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *rte_stats)
> 	rte_stats->imissed = stats->mpc;
> 	rte_stats->ierrors = stats->crcerrs +
> 	                     stats->rlec + stats->ruc + stats->roc +
> -	                     rte_stats->imissed +
> 	                     stats->rxerrc + stats->algnerrc + stats->cexterr;
> 
> 	/* Tx Errors */
> diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
> index 7e68c61..7d68d4d 100644
> --- a/drivers/net/i40e/i40e_ethdev.c
> +++ b/drivers/net/i40e/i40e_ethdev.c
> @@ -2062,8 +2062,7 @@ i40e_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
> 			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 +
> -			stats->imissed;
> +			ns->rx_oversize + ns->rx_fragments + ns->rx_jabber;
> 
> 	PMD_DRV_LOG(DEBUG, "***************** PF stats start *******************");
> 	PMD_DRV_LOG(DEBUG, "rx_bytes:            %"PRIu64"", ns->eth.rx_bytes);
> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
> index 3e6fe86..ba84544 100644
> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> @@ -2552,7 +2552,6 @@ ixgbe_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
> 	                  hw_stats->rlec +
> 	                  hw_stats->ruc +
> 	                  hw_stats->roc +
> -	                  total_missed_rx +
> 	                  hw_stats->illerrc +
> 	                  hw_stats->errbc +
> 	                  hw_stats->rfc +
> -- 
> 2.6.4
>
Wenzhuo Lu March 14, 2016, 8:26 a.m. UTC | #2
Hi,


> -----Original Message-----

> From: Igor Ryzhov [mailto:iryzhov@nfware.com]

> Sent: Monday, March 14, 2016 3:31 PM

> To: Игорь Рыжов

> Cc: dev@dpdk.org; Rahul Lakkireddy; Lu, Wenzhuo; Zhang, Helin; Ananyev,

> Konstantin

> Subject: Re: [dpdk-dev] [PATCH] ethdev: don't count missed packets in

> erroneous packets counter

> 

> Ping.

> 

> CCing to maintainers of affected drivers.

> 

> > 10 марта 2016 г., в 16:03, Igor Ryzhov <iryzhov@nfware.com> написал(а):

> >

> > Comment for "ierrors" counter says that it counts erroneous received packets.

> But for some reason "imissed" counter is added to "ierrors" counter in most

> drivers. It is a mistake, because missed packets are obviously not received. This

> patch fixes it.

> >

> > Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>

Acked-by: Wenzhuo Lu <wenzhuo.lu@intel.com>

Agree with Igor. As we have a counter for imissed, we need not to mix it with ierror.
Rahul Lakkireddy March 17, 2016, 12:02 p.m. UTC | #3
On Thursday, March 03/10/16, 2016 at 16:03:30 +0300, Igor Ryzhov wrote:
> Comment for "ierrors" counter says that it counts erroneous received packets. But for some reason "imissed" counter is added to "ierrors" counter in most drivers. It is a mistake, because missed packets are obviously not received. This patch fixes it.
> 
> Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
> ---

For the cxgbe part,
Acked-by: Rahul Lakkireddy <rahul.lakkireddy@chelsio.com>
Thomas Monjalon March 17, 2016, 4:40 p.m. UTC | #4
CC Maryam and Olivier who had discussions about imissed and other stats:
	http://dpdk.org/ml/archives/dev/2015-August/022905.html
	http://dpdk.org/ml/archives/dev/2015-September/023351.html
	http://dpdk.org/ml/archives/dev/2015-September/023612.html

2016-03-10 16:03, Igor Ryzhov:
> Comment for "ierrors" counter says that it counts erroneous received packets. But for some reason "imissed" counter is added to "ierrors" counter in most drivers. It is a mistake, because missed packets are obviously not received. This patch fixes it.

According to this patch
	http://dpdk.org/browse/dpdk/commit/?id=70bdb186
imissed was kept in ierrors because of backward compatibility.
I'm OK to remove imissed from ierrors.

Fixes: 70bdb18657da ("ethdev: add Rx error counters for missed, badcrc and badlen packets")
Fixes: 6bfe648406b5 ("i40e: add Rx error statistics")
Fixes: 856505d303f4 ("cxgbe: add port statistics")

Acked-by: Thomas Monjalon <thomas.monjalon@6wind.com>
Thomas Monjalon March 22, 2016, 10:06 a.m. UTC | #5
2016-03-17 17:40, Thomas Monjalon:
> CC Maryam and Olivier who had discussions about imissed and other stats:
> 	http://dpdk.org/ml/archives/dev/2015-August/022905.html
> 	http://dpdk.org/ml/archives/dev/2015-September/023351.html
> 	http://dpdk.org/ml/archives/dev/2015-September/023612.html
> 
> 2016-03-10 16:03, Igor Ryzhov:
> > Comment for "ierrors" counter says that it counts erroneous received packets. But for some reason "imissed" counter is added to "ierrors" counter in most drivers. It is a mistake, because missed packets are obviously not received. This patch fixes it.
> 
> According to this patch
> 	http://dpdk.org/browse/dpdk/commit/?id=70bdb186
> imissed was kept in ierrors because of backward compatibility.
> I'm OK to remove imissed from ierrors.
> 
> Fixes: 70bdb18657da ("ethdev: add Rx error counters for missed, badcrc and badlen packets")
> Fixes: 6bfe648406b5 ("i40e: add Rx error statistics")
> Fixes: 856505d303f4 ("cxgbe: add port statistics")
> 
> Acked-by: Thomas Monjalon <thomas.monjalon@6wind.com>

Applied, thanks
Tahhan, Maryam March 22, 2016, 3:23 p.m. UTC | #6
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Thursday, March 17, 2016 4:41 PM
> To: Igor Ryzhov <iryzhov@nfware.com>
> Cc: dev@dpdk.org; Tahhan, Maryam <maryam.tahhan@intel.com>;
> olivier.matz@6wind.com
> Subject: Re: [dpdk-dev] [PATCH] ethdev: don't count missed packets in
> erroneous packets counter
> 
> CC Maryam and Olivier who had discussions about imissed and other
> stats:
> 	http://dpdk.org/ml/archives/dev/2015-August/022905.html
> 	http://dpdk.org/ml/archives/dev/2015-September/023351.html
> 	http://dpdk.org/ml/archives/dev/2015-September/023612.html
> 
> 2016-03-10 16:03, Igor Ryzhov:
> > Comment for "ierrors" counter says that it counts erroneous received
> packets. But for some reason "imissed" counter is added to "ierrors"
> counter in most drivers. It is a mistake, because missed packets are
> obviously not received. This patch fixes it.
> 
> According to this patch
> 	http://dpdk.org/browse/dpdk/commit/?id=70bdb186
> imissed was kept in ierrors because of backward compatibility.
> I'm OK to remove imissed from ierrors.
> 
> Fixes: 70bdb18657da ("ethdev: add Rx error counters for missed, badcrc
> and badlen packets")
> Fixes: 6bfe648406b5 ("i40e: add Rx error statistics")
> Fixes: 856505d303f4 ("cxgbe: add port statistics")
> 
> Acked-by: Thomas Monjalon <thomas.monjalon@6wind.com>

Looks fine, but make sure to add an explicit comment in release notes somewhere to flag the change. In case any apps were accounting for imissed as part of ierrors like testpmd was: 

-		if (((stats->ierrors - stats->imissed) + stats->rx_nombuf) > 0) {
+		if ((stats->ierrors + stats->rx_nombuf) > 0) {
 			printf("  RX-error:%"PRIu64"\n", stats->ierrors);
 			printf("  RX-nombufs:             %14"PRIu64"\n",
 			       stats->rx_nombuf);
Stephen Hemminger March 22, 2016, 6:28 p.m. UTC | #7
On Tue, 22 Mar 2016 15:23:22 +0000
"Tahhan, Maryam" <maryam.tahhan@intel.com> wrote:

> > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > Sent: Thursday, March 17, 2016 4:41 PM
> > To: Igor Ryzhov <iryzhov@nfware.com>
> > Cc: dev@dpdk.org; Tahhan, Maryam <maryam.tahhan@intel.com>;
> > olivier.matz@6wind.com
> > Subject: Re: [dpdk-dev] [PATCH] ethdev: don't count missed packets in
> > erroneous packets counter
> > 
> > CC Maryam and Olivier who had discussions about imissed and other
> > stats:
> > 	http://dpdk.org/ml/archives/dev/2015-August/022905.html
> > 	http://dpdk.org/ml/archives/dev/2015-September/023351.html
> > 	http://dpdk.org/ml/archives/dev/2015-September/023612.html
> > 
> > 2016-03-10 16:03, Igor Ryzhov:  
> > > Comment for "ierrors" counter says that it counts erroneous received  
> > packets. But for some reason "imissed" counter is added to "ierrors"
> > counter in most drivers. It is a mistake, because missed packets are
> > obviously not received. This patch fixes it.
> > 
> > According to this patch
> > 	http://dpdk.org/browse/dpdk/commit/?id=70bdb186
> > imissed was kept in ierrors because of backward compatibility.
> > I'm OK to remove imissed from ierrors.
> > 
> > Fixes: 70bdb18657da ("ethdev: add Rx error counters for missed, badcrc
> > and badlen packets")
> > Fixes: 6bfe648406b5 ("i40e: add Rx error statistics")
> > Fixes: 856505d303f4 ("cxgbe: add port statistics")
> > 
> > Acked-by: Thomas Monjalon <thomas.monjalon@6wind.com>  
> 
> Looks fine, but make sure to add an explicit comment in release notes somewhere to flag the change. In case any apps were accounting for imissed as part of ierrors like testpmd was: 
> 
> -		if (((stats->ierrors - stats->imissed) + stats->rx_nombuf) > 0) {
> +		if ((stats->ierrors + stats->rx_nombuf) > 0) {

Extra () in that expression.
diff mbox

Patch

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 269ef81..d3d733b 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -753,7 +753,7 @@  fwd_port_stats_display(portid_t port_id, struct rte_eth_stats *stats)
 		if (cur_fwd_eng == &csum_fwd_engine)
 			printf("  Bad-ipcsum: %-14"PRIu64" Bad-l4csum: %-14"PRIu64" \n",
 			       port->rx_bad_ip_csum, port->rx_bad_l4_csum);
-		if (((stats->ierrors - stats->imissed) + stats->rx_nombuf) > 0) {
+		if ((stats->ierrors + stats->rx_nombuf) > 0) {
 			printf("  RX-error: %-"PRIu64"\n",  stats->ierrors);
 			printf("  RX-nombufs: %-14"PRIu64"\n", stats->rx_nombuf);
 		}
@@ -772,7 +772,7 @@  fwd_port_stats_display(portid_t port_id, struct rte_eth_stats *stats)
 		if (cur_fwd_eng == &csum_fwd_engine)
 			printf("  Bad-ipcsum:%14"PRIu64"    Bad-l4csum:%14"PRIu64"\n",
 			       port->rx_bad_ip_csum, port->rx_bad_l4_csum);
-		if (((stats->ierrors - stats->imissed) + stats->rx_nombuf) > 0) {
+		if ((stats->ierrors + stats->rx_nombuf) > 0) {
 			printf("  RX-error:%"PRIu64"\n", stats->ierrors);
 			printf("  RX-nombufs:             %14"PRIu64"\n",
 			       stats->rx_nombuf);
diff --git a/drivers/net/cxgbe/cxgbe_ethdev.c b/drivers/net/cxgbe/cxgbe_ethdev.c
index 97ef152..0070e2a 100644
--- a/drivers/net/cxgbe/cxgbe_ethdev.c
+++ b/drivers/net/cxgbe/cxgbe_ethdev.c
@@ -662,7 +662,7 @@  static void cxgbe_dev_stats_get(struct rte_eth_dev *eth_dev,
 			      ps.rx_trunc2 + ps.rx_trunc3;
 	eth_stats->ierrors  = ps.rx_symbol_err + ps.rx_fcs_err +
 			      ps.rx_jabber + ps.rx_too_long + ps.rx_runt +
-			      ps.rx_len_err + eth_stats->imissed;
+			      ps.rx_len_err;
 
 	/* TX Stats */
 	eth_stats->opackets = ps.tx_frames;
diff --git a/drivers/net/e1000/em_ethdev.c b/drivers/net/e1000/em_ethdev.c
index 4a843fe..27ace6d 100644
--- a/drivers/net/e1000/em_ethdev.c
+++ b/drivers/net/e1000/em_ethdev.c
@@ -914,7 +914,6 @@  eth_em_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *rte_stats)
 	rte_stats->imissed = stats->mpc;
 	rte_stats->ierrors = stats->crcerrs +
 	                     stats->rlec + stats->ruc + stats->roc +
-	                     rte_stats->imissed +
 	                     stats->rxerrc + stats->algnerrc + stats->cexterr;
 
 	/* Tx Errors */
diff --git a/drivers/net/e1000/igb_ethdev.c b/drivers/net/e1000/igb_ethdev.c
index 4ed5e95..6e93214 100644
--- a/drivers/net/e1000/igb_ethdev.c
+++ b/drivers/net/e1000/igb_ethdev.c
@@ -1640,7 +1640,6 @@  eth_igb_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *rte_stats)
 	rte_stats->imissed = stats->mpc;
 	rte_stats->ierrors = stats->crcerrs +
 	                     stats->rlec + stats->ruc + stats->roc +
-	                     rte_stats->imissed +
 	                     stats->rxerrc + stats->algnerrc + stats->cexterr;
 
 	/* Tx Errors */
diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 7e68c61..7d68d4d 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -2062,8 +2062,7 @@  i40e_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
 			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 +
-			stats->imissed;
+			ns->rx_oversize + ns->rx_fragments + ns->rx_jabber;
 
 	PMD_DRV_LOG(DEBUG, "***************** PF stats start *******************");
 	PMD_DRV_LOG(DEBUG, "rx_bytes:            %"PRIu64"", ns->eth.rx_bytes);
diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
index 3e6fe86..ba84544 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -2552,7 +2552,6 @@  ixgbe_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
 	                  hw_stats->rlec +
 	                  hw_stats->ruc +
 	                  hw_stats->roc +
-	                  total_missed_rx +
 	                  hw_stats->illerrc +
 	                  hw_stats->errbc +
 	                  hw_stats->rfc +