diff mbox

[dpdk-dev,v4,02/10] ethdev: update xstats_get() strings and Q handling

Message ID 1446204998-17151-3-git-send-email-harry.van.haaren@intel.com (mailing list archive)
State Superseded, archived
Headers

Commit Message

Van Haaren, Harry Oct. 30, 2015, 11:36 a.m. UTC 
  Update the strings used for presenting stats to adhere
to the scheme previously presented. Updated xstats_get()
function to handle Q information only if xstats() is not
implemented in the PMD, providing the PMD with the needed
flexibility to expose its extended Q stats.

Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
---
 lib/librte_ether/rte_ethdev.c | 38 +++++++++++++++++++++++---------------
 1 file changed, 23 insertions(+), 15 deletions(-)

Comments

Thomas Monjalon Nov. 2, 2015, 7:59 a.m. UTC | #1 
2015-10-30 11:36, Harry van Haaren:
> +       /* if xstats_get() is implemented by the PMD, the Q stats are done */
> +       if (dev->dev_ops->xstats_get != NULL)
> +               return count + xcount;
> +
>         /* per-rxq stats */
>         for (q = 0; q < dev->data->nb_rx_queues; q++) {
>                 for (i = 0; i < RTE_NB_RXQ_STATS; i++) {

Please could you explain why the generic per-queue stats are not used when
xstats is implemented in the driver?
Van Haaren, Harry Nov. 2, 2015, 10:17 a.m. UTC | #2 
Hi,

> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Monday, November 2, 2015 7:59 AM
> > +       /* if xstats_get() is implemented by the PMD, the Q stats are done */
> > +       if (dev->dev_ops->xstats_get != NULL)
> > +               return count + xcount;
> > +
> >         /* per-rxq stats */
> >         for (q = 0; q < dev->data->nb_rx_queues; q++) {
> >                 for (i = 0; i < RTE_NB_RXQ_STATS; i++) {
> 
> Please could you explain why the generic per-queue stats are not used when
> xstats is implemented in the driver?

Each PMD exposes its own queue stats so it has the flexibility of presenting them exactly has the hardware counts, in a human-readable order.

If the generic xstats were used, testpmd> xstats output would split a single queue's xstats to two places in the list. As stats are used during debugging, readability and clarity of the stats is vital in my opinion.

-Harry
Thomas Monjalon Nov. 2, 2015, 4:23 p.m. UTC | #3 
2015-11-02 10:17, Van Haaren, Harry:
> > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > Sent: Monday, November 2, 2015 7:59 AM
> > > +       /* if xstats_get() is implemented by the PMD, the Q stats are done */
> > > +       if (dev->dev_ops->xstats_get != NULL)
> > > +               return count + xcount;
> > > +
> > >         /* per-rxq stats */
> > >         for (q = 0; q < dev->data->nb_rx_queues; q++) {
> > >                 for (i = 0; i < RTE_NB_RXQ_STATS; i++) {
> > 
> > Please could you explain why the generic per-queue stats are not used when
> > xstats is implemented in the driver?
> 
> Each PMD exposes its own queue stats so it has the flexibility of presenting them exactly has the hardware counts, in a human-readable order.
> 
> If the generic xstats were used, testpmd> xstats output would split a single queue's xstats to two places in the list. As stats are used during debugging, readability and clarity of the stats is vital in my opinion.

Output control is the role of testpmd, not the driver.
I think you can reorder the stats in testpmd given that you have defined
a clear scheme naming (thanks).
diff mbox

Patch

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index f593f6e..07f0c26 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -137,27 +137,30 @@  struct rte_eth_xstats_name_off {
 };
 
 static const struct rte_eth_xstats_name_off rte_stats_strings[] = {
-	{"rx_packets", offsetof(struct rte_eth_stats, ipackets)},
-	{"tx_packets", offsetof(struct rte_eth_stats, opackets)},
-	{"rx_bytes", offsetof(struct rte_eth_stats, ibytes)},
-	{"tx_bytes", offsetof(struct rte_eth_stats, obytes)},
-	{"tx_errors", offsetof(struct rte_eth_stats, oerrors)},
+	{"rx_good_packets", offsetof(struct rte_eth_stats, ipackets)},
+	{"tx_good_packets", offsetof(struct rte_eth_stats, opackets)},
+	{"rx_good_bytes", offsetof(struct rte_eth_stats, ibytes)},
+	{"tx_good_bytes", offsetof(struct rte_eth_stats, obytes)},
 	{"rx_errors", offsetof(struct rte_eth_stats, ierrors)},
-	{"alloc_rx_buff_failed", offsetof(struct rte_eth_stats, rx_nombuf)},
+	{"tx_errors", offsetof(struct rte_eth_stats, oerrors)},
+	{"rx_mbuf_allocation_errors", offsetof(struct rte_eth_stats,
+		rx_nombuf)},
 };
+
 #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[] = {
-	{"rx_packets", offsetof(struct rte_eth_stats, q_ipackets)},
-	{"rx_bytes", offsetof(struct rte_eth_stats, q_ibytes)},
+	{"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_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[] = {
-	{"tx_packets", offsetof(struct rte_eth_stats, q_opackets)},
-	{"tx_bytes", offsetof(struct rte_eth_stats, q_obytes)},
-	{"tx_errors", offsetof(struct rte_eth_stats, q_errors)},
+	{"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]))
@@ -1666,8 +1669,6 @@  rte_eth_xstats_get(uint8_t port_id, struct rte_eth_xstats *xstats,
 
 	/* 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;
 
 	/* implemented by the driver */
 	if (dev->dev_ops->xstats_get != NULL) {
@@ -1679,6 +1680,9 @@  rte_eth_xstats_get(uint8_t port_id, struct rte_eth_xstats *xstats,
 
 		if (xcount < 0)
 			return xcount;
+	} else {
+		count += dev->data->nb_rx_queues * RTE_NB_RXQ_STATS;
+		count += dev->data->nb_tx_queues * RTE_NB_TXQ_STATS;
 	}
 
 	if (n < count + xcount)
@@ -1698,6 +1702,10 @@  rte_eth_xstats_get(uint8_t port_id, struct rte_eth_xstats *xstats,
 		xstats[count++].value = val;
 	}
 
+	/* if xstats_get() is implemented by the PMD, the Q stats are done */
+	if (dev->dev_ops->xstats_get != NULL)
+		return count + xcount;
+
 	/* per-rxq stats */
 	for (q = 0; q < dev->data->nb_rx_queues; q++) {
 		for (i = 0; i < RTE_NB_RXQ_STATS; i++) {
@@ -1706,7 +1714,7 @@  rte_eth_xstats_get(uint8_t port_id, struct rte_eth_xstats *xstats,
 					q * sizeof(uint64_t));
 			val = *stats_ptr;
 			snprintf(xstats[count].name, sizeof(xstats[count].name),
-				"rx_queue_%u_%s", q,
+				"rx_q%u_%s", q,
 				rte_rxq_stats_strings[i].name);
 			xstats[count++].value = val;
 		}
@@ -1720,7 +1728,7 @@  rte_eth_xstats_get(uint8_t port_id, struct rte_eth_xstats *xstats,
 					q * sizeof(uint64_t));
 			val = *stats_ptr;
 			snprintf(xstats[count].name, sizeof(xstats[count].name),
-				"tx_queue_%u_%s", q,
+				"tx_q%u_%s", q,
 				rte_txq_stats_strings[i].name);
 			xstats[count++].value = val;
 		}