Re: [dpdk-dev] [PATCH v2 2/2] failsafe: implement xstats

["Gaëtan Rivet" <gaetan.rivet@6wind.com>]

Hello Stephen,

On Wed, Jun 26, 2019 at 04:33:46PM -0700, Stephen Hemminger wrote:
> Add support for extended statistics in failsafe driver.
> Reports basic statistics for the failsafe driver, and detailed
> statistics for each sub device.
> 
> Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
> ---
>  drivers/net/failsafe/failsafe_ops.c | 130 ++++++++++++++++++++++++++++
>  1 file changed, 130 insertions(+)
> 
> diff --git a/drivers/net/failsafe/failsafe_ops.c b/drivers/net/failsafe/failsafe_ops.c
> index 96e05d4dc4b1..a250c0528965 100644
> --- a/drivers/net/failsafe/failsafe_ops.c
> +++ b/drivers/net/failsafe/failsafe_ops.c
> @@ -789,6 +789,133 @@ fs_stats_reset(struct rte_eth_dev *dev)
>  	fs_unlock(dev, 0);
>  }
>  
> +static int
> +fs_xstats_count(struct rte_eth_dev *dev)
> +{
> +	struct sub_device *sdev;
> +	uint8_t i;
> +	int count;
> +
> +	count = rte_eth_basic_stats_count(dev);
> +
> +	fs_lock(dev, 0);
> +	FOREACH_SUBDEV(sdev, i, dev) {
> +		count += rte_eth_xstats_get_names(PORT_ID(sdev), NULL, 0);
> +	}
> +	fs_unlock(dev, 0);
> +
> +	return count;
> +}
> +
> +static int
> +fs_xstats_get_names(struct rte_eth_dev *dev,
> +		    struct rte_eth_xstat_name *xstats_names,
> +		    unsigned int limit)
> +{
> +	struct sub_device *sdev;
> +	unsigned int count;
> +	char tmp[RTE_ETH_XSTATS_NAME_SIZE];
> +	uint8_t i;
> +	int r;
> +
> +	if (!xstats_names)
> +		return fs_xstats_count(dev);
> +
> +	r = rte_eth_basic_stats_get_names(dev, xstats_names);
> +	if (r < 0)
> +		return r;
> +
> +	count = r;
> +
> +	fs_lock(dev, 0);
> +	FOREACH_SUBDEV(sdev, i, dev) {
> +		struct rte_eth_xstat_name *sub_names = xstats_names + count;
> +
> +		if (count >= limit)
> +			break;
> +
> +		r = rte_eth_xstats_get_names(PORT_ID(sdev),
> +					     xstats_names + count,

sub_names here?

Or do you want to point our the relationship between + count on the
array and - count on the array length? If so that makes sense.

> +					     limit - count);
> +		if (r < 0) {
> +			fs_unlock(dev, 0);
> +			return r;
> +		}
> +
> +		/* add sub_ prefix to names */
> +		for (i = 0; i < r; i++) {
> +			snprintf(tmp, sizeof(tmp), "sub%u_%s",

Prefixing is a good idea, maybe it would be better with an '_' between
the sub device name and the stat name?

The rest of the implementation seems solid, thanks.

You need to update the feature matrix for fail-safe however.

Regards,
-- 
Gaëtan Rivet
6WIND