net/gve: Enable stats reporting for GQ format
Checks
Commit Message
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(+)
Comments
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.
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.
>
>
>> <...>
>>
>> > 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.
>
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.
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.
>
>
@@ -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,
@@ -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
@@ -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