[dpdk-dev] ethdev: don't count missed packets in erroneous packets counter
Commit Message
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
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
>
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.
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>
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>
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
> 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);
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.
@@ -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);
@@ -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;
@@ -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 */
@@ -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 */
@@ -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);
@@ -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 +