Re: [PATCH] net/gve: add support for basic stats

[Stephen Hemminger <stephen@networkplumber.org>]

On Wed, 7 Dec 2022 15:09:08 +0000
Ferruh Yigit <ferruh.yigit@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.li@intel.com>
> > Signed-off-by: Junfeng Guo <junfeng.guo@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.