[dpdk-dev] [PATCH 00/12] rxq q_errors[] statistics fixes

[dpdk-dev] [PATCH 00/12] rxq q_errors[] statistics fixes

[David Marchand]

According to the api, the q_errors[] per queue statistic is for reception
errors not transmit errors.
This is a first cleanup on statistics before looking at oerrors.

-- 
David Marchand

David Marchand (12):
  net/af_packet: fix incorrect rxq errors stat
  net/avp: fix incorrect rxq errors stat
  net/bnxt: fix incorrect rxq errors stat
  net/cxgbe: fix incorrect rxq errors stat
  net/kni: fix incorrect rxq errors stat
  net/mlx4: fix incorrect rxq errors stat
  net/mlx5: fix incorrect rxq errors stat
  net/null: fix incorrect rxq errors stat
  net/pcap: fix incorrect rxq errors stat
  net/ring: fix incorrect rxq errors stat
  net/szedata2: fix incorrect rxq errors stat
  net/tap: fix incorrect rxq errors stat

 drivers/net/af_packet/rte_eth_af_packet.c | 3 +--
 drivers/net/avp/avp_ethdev.c              | 1 -
 drivers/net/bnxt/bnxt_hwrm.c              | 1 -
 drivers/net/cxgbe/cxgbe_ethdev.c          | 1 -
 drivers/net/cxgbe/cxgbevf_ethdev.c        | 1 -
 drivers/net/kni/rte_eth_kni.c             | 3 +--
 drivers/net/mlx4/mlx4_ethdev.c            | 1 -
 drivers/net/mlx5/mlx5_stats.c             | 1 -
 drivers/net/null/rte_eth_null.c           | 4 +---
 drivers/net/pcap/rte_eth_pcap.c           | 3 +--
 drivers/net/ring/rte_eth_ring.c           | 3 +--
 drivers/net/szedata2/rte_eth_szedata2.c   | 1 -
 drivers/net/tap/rte_eth_tap.c             | 3 +--
 13 files changed, 6 insertions(+), 20 deletions(-)

-- 
1.8.3.1

[dpdk-dev] [PATCH 01/12] net/af_packet: fix incorrect rxq errors stat

[David Marchand]

Transmit errors must not be reported in q_errors[] which is for
reception.

Fixes: 364e08f2bbc0 ("af_packet: add PMD for AF_PACKET-based virtual devices")
Cc: stable@dpdk.org
Cc: John W. Linville <linville@tuxdriver.com>
Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 drivers/net/af_packet/rte_eth_af_packet.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/af_packet/rte_eth_af_packet.c b/drivers/net/af_packet/rte_eth_af_packet.c
index 264cfc0..ec90cc0 100644
--- a/drivers/net/af_packet/rte_eth_af_packet.c
+++ b/drivers/net/af_packet/rte_eth_af_packet.c
@@ -328,10 +328,9 @@ struct pmd_internals {
 	        internal->nb_queues : RTE_ETHDEV_QUEUE_STAT_CNTRS);
 	for (i = 0; i < imax; i++) {
 		igb_stats->q_opackets[i] = internal->tx_queue[i].tx_pkts;
-		igb_stats->q_errors[i] = internal->tx_queue[i].err_pkts;
 		igb_stats->q_obytes[i] = internal->tx_queue[i].tx_bytes;
 		tx_total += igb_stats->q_opackets[i];
-		tx_err_total += igb_stats->q_errors[i];
+		tx_err_total += internal->tx_queue[i].err_pkts;
 		tx_bytes_total += igb_stats->q_obytes[i];
 	}
 
-- 
1.8.3.1

[dpdk-dev] [PATCH 02/12] net/avp: fix incorrect rxq errors stat

[David Marchand]

Transmit errors must not be reported in q_errors[] which is for
reception.

Fixes: 5a5abe2de94b ("net/avp: add device statistics operations")
Cc: stable@dpdk.org
Cc: Allain Legacy <allain.legacy@windriver.com>
Cc: Matt Peters <matt.peters@windriver.com>
Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 drivers/net/avp/avp_ethdev.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/avp/avp_ethdev.c b/drivers/net/avp/avp_ethdev.c
index 09388d0..5d069a2 100644
--- a/drivers/net/avp/avp_ethdev.c
+++ b/drivers/net/avp/avp_ethdev.c
@@ -2228,7 +2228,6 @@ struct avp_queue {
 
 			stats->q_opackets[i] += txq->packets;
 			stats->q_obytes[i] += txq->bytes;
-			stats->q_errors[i] += txq->errors;
 		}
 	}
 
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH 02/12] net/avp: fix incorrect rxq errors stat

[Legacy, Allain]

> -----Original Message-----
> From: David Marchand [mailto:david.marchand@redhat.com]
> Sent: Monday, March 04, 2019 6:18 AM
> To: dev@dpdk.org
> Cc: stable@dpdk.org; Legacy, Allain; Peters, Matt
> Subject: [PATCH 02/12] net/avp: fix incorrect rxq errors stat
> 
> Transmit errors must not be reported in q_errors[] which is for reception.
> 
> Fixes: 5a5abe2de94b ("net/avp: add device statistics operations")
> Cc: stable@dpdk.org
> Cc: Allain Legacy <allain.legacy@windriver.com>
> Cc: Matt Peters <matt.peters@windriver.com>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
Acked-by:  Allain Legacy <allain.legacy@windriver.com>

[dpdk-dev] [PATCH 03/12] net/bnxt: fix incorrect rxq errors stat

[David Marchand]

Transmit errors must not be reported in q_errors[] which is for
reception.

Fixes: 577d3dced0dc ("net/bnxt: refactor the query stats")
Cc: stable@dpdk.org
Cc: Ajit Khaparde <ajit.khaparde@broadcom.com>
Cc: Somnath Kotur <somnath.kotur@broadcom.com>
Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 drivers/net/bnxt/bnxt_hwrm.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/bnxt/bnxt_hwrm.c b/drivers/net/bnxt/bnxt_hwrm.c
index 9999760..8853391 100644
--- a/drivers/net/bnxt/bnxt_hwrm.c
+++ b/drivers/net/bnxt/bnxt_hwrm.c
@@ -3186,7 +3186,6 @@ int bnxt_hwrm_ctx_qstats(struct bnxt *bp, uint32_t cid, int idx,
 		stats->q_obytes[idx] = rte_le_to_cpu_64(resp->tx_ucast_bytes);
 		stats->q_obytes[idx] += rte_le_to_cpu_64(resp->tx_mcast_bytes);
 		stats->q_obytes[idx] += rte_le_to_cpu_64(resp->tx_bcast_bytes);
-		stats->q_errors[idx] += rte_le_to_cpu_64(resp->tx_err_pkts);
 	}
 
 
-- 
1.8.3.1

[dpdk-dev] [PATCH 04/12] net/cxgbe: fix incorrect rxq errors stat

[David Marchand]

Transmit errors must not be reported in q_errors[] which is for
reception.

Fixes: 856505d303f4 ("cxgbe: add port statistics")
Fixes: a0a344a8f728 ("net/cxgbe: add VF port statistics")
Cc: stable@dpdk.org
Cc: Rahul Lakkireddy <rahul.lakkireddy@chelsio.com>
Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 drivers/net/cxgbe/cxgbe_ethdev.c   | 1 -
 drivers/net/cxgbe/cxgbevf_ethdev.c | 1 -
 2 files changed, 2 deletions(-)

diff --git a/drivers/net/cxgbe/cxgbe_ethdev.c b/drivers/net/cxgbe/cxgbe_ethdev.c
index 010a818..7c7a51d 100644
--- a/drivers/net/cxgbe/cxgbe_ethdev.c
+++ b/drivers/net/cxgbe/cxgbe_ethdev.c
@@ -705,7 +705,6 @@ static int cxgbe_dev_stats_get(struct rte_eth_dev *eth_dev,
 
 		eth_stats->q_opackets[i] = txq->stats.pkts;
 		eth_stats->q_obytes[i] = txq->stats.tx_bytes;
-		eth_stats->q_errors[i] = txq->stats.mapping_err;
 	}
 	return 0;
 }
diff --git a/drivers/net/cxgbe/cxgbevf_ethdev.c b/drivers/net/cxgbe/cxgbevf_ethdev.c
index 0e93d99..0af9dd9 100644
--- a/drivers/net/cxgbe/cxgbevf_ethdev.c
+++ b/drivers/net/cxgbe/cxgbevf_ethdev.c
@@ -69,7 +69,6 @@ static int cxgbevf_dev_stats_get(struct rte_eth_dev *eth_dev,
 
 		eth_stats->q_opackets[i] = txq->stats.pkts;
 		eth_stats->q_obytes[i] = txq->stats.tx_bytes;
-		eth_stats->q_errors[i] = txq->stats.mapping_err;
 	}
 	return 0;
 }
-- 
1.8.3.1

[dpdk-dev] [PATCH 05/12] net/kni: fix incorrect rxq errors stat

[David Marchand]

Transmit errors must not be reported in q_errors[] which is for
reception.

Fixes: 75e2bc54c018 ("net/kni: add KNI PMD")
Cc: stable@dpdk.org
Cc: Ferruh Yigit <ferruh.yigit@intel.com>
Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 drivers/net/kni/rte_eth_kni.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/kni/rte_eth_kni.c b/drivers/net/kni/rte_eth_kni.c
index a1e9970..363f80d 100644
--- a/drivers/net/kni/rte_eth_kni.c
+++ b/drivers/net/kni/rte_eth_kni.c
@@ -285,10 +285,9 @@ struct pmd_internals {
 		q = data->tx_queues[i];
 		stats->q_opackets[i] = q->tx.pkts;
 		stats->q_obytes[i] = q->tx.bytes;
-		stats->q_errors[i] = q->tx.err_pkts;
 		tx_packets_total += stats->q_opackets[i];
 		tx_bytes_total += stats->q_obytes[i];
-		tx_packets_err_total += stats->q_errors[i];
+		tx_packets_err_total += q->tx.err_pkts;
 	}
 
 	stats->ipackets = rx_packets_total;
-- 
1.8.3.1

[dpdk-dev] [PATCH 06/12] net/mlx4: fix incorrect rxq errors stat

[David Marchand]

Transmit errors must not be reported in q_errors[] which is for
reception.

Fixes: 7fae69eeff13 ("mlx4: new poll mode driver")
Cc: stable@dpdk.org
Cc: Matan Azrad <matan@mellanox.com>
Cc: Shahaf Shuler <shahafs@mellanox.com>
Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 drivers/net/mlx4/mlx4_ethdev.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/mlx4/mlx4_ethdev.c b/drivers/net/mlx4/mlx4_ethdev.c
index 4dae67a..41c46d3 100644
--- a/drivers/net/mlx4/mlx4_ethdev.c
+++ b/drivers/net/mlx4/mlx4_ethdev.c
@@ -661,7 +661,6 @@ int mlx4_fw_version_get(struct rte_eth_dev *dev, char *fw_ver, size_t fw_size)
 		if (idx < RTE_ETHDEV_QUEUE_STAT_CNTRS) {
 			tmp.q_opackets[idx] += txq->stats.opackets;
 			tmp.q_obytes[idx] += txq->stats.obytes;
-			tmp.q_errors[idx] += txq->stats.odropped;
 		}
 		tmp.opackets += txq->stats.opackets;
 		tmp.obytes += txq->stats.obytes;
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH 06/12] net/mlx4: fix incorrect rxq errors stat

[Shahaf Shuler]

Monday, March 4, 2019 1:18 PM, David Marchand:
> Subject: [PATCH 06/12] net/mlx4: fix incorrect rxq errors stat
> 
> Transmit errors must not be reported in q_errors[] which is for reception.
> 
> Fixes: 7fae69eeff13 ("mlx4: new poll mode driver")
> Cc: stable@dpdk.org
> Cc: Matan Azrad <matan@mellanox.com>
> Cc: Shahaf Shuler <shahafs@mellanox.com>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
>  drivers/net/mlx4/mlx4_ethdev.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/net/mlx4/mlx4_ethdev.c
> b/drivers/net/mlx4/mlx4_ethdev.c index 4dae67a..41c46d3 100644
> --- a/drivers/net/mlx4/mlx4_ethdev.c
> +++ b/drivers/net/mlx4/mlx4_ethdev.c
> @@ -661,7 +661,6 @@ int mlx4_fw_version_get(struct rte_eth_dev *dev,
> char *fw_ver, size_t fw_size)
>  		if (idx < RTE_ETHDEV_QUEUE_STAT_CNTRS) {
>  			tmp.q_opackets[idx] += txq->stats.opackets;
>  			tmp.q_obytes[idx] += txq->stats.obytes;
> -			tmp.q_errors[idx] += txq->stats.odropped;
>  		}
>  		tmp.opackets += txq->stats.opackets;
>  		tmp.obytes += txq->stats.obytes;

Acked-by: Shahaf Shuler <shahafs@mellanox.com>

> --
> 1.8.3.1

[dpdk-dev] [PATCH 07/12] net/mlx5: fix incorrect rxq errors stat

[David Marchand]

Transmit errors must not be reported in q_errors[] which is for
reception.

Fixes: 87011737b715 ("mlx5: add software counters")
Fixes: 9f9a48eb2978 ("net/mlx5: fix Tx stats error counter definition")
Cc: stable@dpdk.org
Cc: Shahaf Shuler <shahafs@mellanox.com>
Cc: Yongseok Koh <yskoh@mellanox.com>
Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 drivers/net/mlx5/mlx5_stats.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/mlx5/mlx5_stats.c b/drivers/net/mlx5/mlx5_stats.c
index 6906dc8..ef7bc14 100644
--- a/drivers/net/mlx5/mlx5_stats.c
+++ b/drivers/net/mlx5/mlx5_stats.c
@@ -409,7 +409,6 @@
 			tmp.q_opackets[idx] += txq->stats.opackets;
 			tmp.q_obytes[idx] += txq->stats.obytes;
 #endif
-			tmp.q_errors[idx] += txq->stats.oerrors;
 		}
 #ifdef MLX5_PMD_SOFT_COUNTERS
 		tmp.opackets += txq->stats.opackets;
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH 07/12] net/mlx5: fix incorrect rxq errors stat

[Shahaf Shuler]

