diff mbox

[dpdk-dev,v3,3/4] bnx2x: Enhance stats get

Message ID 1459903028-3329-3-git-send-email-rasesh.mody@qlogic.com (mailing list archive)
State Changes Requested, archived
Headers

Commit Message

Rasesh Mody April 6, 2016, 12:37 a.m. UTC 
  Enhance the stats_get() routine to display drop counters under
imissed counter.
Added extended stats get support to provide additional info.

Signed-off-by: Rasesh Mody <rasesh.mody@qlogic.com>
---
 drivers/net/bnx2x/bnx2x_ethdev.c |   72 ++++++++++++++++++++++++++++++++++++++
 drivers/net/bnx2x/bnx2x_rxtx.c   |    2 ++
 2 files changed, 74 insertions(+)

Comments

Van Haaren, Harry April 6, 2016, 2:32 p.m. UTC | #1 
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Rasesh Mody
> Subject: [dpdk-dev] [PATCH v3 3/4] bnx2x: Enhance stats get

Hi Rasesh,

> +	snprintf(xstats[num].name, sizeof(xstats[num].name), "brb_drops");

I don't understand what a "brb" drop is.


> +	snprintf(xstats[num].name, sizeof(xstats[num].name), "tx_pfc");

Similarly here, and with some other of the xstats strings, it doesn't
become clear to me what exactly the value represents.

"mac_filter_discard" is descriptive and readable, but the next stat
has "mf_tag_discard" - these small inconsistencies make it much harder
(impossible?) to scrap the xstats strings and retrieve useful metadata.

I'll suggest leaving the xstats implementation part of this patch until
the next release, and we can align on the names of the stats.

-Harry
Rasesh Mody April 7, 2016, 5:10 a.m. UTC | #2 
> From: Van Haaren, Harry [mailto:harry.van.haaren@intel.com]
> Sent: Wednesday, April 06, 2016 7:33 AM
> 
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Rasesh Mody
> > Subject: [dpdk-dev] [PATCH v3 3/4] bnx2x: Enhance stats get
> 
> Hi Rasesh,
> 
> > +	snprintf(xstats[num].name, sizeof(xstats[num].name),
> "brb_drops");
> 
> I don't understand what a "brb" drop is.
> 
> 
> > +	snprintf(xstats[num].name, sizeof(xstats[num].name), "tx_pfc");
> 
> Similarly here, and with some other of the xstats strings, it doesn't become
> clear to me what exactly the value represents.
> 
> "mac_filter_discard" is descriptive and readable, but the next stat has
> "mf_tag_discard" - these small inconsistencies make it much harder
> (impossible?) to scrap the xstats strings and retrieve useful metadata.
> 
> I'll suggest leaving the xstats implementation part of this patch until the next
> release, and we can align on the names of the stats.

We'll re-work the patch to accommodate your suggestions. 

> 
> -Harry
Rasesh Mody May 4, 2016, 5:41 a.m. UTC | #3 
> From: Van Haaren, Harry [mailto:harry.van.haaren@intel.com]
> Sent: Wednesday, April 06, 2016 7:33 AM
> 
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Rasesh Mody
> > Subject: [dpdk-dev] [PATCH v3 3/4] bnx2x: Enhance stats get
> 
> Hi Rasesh,
> 
> > +	snprintf(xstats[num].name, sizeof(xstats[num].name),
> "brb_drops");
> 
> I don't understand what a "brb" drop is.
> 
> 
> > +	snprintf(xstats[num].name, sizeof(xstats[num].name), "tx_pfc");
> 
> Similarly here, and with some other of the xstats strings, it doesn't become
> clear to me what exactly the value represents.
> 
> "mac_filter_discard" is descriptive and readable, but the next stat has
> "mf_tag_discard" - these small inconsistencies make it much harder
> (impossible?) to scrap the xstats strings and retrieve useful metadata.
> 
> I'll suggest leaving the xstats implementation part of this patch until the next
> release, and we can align on the names of the stats.
> 
> -Harry

We have re-worked the patches and submitted v4. It incorporates changes to rename some of the stats.

Thanks!
Rasesh
diff mbox

Patch

