[PATCH] net/gve: Enable stats reporting for GQ format

[PATCH] net/gve: Enable stats reporting for GQ format

[Rushil Gupta]

Read from shared region to retrieve imissed statistics for GQ from device.
Tested using `show port xstats <port-id>` in interactive mode.
This metric can be triggered by using queues > cores.

Signed-off-by: Rushil Gupta <rushilg@google.com>
Reviewed-by: Joshua Washington <joshwash@google.com>
---
 drivers/net/gve/base/gve_adminq.h | 11 ++++
 drivers/net/gve/gve_ethdev.c      | 95 +++++++++++++++++++++++++++++++
 drivers/net/gve/gve_ethdev.h      |  6 ++
 3 files changed, 112 insertions(+)

diff --git a/drivers/net/gve/base/gve_adminq.h b/drivers/net/gve/base/gve_adminq.h
index e30b184913..f05362f85f 100644
--- a/drivers/net/gve/base/gve_adminq.h
+++ b/drivers/net/gve/base/gve_adminq.h
@@ -314,6 +314,17 @@ struct gve_stats_report {
 
 GVE_CHECK_STRUCT_LEN(8, gve_stats_report);
 
+/* Numbers of gve tx/rx stats in stats report. */
+#define GVE_TX_STATS_REPORT_NUM        6
+#define GVE_RX_STATS_REPORT_NUM        2
+
+/* Interval to schedule a stats report update, 20000ms. */
+#define GVE_STATS_REPORT_TIMER_PERIOD  20000
+
+/* Numbers of NIC tx/rx stats in stats report. */
+#define NIC_TX_STATS_REPORT_NUM        0
+#define NIC_RX_STATS_REPORT_NUM        4
+
 enum gve_stat_names {
 	/* stats from gve */
 	TX_WAKE_CNT			= 1,
diff --git a/drivers/net/gve/gve_ethdev.c b/drivers/net/gve/gve_ethdev.c
index ecd37ff37f..836136d993 100644
--- a/drivers/net/gve/gve_ethdev.c
+++ b/drivers/net/gve/gve_ethdev.c
@@ -125,6 +125,73 @@ gve_link_update(struct rte_eth_dev *dev, __rte_unused int wait_to_complete)
 	return rte_eth_linkstatus_set(dev, &link);
 }
 
+static int gve_alloc_stats_report(struct gve_priv *priv,
+		uint16_t nb_tx_queues, uint16_t nb_rx_queues)
+{
+	char z_name[RTE_MEMZONE_NAMESIZE];
+	int tx_stats_cnt;
+	int rx_stats_cnt;
+
+	tx_stats_cnt = (GVE_TX_STATS_REPORT_NUM + NIC_TX_STATS_REPORT_NUM) *
+		nb_tx_queues;
+	rx_stats_cnt = (GVE_RX_STATS_REPORT_NUM + NIC_RX_STATS_REPORT_NUM) *
+		nb_rx_queues;
+	priv->stats_report_len = sizeof(struct gve_stats_report) +
+		sizeof(struct stats) * (tx_stats_cnt + rx_stats_cnt);
+
+	snprintf(z_name, sizeof(z_name), "stats_report_%s", priv->pci_dev->device.name);
+	priv->stats_report_mem = rte_memzone_reserve_aligned(z_name,
+			priv->stats_report_len,
+			rte_socket_id(),
+			RTE_MEMZONE_IOVA_CONTIG, PAGE_SIZE);
+
+	if (!priv->stats_report_mem)
+		return -ENOMEM;
+
+	/* offset by skipping stats written by gve. */
+	priv->stats_start_idx = (GVE_TX_STATS_REPORT_NUM * nb_tx_queues) +
+		(GVE_RX_STATS_REPORT_NUM * nb_rx_queues);
+	priv->stats_end_idx = priv->stats_start_idx +
+		(NIC_TX_STATS_REPORT_NUM * nb_tx_queues) +
+		(NIC_RX_STATS_REPORT_NUM * nb_rx_queues) - 1;
+
+	return 0;
+}
+
+static void gve_free_stats_report(struct rte_eth_dev *dev)
+{
+	struct gve_priv *priv = dev->data->dev_private;
+	rte_memzone_free(priv->stats_report_mem);
+}
+
+/* Read Rx NIC stats from shared region */
+static void gve_get_imissed_from_nic(struct rte_eth_dev *dev)
+{
+	struct gve_stats_report *stats_report;
+	struct gve_rx_queue *rxq;
+	struct gve_priv *priv;
+	struct stats stat;
+	int queue_id;
+	int stat_id;
+	int i;
+
+	priv = dev->data->dev_private;
+	stats_report = (struct gve_stats_report *)
+		priv->stats_report_mem->addr;
+
+	for (i = priv->stats_start_idx; i <= priv->stats_end_idx; i++) {
+		stat = stats_report->stats[i];
+		queue_id = cpu_to_be32(stat.queue_id);
+		rxq = dev->data->rx_queues[queue_id];
+		if (rxq == NULL)
+			continue;
+		stat_id = cpu_to_be32(stat.stat_name);
+		/* Update imissed. */
+		if (stat_id == RX_NO_BUFFERS_POSTED)
+			rxq->stats.imissed = cpu_to_be64(stat.value);
+	}
+}
+
 static int
 gve_start_queues(struct rte_eth_dev *dev)
 {
@@ -176,6 +243,7 @@ gve_start_queues(struct rte_eth_dev *dev)
 static int
 gve_dev_start(struct rte_eth_dev *dev)
 {
+	struct gve_priv *priv;
 	int ret;
 
 	ret = gve_start_queues(dev);
@@ -187,6 +255,26 @@ gve_dev_start(struct rte_eth_dev *dev)
 	dev->data->dev_started = 1;
 	gve_link_update(dev, 0);
 
+	priv = dev->data->dev_private;
+	/* No stats available yet for Dqo. */
+	if (gve_is_gqi(priv)) {
+		ret = gve_alloc_stats_report(priv,
+				dev->data->nb_tx_queues,
+				dev->data->nb_rx_queues);
+		if (ret != 0) {
+			PMD_DRV_LOG(ERR,
+				"Failed to allocate region for stats reporting.");
+			return ret;
+		}
+		ret = gve_adminq_report_stats(priv, priv->stats_report_len,
+				priv->stats_report_mem->iova,
+				GVE_STATS_REPORT_TIMER_PERIOD);
+		if (ret != 0) {
+			PMD_DRV_LOG(ERR, "gve_adminq_report_stats command failed.");
+			return ret;
+		}
+	}
+
 	return 0;
 }
 
@@ -200,6 +288,9 @@ gve_dev_stop(struct rte_eth_dev *dev)
 
 	dev->data->dev_started = 0;
 
+	if (gve_is_gqi(dev->data->dev_private))
+		gve_free_stats_report(dev);
+
 	return 0;
 }
 
@@ -352,6 +443,8 @@ static int
 gve_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
 {
 	uint16_t i;
+	if (gve_is_gqi(dev->data->dev_private))
+		gve_get_imissed_from_nic(dev);
 
 	for (i = 0; i < dev->data->nb_tx_queues; i++) {
 		struct gve_tx_queue *txq = dev->data->tx_queues[i];
@@ -372,6 +465,7 @@ gve_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
 		stats->ibytes += rxq->stats.bytes;
 		stats->ierrors += rxq->stats.errors;
 		stats->rx_nombuf += rxq->stats.no_mbufs;
+		stats->imissed += rxq->stats.imissed;
 	}
 
 	return 0;
@@ -443,6 +537,7 @@ static const struct gve_xstats_name_offset rx_xstats_name_offset[] = {
 	{ "errors",                 RX_QUEUE_STATS_OFFSET(errors) },
 	{ "mbuf_alloc_errors",      RX_QUEUE_STATS_OFFSET(no_mbufs) },
 	{ "mbuf_alloc_errors_bulk", RX_QUEUE_STATS_OFFSET(no_mbufs_bulk) },
+	{ "imissed",                RX_QUEUE_STATS_OFFSET(imissed) },
 };
 
 static int
diff --git a/drivers/net/gve/gve_ethdev.h b/drivers/net/gve/gve_ethdev.h
index 58d8943e71..9893fcfee6 100644
--- a/drivers/net/gve/gve_ethdev.h
+++ b/drivers/net/gve/gve_ethdev.h
@@ -85,6 +85,7 @@ struct gve_rx_stats {
 	uint64_t errors;
 	uint64_t no_mbufs;
 	uint64_t no_mbufs_bulk;
+	uint64_t imissed;
 };
 
 struct gve_xstats_name_offset {
@@ -272,6 +273,11 @@ struct gve_priv {
 
 	struct gve_tx_queue **txqs;
 	struct gve_rx_queue **rxqs;
+
+	uint32_t stats_report_len;
+	const struct rte_memzone *stats_report_mem;
+	uint16_t stats_start_idx; /* start index of array of stats written by NIC */
+	uint16_t stats_end_idx; /* end index of array of stats written by NIC */
 };
 
 static inline bool
-- 
2.43.0.472.g3155946c3a-goog

Re: [PATCH] net/gve: Enable stats reporting for GQ format

[Ferruh Yigit]

On 12/22/2023 3:39 PM, Rushil Gupta wrote:
> Read from shared region to retrieve imissed statistics for GQ from device.
> Tested using `show port xstats <port-id>` in interactive mode.
> This metric can be triggered by using queues > cores.
> 

Looks good but please check following comments:

Checkpatch gives warning on the patch title, and this patch adds
'imissed' support so it can be added to the patch title, something like:
"net/gve: enable imissed stats for GQ format"

<...>

> +static int gve_alloc_stats_report(struct gve_priv *priv,
> +		uint16_t nb_tx_queues, uint16_t nb_rx_queues)
> +{
> +	char z_name[RTE_MEMZONE_NAMESIZE];
> +	int tx_stats_cnt;
> +	int rx_stats_cnt;
> +
> +	tx_stats_cnt = (GVE_TX_STATS_REPORT_NUM + NIC_TX_STATS_REPORT_NUM) *
> +		nb_tx_queues;
> +	rx_stats_cnt = (GVE_RX_STATS_REPORT_NUM + NIC_RX_STATS_REPORT_NUM) *
> +		nb_rx_queues;
> +	priv->stats_report_len = sizeof(struct gve_stats_report) +
> +		sizeof(struct stats) * (tx_stats_cnt + rx_stats_cnt);
> +
> +	snprintf(z_name, sizeof(z_name), "stats_report_%s", priv->pci_dev->device.name);
>

Can you please add 'gve_' prefix to the memzone name, to prevent any
possible collision.

<...>

> +static void gve_free_stats_report(struct rte_eth_dev *dev)
> +{
> +	struct gve_priv *priv = dev->data->dev_private;
> +	rte_memzone_free(priv->stats_report_mem);
>

What will happen if user asks stats/xstats after port stopped?

<...>

>  gve_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
>  {
>  	uint16_t i;
> +	if (gve_is_gqi(dev->data->dev_private))
> +		gve_get_imissed_from_nic(dev);
>  

This updates imissed in RxQ struct for all queues for basic stats, but
what if user only calls xstats, I guess in that case stat won't be updated.

Re: [PATCH] net/gve: Enable stats reporting for GQ format

[Rushil Gupta]

[-- Attachment #1: Type: text/plain, Size: 2370 bytes --]

On Fri, Jan 12, 2024 at 8:36 PM Ferruh Yigit <ferruh.yigit@amd.com> wrote:

> On 12/22/2023 3:39 PM, Rushil Gupta wrote:
> > Read from shared region to retrieve imissed statistics for GQ from
> device.
> > Tested using `show port xstats <port-id>` in interactive mode.
> > This metric can be triggered by using queues > cores.
> >
>
> Looks good but please check following comments:
>
> Checkpatch gives warning on the patch title, and this patch adds
> 'imissed' support so it can be added to the patch title, something like:
> "net/gve: enable imissed stats for GQ format"
>
> <...>
>
> > +static int gve_alloc_stats_report(struct gve_priv *priv,
> > +             uint16_t nb_tx_queues, uint16_t nb_rx_queues)
> > +{
> > +     char z_name[RTE_MEMZONE_NAMESIZE];
> > +     int tx_stats_cnt;
> > +     int rx_stats_cnt;
> > +
> > +     tx_stats_cnt = (GVE_TX_STATS_REPORT_NUM + NIC_TX_STATS_REPORT_NUM)
> *
> > +             nb_tx_queues;
> > +     rx_stats_cnt = (GVE_RX_STATS_REPORT_NUM + NIC_RX_STATS_REPORT_NUM)
> *
> > +             nb_rx_queues;
> > +     priv->stats_report_len = sizeof(struct gve_stats_report) +
> > +             sizeof(struct stats) * (tx_stats_cnt + rx_stats_cnt);
> > +
> > +     snprintf(z_name, sizeof(z_name), "stats_report_%s", priv->pci_dev->
> device.name);
> >
>
> Can you please add 'gve_' prefix to the memzone name, to prevent any
> possible collision.
>
Done.

>
> <...>
>
> > +static void gve_free_stats_report(struct rte_eth_dev *dev)
> > +{
> > +     struct gve_priv *priv = dev->data->dev_private;
> > +     rte_memzone_free(priv->stats_report_mem);
> >
>
> What will happen if user asks stats/xstats after port stopped?
>
Good catch. I have added a null check so that the driver doesn't try to
read stats from memory region that doesn't exist.

>
> <...>
>
> >  gve_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
> >  {
> >       uint16_t i;
> > +     if (gve_is_gqi(dev->data->dev_private))
> > +             gve_get_imissed_from_nic(dev);
> >
>
> This updates imissed in RxQ struct for all queues for basic stats, but
> what if user only calls xstats, I guess in that case stat won't be updated.
>

Yes; that is expected. Since imissed is a member of rte_eth_stats; calling
gve_dev_stats_get is the right way to get this stat.

[-- Attachment #2: Type: text/html, Size: 3409 bytes --]

[v2] net/gve: enable imissed stats for GQ format

[Rushil Gupta]

Read from shared region to retrieve imissed statistics for GQ from device.
Tested using `show port xstats <port-id>` in interactive mode.

Signed-off-by: Rushil Gupta <rushilg@google.com>
Reviewed-by: Joshua Washington <joshwash@google.com>
---
 drivers/net/gve/base/gve_adminq.h |  11 ++++
 drivers/net/gve/gve_ethdev.c      | 100 ++++++++++++++++++++++++++++++
 drivers/net/gve/gve_ethdev.h      |   6 ++
 3 files changed, 117 insertions(+)

diff --git a/drivers/net/gve/base/gve_adminq.h b/drivers/net/gve/base/gve_adminq.h
index e30b184913..f05362f85f 100644
--- a/drivers/net/gve/base/gve_adminq.h
+++ b/drivers/net/gve/base/gve_adminq.h
@@ -314,6 +314,17 @@ struct gve_stats_report {
 
 GVE_CHECK_STRUCT_LEN(8, gve_stats_report);
 
+/* Numbers of gve tx/rx stats in stats report. */
+#define GVE_TX_STATS_REPORT_NUM        6
+#define GVE_RX_STATS_REPORT_NUM        2
+
+/* Interval to schedule a stats report update, 20000ms. */
+#define GVE_STATS_REPORT_TIMER_PERIOD  20000
+
+/* Numbers of NIC tx/rx stats in stats report. */
+#define NIC_TX_STATS_REPORT_NUM        0
+#define NIC_RX_STATS_REPORT_NUM        4
+
 enum gve_stat_names {
 	/* stats from gve */
 	TX_WAKE_CNT			= 1,
diff --git a/drivers/net/gve/gve_ethdev.c b/drivers/net/gve/gve_ethdev.c
index ecd37ff37f..fe91e790bc 100644
--- a/drivers/net/gve/gve_ethdev.c
+++ b/drivers/net/gve/gve_ethdev.c
@@ -125,6 +125,78 @@ gve_link_update(struct rte_eth_dev *dev, __rte_unused int wait_to_complete)
 	return rte_eth_linkstatus_set(dev, &link);
 }
 
+static int
+gve_alloc_stats_report(struct gve_priv *priv,
+		uint16_t nb_tx_queues, uint16_t nb_rx_queues)
+{
+	char z_name[RTE_MEMZONE_NAMESIZE];
+	int tx_stats_cnt;
+	int rx_stats_cnt;
+
+	tx_stats_cnt = (GVE_TX_STATS_REPORT_NUM + NIC_TX_STATS_REPORT_NUM) *
+		nb_tx_queues;
+	rx_stats_cnt = (GVE_RX_STATS_REPORT_NUM + NIC_RX_STATS_REPORT_NUM) *
+		nb_rx_queues;
+	priv->stats_report_len = sizeof(struct gve_stats_report) +
+		sizeof(struct stats) * (tx_stats_cnt + rx_stats_cnt);
+
+	snprintf(z_name, sizeof(z_name), "stats_report_%s", priv->pci_dev->device.name);
+	priv->gve_stats_report_mem = rte_memzone_reserve_aligned(z_name,
+			priv->stats_report_len,
+			rte_socket_id(),
+			RTE_MEMZONE_IOVA_CONTIG, PAGE_SIZE);
+
+	if (!priv->gve_stats_report_mem)
+		return -ENOMEM;
+
+	/* offset by skipping stats written by gve. */
+	priv->stats_start_idx = (GVE_TX_STATS_REPORT_NUM * nb_tx_queues) +
+		(GVE_RX_STATS_REPORT_NUM * nb_rx_queues);
+	priv->stats_end_idx = priv->stats_start_idx +
+		(NIC_TX_STATS_REPORT_NUM * nb_tx_queues) +
+		(NIC_RX_STATS_REPORT_NUM * nb_rx_queues) - 1;
+
+	return 0;
+}
+
+static void
+gve_free_stats_report(struct rte_eth_dev *dev)
+{
+	struct gve_priv *priv = dev->data->dev_private;
+	rte_memzone_free(priv->gve_stats_report_mem);
+	priv->gve_stats_report_mem = NULL;
+}
+
+/* Read Rx NIC stats from shared region */
+static void
+gve_get_imissed_from_nic(struct rte_eth_dev *dev)
+{
+	struct gve_stats_report *stats_report;
+	struct gve_rx_queue *rxq;
+	struct gve_priv *priv;
+	struct stats stat;
+	int queue_id;
+	int stat_id;
+	int i;
+
+	priv = dev->data->dev_private;
+	if (!priv->gve_stats_report_mem)
+		return;
+	stats_report = (struct gve_stats_report *)
+		priv->gve_stats_report_mem->addr;
+	for (i = priv->stats_start_idx; i <= priv->stats_end_idx; i++) {
+		stat = stats_report->stats[i];
+		queue_id = cpu_to_be32(stat.queue_id);
+		rxq = dev->data->rx_queues[queue_id];
+		if (rxq == NULL)
+			continue;
+		stat_id = cpu_to_be32(stat.stat_name);
+		/* Update imissed. */
+		if (stat_id == RX_NO_BUFFERS_POSTED)
+			rxq->stats.imissed = cpu_to_be64(stat.value);
+	}
+}
+
 static int
 gve_start_queues(struct rte_eth_dev *dev)
 {
@@ -176,6 +248,7 @@ gve_start_queues(struct rte_eth_dev *dev)
 static int
 gve_dev_start(struct rte_eth_dev *dev)
 {
+	struct gve_priv *priv;
 	int ret;
 
 	ret = gve_start_queues(dev);
@@ -187,6 +260,26 @@ gve_dev_start(struct rte_eth_dev *dev)
 	dev->data->dev_started = 1;
 	gve_link_update(dev, 0);
 
+	priv = dev->data->dev_private;
+	/* No stats available yet for Dqo. */
+	if (gve_is_gqi(priv)) {
+		ret = gve_alloc_stats_report(priv,
+				dev->data->nb_tx_queues,
+				dev->data->nb_rx_queues);
+		if (ret != 0) {
+			PMD_DRV_LOG(ERR,
+				"Failed to allocate region for stats reporting.");
+			return ret;
+		}
+		ret = gve_adminq_report_stats(priv, priv->stats_report_len,
+				priv->gve_stats_report_mem->iova,
+				GVE_STATS_REPORT_TIMER_PERIOD);
+		if (ret != 0) {
+			PMD_DRV_LOG(ERR, "gve_adminq_report_stats command failed.");
+			return ret;
+		}
+	}
+
 	return 0;
 }
 
@@ -200,6 +293,9 @@ gve_dev_stop(struct rte_eth_dev *dev)
 
 	dev->data->dev_started = 0;
 
+	if (gve_is_gqi(dev->data->dev_private))
+		gve_free_stats_report(dev);
+
 	return 0;
 }
 
@@ -352,6 +448,8 @@ static int
 gve_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
 {
 	uint16_t i;
+	if (gve_is_gqi(dev->data->dev_private))
+		gve_get_imissed_from_nic(dev);
 
 	for (i = 0; i < dev->data->nb_tx_queues; i++) {
 		struct gve_tx_queue *txq = dev->data->tx_queues[i];
@@ -372,6 +470,7 @@ gve_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
 		stats->ibytes += rxq->stats.bytes;
 		stats->ierrors += rxq->stats.errors;
 		stats->rx_nombuf += rxq->stats.no_mbufs;
+		stats->imissed += rxq->stats.imissed;
 	}
 
 	return 0;
@@ -443,6 +542,7 @@ static const struct gve_xstats_name_offset rx_xstats_name_offset[] = {
 	{ "errors",                 RX_QUEUE_STATS_OFFSET(errors) },
 	{ "mbuf_alloc_errors",      RX_QUEUE_STATS_OFFSET(no_mbufs) },
 	{ "mbuf_alloc_errors_bulk", RX_QUEUE_STATS_OFFSET(no_mbufs_bulk) },
+	{ "imissed",                RX_QUEUE_STATS_OFFSET(imissed) },
 };
 
 static int
diff --git a/drivers/net/gve/gve_ethdev.h b/drivers/net/gve/gve_ethdev.h
index 58d8943e71..6ffc5c2deb 100644
--- a/drivers/net/gve/gve_ethdev.h
+++ b/drivers/net/gve/gve_ethdev.h
@@ -85,6 +85,7 @@ struct gve_rx_stats {
 	uint64_t errors;
 	uint64_t no_mbufs;
 	uint64_t no_mbufs_bulk;
+	uint64_t imissed;
 };
 
 struct gve_xstats_name_offset {
@@ -272,6 +273,11 @@ struct gve_priv {
 
 	struct gve_tx_queue **txqs;
 	struct gve_rx_queue **rxqs;
+
+	uint32_t stats_report_len;
+	const struct rte_memzone *gve_stats_report_mem;
+	uint16_t stats_start_idx; /* start index of array of stats written by NIC */
+	uint16_t stats_end_idx; /* end index of array of stats written by NIC */
 };
 
 static inline bool
-- 
2.34.1

Re: [PATCH] net/gve: Enable stats reporting for GQ format

[Joshua Washington]

[-- Attachment #1: Type: text/plain, Size: 3454 bytes --]

>
>
>> <...>
>>
>> >  gve_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
>> >  {
>> >       uint16_t i;
>> > +     if (gve_is_gqi(dev->data->dev_private))
>> > +             gve_get_imissed_from_nic(dev);
>> >
>>
>> This updates imissed in RxQ struct for all queues for basic stats, but
>> what if user only calls xstats, I guess in that case stat won't be
>> updated.
>>
>
> Yes; that is expected. Since imissed is a member of rte_eth_stats; calling
> gve_dev_stats_get is the right way to get this stat.
>

I actually think that this should be a counter backed by an xstat as well.
As far as I can tell xstats is meant to be a more flexible version of
stats, and ultimately should be able to handle a superset of what basic
stats can handle, for the purposes of querying, etc.

On Mon, Jan 15, 2024 at 10:18 PM Rushil Gupta <rushilg@google.com> wrote:

>
>
> On Fri, Jan 12, 2024 at 8:36 PM Ferruh Yigit <ferruh.yigit@amd.com> wrote:
>
>> On 12/22/2023 3:39 PM, Rushil Gupta wrote:
>> > Read from shared region to retrieve imissed statistics for GQ from
>> device.
>> > Tested using `show port xstats <port-id>` in interactive mode.
>> > This metric can be triggered by using queues > cores.
>> >
>>
>> Looks good but please check following comments:
>>
>> Checkpatch gives warning on the patch title, and this patch adds
>> 'imissed' support so it can be added to the patch title, something like:
>> "net/gve: enable imissed stats for GQ format"
>>
>> <...>
>>
>> > +static int gve_alloc_stats_report(struct gve_priv *priv,
>> > +             uint16_t nb_tx_queues, uint16_t nb_rx_queues)
>> > +{
>> > +     char z_name[RTE_MEMZONE_NAMESIZE];
>> > +     int tx_stats_cnt;
>> > +     int rx_stats_cnt;
>> > +
>> > +     tx_stats_cnt = (GVE_TX_STATS_REPORT_NUM +
>> NIC_TX_STATS_REPORT_NUM) *
>> > +             nb_tx_queues;
>> > +     rx_stats_cnt = (GVE_RX_STATS_REPORT_NUM +
>> NIC_RX_STATS_REPORT_NUM) *
>> > +             nb_rx_queues;
>> > +     priv->stats_report_len = sizeof(struct gve_stats_report) +
>> > +             sizeof(struct stats) * (tx_stats_cnt + rx_stats_cnt);
>> > +
>> > +     snprintf(z_name, sizeof(z_name), "stats_report_%s",
>> priv->pci_dev->device.name);
>> >
>>
>> Can you please add 'gve_' prefix to the memzone name, to prevent any
>> possible collision.
>>
> Done.
>
>>
>> <...>
>>
>> > +static void gve_free_stats_report(struct rte_eth_dev *dev)
>> > +{
>> > +     struct gve_priv *priv = dev->data->dev_private;
>> > +     rte_memzone_free(priv->stats_report_mem);
>> >
>>
>> What will happen if user asks stats/xstats after port stopped?
>>
> Good catch. I have added a null check so that the driver doesn't try to
> read stats from memory region that doesn't exist.
>
>>
>> <...>
>>
>> >  gve_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
>> >  {
>> >       uint16_t i;
>> > +     if (gve_is_gqi(dev->data->dev_private))
>> > +             gve_get_imissed_from_nic(dev);
>> >
>>
>> This updates imissed in RxQ struct for all queues for basic stats, but
>> what if user only calls xstats, I guess in that case stat won't be
>> updated.
>>
>
> Yes; that is expected. Since imissed is a member of rte_eth_stats; calling
> gve_dev_stats_get is the right way to get this stat.
>


-- 

Joshua Washington | Software Engineer | joshwash@google.com | (414) 366-4423

[-- Attachment #2: Type: text/html, Size: 5973 bytes --]

Re: [PATCH] net/gve: Enable stats reporting for GQ format

[Ferruh Yigit]

On 1/16/2024 6:18 AM, Rushil Gupta wrote:
> 
> 
> On Fri, Jan 12, 2024 at 8:36 PM Ferruh Yigit <ferruh.yigit@amd.com
> <mailto:ferruh.yigit@amd.com>> wrote:
> 
>     On 12/22/2023 3:39 PM, Rushil Gupta wrote:
>     > Read from shared region to retrieve imissed statistics for GQ from
>     device.
>     > Tested using `show port xstats <port-id>` in interactive mode.
>     > This metric can be triggered by using queues > cores.
>     >
> 
>     Looks good but please check following comments:
> 
>     Checkpatch gives warning on the patch title, and this patch adds
>     'imissed' support so it can be added to the patch title, something like:
>     "net/gve: enable imissed stats for GQ format"
> 
>     <...>
> 
>     > +static int gve_alloc_stats_report(struct gve_priv *priv,
>     > +             uint16_t nb_tx_queues, uint16_t nb_rx_queues)
>     > +{
>     > +     char z_name[RTE_MEMZONE_NAMESIZE];
>     > +     int tx_stats_cnt;
>     > +     int rx_stats_cnt;
>     > +
>     > +     tx_stats_cnt = (GVE_TX_STATS_REPORT_NUM +
>     NIC_TX_STATS_REPORT_NUM) *
>     > +             nb_tx_queues;
>     > +     rx_stats_cnt = (GVE_RX_STATS_REPORT_NUM +
>     NIC_RX_STATS_REPORT_NUM) *
>     > +             nb_rx_queues;
>     > +     priv->stats_report_len = sizeof(struct gve_stats_report) +
>     > +             sizeof(struct stats) * (tx_stats_cnt + rx_stats_cnt);
>     > +
>     > +     snprintf(z_name, sizeof(z_name), "stats_report_%s",
>     priv->pci_dev->device.name <http://device.name>);
>     >
> 
>     Can you please add 'gve_' prefix to the memzone name, to prevent any
>     possible collision.
> 
> Done. 
> 
> 
>     <...>
> 
>     > +static void gve_free_stats_report(struct rte_eth_dev *dev)
>     > +{
>     > +     struct gve_priv *priv = dev->data->dev_private;
>     > +     rte_memzone_free(priv->stats_report_mem);
>     >
> 
>     What will happen if user asks stats/xstats after port stopped?
> 
> Good catch. I have added a null check so that the driver doesn't try to
> read stats from memory region that doesn't exist. 
> 
> 
>     <...>
> 
>     >  gve_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats
>     *stats)
>     >  {
>     >       uint16_t i;
>     > +     if (gve_is_gqi(dev->data->dev_private))
>     > +             gve_get_imissed_from_nic(dev);
>     > 
> 
>     This updates imissed in RxQ struct for all queues for basic stats, but
>     what if user only calls xstats, I guess in that case stat won't be
>     updated.
> 
>  
> Yes; that is expected. Since imissed is a member of rte_eth_stats;
> calling gve_dev_stats_get is the right way to get this stat.
>

I don't think it is expected.
xstats contains the basic stats too, if users calls xstats API,
expectation is to get correct values.

Re: [PATCH] net/gve: Enable stats reporting for GQ format

[Rushil Gupta]

[-- Attachment #1: Type: text/plain, Size: 3212 bytes --]

Those are fair points. I'll fix this by simply
calling gve_get_imissed_from_nic from gve_xstats_get in the v3 patch.

On Wed, Jan 17, 2024 at 3:10 PM Ferruh Yigit <ferruh.yigit@amd.com> wrote:

> On 1/16/2024 6:18 AM, Rushil Gupta wrote:
> >
> >
> > On Fri, Jan 12, 2024 at 8:36 PM Ferruh Yigit <ferruh.yigit@amd.com
> > <mailto:ferruh.yigit@amd.com>> wrote:
> >
> >     On 12/22/2023 3:39 PM, Rushil Gupta wrote:
> >     > Read from shared region to retrieve imissed statistics for GQ from
> >     device.
> >     > Tested using `show port xstats <port-id>` in interactive mode.
> >     > This metric can be triggered by using queues > cores.
> >     >
> >
> >     Looks good but please check following comments:
> >
> >     Checkpatch gives warning on the patch title, and this patch adds
> >     'imissed' support so it can be added to the patch title, something
> like:
> >     "net/gve: enable imissed stats for GQ format"
> >
> >     <...>
> >
> >     > +static int gve_alloc_stats_report(struct gve_priv *priv,
> >     > +             uint16_t nb_tx_queues, uint16_t nb_rx_queues)
> >     > +{
> >     > +     char z_name[RTE_MEMZONE_NAMESIZE];
> >     > +     int tx_stats_cnt;
> >     > +     int rx_stats_cnt;
> >     > +
> >     > +     tx_stats_cnt = (GVE_TX_STATS_REPORT_NUM +
> >     NIC_TX_STATS_REPORT_NUM) *
> >     > +             nb_tx_queues;
> >     > +     rx_stats_cnt = (GVE_RX_STATS_REPORT_NUM +
> >     NIC_RX_STATS_REPORT_NUM) *
> >     > +             nb_rx_queues;
> >     > +     priv->stats_report_len = sizeof(struct gve_stats_report) +
> >     > +             sizeof(struct stats) * (tx_stats_cnt + rx_stats_cnt);
> >     > +
> >     > +     snprintf(z_name, sizeof(z_name), "stats_report_%s",
> >     priv->pci_dev->device.name <http://device.name>);
> >     >
> >
> >     Can you please add 'gve_' prefix to the memzone name, to prevent any
> >     possible collision.
> >
> > Done.
> >
> >
> >     <...>
> >
> >     > +static void gve_free_stats_report(struct rte_eth_dev *dev)
> >     > +{
> >     > +     struct gve_priv *priv = dev->data->dev_private;
> >     > +     rte_memzone_free(priv->stats_report_mem);
> >     >
> >
> >     What will happen if user asks stats/xstats after port stopped?
> >
> > Good catch. I have added a null check so that the driver doesn't try to
> > read stats from memory region that doesn't exist.
> >
> >
> >     <...>
> >
> >     >  gve_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats
> >     *stats)
> >     >  {
> >     >       uint16_t i;
> >     > +     if (gve_is_gqi(dev->data->dev_private))
> >     > +             gve_get_imissed_from_nic(dev);
> >     >
> >
> >     This updates imissed in RxQ struct for all queues for basic stats,
> but
> >     what if user only calls xstats, I guess in that case stat won't be
> >     updated.
> >
> >
> > Yes; that is expected. Since imissed is a member of rte_eth_stats;
> > calling gve_dev_stats_get is the right way to get this stat.
> >
>
> I don't think it is expected.
> xstats contains the basic stats too, if users calls xstats API,
> expectation is to get correct values.
>
>

[-- Attachment #2: Type: text/html, Size: 4599 bytes --]

[v3] net/gve: enable imissed stats for GQ format

[Rushil Gupta]

Read from shared region to retrieve imissed statistics for GQ from device.

Signed-off-by: Rushil Gupta <rushilg@google.com>
Reviewed-by: Joshua Washington <joshwash@google.com>
---
 drivers/net/gve/base/gve_adminq.h |  11 ++++
 drivers/net/gve/gve_ethdev.c      | 103 ++++++++++++++++++++++++++++++
 drivers/net/gve/gve_ethdev.h      |   6 ++
 3 files changed, 120 insertions(+)

diff --git a/drivers/net/gve/base/gve_adminq.h b/drivers/net/gve/base/gve_adminq.h
index e30b184913..f05362f85f 100644
--- a/drivers/net/gve/base/gve_adminq.h
+++ b/drivers/net/gve/base/gve_adminq.h
@@ -314,6 +314,17 @@ struct gve_stats_report {
 
 GVE_CHECK_STRUCT_LEN(8, gve_stats_report);
 
+/* Numbers of gve tx/rx stats in stats report. */
+#define GVE_TX_STATS_REPORT_NUM        6
+#define GVE_RX_STATS_REPORT_NUM        2
+
+/* Interval to schedule a stats report update, 20000ms. */
+#define GVE_STATS_REPORT_TIMER_PERIOD  20000
+
+/* Numbers of NIC tx/rx stats in stats report. */
+#define NIC_TX_STATS_REPORT_NUM        0
+#define NIC_RX_STATS_REPORT_NUM        4
+
 enum gve_stat_names {
 	/* stats from gve */
 	TX_WAKE_CNT			= 1,
diff --git a/drivers/net/gve/gve_ethdev.c b/drivers/net/gve/gve_ethdev.c
index ecd37ff37f..723b9332b8 100644
--- a/drivers/net/gve/gve_ethdev.c
+++ b/drivers/net/gve/gve_ethdev.c
@@ -125,6 +125,78 @@ gve_link_update(struct rte_eth_dev *dev, __rte_unused int wait_to_complete)
 	return rte_eth_linkstatus_set(dev, &link);
 }
 
+static int
+gve_alloc_stats_report(struct gve_priv *priv,
+		uint16_t nb_tx_queues, uint16_t nb_rx_queues)
+{
+	char z_name[RTE_MEMZONE_NAMESIZE];
+	int tx_stats_cnt;
+	int rx_stats_cnt;
+
+	tx_stats_cnt = (GVE_TX_STATS_REPORT_NUM + NIC_TX_STATS_REPORT_NUM) *
+		nb_tx_queues;
+	rx_stats_cnt = (GVE_RX_STATS_REPORT_NUM + NIC_RX_STATS_REPORT_NUM) *
+		nb_rx_queues;
+	priv->stats_report_len = sizeof(struct gve_stats_report) +
+		sizeof(struct stats) * (tx_stats_cnt + rx_stats_cnt);
+
+	snprintf(z_name, sizeof(z_name), "stats_report_%s", priv->pci_dev->device.name);
+	priv->gve_stats_report_mem = rte_memzone_reserve_aligned(z_name,
+			priv->stats_report_len,
+			rte_socket_id(),
+			RTE_MEMZONE_IOVA_CONTIG, PAGE_SIZE);
+
+	if (!priv->gve_stats_report_mem)
+		return -ENOMEM;
+
+	/* offset by skipping stats written by gve. */
+	priv->stats_start_idx = (GVE_TX_STATS_REPORT_NUM * nb_tx_queues) +
+		(GVE_RX_STATS_REPORT_NUM * nb_rx_queues);
+	priv->stats_end_idx = priv->stats_start_idx +
+		(NIC_TX_STATS_REPORT_NUM * nb_tx_queues) +
+		(NIC_RX_STATS_REPORT_NUM * nb_rx_queues) - 1;
+
+	return 0;
+}
+
+static void
+gve_free_stats_report(struct rte_eth_dev *dev)
+{
+	struct gve_priv *priv = dev->data->dev_private;
+	rte_memzone_free(priv->gve_stats_report_mem);
+	priv->gve_stats_report_mem = NULL;
+}
+
+/* Read Rx NIC stats from shared region */
+static void
+gve_get_imissed_from_nic(struct rte_eth_dev *dev)
+{
+	struct gve_stats_report *stats_report;
+	struct gve_rx_queue *rxq;
+	struct gve_priv *priv;
+	struct stats stat;
+	int queue_id;
+	int stat_id;
+	int i;
+
+	priv = dev->data->dev_private;
+	if (!priv->gve_stats_report_mem)
+		return;
+	stats_report = (struct gve_stats_report *)
+		priv->gve_stats_report_mem->addr;
+	for (i = priv->stats_start_idx; i <= priv->stats_end_idx; i++) {
+		stat = stats_report->stats[i];
+		queue_id = cpu_to_be32(stat.queue_id);
+		rxq = dev->data->rx_queues[queue_id];
+		if (rxq == NULL)
+			continue;
+		stat_id = cpu_to_be32(stat.stat_name);
+		/* Update imissed. */
+		if (stat_id == RX_NO_BUFFERS_POSTED)
+			rxq->stats.imissed = cpu_to_be64(stat.value);
+	}
+}
+
 static int
 gve_start_queues(struct rte_eth_dev *dev)
 {
@@ -176,6 +248,7 @@ gve_start_queues(struct rte_eth_dev *dev)
 static int
 gve_dev_start(struct rte_eth_dev *dev)
 {
+	struct gve_priv *priv;
 	int ret;
 
 	ret = gve_start_queues(dev);
@@ -187,6 +260,26 @@ gve_dev_start(struct rte_eth_dev *dev)
 	dev->data->dev_started = 1;
 	gve_link_update(dev, 0);
 
+	priv = dev->data->dev_private;
+	/* No stats available yet for Dqo. */
+	if (gve_is_gqi(priv)) {
+		ret = gve_alloc_stats_report(priv,
+				dev->data->nb_tx_queues,
+				dev->data->nb_rx_queues);
+		if (ret != 0) {
+			PMD_DRV_LOG(ERR,
+				"Failed to allocate region for stats reporting.");
+			return ret;
+		}
+		ret = gve_adminq_report_stats(priv, priv->stats_report_len,
+				priv->gve_stats_report_mem->iova,
+				GVE_STATS_REPORT_TIMER_PERIOD);
+		if (ret != 0) {
+			PMD_DRV_LOG(ERR, "gve_adminq_report_stats command failed.");
+			return ret;
+		}
+	}
+
 	return 0;
 }
 
@@ -200,6 +293,9 @@ gve_dev_stop(struct rte_eth_dev *dev)
 
 	dev->data->dev_started = 0;
 
+	if (gve_is_gqi(dev->data->dev_private))
+		gve_free_stats_report(dev);
+
 	return 0;
 }
 
@@ -352,6 +448,8 @@ static int
 gve_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
 {
 	uint16_t i;
+	if (gve_is_gqi(dev->data->dev_private))
+		gve_get_imissed_from_nic(dev);
 
 	for (i = 0; i < dev->data->nb_tx_queues; i++) {
 		struct gve_tx_queue *txq = dev->data->tx_queues[i];
@@ -372,6 +470,7 @@ gve_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
 		stats->ibytes += rxq->stats.bytes;
 		stats->ierrors += rxq->stats.errors;
 		stats->rx_nombuf += rxq->stats.no_mbufs;
+		stats->imissed += rxq->stats.imissed;
 	}
 
 	return 0;
@@ -443,6 +542,7 @@ static const struct gve_xstats_name_offset rx_xstats_name_offset[] = {
 	{ "errors",                 RX_QUEUE_STATS_OFFSET(errors) },
 	{ "mbuf_alloc_errors",      RX_QUEUE_STATS_OFFSET(no_mbufs) },
 	{ "mbuf_alloc_errors_bulk", RX_QUEUE_STATS_OFFSET(no_mbufs_bulk) },
+	{ "imissed",                RX_QUEUE_STATS_OFFSET(imissed) },
 };
 
 static int
@@ -471,6 +571,9 @@ gve_xstats_get(struct rte_eth_dev *dev,
 	uint16_t i, j, count = gve_xstats_count(dev);
 	const char *stats;
 
+	if (gve_is_gqi(dev->data->dev_private))
+		gve_get_imissed_from_nic(dev);
+
 	if (xstats == NULL || size < count)
 		return count;
 
diff --git a/drivers/net/gve/gve_ethdev.h b/drivers/net/gve/gve_ethdev.h
index 58d8943e71..6ffc5c2deb 100644
--- a/drivers/net/gve/gve_ethdev.h
+++ b/drivers/net/gve/gve_ethdev.h
@@ -85,6 +85,7 @@ struct gve_rx_stats {
 	uint64_t errors;
 	uint64_t no_mbufs;
 	uint64_t no_mbufs_bulk;
+	uint64_t imissed;
 };
 
 struct gve_xstats_name_offset {
@@ -272,6 +273,11 @@ struct gve_priv {
 
 	struct gve_tx_queue **txqs;
 	struct gve_rx_queue **rxqs;
+
+	uint32_t stats_report_len;
+	const struct rte_memzone *gve_stats_report_mem;
+	uint16_t stats_start_idx; /* start index of array of stats written by NIC */
+	uint16_t stats_end_idx; /* end index of array of stats written by NIC */
 };
 
 static inline bool
-- 
2.34.1

Re: [v3] net/gve: enable imissed stats for GQ format

[Ferruh Yigit]

On 1/19/2024 2:26 PM, Rushil Gupta wrote:
> Read from shared region to retrieve imissed statistics for GQ from device.
> 
> Signed-off-by: Rushil Gupta <rushilg@google.com>
> Reviewed-by: Joshua Washington <joshwash@google.com>
>

<...>

> +static int
> +gve_alloc_stats_report(struct gve_priv *priv,
> +		uint16_t nb_tx_queues, uint16_t nb_rx_queues)
> +{
> +	char z_name[RTE_MEMZONE_NAMESIZE];
> +	int tx_stats_cnt;
> +	int rx_stats_cnt;
> +
> +	tx_stats_cnt = (GVE_TX_STATS_REPORT_NUM + NIC_TX_STATS_REPORT_NUM) *
> +		nb_tx_queues;
> +	rx_stats_cnt = (GVE_RX_STATS_REPORT_NUM + NIC_RX_STATS_REPORT_NUM) *
> +		nb_rx_queues;
> +	priv->stats_report_len = sizeof(struct gve_stats_report) +
> +		sizeof(struct stats) * (tx_stats_cnt + rx_stats_cnt);
> +
> +	snprintf(z_name, sizeof(z_name), "stats_report_%s", priv->pci_dev->device.name);
> +	priv->gve_stats_report_mem = rte_memzone_reserve_aligned(z_name,
> +			priv->stats_report_len,
> +			rte_socket_id(),
> +			RTE_MEMZONE_IOVA_CONTIG, PAGE_SIZE);
> 

Adding 'gve_' prefix to memzone name comment seems missed.

Re: [v3] net/gve: enable imissed stats for GQ format

[Rushil Gupta]

[-- Attachment #1: Type: text/plain, Size: 1527 bytes --]

I misinterpreted your comment earlier and prefixed memzone variable name
with "gve" instead of memzone name. Will fix it in v4. Thanks!

On Fri, Jan 19, 2024 at 8:55 PM Ferruh Yigit <ferruh.yigit@amd.com> wrote:

> On 1/19/2024 2:26 PM, Rushil Gupta wrote:
> > Read from shared region to retrieve imissed statistics for GQ from
> device.
> >
> > Signed-off-by: Rushil Gupta <rushilg@google.com>
> > Reviewed-by: Joshua Washington <joshwash@google.com>
> >
>
> <...>
>
> > +static int
> > +gve_alloc_stats_report(struct gve_priv *priv,
> > +             uint16_t nb_tx_queues, uint16_t nb_rx_queues)
> > +{
> > +     char z_name[RTE_MEMZONE_NAMESIZE];
> > +     int tx_stats_cnt;
> > +     int rx_stats_cnt;
> > +
> > +     tx_stats_cnt = (GVE_TX_STATS_REPORT_NUM + NIC_TX_STATS_REPORT_NUM)
> *
> > +             nb_tx_queues;
> > +     rx_stats_cnt = (GVE_RX_STATS_REPORT_NUM + NIC_RX_STATS_REPORT_NUM)
> *
> > +             nb_rx_queues;
> > +     priv->stats_report_len = sizeof(struct gve_stats_report) +
> > +             sizeof(struct stats) * (tx_stats_cnt + rx_stats_cnt);
> > +
> > +     snprintf(z_name, sizeof(z_name), "stats_report_%s", priv->pci_dev->
> device.name);
> > +     priv->gve_stats_report_mem = rte_memzone_reserve_aligned(z_name,
> > +                     priv->stats_report_len,
> > +                     rte_socket_id(),
> > +                     RTE_MEMZONE_IOVA_CONTIG, PAGE_SIZE);
> >
>
> Adding 'gve_' prefix to memzone name comment seems missed.
>
>

[-- Attachment #2: Type: text/html, Size: 2277 bytes --]

[v4] net/gve: enable imissed stats for GQ format

[Rushil Gupta]

Read from shared region to retrieve imissed statistics for GQ from device.

Signed-off-by: Rushil Gupta <rushilg@google.com>
Reviewed-by: Joshua Washington <joshwash@google.com>
---
 drivers/net/gve/base/gve_adminq.h |  11 ++++
 drivers/net/gve/gve_ethdev.c      | 104 ++++++++++++++++++++++++++++++
 drivers/net/gve/gve_ethdev.h      |   6 ++
 3 files changed, 121 insertions(+)

diff --git a/drivers/net/gve/base/gve_adminq.h b/drivers/net/gve/base/gve_adminq.h
index e30b184913..f05362f85f 100644
--- a/drivers/net/gve/base/gve_adminq.h
+++ b/drivers/net/gve/base/gve_adminq.h
@@ -314,6 +314,17 @@ struct gve_stats_report {
 
 GVE_CHECK_STRUCT_LEN(8, gve_stats_report);
 
+/* Numbers of gve tx/rx stats in stats report. */
+#define GVE_TX_STATS_REPORT_NUM        6
+#define GVE_RX_STATS_REPORT_NUM        2
+
+/* Interval to schedule a stats report update, 20000ms. */
+#define GVE_STATS_REPORT_TIMER_PERIOD  20000
+
+/* Numbers of NIC tx/rx stats in stats report. */
+#define NIC_TX_STATS_REPORT_NUM        0
+#define NIC_RX_STATS_REPORT_NUM        4
+
 enum gve_stat_names {
 	/* stats from gve */
 	TX_WAKE_CNT			= 1,
diff --git a/drivers/net/gve/gve_ethdev.c b/drivers/net/gve/gve_ethdev.c
index ecd37ff37f..d162fd3864 100644
--- a/drivers/net/gve/gve_ethdev.c
+++ b/drivers/net/gve/gve_ethdev.c
@@ -125,6 +125,79 @@ gve_link_update(struct rte_eth_dev *dev, __rte_unused int wait_to_complete)
 	return rte_eth_linkstatus_set(dev, &link);
 }
 
+static int
+gve_alloc_stats_report(struct gve_priv *priv,
+		uint16_t nb_tx_queues, uint16_t nb_rx_queues)
+{
+	char z_name[RTE_MEMZONE_NAMESIZE];
+	int tx_stats_cnt;
+	int rx_stats_cnt;
+
+	tx_stats_cnt = (GVE_TX_STATS_REPORT_NUM + NIC_TX_STATS_REPORT_NUM) *
+		nb_tx_queues;
+	rx_stats_cnt = (GVE_RX_STATS_REPORT_NUM + NIC_RX_STATS_REPORT_NUM) *
+		nb_rx_queues;
+	priv->stats_report_len = sizeof(struct gve_stats_report) +
+		sizeof(struct stats) * (tx_stats_cnt + rx_stats_cnt);
+
+	snprintf(z_name, sizeof(z_name), "gve_stats_report_%s",
+			priv->pci_dev->device.name);
+	priv->stats_report_mem = rte_memzone_reserve_aligned(z_name,
+			priv->stats_report_len,
+			rte_socket_id(),
+			RTE_MEMZONE_IOVA_CONTIG, PAGE_SIZE);
+
+	if (!priv->stats_report_mem)
+		return -ENOMEM;
+
+	/* offset by skipping stats written by gve. */
+	priv->stats_start_idx = (GVE_TX_STATS_REPORT_NUM * nb_tx_queues) +
+		(GVE_RX_STATS_REPORT_NUM * nb_rx_queues);
+	priv->stats_end_idx = priv->stats_start_idx +
+		(NIC_TX_STATS_REPORT_NUM * nb_tx_queues) +
+		(NIC_RX_STATS_REPORT_NUM * nb_rx_queues) - 1;
+
+	return 0;
+}
+
+static void
+gve_free_stats_report(struct rte_eth_dev *dev)
+{
+	struct gve_priv *priv = dev->data->dev_private;
+	rte_memzone_free(priv->stats_report_mem);
+	priv->stats_report_mem = NULL;
+}
+
+/* Read Rx NIC stats from shared region */
+static void
+gve_get_imissed_from_nic(struct rte_eth_dev *dev)
+{
+	struct gve_stats_report *stats_report;
+	struct gve_rx_queue *rxq;
+	struct gve_priv *priv;
+	struct stats stat;
+	int queue_id;
+	int stat_id;
+	int i;
+
+	priv = dev->data->dev_private;
+	if (!priv->stats_report_mem)
+		return;
+	stats_report = (struct gve_stats_report *)
+		priv->stats_report_mem->addr;
+	for (i = priv->stats_start_idx; i <= priv->stats_end_idx; i++) {
+		stat = stats_report->stats[i];
+		queue_id = cpu_to_be32(stat.queue_id);
+		rxq = dev->data->rx_queues[queue_id];
+		if (rxq == NULL)
+			continue;
+		stat_id = cpu_to_be32(stat.stat_name);
+		/* Update imissed. */
+		if (stat_id == RX_NO_BUFFERS_POSTED)
+			rxq->stats.imissed = cpu_to_be64(stat.value);
+	}
+}
+
 static int
 gve_start_queues(struct rte_eth_dev *dev)
 {
@@ -176,6 +249,7 @@ gve_start_queues(struct rte_eth_dev *dev)
 static int
 gve_dev_start(struct rte_eth_dev *dev)
 {
+	struct gve_priv *priv;
 	int ret;
 
 	ret = gve_start_queues(dev);
@@ -187,6 +261,26 @@ gve_dev_start(struct rte_eth_dev *dev)
 	dev->data->dev_started = 1;
 	gve_link_update(dev, 0);
 
+	priv = dev->data->dev_private;
+	/* No stats available yet for Dqo. */
+	if (gve_is_gqi(priv)) {
+		ret = gve_alloc_stats_report(priv,
+				dev->data->nb_tx_queues,
+				dev->data->nb_rx_queues);
+		if (ret != 0) {
+			PMD_DRV_LOG(ERR,
+				"Failed to allocate region for stats reporting.");
+			return ret;
+		}
+		ret = gve_adminq_report_stats(priv, priv->stats_report_len,
+				priv->stats_report_mem->iova,
+				GVE_STATS_REPORT_TIMER_PERIOD);
+		if (ret != 0) {
+			PMD_DRV_LOG(ERR, "gve_adminq_report_stats command failed.");
+			return ret;
+		}
+	}
+
 	return 0;
 }
 
@@ -200,6 +294,9 @@ gve_dev_stop(struct rte_eth_dev *dev)
 
 	dev->data->dev_started = 0;
 
+	if (gve_is_gqi(dev->data->dev_private))
+		gve_free_stats_report(dev);
+
 	return 0;
 }
 
@@ -352,6 +449,8 @@ static int
 gve_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
 {
 	uint16_t i;
+	if (gve_is_gqi(dev->data->dev_private))
+		gve_get_imissed_from_nic(dev);
 
 	for (i = 0; i < dev->data->nb_tx_queues; i++) {
 		struct gve_tx_queue *txq = dev->data->tx_queues[i];
@@ -372,6 +471,7 @@ gve_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
 		stats->ibytes += rxq->stats.bytes;
 		stats->ierrors += rxq->stats.errors;
 		stats->rx_nombuf += rxq->stats.no_mbufs;
+		stats->imissed += rxq->stats.imissed;
 	}
 
 	return 0;
@@ -443,6 +543,7 @@ static const struct gve_xstats_name_offset rx_xstats_name_offset[] = {
 	{ "errors",                 RX_QUEUE_STATS_OFFSET(errors) },
 	{ "mbuf_alloc_errors",      RX_QUEUE_STATS_OFFSET(no_mbufs) },
 	{ "mbuf_alloc_errors_bulk", RX_QUEUE_STATS_OFFSET(no_mbufs_bulk) },
+	{ "imissed",                RX_QUEUE_STATS_OFFSET(imissed) },
 };
 
 static int
@@ -471,6 +572,9 @@ gve_xstats_get(struct rte_eth_dev *dev,
 	uint16_t i, j, count = gve_xstats_count(dev);
 	const char *stats;
 
+	if (gve_is_gqi(dev->data->dev_private))
+		gve_get_imissed_from_nic(dev);
+
 	if (xstats == NULL || size < count)
 		return count;
 
diff --git a/drivers/net/gve/gve_ethdev.h b/drivers/net/gve/gve_ethdev.h
index 58d8943e71..9893fcfee6 100644
--- a/drivers/net/gve/gve_ethdev.h
+++ b/drivers/net/gve/gve_ethdev.h
@@ -85,6 +85,7 @@ struct gve_rx_stats {
 	uint64_t errors;
 	uint64_t no_mbufs;
 	uint64_t no_mbufs_bulk;
+	uint64_t imissed;
 };
 
 struct gve_xstats_name_offset {
@@ -272,6 +273,11 @@ struct gve_priv {
 
 	struct gve_tx_queue **txqs;
 	struct gve_rx_queue **rxqs;
+
+	uint32_t stats_report_len;
+	const struct rte_memzone *stats_report_mem;
+	uint16_t stats_start_idx; /* start index of array of stats written by NIC */
+	uint16_t stats_end_idx; /* end index of array of stats written by NIC */
 };
 
 static inline bool
-- 
2.34.1

Re: [v4] net/gve: enable imissed stats for GQ format

[Ferruh Yigit]

On 1/19/2024 5:09 PM, Rushil Gupta wrote:
> Read from shared region to retrieve imissed statistics for GQ from device.
> 
> Signed-off-by: Rushil Gupta <rushilg@google.com>
> Reviewed-by: Joshua Washington <joshwash@google.com>
>

Acked-by: Ferruh Yigit <ferruh.yigit@amd.com>

Applied to dpdk-next-net/main, thanks.

[PATCH] net/gve: Enable stats reporting for GQ format

[Rushil Gupta]

Read from shared region to retrieve imissed statistics for GQ from device.
Tested using `show port xstats <port-id>` in interactive mode.
This metric can be triggered by using queues > cores.

Signed-off-by: Rushil Gupta <rushilg@google.com>
Reviewed-by: Joshua Washington <joshwash@google.com>
---
 drivers/net/gve/base/gve_adminq.h | 11 ++++
 drivers/net/gve/gve_ethdev.c      | 95 +++++++++++++++++++++++++++++++
 drivers/net/gve/gve_ethdev.h      |  6 ++
 3 files changed, 112 insertions(+)

diff --git a/drivers/net/gve/base/gve_adminq.h b/drivers/net/gve/base/gve_adminq.h
index e30b184913..f05362f85f 100644
--- a/drivers/net/gve/base/gve_adminq.h
+++ b/drivers/net/gve/base/gve_adminq.h
@@ -314,6 +314,17 @@ struct gve_stats_report {
 
 GVE_CHECK_STRUCT_LEN(8, gve_stats_report);
 
+/* Numbers of gve tx/rx stats in stats report. */
+#define GVE_TX_STATS_REPORT_NUM        6
+#define GVE_RX_STATS_REPORT_NUM        2
+
+/* Interval to schedule a stats report update, 20000ms. */
+#define GVE_STATS_REPORT_TIMER_PERIOD  20000
+
+/* Numbers of NIC tx/rx stats in stats report. */
+#define NIC_TX_STATS_REPORT_NUM        0
+#define NIC_RX_STATS_REPORT_NUM        4
+
 enum gve_stat_names {
 	/* stats from gve */
 	TX_WAKE_CNT			= 1,
diff --git a/drivers/net/gve/gve_ethdev.c b/drivers/net/gve/gve_ethdev.c
index ecd37ff37f..986418cf5b 100644
--- a/drivers/net/gve/gve_ethdev.c
+++ b/drivers/net/gve/gve_ethdev.c
@@ -125,6 +125,73 @@ gve_link_update(struct rte_eth_dev *dev, __rte_unused int wait_to_complete)
 	return rte_eth_linkstatus_set(dev, &link);
 }
 
+static int gve_alloc_stats_report(struct gve_priv *priv,
+		uint16_t nb_tx_queues, uint16_t nb_rx_queues)
+{
+	char z_name[RTE_MEMZONE_NAMESIZE];
+	int tx_stats_cnt;
+	int rx_stats_cnt;
+
+	tx_stats_cnt = (GVE_TX_STATS_REPORT_NUM + NIC_TX_STATS_REPORT_NUM) *
+		nb_tx_queues;
+	rx_stats_cnt = (GVE_RX_STATS_REPORT_NUM + NIC_RX_STATS_REPORT_NUM) *
+		nb_rx_queues;
+	priv->stats_report_len = sizeof(struct gve_stats_report) +
+		sizeof(struct stats) * (tx_stats_cnt + rx_stats_cnt);
+
+	snprintf(z_name, sizeof(z_name), "stats_report_%s", priv->pci_dev->device.name);
+	priv->stats_report_mem = rte_memzone_reserve_aligned(z_name,
+			priv->stats_report_len,
+			rte_socket_id(),
+			RTE_MEMZONE_IOVA_CONTIG, PAGE_SIZE);
+
+	if (!priv->stats_report_mem)
+		return -ENOMEM;
+
+	/* offset by skipping stats written by gve. */
+	priv->stats_start_idx = (GVE_TX_STATS_REPORT_NUM * nb_tx_queues) +
+		(GVE_RX_STATS_REPORT_NUM * nb_rx_queues);
+	priv->stats_end_idx = priv->stats_start_idx +
+		(NIC_TX_STATS_REPORT_NUM * nb_tx_queues) +
+		(NIC_RX_STATS_REPORT_NUM * nb_rx_queues) - 1;
+
+	return 0;
+}
+
+static void gve_free_stats_report(struct rte_eth_dev *dev)
+{
+	struct gve_priv *priv = dev->data->dev_private;
+	rte_memzone_free(priv->stats_report_mem);
+}
+
+/* Read Rx NIC stats from shared region */
+static void gve_get_imissed_from_nic(struct rte_eth_dev *dev)
+{
+	struct gve_stats_report *stats_report;
+	struct gve_rx_queue *rxq;
+	struct gve_priv *priv;
+	struct stats stat;
+	int queue_id;
+	int stat_id;
+	int i;
+
+	priv = dev->data->dev_private;
+	stats_report = (struct gve_stats_report *)
+		priv->stats_report_mem->addr;
+
+	for (i = priv->stats_start_idx; i <= priv->stats_end_idx; i++) {
+		stat = stats_report->stats[i];
+		queue_id = cpu_to_be32(stat.queue_id);
+		rxq = dev->data->rx_queues[queue_id];
+		if (rxq == NULL)
+			continue;
+		stat_id = cpu_to_be32(stat.stat_name);
+		/* Update imissed. */
+		if (stat_id == RX_NO_BUFFERS_POSTED)
+			rxq->stats.imissed = cpu_to_be64(stat.value);
+	}
+}
+
 static int
 gve_start_queues(struct rte_eth_dev *dev)
 {
@@ -176,6 +243,7 @@ gve_start_queues(struct rte_eth_dev *dev)
 static int
 gve_dev_start(struct rte_eth_dev *dev)
 {
+	struct gve_priv *priv;
 	int ret;
 
 	ret = gve_start_queues(dev);
@@ -187,6 +255,26 @@ gve_dev_start(struct rte_eth_dev *dev)
 	dev->data->dev_started = 1;
 	gve_link_update(dev, 0);
 
+	priv = dev->data->dev_private;
+	/* No stats available yet for Dqo. */
+	if (gve_is_gqi(priv) {
+		ret = gve_alloc_stats_report(priv,
+				dev->data->nb_tx_queues,
+				dev->data->nb_rx_queues);
+		if (ret != 0) {
+			PMD_DRV_LOG(ERR,
+				"Failed to allocate region for stats reporting.");
+			return ret;
+		}
+		ret = gve_adminq_report_stats(priv, priv->stats_report_len,
+				priv->stats_report_mem->iova,
+				GVE_STATS_REPORT_TIMER_PERIOD);
+		if (ret != 0) {
+			PMD_DRV_LOG(ERR, "gve_adminq_report_stats command failed.");
+			return ret;
+		}
+	}
+
 	return 0;
 }
 
@@ -200,6 +288,9 @@ gve_dev_stop(struct rte_eth_dev *dev)
 
 	dev->data->dev_started = 0;
 
+	if (gve_is_gqi(dev->data->dev_private))
+		gve_free_stats_report(dev);
+
 	return 0;
 }
 
@@ -352,6 +443,8 @@ static int
 gve_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
 {
 	uint16_t i;
+	if (gve_is_gqi(dev->data->dev_private))
+		gve_get_imissed_from_nic(dev);
 
 	for (i = 0; i < dev->data->nb_tx_queues; i++) {
 		struct gve_tx_queue *txq = dev->data->tx_queues[i];
@@ -372,6 +465,7 @@ gve_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
 		stats->ibytes += rxq->stats.bytes;
 		stats->ierrors += rxq->stats.errors;
 		stats->rx_nombuf += rxq->stats.no_mbufs;
+		stats->imissed += rxq->stats.imissed;
 	}
 
 	return 0;
@@ -443,6 +537,7 @@ static const struct gve_xstats_name_offset rx_xstats_name_offset[] = {
 	{ "errors",                 RX_QUEUE_STATS_OFFSET(errors) },
 	{ "mbuf_alloc_errors",      RX_QUEUE_STATS_OFFSET(no_mbufs) },
 	{ "mbuf_alloc_errors_bulk", RX_QUEUE_STATS_OFFSET(no_mbufs_bulk) },
+	{ "imissed",                RX_QUEUE_STATS_OFFSET(imissed) },
 };
 
 static int
diff --git a/drivers/net/gve/gve_ethdev.h b/drivers/net/gve/gve_ethdev.h
index 58d8943e71..9893fcfee6 100644
--- a/drivers/net/gve/gve_ethdev.h
+++ b/drivers/net/gve/gve_ethdev.h
@@ -85,6 +85,7 @@ struct gve_rx_stats {
 	uint64_t errors;
 	uint64_t no_mbufs;
 	uint64_t no_mbufs_bulk;
+	uint64_t imissed;
 };
 
 struct gve_xstats_name_offset {
@@ -272,6 +273,11 @@ struct gve_priv {
 
 	struct gve_tx_queue **txqs;
 	struct gve_rx_queue **rxqs;
+
+	uint32_t stats_report_len;
+	const struct rte_memzone *stats_report_mem;
+	uint16_t stats_start_idx; /* start index of array of stats written by NIC */
+	uint16_t stats_end_idx; /* end index of array of stats written by NIC */
 };
 
 static inline bool
-- 
2.43.0.472.g3155946c3a-goog

[PATCH] net/gve: Enable stats reporting for GQ format

[Rushil Gupta]

Read from shared region to retrieve imissed statistics for GQ from device.
Tested using `show port xstats <port-id>` in interactive mode.
This metric can be triggered by using queues > cores.
---
 drivers/net/gve/base/gve_adminq.h |  11 +++
 drivers/net/gve/gve_ethdev.c      | 117 ++++++++++++++++++++++++++++++
 drivers/net/gve/gve_ethdev.h      |   6 ++
 3 files changed, 134 insertions(+)

diff --git a/drivers/net/gve/base/gve_adminq.h b/drivers/net/gve/base/gve_adminq.h
index e30b184913..f05362f85f 100644
--- a/drivers/net/gve/base/gve_adminq.h
+++ b/drivers/net/gve/base/gve_adminq.h
@@ -314,6 +314,17 @@ struct gve_stats_report {
 
 GVE_CHECK_STRUCT_LEN(8, gve_stats_report);
 
+/* Numbers of gve tx/rx stats in stats report. */
+#define GVE_TX_STATS_REPORT_NUM        6
+#define GVE_RX_STATS_REPORT_NUM        2
+
+/* Interval to schedule a stats report update, 20000ms. */
+#define GVE_STATS_REPORT_TIMER_PERIOD  20000
+
+/* Numbers of NIC tx/rx stats in stats report. */
+#define NIC_TX_STATS_REPORT_NUM        0
+#define NIC_RX_STATS_REPORT_NUM        4
+
 enum gve_stat_names {
 	/* stats from gve */
 	TX_WAKE_CNT			= 1,
diff --git a/drivers/net/gve/gve_ethdev.c b/drivers/net/gve/gve_ethdev.c
index ecd37ff37f..8e9596bb83 100644
--- a/drivers/net/gve/gve_ethdev.c
+++ b/drivers/net/gve/gve_ethdev.c
@@ -125,6 +125,73 @@ gve_link_update(struct rte_eth_dev *dev, __rte_unused int wait_to_complete)
 	return rte_eth_linkstatus_set(dev, &link);
 }
 
+static int gve_alloc_stats_report(struct gve_priv *priv,
+		uint16_t nb_tx_queues, uint16_t nb_rx_queues)
+{
+	char z_name[RTE_MEMZONE_NAMESIZE];
+	int tx_stats_cnt;
+	int rx_stats_cnt;
+
+	tx_stats_cnt = (GVE_TX_STATS_REPORT_NUM + NIC_TX_STATS_REPORT_NUM) *
+		nb_tx_queues;
+	rx_stats_cnt = (GVE_RX_STATS_REPORT_NUM + NIC_RX_STATS_REPORT_NUM) *
+		nb_rx_queues;
+	priv->stats_report_len = sizeof(struct gve_stats_report) +
+		sizeof(struct stats) * (tx_stats_cnt + rx_stats_cnt);
+
+	snprintf(z_name, sizeof(z_name), "stats_report_%s", priv->pci_dev->device.name);
+	priv->stats_report_mem = rte_memzone_reserve_aligned(z_name,
+			priv->stats_report_len,
+			rte_socket_id(),
+			RTE_MEMZONE_IOVA_CONTIG, PAGE_SIZE);
+
+	if (!priv->stats_report_mem)
+		return -ENOMEM;
+
+	/* offset by skipping stats written by gve. */
+	priv->stats_start_idx = (GVE_TX_STATS_REPORT_NUM * nb_tx_queues) +
+		(GVE_RX_STATS_REPORT_NUM * nb_rx_queues);
+	priv->stats_end_idx = priv->stats_start_idx +
+		(NIC_TX_STATS_REPORT_NUM * nb_tx_queues) +
+		(NIC_RX_STATS_REPORT_NUM * nb_rx_queues) - 1;
+
+	return 0;
+}
+
+static void gve_free_stats_report(struct rte_eth_dev *dev)
+{
+	struct gve_priv *priv = dev->data->dev_private;
+	rte_memzone_free(priv->stats_report_mem);
+}
+
+/* Read Rx NIC stats from shared region */
+static void gve_get_imissed_from_nic(struct rte_eth_dev *dev)
+{
+	struct gve_stats_report *stats_report;
+	struct gve_rx_queue *rxq;
+	struct gve_priv *priv;
+	struct stats stat;
+	int queue_id;
+	int stat_id;
+	int i;
+
+	priv = dev->data->dev_private;
+	stats_report = (struct gve_stats_report *)
+		priv->stats_report_mem->addr;
+
+	for (i = priv->stats_start_idx; i <= priv->stats_end_idx; i++) {
+		stat = stats_report->stats[i];
+		queue_id = cpu_to_be32(stat.queue_id);
+		rxq = dev->data->rx_queues[queue_id];
+		if (rxq == NULL)
+			continue;
+		stat_id = cpu_to_be32(stat.stat_name);
+		/* Update imissed. */
+		if (stat_id == RX_NO_BUFFERS_POSTED)
+			rxq->stats.imissed = cpu_to_be64(stat.value);
+	}
+}
+
 static int
 gve_start_queues(struct rte_eth_dev *dev)
 {
@@ -176,6 +243,7 @@ gve_start_queues(struct rte_eth_dev *dev)
 static int
 gve_dev_start(struct rte_eth_dev *dev)
 {
+	struct gve_priv *priv;
 	int ret;
 
 	ret = gve_start_queues(dev);
@@ -187,6 +255,26 @@ gve_dev_start(struct rte_eth_dev *dev)
 	dev->data->dev_started = 1;
 	gve_link_update(dev, 0);
 
+	priv = dev->data->dev_private;
+	/* No stats available yet for Dqo. */
+	if (gve_is_gqi(priv) {
+		ret = gve_alloc_stats_report(priv,
+				dev->data->nb_tx_queues,
+				dev->data->nb_rx_queues);
+		if (ret != 0) {
+			PMD_DRV_LOG(ERR,
+				"Failed to allocate region for stats reporting.");
+			return ret;
+		}
+		ret = gve_adminq_report_stats(priv, priv->stats_report_len,
+				priv->stats_report_mem->iova,
+				GVE_STATS_REPORT_TIMER_PERIOD);
+		if (ret != 0) {
+			PMD_DRV_LOG(ERR, "gve_adminq_report_stats command failed.");
+			return ret;
+		}
+	}
+
 	return 0;
 }
 
@@ -200,6 +288,9 @@ gve_dev_stop(struct rte_eth_dev *dev)
 
 	dev->data->dev_started = 0;
 
+	if (gve_is_gqi(dev->data->dev_private))
+		gve_free_stats_report(dev);
+
 	return 0;
 }
 
@@ -352,6 +443,8 @@ static int
 gve_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
 {
 	uint16_t i;
+	if (gve_is_gqi(dev->data->dev_private))
+		gve_get_imissed_from_nic(dev);
 
 	for (i = 0; i < dev->data->nb_tx_queues; i++) {
 		struct gve_tx_queue *txq = dev->data->tx_queues[i];
@@ -372,6 +465,7 @@ gve_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
 		stats->ibytes += rxq->stats.bytes;
 		stats->ierrors += rxq->stats.errors;
 		stats->rx_nombuf += rxq->stats.no_mbufs;
+		stats->imissed += rxq->stats.imissed;
 	}
 
 	return 0;
@@ -443,6 +537,7 @@ static const struct gve_xstats_name_offset rx_xstats_name_offset[] = {
 	{ "errors",                 RX_QUEUE_STATS_OFFSET(errors) },
 	{ "mbuf_alloc_errors",      RX_QUEUE_STATS_OFFSET(no_mbufs) },
 	{ "mbuf_alloc_errors_bulk", RX_QUEUE_STATS_OFFSET(no_mbufs_bulk) },
+	{ "imissed",                RX_QUEUE_STATS_OFFSET(imissed) },
 };
 
 static int
@@ -627,6 +722,28 @@ pci_dev_msix_vec_count(struct rte_pci_device *pdev)
 	return 0;
 }
 
+static int
+pci_dev_msix_vec_count(struct rte_pci_device *pdev)
+{
+	off_t msix_pos = rte_pci_find_capability(pdev, RTE_PCI_CAP_ID_MSIX);
+	uint8_t msix_cap = pci_dev_find_capability(pdev, PCI_CAP_ID_MSIX);
+	uint16_t control;
+	int ret;
+
+	if (msix_pos > 0 && rte_pci_read_config(pdev, &control, sizeof(control),
+		msix_pos + RTE_PCI_MSIX_FLAGS) == sizeof(control))
+		return (control & RTE_PCI_MSIX_FLAGS_QSIZE) + 1;
+	if (!msix_cap)
+		return 0;
+
+	return 0;
+	ret = rte_pci_read_config(pdev, &control, sizeof(control), msix_cap + PCI_MSIX_FLAGS);
+	if (ret != sizeof(control))
+		return 0;
+
+	return (control & PCI_MSIX_FLAGS_QSIZE) + 1;
+}
+
 static int
 gve_setup_device_resources(struct gve_priv *priv)
 {
diff --git a/drivers/net/gve/gve_ethdev.h b/drivers/net/gve/gve_ethdev.h
index 58d8943e71..9893fcfee6 100644
--- a/drivers/net/gve/gve_ethdev.h
+++ b/drivers/net/gve/gve_ethdev.h
@@ -85,6 +85,7 @@ struct gve_rx_stats {
 	uint64_t errors;
 	uint64_t no_mbufs;
 	uint64_t no_mbufs_bulk;
+	uint64_t imissed;
 };
 
 struct gve_xstats_name_offset {
@@ -272,6 +273,11 @@ struct gve_priv {
 
 	struct gve_tx_queue **txqs;
 	struct gve_rx_queue **rxqs;
+
+	uint32_t stats_report_len;
+	const struct rte_memzone *stats_report_mem;
+	uint16_t stats_start_idx; /* start index of array of stats written by NIC */
+	uint16_t stats_end_idx; /* end index of array of stats written by NIC */
 };
 
 static inline bool
-- 
2.43.0.472.g3155946c3a-goog

[PATCH] net/gve: Enable stats reporting for GQ format

[Rushil Gupta]

Read from shared region to retrieve imissed statistics for GQ.
Tested using `show port xstats <port-id>` in interactive mode.
This metric can be triggered by using queues > cores.

Signed-off-by: Rushil Gupta <rushilg@google.com>
Reviewed-by: Joshua Washington <joshwash@google.com>
---
 drivers/net/gve/base/gve_adminq.h |  11 +++
 drivers/net/gve/gve_ethdev.c      | 142 ++++++++++++++++++++++++++++--
 drivers/net/gve/gve_ethdev.h      |  20 ++++-
 3 files changed, 167 insertions(+), 6 deletions(-)

diff --git a/drivers/net/gve/base/gve_adminq.h b/drivers/net/gve/base/gve_adminq.h
index e30b184913..f05362f85f 100644
--- a/drivers/net/gve/base/gve_adminq.h
+++ b/drivers/net/gve/base/gve_adminq.h
@@ -314,6 +314,17 @@ struct gve_stats_report {
 
 GVE_CHECK_STRUCT_LEN(8, gve_stats_report);
 
+/* Numbers of gve tx/rx stats in stats report. */
+#define GVE_TX_STATS_REPORT_NUM        6
+#define GVE_RX_STATS_REPORT_NUM        2
+
+/* Interval to schedule a stats report update, 20000ms. */
+#define GVE_STATS_REPORT_TIMER_PERIOD  20000
+
+/* Numbers of NIC tx/rx stats in stats report. */
+#define NIC_TX_STATS_REPORT_NUM        0
+#define NIC_RX_STATS_REPORT_NUM        4
+
 enum gve_stat_names {
 	/* stats from gve */
 	TX_WAKE_CNT			= 1,
diff --git a/drivers/net/gve/gve_ethdev.c b/drivers/net/gve/gve_ethdev.c
index ecd37ff37f..bb535a863f 100644
--- a/drivers/net/gve/gve_ethdev.c
+++ b/drivers/net/gve/gve_ethdev.c
@@ -125,6 +125,73 @@ gve_link_update(struct rte_eth_dev *dev, __rte_unused int wait_to_complete)
 	return rte_eth_linkstatus_set(dev, &link);
 }
 
+static int gve_alloc_stats_report(struct gve_priv *priv,
+               uint16_t nb_tx_queues, uint16_t nb_rx_queues)
+{
+	char z_name[RTE_MEMZONE_NAMESIZE];
+	int tx_stats_cnt;
+	int rx_stats_cnt;
+
+	tx_stats_cnt = (GVE_TX_STATS_REPORT_NUM + NIC_TX_STATS_REPORT_NUM) *
+		nb_tx_queues;
+	rx_stats_cnt = (GVE_RX_STATS_REPORT_NUM + NIC_RX_STATS_REPORT_NUM) *
+		nb_rx_queues;
+	priv->stats_report_len = sizeof(struct gve_stats_report) +
+		sizeof(struct stats) * (tx_stats_cnt + rx_stats_cnt);
+
+	snprintf(z_name, sizeof(z_name), "stats_report_%s", priv->pci_dev->device.name);
+	priv->stats_report_mem = rte_memzone_reserve_aligned(z_name,
+			priv->stats_report_len,
+			rte_socket_id(),
+			RTE_MEMZONE_IOVA_CONTIG, PAGE_SIZE);
+
+	if (!priv->stats_report_mem)
+		return -ENOMEM;
+
+	/* offset by skipping stats written by gve. */
+	priv->stats_start_idx = (GVE_TX_STATS_REPORT_NUM * nb_tx_queues) +
+		(GVE_RX_STATS_REPORT_NUM * nb_rx_queues);
+	priv->stats_end_idx = priv->stats_start_idx +
+		(NIC_TX_STATS_REPORT_NUM * nb_tx_queues) +
+		(NIC_RX_STATS_REPORT_NUM * nb_rx_queues) - 1;
+
+	return 0;
+}
+
+static void gve_free_stats_report(struct rte_eth_dev *dev)
+{
+        struct gve_priv *priv = dev->data->dev_private;
+        rte_memzone_free(priv->stats_report_mem);
+}
+
+/* Read Rx NIC stats from shared region */
+static void gve_get_imissed_from_nic(struct rte_eth_dev *dev)
+{
+	struct gve_stats_report *stats_report;
+	struct gve_rx_queue *rxq;
+	struct gve_priv *priv;
+	struct stats stat;
+	int queue_id;
+	int stat_id;
+	int i;
+
+	priv = dev->data->dev_private;
+	stats_report = (struct gve_stats_report *)
+		priv->stats_report_mem->addr;
+
+	for (i = priv->stats_start_idx; i <= priv->stats_end_idx; i++) {
+		stat = stats_report->stats[i];
+		queue_id = cpu_to_be32(stat.queue_id);
+		rxq = dev->data->rx_queues[queue_id];
+		if (rxq == NULL)
+			continue;
+		stat_id = cpu_to_be32(stat.stat_name);
+		/* Update imissed. */
+		if (stat_id == RX_NO_BUFFERS_POSTED)
+			rxq->stats.imissed = cpu_to_be64(stat.value);
+	}
+}
+
 static int
 gve_start_queues(struct rte_eth_dev *dev)
 {
@@ -176,6 +243,7 @@ gve_start_queues(struct rte_eth_dev *dev)
 static int
 gve_dev_start(struct rte_eth_dev *dev)
 {
+	struct gve_priv *priv;
 	int ret;
 
 	ret = gve_start_queues(dev);
@@ -187,6 +255,27 @@ gve_dev_start(struct rte_eth_dev *dev)
 	dev->data->dev_started = 1;
 	gve_link_update(dev, 0);
 
+	priv = dev->data->dev_private;
+	/* No stats available yet for Dqo. */
+	if (gve_is_gqi(priv))
+	{
+		ret = gve_alloc_stats_report(priv,
+				dev->data->nb_tx_queues,
+				dev->data->nb_rx_queues);
+		if (ret != 0) {
+			PMD_DRV_LOG(ERR,
+				"Failed to allocate region for stats reporting.");
+			return ret;
+		}
+		ret = gve_adminq_report_stats(priv, priv->stats_report_len,
+				priv->stats_report_mem->iova,
+				GVE_STATS_REPORT_TIMER_PERIOD);
+		if (ret != 0) {
+			PMD_DRV_LOG(ERR, "gve_adminq_report_stats command failed.");
+			return ret;
+		}
+	}
+
 	return 0;
 }
 
@@ -200,6 +289,9 @@ gve_dev_stop(struct rte_eth_dev *dev)
 
 	dev->data->dev_started = 0;
 
+	if (gve_is_gqi(dev->data->dev_private))
+		gve_free_stats_report(dev);
+
 	return 0;
 }
 
@@ -352,6 +444,8 @@ static int
 gve_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
 {
 	uint16_t i;
+	if (gve_is_gqi(dev->data->dev_private))
+		gve_get_imissed_from_nic(dev);
 
 	for (i = 0; i < dev->data->nb_tx_queues; i++) {
 		struct gve_tx_queue *txq = dev->data->tx_queues[i];
@@ -372,6 +466,7 @@ gve_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
 		stats->ibytes += rxq->stats.bytes;
 		stats->ierrors += rxq->stats.errors;
 		stats->rx_nombuf += rxq->stats.no_mbufs;
+		stats->imissed += rxq->stats.imissed;
 	}
 
 	return 0;
@@ -443,6 +538,7 @@ static const struct gve_xstats_name_offset rx_xstats_name_offset[] = {
 	{ "errors",                 RX_QUEUE_STATS_OFFSET(errors) },
 	{ "mbuf_alloc_errors",      RX_QUEUE_STATS_OFFSET(no_mbufs) },
 	{ "mbuf_alloc_errors_bulk", RX_QUEUE_STATS_OFFSET(no_mbufs_bulk) },
+	{ "imissed",                RX_QUEUE_STATS_OFFSET(imissed) },
 };
 
 static int
@@ -614,17 +710,53 @@ gve_teardown_device_resources(struct gve_priv *priv)
 	gve_clear_device_resources_ok(priv);
 }
 
+static uint8_t
+pci_dev_find_capability(struct rte_pci_device *pdev, int cap)
+{
+	uint8_t pos, id;
+	uint16_t ent;
+	int loops;
+	int ret;
+
+	ret = rte_pci_read_config(pdev, &pos, sizeof(pos), PCI_CAPABILITY_LIST);
+	if (ret != sizeof(pos))
+		return 0;
+
+	loops = (PCI_CFG_SPACE_SIZE - PCI_STD_HEADER_SIZEOF) / PCI_CAP_SIZEOF;
+
+	while (pos && loops--) {
+		ret = rte_pci_read_config(pdev, &ent, sizeof(ent), pos);
+		if (ret != sizeof(ent))
+			return 0;
+
+		id = ent & 0xff;
+		if (id == 0xff)
+			break;
+
+		if (id == cap)
+			return pos;
+
+		pos = (ent >> 8);
+	}
+
+	return 0;
+}
+
 static int
 pci_dev_msix_vec_count(struct rte_pci_device *pdev)
 {
-	off_t msix_pos = rte_pci_find_capability(pdev, RTE_PCI_CAP_ID_MSIX);
+	uint8_t msix_cap = pci_dev_find_capability(pdev, PCI_CAP_ID_MSIX);
 	uint16_t control;
+	int ret;
 
-	if (msix_pos > 0 && rte_pci_read_config(pdev, &control, sizeof(control),
-			msix_pos + RTE_PCI_MSIX_FLAGS) == sizeof(control))
-		return (control & RTE_PCI_MSIX_FLAGS_QSIZE) + 1;
+	if (!msix_cap)
+		return 0;
 
-	return 0;
+	ret = rte_pci_read_config(pdev, &control, sizeof(control), msix_cap + PCI_MSIX_FLAGS);
+	if (ret != sizeof(control))
+		return 0;
+
+	return (control & PCI_MSIX_FLAGS_QSIZE) + 1;
 }
 
 static int
diff --git a/drivers/net/gve/gve_ethdev.h b/drivers/net/gve/gve_ethdev.h
index 58d8943e71..0e1bfec871 100644
--- a/drivers/net/gve/gve_ethdev.h
+++ b/drivers/net/gve/gve_ethdev.h
@@ -8,13 +8,25 @@
 #include <ethdev_driver.h>
 #include <ethdev_pci.h>
 #include <rte_ether.h>
-#include <rte_pci.h>
 
 #include "base/gve.h"
 
 /* TODO: this is a workaround to ensure that Tx complq is enough */
 #define DQO_TX_MULTIPLIER 4
 
+/*
+ * Following macros are derived from linux/pci_regs.h, however,
+ * we can't simply include that header here, as there is no such
+ * file for non-Linux platform.
+ */
+#define PCI_CFG_SPACE_SIZE	256
+#define PCI_CAPABILITY_LIST	0x34	/* Offset of first capability list entry */
+#define PCI_STD_HEADER_SIZEOF	64
+#define PCI_CAP_SIZEOF		4
+#define PCI_CAP_ID_MSIX		0x11	/* MSI-X */
+#define PCI_MSIX_FLAGS		2	/* Message Control */
+#define PCI_MSIX_FLAGS_QSIZE	0x07FF	/* Table size */
+
 #define GVE_DEFAULT_RX_FREE_THRESH   64
 #define GVE_DEFAULT_TX_FREE_THRESH   32
 #define GVE_DEFAULT_TX_RS_THRESH     32
@@ -85,6 +97,7 @@ struct gve_rx_stats {
 	uint64_t errors;
 	uint64_t no_mbufs;
 	uint64_t no_mbufs_bulk;
+	uint64_t imissed;
 };
 
 struct gve_xstats_name_offset {
@@ -272,6 +285,11 @@ struct gve_priv {
 
 	struct gve_tx_queue **txqs;
 	struct gve_rx_queue **rxqs;
+
+	uint32_t stats_report_len;
+	const struct rte_memzone *stats_report_mem;
+	uint16_t stats_start_idx; /* start index of array of stats written by NIC */
+	uint16_t stats_end_idx; /* end index of array of stats written by NIC */
 };
 
 static inline bool
-- 
2.43.0.472.g3155946c3a-goog

RE: [PATCH] net/gve: Enable stats reporting for GQ format

[Guo, Junfeng]

> -----Original Message-----
> From: Rushil Gupta <rushilg@google.com>
> Sent: Tuesday, December 19, 2023 10:17
> To: Guo, Junfeng <junfeng.guo@intel.com>; jeroendb@google.com;
> joshwash@google.com; ferruh.yigit@amd.com
> Cc: dev@dpdk.org; Rushil Gupta <rushilg@google.com>
> Subject: [PATCH] net/gve: Enable stats reporting for GQ format
> 
> Read from shared region to retrieve imissed statistics for GQ.
> Tested using `show port xstats <port-id>` in interactive mode.
> This metric can be triggered by using queues > cores.
> 
> Signed-off-by: Rushil Gupta <rushilg@google.com>
> Reviewed-by: Joshua Washington <joshwash@google.com>
> ---
>  drivers/net/gve/base/gve_adminq.h | 11 ++++
>  drivers/net/gve/gve_ethdev.c      | 83
> +++++++++++++++++++++++++++++++
>  drivers/net/gve/gve_ethdev.h      |  6 +++
>  3 files changed, 100 insertions(+)
> 
> diff --git a/drivers/net/gve/base/gve_adminq.h
> b/drivers/net/gve/base/gve_adminq.h
> index e30b184913..f05362f85f 100644
> --- a/drivers/net/gve/base/gve_adminq.h
> +++ b/drivers/net/gve/base/gve_adminq.h
> @@ -314,6 +314,17 @@ struct gve_stats_report {
> 
>  GVE_CHECK_STRUCT_LEN(8, gve_stats_report);
> 
> +/* Numbers of gve tx/rx stats in stats report. */
> +#define GVE_TX_STATS_REPORT_NUM        6
> +#define GVE_RX_STATS_REPORT_NUM        2
> +
> +/* Interval to schedule a stats report update, 20000ms. */
> +#define GVE_STATS_REPORT_TIMER_PERIOD  20000
> +
> +/* Numbers of NIC tx/rx stats in stats report. */
> +#define NIC_TX_STATS_REPORT_NUM        0
> +#define NIC_RX_STATS_REPORT_NUM        4
> +
>  enum gve_stat_names {
>  	/* stats from gve */
>  	TX_WAKE_CNT			= 1,
> diff --git a/drivers/net/gve/gve_ethdev.c b/drivers/net/gve/gve_ethdev.c
> index ecd37ff37f..0db612f25c 100644
> --- a/drivers/net/gve/gve_ethdev.c
> +++ b/drivers/net/gve/gve_ethdev.c
> @@ -125,6 +125,70 @@ gve_link_update(struct rte_eth_dev *dev,
> __rte_unused int wait_to_complete)
>  	return rte_eth_linkstatus_set(dev, &link);
>  }
> 
> +static int gve_alloc_stats_report(struct gve_priv *priv,

Minor observation here and for below newly-added functions about the coding style.

In DPDK, the function type is placed on a new line by itself preceding the function, while in the kernel, it is placed on the same line as the function.
But you can keep this kernel coding style for the code under the base folder of the driver.

It's always good to keep the coding style be consistent within each individual file. : )
https://doc.dpdk.org/guides-23.11/contributing/coding_style.html

Regards,
Junfeng

> +		uint16_t nb_tx_queues, uint16_t nb_rx_queues)
> +{
> +	int tx_stats_cnt;
> +	int rx_stats_cnt;
> +
> +	tx_stats_cnt = (GVE_TX_STATS_REPORT_NUM +
> NIC_TX_STATS_REPORT_NUM) *
> +		nb_tx_queues;
> +	rx_stats_cnt = (GVE_RX_STATS_REPORT_NUM +
> NIC_RX_STATS_REPORT_NUM) *
> +		nb_rx_queues;
> +	priv->stats_report_len = sizeof(struct gve_stats_report) +
> +		sizeof(struct stats) * (tx_stats_cnt + rx_stats_cnt);
> +	priv->stats_report_mem =
> rte_memzone_reserve_aligned("report_stats",
> +			priv->stats_report_len,
> +			rte_socket_id(),
> +			RTE_MEMZONE_IOVA_CONTIG, PAGE_SIZE);
> +
> +	if (!priv->stats_report_mem)
> +		return -ENOMEM;
> +
> +	/* offset by skipping stats written by gve. */
> +	priv->stats_start_idx = (GVE_TX_STATS_REPORT_NUM *
> nb_tx_queues) +
> +		(GVE_RX_STATS_REPORT_NUM * nb_rx_queues);
> +	priv->stats_end_idx = priv->stats_start_idx +
> +		(NIC_TX_STATS_REPORT_NUM * nb_tx_queues) +
> +		(NIC_RX_STATS_REPORT_NUM * nb_rx_queues) - 1;
> +
> +	return 0;
> +}
> +
> +static void gve_free_stats_report(struct rte_eth_dev *dev)
> +{
> +	struct gve_priv *priv = dev->data->dev_private;
> +	rte_memzone_free(priv->stats_report_mem);
> +}
> +
> +/* Read Rx NIC stats from shared region */
> +static void gve_get_imissed_from_nic(struct rte_eth_dev *dev)
> +{
> +	struct gve_stats_report *stats_report;
> +	struct gve_rx_queue *rxq;
> +	struct gve_priv *priv;
> +	struct stats stat;
> +	int queue_id;
> +	int stat_id;
> +	int i;
> +
> +	priv = dev->data->dev_private;
> +	stats_report = (struct gve_stats_report *)
> +		priv->stats_report_mem->addr;
> +
> +	for (i = priv->stats_start_idx; i <= priv->stats_end_idx; i++) {
> +		stat = stats_report->stats[i];
> +		queue_id = cpu_to_be32(stat.queue_id);
> +		rxq = dev->data->rx_queues[queue_id];
> +		if (rxq == NULL)
> +			continue;
> +		stat_id = cpu_to_be32(stat.stat_name);
> +		/* Update imissed. */
> +		if (stat_id == RX_NO_BUFFERS_POSTED)
> +			rxq->stats.imissed = cpu_to_be64(stat.value);
> +	}
> +}
> +
>  static int
>  gve_start_queues(struct rte_eth_dev *dev)
>  {
> @@ -176,6 +240,7 @@ gve_start_queues(struct rte_eth_dev *dev)
>  static int
>  gve_dev_start(struct rte_eth_dev *dev)
>  {
> +	struct gve_priv *priv;
>  	int ret;
> 
>  	ret = gve_start_queues(dev);
> @@ -187,6 +252,17 @@ gve_dev_start(struct rte_eth_dev *dev)
>  	dev->data->dev_started = 1;
>  	gve_link_update(dev, 0);
> 
> +	priv = dev->data->dev_private;
> +	/* No stats available yet for Dqo. */
> +	if (gve_is_gqi(priv)) {
> +		gve_alloc_stats_report(priv,
> +			dev->data->nb_tx_queues,
> +			dev->data->nb_rx_queues);
> +		ret = gve_adminq_report_stats(priv, priv-
> >stats_report_len,
> +				priv->stats_report_mem->iova,
> +				GVE_STATS_REPORT_TIMER_PERIOD);
> +	}
> +
>  	return 0;
>  }
> 
> @@ -200,6 +276,9 @@ gve_dev_stop(struct rte_eth_dev *dev)
> 
>  	dev->data->dev_started = 0;
> 
> +	if (gve_is_gqi(dev->data->dev_private))
> +		gve_free_stats_report(dev);
> +
>  	return 0;
>  }
> 
> @@ -352,6 +431,8 @@ static int
>  gve_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
>  {
>  	uint16_t i;
> +	if (gve_is_gqi(dev->data->dev_private))
> +		gve_get_imissed_from_nic(dev);
> 
>  	for (i = 0; i < dev->data->nb_tx_queues; i++) {
>  		struct gve_tx_queue *txq = dev->data->tx_queues[i];
> @@ -372,6 +453,7 @@ gve_dev_stats_get(struct rte_eth_dev *dev, struct
> rte_eth_stats *stats)
>  		stats->ibytes += rxq->stats.bytes;
>  		stats->ierrors += rxq->stats.errors;
>  		stats->rx_nombuf += rxq->stats.no_mbufs;
> +		stats->imissed += rxq->stats.imissed;
>  	}
> 
>  	return 0;
> @@ -443,6 +525,7 @@ static const struct gve_xstats_name_offset
> rx_xstats_name_offset[] = {
>  	{ "errors",                 RX_QUEUE_STATS_OFFSET(errors) },
>  	{ "mbuf_alloc_errors",      RX_QUEUE_STATS_OFFSET(no_mbufs) },
>  	{ "mbuf_alloc_errors_bulk",
> RX_QUEUE_STATS_OFFSET(no_mbufs_bulk) },
> +	{ "imissed",                RX_QUEUE_STATS_OFFSET(imissed) },
>  };
> 
>  static int
> diff --git a/drivers/net/gve/gve_ethdev.h b/drivers/net/gve/gve_ethdev.h
> index 58d8943e71..9893fcfee6 100644
> --- a/drivers/net/gve/gve_ethdev.h
> +++ b/drivers/net/gve/gve_ethdev.h
> @@ -85,6 +85,7 @@ struct gve_rx_stats {
>  	uint64_t errors;
>  	uint64_t no_mbufs;
>  	uint64_t no_mbufs_bulk;
> +	uint64_t imissed;
>  };
> 
>  struct gve_xstats_name_offset {
> @@ -272,6 +273,11 @@ struct gve_priv {
> 
>  	struct gve_tx_queue **txqs;
>  	struct gve_rx_queue **rxqs;
> +
> +	uint32_t stats_report_len;
> +	const struct rte_memzone *stats_report_mem;
> +	uint16_t stats_start_idx; /* start index of array of stats written by
> NIC */
> +	uint16_t stats_end_idx; /* end index of array of stats written by
> NIC */
>  };
> 
>  static inline bool
> --
> 2.43.0.472.g3155946c3a-goog

[PATCH] net/gve: Enable stats reporting for GQ format

[Rushil Gupta]

Read from shared region to retrieve imissed statistics for GQ.
Tested using `show port xstats <port-id>` in interactive mode.
This metric can be triggered by using queues > cores.

Signed-off-by: Rushil Gupta <rushilg@google.com>
Reviewed-by: Joshua Washington <joshwash@google.com>
---
 drivers/net/gve/base/gve_adminq.h | 11 ++++
 drivers/net/gve/gve_ethdev.c      | 83 +++++++++++++++++++++++++++++++
 drivers/net/gve/gve_ethdev.h      |  6 +++
 3 files changed, 100 insertions(+)

diff --git a/drivers/net/gve/base/gve_adminq.h b/drivers/net/gve/base/gve_adminq.h
index e30b184913..f05362f85f 100644
--- a/drivers/net/gve/base/gve_adminq.h
+++ b/drivers/net/gve/base/gve_adminq.h
@@ -314,6 +314,17 @@ struct gve_stats_report {
 
 GVE_CHECK_STRUCT_LEN(8, gve_stats_report);
 
+/* Numbers of gve tx/rx stats in stats report. */
+#define GVE_TX_STATS_REPORT_NUM        6
+#define GVE_RX_STATS_REPORT_NUM        2
+
+/* Interval to schedule a stats report update, 20000ms. */
+#define GVE_STATS_REPORT_TIMER_PERIOD  20000
+
+/* Numbers of NIC tx/rx stats in stats report. */
+#define NIC_TX_STATS_REPORT_NUM        0
+#define NIC_RX_STATS_REPORT_NUM        4
+
 enum gve_stat_names {
 	/* stats from gve */
 	TX_WAKE_CNT			= 1,
diff --git a/drivers/net/gve/gve_ethdev.c b/drivers/net/gve/gve_ethdev.c
index ecd37ff37f..0db612f25c 100644
--- a/drivers/net/gve/gve_ethdev.c
+++ b/drivers/net/gve/gve_ethdev.c
@@ -125,6 +125,70 @@ gve_link_update(struct rte_eth_dev *dev, __rte_unused int wait_to_complete)
 	return rte_eth_linkstatus_set(dev, &link);
 }
 
+static int gve_alloc_stats_report(struct gve_priv *priv,
+		uint16_t nb_tx_queues, uint16_t nb_rx_queues)
+{
+	int tx_stats_cnt;
+	int rx_stats_cnt;
+
+	tx_stats_cnt = (GVE_TX_STATS_REPORT_NUM + NIC_TX_STATS_REPORT_NUM) *
+		nb_tx_queues;
+	rx_stats_cnt = (GVE_RX_STATS_REPORT_NUM + NIC_RX_STATS_REPORT_NUM) *
+		nb_rx_queues;
+	priv->stats_report_len = sizeof(struct gve_stats_report) +
+		sizeof(struct stats) * (tx_stats_cnt + rx_stats_cnt);
+	priv->stats_report_mem = rte_memzone_reserve_aligned("report_stats",
+			priv->stats_report_len,
+			rte_socket_id(),
+			RTE_MEMZONE_IOVA_CONTIG, PAGE_SIZE);
+
+	if (!priv->stats_report_mem)
+		return -ENOMEM;
+
+	/* offset by skipping stats written by gve. */
+	priv->stats_start_idx = (GVE_TX_STATS_REPORT_NUM * nb_tx_queues) +
+		(GVE_RX_STATS_REPORT_NUM * nb_rx_queues);
+	priv->stats_end_idx = priv->stats_start_idx +
+		(NIC_TX_STATS_REPORT_NUM * nb_tx_queues) +
+		(NIC_RX_STATS_REPORT_NUM * nb_rx_queues) - 1;
+
+	return 0;
+}
+
+static void gve_free_stats_report(struct rte_eth_dev *dev)
+{
+	struct gve_priv *priv = dev->data->dev_private;
+	rte_memzone_free(priv->stats_report_mem);
+}
+
+/* Read Rx NIC stats from shared region */
+static void gve_get_imissed_from_nic(struct rte_eth_dev *dev)
+{
+	struct gve_stats_report *stats_report;
+	struct gve_rx_queue *rxq;
+	struct gve_priv *priv;
+	struct stats stat;
+	int queue_id;
+	int stat_id;
+	int i;
+
+	priv = dev->data->dev_private;
+	stats_report = (struct gve_stats_report *)
+		priv->stats_report_mem->addr;
+
+	for (i = priv->stats_start_idx; i <= priv->stats_end_idx; i++) {
+		stat = stats_report->stats[i];
+		queue_id = cpu_to_be32(stat.queue_id);
+		rxq = dev->data->rx_queues[queue_id];
+		if (rxq == NULL)
+			continue;
+		stat_id = cpu_to_be32(stat.stat_name);
+		/* Update imissed. */
+		if (stat_id == RX_NO_BUFFERS_POSTED)
+			rxq->stats.imissed = cpu_to_be64(stat.value);
+	}
+}
+
 static int
 gve_start_queues(struct rte_eth_dev *dev)
 {
@@ -176,6 +240,7 @@ gve_start_queues(struct rte_eth_dev *dev)
 static int
 gve_dev_start(struct rte_eth_dev *dev)
 {
+	struct gve_priv *priv;
 	int ret;
 
 	ret = gve_start_queues(dev);
@@ -187,6 +252,17 @@ gve_dev_start(struct rte_eth_dev *dev)
 	dev->data->dev_started = 1;
 	gve_link_update(dev, 0);
 
+	priv = dev->data->dev_private;
+	/* No stats available yet for Dqo. */
+	if (gve_is_gqi(priv)) {
+		gve_alloc_stats_report(priv,
+			dev->data->nb_tx_queues,
+			dev->data->nb_rx_queues);
+		ret = gve_adminq_report_stats(priv, priv->stats_report_len,
+				priv->stats_report_mem->iova,
+				GVE_STATS_REPORT_TIMER_PERIOD);
+	}
+
 	return 0;
 }
 
@@ -200,6 +276,9 @@ gve_dev_stop(struct rte_eth_dev *dev)
 
 	dev->data->dev_started = 0;
 
+	if (gve_is_gqi(dev->data->dev_private))
+		gve_free_stats_report(dev);
+
 	return 0;
 }
 
@@ -352,6 +431,8 @@ static int
 gve_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
 {
 	uint16_t i;
+	if (gve_is_gqi(dev->data->dev_private))
+		gve_get_imissed_from_nic(dev);
 
 	for (i = 0; i < dev->data->nb_tx_queues; i++) {
 		struct gve_tx_queue *txq = dev->data->tx_queues[i];
@@ -372,6 +453,7 @@ gve_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
 		stats->ibytes += rxq->stats.bytes;
 		stats->ierrors += rxq->stats.errors;
 		stats->rx_nombuf += rxq->stats.no_mbufs;
+		stats->imissed += rxq->stats.imissed;
 	}
 
 	return 0;
@@ -443,6 +525,7 @@ static const struct gve_xstats_name_offset rx_xstats_name_offset[] = {
 	{ "errors",                 RX_QUEUE_STATS_OFFSET(errors) },
 	{ "mbuf_alloc_errors",      RX_QUEUE_STATS_OFFSET(no_mbufs) },
 	{ "mbuf_alloc_errors_bulk", RX_QUEUE_STATS_OFFSET(no_mbufs_bulk) },
+	{ "imissed",                RX_QUEUE_STATS_OFFSET(imissed) },
 };
 
 static int
diff --git a/drivers/net/gve/gve_ethdev.h b/drivers/net/gve/gve_ethdev.h
index 58d8943e71..9893fcfee6 100644
--- a/drivers/net/gve/gve_ethdev.h
+++ b/drivers/net/gve/gve_ethdev.h
@@ -85,6 +85,7 @@ struct gve_rx_stats {
 	uint64_t errors;
 	uint64_t no_mbufs;
 	uint64_t no_mbufs_bulk;
+	uint64_t imissed;
 };
 
 struct gve_xstats_name_offset {
@@ -272,6 +273,11 @@ struct gve_priv {
 
 	struct gve_tx_queue **txqs;
 	struct gve_rx_queue **rxqs;
+
+	uint32_t stats_report_len;
+	const struct rte_memzone *stats_report_mem;
+	uint16_t stats_start_idx; /* start index of array of stats written by NIC */
+	uint16_t stats_end_idx; /* end index of array of stats written by NIC */
 };
 
 static inline bool
-- 
2.43.0.472.g3155946c3a-goog

[PATCH] net/gve: Enable stats reporting for GQ format

[Rushil Gupta]

Read from shared region to retrieve imissed statistics for GQ.
Tested using `show port xstats <port-id>` in interactive mode.
This metric can be triggered by using queues > cores.

Signed-off-by: Rushil Gupta <rushilg@google.com>
Reviewed-by: Joshua Washington <joshwash@google.com>
---
 drivers/net/gve/base/gve_adminq.h | 11 ++++
 drivers/net/gve/gve_ethdev.c      | 84 +++++++++++++++++++++++++++++++
 drivers/net/gve/gve_ethdev.h      |  6 +++
 3 files changed, 101 insertions(+)

diff --git a/drivers/net/gve/base/gve_adminq.h b/drivers/net/gve/base/gve_adminq.h
index e30b184913..f05362f85f 100644
--- a/drivers/net/gve/base/gve_adminq.h
+++ b/drivers/net/gve/base/gve_adminq.h
@@ -314,6 +314,17 @@ struct gve_stats_report {
 
 GVE_CHECK_STRUCT_LEN(8, gve_stats_report);
 
+/* Numbers of gve tx/rx stats in stats report. */
+#define GVE_TX_STATS_REPORT_NUM        6
+#define GVE_RX_STATS_REPORT_NUM        2
+
+/* Interval to schedule a stats report update, 20000ms. */
+#define GVE_STATS_REPORT_TIMER_PERIOD  20000
+
+/* Numbers of NIC tx/rx stats in stats report. */
+#define NIC_TX_STATS_REPORT_NUM        0
+#define NIC_RX_STATS_REPORT_NUM        4
+
 enum gve_stat_names {
 	/* stats from gve */
 	TX_WAKE_CNT			= 1,
diff --git a/drivers/net/gve/gve_ethdev.c b/drivers/net/gve/gve_ethdev.c
index ecd37ff37f..0829c17fc2 100644
--- a/drivers/net/gve/gve_ethdev.c
+++ b/drivers/net/gve/gve_ethdev.c
@@ -125,6 +125,70 @@ gve_link_update(struct rte_eth_dev *dev, __rte_unused int wait_to_complete)
 	return rte_eth_linkstatus_set(dev, &link);
 }
 
+static int gve_alloc_stats_report(struct gve_priv *priv,
+		uint16_t nb_tx_queues, uint16_t nb_rx_queues)
+{
+	int tx_stats_cnt;
+	int rx_stats_cnt;
+
+	tx_stats_cnt = (GVE_TX_STATS_REPORT_NUM + NIC_TX_STATS_REPORT_NUM) *
+		nb_tx_queues;
+	rx_stats_cnt = (GVE_RX_STATS_REPORT_NUM + NIC_RX_STATS_REPORT_NUM) *
+		nb_rx_queues;
+	priv->stats_report_len = sizeof(struct gve_stats_report) +
+		sizeof(struct stats) * (tx_stats_cnt + rx_stats_cnt);
+	priv->stats_report_mem = rte_memzone_reserve_aligned("report_stats",
+			priv->stats_report_len,
+			rte_socket_id(),
+			RTE_MEMZONE_IOVA_CONTIG, PAGE_SIZE);
+
+	if (!priv->stats_report_mem)
+		return -ENOMEM;
+
+	/* offset by skipping stats written by gve. */
+	priv->stats_start_idx = (GVE_TX_STATS_REPORT_NUM * nb_tx_queues) +
+		(GVE_RX_STATS_REPORT_NUM * nb_rx_queues);
+	priv->stats_end_idx = priv->stats_start_idx +
+		(NIC_TX_STATS_REPORT_NUM * nb_tx_queues) +
+		(NIC_RX_STATS_REPORT_NUM * nb_rx_queues) - 1;
+
+	return 0;
+}
+
+static void gve_free_stats_report(struct rte_eth_dev *dev)
+{
+	struct gve_priv *priv = dev->data->dev_private;
+        rte_memzone_free(priv->stats_report_mem);
+}
+
+/* Read Rx NIC stats from shared region */
+static void gve_get_imissed_from_nic(struct rte_eth_dev *dev)
+{
+	struct gve_stats_report *stats_report;
+	struct gve_rx_queue *rxq;
+	struct gve_priv *priv;
+	struct stats stat;
+	int queue_id;
+	int stat_id;
+	int i;
+
+	priv = dev->data->dev_private;
+	stats_report = (struct gve_stats_report *)
+		priv->stats_report_mem->addr;
+
+	for (i = priv->stats_start_idx; i <= priv->stats_end_idx; i++) {
+		stat = stats_report->stats[i];
+		queue_id = cpu_to_be32(stat.queue_id);
+		rxq = dev->data->rx_queues[queue_id];
+		if (rxq == NULL)
+			continue;
+		stat_id = cpu_to_be32(stat.stat_name);
+		/* Update imissed. */
+		if (stat_id == RX_NO_BUFFERS_POSTED)
+			rxq->stats.imissed = cpu_to_be64(stat.value);
+	}
+}
+
 static int
 gve_start_queues(struct rte_eth_dev *dev)
 {
@@ -176,6 +240,7 @@ gve_start_queues(struct rte_eth_dev *dev)
 static int
 gve_dev_start(struct rte_eth_dev *dev)
 {
+	struct gve_priv *priv;
 	int ret;
 
 	ret = gve_start_queues(dev);
@@ -187,6 +252,18 @@ gve_dev_start(struct rte_eth_dev *dev)
 	dev->data->dev_started = 1;
 	gve_link_update(dev, 0);
 
+	priv = dev->data->dev_private;
+	/* No stats available yet for Dqo. */
+	if (gve_is_gqi(priv))
+	{
+		gve_alloc_stats_report(priv,
+			dev->data->nb_tx_queues,
+			dev->data->nb_rx_queues);
+		ret = gve_adminq_report_stats(priv, priv->stats_report_len,
+				priv->stats_report_mem->iova,
+				GVE_STATS_REPORT_TIMER_PERIOD);
+	}
+
 	return 0;
 }
 
@@ -200,6 +277,9 @@ gve_dev_stop(struct rte_eth_dev *dev)
 
 	dev->data->dev_started = 0;
 
+	if (gve_is_gqi(dev->data->dev_private))
+		gve_free_stats_report(dev);
+
 	return 0;
 }
 
@@ -352,6 +432,8 @@ static int
 gve_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
 {
 	uint16_t i;
+	if (gve_is_gqi(dev->data->dev_private))
+		gve_get_imissed_from_nic(dev);
 
 	for (i = 0; i < dev->data->nb_tx_queues; i++) {
 		struct gve_tx_queue *txq = dev->data->tx_queues[i];
@@ -372,6 +454,7 @@ gve_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
 		stats->ibytes += rxq->stats.bytes;
 		stats->ierrors += rxq->stats.errors;
 		stats->rx_nombuf += rxq->stats.no_mbufs;
+		stats->imissed += rxq->stats.imissed;
 	}
 
 	return 0;
@@ -443,6 +526,7 @@ static const struct gve_xstats_name_offset rx_xstats_name_offset[] = {
 	{ "errors",                 RX_QUEUE_STATS_OFFSET(errors) },
 	{ "mbuf_alloc_errors",      RX_QUEUE_STATS_OFFSET(no_mbufs) },
 	{ "mbuf_alloc_errors_bulk", RX_QUEUE_STATS_OFFSET(no_mbufs_bulk) },
+	{ "imissed",                RX_QUEUE_STATS_OFFSET(imissed) },
 };
 
 static int
diff --git a/drivers/net/gve/gve_ethdev.h b/drivers/net/gve/gve_ethdev.h
index 58d8943e71..9893fcfee6 100644
--- a/drivers/net/gve/gve_ethdev.h
+++ b/drivers/net/gve/gve_ethdev.h
@@ -85,6 +85,7 @@ struct gve_rx_stats {
 	uint64_t errors;
 	uint64_t no_mbufs;
 	uint64_t no_mbufs_bulk;
+	uint64_t imissed;
 };
 
 struct gve_xstats_name_offset {
@@ -272,6 +273,11 @@ struct gve_priv {
 
 	struct gve_tx_queue **txqs;
 	struct gve_rx_queue **rxqs;
+
+	uint32_t stats_report_len;
+	const struct rte_memzone *stats_report_mem;
+	uint16_t stats_start_idx; /* start index of array of stats written by NIC */
+	uint16_t stats_end_idx; /* end index of array of stats written by NIC */
 };
 
 static inline bool
-- 
2.43.0.472.g3155946c3a-goog