Monday, March 4, 2019 1:19 PM, David Marchand:
> Subject: [PATCH 07/12] net/mlx5: fix incorrect rxq errors stat
> 
> Transmit errors must not be reported in q_errors[] which is for reception.
> 
> Fixes: 87011737b715 ("mlx5: add software counters")
> Fixes: 9f9a48eb2978 ("net/mlx5: fix Tx stats error counter definition")
> Cc: stable@dpdk.org
> Cc: Shahaf Shuler <shahafs@mellanox.com>
> Cc: Yongseok Koh <yskoh@mellanox.com>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
>  drivers/net/mlx5/mlx5_stats.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/net/mlx5/mlx5_stats.c b/drivers/net/mlx5/mlx5_stats.c
> index 6906dc8..ef7bc14 100644
> --- a/drivers/net/mlx5/mlx5_stats.c
> +++ b/drivers/net/mlx5/mlx5_stats.c
> @@ -409,7 +409,6 @@
>  			tmp.q_opackets[idx] += txq->stats.opackets;
>  			tmp.q_obytes[idx] += txq->stats.obytes;  #endif
> -			tmp.q_errors[idx] += txq->stats.oerrors;
>  		}
>  #ifdef MLX5_PMD_SOFT_COUNTERS
>  		tmp.opackets += txq->stats.opackets;
> --
> 1.8.3.1

Acked-by: Shahaf Shuler <shahafs@mellanox.com>

[dpdk-dev] [PATCH 08/12] net/null: fix incorrect rxq errors stat

[David Marchand]

Transmit errors must not be reported in q_errors[] which is for
reception.

