<div dir="ltr">Those are fair points. I'll fix this by simply calling gve_get_imissed_from_nic from gve_xstats_get in the v3 patch.</div> <div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Jan 17, 2024 at 3:10 PM Ferruh Yigit <<a href="mailto:ferruh.yigit@amd.com">ferruh.yigit@amd.com</a>> wrote: </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On 1/16/2024 6:18 AM, Rushil Gupta wrote: > > > 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> > <mailto:<a href="mailto:ferruh.yigit@amd.com" target="_blank">ferruh.yigit@amd.com</a>>> 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-><a href="http://device.name" rel="noreferrer" target="_blank">device.name</a> <<a href="http://device.name" rel="noreferrer" target="_blank">http://device.name</a>>); > > > > 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. </blockquote></div>