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

[Ferruh Yigit <ferruh.yigit@amd.com>]

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