Re: [PATCH 2/2] net/gve: add extended statistics

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

On 2/16/2023 6:58 PM, Levend Sayar wrote:
> Google Virtual NIC PMD is enriched with extended statistics info.

Only queue stats added to xstats, can you please highlight this in the
commit log?

> eth_dev_ops callback names are also synched with eth_dev_ops field names
> 

Renaming eth_dev_ops is not related to xstats, and I am not sure if the
change is necessary, can you please drop it from this patch?

> Signed-off-by: Levend Sayar <levendsayar@gmail.com>
> ---
>  drivers/net/gve/gve_ethdev.c | 152 ++++++++++++++++++++++++++++++-----
>  drivers/net/gve/gve_rx.c     |   8 +-
>  2 files changed, 138 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/net/gve/gve_ethdev.c b/drivers/net/gve/gve_ethdev.c
> index fef2458a16..e31fdce960 100644
> --- a/drivers/net/gve/gve_ethdev.c
> +++ b/drivers/net/gve/gve_ethdev.c
> @@ -266,7 +266,7 @@ gve_dev_close(struct rte_eth_dev *dev)
>  }
>  
>  static int
> -gve_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
> +gve_dev_infos_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
>  {
>  	struct gve_priv *priv = dev->data->dev_private;
>  
> @@ -319,15 +319,12 @@ gve_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
>  }
>  
>  static int
> -gve_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
> +gve_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;
> -

I assume check is removed because it is unnecessary, but again not
directly related with the patch, can you also drop these from the patch
to reduce the noise.

