DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH 0/4] expose ixgbe extended stats to dpdk apps
@ 2015-06-05 17:35 Maryam Tahhan
  2015-06-05 17:35 ` [dpdk-dev] [PATCH 1/4] ixgbe: expose extended error statistics Maryam Tahhan
                   ` (3 more replies)
  0 siblings, 4 replies; 17+ messages in thread
From: Maryam Tahhan @ 2015-06-05 17:35 UTC (permalink / raw)
  To: dev

This patch implements xstats_get() and xstats_reset() in dev_ops for ixgbe to
expose detailed error statistics to DPDK applications.

The dump_cfg application is extended to demonstrate the usage of retrieving
statistics for DPDK interfaces and renamed to proc_info in order reflect this
new functionality.

The testpmd app was also extended to display additional statistics.

Maryam Tahhan (4):
  ixgbe: expose extended error statistics
  ethdev: expose extended error stats
  testpmd: extend testpmd to show all extended stats
  app: replace dump_cfg with proc_info

 app/Makefile                     |   2 +-
 app/dump_cfg/Makefile            |  45 ----
 app/dump_cfg/main.c              |  92 -------
 app/proc_info/Makefile           |  45 ++++
 app/proc_info/main.c             | 525 +++++++++++++++++++++++++++++++++++++++
 app/test-pmd/config.c            |   5 +
 drivers/net/ixgbe/ixgbe_ethdev.c | 160 ++++++++++--
 lib/librte_ether/rte_ethdev.c    |  12 +-
 lib/librte_ether/rte_ethdev.h    |   4 +
 mk/rte.sdktest.mk                |   4 +-
 10 files changed, 728 insertions(+), 166 deletions(-)
 delete mode 100644 app/dump_cfg/Makefile
 delete mode 100644 app/dump_cfg/main.c
 create mode 100644 app/proc_info/Makefile
 create mode 100644 app/proc_info/main.c

-- 
1.8.1.4

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

* [dpdk-dev] [PATCH 1/4] ixgbe: expose extended error statistics
  2015-06-05 17:35 [dpdk-dev] [PATCH 0/4] expose ixgbe extended stats to dpdk apps Maryam Tahhan
@ 2015-06-05 17:35 ` Maryam Tahhan
  2015-06-08 13:26   ` Tahhan, Maryam
  2015-06-10  0:51   ` Stephen Hemminger
  2015-06-05 17:35 ` [dpdk-dev] [PATCH 2/4] ethdev: expose extended error stats Maryam Tahhan
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 17+ messages in thread
From: Maryam Tahhan @ 2015-06-05 17:35 UTC (permalink / raw)
  To: dev

Implement xstats_get() and xstats_reset() in dev_ops for ixgbe to expose
detailed error statistics to DPDK applications.

Signed-off-by: Maryam Tahhan <maryam.tahhan@intel.com>
---
 drivers/net/ixgbe/ixgbe_ethdev.c | 160 +++++++++++++++++++++++++++++++++------
 1 file changed, 138 insertions(+), 22 deletions(-)

diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
index 0d9f9b2..f789aba 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -131,7 +131,10 @@ static int ixgbe_dev_link_update(struct rte_eth_dev *dev,
 				int wait_to_complete);
 static void ixgbe_dev_stats_get(struct rte_eth_dev *dev,
 				struct rte_eth_stats *stats);
+static int ixgbe_dev_xstats_get(struct rte_eth_dev *dev,
+				struct rte_eth_xstats *xstats, unsigned n);
 static void ixgbe_dev_stats_reset(struct rte_eth_dev *dev);
