On 1/16/2024 6:18 AM, Rushil Gupta wrote: > > > On Fri, Jan 12, 2024 at 8:36 PM Ferruh Yigit <ferruh.yi...@amd.com > <mailto:ferruh.yi...@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.
