Re: [PATCH v3 2/2] net/gve: add Rx/Tx queue stats as extended stats

[Levend Sayar <levendsayar@gmail.com>]

Thanks Ferruh for the review.
My comments are inlined.

> On 21 Feb 2023, at 01:57, Ferruh Yigit <ferruh.yigit@amd.com> wrote:
> 
> On 2/20/2023 9:11 PM, Levend Sayar wrote:
>> Google Virtual NIC rx/tx queue stats are added as extended stats.
>> 
>> Signed-off-by: Levend Sayar <levendsayar@gmail.com>
> 
> Thank you for the update, mainly looks good to me, there area a few
> minor questions/comments below.
> 
> <...>
>> +static const struct gve_xstats_name_offset tx_xstats_name_offset[] = {
>> +	{ "packets", offsetof(struct gve_tx_stats, packets) },
>> +	{ "bytes",   offsetof(struct gve_tx_stats, bytes) },
>> +	{ "errors",  offsetof(struct gve_tx_stats, errors) },
>> +};
>> +
> 
> It is possible to create macros to get offsets to prevent any mistakes
> but not quite sure if it is needed with above limited number of items,
> so up to you, I mean something like:
> 
> #define RX_QUEUE_STATS_OFFSET(x) offsetof(struct gve_rx_stats, X)
> #define TX_QUEUE_STATS_OFFSET(x) offsetof(struct gve_tx_stats, X)
> 

LS: I take dpdk code as reference and mimicked the usage on rte_ethdev.c here. 
      Your proposal surely applicable.

>> +static const struct gve_xstats_name_offset rx_xstats_name_offset[] = {
>> +	{ "packets", offsetof(struct gve_rx_stats, packets) },
>> +	{ "bytes",   offsetof(struct gve_rx_stats, bytes) },
>> +	{ "errors",  offsetof(struct gve_rx_stats, errors) },
>> +};
>> +
> 
> Is 'no_mbufs' omitted intentionally?
> 
> <...>

LS: no_mbufs are accumulated as rx_mbuf_allocation_errors on basic stats. But yes, it can be queue based also. 

