[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.