Re: [dpdk-dev] [PATCH 08/32] net/ngbe: support basic statistics

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

On 9/8/2021 9:37 AM, Jiawen Wu wrote:
> Support to read and clear basic statistics, and configure per-queue
> stats counter mapping.
> 
> Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
> ---
>  doc/guides/nics/features/ngbe.ini  |   2 +
>  doc/guides/nics/ngbe.rst           |   1 +
>  drivers/net/ngbe/base/ngbe_dummy.h |   5 +
>  drivers/net/ngbe/base/ngbe_hw.c    | 101 ++++++++++
>  drivers/net/ngbe/base/ngbe_hw.h    |   1 +
>  drivers/net/ngbe/base/ngbe_type.h  | 134 +++++++++++++
>  drivers/net/ngbe/ngbe_ethdev.c     | 300 +++++++++++++++++++++++++++++
>  drivers/net/ngbe/ngbe_ethdev.h     |  19 ++
>  8 files changed, 563 insertions(+)
> 

<...>

> +static int
> +ngbe_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
> +{
> +	struct ngbe_hw *hw = ngbe_dev_hw(dev);
> +	struct ngbe_hw_stats *hw_stats = NGBE_DEV_STATS(dev);
> +	struct ngbe_stat_mappings *stat_mappings =
> +			NGBE_DEV_STAT_MAPPINGS(dev);
> +	uint32_t i, j;
> +
> +	ngbe_read_stats_registers(hw, hw_stats);
> +
> +	if (stats == NULL)
> +		return -EINVAL;
> +
> +	/* Fill out the rte_eth_stats statistics structure */
> +	stats->ipackets = hw_stats->rx_packets;
> +	stats->ibytes = hw_stats->rx_bytes;
> +	stats->opackets = hw_stats->tx_packets;
> +	stats->obytes = hw_stats->tx_bytes;
> +
> +	memset(&stats->q_ipackets, 0, sizeof(stats->q_ipackets));
> +	memset(&stats->q_opackets, 0, sizeof(stats->q_opackets));
> +	memset(&stats->q_ibytes, 0, sizeof(stats->q_ibytes));
> +	memset(&stats->q_obytes, 0, sizeof(stats->q_obytes));
> +	memset(&stats->q_errors, 0, sizeof(stats->q_errors));
> +	for (i = 0; i < NGBE_MAX_QP; i++) {
> +		uint32_t n = i / NB_QMAP_FIELDS_PER_QSM_REG;
> +		uint32_t offset = (i % NB_QMAP_FIELDS_PER_QSM_REG) * 8;
> +		uint32_t q_map;
> +
> +		q_map = (stat_mappings->rqsm[n] >> offset)
> +				& QMAP_FIELD_RESERVED_BITS_MASK;
> +		j = (q_map < RTE_ETHDEV_QUEUE_STAT_CNTRS
> +		     ? q_map : q_map % RTE_ETHDEV_QUEUE_STAT_CNTRS);
> +		stats->q_ipackets[j] += hw_stats->qp[i].rx_qp_packets;
> +		stats->q_ibytes[j] += hw_stats->qp[i].rx_qp_bytes;
> +
> +		q_map = (stat_mappings->tqsm[n] >> offset)
> +				& QMAP_FIELD_RESERVED_BITS_MASK;
> +		j = (q_map < RTE_ETHDEV_QUEUE_STAT_CNTRS
> +		     ? q_map : q_map % RTE_ETHDEV_QUEUE_STAT_CNTRS);
> +		stats->q_opackets[j] += hw_stats->qp[i].tx_qp_packets;
> +		stats->q_obytes[j] += hw_stats->qp[i].tx_qp_bytes;
> +	}
> +
> +	/* Rx Errors */
> +	stats->imissed  = hw_stats->rx_total_missed_packets +
> +			  hw_stats->rx_dma_drop;
> +	stats->ierrors  = hw_stats->rx_crc_errors +
> +			  hw_stats->rx_mac_short_packet_dropped +
> +			  hw_stats->rx_length_errors +
> +			  hw_stats->rx_undersize_errors +
> +			  hw_stats->rx_oversize_errors +
> +			  hw_stats->rx_illegal_byte_errors +
> +			  hw_stats->rx_error_bytes +
> +			  hw_stats->rx_fragment_errors;
> +
> +	/* Tx Errors */
> +	stats->oerrors  = 0;
> +	return 0;

You can consider keeping 'stats->rx_nombuf' stats too, this needs to be
calculated by driver.

<...>

> +
>  static int
>  ngbe_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
>  {
> @@ -1462,6 +1759,9 @@ static const struct eth_dev_ops ngbe_eth_dev_ops = {
>  	.dev_close                  = ngbe_dev_close,
>  	.dev_reset                  = ngbe_dev_reset,
>  	.link_update                = ngbe_dev_link_update,
> +	.stats_get                  = ngbe_dev_stats_get,
> +	.stats_reset                = ngbe_dev_stats_reset,
> +	.queue_stats_mapping_set    = ngbe_dev_queue_stats_mapping_set,

'queue_stats_mapping_set' is only needed when number of stats registers are less
than number of queues. If this is not the case for you please drop this support.

And we are switching to qstats on extending stat, please see
'RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS'. This is mainly done to remove the compile
time 'RTE_ETHDEV_QUEUE_STAT_CNTRS' limitation.

Btw, 'RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS' seems missing, you should have it in
the driver.