>  		stats->opackets += txq->packets;
>  		stats->obytes += txq->bytes;
>  		stats->oerrors += txq->errors;
> @@ -335,9 +332,6 @@ gve_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
>  
>  	for (i = 0; i < dev->data->nb_rx_queues; i++) {
>  		struct gve_rx_queue *rxq = dev->data->rx_queues[i];
> -		if (rxq == NULL)
> -			continue;
> -
>  		stats->ipackets += rxq->packets;
>  		stats->ibytes += rxq->bytes;
>  		stats->ierrors += rxq->errors;
> @@ -348,15 +342,12 @@ gve_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
>  }
>  
>  static int
> -gve_dev_stats_reset(struct rte_eth_dev *dev)
> +gve_stats_reset(struct rte_eth_dev *dev)
>  {
>  	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;
> -
>  		txq->packets  = 0;
>  		txq->bytes = 0;
>  		txq->errors = 0;
> @@ -364,9 +355,6 @@ gve_dev_stats_reset(struct rte_eth_dev *dev)
>  
>  	for (i = 0; i < dev->data->nb_rx_queues; i++) {
>  		struct gve_rx_queue *rxq = dev->data->rx_queues[i];
> -		if (rxq == NULL)
> -			continue;
> -
>  		rxq->packets  = 0;
>  		rxq->bytes = 0;
>  		rxq->errors = 0;
> @@ -377,7 +365,7 @@ gve_dev_stats_reset(struct rte_eth_dev *dev)
>  }
>  
>  static int
> -gve_dev_mtu_set(struct rte_eth_dev *dev, uint16_t mtu)
> +gve_mtu_set(struct rte_eth_dev *dev, uint16_t mtu)
>  {
>  	struct gve_priv *priv = dev->data->dev_private;
>  	int err;
> @@ -403,20 +391,144 @@ gve_dev_mtu_set(struct rte_eth_dev *dev, uint16_t mtu)
>  	return 0;
>  }
>  
> +static int
> +gve_xstats_get(struct rte_eth_dev *dev, struct rte_eth_xstat *xstats, unsigned int n)
> +{
> +	if (xstats) {

To reduce indentation (and increase readability) you can prefer:
``
if !xstats
	return count;

<put rest of logic here>
``

> +		uint requested = n;

uint? C#? Please prefer standard "unsigned int" type.

> +		uint64_t indx = 0;
> +		struct rte_eth_xstat *xstat = xstats;
> +		uint16_t i;
> +
> +		for (i = 0; i < dev->data->nb_rx_queues; i++) {
> +			struct gve_rx_queue *rxq = dev->data->rx_queues[i];
> +			xstat->id = indx++;
> +			xstat->value = rxq->packets;
> +			if (--requested == 0)
> +				return n;

And in this case, ethdev layer does the checks and return accordingly,
so need to try to fill the stats as much as possible, can you please
double check the ethdev API?

> +			xstat++;
> +
> +			xstat->id = indx++;
> +			xstat->value = rxq->bytes;
> +			if (--requested == 0)
> +				return n;
> +			xstat++;
> +
> +			xstat->id = indx++;
> +			xstat->value = rxq->errors;
> +			if (--requested == 0)
> +				return n;
> +			xstat++;
> +
> +			xstat->id = indx++;
> +			xstat->value = rxq->no_mbufs;
> +			if (--requested == 0)
> +				return n;
> +			xstat++;
> +		}
> +
> +		for (i = 0; i < dev->data->nb_tx_queues; i++) {
> +			struct gve_tx_queue *txq = dev->data->tx_queues[i];
> +			xstat->id = indx++;
> +			xstat->value = txq->packets;
> +			if (--requested == 0)
> +				return n;
> +			xstat++;
> +
> +			xstat->id = indx++;
> +			xstat->value = txq->bytes;
> +			if (--requested == 0)
> +				return n;
> +			xstat++;
> +
> +			xstat->id = indx++;
> +			xstat->value = txq->errors;
> +			if (--requested == 0)
> +				return n;
> +			xstat++;
> +		}


This approach is error to prone and close to extension,
many inplementations prefer to have an itnernal struct to link between
names and values, something like:
struct name_offset {
	char name[RTE_ETH_XSTATS_NAME_SIZE];
	uint32_t offset;
};

'name' holds the xstat name, for this patch it will be only repeating
part of name like 'packets', 'bytes', etc.. and you need to construct
the full name on the fly, that is why it you may prefer to add type to
above struct to diffrenciate Rx and Tx and use it for name creation, up
to you.


'offset' holds the offset of corresponding value in a struct, for you it
is "struct gve_rx_queue" & "struct gve_tx_queue", since there are two
different structs it helps to create helper macro that gets offset from
correct struct.

struct name_offset rx_name_offset[] = {
	{ "packets", offsetof(struct gve_rx_queue, packets) },
        ....
}


for (i = 0; i < dev->data->nb_rx_queues; i++) {
	struct gve_rx_queue *rxq = dev->data->rx_queues[i];
	addr = (char *)rxq + rx_name_offset[i].offset;
	xstats[index++].value = *addr;
}
for (i = 0; i < dev->data->nb_tx_queues; i++) {
	struct gve_tx_queue *txq = dev->data->tx_queues[i];
	addr = (char *)txq + tx_name_offset[i].offset;
	xstats[index++].value = *addr;
}

There are many sample of this in other drivers.

And since for this case instead of having fixed numbe of names, there
are dynamiccaly changing queue names,


> +	}
> +
> +	return (dev->data->nb_rx_queues * 4) + (dev->data->nb_tx_queues * 3);

When above suggested 'name_offset' struct used, you can use size of it
instead of hardcoded 4 & 3 values.

With above sample, it becomes:

return (dev->data->nb_rx_queues * RTE_DIM(rx_name_offset)) +
	(dev->data->nb_tx_queues * RTE_DIM(tx_name_offset));


> +}
> +
> +static int
> +gve_xstats_reset(struct rte_eth_dev *dev)
> +{
> +	return gve_stats_reset(dev);
> +}
> +
> +static int
> +gve_xstats_get_names(struct rte_eth_dev *dev, struct rte_eth_xstat_name *xstats_names,
> +						unsigned int n)
> +{
> +	if (xstats_names) {
> +		uint requested = n;
> +		struct rte_eth_xstat_name *xstats_name = xstats_names;
> +		uint16_t i;
> +
> +		for (i = 0; i < dev->data->nb_rx_queues; i++) {
> +			snprintf(xstats_name->name, sizeof(xstats_name->name),
> +					"rx_q%d_packets", i);
> +			if (--requested == 0)
> +				return n;
> +			xstats_name++;
> +			snprintf(xstats_name->name, sizeof(xstats_name->name),
> +					"rx_q%d_bytes", i);
> +			if (--requested == 0)
> +				return n;
> +			xstats_name++;
> +			snprintf(xstats_name->name, sizeof(xstats_name->name),
> +					"rx_q%d_errors", i);
> +			if (--requested == 0)
> +				return n;
> +			xstats_name++;
> +			snprintf(xstats_name->name, sizeof(xstats_name->name),
> +					"rx_q%d_no_mbufs", i);
> +			if (--requested == 0)
> +				return n;
> +			xstats_name++;
> +		}
> +

And again according above samples this becomes:

for (i = 0; i < dev->data->nb_rx_queues; i++) {
    for (j = 0; j < RTE_DIM(rx_name_offset); j++) {
	snprintf(xstats_names[index++].name, sizeof(), "rx_q%d_%s",
		i, rx_name_offset[j].name);
}


> +		for (i = 0; i < dev->data->nb_tx_queues; i++) {
> +			snprintf(xstats_name->name, sizeof(xstats_name->name),
> +					"tx_q%d_packets", i);
> +			if (--requested == 0)
> +				return n;
> +			xstats_name++;
> +			snprintf(xstats_name->name, sizeof(xstats_name->name),
> +					"tx_q%d_bytes", i);
> +			if (--requested == 0)
> +				return n;
> +			xstats_name++;
> +			snprintf(xstats_name->name, sizeof(xstats_name->name),
> +					"tx_q%d_errors", i);
> +			if (--requested == 0)
> +				return n;
> +			xstats_name++;
> +		}
> +	}
> +
> +	return (dev->data->nb_rx_queues * 4) + (dev->data->nb_tx_queues * 3);
> +}
> +
>  static const struct eth_dev_ops gve_eth_dev_ops = {
>  	.dev_configure        = gve_dev_configure,
>  	.dev_start            = gve_dev_start,
>  	.dev_stop             = gve_dev_stop,
>  	.dev_close            = gve_dev_close,
> -	.dev_infos_get        = gve_dev_info_get,
> +	.dev_infos_get        = gve_dev_infos_get,
>  	.rx_queue_setup       = gve_rx_queue_setup,
>  	.tx_queue_setup       = gve_tx_queue_setup,
>  	.rx_queue_release     = gve_rx_queue_release,
>  	.tx_queue_release     = gve_tx_queue_release,
>  	.link_update          = gve_link_update,
> -	.stats_get            = gve_dev_stats_get,
> -	.stats_reset          = gve_dev_stats_reset,
> -	.mtu_set              = gve_dev_mtu_set,
> +	.stats_get            = gve_stats_get,
> +	.stats_reset          = gve_stats_reset,
> +	.mtu_set              = gve_mtu_set,
> +	.xstats_get           = gve_xstats_get,
> +	.xstats_reset         = gve_xstats_reset,
> +	.xstats_get_names     = gve_xstats_get_names,
>  };
>  
>  static void
> diff --git a/drivers/net/gve/gve_rx.c b/drivers/net/gve/gve_rx.c
> index 66fbcf3930..7687977003 100644
> --- a/drivers/net/gve/gve_rx.c
> +++ b/drivers/net/gve/gve_rx.c
> @@ -22,8 +22,10 @@ gve_rx_refill(struct gve_rx_queue *rxq)
>  		if (diag < 0) {
>  			for (i = 0; i < nb_alloc; i++) {
>  				nmb = rte_pktmbuf_alloc(rxq->mpool);
> -				if (!nmb)
> +				if (!nmb) {
> +					rxq->no_mbufs++;

Why this is needed, original code is as following:

``
for (i = 0; i < nb_alloc; i++) {
	nmb = rte_pktmbuf_alloc(rxq->mpool);
	if (!nmb)
		break;
	rxq->sw_ring[idx + i] = nmb;
}
if (i != nb_alloc) {
	rxq->no_mbufs += nb_alloc - i;
	nb_alloc = i;
}
``

"if (i != nb_alloc)" block already takes care of 'rxq->no_mbufs' value,
is an additional increment required in the for loop?


And change is unrelated with the patch anyway.

>  					break;
> +				}
>  				rxq->sw_ring[idx + i] = nmb;
>  			}
>  			if (i != nb_alloc) {
> @@ -57,8 +59,10 @@ gve_rx_refill(struct gve_rx_queue *rxq)
>  		if (diag < 0) {
>  			for (i = 0; i < nb_alloc; i++) {
>  				nmb = rte_pktmbuf_alloc(rxq->mpool);
> -				if (!nmb)
> +				if (!nmb) {
> +					rxq->no_mbufs++;
>  					break;
> +				}
>  				rxq->sw_ring[idx + i] = nmb;
>  			}
>  			nb_alloc = i;