Re: [dpdk-dev] [PATCH v4 3/8] ethdev: expose extended error stats

[Olivier MATZ <olivier.matz@6wind.com>]

Hi Maryam,

The API of the driver-specific function should be the same than
the generic one, described in rte_ethdev.h.

What I mean is:
  - the xstats pointer is the place where the xstats should be written
    by the driver
  - the n argument is the number of entries in the xstats structure

The driver should not have to worry about the generic stats written
by the generic layer.

Please see some comments below to fix it.

On 07/05/2015 07:39 PM, Maryam Tahhan wrote:
> Extend rte_eth_xstats_get to retrieve additional stats from the device
> driver as well the ethdev generic stats.
>
> Signed-off-by: Maryam Tahhan <maryam.tahhan@intel.com>
> ---
>   drivers/net/ixgbe/ixgbe_ethdev.c |  7 ++++---
>   lib/librte_ether/rte_ethdev.c    | 25 ++++++++++++++++---------
>   2 files changed, 20 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
> index 7feb03c..5971d41 100644
> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> @@ -2035,6 +2035,7 @@ ixgbe_dev_xstats_get(struct rte_eth_dev *dev, struct rte_eth_xstats *xstats,
>   	total_qprdc = 0;
>   	rxnfgpc = 0;
>   	txdgpc = 0;
> +	count = n - IXGBE_NB_XSTATS;
>
>   	ixgbe_read_stats_registers(hw, hw_stats, &total_missed_rx, &total_qbrc,
>   							   &total_qprc, &rxnfgpc, &txdgpc, &total_qprdc);
> @@ -2047,13 +2048,13 @@ ixgbe_dev_xstats_get(struct rte_eth_dev *dev, struct rte_eth_xstats *xstats,
>
>   	/* Extended stats */
>   	for (i = 0; i < IXGBE_NB_XSTATS; i++) {
> -		snprintf(xstats[i].name, sizeof(xstats[i].name),
> +		snprintf(xstats[count].name, sizeof(xstats[i].name),
>   				"%s", rte_ixgbe_stats_strings[i].name);
> -		xstats[i].value = *(uint64_t *)(((char *)hw_stats) +
> +		xstats[count++].value = *(uint64_t *)(((char *)hw_stats) +
>   							rte_ixgbe_stats_strings[i].offset);
>   	}
>
> -	return count;
> +	return IXGBE_NB_XSTATS;

The modifications in ixgbe driver can be removed: the patch 2/8 already
provides everything that is required.


>   }
>
>   static void
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> index da915db..e392f60 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -1681,26 +1681,33 @@ rte_eth_xstats_get(uint8_t port_id, struct rte_eth_xstats *xstats,
>   {
>   	struct rte_eth_stats eth_stats;
>   	struct rte_eth_dev *dev;
> -	unsigned count, i, q;
> +	unsigned count = 0, i, q;
> +	signed xcount = 0;
>   	uint64_t val, *stats_ptr;
>
>   	VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
>
>   	dev = &rte_eth_devices[port_id];
>
> -	/* implemented by the driver */
> -	if (dev->dev_ops->xstats_get != NULL)
> -		return (*dev->dev_ops->xstats_get)(dev, xstats, n);
> -
> -	/* else, return generic statistics */
> +	/* Return generic statistics */
>   	count = RTE_NB_STATS;
>   	count += dev->data->nb_rx_queues * RTE_NB_RXQ_STATS;
>   	count += dev->data->nb_tx_queues * RTE_NB_TXQ_STATS;
> +
> +	/* implemented by the driver */
> +	if (dev->dev_ops->xstats_get != NULL) {
> +		/* Retrieve the xstats from the driver at the end of the
> +		 * xstats struct.
> +		 */
> +		xcount = (*dev->dev_ops->xstats_get)(dev, xstats, n);

it should be:

   xcount = (*dev->dev_ops->xstats_get)(dev, &xstats[count],
   		(n > count) ? n - count : 0);


> +		if (xcount < 0)
> +			return xcount;
> +	}
> +
>   	if (n < count)
> -		return count;
> +		return count + xcount;

it should be:

   if (n < count + xcount)
   	return count + xcount;


>
>   	/* now fill the xstats structure */
> -
>   	count = 0;
>   	rte_eth_stats_get(port_id, &eth_stats);
>
> @@ -1742,7 +1749,7 @@ rte_eth_xstats_get(uint8_t port_id, struct rte_eth_xstats *xstats,
>   		}
>   	}
>
> -	return count;
> +	return count + xcount;
>   }
>
>   /* reset ethdev extended statistics */
>

Regards,
Olivier