Fixes: c743e50c475f ("null: new poll mode driver")
Cc: stable@dpdk.org
Cc: Tetsuya Mukawa <mtetsuyah@gmail.com>
Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 drivers/net/null/rte_eth_null.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/null/rte_eth_null.c b/drivers/net/null/rte_eth_null.c
index 159c1c1..0e30886 100644
--- a/drivers/net/null/rte_eth_null.c
+++ b/drivers/net/null/rte_eth_null.c
@@ -333,10 +333,8 @@ struct pmd_internals {
 	for (i = 0; i < num_stats; i++) {
 		igb_stats->q_opackets[i] =
 			internal->tx_null_queues[i].tx_pkts.cnt;
-		igb_stats->q_errors[i] =
-			internal->tx_null_queues[i].err_pkts.cnt;
 		tx_total += igb_stats->q_opackets[i];
-		tx_err_total += igb_stats->q_errors[i];
+		tx_err_total += internal->tx_null_queues[i].err_pkts.cnt;
 	}
 
 	igb_stats->ipackets = rx_total;
-- 
1.8.3.1

[dpdk-dev] [PATCH 09/12] net/pcap: fix incorrect rxq errors stat

[David Marchand]

Transmit errors must not be reported in q_errors[] which is for
reception.

Fixes: 4c173302c307 ("pcap: add new driver")
Cc: stable@dpdk.org
Cc: Ferruh Yigit <ferruh.yigit@intel.com>
Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 drivers/net/pcap/rte_eth_pcap.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/pcap/rte_eth_pcap.c b/drivers/net/pcap/rte_eth_pcap.c
index 65bbd7e..4d5e6ba 100644
--- a/drivers/net/pcap/rte_eth_pcap.c
+++ b/drivers/net/pcap/rte_eth_pcap.c
@@ -605,10 +605,9 @@ struct pmd_devargs {
 			i < dev->data->nb_tx_queues; i++) {
 		stats->q_opackets[i] = internal->tx_queue[i].tx_stat.pkts;
 		stats->q_obytes[i] = internal->tx_queue[i].tx_stat.bytes;
-		stats->q_errors[i] = internal->tx_queue[i].tx_stat.err_pkts;
 		tx_packets_total += stats->q_opackets[i];
 		tx_bytes_total += stats->q_obytes[i];
-		tx_packets_err_total += stats->q_errors[i];
+		tx_packets_err_total += internal->tx_queue[i].tx_stat.err_pkts;
 	}
 
 	stats->ipackets = rx_packets_total;
-- 
1.8.3.1

[dpdk-dev] [PATCH 10/12] net/ring: fix incorrect rxq errors stat

[David Marchand]

Transmit errors must not be reported in q_errors[] which is for
reception.

Fixes: e1e4017751f1 ("ring: add new driver")
Cc: stable@dpdk.org
Cc: Bruce Richardson <bruce.richardson@intel.com>
Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 drivers/net/ring/rte_eth_ring.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/ring/rte_eth_ring.c b/drivers/net/ring/rte_eth_ring.c
index aeb48f5..4865763 100644
--- a/drivers/net/ring/rte_eth_ring.c
+++ b/drivers/net/ring/rte_eth_ring.c
@@ -182,9 +182,8 @@ struct pmd_internals {
 	for (i = 0; i < RTE_ETHDEV_QUEUE_STAT_CNTRS &&
 			i < dev->data->nb_tx_queues; i++) {
 		stats->q_opackets[i] = internal->tx_ring_queues[i].tx_pkts.cnt;
-		stats->q_errors[i] = internal->tx_ring_queues[i].err_pkts.cnt;
 		tx_total += stats->q_opackets[i];
-		tx_err_total += stats->q_errors[i];
+		tx_err_total += internal->tx_ring_queues[i].err_pkts.cnt;
 	}
 
 	stats->ipackets = rx_total;
-- 
1.8.3.1

[dpdk-dev] [PATCH 11/12] net/szedata2: fix incorrect rxq errors stat

[David Marchand]

Transmit errors must not be reported in q_errors[] which is for
reception.

Fixes: abef3dd62e7a ("szedata2: add new poll mode driver")
Cc: stable@dpdk.org
Cc: Jan Remes <remes@netcope.com>
Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 drivers/net/szedata2/rte_eth_szedata2.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/szedata2/rte_eth_szedata2.c b/drivers/net/szedata2/rte_eth_szedata2.c
index 88448ef..a6fbfe3 100644
--- a/drivers/net/szedata2/rte_eth_szedata2.c
+++ b/drivers/net/szedata2/rte_eth_szedata2.c
@@ -1093,7 +1093,6 @@ struct szedata2_tx_queue {
 		if (i < RTE_ETHDEV_QUEUE_STAT_CNTRS) {
 			stats->q_opackets[i] = txq->tx_pkts;
 			stats->q_obytes[i] = txq->tx_bytes;
-			stats->q_errors[i] = txq->err_pkts;
 		}
 		tx_total += txq->tx_pkts;
 		tx_total_bytes += txq->tx_bytes;
-- 
1.8.3.1

[dpdk-dev] [PATCH 12/12] net/tap: fix incorrect rxq errors stat

[David Marchand]

Transmit errors must not be reported in q_errors[] which is for
reception.

Fixes: 02f96a0a82d1 ("net/tap: add TUN/TAP device PMD")
Cc: stable@dpdk.org
Cc: Keith Wiles <keith.wiles@intel.com>
Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 drivers/net/tap/rte_eth_tap.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index 6f5109f..94c728f 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -968,10 +968,9 @@ struct ipc_queues {
 
 	for (i = 0; i < imax; i++) {
 		tap_stats->q_opackets[i] = pmd->txq[i].stats.opackets;
-		tap_stats->q_errors[i] = pmd->txq[i].stats.errs;
 		tap_stats->q_obytes[i] = pmd->txq[i].stats.obytes;
 		tx_total += tap_stats->q_opackets[i];
-		tx_err_total += tap_stats->q_errors[i];
+		tx_err_total += pmd->txq[i].stats.errs;
 		tx_bytes_total += tap_stats->q_obytes[i];
 	}
 
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH 12/12] net/tap: fix incorrect rxq errors stat

[Wiles, Keith]

> On Mar 4, 2019, at 5:18 AM, David Marchand <david.marchand@redhat.com> wrote:
> 
> Transmit errors must not be reported in q_errors[] which is for
> reception.
> 
> Fixes: 02f96a0a82d1 ("net/tap: add TUN/TAP device PMD")
> Cc: stable@dpdk.org
> Cc: Keith Wiles <keith.wiles@intel.com>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
> drivers/net/tap/rte_eth_tap.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> index 6f5109f..94c728f 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -968,10 +968,9 @@ struct ipc_queues {
> 
> 	for (i = 0; i < imax; i++) {
> 		tap_stats->q_opackets[i] = pmd->txq[i].stats.opackets;
> -		tap_stats->q_errors[i] = pmd->txq[i].stats.errs;
> 		tap_stats->q_obytes[i] = pmd->txq[i].stats.obytes;
> 		tx_total += tap_stats->q_opackets[i];
> -		tx_err_total += tap_stats->q_errors[i];
> +		tx_err_total += pmd->txq[i].stats.errs;
> 		tx_bytes_total += tap_stats->q_obytes[i];
> 	}
> 
> -- 
> 1.8.3.1
> 
Acked-by: Keith Wiles <keith.wiles@intel.com>

Regards,
Keith

Re: [dpdk-dev] [PATCH 00/12] rxq q_errors[] statistics fixes

[Ferruh Yigit]

On 3/4/2019 11:18 AM, David Marchand wrote:
> According to the api, the q_errors[] per queue statistic is for reception
> errors not transmit errors.
> This is a first cleanup on statistics before looking at oerrors.
> 

Yes, the patchset looks aligned with the API documentation [1].

What can be the solution after cleanup? We can merge this cleanup and solution
next to each-other to not leave a gap?
1- Different variables for Rx and Tx errors?
2- Combine Rx & Tx into this single variable?

It can be good to find a solution because new PMDs doing same mistake because of
copy/paste...

[1]
https://git.dpdk.org/dpdk/tree/lib/librte_ethdev/rte_ethdev.h?h=v19.02#n263

Re: [dpdk-dev] [PATCH 00/12] rxq q_errors[] statistics fixes

[David Marchand]

On Mon, Mar 11, 2019 at 6:22 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:

> On 3/4/2019 11:18 AM, David Marchand wrote:
> > According to the api, the q_errors[] per queue statistic is for reception
> > errors not transmit errors.
> > This is a first cleanup on statistics before looking at oerrors.
> >
>
> Yes, the patchset looks aligned with the API documentation [1].
>
> What can be the solution after cleanup? We can merge this cleanup and
> solution
> next to each-other to not leave a gap?
> 1- Different variables for Rx and Tx errors?
> 2- Combine Rx & Tx into this single variable?
>
> It can be good to find a solution because new PMDs doing same mistake
> because of
> copy/paste...
>

Might not be feasible but how about we could introduce an internal stats
structure containing the needed field for tx.
pmd would use it but ethdev would translate it to the current exposed api
rte_eth_dev_stats_get ?
The additional field would be formatted by ethdev to be provided through
the xstats api.


-- 
David Marchand

Re: [dpdk-dev] [PATCH 00/12] rxq q_errors[] statistics fixes

[David Marchand]

On Mon, Mar 11, 2019 at 7:09 PM David Marchand <david.marchand@redhat.com>
wrote:

> On Mon, Mar 11, 2019 at 6:22 PM Ferruh Yigit <ferruh.yigit@intel.com>
> wrote:
>
>> On 3/4/2019 11:18 AM, David Marchand wrote:
>> > According to the api, the q_errors[] per queue statistic is for
>> reception
>> > errors not transmit errors.
>> > This is a first cleanup on statistics before looking at oerrors.
>> >
>>
>> Yes, the patchset looks aligned with the API documentation [1].
>>
>> What can be the solution after cleanup? We can merge this cleanup and
>> solution
>> next to each-other to not leave a gap?
>> 1- Different variables for Rx and Tx errors?
>> 2- Combine Rx & Tx into this single variable?
>>
>> It can be good to find a solution because new PMDs doing same mistake
>> because of
>> copy/paste...
>>
>
> Might not be feasible but how about we could introduce an internal stats
> structure containing the needed field for tx.
> pmd would use it but ethdev would translate it to the current exposed api
> rte_eth_dev_stats_get ?
> The additional field would be formatted by ethdev to be provided through
> the xstats api.
>

Sending RFC patches to have something to discuss on.


-- 
David Marchand

Re: [dpdk-dev] [PATCH 00/12] rxq q_errors[] statistics fixes

[David Marchand]

On Mon, Mar 11, 2019 at 7:09 PM David Marchand <david.marchand@redhat.com>
wrote:

> On Mon, Mar 11, 2019 at 6:22 PM Ferruh Yigit <ferruh.yigit@intel.com>
> wrote:
>
>> On 3/4/2019 11:18 AM, David Marchand wrote:
>> > According to the api, the q_errors[] per queue statistic is for
>> reception
>> > errors not transmit errors.
>> > This is a first cleanup on statistics before looking at oerrors.
>> >
>>
>> Yes, the patchset looks aligned with the API documentation [1].
>>
>> What can be the solution after cleanup? We can merge this cleanup and
>> solution
>> next to each-other to not leave a gap?
>> 1- Different variables for Rx and Tx errors?
>> 2- Combine Rx & Tx into this single variable?
>>
>> It can be good to find a solution because new PMDs doing same mistake
>> because of
>> copy/paste...
>>
>
> Might not be feasible but how about we could introduce an internal stats
> structure containing the needed field for tx.
> pmd would use it but ethdev would translate it to the current exposed api
> rte_eth_dev_stats_get ?
> The additional field would be formatted by ethdev to be provided through
> the xstats api.
>

Sending RFC patches to have something to discuss on.


-- 
David Marchand

[dpdk-dev] [RFC PATCH 1/2] ethdev: introduce internal rxq/txq stats API

[David Marchand]

Introduce a new api to retrieve per queue statistics from the drivers.
The api objectives:
- easily add some common per queue statistics and have it exposed
  through the user xstats api while the user stats api is left untouched
- remove the limitations on the per queue statistics count (inherited
  from ixgbe) and avoid recurrent bugs on stats array overflow

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 lib/librte_ethdev/rte_ethdev.c        | 191 ++++++++++++++++++++++++++++------
 lib/librte_ethdev/rte_ethdev_core.h   |  13 +++
 lib/librte_ethdev/rte_ethdev_driver.h |  18 ++++
 3 files changed, 192 insertions(+), 30 deletions(-)

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 85c1794..058fbd1 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -88,21 +88,30 @@ struct rte_eth_xstats_name_off {
 
 #define RTE_NB_STATS (sizeof(rte_stats_strings) / sizeof(rte_stats_strings[0]))
 
-static const struct rte_eth_xstats_name_off rte_rxq_stats_strings[] = {
+static const struct rte_eth_xstats_name_off legacy_rxq_stats_map[] = {
 	{"packets", offsetof(struct rte_eth_stats, q_ipackets)},
 	{"bytes", offsetof(struct rte_eth_stats, q_ibytes)},
 	{"errors", offsetof(struct rte_eth_stats, q_errors)},
 };
+#define RTE_NB_LEGACY_RXQ_STATS RTE_DIM(legacy_rxq_stats_map)
+static const struct rte_eth_xstats_name_off rxq_stats_map[] = {
+	{"packets", offsetof(struct pmd_eth_rxq_stats, packets)},
+	{"bytes", offsetof(struct pmd_eth_rxq_stats, bytes)},
+	{"errors", offsetof(struct pmd_eth_rxq_stats, errors)},
+};
+#define RTE_NB_RXQ_STATS RTE_DIM(rxq_stats_map)
 
-#define RTE_NB_RXQ_STATS (sizeof(rte_rxq_stats_strings) /	\
-		sizeof(rte_rxq_stats_strings[0]))
-
-static const struct rte_eth_xstats_name_off rte_txq_stats_strings[] = {
+static const struct rte_eth_xstats_name_off legacy_txq_stats_map[] = {
 	{"packets", offsetof(struct rte_eth_stats, q_opackets)},
 	{"bytes", offsetof(struct rte_eth_stats, q_obytes)},
 };
-#define RTE_NB_TXQ_STATS (sizeof(rte_txq_stats_strings) /	\
-		sizeof(rte_txq_stats_strings[0]))
+#define RTE_NB_LEGACY_TXQ_STATS RTE_DIM(legacy_txq_stats_map)
+static const struct rte_eth_xstats_name_off txq_stats_map[] = {
+	{"packets", offsetof(struct pmd_eth_txq_stats, packets)},
+	{"bytes", offsetof(struct pmd_eth_txq_stats, bytes)},
+	{"errors", offsetof(struct pmd_eth_txq_stats, errors)},
+};
+#define RTE_NB_TXQ_STATS RTE_DIM(txq_stats_map)
 
 #define RTE_RX_OFFLOAD_BIT2STR(_name)	\
 	{ DEV_RX_OFFLOAD_##_name, #_name }
@@ -1937,6 +1946,10 @@ struct rte_eth_dev *
 rte_eth_stats_get(uint16_t port_id, struct rte_eth_stats *stats)
 {
 	struct rte_eth_dev *dev;
+	unsigned int nb_rxqs;
+	unsigned int nb_txqs;
+	unsigned int qid;
+	int ret;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
 
@@ -1945,7 +1958,44 @@ struct rte_eth_dev *
 
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->stats_get, -ENOTSUP);
 	stats->rx_nombuf = dev->data->rx_mbuf_alloc_failed;
-	return eth_err(port_id, (*dev->dev_ops->stats_get)(dev, stats));
+	ret = eth_err(port_id, (*dev->dev_ops->stats_get)(dev, stats));
+	if (ret)
+		goto out;
+
+	if (!dev->dev_ops->rxq_stats_get)
+		goto skip_rxq;
+	nb_rxqs = RTE_MIN(dev->data->nb_rx_queues,
+			  RTE_ETHDEV_QUEUE_STAT_CNTRS);
+	for (qid = 0; qid < nb_rxqs; qid++) {
+		struct pmd_eth_rxq_stats rxq_stats;
+
+		memset(&rxq_stats, 0, sizeof(rxq_stats));
+		if (dev->dev_ops->rxq_stats_get(dev, qid, &rxq_stats))
+			continue;
+
+		stats->q_ipackets[qid] = rxq_stats.packets;
+		stats->q_ibytes[qid] = rxq_stats.bytes;
+		stats->q_errors[qid] = rxq_stats.errors;
+	}
+
+skip_rxq:
+	if (!dev->dev_ops->txq_stats_get)
+		goto out;
+	nb_txqs = RTE_MIN(dev->data->nb_tx_queues,
+			  RTE_ETHDEV_QUEUE_STAT_CNTRS);
+	for (qid = 0; qid < nb_txqs; qid++) {
+		struct pmd_eth_txq_stats txq_stats;
+
+		memset(&txq_stats, 0, sizeof(txq_stats));
+		if (dev->dev_ops->txq_stats_get(dev, qid, &txq_stats))
+			continue;
+
+		stats->q_opackets[qid] = txq_stats.packets;
+		stats->q_obytes[qid] = txq_stats.bytes;
+	}
+
+out:
+	return ret;
 }
 
 int
@@ -1969,12 +2019,24 @@ struct rte_eth_dev *
 	uint16_t nb_rxqs, nb_txqs;
 	int count;
 
-	nb_rxqs = RTE_MIN(dev->data->nb_rx_queues, RTE_ETHDEV_QUEUE_STAT_CNTRS);
-	nb_txqs = RTE_MIN(dev->data->nb_tx_queues, RTE_ETHDEV_QUEUE_STAT_CNTRS);
-
 	count = RTE_NB_STATS;
-	count += nb_rxqs * RTE_NB_RXQ_STATS;
-	count += nb_txqs * RTE_NB_TXQ_STATS;
+	if (dev->dev_ops->rxq_stats_get) {
+		nb_rxqs = dev->data->nb_rx_queues;
+		count += nb_rxqs * RTE_NB_RXQ_STATS;
+	} else {
+		nb_rxqs = RTE_MIN(dev->data->nb_rx_queues,
+				  RTE_ETHDEV_QUEUE_STAT_CNTRS);
+		count += nb_rxqs * RTE_NB_LEGACY_RXQ_STATS;
+	}
+
+	if (dev->dev_ops->txq_stats_get) {
+		nb_txqs = dev->data->nb_tx_queues;
+		count += nb_txqs * RTE_NB_TXQ_STATS;
+	} else {
+		nb_txqs = RTE_MIN(dev->data->nb_tx_queues,
+				  RTE_ETHDEV_QUEUE_STAT_CNTRS);
+		count += nb_txqs * RTE_NB_LEGACY_TXQ_STATS;
+	}
 
 	return count;
 }
@@ -2065,27 +2127,59 @@ struct rte_eth_dev *
 			"%s", rte_stats_strings[idx].name);
 		cnt_used_entries++;
 	}
+
+	if (dev->dev_ops->rxq_stats_get) {
+		num_q = dev->data->nb_rx_queues;
+		for (id_queue = 0; id_queue < num_q; id_queue++) {
+			for (idx = 0; idx < RTE_NB_RXQ_STATS; idx++) {
+				snprintf(xstats_names[cnt_used_entries].name,
+					sizeof(xstats_names[0].name),
+					"rx_q%u%s",
+					id_queue, rxq_stats_map[idx].name);
+				cnt_used_entries++;
+			}
+		}
+		goto skip_legacy_rxq;
+	}
+
 	num_q = RTE_MIN(dev->data->nb_rx_queues, RTE_ETHDEV_QUEUE_STAT_CNTRS);
 	for (id_queue = 0; id_queue < num_q; id_queue++) {
-		for (idx = 0; idx < RTE_NB_RXQ_STATS; idx++) {
+		for (idx = 0; idx < RTE_NB_LEGACY_RXQ_STATS; idx++) {
 			snprintf(xstats_names[cnt_used_entries].name,
 				sizeof(xstats_names[0].name),
 				"rx_q%u%s",
-				id_queue, rte_rxq_stats_strings[idx].name);
+				id_queue, legacy_rxq_stats_map[idx].name);
 			cnt_used_entries++;
 		}
+	}
 
+skip_legacy_rxq:
+	if (dev->dev_ops->txq_stats_get) {
+		num_q = dev->data->nb_tx_queues;
+		for (id_queue = 0; id_queue < num_q; id_queue++) {
+			for (idx = 0; idx < RTE_NB_TXQ_STATS; idx++) {
+				snprintf(xstats_names[cnt_used_entries].name,
+					sizeof(xstats_names[0].name),
+					"tx_q%u%s",
+					id_queue, txq_stats_map[idx].name);
+				cnt_used_entries++;
+			}
+		}
+		goto skip_legacy_txq;
 	}
+
 	num_q = RTE_MIN(dev->data->nb_tx_queues, RTE_ETHDEV_QUEUE_STAT_CNTRS);
 	for (id_queue = 0; id_queue < num_q; id_queue++) {
-		for (idx = 0; idx < RTE_NB_TXQ_STATS; idx++) {
+		for (idx = 0; idx < RTE_NB_LEGACY_TXQ_STATS; idx++) {
 			snprintf(xstats_names[cnt_used_entries].name,
 				sizeof(xstats_names[0].name),
 				"tx_q%u%s",
-				id_queue, rte_txq_stats_strings[idx].name);
+				id_queue, legacy_txq_stats_map[idx].name);
 			cnt_used_entries++;
 		}
 	}
+
+skip_legacy_txq:
 	return cnt_used_entries;
 }
 
@@ -2252,9 +2346,6 @@ struct rte_eth_dev *
 
 	dev = &rte_eth_devices[port_id];
 
-	nb_rxqs = RTE_MIN(dev->data->nb_rx_queues, RTE_ETHDEV_QUEUE_STAT_CNTRS);
-	nb_txqs = RTE_MIN(dev->data->nb_tx_queues, RTE_ETHDEV_QUEUE_STAT_CNTRS);
-
 	/* global stats */
 	for (i = 0; i < RTE_NB_STATS; i++) {
 		stats_ptr = RTE_PTR_ADD(&eth_stats,
@@ -2264,26 +2355,71 @@ struct rte_eth_dev *
 	}
 
 	/* per-rxq stats */
+	if (dev->dev_ops->rxq_stats_get) {
+		nb_rxqs = dev->data->nb_rx_queues;
+		for (q = 0; q < nb_rxqs; q++) {
+			struct pmd_eth_rxq_stats rxq_stats;
+
+			memset(&rxq_stats, 0, sizeof(rxq_stats));
+			if (dev->dev_ops->rxq_stats_get(dev, q, &rxq_stats)) {
+				count += RTE_NB_RXQ_STATS;
+				continue;
+			}
+			for (i = 0; i < RTE_NB_RXQ_STATS; i++) {
+				stats_ptr = RTE_PTR_ADD(&rxq_stats,
+						rxq_stats_map[i].offset);
+				val = *stats_ptr;
+				xstats[count++].value = val;
+			}
+		}
+		goto skip_legacy_rxq;
+	}
+
+	nb_rxqs = RTE_MIN(dev->data->nb_rx_queues, RTE_ETHDEV_QUEUE_STAT_CNTRS);
 	for (q = 0; q < nb_rxqs; q++) {
-		for (i = 0; i < RTE_NB_RXQ_STATS; i++) {
+		for (i = 0; i < RTE_NB_LEGACY_RXQ_STATS; i++) {
 			stats_ptr = RTE_PTR_ADD(&eth_stats,
-					rte_rxq_stats_strings[i].offset +
+					legacy_rxq_stats_map[i].offset +
 					q * sizeof(uint64_t));
 			val = *stats_ptr;
 			xstats[count++].value = val;
 		}
 	}
 
+skip_legacy_rxq:
 	/* per-txq stats */
+	if (dev->dev_ops->txq_stats_get) {
+		nb_txqs = dev->data->nb_tx_queues;
+		for (q = 0; q < nb_txqs; q++) {
+			struct pmd_eth_txq_stats txq_stats;
+
+			memset(&txq_stats, 0, sizeof(txq_stats));
+			if (dev->dev_ops->txq_stats_get(dev, q, &txq_stats)) {
+				count += RTE_NB_TXQ_STATS;
+				continue;
+			}
+			for (i = 0; i < RTE_NB_TXQ_STATS; i++) {
+				stats_ptr = RTE_PTR_ADD(&txq_stats,
+						txq_stats_map[i].offset);
+				val = *stats_ptr;
+				xstats[count++].value = val;
+			}
+		}
+		goto skip_legacy_txq;
+	}
+
+	nb_txqs = RTE_MIN(dev->data->nb_tx_queues, RTE_ETHDEV_QUEUE_STAT_CNTRS);
 	for (q = 0; q < nb_txqs; q++) {
-		for (i = 0; i < RTE_NB_TXQ_STATS; i++) {
+		for (i = 0; i < RTE_NB_LEGACY_TXQ_STATS; i++) {
 			stats_ptr = RTE_PTR_ADD(&eth_stats,
-					rte_txq_stats_strings[i].offset +
+					legacy_txq_stats_map[i].offset +
 					q * sizeof(uint64_t));
 			val = *stats_ptr;
 			xstats[count++].value = val;
 		}
 	}
+
+skip_legacy_txq:
 	return count;
 }
 
@@ -2387,19 +2523,14 @@ struct rte_eth_dev *
 	struct rte_eth_dev *dev;
 	unsigned int count = 0, i;
 	signed int xcount = 0;
-	uint16_t nb_rxqs, nb_txqs;
 	int ret;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
 
 	dev = &rte_eth_devices[port_id];
 
-	nb_rxqs = RTE_MIN(dev->data->nb_rx_queues, RTE_ETHDEV_QUEUE_STAT_CNTRS);
-	nb_txqs = RTE_MIN(dev->data->nb_tx_queues, RTE_ETHDEV_QUEUE_STAT_CNTRS);
-
 	/* Return generic statistics */
-	count = RTE_NB_STATS + (nb_rxqs * RTE_NB_RXQ_STATS) +
-		(nb_txqs * RTE_NB_TXQ_STATS);
+	count = get_xstats_basic_count(dev);
 
 	/* implemented by the driver */
 	if (dev->dev_ops->xstats_get != NULL) {
diff --git a/lib/librte_ethdev/rte_ethdev_core.h b/lib/librte_ethdev/rte_ethdev_core.h
index 8f03f83..63375fe 100644
--- a/lib/librte_ethdev/rte_ethdev_core.h
+++ b/lib/librte_ethdev/rte_ethdev_core.h
@@ -97,6 +97,16 @@ typedef int (*eth_xstats_get_names_by_id_t)(struct rte_eth_dev *dev,
 	unsigned int size);
 /**< @internal Get names of extended stats of an Ethernet device. */
 
+struct pmd_eth_rxq_stats;
+typedef int (*eth_rxq_stats_get)(struct rte_eth_dev *dev, uint16_t rx_queue_id,
+	struct pmd_eth_rxq_stats *rx_queue_stats);
+/**< @internal Get statistics for a rx queue of an Ethernet device. */
+
+struct pmd_eth_txq_stats;
+typedef int (*eth_txq_stats_get)(struct rte_eth_dev *dev, uint16_t tx_queue_id,
+	struct pmd_eth_txq_stats *tx_queue_stats);
+/**< @internal Get statistics for a tx queue of an Ethernet device. */
+
 typedef int (*eth_queue_stats_mapping_set_t)(struct rte_eth_dev *dev,
 					     uint16_t queue_id,
 					     uint8_t stat_idx,
@@ -501,6 +511,9 @@ struct eth_dev_ops {
 	eth_xstats_get_names_by_id_t xstats_get_names_by_id;
 	/**< Get name of extended device statistics by ID. */
 
+	eth_rxq_stats_get rxq_stats_get; /**< Stats per rxq */
+	eth_txq_stats_get txq_stats_get; /**< Stats per txq */
+
 	eth_tm_ops_get_t tm_ops_get;
 	/**< Get Traffic Management (TM) operations. */
 
diff --git a/lib/librte_ethdev/rte_ethdev_driver.h b/lib/librte_ethdev/rte_ethdev_driver.h
index c2ac263..33a4b22 100644
--- a/lib/librte_ethdev/rte_ethdev_driver.h
+++ b/lib/librte_ethdev/rte_ethdev_driver.h
@@ -331,6 +331,24 @@ typedef int (*ethdev_bus_specific_init)(struct rte_eth_dev *ethdev,
 int __rte_experimental
 rte_eth_dev_destroy(struct rte_eth_dev *ethdev, ethdev_uninit_t ethdev_uninit);
 
+/**
+ * @internal
+ *
+ * Internal structures used by PMD to provide the per rx/tx queues to the
+ * ethdev layer.
+ */
+struct pmd_eth_rxq_stats {
+	uint64_t packets;
+	uint64_t bytes;
+	uint64_t errors;
+};
+
+struct pmd_eth_txq_stats {
+	uint64_t packets;
+	uint64_t bytes;
+	uint64_t errors;
+};
+
 #ifdef __cplusplus
 }
 #endif
-- 
1.8.3.1

[dpdk-dev] [RFC PATCH 1/2] ethdev: introduce internal rxq/txq stats API

[David Marchand]

Introduce a new api to retrieve per queue statistics from the drivers.
The api objectives:
- easily add some common per queue statistics and have it exposed
  through the user xstats api while the user stats api is left untouched
- remove the limitations on the per queue statistics count (inherited
  from ixgbe) and avoid recurrent bugs on stats array overflow

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 lib/librte_ethdev/rte_ethdev.c        | 191 ++++++++++++++++++++++++++++------
 lib/librte_ethdev/rte_ethdev_core.h   |  13 +++
 lib/librte_ethdev/rte_ethdev_driver.h |  18 ++++
 3 files changed, 192 insertions(+), 30 deletions(-)

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 85c1794..058fbd1 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -88,21 +88,30 @@ struct rte_eth_xstats_name_off {
 
 #define RTE_NB_STATS (sizeof(rte_stats_strings) / sizeof(rte_stats_strings[0]))
 
-static const struct rte_eth_xstats_name_off rte_rxq_stats_strings[] = {
+static const struct rte_eth_xstats_name_off legacy_rxq_stats_map[] = {
 	{"packets", offsetof(struct rte_eth_stats, q_ipackets)},
 	{"bytes", offsetof(struct rte_eth_stats, q_ibytes)},
 	{"errors", offsetof(struct rte_eth_stats, q_errors)},
 };
+#define RTE_NB_LEGACY_RXQ_STATS RTE_DIM(legacy_rxq_stats_map)
+static const struct rte_eth_xstats_name_off rxq_stats_map[] = {
+	{"packets", offsetof(struct pmd_eth_rxq_stats, packets)},
+	{"bytes", offsetof(struct pmd_eth_rxq_stats, bytes)},
+	{"errors", offsetof(struct pmd_eth_rxq_stats, errors)},
+};
+#define RTE_NB_RXQ_STATS RTE_DIM(rxq_stats_map)
 
-#define RTE_NB_RXQ_STATS (sizeof(rte_rxq_stats_strings) /	\
-		sizeof(rte_rxq_stats_strings[0]))
-
-static const struct rte_eth_xstats_name_off rte_txq_stats_strings[] = {
+static const struct rte_eth_xstats_name_off legacy_txq_stats_map[] = {
 	{"packets", offsetof(struct rte_eth_stats, q_opackets)},
 	{"bytes", offsetof(struct rte_eth_stats, q_obytes)},
 };
-#define RTE_NB_TXQ_STATS (sizeof(rte_txq_stats_strings) /	\
-		sizeof(rte_txq_stats_strings[0]))
+#define RTE_NB_LEGACY_TXQ_STATS RTE_DIM(legacy_txq_stats_map)
+static const struct rte_eth_xstats_name_off txq_stats_map[] = {
+	{"packets", offsetof(struct pmd_eth_txq_stats, packets)},
+	{"bytes", offsetof(struct pmd_eth_txq_stats, bytes)},
+	{"errors", offsetof(struct pmd_eth_txq_stats, errors)},
+};
+#define RTE_NB_TXQ_STATS RTE_DIM(txq_stats_map)
 
 #define RTE_RX_OFFLOAD_BIT2STR(_name)	\
 	{ DEV_RX_OFFLOAD_##_name, #_name }
@@ -1937,6 +1946,10 @@ struct rte_eth_dev *
 rte_eth_stats_get(uint16_t port_id, struct rte_eth_stats *stats)
 {
 	struct rte_eth_dev *dev;
+	unsigned int nb_rxqs;
+	unsigned int nb_txqs;
+	unsigned int qid;
+	int ret;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
 
@@ -1945,7 +1958,44 @@ struct rte_eth_dev *
 
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->stats_get, -ENOTSUP);
 	stats->rx_nombuf = dev->data->rx_mbuf_alloc_failed;
-	return eth_err(port_id, (*dev->dev_ops->stats_get)(dev, stats));
+	ret = eth_err(port_id, (*dev->dev_ops->stats_get)(dev, stats));
+	if (ret)
+		goto out;
+
+	if (!dev->dev_ops->rxq_stats_get)
+		goto skip_rxq;
+	nb_rxqs = RTE_MIN(dev->data->nb_rx_queues,
+			  RTE_ETHDEV_QUEUE_STAT_CNTRS);
+	for (qid = 0; qid < nb_rxqs; qid++) {
+		struct pmd_eth_rxq_stats rxq_stats;
+
+		memset(&rxq_stats, 0, sizeof(rxq_stats));
+		if (dev->dev_ops->rxq_stats_get(dev, qid, &rxq_stats))
+			continue;
+
+		stats->q_ipackets[qid] = rxq_stats.packets;
+		stats->q_ibytes[qid] = rxq_stats.bytes;
+		stats->q_errors[qid] = rxq_stats.errors;
+	}
+
+skip_rxq:
+	if (!dev->dev_ops->txq_stats_get)
+		goto out;
+	nb_txqs = RTE_MIN(dev->data->nb_tx_queues,
+			  RTE_ETHDEV_QUEUE_STAT_CNTRS);
+	for (qid = 0; qid < nb_txqs; qid++) {
+		struct pmd_eth_txq_stats txq_stats;
+
+		memset(&txq_stats, 0, sizeof(txq_stats));
+		if (dev->dev_ops->txq_stats_get(dev, qid, &txq_stats))
+			continue;
+
+		stats->q_opackets[qid] = txq_stats.packets;
+		stats->q_obytes[qid] = txq_stats.bytes;
+	}
+
+out:
+	return ret;
 }
 
 int
@@ -1969,12 +2019,24 @@ struct rte_eth_dev *
 	uint16_t nb_rxqs, nb_txqs;
 	int count;
 
-	nb_rxqs = RTE_MIN(dev->data->nb_rx_queues, RTE_ETHDEV_QUEUE_STAT_CNTRS);
-	nb_txqs = RTE_MIN(dev->data->nb_tx_queues, RTE_ETHDEV_QUEUE_STAT_CNTRS);
-
 	count = RTE_NB_STATS;
-	count += nb_rxqs * RTE_NB_RXQ_STATS;
-	count += nb_txqs * RTE_NB_TXQ_STATS;
+	if (dev->dev_ops->rxq_stats_get) {
+		nb_rxqs = dev->data->nb_rx_queues;
+		count += nb_rxqs * RTE_NB_RXQ_STATS;
+	} else {
+		nb_rxqs = RTE_MIN(dev->data->nb_rx_queues,
+				  RTE_ETHDEV_QUEUE_STAT_CNTRS);
+		count += nb_rxqs * RTE_NB_LEGACY_RXQ_STATS;
+	}
+
+	if (dev->dev_ops->txq_stats_get) {
+		nb_txqs = dev->data->nb_tx_queues;
+		count += nb_txqs * RTE_NB_TXQ_STATS;
+	} else {
+		nb_txqs = RTE_MIN(dev->data->nb_tx_queues,
+				  RTE_ETHDEV_QUEUE_STAT_CNTRS);
+		count += nb_txqs * RTE_NB_LEGACY_TXQ_STATS;
+	}
 
 	return count;
 }
@@ -2065,27 +2127,59 @@ struct rte_eth_dev *
 			"%s", rte_stats_strings[idx].name);
 		cnt_used_entries++;
 	}
+
+	if (dev->dev_ops->rxq_stats_get) {
+		num_q = dev->data->nb_rx_queues;
+		for (id_queue = 0; id_queue < num_q; id_queue++) {
+			for (idx = 0; idx < RTE_NB_RXQ_STATS; idx++) {
+				snprintf(xstats_names[cnt_used_entries].name,
+					sizeof(xstats_names[0].name),
+					"rx_q%u%s",
+					id_queue, rxq_stats_map[idx].name);
+				cnt_used_entries++;
+			}
+		}
+		goto skip_legacy_rxq;
+	}
+
 	num_q = RTE_MIN(dev->data->nb_rx_queues, RTE_ETHDEV_QUEUE_STAT_CNTRS);
 	for (id_queue = 0; id_queue < num_q; id_queue++) {
-		for (idx = 0; idx < RTE_NB_RXQ_STATS; idx++) {
+		for (idx = 0; idx < RTE_NB_LEGACY_RXQ_STATS; idx++) {
 			snprintf(xstats_names[cnt_used_entries].name,
 				sizeof(xstats_names[0].name),
 				"rx_q%u%s",
-				id_queue, rte_rxq_stats_strings[idx].name);
+				id_queue, legacy_rxq_stats_map[idx].name);
 			cnt_used_entries++;
 		}
+	}
 
+skip_legacy_rxq:
+	if (dev->dev_ops->txq_stats_get) {
+		num_q = dev->data->nb_tx_queues;
+		for (id_queue = 0; id_queue < num_q; id_queue++) {
+			for (idx = 0; idx < RTE_NB_TXQ_STATS; idx++) {
+				snprintf(xstats_names[cnt_used_entries].name,
+					sizeof(xstats_names[0].name),
+					"tx_q%u%s",
+					id_queue, txq_stats_map[idx].name);
+				cnt_used_entries++;
+			}
+		}
+		goto skip_legacy_txq;
 	}
+
 	num_q = RTE_MIN(dev->data->nb_tx_queues, RTE_ETHDEV_QUEUE_STAT_CNTRS);
 	for (id_queue = 0; id_queue < num_q; id_queue++) {
-		for (idx = 0; idx < RTE_NB_TXQ_STATS; idx++) {
+		for (idx = 0; idx < RTE_NB_LEGACY_TXQ_STATS; idx++) {
 			snprintf(xstats_names[cnt_used_entries].name,
 				sizeof(xstats_names[0].name),
 				"tx_q%u%s",
-				id_queue, rte_txq_stats_strings[idx].name);
+				id_queue, legacy_txq_stats_map[idx].name);
 			cnt_used_entries++;
 		}
 	}
+
+skip_legacy_txq:
 	return cnt_used_entries;
 }
 
@@ -2252,9 +2346,6 @@ struct rte_eth_dev *
 
 	dev = &rte_eth_devices[port_id];
 
-	nb_rxqs = RTE_MIN(dev->data->nb_rx_queues, RTE_ETHDEV_QUEUE_STAT_CNTRS);
-	nb_txqs = RTE_MIN(dev->data->nb_tx_queues, RTE_ETHDEV_QUEUE_STAT_CNTRS);
-
 	/* global stats */
 	for (i = 0; i < RTE_NB_STATS; i++) {
 		stats_ptr = RTE_PTR_ADD(&eth_stats,
@@ -2264,26 +2355,71 @@ struct rte_eth_dev *
 	}
 
 	/* per-rxq stats */
+	if (dev->dev_ops->rxq_stats_get) {
+		nb_rxqs = dev->data->nb_rx_queues;
+		for (q = 0; q < nb_rxqs; q++) {
+			struct pmd_eth_rxq_stats rxq_stats;
+
+			memset(&rxq_stats, 0, sizeof(rxq_stats));
+			if (dev->dev_ops->rxq_stats_get(dev, q, &rxq_stats)) {
+				count += RTE_NB_RXQ_STATS;
+				continue;
+			}
+			for (i = 0; i < RTE_NB_RXQ_STATS; i++) {
+				stats_ptr = RTE_PTR_ADD(&rxq_stats,
+						rxq_stats_map[i].offset);
+				val = *stats_ptr;
+				xstats[count++].value = val;
+			}
+		}
+		goto skip_legacy_rxq;
+	}
+
+	nb_rxqs = RTE_MIN(dev->data->nb_rx_queues, RTE_ETHDEV_QUEUE_STAT_CNTRS);
 	for (q = 0; q < nb_rxqs; q++) {
-		for (i = 0; i < RTE_NB_RXQ_STATS; i++) {
+		for (i = 0; i < RTE_NB_LEGACY_RXQ_STATS; i++) {
 			stats_ptr = RTE_PTR_ADD(&eth_stats,
-					rte_rxq_stats_strings[i].offset +
+					legacy_rxq_stats_map[i].offset +
 					q * sizeof(uint64_t));
 			val = *stats_ptr;
 			xstats[count++].value = val;
 		}
 	}
 
+skip_legacy_rxq:
 	/* per-txq stats */
+	if (dev->dev_ops->txq_stats_get) {
+		nb_txqs = dev->data->nb_tx_queues;
+		for (q = 0; q < nb_txqs; q++) {
+			struct pmd_eth_txq_stats txq_stats;
+
+			memset(&txq_stats, 0, sizeof(txq_stats));
+			if (dev->dev_ops->txq_stats_get(dev, q, &txq_stats)) {
+				count += RTE_NB_TXQ_STATS;
+				continue;
+			}
+			for (i = 0; i < RTE_NB_TXQ_STATS; i++) {
+				stats_ptr = RTE_PTR_ADD(&txq_stats,
+						txq_stats_map[i].offset);
+				val = *stats_ptr;
+				xstats[count++].value = val;
+			}
+		}
+		goto skip_legacy_txq;
+	}
+
+	nb_txqs = RTE_MIN(dev->data->nb_tx_queues, RTE_ETHDEV_QUEUE_STAT_CNTRS);
 	for (q = 0; q < nb_txqs; q++) {
-		for (i = 0; i < RTE_NB_TXQ_STATS; i++) {
+		for (i = 0; i < RTE_NB_LEGACY_TXQ_STATS; i++) {
 			stats_ptr = RTE_PTR_ADD(&eth_stats,
-					rte_txq_stats_strings[i].offset +
+					legacy_txq_stats_map[i].offset +
 					q * sizeof(uint64_t));
 			val = *stats_ptr;
 			xstats[count++].value = val;
 		}
 	}
+
+skip_legacy_txq:
 	return count;
 }
 
@@ -2387,19 +2523,14 @@ struct rte_eth_dev *
 	struct rte_eth_dev *dev;
 	unsigned int count = 0, i;
 	signed int xcount = 0;
-	uint16_t nb_rxqs, nb_txqs;
 	int ret;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
 
 	dev = &rte_eth_devices[port_id];
 
-	nb_rxqs = RTE_MIN(dev->data->nb_rx_queues, RTE_ETHDEV_QUEUE_STAT_CNTRS);
-	nb_txqs = RTE_MIN(dev->data->nb_tx_queues, RTE_ETHDEV_QUEUE_STAT_CNTRS);
-
 	/* Return generic statistics */
-	count = RTE_NB_STATS + (nb_rxqs * RTE_NB_RXQ_STATS) +
-		(nb_txqs * RTE_NB_TXQ_STATS);
+	count = get_xstats_basic_count(dev);
 
 	/* implemented by the driver */
 	if (dev->dev_ops->xstats_get != NULL) {
diff --git a/lib/librte_ethdev/rte_ethdev_core.h b/lib/librte_ethdev/rte_ethdev_core.h
index 8f03f83..63375fe 100644
--- a/lib/librte_ethdev/rte_ethdev_core.h
+++ b/lib/librte_ethdev/rte_ethdev_core.h
@@ -97,6 +97,16 @@ typedef int (*eth_xstats_get_names_by_id_t)(struct rte_eth_dev *dev,
 	unsigned int size);
 /**< @internal Get names of extended stats of an Ethernet device. */
 
+struct pmd_eth_rxq_stats;
+typedef int (*eth_rxq_stats_get)(struct rte_eth_dev *dev, uint16_t rx_queue_id,
+	struct pmd_eth_rxq_stats *rx_queue_stats);
+/**< @internal Get statistics for a rx queue of an Ethernet device. */
+
+struct pmd_eth_txq_stats;
+typedef int (*eth_txq_stats_get)(struct rte_eth_dev *dev, uint16_t tx_queue_id,
+	struct pmd_eth_txq_stats *tx_queue_stats);
+/**< @internal Get statistics for a tx queue of an Ethernet device. */
+
 typedef int (*eth_queue_stats_mapping_set_t)(struct rte_eth_dev *dev,
 					     uint16_t queue_id,
 					     uint8_t stat_idx,
@@ -501,6 +511,9 @@ struct eth_dev_ops {
 	eth_xstats_get_names_by_id_t xstats_get_names_by_id;
 	/**< Get name of extended device statistics by ID. */
 
+	eth_rxq_stats_get rxq_stats_get; /**< Stats per rxq */
+	eth_txq_stats_get txq_stats_get; /**< Stats per txq */
+
 	eth_tm_ops_get_t tm_ops_get;
 	/**< Get Traffic Management (TM) operations. */
 
diff --git a/lib/librte_ethdev/rte_ethdev_driver.h b/lib/librte_ethdev/rte_ethdev_driver.h
index c2ac263..33a4b22 100644
--- a/lib/librte_ethdev/rte_ethdev_driver.h
+++ b/lib/librte_ethdev/rte_ethdev_driver.h
@@ -331,6 +331,24 @@ typedef int (*ethdev_bus_specific_init)(struct rte_eth_dev *ethdev,
 int __rte_experimental
 rte_eth_dev_destroy(struct rte_eth_dev *ethdev, ethdev_uninit_t ethdev_uninit);
 
+/**
+ * @internal
+ *
+ * Internal structures used by PMD to provide the per rx/tx queues to the
+ * ethdev layer.
+ */
+struct pmd_eth_rxq_stats {
+	uint64_t packets;
+	uint64_t bytes;
+	uint64_t errors;
+};
+
+struct pmd_eth_txq_stats {
+	uint64_t packets;
+	uint64_t bytes;
+	uint64_t errors;
+};
+
 #ifdef __cplusplus
 }
 #endif
-- 
1.8.3.1

[dpdk-dev] [RFC PATCH 2/2] net/af_packet: convert to new rxq/txq stats API

[David Marchand]

Nothing more than the title.

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 drivers/net/af_packet/rte_eth_af_packet.c | 49 +++++++++++++++++++++----------
 1 file changed, 33 insertions(+), 16 deletions(-)

diff --git a/drivers/net/af_packet/rte_eth_af_packet.c b/drivers/net/af_packet/rte_eth_af_packet.c
index ec90cc0..cd36eb6 100644
--- a/drivers/net/af_packet/rte_eth_af_packet.c
+++ b/drivers/net/af_packet/rte_eth_af_packet.c
@@ -310,28 +310,20 @@ struct pmd_internals {
 static int
 eth_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *igb_stats)
 {
-	unsigned i, imax;
+	const struct pmd_internals *internal = dev->data->dev_private;
 	unsigned long rx_total = 0, tx_total = 0, tx_err_total = 0;
 	unsigned long rx_bytes_total = 0, tx_bytes_total = 0;
-	const struct pmd_internals *internal = dev->data->dev_private;
+	unsigned int i;
 
-	imax = (internal->nb_queues < RTE_ETHDEV_QUEUE_STAT_CNTRS ?
-	        internal->nb_queues : RTE_ETHDEV_QUEUE_STAT_CNTRS);
-	for (i = 0; i < imax; i++) {
-		igb_stats->q_ipackets[i] = internal->rx_queue[i].rx_pkts;
-		igb_stats->q_ibytes[i] = internal->rx_queue[i].rx_bytes;
-		rx_total += igb_stats->q_ipackets[i];
-		rx_bytes_total += igb_stats->q_ibytes[i];
+	for (i = 0; i < internal->nb_queues; i++) {
+		rx_total += internal->rx_queue[i].rx_pkts;
+		rx_bytes_total += internal->rx_queue[i].rx_bytes;
 	}
 
-	imax = (internal->nb_queues < RTE_ETHDEV_QUEUE_STAT_CNTRS ?
-	        internal->nb_queues : RTE_ETHDEV_QUEUE_STAT_CNTRS);
-	for (i = 0; i < imax; i++) {
-		igb_stats->q_opackets[i] = internal->tx_queue[i].tx_pkts;
-		igb_stats->q_obytes[i] = internal->tx_queue[i].tx_bytes;
-		tx_total += igb_stats->q_opackets[i];
+	for (i = 0; i < internal->nb_queues; i++) {
+		tx_total += internal->tx_queue[i].tx_pkts;
 		tx_err_total += internal->tx_queue[i].err_pkts;
-		tx_bytes_total += igb_stats->q_obytes[i];
+		tx_bytes_total += internal->tx_queue[i].tx_bytes;
 	}
 
 	igb_stats->ipackets = rx_total;
@@ -342,6 +334,29 @@ struct pmd_internals {
 	return 0;
 }
 
+static int
+rxq_stats_get(struct rte_eth_dev *dev, uint16_t rx_queue_id,
+	      struct pmd_eth_rxq_stats *rxq_stats)
+{
+	const struct pmd_internals *internal = dev->data->dev_private;
+
+	rxq_stats->packets = internal->rx_queue[rx_queue_id].rx_pkts;
+	rxq_stats->bytes = internal->rx_queue[rx_queue_id].rx_bytes;
+	return 0;
+}
+
+static int
+txq_stats_get(struct rte_eth_dev *dev, uint16_t tx_queue_id,
+	      struct pmd_eth_txq_stats *txq_stats)
+{
+	const struct pmd_internals *internal = dev->data->dev_private;
+
+	txq_stats->packets = internal->tx_queue[tx_queue_id].tx_pkts;
+	txq_stats->bytes = internal->tx_queue[tx_queue_id].tx_bytes;
+	txq_stats->errors = internal->tx_queue[tx_queue_id].err_pkts;
+	return 0;
+}
+
 static void
 eth_stats_reset(struct rte_eth_dev *dev)
 {
@@ -503,6 +518,8 @@ struct pmd_internals {
 	.tx_queue_release = eth_queue_release,
 	.link_update = eth_link_update,
 	.stats_get = eth_stats_get,
+	.rxq_stats_get = rxq_stats_get,
+	.txq_stats_get = txq_stats_get,
 	.stats_reset = eth_stats_reset,
 };
 
-- 
1.8.3.1

[dpdk-dev] [RFC PATCH 2/2] net/af_packet: convert to new rxq/txq stats API

[David Marchand]

Nothing more than the title.

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 drivers/net/af_packet/rte_eth_af_packet.c | 49 +++++++++++++++++++++----------
 1 file changed, 33 insertions(+), 16 deletions(-)

diff --git a/drivers/net/af_packet/rte_eth_af_packet.c b/drivers/net/af_packet/rte_eth_af_packet.c
index ec90cc0..cd36eb6 100644
--- a/drivers/net/af_packet/rte_eth_af_packet.c
+++ b/drivers/net/af_packet/rte_eth_af_packet.c
@@ -310,28 +310,20 @@ struct pmd_internals {
 static int
 eth_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *igb_stats)
 {
-	unsigned i, imax;
+	const struct pmd_internals *internal = dev->data->dev_private;
 	unsigned long rx_total = 0, tx_total = 0, tx_err_total = 0;
 	unsigned long rx_bytes_total = 0, tx_bytes_total = 0;
-	const struct pmd_internals *internal = dev->data->dev_private;
+	unsigned int i;
 
-	imax = (internal->nb_queues < RTE_ETHDEV_QUEUE_STAT_CNTRS ?
-	        internal->nb_queues : RTE_ETHDEV_QUEUE_STAT_CNTRS);
-	for (i = 0; i < imax; i++) {
-		igb_stats->q_ipackets[i] = internal->rx_queue[i].rx_pkts;
-		igb_stats->q_ibytes[i] = internal->rx_queue[i].rx_bytes;
-		rx_total += igb_stats->q_ipackets[i];
-		rx_bytes_total += igb_stats->q_ibytes[i];
+	for (i = 0; i < internal->nb_queues; i++) {
+		rx_total += internal->rx_queue[i].rx_pkts;
+		rx_bytes_total += internal->rx_queue[i].rx_bytes;
 	}
 
-	imax = (internal->nb_queues < RTE_ETHDEV_QUEUE_STAT_CNTRS ?
-	        internal->nb_queues : RTE_ETHDEV_QUEUE_STAT_CNTRS);
-	for (i = 0; i < imax; i++) {
-		igb_stats->q_opackets[i] = internal->tx_queue[i].tx_pkts;
-		igb_stats->q_obytes[i] = internal->tx_queue[i].tx_bytes;
-		tx_total += igb_stats->q_opackets[i];
+	for (i = 0; i < internal->nb_queues; i++) {
+		tx_total += internal->tx_queue[i].tx_pkts;
 		tx_err_total += internal->tx_queue[i].err_pkts;
-		tx_bytes_total += igb_stats->q_obytes[i];
+		tx_bytes_total += internal->tx_queue[i].tx_bytes;
 	}
 
 	igb_stats->ipackets = rx_total;
@@ -342,6 +334,29 @@ struct pmd_internals {
 	return 0;
 }
 
+static int
+rxq_stats_get(struct rte_eth_dev *dev, uint16_t rx_queue_id,
+	      struct pmd_eth_rxq_stats *rxq_stats)
+{
+	const struct pmd_internals *internal = dev->data->dev_private;
+
+	rxq_stats->packets = internal->rx_queue[rx_queue_id].rx_pkts;
+	rxq_stats->bytes = internal->rx_queue[rx_queue_id].rx_bytes;
+	return 0;
+}
+
+static int
+txq_stats_get(struct rte_eth_dev *dev, uint16_t tx_queue_id,
+	      struct pmd_eth_txq_stats *txq_stats)
+{
+	const struct pmd_internals *internal = dev->data->dev_private;
+
+	txq_stats->packets = internal->tx_queue[tx_queue_id].tx_pkts;
+	txq_stats->bytes = internal->tx_queue[tx_queue_id].tx_bytes;
+	txq_stats->errors = internal->tx_queue[tx_queue_id].err_pkts;
+	return 0;
+}
+
 static void
 eth_stats_reset(struct rte_eth_dev *dev)
 {
@@ -503,6 +518,8 @@ struct pmd_internals {
 	.tx_queue_release = eth_queue_release,
 	.link_update = eth_link_update,
 	.stats_get = eth_stats_get,
+	.rxq_stats_get = rxq_stats_get,
+	.txq_stats_get = txq_stats_get,
 	.stats_reset = eth_stats_reset,
 };
 
-- 
1.8.3.1

Re: [dpdk-dev] [RFC PATCH 1/2] ethdev: introduce internal rxq/txq stats API

[David Marchand]

On Thu, Mar 14, 2019 at 4:13 PM David Marchand <david.marchand@redhat.com>
wrote:

> Introduce a new api to retrieve per queue statistics from the drivers.
> The api objectives:
> - easily add some common per queue statistics and have it exposed
>   through the user xstats api while the user stats api is left untouched
> - remove the limitations on the per queue statistics count (inherited
>   from ixgbe) and avoid recurrent bugs on stats array overflow
>

I have looked at more drivers.
I will try to handle the case where drivers are only keeping tracks of per
q stats and aggregate then when called by .stats_get.
The drivers with hw counters would need both .stats_get and per q
functions, so I need to accomodate with this.
v2 rfc for next week unless someone totally disagrees with the approach
before :-)


-- 
David Marchand

Re: [dpdk-dev] [RFC PATCH 1/2] ethdev: introduce internal rxq/txq stats API

[David Marchand]

On Thu, Mar 14, 2019 at 4:13 PM David Marchand <david.marchand@redhat.com>
wrote:

> Introduce a new api to retrieve per queue statistics from the drivers.
> The api objectives:
> - easily add some common per queue statistics and have it exposed
>   through the user xstats api while the user stats api is left untouched
> - remove the limitations on the per queue statistics count (inherited
>   from ixgbe) and avoid recurrent bugs on stats array overflow
>

I have looked at more drivers.
I will try to handle the case where drivers are only keeping tracks of per
q stats and aggregate then when called by .stats_get.
The drivers with hw counters would need both .stats_get and per q
functions, so I need to accomodate with this.
v2 rfc for next week unless someone totally disagrees with the approach
before :-)


-- 
David Marchand

Re: [dpdk-dev] [RFC PATCH 1/2] ethdev: introduce internal rxq/txq stats API

[Ferruh Yigit]

On 3/14/2019 3:13 PM, David Marchand wrote:
> Introduce a new api to retrieve per queue statistics from the drivers.
> The api objectives:
> - easily add some common per queue statistics and have it exposed
>   through the user xstats api while the user stats api is left untouched
> - remove the limitations on the per queue statistics count (inherited
>   from ixgbe) and avoid recurrent bugs on stats array overflow

The patch is adding two new dev_ops 'rxq_stats_get' & 'txq_stats_get', my
concern is if it is overkill to have three dev_ops to get stats
and I am feeling that is making xstat code more complex.

Would it be simpler to add 'q_ierrors' & 'q_oerrors' to 'struct rte_eth_stats'?

And perhaps we can do the 'fix rxq q_errors' patchset [1] after this change, so
fix can be done with less changes, although it will push the fix into next
release because of the ABI break.
OR ethdev will be broken this release, because of max_mtu, since ABI is already
broken perhaps we can squeeze this in.

Overall I would like to get more comment on this, Andrew, Thomas?

> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>

<...>

Re: [dpdk-dev] [RFC PATCH 1/2] ethdev: introduce internal rxq/txq stats API

[Ferruh Yigit]

On 3/14/2019 3:13 PM, David Marchand wrote:
> Introduce a new api to retrieve per queue statistics from the drivers.
> The api objectives:
> - easily add some common per queue statistics and have it exposed
>   through the user xstats api while the user stats api is left untouched
> - remove the limitations on the per queue statistics count (inherited
>   from ixgbe) and avoid recurrent bugs on stats array overflow

The patch is adding two new dev_ops 'rxq_stats_get' & 'txq_stats_get', my
concern is if it is overkill to have three dev_ops to get stats
and I am feeling that is making xstat code more complex.

Would it be simpler to add 'q_ierrors' & 'q_oerrors' to 'struct rte_eth_stats'?

And perhaps we can do the 'fix rxq q_errors' patchset [1] after this change, so
fix can be done with less changes, although it will push the fix into next
release because of the ABI break.
OR ethdev will be broken this release, because of max_mtu, since ABI is already
broken perhaps we can squeeze this in.

Overall I would like to get more comment on this, Andrew, Thomas?

> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>

<...>

Re: [dpdk-dev] [RFC PATCH 1/2] ethdev: introduce internal rxq/txq stats API

[Stephen Hemminger]

On Tue, 19 Mar 2019 17:18:08 +0000
Ferruh Yigit <ferruh.yigit@intel.com> wrote:

> On 3/14/2019 3:13 PM, David Marchand wrote:
> > Introduce a new api to retrieve per queue statistics from the drivers.
> > The api objectives:
> > - easily add some common per queue statistics and have it exposed
> >   through the user xstats api while the user stats api is left untouched
> > - remove the limitations on the per queue statistics count (inherited
> >   from ixgbe) and avoid recurrent bugs on stats array overflow  
> 
> The patch is adding two new dev_ops 'rxq_stats_get' & 'txq_stats_get', my
> concern is if it is overkill to have three dev_ops to get stats
> and I am feeling that is making xstat code more complex.
> 
> Would it be simpler to add 'q_ierrors' & 'q_oerrors' to 'struct rte_eth_stats'?
> 
> And perhaps we can do the 'fix rxq q_errors' patchset [1] after this change, so
> fix can be done with less changes, although it will push the fix into next
> release because of the ABI break.
> OR ethdev will be broken this release, because of max_mtu, since ABI is already
> broken perhaps we can squeeze this in.
> 
> Overall I would like to get more comment on this, Andrew, Thomas?
> 
> > 
> > Signed-off-by: David Marchand <david.marchand@redhat.com>  
> 
> <...>
> 

My preference would be:
  1. Make all DPDK drivers consistent in usage of current statistic values.
  2. Propose an enhancement to have new ethdev statistics match some pre-existing
     standard like SNMP or other RFC.
  3. Reduce custom (xstats) values by using #2. Leave it for driver specific stuff.

I.e: don't modify existing API at all, make a new one.

PS: ethdev is one of those structures that needs to get removed/hidden from
application headers.  It should be possible to add/remove stuff from ethdev internals, device and bus
without breaking API/ABI.

Re: [dpdk-dev] [RFC PATCH 1/2] ethdev: introduce internal rxq/txq stats API

[Stephen Hemminger]

On Tue, 19 Mar 2019 17:18:08 +0000
Ferruh Yigit <ferruh.yigit@intel.com> wrote:

> On 3/14/2019 3:13 PM, David Marchand wrote:
> > Introduce a new api to retrieve per queue statistics from the drivers.
> > The api objectives:
> > - easily add some common per queue statistics and have it exposed
> >   through the user xstats api while the user stats api is left untouched
> > - remove the limitations on the per queue statistics count (inherited
> >   from ixgbe) and avoid recurrent bugs on stats array overflow  
> 
> The patch is adding two new dev_ops 'rxq_stats_get' & 'txq_stats_get', my
> concern is if it is overkill to have three dev_ops to get stats
> and I am feeling that is making xstat code more complex.
> 
> Would it be simpler to add 'q_ierrors' & 'q_oerrors' to 'struct rte_eth_stats'?
> 
> And perhaps we can do the 'fix rxq q_errors' patchset [1] after this change, so
> fix can be done with less changes, although it will push the fix into next
> release because of the ABI break.
> OR ethdev will be broken this release, because of max_mtu, since ABI is already
> broken perhaps we can squeeze this in.
> 
> Overall I would like to get more comment on this, Andrew, Thomas?
> 
> > 
> > Signed-off-by: David Marchand <david.marchand@redhat.com>  
> 
> <...>
> 

My preference would be:
  1. Make all DPDK drivers consistent in usage of current statistic values.
  2. Propose an enhancement to have new ethdev statistics match some pre-existing
     standard like SNMP or other RFC.
  3. Reduce custom (xstats) values by using #2. Leave it for driver specific stuff.

I.e: don't modify existing API at all, make a new one.

PS: ethdev is one of those structures that needs to get removed/hidden from
application headers.  It should be possible to add/remove stuff from ethdev internals, device and bus
without breaking API/ABI.

Re: [dpdk-dev] [RFC PATCH 1/2] ethdev: introduce internal rxq/txq stats API

[Thomas Monjalon]

19/03/2019 18:54, Stephen Hemminger:
> My preference would be:
>   1. Make all DPDK drivers consistent in usage of current statistic values.
>   2. Propose an enhancement to have new ethdev statistics match some pre-existing
>      standard like SNMP or other RFC.

This patch is about basic stats.
What would be different if looking at SNMP or other?

>   3. Reduce custom (xstats) values by using #2. Leave it for driver specific stuff.
> 
> I.e: don't modify existing API at all, make a new one.

This patch does not modify the API.

Re: [dpdk-dev] [RFC PATCH 1/2] ethdev: introduce internal rxq/txq stats API

[Thomas Monjalon]

19/03/2019 18:54, Stephen Hemminger:
> My preference would be:
>   1. Make all DPDK drivers consistent in usage of current statistic values.
>   2. Propose an enhancement to have new ethdev statistics match some pre-existing
>      standard like SNMP or other RFC.

This patch is about basic stats.
What would be different if looking at SNMP or other?

>   3. Reduce custom (xstats) values by using #2. Leave it for driver specific stuff.
> 
> I.e: don't modify existing API at all, make a new one.

This patch does not modify the API.

Re: [dpdk-dev] [RFC PATCH 1/2] ethdev: introduce internal rxq/txq stats API

[David Marchand]

On Tue, Mar 19, 2019 at 6:18 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:

> On 3/14/2019 3:13 PM, David Marchand wrote:
> > Introduce a new api to retrieve per queue statistics from the drivers.
> > The api objectives:
> > - easily add some common per queue statistics and have it exposed
> >   through the user xstats api while the user stats api is left untouched
> > - remove the limitations on the per queue statistics count (inherited
> >   from ixgbe) and avoid recurrent bugs on stats array overflow
>
> The patch is adding two new dev_ops 'rxq_stats_get' & 'txq_stats_get', my
> concern is if it is overkill to have three dev_ops to get stats
> and I am feeling that is making xstat code more complex.
>

Having these new (meant to be) internal dev_ops has the avantage of
separating the statistics reported from the drivers from the exported api.
This is also why I did not prefix the structure names with rte_.

The "complex" part is in a single place, ethdev and this is when
translating from an internal representation to the exposed bits in the
public apis.


Would it be simpler to add 'q_ierrors' & 'q_oerrors' to 'struct
> rte_eth_stats'?
>

It does not solve the problem of drivers that are buggy because of the
limit on RTE_ETHDEV_QUEUE_STAT_CNTRS.
All drivers need to be aware of this limitation of the rte_eth_stats
structure.

The drivers manipulate queues in rx/tx_queue_setup dev_ops for example,
having rxq/txq_stats_get dev_ops is more consistent to me.


> And perhaps we can do the 'fix rxq q_errors' patchset [1] after this
> change, so
> fix can be done with less changes, although it will push the fix into next
> release because of the ABI break.
>

I am fine with merging this together, we don't want to backport this
anyway, right?

But so far, I don't feel the need to break the rte_eth_stats_get API, if we
want to go to this, we can define an entirely new api to expose
standardized bits and still use the rxq/txq_stats_get internal dev_ops I
propose.


Thomas, Andrew, can you provide feedback please ?
rc1 is coming.


-- 
David Marchand

Re: [dpdk-dev] [RFC PATCH 1/2] ethdev: introduce internal rxq/txq stats API

[David Marchand]

On Tue, Mar 19, 2019 at 6:18 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:

> On 3/14/2019 3:13 PM, David Marchand wrote:
> > Introduce a new api to retrieve per queue statistics from the drivers.
> > The api objectives:
> > - easily add some common per queue statistics and have it exposed
> >   through the user xstats api while the user stats api is left untouched
> > - remove the limitations on the per queue statistics count (inherited
> >   from ixgbe) and avoid recurrent bugs on stats array overflow
>
> The patch is adding two new dev_ops 'rxq_stats_get' & 'txq_stats_get', my
> concern is if it is overkill to have three dev_ops to get stats
> and I am feeling that is making xstat code more complex.
>

Having these new (meant to be) internal dev_ops has the avantage of
separating the statistics reported from the drivers from the exported api.
This is also why I did not prefix the structure names with rte_.

The "complex" part is in a single place, ethdev and this is when
translating from an internal representation to the exposed bits in the
public apis.


Would it be simpler to add 'q_ierrors' & 'q_oerrors' to 'struct
> rte_eth_stats'?
>

It does not solve the problem of drivers that are buggy because of the
limit on RTE_ETHDEV_QUEUE_STAT_CNTRS.
All drivers need to be aware of this limitation of the rte_eth_stats
structure.

The drivers manipulate queues in rx/tx_queue_setup dev_ops for example,
having rxq/txq_stats_get dev_ops is more consistent to me.


> And perhaps we can do the 'fix rxq q_errors' patchset [1] after this
> change, so
> fix can be done with less changes, although it will push the fix into next
> release because of the ABI break.
>

I am fine with merging this together, we don't want to backport this
anyway, right?

But so far, I don't feel the need to break the rte_eth_stats_get API, if we
want to go to this, we can define an entirely new api to expose
standardized bits and still use the rxq/txq_stats_get internal dev_ops I
propose.


Thomas, Andrew, can you provide feedback please ?
rc1 is coming.


-- 
David Marchand

Re: [dpdk-dev] [RFC PATCH 1/2] ethdev: introduce internal rxq/txq stats API

[Thomas Monjalon]

26/03/2019 10:29, David Marchand:
> On Tue, Mar 19, 2019 at 6:18 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> 
> > On 3/14/2019 3:13 PM, David Marchand wrote:
> > > Introduce a new api to retrieve per queue statistics from the drivers.
> > > The api objectives:
> > > - easily add some common per queue statistics and have it exposed
> > >   through the user xstats api while the user stats api is left untouched
> > > - remove the limitations on the per queue statistics count (inherited
> > >   from ixgbe) and avoid recurrent bugs on stats array overflow

First comment, I think it would be easier to read if renaming the legacy
basic stats interface was in a separate patch.

> > The patch is adding two new dev_ops 'rxq_stats_get' & 'txq_stats_get', my
> > concern is if it is overkill to have three dev_ops to get stats
> > and I am feeling that is making xstat code more complex.
> 
> Having these new (meant to be) internal dev_ops has the avantage of
> separating the statistics reported from the drivers from the exported api.
> This is also why I did not prefix the structure names with rte_.

Yes, and to make it clear, please do not talk about API,
as it is only a driver interface.

> The "complex" part is in a single place, ethdev and this is when
> translating from an internal representation to the exposed bits in the
> public apis.
> 
> Would it be simpler to add 'q_ierrors' & 'q_oerrors' to 'struct
> > rte_eth_stats'?
> >
> 
> It does not solve the problem of drivers that are buggy because of the
> limit on RTE_ETHDEV_QUEUE_STAT_CNTRS.
> All drivers need to be aware of this limitation of the rte_eth_stats
> structure.

Yes, this limitation should be dropped.
I would like to see the functions rte_eth_dev_set_?x_queue_stats_mapping()
deprecated as they were a bad abstraction of ixgbe limitation.

> The drivers manipulate queues in rx/tx_queue_setup dev_ops for example,
> having rxq/txq_stats_get dev_ops is more consistent to me.

+1

> > And perhaps we can do the 'fix rxq q_errors' patchset [1] after this
> > change, so
> > fix can be done with less changes, although it will push the fix into next
> > release because of the ABI break.
> 
> I am fine with merging this together, we don't want to backport this
> anyway, right?

No, it would make some behaviours changing in stable releases,
so better to not backport it and keep the buggy behaviour in old branches.

> But so far, I don't feel the need to break the rte_eth_stats_get API, if we
> want to go to this, we can define an entirely new api to expose
> standardized bits and still use the rxq/txq_stats_get internal dev_ops I
> propose.

Good

Re: [dpdk-dev] [RFC PATCH 1/2] ethdev: introduce internal rxq/txq stats API

[Thomas Monjalon]

26/03/2019 10:29, David Marchand:
> On Tue, Mar 19, 2019 at 6:18 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> 
> > On 3/14/2019 3:13 PM, David Marchand wrote:
> > > Introduce a new api to retrieve per queue statistics from the drivers.
> > > The api objectives:
> > > - easily add some common per queue statistics and have it exposed
> > >   through the user xstats api while the user stats api is left untouched
> > > - remove the limitations on the per queue statistics count (inherited
> > >   from ixgbe) and avoid recurrent bugs on stats array overflow

First comment, I think it would be easier to read if renaming the legacy
basic stats interface was in a separate patch.

> > The patch is adding two new dev_ops 'rxq_stats_get' & 'txq_stats_get', my
> > concern is if it is overkill to have three dev_ops to get stats
> > and I am feeling that is making xstat code more complex.
> 
> Having these new (meant to be) internal dev_ops has the avantage of
> separating the statistics reported from the drivers from the exported api.
> This is also why I did not prefix the structure names with rte_.

Yes, and to make it clear, please do not talk about API,
as it is only a driver interface.

> The "complex" part is in a single place, ethdev and this is when
> translating from an internal representation to the exposed bits in the
> public apis.
> 
> Would it be simpler to add 'q_ierrors' & 'q_oerrors' to 'struct
> > rte_eth_stats'?
> >
> 
> It does not solve the problem of drivers that are buggy because of the
> limit on RTE_ETHDEV_QUEUE_STAT_CNTRS.
> All drivers need to be aware of this limitation of the rte_eth_stats
> structure.

Yes, this limitation should be dropped.
I would like to see the functions rte_eth_dev_set_?x_queue_stats_mapping()
deprecated as they were a bad abstraction of ixgbe limitation.

> The drivers manipulate queues in rx/tx_queue_setup dev_ops for example,
> having rxq/txq_stats_get dev_ops is more consistent to me.

+1

> > And perhaps we can do the 'fix rxq q_errors' patchset [1] after this
> > change, so
> > fix can be done with less changes, although it will push the fix into next
> > release because of the ABI break.
> 
> I am fine with merging this together, we don't want to backport this
> anyway, right?

No, it would make some behaviours changing in stable releases,
so better to not backport it and keep the buggy behaviour in old branches.

> But so far, I don't feel the need to break the rte_eth_stats_get API, if we
> want to go to this, we can define an entirely new api to expose
> standardized bits and still use the rxq/txq_stats_get internal dev_ops I
> propose.

Good

Re: [dpdk-dev] [RFC PATCH 1/2] ethdev: introduce internal rxq/txq stats API

[David Marchand]

On Fri, Apr 12, 2019 at 3:29 PM Thomas Monjalon <thomas@monjalon.net> wrote:

> 26/03/2019 10:29, David Marchand:
> > On Tue, Mar 19, 2019 at 6:18 PM Ferruh Yigit <ferruh.yigit@intel.com>
> wrote:
> >
> > > On 3/14/2019 3:13 PM, David Marchand wrote:
> > > > Introduce a new api to retrieve per queue statistics from the
> drivers.
> > > > The api objectives:
> > > > - easily add some common per queue statistics and have it exposed
> > > >   through the user xstats api while the user stats api is left
> untouched
> > > > - remove the limitations on the per queue statistics count (inherited
> > > >   from ixgbe) and avoid recurrent bugs on stats array overflow
>
> First comment, I think it would be easier to read if renaming the legacy
> basic stats interface was in a separate patch.
>

It will be quite artificial, but I can do this yes.


> > > The patch is adding two new dev_ops 'rxq_stats_get' & 'txq_stats_get',
> my
> > > concern is if it is overkill to have three dev_ops to get stats
> > > and I am feeling that is making xstat code more complex.
> >
> > Having these new (meant to be) internal dev_ops has the avantage of
> > separating the statistics reported from the drivers from the exported
> api.
> > This is also why I did not prefix the structure names with rte_.
>
> Yes, and to make it clear, please do not talk about API,
> as it is only a driver interface.
>

Ok, so I will describe this as a "driver interface" update.



> > The "complex" part is in a single place, ethdev and this is when
> > translating from an internal representation to the exposed bits in the
> > public apis.
> >
> > Would it be simpler to add 'q_ierrors' & 'q_oerrors' to 'struct
> > > rte_eth_stats'?
> > >
> >
> > It does not solve the problem of drivers that are buggy because of the
> > limit on RTE_ETHDEV_QUEUE_STAT_CNTRS.
> > All drivers need to be aware of this limitation of the rte_eth_stats
> > structure.
>
> Yes, this limitation should be dropped.
> I would like to see the functions rte_eth_dev_set_?x_queue_stats_mapping()
> deprecated as they were a bad abstraction of ixgbe limitation.
>

That's a different topic from my pov, but yes, this mapping stuff should go
away, later.


> > And perhaps we can do the 'fix rxq q_errors' patchset [1] after this
> > > change, so
> > > fix can be done with less changes, although it will push the fix into
> next
> > > release because of the ABI break.
> >
> > I am fine with merging this together, we don't want to backport this
> > anyway, right?
>
> No, it would make some behaviours changing in stable releases,
> so better to not backport it and keep the buggy behaviour in old branches.
>

Since the time I had posted this RFC, I have worked on a RFC v2, I will
post this next week, with the drivers I found time to convert.
We will have to take a decision on what goes to -rc2 between this and the
q_errors[] patchset.


-- 
David Marchand

Re: [dpdk-dev] [RFC PATCH 1/2] ethdev: introduce internal rxq/txq stats API

[David Marchand]

On Fri, Apr 12, 2019 at 3:29 PM Thomas Monjalon <thomas@monjalon.net> wrote:

> 26/03/2019 10:29, David Marchand:
> > On Tue, Mar 19, 2019 at 6:18 PM Ferruh Yigit <ferruh.yigit@intel.com>
> wrote:
> >
> > > On 3/14/2019 3:13 PM, David Marchand wrote:
> > > > Introduce a new api to retrieve per queue statistics from the
> drivers.
> > > > The api objectives:
> > > > - easily add some common per queue statistics and have it exposed
> > > >   through the user xstats api while the user stats api is left
> untouched
> > > > - remove the limitations on the per queue statistics count (inherited
> > > >   from ixgbe) and avoid recurrent bugs on stats array overflow
>
> First comment, I think it would be easier to read if renaming the legacy
> basic stats interface was in a separate patch.
>

It will be quite artificial, but I can do this yes.


> > > The patch is adding two new dev_ops 'rxq_stats_get' & 'txq_stats_get',
> my
> > > concern is if it is overkill to have three dev_ops to get stats
> > > and I am feeling that is making xstat code more complex.
> >
> > Having these new (meant to be) internal dev_ops has the avantage of
> > separating the statistics reported from the drivers from the exported
> api.
> > This is also why I did not prefix the structure names with rte_.
>
> Yes, and to make it clear, please do not talk about API,
> as it is only a driver interface.
>

Ok, so I will describe this as a "driver interface" update.



> > The "complex" part is in a single place, ethdev and this is when
> > translating from an internal representation to the exposed bits in the
> > public apis.
> >
> > Would it be simpler to add 'q_ierrors' & 'q_oerrors' to 'struct
> > > rte_eth_stats'?
> > >
> >
> > It does not solve the problem of drivers that are buggy because of the
> > limit on RTE_ETHDEV_QUEUE_STAT_CNTRS.
> > All drivers need to be aware of this limitation of the rte_eth_stats
> > structure.
>
> Yes, this limitation should be dropped.
> I would like to see the functions rte_eth_dev_set_?x_queue_stats_mapping()
> deprecated as they were a bad abstraction of ixgbe limitation.
>

That's a different topic from my pov, but yes, this mapping stuff should go
away, later.


> > And perhaps we can do the 'fix rxq q_errors' patchset [1] after this
> > > change, so
> > > fix can be done with less changes, although it will push the fix into
> next
> > > release because of the ABI break.
> >
> > I am fine with merging this together, we don't want to backport this
> > anyway, right?
>
> No, it would make some behaviours changing in stable releases,
> so better to not backport it and keep the buggy behaviour in old branches.
>

Since the time I had posted this RFC, I have worked on a RFC v2, I will
post this next week, with the drivers I found time to convert.
We will have to take a decision on what goes to -rc2 between this and the
q_errors[] patchset.


-- 
David Marchand

Re: [dpdk-dev] [RFC PATCH 1/2] ethdev: introduce internal rxq/txq stats API

[Stephen Hemminger]

On Fri, 12 Apr 2019 16:32:01 +0200
David Marchand <david.marchand@redhat.com> wrote:

> On Fri, Apr 12, 2019 at 3:29 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> 
> > 26/03/2019 10:29, David Marchand:  
> > > On Tue, Mar 19, 2019 at 6:18 PM Ferruh Yigit <ferruh.yigit@intel.com>  
> > wrote:  
> > >  
> > > > On 3/14/2019 3:13 PM, David Marchand wrote:  
> > > > > Introduce a new api to retrieve per queue statistics from the  
> > drivers.  
> > > > > The api objectives:
> > > > > - easily add some common per queue statistics and have it exposed
> > > > >   through the user xstats api while the user stats api is left  
> > untouched  
> > > > > - remove the limitations on the per queue statistics count (inherited
> > > > >   from ixgbe) and avoid recurrent bugs on stats array overflow  
> >
> > First comment, I think it would be easier to read if renaming the legacy
> > basic stats interface was in a separate patch.
> >  
> 
> It will be quite artificial, but I can do this yes.
> 
> 
> > > > The patch is adding two new dev_ops 'rxq_stats_get' & 'txq_stats_get',  
> > my  
> > > > concern is if it is overkill to have three dev_ops to get stats
> > > > and I am feeling that is making xstat code more complex.  
> > >
> > > Having these new (meant to be) internal dev_ops has the avantage of
> > > separating the statistics reported from the drivers from the exported  
> > api.  
> > > This is also why I did not prefix the structure names with rte_.  
> >
> > Yes, and to make it clear, please do not talk about API,
> > as it is only a driver interface.
> >  
> 
> Ok, so I will describe this as a "driver interface" update.
> 
> 
> 
> > > The "complex" part is in a single place, ethdev and this is when
> > > translating from an internal representation to the exposed bits in the
> > > public apis.
> > >
> > > Would it be simpler to add 'q_ierrors' & 'q_oerrors' to 'struct  
> > > > rte_eth_stats'?
> > > >  
> > >
> > > It does not solve the problem of drivers that are buggy because of the
> > > limit on RTE_ETHDEV_QUEUE_STAT_CNTRS.
> > > All drivers need to be aware of this limitation of the rte_eth_stats
> > > structure.  
> >
> > Yes, this limitation should be dropped.
> > I would like to see the functions rte_eth_dev_set_?x_queue_stats_mapping()
> > deprecated as they were a bad abstraction of ixgbe limitation.
> >  
> 
> That's a different topic from my pov, but yes, this mapping stuff should go
> away, later.
> 
> 
> > > And perhaps we can do the 'fix rxq q_errors' patchset [1] after this  
> > > > change, so
> > > > fix can be done with less changes, although it will push the fix into  
> > next  
> > > > release because of the ABI break.  
> > >
> > > I am fine with merging this together, we don't want to backport this
> > > anyway, right?  
> >
> > No, it would make some behaviours changing in stable releases,
> > so better to not backport it and keep the buggy behaviour in old branches.
> >  
> 
> Since the time I had posted this RFC, I have worked on a RFC v2, I will
> post this next week, with the drivers I found time to convert.
> We will have to take a decision on what goes to -rc2 between this and the
> q_errors[] patchset.
> 
> 

It looks like this all about maintaining source compatiablity with older
or out of tree drivers. This is not something DPDK has to worry about.
Why not just do a mondo patch that fixes all the drivers to use the new stats API.
You do need to keep the same ethdev interface for applications, but driver
API can change.

Re: [dpdk-dev] [RFC PATCH 1/2] ethdev: introduce internal rxq/txq stats API

[Stephen Hemminger]

On Fri, 12 Apr 2019 16:32:01 +0200
David Marchand <david.marchand@redhat.com> wrote:

> On Fri, Apr 12, 2019 at 3:29 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> 
> > 26/03/2019 10:29, David Marchand:  
> > > On Tue, Mar 19, 2019 at 6:18 PM Ferruh Yigit <ferruh.yigit@intel.com>  
> > wrote:  
> > >  
> > > > On 3/14/2019 3:13 PM, David Marchand wrote:  
> > > > > Introduce a new api to retrieve per queue statistics from the  
> > drivers.  
> > > > > The api objectives:
> > > > > - easily add some common per queue statistics and have it exposed
> > > > >   through the user xstats api while the user stats api is left  
> > untouched  
> > > > > - remove the limitations on the per queue statistics count (inherited
> > > > >   from ixgbe) and avoid recurrent bugs on stats array overflow  
> >
> > First comment, I think it would be easier to read if renaming the legacy
> > basic stats interface was in a separate patch.
> >  
> 
> It will be quite artificial, but I can do this yes.
> 
> 
> > > > The patch is adding two new dev_ops 'rxq_stats_get' & 'txq_stats_get',  
> > my  
> > > > concern is if it is overkill to have three dev_ops to get stats
> > > > and I am feeling that is making xstat code more complex.  
> > >
> > > Having these new (meant to be) internal dev_ops has the avantage of
> > > separating the statistics reported from the drivers from the exported  
> > api.  
> > > This is also why I did not prefix the structure names with rte_.  
> >
> > Yes, and to make it clear, please do not talk about API,
> > as it is only a driver interface.
> >  
> 
> Ok, so I will describe this as a "driver interface" update.
> 
> 
> 
> > > The "complex" part is in a single place, ethdev and this is when
> > > translating from an internal representation to the exposed bits in the
> > > public apis.
> > >
> > > Would it be simpler to add 'q_ierrors' & 'q_oerrors' to 'struct  
> > > > rte_eth_stats'?
> > > >  
> > >
> > > It does not solve the problem of drivers that are buggy because of the
> > > limit on RTE_ETHDEV_QUEUE_STAT_CNTRS.
> > > All drivers need to be aware of this limitation of the rte_eth_stats
> > > structure.  
> >
> > Yes, this limitation should be dropped.
> > I would like to see the functions rte_eth_dev_set_?x_queue_stats_mapping()
> > deprecated as they were a bad abstraction of ixgbe limitation.
> >  
> 
> That's a different topic from my pov, but yes, this mapping stuff should go
> away, later.
> 
> 
> > > And perhaps we can do the 'fix rxq q_errors' patchset [1] after this  
> > > > change, so
> > > > fix can be done with less changes, although it will push the fix into  
> > next  
> > > > release because of the ABI break.  
> > >
> > > I am fine with merging this together, we don't want to backport this
> > > anyway, right?  
> >
> > No, it would make some behaviours changing in stable releases,
> > so better to not backport it and keep the buggy behaviour in old branches.
> >  
> 
> Since the time I had posted this RFC, I have worked on a RFC v2, I will
> post this next week, with the drivers I found time to convert.
> We will have to take a decision on what goes to -rc2 between this and the
> q_errors[] patchset.
> 
> 

It looks like this all about maintaining source compatiablity with older
or out of tree drivers. This is not something DPDK has to worry about.
Why not just do a mondo patch that fixes all the drivers to use the new stats API.
You do need to keep the same ethdev interface for applications, but driver
API can change.

Re: [dpdk-dev] [PATCH 00/12] rxq q_errors[] statistics fixes

[Thomas Monjalon]

11/03/2019 18:22, Ferruh Yigit:
> On 3/4/2019 11:18 AM, David Marchand wrote:
> > According to the api, the q_errors[] per queue statistic is for reception
> > errors not transmit errors.
> > This is a first cleanup on statistics before looking at oerrors.
> > 
> 
> Yes, the patchset looks aligned with the API documentation [1].
> 
> What can be the solution after cleanup? We can merge this cleanup and solution
> next to each-other to not leave a gap?

I think we should merge those fixes in 19.05-rc2.

It seems there is a lot more work to achieve on stats, so better
to start without waiting for the full picture.

Re: [dpdk-dev] [PATCH 00/12] rxq q_errors[] statistics fixes

[Thomas Monjalon]

11/03/2019 18:22, Ferruh Yigit:
> On 3/4/2019 11:18 AM, David Marchand wrote:
> > According to the api, the q_errors[] per queue statistic is for reception
> > errors not transmit errors.
> > This is a first cleanup on statistics before looking at oerrors.
> > 
> 
> Yes, the patchset looks aligned with the API documentation [1].
> 
> What can be the solution after cleanup? We can merge this cleanup and solution
> next to each-other to not leave a gap?

I think we should merge those fixes in 19.05-rc2.

It seems there is a lot more work to achieve on stats, so better
to start without waiting for the full picture.

Re: [dpdk-dev] [PATCH 00/12] rxq q_errors[] statistics fixes

[Ferruh Yigit]

On 4/12/2019 4:07 PM, Thomas Monjalon wrote:
> 11/03/2019 18:22, Ferruh Yigit:
>> On 3/4/2019 11:18 AM, David Marchand wrote:
>>> According to the api, the q_errors[] per queue statistic is for reception
>>> errors not transmit errors.
>>> This is a first cleanup on statistics before looking at oerrors.
>>>
>>
>> Yes, the patchset looks aligned with the API documentation [1].
>>
>> What can be the solution after cleanup? We can merge this cleanup and solution
>> next to each-other to not leave a gap?
> 
> I think we should merge those fixes in 19.05-rc2.
> 
> It seems there is a lot more work to achieve on stats, so better
> to start without waiting for the full picture.
> 

The problem is "q_errors" is available only for Rx queues, and David's patch is
preventing drivers to put Tx error stats into "q_errors" field.

But it is clear that there is a need for a field for Tx queues errors. David has
another patch to using xstats for this. But I believe xstats is making solution
confusing, and now approach is unbalanced for Rx and Tx queues.

I am for adding a new field for Tx queues "q_errors", and this will make getting
stats and David's patch very simple.

The problem with the new fields is it breaks the ABI, but we already increased
the ABIVER for ethdev this release, I believe this is very good timing for this fix.

Re: [dpdk-dev] [PATCH 00/12] rxq q_errors[] statistics fixes

[Ferruh Yigit]

On 4/12/2019 4:07 PM, Thomas Monjalon wrote:
> 11/03/2019 18:22, Ferruh Yigit:
>> On 3/4/2019 11:18 AM, David Marchand wrote:
>>> According to the api, the q_errors[] per queue statistic is for reception
>>> errors not transmit errors.
>>> This is a first cleanup on statistics before looking at oerrors.
>>>
>>
>> Yes, the patchset looks aligned with the API documentation [1].
>>
>> What can be the solution after cleanup? We can merge this cleanup and solution
>> next to each-other to not leave a gap?
> 
> I think we should merge those fixes in 19.05-rc2.
> 
> It seems there is a lot more work to achieve on stats, so better
> to start without waiting for the full picture.
> 

The problem is "q_errors" is available only for Rx queues, and David's patch is
preventing drivers to put Tx error stats into "q_errors" field.

But it is clear that there is a need for a field for Tx queues errors. David has
another patch to using xstats for this. But I believe xstats is making solution
confusing, and now approach is unbalanced for Rx and Tx queues.

I am for adding a new field for Tx queues "q_errors", and this will make getting
stats and David's patch very simple.

The problem with the new fields is it breaks the ABI, but we already increased
the ABIVER for ethdev this release, I believe this is very good timing for this fix.

Re: [dpdk-dev] [PATCH 00/12] rxq q_errors[] statistics fixes

[Thomas Monjalon]

12/04/2019 17:38, Ferruh Yigit:
> On 4/12/2019 4:07 PM, Thomas Monjalon wrote:
> > 11/03/2019 18:22, Ferruh Yigit:
> >> On 3/4/2019 11:18 AM, David Marchand wrote:
> >>> According to the api, the q_errors[] per queue statistic is for reception
> >>> errors not transmit errors.
> >>> This is a first cleanup on statistics before looking at oerrors.
> >>>
> >>
> >> Yes, the patchset looks aligned with the API documentation [1].
> >>
> >> What can be the solution after cleanup? We can merge this cleanup and solution
> >> next to each-other to not leave a gap?
> > 
> > I think we should merge those fixes in 19.05-rc2.
> > 
> > It seems there is a lot more work to achieve on stats, so better
> > to start without waiting for the full picture.
> > 
> 
> The problem is "q_errors" is available only for Rx queues, and David's patch is
> preventing drivers to put Tx error stats into "q_errors" field.
> 
> But it is clear that there is a need for a field for Tx queues errors. David has
> another patch to using xstats for this. But I believe xstats is making solution
> confusing, and now approach is unbalanced for Rx and Tx queues.
> 
> I am for adding a new field for Tx queues "q_errors", and this will make getting
> stats and David's patch very simple.
> 
> The problem with the new fields is it breaks the ABI, but we already increased
> the ABIVER for ethdev this release, I believe this is very good timing for this fix.

If changing the stats API, we should increase the number of stats per queue:
	#define RTE_ETHDEV_QUEUE_STAT_CNTRS 16
What about 128 queues per port? 256?

Re: [dpdk-dev] [PATCH 00/12] rxq q_errors[] statistics fixes

[Thomas Monjalon]

12/04/2019 17:38, Ferruh Yigit:
> On 4/12/2019 4:07 PM, Thomas Monjalon wrote:
> > 11/03/2019 18:22, Ferruh Yigit:
> >> On 3/4/2019 11:18 AM, David Marchand wrote:
> >>> According to the api, the q_errors[] per queue statistic is for reception
> >>> errors not transmit errors.
> >>> This is a first cleanup on statistics before looking at oerrors.
> >>>
> >>
> >> Yes, the patchset looks aligned with the API documentation [1].
> >>
> >> What can be the solution after cleanup? We can merge this cleanup and solution
> >> next to each-other to not leave a gap?
> > 
> > I think we should merge those fixes in 19.05-rc2.
> > 
> > It seems there is a lot more work to achieve on stats, so better
> > to start without waiting for the full picture.
> > 
> 
> The problem is "q_errors" is available only for Rx queues, and David's patch is
> preventing drivers to put Tx error stats into "q_errors" field.
> 
> But it is clear that there is a need for a field for Tx queues errors. David has
> another patch to using xstats for this. But I believe xstats is making solution
> confusing, and now approach is unbalanced for Rx and Tx queues.
> 
> I am for adding a new field for Tx queues "q_errors", and this will make getting
> stats and David's patch very simple.
> 
> The problem with the new fields is it breaks the ABI, but we already increased
> the ABIVER for ethdev this release, I believe this is very good timing for this fix.

If changing the stats API, we should increase the number of stats per queue:
	#define RTE_ETHDEV_QUEUE_STAT_CNTRS 16
What about 128 queues per port? 256?

Re: [dpdk-dev] [PATCH 00/12] rxq q_errors[] statistics fixes

[Ferruh Yigit]

On 4/12/2019 4:45 PM, Thomas Monjalon wrote:
> 12/04/2019 17:38, Ferruh Yigit:
>> On 4/12/2019 4:07 PM, Thomas Monjalon wrote:
>>> 11/03/2019 18:22, Ferruh Yigit:
>>>> On 3/4/2019 11:18 AM, David Marchand wrote:
>>>>> According to the api, the q_errors[] per queue statistic is for reception
>>>>> errors not transmit errors.
>>>>> This is a first cleanup on statistics before looking at oerrors.
>>>>>
>>>>
>>>> Yes, the patchset looks aligned with the API documentation [1].
>>>>
>>>> What can be the solution after cleanup? We can merge this cleanup and solution
>>>> next to each-other to not leave a gap?
>>>
>>> I think we should merge those fixes in 19.05-rc2.
>>>
>>> It seems there is a lot more work to achieve on stats, so better
>>> to start without waiting for the full picture.
>>>
>>
>> The problem is "q_errors" is available only for Rx queues, and David's patch is
>> preventing drivers to put Tx error stats into "q_errors" field.
>>
>> But it is clear that there is a need for a field for Tx queues errors. David has
>> another patch to using xstats for this. But I believe xstats is making solution
>> confusing, and now approach is unbalanced for Rx and Tx queues.
>>
>> I am for adding a new field for Tx queues "q_errors", and this will make getting
>> stats and David's patch very simple.
>>
>> The problem with the new fields is it breaks the ABI, but we already increased
>> the ABIVER for ethdev this release, I believe this is very good timing for this fix.
> 
> If changing the stats API, we should increase the number of stats per queue:
> 	#define RTE_ETHDEV_QUEUE_STAT_CNTRS 16
> What about 128 queues per port? 256?

We have 5 uint64_t using this [1], it will be 6 if we add new one.
So having 256 queues, will cost 12K memory, this is not a big number.

Is there any other concern of having large array other than possible memory waste?

[1]
uint64_t q_ipackets[RTE_ETHDEV_QUEUE_STAT_CNTRS];
uint64_t q_opackets[RTE_ETHDEV_QUEUE_STAT_CNTRS];
uint64_t q_ibytes[RTE_ETHDEV_QUEUE_STAT_CNTRS];
uint64_t q_obytes[RTE_ETHDEV_QUEUE_STAT_CNTRS];
uint64_t q_errors[RTE_ETHDEV_QUEUE_STAT_CNTRS];

Re: [dpdk-dev] [PATCH 00/12] rxq q_errors[] statistics fixes

[Ferruh Yigit]

On 4/12/2019 4:45 PM, Thomas Monjalon wrote:
> 12/04/2019 17:38, Ferruh Yigit:
>> On 4/12/2019 4:07 PM, Thomas Monjalon wrote:
>>> 11/03/2019 18:22, Ferruh Yigit:
>>>> On 3/4/2019 11:18 AM, David Marchand wrote:
>>>>> According to the api, the q_errors[] per queue statistic is for reception
>>>>> errors not transmit errors.
>>>>> This is a first cleanup on statistics before looking at oerrors.
>>>>>
>>>>
>>>> Yes, the patchset looks aligned with the API documentation [1].
>>>>
>>>> What can be the solution after cleanup? We can merge this cleanup and solution
>>>> next to each-other to not leave a gap?
>>>
>>> I think we should merge those fixes in 19.05-rc2.
>>>
>>> It seems there is a lot more work to achieve on stats, so better
>>> to start without waiting for the full picture.
>>>
>>
>> The problem is "q_errors" is available only for Rx queues, and David's patch is
>> preventing drivers to put Tx error stats into "q_errors" field.
>>
>> But it is clear that there is a need for a field for Tx queues errors. David has
>> another patch to using xstats for this. But I believe xstats is making solution
>> confusing, and now approach is unbalanced for Rx and Tx queues.
>>
>> I am for adding a new field for Tx queues "q_errors", and this will make getting
>> stats and David's patch very simple.
>>
>> The problem with the new fields is it breaks the ABI, but we already increased
>> the ABIVER for ethdev this release, I believe this is very good timing for this fix.
> 
> If changing the stats API, we should increase the number of stats per queue:
> 	#define RTE_ETHDEV_QUEUE_STAT_CNTRS 16
> What about 128 queues per port? 256?

We have 5 uint64_t using this [1], it will be 6 if we add new one.
So having 256 queues, will cost 12K memory, this is not a big number.

Is there any other concern of having large array other than possible memory waste?

[1]
uint64_t q_ipackets[RTE_ETHDEV_QUEUE_STAT_CNTRS];
uint64_t q_opackets[RTE_ETHDEV_QUEUE_STAT_CNTRS];
uint64_t q_ibytes[RTE_ETHDEV_QUEUE_STAT_CNTRS];
uint64_t q_obytes[RTE_ETHDEV_QUEUE_STAT_CNTRS];
uint64_t q_errors[RTE_ETHDEV_QUEUE_STAT_CNTRS];

Re: [dpdk-dev] [PATCH 00/12] rxq q_errors[] statistics fixes

[Yigit, Ferruh]

On 3/4/2019 11:18 AM, David Marchand wrote:
> According to the api, the q_errors[] per queue statistic is for reception
> errors not transmit errors.
> This is a first cleanup on statistics before looking at oerrors.
> 

The fix is correct according the documentation but it was waiting for a solution
to capture the Tx queue errors which we are removing this support from some PMDs
because they were storing this information into wrong field.

Since the affected stats are Tx queue error stats, and all Tx errors still can
be stored on 'oerrors' I am for getting the fix, although we need to figure out
how to store Tx queue error stats.

For series,
Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

Series applied to dpdk-next-net/master, thanks.