diff --git a/drivers/net/bnx2x/bnx2x_ethdev.c b/drivers/net/bnx2x/bnx2x_ethdev.c
index 071b44f..1f38f6d 100644
--- a/drivers/net/bnx2x/bnx2x_ethdev.c
+++ b/drivers/net/bnx2x/bnx2x_ethdev.c
@@ -276,6 +276,9 @@  static void
 bnx2x_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
 {
 	struct bnx2x_softc *sc = dev->data->dev_private;
+	uint32_t brb_truncate_discard;
+	uint64_t brb_drops;
+	uint64_t brb_truncates;
 
 	PMD_INIT_FUNC_TRACE();
 
@@ -316,6 +319,73 @@  bnx2x_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
 	stats->rx_nombuf =
 		HILO_U64(sc->eth_stats.no_buff_discard_hi,
 				sc->eth_stats.no_buff_discard_lo);
+
+	brb_drops =
+		HILO_U64(sc->eth_stats.brb_drop_hi,
+			 sc->eth_stats.brb_drop_lo);
+
+	brb_truncates =
+		HILO_U64(sc->eth_stats.brb_truncate_hi,
+			 sc->eth_stats.brb_truncate_lo);
+
+	brb_truncate_discard = sc->eth_stats.brb_truncate_discard;
+
+	stats->imissed = brb_drops + brb_truncates +
+			 brb_truncate_discard + stats->rx_nombuf;
+}
+
+#define BNX2X_EXTENDED_STATS 9
+
+static int
+bnx2x_dev_xstats_get(struct rte_eth_dev *dev, struct rte_eth_xstats *xstats,
+		     unsigned n)
+{
+	struct bnx2x_softc *sc = dev->data->dev_private;
+	unsigned num = BNX2X_EXTENDED_STATS;
+
+	if (n < num)
+		return num;
+
+	num = 0;
+
+	bnx2x_stats_handle(sc, STATS_EVENT_UPDATE);
+
+	snprintf(xstats[num].name, sizeof(xstats[num].name), "brb_drops");
+	xstats[num++].value = HILO_U64(sc->eth_stats.brb_drop_hi,
+				       sc->eth_stats.brb_drop_lo);
+
+	snprintf(xstats[num].name, sizeof(xstats[num].name), "brb_truncates");
+	xstats[num++].value = HILO_U64(sc->eth_stats.brb_truncate_hi,
+				       sc->eth_stats.brb_truncate_lo);
+
+	snprintf(xstats[num].name, sizeof(xstats[num].name),
+		 "brb_truncate_discard");
+	xstats[num++].value = sc->eth_stats.brb_truncate_discard;
+
+	snprintf(xstats[num].name, sizeof(xstats[num].name),
+		 "mac_filter_discard");
+	xstats[num++].value = sc->eth_stats.mac_filter_discard;
+
+	snprintf(xstats[num].name, sizeof(xstats[num].name), "mf_tag_discard");
+	xstats[num++].value = sc->eth_stats.mf_tag_discard;
+
+	snprintf(xstats[num].name, sizeof(xstats[num].name), "tx_pause");
+	xstats[num++].value = HILO_U64(sc->eth_stats.pause_frames_sent_hi,
+				       sc->eth_stats.pause_frames_sent_lo);
+
+	snprintf(xstats[num].name, sizeof(xstats[num].name), "rx_pause");
+	xstats[num++].value = HILO_U64(sc->eth_stats.pause_frames_received_hi,
+				       sc->eth_stats.pause_frames_received_lo);
+
+	snprintf(xstats[num].name, sizeof(xstats[num].name), "tx_pfc");
+	xstats[num++].value = HILO_U64(sc->eth_stats.pfc_frames_sent_hi,
+				       sc->eth_stats.pfc_frames_sent_lo);
+
+	snprintf(xstats[num].name, sizeof(xstats[num].name), "rx_pfc");
+	xstats[num++].value = HILO_U64(sc->eth_stats.pfc_frames_received_hi,
+				       sc->eth_stats.pfc_frames_received_lo);
+
+	return num;
 }
 
 static void
@@ -360,6 +430,7 @@  static const struct eth_dev_ops bnx2x_eth_dev_ops = {
 	.allmulticast_disable         = bnx2x_dev_allmulticast_disable,
 	.link_update                  = bnx2x_dev_link_update,
 	.stats_get                    = bnx2x_dev_stats_get,
+	.xstats_get                   = bnx2x_dev_xstats_get,
 	.dev_infos_get                = bnx2x_dev_infos_get,
 	.rx_queue_setup               = bnx2x_dev_rx_queue_setup,
 	.rx_queue_release             = bnx2x_dev_rx_queue_release,
@@ -383,6 +454,7 @@  static const struct eth_dev_ops bnx2xvf_eth_dev_ops = {
 	.allmulticast_disable         = bnx2x_dev_allmulticast_disable,
 	.link_update                  = bnx2xvf_dev_link_update,
 	.stats_get                    = bnx2x_dev_stats_get,
+	.xstats_get                   = bnx2x_dev_xstats_get,
 	.dev_infos_get                = bnx2x_dev_infos_get,
 	.rx_queue_setup               = bnx2x_dev_rx_queue_setup,
 	.rx_queue_release             = bnx2x_dev_rx_queue_release,
diff --git a/drivers/net/bnx2x/bnx2x_rxtx.c b/drivers/net/bnx2x/bnx2x_rxtx.c
index 8b047d4..60bd08b 100644
--- a/drivers/net/bnx2x/bnx2x_rxtx.c
+++ b/drivers/net/bnx2x/bnx2x_rxtx.c
@@ -405,6 +405,8 @@  bnx2x_recv_pkts(void *p_rxq, struct rte_mbuf **rx_pkts, uint16_t nb_pkts)
 		new_mb = bnx2x_rxmbuf_alloc(rxq->mb_pool);
 		if (unlikely(!new_mb)) {
 			PMD_RX_LOG(ERR, "mbuf alloc fail fp[%02d]", fp->index);
+			rte_eth_devices[rxq->port_id].data->
+					rx_mbuf_alloc_failed++;
 			goto next_rx;
 		}