+static void ixgbe_dev_xstats_reset(struct rte_eth_dev *dev);
 static int ixgbe_dev_queue_stats_mapping_set(struct rte_eth_dev *eth_dev,
 					     uint16_t queue_id,
 					     uint8_t stat_idx,
@@ -330,7 +333,9 @@ static const struct eth_dev_ops ixgbe_eth_dev_ops = {
 	.allmulticast_disable = ixgbe_dev_allmulticast_disable,
 	.link_update          = ixgbe_dev_link_update,
 	.stats_get            = ixgbe_dev_stats_get,
+	.xstats_get           = ixgbe_dev_xstats_get,
 	.stats_reset          = ixgbe_dev_stats_reset,
+	.xstats_reset         = ixgbe_dev_xstats_reset,
 	.queue_stats_mapping_set = ixgbe_dev_queue_stats_mapping_set,
 	.dev_infos_get        = ixgbe_dev_info_get,
 	.mtu_set              = ixgbe_dev_mtu_set,
@@ -408,6 +413,34 @@ static const struct eth_dev_ops ixgbevf_eth_dev_ops = {
 	.mac_addr_remove      = ixgbevf_remove_mac_addr,
 };
 
+/* store statistics names and its offset in stats structure  */
+struct rte_ixgbe_xstats_name_off {
+	char name[RTE_ETH_XSTATS_NAME_SIZE];
+	unsigned offset;
+};
+
+static const struct rte_ixgbe_xstats_name_off rte_ixgbe_stats_strings[] = {
+	{"rx_illegal_byte_err", offsetof(struct ixgbe_hw_stats, errbc)},
+	{"rx_len_err", offsetof(struct ixgbe_hw_stats, rlec)},
+	{"rx_undersize_count", offsetof(struct ixgbe_hw_stats, ruc)},
+	{"rx_oversize_count", offsetof(struct ixgbe_hw_stats, roc)},
+	{"rx_fragment_count", offsetof(struct ixgbe_hw_stats, rfc)},
+	{"rx_jabber_count", offsetof(struct ixgbe_hw_stats, rjc)},
+	{"l3_l4_xsum_error", offsetof(struct ixgbe_hw_stats, xec)},
+	{"mac_local_fault", offsetof(struct ixgbe_hw_stats, mlfc)},
+	{"mac_remote_fault", offsetof(struct ixgbe_hw_stats, mrfc)},
+	{"mac_short_pkt_discard", offsetof(struct ixgbe_hw_stats, mspdc)},
+	{"fccrc_error", offsetof(struct ixgbe_hw_stats, fccrc)},
+	{"fcoe_drop", offsetof(struct ixgbe_hw_stats, fcoerpdc)},
+	{"fc_last_error", offsetof(struct ixgbe_hw_stats, fclast)},
+	{"rx_multicast_packets", offsetof(struct ixgbe_hw_stats, mprc)},
+	{"rx_broadcast_packets", offsetof(struct ixgbe_hw_stats, bprc)},
+	{"mgmt_pkts_dropped", offsetof(struct ixgbe_hw_stats, mngpdc)},
+};
+
+#define RTE_NB_XSTATS (sizeof(rte_ixgbe_stats_strings) /	\
+		sizeof(rte_ixgbe_stats_strings[0]))
+
 /**
  * Atomically reads the link status information from global
  * structure rte_eth_dev.
@@ -1739,26 +1772,18 @@ ixgbe_dev_close(struct rte_eth_dev *dev)
 	ixgbe_set_rar(hw, 0, hw->mac.addr, 0, IXGBE_RAH_AV);
 }
 
-/*
- * This function is based on ixgbe_update_stats_counters() in base/ixgbe.c
- */
 static void
-ixgbe_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
+ixgbe_read_stats_registers(struct ixgbe_hw *hw, struct ixgbe_hw_stats *hw_stats,
+			   uint64_t *total_missed_rx, uint64_t *total_qbrc,
+			   uint64_t *total_qprc, uint64_t *rxnfgpc,
+			   uint64_t *txdgpc)
 {
-	struct ixgbe_hw *hw =
-			IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
-	struct ixgbe_hw_stats *hw_stats =
-			IXGBE_DEV_PRIVATE_TO_STATS(dev->data->dev_private);
 	uint32_t bprc, lxon, lxoff, total;
-	uint64_t total_missed_rx, total_qbrc, total_qprc;
 	unsigned i;
 
-	total_missed_rx = 0;
-	total_qbrc = 0;
-	total_qprc = 0;
-
 	hw_stats->crcerrs += IXGBE_READ_REG(hw, IXGBE_CRCERRS);
 	hw_stats->illerrc += IXGBE_READ_REG(hw, IXGBE_ILLERRC);
+
 	hw_stats->errbc += IXGBE_READ_REG(hw, IXGBE_ERRBC);
 	hw_stats->mspdc += IXGBE_READ_REG(hw, IXGBE_MSPDC);
 
@@ -1768,7 +1793,7 @@ ixgbe_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
 		/* global total per queue */
 		hw_stats->mpc[i] += mp;
 		/* Running comprehensive total for stats display */
-		total_missed_rx += hw_stats->mpc[i];
+		*total_missed_rx += hw_stats->mpc[i];
 		if (hw->mac.type == ixgbe_mac_82598EB)
 			hw_stats->rnbc[i] +=
 			    IXGBE_READ_REG(hw, IXGBE_RNBC(i));
@@ -1794,15 +1819,18 @@ ixgbe_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
 		    ((uint64_t)IXGBE_READ_REG(hw, IXGBE_QBTC_H(i)) << 32);
 		hw_stats->qprdc[i] += IXGBE_READ_REG(hw, IXGBE_QPRDC(i));
 
-		total_qprc += hw_stats->qprc[i];
-		total_qbrc += hw_stats->qbrc[i];
+		*total_qprc += hw_stats->qprc[i];
+		*total_qbrc += hw_stats->qbrc[i];
 	}
+
 	hw_stats->mlfc += IXGBE_READ_REG(hw, IXGBE_MLFC);
 	hw_stats->mrfc += IXGBE_READ_REG(hw, IXGBE_MRFC);
 	hw_stats->rlec += IXGBE_READ_REG(hw, IXGBE_RLEC);
 
 	/* Note that gprc counts missed packets */
 	hw_stats->gprc += IXGBE_READ_REG(hw, IXGBE_GPRC);
+	*rxnfgpc += IXGBE_READ_REG(hw, IXGBE_RXNFGPC);
+	*txdgpc += IXGBE_READ_REG(hw, IXGBE_TXDGPC);
 
 	if (hw->mac.type != ixgbe_mac_82598EB) {
 		hw_stats->gorc += IXGBE_READ_REG(hw, IXGBE_GORCL);
@@ -1879,6 +1907,27 @@ ixgbe_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
 		hw_stats->fcoedwrc += IXGBE_READ_REG(hw, IXGBE_FCOEDWRC);
 		hw_stats->fcoedwtc += IXGBE_READ_REG(hw, IXGBE_FCOEDWTC);
 	}
+}
+
+/* This function is based on ixgbe_update_stats_counters() in ixgbe/ixgbe.c */
+static void
+ixgbe_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
+{
+	struct ixgbe_hw *hw =
+			IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+	struct ixgbe_hw_stats *hw_stats =
+			IXGBE_DEV_PRIVATE_TO_STATS(dev->data->dev_private);
+	uint64_t total_missed_rx, total_qbrc, total_qprc, rxnfgpc, txdgpc;
+	unsigned i;
+
+	total_missed_rx = 0;
+	total_qbrc = 0;
+	total_qprc = 0;
+	rxnfgpc = 0;
+	txdgpc = 0;
+
+	ixgbe_read_stats_registers(hw, hw_stats, &total_missed_rx, &total_qbrc,
+				   &total_qprc, &rxnfgpc, &txdgpc);
 
 	if (stats == NULL)
 		return;
@@ -1888,7 +1937,6 @@ ixgbe_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
 	stats->ibytes = total_qbrc;
 	stats->opackets = hw_stats->gptc;
 	stats->obytes = hw_stats->gotc;
-	stats->imcasts = hw_stats->mprc;
 
 	for (i = 0; i < IXGBE_QUEUE_STAT_COUNTERS; i++) {
 		stats->q_ipackets[i] = hw_stats->qprc[i];
@@ -1900,15 +1948,32 @@ ixgbe_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
 
 	/* Rx Errors */
 	stats->ibadcrc  = hw_stats->crcerrs;
-	stats->ibadlen  = hw_stats->rlec + hw_stats->ruc + hw_stats->roc;
+	stats->ibadlen  = hw_stats->rlec +
+		hw_stats->ruc +
+		hw_stats->roc;
 	stats->imissed  = total_missed_rx;
-	stats->ierrors  = stats->ibadcrc +
-	                  stats->ibadlen +
-	                  stats->imissed +
-	                  hw_stats->illerrc + hw_stats->errbc;
+	stats->imacerr  = stats->ibadlen +
+		hw_stats->xec +
+		hw_stats->crcerrs +
+		hw_stats->illerrc +
+		hw_stats->errbc +
+		hw_stats->mlfc +
+		hw_stats->mrfc +
+		hw_stats->rfc +
+		hw_stats->rjc +
+		hw_stats->fccrc +
+		hw_stats->fclast;
+	stats->iphyerr  = rxnfgpc - hw_stats->gprc;
+	stats->ierrors  = stats->imacerr +
+		stats->iphyerr +
+		stats->imissed;
+	stats->idrop = hw_stats->mngpdc +
+		hw_stats->fcoerpdc +
+		total_qbrc;
 
 	/* Tx Errors */
 	stats->oerrors  = 0;
+	stats->odrop = txdgpc - hw_stats->gptc;
 
 	/* XON/XOFF pause frames */
 	stats->tx_pause_xon  = hw_stats->lxontxc;
@@ -1936,6 +2001,57 @@ ixgbe_dev_stats_reset(struct rte_eth_dev *dev)
 	memset(stats, 0, sizeof(*stats));
 }
 
+static int
+ixgbe_dev_xstats_get(struct rte_eth_dev *dev, struct rte_eth_xstats *xstats,
+		     unsigned n)
+{
+	struct ixgbe_hw *hw =
+			IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+	struct ixgbe_hw_stats *hw_stats =
+			IXGBE_DEV_PRIVATE_TO_STATS(dev->data->dev_private);
+	uint64_t total_missed_rx, total_qbrc, total_qprc, rxnfgpc, txdgpc;
+	unsigned i, count = RTE_NB_XSTATS;
+
+	if (n < count)
+		return count;
+
+	total_missed_rx = 0;
+	total_qbrc = 0;
+	total_qprc = 0;
+	rxnfgpc = 0;
+	txdgpc = 0;
+	count = 0;
+
+	ixgbe_read_stats_registers(hw, hw_stats, &total_missed_rx, &total_qbrc,
+				   &total_qprc, &rxnfgpc, &txdgpc);
+
+	if (!xstats)
+		return 0;
+
+	/* Error stats */
+	for (i = 0; i < RTE_NB_XSTATS; i++) {
+		snprintf(xstats[count].name, sizeof(xstats[count].name),
+			 "%s", rte_ixgbe_stats_strings[i].name);
+		xstats[count++].value = *(uint64_t *)(((char *)hw_stats) +
+					rte_ixgbe_stats_strings[i].offset);
+	}
+
+	return count;
+}
+
+static void
+ixgbe_dev_xstats_reset(struct rte_eth_dev *dev)
+{
+	struct ixgbe_hw_stats *stats =
+			IXGBE_DEV_PRIVATE_TO_STATS(dev->data->dev_private);
+
+	/* HW registers are cleared on read */
+	ixgbe_dev_xstats_get(dev, NULL, RTE_NB_XSTATS);
+
+	/* Reset software totals */
+	memset(stats, 0, sizeof(*stats));
+}
+
 static void
 ixgbevf_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
 {
-- 
1.8.1.4

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

* [dpdk-dev] [PATCH 2/4] ethdev: expose extended error stats
  2015-06-05 17:35 [dpdk-dev] [PATCH 0/4] expose ixgbe extended stats to dpdk apps Maryam Tahhan
  2015-06-05 17:35 ` [dpdk-dev] [PATCH 1/4] ixgbe: expose extended error statistics Maryam Tahhan
@ 2015-06-05 17:35 ` Maryam Tahhan
  2015-06-17 13:58   ` Thomas Monjalon
  2015-06-05 17:35 ` [dpdk-dev] [PATCH 3/4] testpmd: extend testpmd to show all extended stats Maryam Tahhan
  2015-06-05 17:35 ` [dpdk-dev] [PATCH 4/4] app: replace dump_cfg with proc_info Maryam Tahhan
  3 siblings, 1 reply; 17+ messages in thread
From: Maryam Tahhan @ 2015-06-05 17:35 UTC (permalink / raw)
  To: dev

Extend rte_eth_xstats_get to retrieve additional stats from the device
driver as well the top level extended stats. Add additional drop
counters to the extended stats.

Signed-off-by: Maryam Tahhan <maryam.tahhan@intel.com>
---
 lib/librte_ether/rte_ethdev.c | 12 ++++++++----
 lib/librte_ether/rte_ethdev.h |  4 ++++
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index 5a94654..8c22cda 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -129,6 +129,8 @@ static const struct rte_eth_xstats_name_off rte_stats_strings[] = {
 	{"rx_crc_errors", offsetof(struct rte_eth_stats, ibadcrc)},
 	{"rx_bad_length_errors", offsetof(struct rte_eth_stats, ibadlen)},
 	{"rx_errors", offsetof(struct rte_eth_stats, ierrors)},
+	{"rx_mac_err", offsetof(struct rte_eth_stats, imacerr)},
+	{"rx_phy_err", offsetof(struct rte_eth_stats, iphyerr)},
 	{"alloc_rx_buff_failed", offsetof(struct rte_eth_stats, rx_nombuf)},
 	{"fdir_match", offsetof(struct rte_eth_stats, fdirmatch)},
 	{"fdir_miss", offsetof(struct rte_eth_stats, fdirmiss)},
@@ -136,6 +138,8 @@ static const struct rte_eth_xstats_name_off rte_stats_strings[] = {
 	{"rx_flow_control_xon", offsetof(struct rte_eth_stats, rx_pause_xon)},
 	{"tx_flow_control_xoff", offsetof(struct rte_eth_stats, tx_pause_xoff)},
 	{"rx_flow_control_xoff", offsetof(struct rte_eth_stats, rx_pause_xoff)},
+	{"tx_drops", offsetof(struct rte_eth_stats, odrop)},
+	{"rx_drops", offsetof(struct rte_eth_stats, idrop)},
 };
 #define RTE_NB_STATS (sizeof(rte_stats_strings) / sizeof(rte_stats_strings[0]))
 
@@ -154,7 +158,6 @@ static const struct rte_eth_xstats_name_off rte_txq_stats_strings[] = {
 #define RTE_NB_TXQ_STATS (sizeof(rte_txq_stats_strings) /	\
 		sizeof(rte_txq_stats_strings[0]))
 
-
 /**
  * The user application callback description.
  *
@@ -1741,7 +1744,7 @@ rte_eth_xstats_get(uint8_t port_id, struct rte_eth_xstats *xstats,
 {
 	struct rte_eth_stats eth_stats;
 	struct rte_eth_dev *dev;
-	unsigned count, i, q;
+	unsigned count = 0, xcount = 0, i, q;
 	uint64_t val;
 	char *stats_ptr;
 
@@ -1754,18 +1757,19 @@ rte_eth_xstats_get(uint8_t port_id, struct rte_eth_xstats *xstats,
 
 	/* implemented by the driver */
 	if (dev->dev_ops->xstats_get != NULL)
-		return (*dev->dev_ops->xstats_get)(dev, xstats, n);
+		xcount = (*dev->dev_ops->xstats_get)(dev, xstats, n);
 
 	/* else, return generic statistics */
 	count = RTE_NB_STATS;
 	count += dev->data->nb_rx_queues * RTE_NB_RXQ_STATS;
 	count += dev->data->nb_tx_queues * RTE_NB_TXQ_STATS;
+	count += xcount;
 	if (n < count)
 		return count;
 
 	/* now fill the xstats structure */
 
-	count = 0;
+	count = xcount;
 	rte_eth_stats_get(port_id, &eth_stats);
 
 	/* global stats */
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index 16dbe00..5bc3b81 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -224,6 +224,10 @@ struct rte_eth_stats {
 	/**< Total number of good bytes received from loopback,VF Only */
 	uint64_t olbbytes;
 	/**< Total number of good bytes transmitted to loopback,VF Only */
+	uint64_t imacerr;   /**< Total of RX packets with MAC Errors. */
+	uint64_t iphyerr;   /**< Total of RX packets with PHY Errors. */
+	uint64_t idrop;  /**< Total number of dropped received packets. */
+	uint64_t odrop;  /**< Total number of dropped transmitted packets. */
 };
 
 /**
-- 
1.8.1.4

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

* [dpdk-dev] [PATCH 3/4] testpmd: extend testpmd to show all extended stats
  2015-06-05 17:35 [dpdk-dev] [PATCH 0/4] expose ixgbe extended stats to dpdk apps Maryam Tahhan
  2015-06-05 17:35 ` [dpdk-dev] [PATCH 1/4] ixgbe: expose extended error statistics Maryam Tahhan
  2015-06-05 17:35 ` [dpdk-dev] [PATCH 2/4] ethdev: expose extended error stats Maryam Tahhan
@ 2015-06-05 17:35 ` Maryam Tahhan
  2015-06-05 17:35 ` [dpdk-dev] [PATCH 4/4] app: replace dump_cfg with proc_info Maryam Tahhan
  3 siblings, 0 replies; 17+ messages in thread
From: Maryam Tahhan @ 2015-06-05 17:35 UTC (permalink / raw)
  To: dev

Extend testpmd to show additional aggregate extended stats.

Signed-off-by: Maryam Tahhan <maryam.tahhan@intel.com>
---
 app/test-pmd/config.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index f788ed5..b42d83f 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -153,6 +153,11 @@ nic_stats_display(portid_t port_id)
 		       stats.opackets, stats.oerrors, stats.obytes);
 	}
 
+	printf("  RX-MAC-errors: %-10"PRIu64" RX-PHY-errors: %-10"PRIu64"\n",
+	       stats.imacerr, stats.iphyerr);
+	printf("  RX-nombuf:  %-10"PRIu64"  RX-dropped: %-10"PRIu64"\n",
+	       stats.rx_nombuf, stats.idrop);
+
 	/* stats fdir */
 	if (fdir_conf.mode != RTE_FDIR_MODE_NONE)
 		printf("  Fdirmiss:   %-10"PRIu64" Fdirmatch: %-10"PRIu64"\n",
-- 
1.8.1.4

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

* [dpdk-dev] [PATCH 4/4] app: replace dump_cfg with proc_info
  2015-06-05 17:35 [dpdk-dev] [PATCH 0/4] expose ixgbe extended stats to dpdk apps Maryam Tahhan
                   ` (2 preceding siblings ...)
  2015-06-05 17:35 ` [dpdk-dev] [PATCH 3/4] testpmd: extend testpmd to show all extended stats Maryam Tahhan
@ 2015-06-05 17:35 ` Maryam Tahhan
  2015-06-05 21:08   ` Thomas Monjalon
  3 siblings, 1 reply; 17+ messages in thread
From: Maryam Tahhan @ 2015-06-05 17:35 UTC (permalink / raw)
  To: dev

Extend dump_cfg to also display statistcs information for given DPDK
ports and rename the application to proc_info as it's now a utility
doing a little more than just dumping the memory information for DPDK.

Signed-off-by: Maryam Tahhan <maryam.tahhan@intel.com>
---
 app/Makefile           |   2 +-
 app/dump_cfg/Makefile  |  45 -----
 app/dump_cfg/main.c    |  92 ---------
 app/proc_info/Makefile |  45 +++++
 app/proc_info/main.c   | 525 +++++++++++++++++++++++++++++++++++++++++++++++++
 mk/rte.sdktest.mk      |   4 +-
 6 files changed, 573 insertions(+), 140 deletions(-)
 delete mode 100644 app/dump_cfg/Makefile
 delete mode 100644 app/dump_cfg/main.c
 create mode 100644 app/proc_info/Makefile
 create mode 100644 app/proc_info/main.c

diff --git a/app/Makefile b/app/Makefile
index 50c670b..88c0bad 100644
--- a/app/Makefile
+++ b/app/Makefile
@@ -36,6 +36,6 @@ DIRS-$(CONFIG_RTE_LIBRTE_ACL) += test-acl
 DIRS-$(CONFIG_RTE_LIBRTE_PIPELINE) += test-pipeline
 DIRS-$(CONFIG_RTE_TEST_PMD) += test-pmd
 DIRS-$(CONFIG_RTE_LIBRTE_CMDLINE) += cmdline_test
-DIRS-$(CONFIG_RTE_LIBRTE_EAL_LINUXAPP) += dump_cfg
+DIRS-$(CONFIG_RTE_LIBRTE_EAL_LINUXAPP) += proc_info
 
 include $(RTE_SDK)/mk/rte.subdir.mk
diff --git a/app/dump_cfg/Makefile b/app/dump_cfg/Makefile
deleted file mode 100644
index 3257127..0000000
--- a/app/dump_cfg/Makefile
+++ /dev/null
@@ -1,45 +0,0 @@
-#   BSD LICENSE
-#
-#   Copyright(c) 2010-2014 Intel Corporation. All rights reserved.
-#   All rights reserved.
-#
-#   Redistribution and use in source and binary forms, with or without
-#   modification, are permitted provided that the following conditions
-#   are met:
-#
-#     * Redistributions of source code must retain the above copyright
-#       notice, this list of conditions and the following disclaimer.
-#     * Redistributions in binary form must reproduce the above copyright
-#       notice, this list of conditions and the following disclaimer in
-#       the documentation and/or other materials provided with the
-#       distribution.
-#     * Neither the name of Intel Corporation nor the names of its
-#       contributors may be used to endorse or promote products derived
-#       from this software without specific prior written permission.
-#
-#   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
-#   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
-#   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
-#   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
-#   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
-#   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
-#   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
-#   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
-#   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
-#   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
-#   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
-
-include $(RTE_SDK)/mk/rte.vars.mk
-
-APP = dump_cfg
-
-CFLAGS += $(WERROR_FLAGS)
-
-# all source are stored in SRCS-y
-
-SRCS-y := main.c
-
-# this application needs libraries first
-DEPDIRS-y += lib
-
-include $(RTE_SDK)/mk/rte.app.mk
diff --git a/app/dump_cfg/main.c b/app/dump_cfg/main.c
deleted file mode 100644
index 127dbb1..0000000
--- a/app/dump_cfg/main.c
+++ /dev/null
@@ -1,92 +0,0 @@
-/*-
- *   BSD LICENSE
- *
- *   Copyright(c) 2010-2014 Intel Corporation. All rights reserved.
- *   All rights reserved.
- *
- *   Redistribution and use in source and binary forms, with or without
- *   modification, are permitted provided that the following conditions
- *   are met:
- *
- *     * Redistributions of source code must retain the above copyright
- *       notice, this list of conditions and the following disclaimer.
- *     * Redistributions in binary form must reproduce the above copyright
- *       notice, this list of conditions and the following disclaimer in
- *       the documentation and/or other materials provided with the
- *       distribution.
- *     * Neither the name of Intel Corporation nor the names of its
- *       contributors may be used to endorse or promote products derived
- *       from this software without specific prior written permission.
- *
- *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
- *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
- *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
- *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
- *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
- *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
- *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
- *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
- *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
- *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
- *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
- */
-
-#include <stdio.h>
-#include <string.h>
-#include <stdint.h>
-#include <errno.h>
-#include <stdarg.h>
-#include <inttypes.h>
-#include <sys/queue.h>
-#include <stdlib.h>
-
-#include <rte_memory.h>
-#include <rte_memzone.h>
-#include <rte_launch.h>
-#include <rte_tailq.h>
-#include <rte_eal.h>
-#include <rte_per_lcore.h>
-#include <rte_lcore.h>
-#include <rte_debug.h>
-#include <rte_log.h>
-#include <rte_atomic.h>
-#include <rte_branch_prediction.h>
-#include <rte_string_fns.h>
-
-int
-main(int argc, char **argv)
-{
-	int ret;
-	int i;
-	char c_flag[] = "-c1";
-	char n_flag[] = "-n4";
-	char mp_flag[] = "--proc-type=secondary";
-	char *argp[argc + 3];
-	argp[0] = argv[0];
-	argp[1] = c_flag;
-	argp[2] = n_flag;
-	argp[3] = mp_flag;
-
-	for(i = 1; i < argc; i++) {
-		argp[i + 3] = argv[i];
-	}
-	argc += 3;
-
-	ret = rte_eal_init(argc, argp);
-	if (ret < 0)
-		rte_panic("Cannot init EAL\n");
-
-	printf("----------- MEMORY_SEGMENTS -----------\n");
-	rte_dump_physmem_layout(stdout);
-	printf("--------- END_MEMORY_SEGMENTS ---------\n");
-
-	printf("------------ MEMORY_ZONES -------------\n");
-	rte_memzone_dump(stdout);
-	printf("---------- END_MEMORY_ZONES -----------\n");
-
-	printf("------------- TAIL_QUEUES -------------\n");
-	rte_dump_tailq(stdout);
-	printf("---------- END_TAIL_QUEUES ------------\n");
-
-	return 0;
-}
diff --git a/app/proc_info/Makefile b/app/proc_info/Makefile
new file mode 100644
index 0000000..6759547
--- /dev/null
+++ b/app/proc_info/Makefile
@@ -0,0 +1,45 @@
+#   BSD LICENSE
+#
+#   Copyright(c) 2010-2014 Intel Corporation. All rights reserved.
+#   All rights reserved.
+#
+#   Redistribution and use in source and binary forms, with or without
+#   modification, are permitted provided that the following conditions
+#   are met:
+#
+#     * Redistributions of source code must retain the above copyright
+#       notice, this list of conditions and the following disclaimer.
+#     * Redistributions in binary form must reproduce the above copyright
+#       notice, this list of conditions and the following disclaimer in
+#       the documentation and/or other materials provided with the
+#       distribution.
+#     * Neither the name of Intel Corporation nor the names of its
+#       contributors may be used to endorse or promote products derived
+#       from this software without specific prior written permission.
+#
+#   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+#   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+#   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+#   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+#   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+#   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+#   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+#   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+#   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+#   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+#   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+include $(RTE_SDK)/mk/rte.vars.mk
+
+APP = proc_info
+
+CFLAGS += $(WERROR_FLAGS)
+
+# all source are stored in SRCS-y
+
+SRCS-y := main.c
+
+# this application needs libraries first
+DEPDIRS-y += lib
+
+include $(RTE_SDK)/mk/rte.app.mk
diff --git a/app/proc_info/main.c b/app/proc_info/main.c
new file mode 100644
index 0000000..d1a5fd3
--- /dev/null
+++ b/app/proc_info/main.c
@@ -0,0 +1,525 @@
+/*-
+ *   BSD LICENSE
+ *
+ *   Copyright(c) 2010-2014 Intel Corporation. All rights reserved.
+ *   All rights reserved.
+ *
+ *   Redistribution and use in source and binary forms, with or without
+ *   modification, are permitted provided that the following conditions
+ *   are met:
+ *
+ *     * Redistributions of source code must retain the above copyright
+ *       notice, this list of conditions and the following disclaimer.
+ *     * Redistributions in binary form must reproduce the above copyright
+ *       notice, this list of conditions and the following disclaimer in
+ *       the documentation and/or other materials provided with the
+ *       distribution.
+ *     * Neither the name of Intel Corporation nor the names of its
+ *       contributors may be used to endorse or promote products derived
+ *       from this software without specific prior written permission.
+ *
+ *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#include <stdio.h>
+#include <string.h>
+#include <stdint.h>
+#include <errno.h>
+#include <stdarg.h>
+#include <inttypes.h>
+#include <sys/queue.h>
+#include <stdlib.h>
+#include <getopt.h>
+
+#include <rte_eal.h>
+#include <rte_config.h>
+#include <rte_common.h>
+#include <rte_debug.h>
+#include <rte_ethdev.h>
+#include <rte_malloc.h>
+#include <rte_memory.h>
+#include <rte_memzone.h>
+#include <rte_launch.h>
+#include <rte_tailq.h>
+#include <rte_per_lcore.h>
+#include <rte_lcore.h>
+#include <rte_debug.h>
+#include <rte_log.h>
+#include <rte_atomic.h>
+#include <rte_branch_prediction.h>
+#include <rte_string_fns.h>
+
+/* Maximum long option length for option parsing. */
+#define MAX_LONG_OPT_SZ 64
+#define RTE_LOGTYPE_APP RTE_LOGTYPE_USER1
+
+/* MAX_PORT of 32 @ 32 tx_queues/port */
+#define MAX_TX_QUEUE_STATS_MAPPINGS 1024
+/* MAX_PORT of 32 @ 128 rx_queues/port */
+#define MAX_RX_QUEUE_STATS_MAPPINGS 4096
+
+/**< mask of enabled ports */
+static uint32_t enabled_port_mask;
+/**< Enable stats. */
+static uint32_t enable_stats;
+/**< Enable xstats. */
+static uint32_t enable_xstats;
+/**< Enable stats reset. */
+static uint32_t reset_stats;
+/**< Enable xstats reset. */
+static uint32_t reset_xstats;
+/**< Enable memory info. */
+static uint32_t mem_info;
+/**
+ * The data structure associated with each port.
+ */
+struct rte_port {
+	uint8_t tx_queue_stats_mapping_enabled;
+	uint8_t rx_queue_stats_mapping_enabled;
+};
+
+struct rte_port *ports;	       /**< For all probed ethernet ports. */
+
+struct queue_stats_mappings {
+	uint8_t port_id;
+	uint16_t queue_id;
+	uint8_t stats_counter_id;
+} __rte_cache_aligned;
+
+static struct queue_stats_mappings
+		tx_queue_stats_mappings_array[MAX_TX_QUEUE_STATS_MAPPINGS];
+static struct queue_stats_mappings
+		rx_queue_stats_mappings_array[MAX_RX_QUEUE_STATS_MAPPINGS];
+
+static struct queue_stats_mappings *tx_queue_stats_mappings =
+		tx_queue_stats_mappings_array;
+static struct queue_stats_mappings *rx_queue_stats_mappings =
+		rx_queue_stats_mappings_array;
+
+static uint16_t nb_tx_queue_stats_mappings;
+static uint16_t nb_rx_queue_stats_mappings;
+
+/*
+ * Configurable number of RX/TX queues.
+ */
+static uint16_t nb_rxq = 1; /**< Number of RX queues per port. */
+static uint16_t nb_txq = 1; /**< Number of TX queues per port. */
+
+/**< display usage */
+static void
+proc_info_usage(const char *prgname)
+{
+	printf("%s [EAL options] -- -p PORTMASK\n"
+		"  -m to display DPDK memory zones, segments and TAILQ information\n"
+		"  -p PORTMASK: hexadecimal bitmask of ports to retrieve stats for\n"
+		"  --stats: to display port statistics, enabled by default\n"
+		"  --xstats: to display extended port statistics, disabled by "
+			"default\n"
+		"  --stats-reset: to reset port statistics\n"
+		"  --xstats-reset: to reset port extended statistics\n",
+		prgname);
+}
+
+/*
+ * Parse the portmask provided at run time.
+ */
+static int
+parse_portmask(const char *portmask)
+{
+	char *end = NULL;
+	unsigned long pm;
+
+	errno = 0;
+
+	/* parse hexadecimal string */
+	pm = strtoul(portmask, &end, 16);
+	if ((portmask[0] == '\0') || (end == NULL) || (*end != '\0') ||
+		(errno != 0)) {
+		printf("%s ERROR parsing the port mask\n", __func__);
+		return -1;
+	}
+
+	if (pm == 0)
+		return -1;
+
+	return pm;
+
+}
+
+/* Parse the argument given in the command line of the application */
+static int
+proc_info_parse_args(int argc, char **argv)
+{
+	int opt;
+	int option_index;
+	char *prgname = argv[0];
+	static struct option long_option[] = {
+		{"stats", 0, NULL, 0},
+		{"stats-reset", 0, NULL, 0},
+		{"xstats", 0, NULL, 0},
+		{"xstats-reset", 0, NULL, 0},
+		{NULL, 0, 0, 0}
+	};
+
+	/* Parse command line */
+	while ((opt = getopt_long(argc, argv, "p:m",
+			long_option, &option_index)) != EOF) {
+		switch (opt) {
+		/* portmask */
+		case 'p':
+			enabled_port_mask = parse_portmask(optarg);
+			if (enabled_port_mask == 0) {
+				printf("invalid portmask\n");
+				proc_info_usage(prgname);
+				return -1;
+			}
+			break;
+		case 'm':
+			mem_info = 1;
+			break;
+		case 0:
+			/* Print stats */
+			if (!strncmp(long_option[option_index].name,
+				     "stats",
+				     MAX_LONG_OPT_SZ))
+				enable_stats = 1;
+			/* Print xstats */
+			else if (!strncmp(long_option[option_index].name,
+					  "xstats",
+					  MAX_LONG_OPT_SZ))
+				enable_xstats = 1;
+			/* Print stats */
+			if (!strncmp(long_option[option_index].name,
+				     "stats-reset",
+				     MAX_LONG_OPT_SZ))
+				reset_stats = 1;
+			/* Print xstats */
+			else if (!strncmp(long_option[option_index].name,
+					  "xstats-reset",
+					  MAX_LONG_OPT_SZ))
+				reset_xstats = 1;
+			break;
+
+		default:
+			proc_info_usage(prgname);
+			return -1;
+		}
+	}
+	return 0;
+}
+
+static void
+meminfo_display(void)
+{
+	printf("----------- MEMORY_SEGMENTS -----------\n");
+	rte_dump_physmem_layout(stdout);
+	printf("--------- END_MEMORY_SEGMENTS ---------\n");
+
+	printf("------------ MEMORY_ZONES -------------\n");
+	rte_memzone_dump(stdout);
+	printf("---------- END_MEMORY_ZONES -----------\n");
+
+	printf("------------- TAIL_QUEUES -------------\n");
+	rte_dump_tailq(stdout);
+	printf("---------- END_TAIL_QUEUES ------------\n");
+}
+
+static void
+nic_stats_display(uint8_t port_id)
+{
+	struct rte_eth_stats stats;
+	uint8_t i;
+	struct rte_port *port = &ports[port_id];
+
+	static const char *nic_stats_border = "########################";
+
+	rte_eth_stats_get(port_id, &stats);
+	printf("\n  %s NIC statistics for port %-2d %s\n",
+		   nic_stats_border, port_id, nic_stats_border);
+
+	if ((!port->rx_queue_stats_mapping_enabled) &&
+	    (!port->tx_queue_stats_mapping_enabled)) {
+		printf("  RX-packets: %-10"PRIu64" RX-missed: %-10"PRIu64
+		       " RX-bytes:  %-"PRIu64"\n",
+		       stats.ipackets, stats.imissed, stats.ibytes);
+		printf("  RX-badcrc:  %-10"PRIu64" RX-badlen: %-10"PRIu64
+		       " RX-errors: %-"PRIu64"\n",
+		       stats.ibadcrc, stats.ibadlen, stats.ierrors);
+		printf("  RX-nombuf:  %-10"PRIu64"\n", stats.rx_nombuf);
+		printf("  TX-packets: %-10"PRIu64" TX-errors: %-10"PRIu64
+		       " TX-bytes:  %-"PRIu64"\n",
+		       stats.opackets, stats.oerrors, stats.obytes);
+	} else {
+		printf("  RX-packets:              %10"PRIu64
+		       "    RX-errors: %10"PRIu64
+		       "    RX-bytes: %10"PRIu64"\n",
+		       stats.ipackets, stats.ierrors, stats.ibytes);
+		printf("  RX-badcrc:               %10"PRIu64
+		       "    RX-badlen: %10"PRIu64
+		       "  RX-errors:  %10"PRIu64"\n",
+		       stats.ibadcrc, stats.ibadlen, stats.ierrors);
+		printf("  RX-nombuf:               %10"PRIu64"\n",
+		       stats.rx_nombuf);
+		printf("  TX-packets:              %10"PRIu64
+		       "    TX-errors: %10"PRIu64
+		       "    TX-bytes: %10"PRIu64"\n",
+		       stats.opackets, stats.oerrors, stats.obytes);
+	}
+
+	printf("  RX-MAC-errors: %-10"PRIu64" RX-PHY-errors: %-10"PRIu64"\n",
+	       stats.imacerr, stats.iphyerr);
+	printf("  RX-nombuf:  %-10"PRIu64"  RX-dropped: %-10"PRIu64"\n",
+	       stats.rx_nombuf, stats.idrop);
+
+	if (port->rx_queue_stats_mapping_enabled) {
+		printf("\n");
+		for (i = 0; i < RTE_ETHDEV_QUEUE_STAT_CNTRS; i++) {
+			printf("  Stats reg %2d RX-packets: %10"PRIu64
+			       "    RX-errors: %10"PRIu64
+			       "    RX-bytes: %10"PRIu64"\n",
+			       i, stats.q_ipackets[i], stats.q_errors[i],
+			       stats.q_ibytes[i]);
+		}
+	}
+	if (port->tx_queue_stats_mapping_enabled) {
+		printf("\n");
+		for (i = 0; i < RTE_ETHDEV_QUEUE_STAT_CNTRS; i++) {
+			printf("  Stats reg %2d TX-packets: %10"PRIu64
+			       "                             TX-bytes: %10"
+			       PRIu64"\n",
+			       i, stats.q_opackets[i], stats.q_obytes[i]);
+		}
+	}
+
+	/* Display statistics of XON/XOFF pause frames, if any. */
+	if ((stats.tx_pause_xon  | stats.rx_pause_xon |
+		 stats.tx_pause_xoff | stats.rx_pause_xoff) > 0) {
+		printf("  RX-XOFF:    %-10"PRIu64" RX-XON:    %-10"PRIu64"\n",
+		       stats.rx_pause_xoff, stats.rx_pause_xon);
+		printf("  TX-XOFF:    %-10"PRIu64" TX-XON:    %-10"PRIu64"\n",
+		       stats.tx_pause_xoff, stats.tx_pause_xon);
+	}
+	printf("  %s############################%s\n",
+		   nic_stats_border, nic_stats_border);
+}
+
+static void
+nic_stats_clear(uint8_t port_id)
+{
+	printf("\n Clearing NIC stats for port %d\n", port_id);
+	rte_eth_stats_reset(port_id);
+	printf("\n  NIC statistics for port %d cleared\n", port_id);
+}
+
+static void
+nic_xstats_display(uint8_t port_id)
+{
+	struct rte_eth_xstats *xstats;
+	int len, ret, i;
+	static const char *nic_stats_border = "########################";
+
+	len = rte_eth_xstats_get(port_id, NULL, 0);
+	if (len < 0) {
+		printf("Cannot get xstats count\n");
+		return;
+	}
+	xstats = malloc(sizeof(xstats[0]) * len);
+	if (xstats == NULL) {
+		printf("Cannot allocate memory for xstats\n");
+		return;
+	}
+
+	printf("###### NIC extended statistics for port %-2d #########\n",
+			   port_id);
+	printf("%s############################\n",
+			   nic_stats_border);
+	ret = rte_eth_xstats_get(port_id, xstats, len);
+	if (ret < 0 || ret > len) {
+		printf("Cannot get xstats\n");
+		free(xstats);
+		return;
+	}
+
+	for (i = 0; i < len; i++)
+		printf("%s: %"PRIu64"\n", xstats[i].name, xstats[i].value);
+
+	printf("%s############################\n",
+			   nic_stats_border);
+	free(xstats);
+}
+
+static void
+nic_xstats_clear(uint8_t port_id)
+{
+	printf("\n Clearing NIC xstats for port %d\n", port_id);
+	rte_eth_xstats_reset(port_id);
+	printf("\n  NIC extended statistics for port %d cleared\n", port_id);
+}
+
+static int
+set_tx_queue_stats_mapping_registers(uint8_t port_id, struct rte_port *port)
+{
+	uint16_t i;
+	int diag;
+	uint8_t mapping_found = 0;
+
+	for (i = 0; i < nb_tx_queue_stats_mappings; i++) {
+		if ((tx_queue_stats_mappings[i].port_id == port_id) &&
+		    (tx_queue_stats_mappings[i].queue_id < nb_txq)) {
+			diag = rte_eth_dev_set_tx_queue_stats_mapping(port_id,
+				tx_queue_stats_mappings[i].queue_id,
+				tx_queue_stats_mappings[i].stats_counter_id);
+			if (diag != 0)
+				return diag;
+			mapping_found = 1;
+		}
+	}
+
+	if (mapping_found)
+		port->tx_queue_stats_mapping_enabled = 1;
+
+	return 0;
+}
+
+static int
+set_rx_queue_stats_mapping_registers(uint8_t port_id, struct rte_port *port)
+{
+	uint16_t i;
+	int diag;
+	uint8_t mapping_found = 0;
+
+	for (i = 0; i < nb_rx_queue_stats_mappings; i++) {
+		if ((rx_queue_stats_mappings[i].port_id == port_id) &&
+		    (rx_queue_stats_mappings[i].queue_id < nb_rxq)) {
+			diag = rte_eth_dev_set_rx_queue_stats_mapping(port_id,
+				rx_queue_stats_mappings[i].queue_id,
+				rx_queue_stats_mappings[i].stats_counter_id);
+			if (diag != 0)
+				return diag;
+			mapping_found = 1;
+		}
+	}
+
+	if (mapping_found)
+		port->rx_queue_stats_mapping_enabled = 1;
+
+	return 0;
+}
+
+static void
+map_port_queue_stats_mapping_registers(uint8_t pi, struct rte_port *port)
+{
+	int diag = 0;
+
+	diag = set_tx_queue_stats_mapping_registers(pi, port);
+	if (diag != 0) {
+		if (diag == -ENOTSUP) {
+			port->tx_queue_stats_mapping_enabled = 0;
+			printf("TX queue stats mapping not supported "
+			       "port id=%d\n", pi);
+		} else
+			rte_exit(EXIT_FAILURE,
+				 "set_tx_queue_stats_mapping_registers "
+				 "failed for port id=%d diag=%d\n",
+				 pi, diag);
+	}
+
+	diag = set_rx_queue_stats_mapping_registers(pi, port);
+	if (diag != 0) {
+		if (diag == -ENOTSUP) {
+			port->rx_queue_stats_mapping_enabled = 0;
+			printf("RX queue stats mapping not supported "
+			       "port id=%d\n", pi);
+		} else
+			rte_exit(EXIT_FAILURE,
+				 "set_rx_queue_stats_mapping_registers "
+				 "failed for port id=%d diag=%d\n",
+				 pi, diag);
+	}
+}
+
+int
+main(int argc, char **argv)
+{
+	int ret;
+	int i;
+	char c_flag[] = "-c1";
+	char n_flag[] = "-n4";
+	char mp_flag[] = "--proc-type=secondary";
+	char *argp[argc + 3];
+	uint8_t nb_ports;
+
+	argp[0] = argv[0];
+	argp[1] = c_flag;
+	argp[2] = n_flag;
+	argp[3] = mp_flag;
+
+	for (i = 1; i < argc; i++)
+		argp[i + 3] = argv[i];
+
+	argc += 3;
+
+	ret = rte_eal_init(argc, argp);
+	if (ret < 0)
+		rte_panic("Cannot init EAL\n");
+
+	argc -= ret;
+	argv += (ret - 3);
+
+	/* parse app arguments */
+	ret = proc_info_parse_args(argc, argv);
+	if (ret < 0)
+		rte_exit(EXIT_FAILURE, "Invalid argument\n");
+
+	if (mem_info) {
+		meminfo_display();
+		return 0;
+	}
+
+	nb_ports = rte_eth_dev_count();
+	if (nb_ports == 0)
+		rte_exit(EXIT_FAILURE, "No Ethernet ports - bye\n");
+
+
+	if (nb_ports > RTE_MAX_ETHPORTS)
+		nb_ports = RTE_MAX_ETHPORTS;
+
+	/* Configuration of Ethernet ports. */
+	ports = rte_zmalloc("proc_info: ports",
+				sizeof(struct rte_port) * nb_ports,
+				RTE_CACHE_LINE_SIZE);
+	if (ports == NULL)
+		rte_exit(EXIT_FAILURE,
+				"rte_zmalloc(%d struct rte_port) failed\n",
+				nb_ports);
+
+	/* If no port mask was specified*/
+	if (enabled_port_mask == 0 && (enable_xstats || enable_stats))
+		enabled_port_mask = 0xffff;
+
+	for (i = 0; i < nb_ports; i++) {
+		map_port_queue_stats_mapping_registers(i, &ports[i]);
+		if (enabled_port_mask & (1 << i)) {
+			if (enable_stats)
+				nic_stats_display(i);
+			else if (enable_xstats)
+				nic_xstats_display(i);
+			else if (reset_stats)
+				nic_stats_clear(i);
+			else if (reset_xstats)
+				nic_xstats_clear(i);
+		}
+	}
+
+	return 0;
+}
diff --git a/mk/rte.sdktest.mk b/mk/rte.sdktest.mk
index 2696142..59a29de 100644
--- a/mk/rte.sdktest.mk
+++ b/mk/rte.sdktest.mk
@@ -66,7 +66,7 @@ test fast_test ring_test mempool_test perf_test:
 	fi
 
 # this is a special target to ease the pain of running coverage tests
-# this runs all the autotests, cmdline_test script and dump_cfg
+# this runs all the autotests, cmdline_test script and proc_info
 coverage:
 	@mkdir -p $(AUTOTEST_DIR) ; \
 	cd $(AUTOTEST_DIR) ; \
@@ -78,7 +78,7 @@ coverage:
 			$(RTE_OUTPUT)/app/test \
 			$(RTE_TARGET) \
 			$(BLACKLIST) $(WHITELIST) ; \
-		$(RTE_OUTPUT)/app/dump_cfg --file-prefix=ring_perf ; \
+		$(RTE_OUTPUT)/app/proc_info --file-prefix=ring_perf -- -m; \
 	else \
 		echo "No test found, please do a 'make build' first, or specify O=" ;\
 	fi
-- 
1.8.1.4

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

* Re: [dpdk-dev] [PATCH 4/4] app: replace dump_cfg with proc_info
  2015-06-05 17:35 ` [dpdk-dev] [PATCH 4/4] app: replace dump_cfg with proc_info Maryam Tahhan
@ 2015-06-05 21:08   ` Thomas Monjalon
  2015-06-08 13:45     ` Tahhan, Maryam
  0 siblings, 1 reply; 17+ messages in thread
From: Thomas Monjalon @ 2015-06-05 21:08 UTC (permalink / raw)
  To: Maryam Tahhan; +Cc: dev

2015-06-05 18:35, Maryam Tahhan:
> Extend dump_cfg to also display statistcs information for given DPDK
> ports and rename the application to proc_info as it's now a utility
> doing a little more than just dumping the memory information for DPDK.
> 
> Signed-off-by: Maryam Tahhan <maryam.tahhan@intel.com>
> ---
>  app/Makefile           |   2 +-
>  app/dump_cfg/Makefile  |  45 -----
>  app/dump_cfg/main.c    |  92 ---------
>  app/proc_info/Makefile |  45 +++++
>  app/proc_info/main.c   | 525 +++++++++++++++++++++++++++++++++++++++++++++++++
>  mk/rte.sdktest.mk      |   4 +-

It looks promising, thanks.
Would you consider adding yourself as a maintainer of this app?

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

* Re: [dpdk-dev] [PATCH 1/4] ixgbe: expose extended error statistics
  2015-06-05 17:35 ` [dpdk-dev] [PATCH 1/4] ixgbe: expose extended error statistics Maryam Tahhan
@ 2015-06-08 13:26   ` Tahhan, Maryam
  2015-06-10  0:51   ` Stephen Hemminger
  1 sibling, 0 replies; 17+ messages in thread
From: Tahhan, Maryam @ 2015-06-08 13:26 UTC (permalink / raw)
  To: dev


<snip>
> +	stats->idrop = hw_stats->mngpdc +
> +		hw_stats->fcoerpdc +
> +		total_qbrc;


Should use qprdc instead of total_qbrc

<snip>

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

* Re: [dpdk-dev] [PATCH 4/4] app: replace dump_cfg with proc_info
  2015-06-05 21:08   ` Thomas Monjalon
@ 2015-06-08 13:45     ` Tahhan, Maryam
  2015-06-08 14:22       ` Thomas Monjalon
  0 siblings, 1 reply; 17+ messages in thread
From: Tahhan, Maryam @ 2015-06-08 13:45 UTC (permalink / raw)
  To: Thomas Monjalon; +Cc: dev

> > Extend dump_cfg to also display statistcs information for given DPDK
> > ports and rename the application to proc_info as it's now a utility
> > doing a little more than just dumping the memory information for DPDK.
> >
> > Signed-off-by: Maryam Tahhan <maryam.tahhan@intel.com>
> > ---
> >  app/Makefile           |   2 +-
> >  app/dump_cfg/Makefile  |  45 -----
> >  app/dump_cfg/main.c    |  92 ---------
> >  app/proc_info/Makefile |  45 +++++
> >  app/proc_info/main.c   | 525
> +++++++++++++++++++++++++++++++++++++++++++++++++
> >  mk/rte.sdktest.mk      |   4 +-
> 
> It looks promising, thanks.
> Would you consider adding yourself as a maintainer of this app?

Yep, I can do that. Should I add myself to the maintainers file in a separate patch, or as a reworked version of this patch?

BR
Maryam

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

* Re: [dpdk-dev] [PATCH 4/4] app: replace dump_cfg with proc_info
  2015-06-08 13:45     ` Tahhan, Maryam
@ 2015-06-08 14:22       ` Thomas Monjalon
  0 siblings, 0 replies; 17+ messages in thread
From: Thomas Monjalon @ 2015-06-08 14:22 UTC (permalink / raw)
  To: Tahhan, Maryam; +Cc: dev

2015-06-08 13:45, Tahhan, Maryam:
> > > Extend dump_cfg to also display statistcs information for given DPDK
> > > ports and rename the application to proc_info as it's now a utility
> > > doing a little more than just dumping the memory information for DPDK.
> > >
> > > Signed-off-by: Maryam Tahhan <maryam.tahhan@intel.com>
> > > ---
> > >  app/Makefile           |   2 +-
> > >  app/dump_cfg/Makefile  |  45 -----
> > >  app/dump_cfg/main.c    |  92 ---------
> > >  app/proc_info/Makefile |  45 +++++
> > >  app/proc_info/main.c   | 525
> > +++++++++++++++++++++++++++++++++++++++++++++++++
> > >  mk/rte.sdktest.mk      |   4 +-
> > 
> > It looks promising, thanks.
> > Would you consider adding yourself as a maintainer of this app?
> 
> Yep, I can do that. Should I add myself to the maintainers file in a separate patch, or as a reworked version of this patch?

In case there is no other comment, a separate patch is OK.
Thanks

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

* Re: [dpdk-dev] [PATCH 1/4] ixgbe: expose extended error statistics
  2015-06-05 17:35 ` [dpdk-dev] [PATCH 1/4] ixgbe: expose extended error statistics Maryam Tahhan
  2015-06-08 13:26   ` Tahhan, Maryam
@ 2015-06-10  0:51   ` Stephen Hemminger
  2015-06-10 14:24     ` Tahhan, Maryam
  2015-06-17 12:52     ` Thomas Monjalon
  1 sibling, 2 replies; 17+ messages in thread
From: Stephen Hemminger @ 2015-06-10  0:51 UTC (permalink / raw)
  To: Maryam Tahhan; +Cc: dev

On Fri,  5 Jun 2015 18:35:02 +0100
Maryam Tahhan <maryam.tahhan@intel.com> wrote:

> Implement xstats_get() and xstats_reset() in dev_ops for ixgbe to expose
> detailed error statistics to DPDK applications.
> 
> Signed-off-by: Maryam Tahhan <maryam.tahhan@intel.com>

Also, the bug where CRC is included in Tx byte count but
not in the Rx byte count has not been addressed.

You seem to have ignored my earlier patch.

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

* Re: [dpdk-dev] [PATCH 1/4] ixgbe: expose extended error statistics
  2015-06-10  0:51   ` Stephen Hemminger
@ 2015-06-10 14:24     ` Tahhan, Maryam
  2015-06-17 12:52     ` Thomas Monjalon
  1 sibling, 0 replies; 17+ messages in thread
From: Tahhan, Maryam @ 2015-06-10 14:24 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: dev

> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Wednesday, June 10, 2015 1:51 AM
> To: Tahhan, Maryam
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 1/4] ixgbe: expose extended error statistics
> 
> On Fri,  5 Jun 2015 18:35:02 +0100
> Maryam Tahhan <maryam.tahhan@intel.com> wrote:
> 
> > Implement xstats_get() and xstats_reset() in dev_ops for ixgbe to
> > expose detailed error statistics to DPDK applications.
> >
> > Signed-off-by: Maryam Tahhan <maryam.tahhan@intel.com>
> 
> Also, the bug where CRC is included in Tx byte count but not in the Rx byte
> count has not been addressed.
> 
> You seem to have ignored my earlier patch.

I wasn't aware of your patch. But I will have a look.

Best Regards
Maryam

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

* Re: [dpdk-dev] [PATCH 1/4] ixgbe: expose extended error statistics
  2015-06-10  0:51   ` Stephen Hemminger
  2015-06-10 14:24     ` Tahhan, Maryam
@ 2015-06-17 12:52     ` Thomas Monjalon
  1 sibling, 0 replies; 17+ messages in thread
From: Thomas Monjalon @ 2015-06-17 12:52 UTC (permalink / raw)
  To: Stephen Hemminger, Maryam Tahhan; +Cc: dev

2015-06-09 17:51, Stephen Hemminger:
> On Fri,  5 Jun 2015 18:35:02 +0100
> Maryam Tahhan <maryam.tahhan@intel.com> wrote:
> 
> > Implement xstats_get() and xstats_reset() in dev_ops for ixgbe to expose
> > detailed error statistics to DPDK applications.
> > 
> > Signed-off-by: Maryam Tahhan <maryam.tahhan@intel.com>
> 
> Also, the bug where CRC is included in Tx byte count but
> not in the Rx byte count has not been addressed.
> 
> You seem to have ignored my earlier patch.

Stephen, providing a link would be helpful.
Maryam, please check:
	http://dpdk.org/ml/archives/dev/2015-April/016096.html
	http://dpdk.org/dev/patchwork/patch/2495/

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

* Re: [dpdk-dev] [PATCH 2/4] ethdev: expose extended error stats
  2015-06-05 17:35 ` [dpdk-dev] [PATCH 2/4] ethdev: expose extended error stats Maryam Tahhan
@ 2015-06-17 13:58   ` Thomas Monjalon
  2015-06-17 14:47     ` Kyle Larose
  2015-06-22 14:12     ` Tahhan, Maryam
  0 siblings, 2 replies; 17+ messages in thread
From: Thomas Monjalon @ 2015-06-17 13:58 UTC (permalink / raw)
  To: Maryam Tahhan; +Cc: dev

2015-06-05 18:35, Maryam Tahhan:
> Extend rte_eth_xstats_get to retrieve additional stats from the device
> driver as well the top level extended stats. Add additional drop
> counters to the extended stats.
> 
> Signed-off-by: Maryam Tahhan <maryam.tahhan@intel.com>
[..]
Patch 1/4 doesn't compile without patch 2/4.

> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -129,6 +129,8 @@ static const struct rte_eth_xstats_name_off rte_stats_strings[] = {
>  	{"rx_crc_errors", offsetof(struct rte_eth_stats, ibadcrc)},
>  	{"rx_bad_length_errors", offsetof(struct rte_eth_stats, ibadlen)},
>  	{"rx_errors", offsetof(struct rte_eth_stats, ierrors)},
> +	{"rx_mac_err", offsetof(struct rte_eth_stats, imacerr)},
> +	{"rx_phy_err", offsetof(struct rte_eth_stats, iphyerr)},
>  	{"alloc_rx_buff_failed", offsetof(struct rte_eth_stats, rx_nombuf)},
>  	{"fdir_match", offsetof(struct rte_eth_stats, fdirmatch)},
>  	{"fdir_miss", offsetof(struct rte_eth_stats, fdirmiss)},
> @@ -136,6 +138,8 @@ static const struct rte_eth_xstats_name_off rte_stats_strings[] = {
>  	{"rx_flow_control_xon", offsetof(struct rte_eth_stats, rx_pause_xon)},
>  	{"tx_flow_control_xoff", offsetof(struct rte_eth_stats, tx_pause_xoff)},
>  	{"rx_flow_control_xoff", offsetof(struct rte_eth_stats, rx_pause_xoff)},
> +	{"tx_drops", offsetof(struct rte_eth_stats, odrop)},
> +	{"rx_drops", offsetof(struct rte_eth_stats, idrop)},
>  };
[...]
> --- a/lib/librte_ether/rte_ethdev.h
> +++ b/lib/librte_ether/rte_ethdev.h
> @@ -224,6 +224,10 @@ struct rte_eth_stats {
> 
>         /**< Total number of good bytes received from loopback,VF Only */
>         uint64_t olbbytes;
>         /**< Total number of good bytes transmitted to loopback,VF Only */
> 
> +       uint64_t imacerr;   /**< Total of RX packets with MAC Errors. */
> +       uint64_t iphyerr;   /**< Total of RX packets with PHY Errors. */
> +       uint64_t idrop;  /**< Total number of dropped received packets. */
> +       uint64_t odrop;  /**< Total number of dropped transmitted packets. */
>  };

You are extending the generic stats. This is not the idea behind xstats.
The xstats are specific to the driver.
Furthermore we should migrate some "not really generic stats" to xstats
in order to keep only the really basic and common stats in rte_eth_stats.
By the way, in order to avoid duplicated code when getting generic stats
through xstats API, we need to change the implementation of
rte_eth_xstats_get() to add generic stats automatically, even if the
driver provide some xstats.

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

* Re: [dpdk-dev] [PATCH 2/4] ethdev: expose extended error stats
  2015-06-17 13:58   ` Thomas Monjalon
@ 2015-06-17 14:47     ` Kyle Larose
  2015-06-22 14:12     ` Tahhan, Maryam
  1 sibling, 0 replies; 17+ messages in thread
From: Kyle Larose @ 2015-06-17 14:47 UTC (permalink / raw)
  To: Thomas Monjalon; +Cc: dev

On Wed, Jun 17, 2015 at 9:58 AM, Thomas Monjalon
<thomas.monjalon@6wind.com> wrote:
[...]
>
> You are extending the generic stats. This is not the idea behind xstats.
> The xstats are specific to the driver.
> Furthermore we should migrate some "not really generic stats" to xstats
> in order to keep only the really basic and common stats in rte_eth_stats.
> By the way, in order to avoid duplicated code when getting generic stats
> through xstats API, we need to change the implementation of
> rte_eth_xstats_get() to add generic stats automatically, even if the
> driver provide some xstats.

This may be the wrong thread for this, and perhaps this question has
been answered (I couldn't find anything relating to it), but I have a
related question regarding exposing stats that are not captured in the
current device independent api (rte_eth_stats_get).

Is there a recommended strategy for displaying MIB interface stats for
applications using DPDK? For example, consider the IF-MIB
(http://www.ietf.org/rfc/rfc2233.txt). It provides three "normal" in
packet counters:

   ifHCInUcastPkts
   ifHCInMulticastPkts
   ifHCInBroadcastPkts

Looking at rte_eth_stats, the structure returned by rte_eth_stats_get,
we can only retrieve the total number of in packets, and the total
number of in multicast packets. We can't retrieve the number of in
broadcast packets. This means that we neither display ifHCInUcastPkts
(which needs us to subtract mulicast and broadcast from the total),
nor ifHCInBroadcastPkts.

While I understand that some devices may not support classifying
packets into such categories, I would think that most NICs would try
to allow applications to conform the fairly established standards such
as the IF-MIB. One would think that maybe xstats would allow this:
these nics have low level stats that provide this information.
However, consumers of the API would need to understand each NIC's
implementation of the xstats API to use it, making it somewhat
cumbersome to use. I'd think that it makes sense for the MIB
information to live in a device-independent, well-defined API.

Does it make sense to provide sufficient information in the
eth_stats_get function to provide support for the various interface
stats mibs? Alternatively, does it make sense to make a new function
or API tailored to providing this support? It could be device
independent (unlike xstats), while allowing each driver to choose
whether or not it supports it.

I'm using a fairly old version of DPDK (1.7.1), but from looking at
the master branch
(http://dpdk.org/browse/dpdk/tree/lib/librte_ether/rte_ethdev.h), it
looks like the structure hasn't really changed.

I've been struggling to figure out the right away to approach this, so
any thoughts/insights would be much appreciated.

Thanks,

Kyle

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

* Re: [dpdk-dev] [PATCH 2/4] ethdev: expose extended error stats
  2015-06-17 13:58   ` Thomas Monjalon
  2015-06-17 14:47     ` Kyle Larose
@ 2015-06-22 14:12     ` Tahhan, Maryam
  2015-06-22 15:12       ` Olivier MATZ
  1 sibling, 1 reply; 17+ messages in thread
From: Tahhan, Maryam @ 2015-06-22 14:12 UTC (permalink / raw)
  To: Thomas Monjalon; +Cc: dev


> 2015-06-05 18:35, Maryam Tahhan:
> > Extend rte_eth_xstats_get to retrieve additional stats from the device
> > driver as well the top level extended stats. Add additional drop
> > counters to the extended stats.
> >
> > Signed-off-by: Maryam Tahhan <maryam.tahhan@intel.com>
> [..]
> Patch 1/4 doesn't compile without patch 2/4.

The rebased patches should fix this issue.
> 
> > --- a/lib/librte_ether/rte_ethdev.c
> > +++ b/lib/librte_ether/rte_ethdev.c
> > @@ -129,6 +129,8 @@ static const struct rte_eth_xstats_name_off
> rte_stats_strings[] = {
> >  	{"rx_crc_errors", offsetof(struct rte_eth_stats, ibadcrc)},
> >  	{"rx_bad_length_errors", offsetof(struct rte_eth_stats, ibadlen)},
> >  	{"rx_errors", offsetof(struct rte_eth_stats, ierrors)},
> > +	{"rx_mac_err", offsetof(struct rte_eth_stats, imacerr)},
> > +	{"rx_phy_err", offsetof(struct rte_eth_stats, iphyerr)},
> >  	{"alloc_rx_buff_failed", offsetof(struct rte_eth_stats, rx_nombuf)},
> >  	{"fdir_match", offsetof(struct rte_eth_stats, fdirmatch)},
> >  	{"fdir_miss", offsetof(struct rte_eth_stats, fdirmiss)}, @@ -136,6
> > +138,8 @@ static const struct rte_eth_xstats_name_off rte_stats_strings[]
> = {
> >  	{"rx_flow_control_xon", offsetof(struct rte_eth_stats,
> rx_pause_xon)},
> >  	{"tx_flow_control_xoff", offsetof(struct rte_eth_stats,
> tx_pause_xoff)},
> >  	{"rx_flow_control_xoff", offsetof(struct rte_eth_stats,
> > rx_pause_xoff)},
> > +	{"tx_drops", offsetof(struct rte_eth_stats, odrop)},
> > +	{"rx_drops", offsetof(struct rte_eth_stats, idrop)},
> >  };
> [...]
> > --- a/lib/librte_ether/rte_ethdev.h
> > +++ b/lib/librte_ether/rte_ethdev.h
> > @@ -224,6 +224,10 @@ struct rte_eth_stats {
> >
> >         /**< Total number of good bytes received from loopback,VF Only */
> >         uint64_t olbbytes;
> >         /**< Total number of good bytes transmitted to loopback,VF
> > Only */
> >
> > +       uint64_t imacerr;   /**< Total of RX packets with MAC Errors. */
> > +       uint64_t iphyerr;   /**< Total of RX packets with PHY Errors. */
> > +       uint64_t idrop;  /**< Total number of dropped received packets. */
> > +       uint64_t odrop;  /**< Total number of dropped transmitted
> > + packets. */
> >  };
> 
> You are extending the generic stats. This is not the idea behind xstats.
> The xstats are specific to the driver.

I'd followed the example of: http://patchwork.dpdk.org/dev/patchwork/patch/85/ 
to added generic extended stats (at least what I thought were generic). I think that
dropped packets should fall under struct rte_eth_stats, and should perhaps be left
there, as most NICs/drivers should be able to provide that number. Would this be an
agreeable solution?

I have no other way to expose the total MAC errors and the total PHY errors without 
Adding counters into struct ixgbe_hw_stats, but I wasn't sure if this was allowable, is it?

The only other option is to round up all the errors into ierrors, without having the
granularity of what errors fall under. Is the latter option to sum up the values under one
umbrella preferred?

> Furthermore we should migrate some "not really generic stats" to xstats in
> order to keep only the really basic and common stats in rte_eth_stats.
> By the way, in order to avoid duplicated code when getting generic stats
> through xstats API, we need to change the implementation of
> rte_eth_xstats_get() to add generic stats automatically, even if the driver
> provide some xstats.

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

* Re: [dpdk-dev] [PATCH 2/4] ethdev: expose extended error stats
  2015-06-22 14:12     ` Tahhan, Maryam
@ 2015-06-22 15:12       ` Olivier MATZ
  2015-06-22 15:35         ` Tahhan, Maryam
  0 siblings, 1 reply; 17+ messages in thread
From: Olivier MATZ @ 2015-06-22 15:12 UTC (permalink / raw)
  To: Tahhan, Maryam, Thomas Monjalon; +Cc: dev

Hello Maryam,

On 06/22/2015 04:12 PM, Tahhan, Maryam wrote:
> 
>> 2015-06-05 18:35, Maryam Tahhan:
>>> Extend rte_eth_xstats_get to retrieve additional stats from the device
>>> driver as well the top level extended stats. Add additional drop
>>> counters to the extended stats.
>>>
>>> Signed-off-by: Maryam Tahhan <maryam.tahhan@intel.com>
>> [..]
>> Patch 1/4 doesn't compile without patch 2/4.
> 
> The rebased patches should fix this issue.
>>
>>> --- a/lib/librte_ether/rte_ethdev.c
>>> +++ b/lib/librte_ether/rte_ethdev.c
>>> @@ -129,6 +129,8 @@ static const struct rte_eth_xstats_name_off
>> rte_stats_strings[] = {
>>>  	{"rx_crc_errors", offsetof(struct rte_eth_stats, ibadcrc)},
>>>  	{"rx_bad_length_errors", offsetof(struct rte_eth_stats, ibadlen)},
>>>  	{"rx_errors", offsetof(struct rte_eth_stats, ierrors)},
>>> +	{"rx_mac_err", offsetof(struct rte_eth_stats, imacerr)},
>>> +	{"rx_phy_err", offsetof(struct rte_eth_stats, iphyerr)},
>>>  	{"alloc_rx_buff_failed", offsetof(struct rte_eth_stats, rx_nombuf)},
>>>  	{"fdir_match", offsetof(struct rte_eth_stats, fdirmatch)},
>>>  	{"fdir_miss", offsetof(struct rte_eth_stats, fdirmiss)}, @@ -136,6
>>> +138,8 @@ static const struct rte_eth_xstats_name_off rte_stats_strings[]
>> = {
>>>  	{"rx_flow_control_xon", offsetof(struct rte_eth_stats,
>> rx_pause_xon)},
>>>  	{"tx_flow_control_xoff", offsetof(struct rte_eth_stats,
>> tx_pause_xoff)},
>>>  	{"rx_flow_control_xoff", offsetof(struct rte_eth_stats,
>>> rx_pause_xoff)},
>>> +	{"tx_drops", offsetof(struct rte_eth_stats, odrop)},
>>> +	{"rx_drops", offsetof(struct rte_eth_stats, idrop)},
>>>  };
>> [...]
>>> --- a/lib/librte_ether/rte_ethdev.h
>>> +++ b/lib/librte_ether/rte_ethdev.h
>>> @@ -224,6 +224,10 @@ struct rte_eth_stats {
>>>
>>>         /**< Total number of good bytes received from loopback,VF Only */
>>>         uint64_t olbbytes;
>>>         /**< Total number of good bytes transmitted to loopback,VF
>>> Only */
>>>
>>> +       uint64_t imacerr;   /**< Total of RX packets with MAC Errors. */
>>> +       uint64_t iphyerr;   /**< Total of RX packets with PHY Errors. */
>>> +       uint64_t idrop;  /**< Total number of dropped received packets. */
>>> +       uint64_t odrop;  /**< Total number of dropped transmitted
>>> + packets. */
>>>  };
>>
>> You are extending the generic stats. This is not the idea behind xstats.
>> The xstats are specific to the driver.
> 
> I'd followed the example of: http://patchwork.dpdk.org/dev/patchwork/patch/85/ 
> to added generic extended stats (at least what I thought were generic). I think that
> dropped packets should fall under struct rte_eth_stats, and should perhaps be left
> there, as most NICs/drivers should be able to provide that number. Would this be an
> agreeable solution?
> 
> I have no other way to expose the total MAC errors and the total PHY errors without 
> Adding counters into struct ixgbe_hw_stats, but I wasn't sure if this was allowable, is it?
> 
> The only other option is to round up all the errors into ierrors, without having the
> granularity of what errors fall under. Is the latter option to sum up the values under one
> umbrella preferred?

As Thomas explained, I think we should avoid adding fields in
struct rte_eth_stats. This structure already contains statistics
that are specific to some hardware drivers. We should try to go
in the opposite direction and remove all the fields that do not
apply to all nics (physical or virtual). For me, it would be only
something like (rx, tx, rx_bytes, tx_bytes, rx_errors, tx_errors).

The xstats framework allows you to add any driver or hardware
specific stats and I think it's the proper place to add them
for ixgbe.

By the way, something could be modified in xstats framework. Today,
the behavior is:
- if ethdev->dev_ops->xstats_get != NULL, call it
- else, dump the generic stats in xstats format

A better behavior could be:
- Always dump the generic stats in xstats format
- and if thdev->dev_ops->xstats_get != NULL, add driver-specific stats

This would avoid to duplicate code that dumps generic stats in
all drivers.

So, to summarize what I think should be done for stats/xstats:
1. modify xstats behavior as described above
2. add the xstats dev_ops in ixgbe, it will dump the new stats

Bonus:
3. identify all the specific stats that could be removed from
   rte_eth_stats and start to mark them as deprecated.


Regards,
Olivier


> 
>> Furthermore we should migrate some "not really generic stats" to xstats in
>> order to keep only the really basic and common stats in rte_eth_stats.
>> By the way, in order to avoid duplicated code when getting generic stats
>> through xstats API, we need to change the implementation of
>> rte_eth_xstats_get() to add generic stats automatically, even if the driver
>> provide some xstats.

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

* Re: [dpdk-dev] [PATCH 2/4] ethdev: expose extended error stats
  2015-06-22 15:12       ` Olivier MATZ
@ 2015-06-22 15:35         ` Tahhan, Maryam
  0 siblings, 0 replies; 17+ messages in thread
From: Tahhan, Maryam @ 2015-06-22 15:35 UTC (permalink / raw)
  To: Olivier MATZ, Thomas Monjalon; +Cc: dev

<snip>
> >>> + packets. */
> >>>  };
> >>
> >> You are extending the generic stats. This is not the idea behind xstats.
> >> The xstats are specific to the driver.
> >
> > I'd followed the example of:
> > http://patchwork.dpdk.org/dev/patchwork/patch/85/
> > to added generic extended stats (at least what I thought were
> > generic). I think that dropped packets should fall under struct
> > rte_eth_stats, and should perhaps be left there, as most NICs/drivers
> > should be able to provide that number. Would this be an agreeable
> solution?
> >
> > I have no other way to expose the total MAC errors and the total PHY
> > errors without Adding counters into struct ixgbe_hw_stats, but I wasn't sure
> if this was allowable, is it?
> >
> > The only other option is to round up all the errors into ierrors,
> > without having the granularity of what errors fall under. Is the
> > latter option to sum up the values under one umbrella preferred?
> 
> As Thomas explained, I think we should avoid adding fields in struct
> rte_eth_stats. This structure already contains statistics that are specific to
> some hardware drivers. We should try to go in the opposite direction and
> remove all the fields that do not apply to all nics (physical or virtual). For me, it
> would be only something like (rx, tx, rx_bytes, tx_bytes, rx_errors, tx_errors).
> 
> The xstats framework allows you to add any driver or hardware specific stats
> and I think it's the proper place to add them for ixgbe.
> 


Alright that sounds good.

> By the way, something could be modified in xstats framework. Today, the
> behavior is:
> - if ethdev->dev_ops->xstats_get != NULL, call it
> - else, dump the generic stats in xstats format

As it happens I did do this in: http://dpdk.org/dev/patchwork/patch/5324/ 
but I will reverse the order as what happens now is xstats from the driver
are displayed first and will look at always dumping generic stats in xstats format
as you mention below.

> 
> A better behavior could be:
> - Always dump the generic stats in xstats format
> - and if thdev->dev_ops->xstats_get != NULL, add driver-specific stats
> 
> This would avoid to duplicate code that dumps generic stats in all drivers.
> 
> So, to summarize what I think should be done for stats/xstats:
> 1. modify xstats behavior as described above 2. add the xstats dev_ops in
> ixgbe, it will dump the new stats
> 
> Bonus:
> 3. identify all the specific stats that could be removed from
>    rte_eth_stats and start to mark them as deprecated.
> 

Will do.

> 
> Regards,
> Olivier

Thanks
Maryam
<snip>

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

end of thread, other threads:[~2015-06-22 15:35 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-06-05 17:35 [dpdk-dev] [PATCH 0/4] expose ixgbe extended stats to dpdk apps Maryam Tahhan
2015-06-05 17:35 ` [dpdk-dev] [PATCH 1/4] ixgbe: expose extended error statistics Maryam Tahhan
2015-06-08 13:26   ` Tahhan, Maryam
2015-06-10  0:51   ` Stephen Hemminger
2015-06-10 14:24     ` Tahhan, Maryam
2015-06-17 12:52     ` Thomas Monjalon
2015-06-05 17:35 ` [dpdk-dev] [PATCH 2/4] ethdev: expose extended error stats Maryam Tahhan
2015-06-17 13:58   ` Thomas Monjalon
2015-06-17 14:47     ` Kyle Larose
2015-06-22 14:12     ` Tahhan, Maryam
2015-06-22 15:12       ` Olivier MATZ
2015-06-22 15:35         ` Tahhan, Maryam
2015-06-05 17:35 ` [dpdk-dev] [PATCH 3/4] testpmd: extend testpmd to show all extended stats Maryam Tahhan
2015-06-05 17:35 ` [dpdk-dev] [PATCH 4/4] app: replace dump_cfg with proc_info Maryam Tahhan
2015-06-05 21:08   ` Thomas Monjalon
2015-06-08 13:45     ` Tahhan, Maryam
2015-06-08 14:22       ` Thomas Monjalon

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