>> 
>> +static int
>> +gve_xstats_count(struct rte_eth_dev *dev)
>> +{
>> +	uint16_t i, count = 0;
>> +
>> +	for (i = 0; i < dev->data->nb_tx_queues; i++) {
>> +		if (dev->data->tx_queues[i])
> 
> Normally 'dev->data->tx_queues[i]' shouldn't be NULL, but I can see
> driver checks this in a few locations.
> 
> Is there a case that 'dev->data->tx_queues[i]' can be NULL where "0 <= i
> < dev->data->nb_tx_queues" ?

LS: You’re right. On my previous patches I erased that checks but that parts are reviewed as noise.
      So I am aligned with the rest of the code. In fact, there is no gap like that. In dev_start,
      Queues are created and whenever queue can not be created, it returns error at that point.
      So dev_start will return an error at the end. So if device successfully started, all rx/tx queues must be created
      Successfully. Therefore no need to check. 

> 
>> +			count += RTE_DIM(tx_xstats_name_offset);
>> +	}
>> +
>> +	for (i = 0; i < dev->data->nb_rx_queues; i++) {
>> +		if (dev->data->rx_queues[i])
>> +			count += RTE_DIM(rx_xstats_name_offset);
>> +	}
>> +
>> +	return count;
>> +}
>> +
>> +static int
>> +gve_xstats_get(struct rte_eth_dev *dev,
>> +			struct rte_eth_xstat *xstats,
>> +			unsigned int size)
>> +{
>> +	uint16_t i, count = 0;
>> +
>> +	if (!xstats)
>> +		return (size == 0) ? gve_xstats_count(dev) : -EINVAL;
> 
> Error case (xstats == NULL && size != 0) should be handled by ethdev, so
> driver can only check "xstats == NULL" case.
> 
> btw, although we have mixed usage and not very strict on it, coding
> convention requires testing against NULL [1] instead of '!'.
> 
> [1]
> https://doc.dpdk.org/guides/contributing/coding_style.html#null-pointers

LS: You’re right. rte_eth_xstats_get checks for (xstats == NULL && size != 0).
      Let me correct that part and use NULL.   

> 
>> +
>> +	for (i = 0; i < dev->data->nb_tx_queues; i++) {
>> +		const struct gve_tx_queue *txq = dev->data->tx_queues[i];
>> +		if (!txq)
>> +			continue;
>> +
> 
> similar to above comment, I expect 'txq' not to be NULL, isn't this
> correct for the driver?
> 
>> +		if (count >= size)
>> +			break;
>> +
> 
> Instead of above check in a loop, it is possible to check once at the
> beginning of the function like
> 
> count = gve_xstats_count(dev)
> if (size < count)
> 	return count;
> 
> Because when there is not enough room in the xstats array, API doesn't
> expect existing elements of array to be correct, returning a value
> bigger than 'size' indicates error case and content of xstats is invalid
> anyway.

LS: Let’s say NIC has 20 xstats. Your application allocates xstats memory enough to hold 30
and request 30 xtstats. NIC will return 20 xstats. That’s ok. But if application allocates memory
For 10 xstats and request 10, it’s taken as error although Nic can fill first 10 xstats.
So size must be higher or equal to number of xstats. Right?   

> 
>> +		uint16_t j = 0;
>> +		const char *stats = (const char *)&txq->stats;
> 
> Can you please move variable declarations at the beginning of the scope,
> for above variables it is just after for statement, according coding
> convention.

LS: Sure I can do. I supposed minimizing scope is better approach.
But not here I guess. What is the rationale behind not declaring a variable in a for loop like?
for (int i =0 ...
That’s a quite surprise for me when I got an error from checkpatches.sh. 

> 
>> +		for (j = 0; j < RTE_DIM(tx_xstats_name_offset); j++, count++) {
>> +			xstats[count].id = count;
>> +			xstats[count].value = *(const uint64_t *)
>> +				(stats + tx_xstats_name_offset[j].offset);
>> +		}
>> +	}
>> +
>> +	for (i = 0; i < dev->data->nb_rx_queues; i++) {
>> +		const struct gve_rx_queue *rxq = dev->data->rx_queues[i];
>> +		if (!rxq)
>> +			continue;
>> +
>> +		if (count >= size)
>> +			break;
>> +
>> +		uint16_t j = 0;
>> +		const char *stats = (const char *)&rxq->stats;
>> +		for (j = 0; j < RTE_DIM(rx_xstats_name_offset); j++, count++) {
>> +			xstats[count].id = count;
>> +			xstats[count].value = *(const uint64_t *)
>> +				(stats + rx_xstats_name_offset[j].offset);
>> +		}
>> +	}
>> +
>> +	return count;
>> +}
>> +
>> +static int
>> +gve_xstats_get_names(struct rte_eth_dev *dev,
>> +			struct rte_eth_xstat_name *xstats_names,
>> +			unsigned int size)
>> +{
>> +	uint16_t i, count = 0;
>> +
>> +	if (!xstats_names)
>> +		return (size == 0) ? gve_xstats_count(dev) : -EINVAL;
>> +
>> +	for (i = 0; i < dev->data->nb_tx_queues; i++) {
>> +		if (!dev->data->tx_queues[i])
>> +			continue;
>> +
>> +		if (count >= size)
>> +			break;
>> +
>> +		uint16_t j = 0;
>> +		for (; j < RTE_DIM(tx_xstats_name_offset); j++)
>> +			snprintf(xstats_names[count++].name,
>> +				 RTE_ETH_XSTATS_NAME_SIZE,
>> +				 "tx_q%u_%s", i, tx_xstats_name_offset[j].name);
>> +	}
>> +
>> +	for (i = 0; i < dev->data->nb_rx_queues; i++) {
>> +		if (!dev->data->rx_queues[i])
>> +			continue;
>> +
>> +		if (count >= size)
>> +			break;
>> +
>> +		uint16_t j = 0;
>> +		for (; j < RTE_DIM(rx_xstats_name_offset); j++)
>> +			snprintf(xstats_names[count++].name,
>> +				 RTE_ETH_XSTATS_NAME_SIZE,
>> +				 "rx_q%u_%s", i, rx_xstats_name_offset[j].name);
>> +	}
>> +
>> +	return count;
>> +}
>> +
> 
> comments on 'gve_xstats_get()' valid here too.
>