Re: [dpdk-dev] [PATCH 1/2] net/cxgbe: add support for xstats API

[Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>]

On 6/3/21 6:30 PM, Rahul Lakkireddy wrote:
> Add support to fetch port and queue stats via xstats API.
> 
> Signed-off-by: Rahul Lakkireddy <rahul.lakkireddy@chelsio.com>

[snip]

> +	count = 0;
> +	xstats_str = cxgbe_dev_port_stats_strings;
> +	for (i = 0; i < CXGBE_NB_PORT_STATS; i++, count++) {
> +		if (xstats_names)

!= NULL

DPDK coding style requires explicit comparison vs null [1]

[1] https://doc.dpdk.org/guides/contributing/coding_style.html#null-pointers

> +			snprintf(xstats_names[count].name,
> +				 sizeof(xstats_names[count].name),
> +				 "%s", xstats_str[i].name);
> +		if (xstats) {

!= NULL

> +			stats_ptr = RTE_PTR_ADD(&ps,
> +						xstats_str[i].offset);
> +			xstats[count].value = *stats_ptr;
> +			xstats[count].id = count;
> +		}
> +	}
> +
> +	/* per-txq stats */
> +	xstats_str = cxgbe_dev_txq_stats_strings;
> +	for (qid = 0; qid < pi->n_tx_qsets; qid++) {
> +		struct sge_eth_txq *txq = &s->ethtxq[pi->first_txqset + qid];
> +
> +		for (i = 0; i < CXGBE_NB_TXQ_STATS; i++, count++) {
> +			if (xstats_names)

!= NULL

> +				snprintf(xstats_names[count].name,
> +					 sizeof(xstats_names[count].name),
> +					 "tx_q%u_%s",
> +					 qid, xstats_str[i].name);
> +			if (xstats) {

!= NULL

> +				stats_ptr = RTE_PTR_ADD(&txq->stats,
> +							xstats_str[i].offset);
> +				xstats[count].value = *stats_ptr;
> +				xstats[count].id = count;
> +			}
> +		}
> +	}
> +
> +	/* per-rxq stats */
> +	xstats_str = cxgbe_dev_rxq_stats_strings;
> +	for (qid = 0; qid < pi->n_rx_qsets; qid++) {
> +		struct sge_eth_rxq *rxq = &s->ethrxq[pi->first_rxqset + qid];
> +
> +		for (i = 0; i < CXGBE_NB_RXQ_STATS; i++, count++) {
> +			if (xstats_names)

!= NULL

> +				snprintf(xstats_names[count].name,
> +					 sizeof(xstats_names[count].name),
> +					 "rx_q%u_%s",
> +					 qid, xstats_str[i].name);
> +			if (xstats) {

!= NULL

> +				stats_ptr = RTE_PTR_ADD(&rxq->stats,
> +							xstats_str[i].offset);
> +				xstats[count].value = *stats_ptr;
> +				xstats[count].id = count;
> +			}
> +		}
> +	}
> +
> +	return count;
> +}
> +
> +/* Get port extended statistics by id.
> + */
> +static int cxgbe_dev_xstats_get_by_id(struct rte_eth_dev *dev,
> +				      const uint64_t *ids, uint64_t *values,
> +				      unsigned int n)
> +{
> +	struct port_info *pi = dev->data->dev_private;
> +	struct rte_eth_xstat *xstats_copy;
> +	u16 count, i;
> +	int ret = 0;
> +
> +	count = cxgbe_dev_xstats_count(pi);
> +	if (!ids || !values)

== NULL twice

> +		return count;
> +
> +	xstats_copy = rte_calloc(NULL, count, sizeof(*xstats_copy), 0);
> +	if (!xstats_copy)

== NULL

> +		return -ENOMEM;
> +
> +	cxgbe_dev_xstats(dev, NULL, xstats_copy, count);
> +
> +	for (i = 0; i < n; i++) {
> +		if (ids[i] >= count) {
> +			ret = -EINVAL;
> +			goto out_err;
> +		}
> +		values[i] = xstats_copy[ids[i]].value;
> +	}
> +
> +	ret = n;
> +
> +out_err:
> +	rte_free(xstats_copy);
> +	return ret;
> +}
> +
> +/* Get names of port extended statistics by id.
> + */
> +static int cxgbe_dev_xstats_get_names_by_id(struct rte_eth_dev *dev,
> +					    struct rte_eth_xstat_name *xnames,
> +					    const uint64_t *ids, unsigned int n)
> +{
> +	struct port_info *pi = dev->data->dev_private;
> +	struct rte_eth_xstat_name *xnames_copy;
> +	u16 count, i;
> +	int ret = 0;
> +
> +	count = cxgbe_dev_xstats_count(pi);
> +	if (!ids || !xnames)

== NULL twice

> +		return count;
> +
> +	xnames_copy = rte_calloc(NULL, count, sizeof(*xnames_copy), 0);
> +	if (!xnames_copy)

== NULL

> +		return -ENOMEM;
> +
> +	cxgbe_dev_xstats(dev, xnames_copy, NULL, count);
> +
> +	for (i = 0; i < n; i++) {
> +		if (ids[i] >= count) {
> +			ret = -EINVAL;
> +			goto out_err;
> +		}
> +		strcpy(xnames[i].name, xnames_copy[ids[i]].name);

rte_strlcpy, please

> +	}
> +
> +	ret = n;
> +
> +out_err:
> +	rte_free(xnames_copy);
> +	return ret;
> +}
> +
> +/* Get port extended statistics.
> + */
> +static int cxgbe_dev_xstats_get(struct rte_eth_dev *dev,
> +				struct rte_eth_xstat *xstats, unsigned int n)
> +{
> +	return cxgbe_dev_xstats(dev, NULL, xstats, n);
> +}
> +
> +/* Get names of port extended statistics.
> + */
> +static int cxgbe_dev_xstats_get_names(struct rte_eth_dev *dev,
> +				      struct rte_eth_xstat_name *xstats_names,
> +				      unsigned int n)
> +{
> +	return cxgbe_dev_xstats(dev, xstats_names, NULL, n);
> +}
> +
> +/* Reset port extended statistics.
> + */
> +static int cxgbe_dev_xstats_reset(struct rte_eth_dev *dev)
> +{
> +	return cxgbe_dev_stats_reset(dev);
> +}
> +
>  static int cxgbe_flow_ctrl_get(struct rte_eth_dev *eth_dev,
>  			       struct rte_eth_fc_conf *fc_conf)
>  {
> @@ -1351,6 +1630,11 @@ static const struct eth_dev_ops cxgbe_eth_dev_ops = {
>  	.flow_ops_get           = cxgbe_dev_flow_ops_get,
>  	.stats_get		= cxgbe_dev_stats_get,
>  	.stats_reset		= cxgbe_dev_stats_reset,
> +	.xstats_get             = cxgbe_dev_xstats_get,
> +	.xstats_get_by_id       = cxgbe_dev_xstats_get_by_id,
> +	.xstats_get_names       = cxgbe_dev_xstats_get_names,
> +	.xstats_get_names_by_id = cxgbe_dev_xstats_get_names_by_id,
> +	.xstats_reset           = cxgbe_dev_xstats_reset,
>  	.flow_ctrl_get		= cxgbe_flow_ctrl_get,
>  	.flow_ctrl_set		= cxgbe_flow_ctrl_set,
>  	.get_eeprom_length	= cxgbe_get_eeprom_length,
>