<div dir="ltr"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="gmail-im" style="color:rgb(80,0,80)"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br><...><br><br>>  gve_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)<br>>  {<br>>       uint16_t i;<br>> +     if (gve_is_gqi(dev->data->dev_private))<br>> +             gve_get_imissed_from_nic(dev);<br>> <br><br>This updates imissed in RxQ struct for all queues for basic stats, but<br>what if user only calls xstats, I guess in that case stat won't be updated.<br></blockquote><div> </div></span><div>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.</div></blockquote><div><br></div><div>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.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Jan 15, 2024 at 10:18 PM Rushil Gupta <<a href="mailto:rushilg@google.com">rushilg@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Jan 12, 2024 at 8:36 PM Ferruh Yigit <<a href="mailto:ferruh.yigit@amd.com" target="_blank">ferruh.yigit@amd.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On 12/22/2023 3:39 PM, Rushil Gupta wrote:<br>
> Read from shared region to retrieve imissed statistics for GQ from device.<br>
> Tested using `show port xstats <port-id>` in interactive mode.<br>
> This metric can be triggered by using queues > cores.<br>
> <br>
<br>
Looks good but please check following comments:<br>
<br>
Checkpatch gives warning on the patch title, and this patch adds<br>
'imissed' support so it can be added to the patch title, something like:<br>
"net/gve: enable imissed stats for GQ format"<br>
<br>
<...><br>
<br>
> +static int gve_alloc_stats_report(struct gve_priv *priv,<br>
> +             uint16_t nb_tx_queues, uint16_t nb_rx_queues)<br>
> +{<br>
> +     char z_name[RTE_MEMZONE_NAMESIZE];<br>
> +     int tx_stats_cnt;<br>
> +     int rx_stats_cnt;<br>
> +<br>
> +     tx_stats_cnt = (GVE_TX_STATS_REPORT_NUM + NIC_TX_STATS_REPORT_NUM) *<br>
> +             nb_tx_queues;<br>
> +     rx_stats_cnt = (GVE_RX_STATS_REPORT_NUM + NIC_RX_STATS_REPORT_NUM) *<br>
> +             nb_rx_queues;<br>
> +     priv->stats_report_len = sizeof(struct gve_stats_report) +<br>
> +             sizeof(struct stats) * (tx_stats_cnt + rx_stats_cnt);<br>
> +<br>
> +     snprintf(z_name, sizeof(z_name), "stats_report_%s", priv->pci_dev-><a href="http://device.name" rel="noreferrer" target="_blank">device.name</a>);<br>
><br>
<br>
Can you please add 'gve_' prefix to the memzone name, to prevent any<br>
possible collision.<br></blockquote><div>Done. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
<...><br>
<br>
> +static void gve_free_stats_report(struct rte_eth_dev *dev)<br>
> +{<br>
> +     struct gve_priv *priv = dev->data->dev_private;<br>
> +     rte_memzone_free(priv->stats_report_mem);<br>
><br>
<br>
What will happen if user asks stats/xstats after port stopped?<br></blockquote><div>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. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
<...><br>
<br>
>  gve_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)<br>
>  {<br>
>       uint16_t i;<br>
> +     if (gve_is_gqi(dev->data->dev_private))<br>
> +             gve_get_imissed_from_nic(dev);<br>
>  <br>
<br>
This updates imissed in RxQ struct for all queues for basic stats, but<br>
what if user only calls xstats, I guess in that case stat won't be updated.<br></blockquote><div> </div><div>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.</div></div></div>
</blockquote></div><br clear="all"><div><br></div><span class="gmail_signature_prefix">-- </span><br><div dir="ltr" class="gmail_signature"><div dir="ltr"><br><div style="line-height:1.5em;padding-top:10px;margin-top:10px;color:rgb(85,85,85);font-family:sans-serif"><span style="border-width:2px 0px 0px;border-style:solid;border-color:rgb(213,15,37);padding-top:2px;margin-top:2px">Joshua Washington |</span><span style="border-width:2px 0px 0px;border-style:solid;border-color:rgb(51,105,232);padding-top:2px;margin-top:2px"> Software Engineer |</span><span style="border-width:2px 0px 0px;border-style:solid;border-color:rgb(0,153,57);padding-top:2px;margin-top:2px"> <a href="mailto:joshwash@google.com" target="_blank">joshwash@google.com</a> |</span><span style="border-width:2px 0px 0px;border-style:solid;border-color:rgb(238,178,17);padding-top:2px;margin-top:2px"> (414) 366-4423</span></div><span style="color:rgb(0,0,0);font-family:Tinos;font-size:medium"> </span><br></div></div>