On Wed, 7 Dec 2022 15:09:08 +0000 Ferruh Yigit <ferruh.yi...@amd.com> wrote:
> On 11/24/2022 7:33 AM, Junfeng Guo wrote: > > Add support for dev_ops of stats_get and stats_reset. > > > > Queue stats update will be moved into xstat [1], but the basic stats > > items may still be required. So we just keep the remaining ones and > > will implement the queue stats via xstats in the coming release. > > > > [1] > > https://elixir.bootlin.com/dpdk/v22.07/ \ > > source/doc/guides/rel_notes/deprecation.rst#L118 > > > > Signed-off-by: Xiaoyun Li <xiaoyun...@intel.com> > > Signed-off-by: Junfeng Guo <junfeng....@intel.com> > > <...> > > > +static int > > +gve_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats) > > +{ > > + uint16_t i; > > + > > + for (i = 0; i < dev->data->nb_tx_queues; i++) { > > + struct gve_tx_queue *txq = dev->data->tx_queues[i]; > > + if (txq == NULL) > > + continue; > > + > > + stats->opackets += txq->packets; > > + stats->obytes += txq->bytes; > > + stats->oerrors += txq->errors; > > Hi Junfeng, Qi, Jingjing, Beilei, > > Above logic looks wrong to me, did you test it? > > If the 'gve_dev_stats_get()' called multiple times (without stats reset > in between), same values will be keep added to stats. > Some hw based implementations does this, because reading from stats > registers automatically reset those registers but this shouldn't be case > for this driver. > > I expect it to be something like: > > local_stats = 0 > foreach queue > local_stats += queue->stats > stats = local_stats The zero of local_stats is unnecessary. The only caller of the PMD stats_get is rte_ethdev_stats_get and it zeros the stats structure before calling the PMD. int rte_eth_stats_get(uint16_t port_id, struct rte_eth_stats *stats) { struct rte_eth_dev *dev; RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV); dev = &rte_eth_devices[port_id]; memset(stats, 0, sizeof(*stats)); ... stats->rx_nombuf = dev->data->rx_mbuf_alloc_failed; return eth_err(port_id, (*dev->dev_ops->stats_get)(dev, stats)); If any PMD has extra memset in their stats get that could be removed.
