On Fri, Jan 12, 2024 at 8:36 PM Ferruh Yigit 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 ` 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.