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

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

On 9/19/2019 2:17 PM, 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.

+1, but doesn't need to pull basic stats to the xstats in the PMD level, that
part is already done by xstats API [1],
so 'fs_xstats_get.*()' functions return only sub-device basic stats is good
enough which won't require the new 'rte_eth_basic_stats_.*()' APIs.

Current implementation cause basic stats part duplicated for failsafe device.

[1]
rte_eth_xstats_get_names
  cnt_used_entries = rte_eth_basic_stats_get_names(dev, xstats_names)
  cnt_driver_entries = (*dev->dev_ops->xstats_get_names)(...)
  cnt_used_entries += cnt_driver_entries
  return cnt_used_entries;


btw, while testing this I have observed an always reproducible seg fault by
failsafe on testpmd quit [2], but didn't checked more, @Gaetan can you please
check this when possible?


[2]
Shutting down port 1...
Closing ports...
Done
Segmentation fault (core dumped)

With command:
testpmd -w 0:0.0 --vdev net_null0 --vdev
"net_failsafe0,dev(86:00.1),dev(86:00.3)" -- -i


> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> Acked-by: Gaetan Rivet <gaetan.rivet@6wind.com>

<...>

> +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;
> +
> +	/* Caller only cares about count */
> +	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), sub_names,
> +					     limit - count);
> +		if (r < 0) {
> +			fs_unlock(dev, 0);
> +			return r;
> +		}
> +
> +		/* add sub_ prefix to names */
> +		for (i = 0; i < r; i++) {

'i' is used in outer loop as well
thanks to 'C' to let us do these kind of things without any kind of warning.
Also it shows maintainer acked without testing :(

> +			snprintf(tmp, sizeof(tmp), "sub%u_%s",
> +				 i, sub_names[i].name);
> +			memcpy(sub_names[i].name, tmp,
> +			       RTE_ETH_XSTATS_NAME_SIZE);
> +		}
> +
> +		count += r;
> +	}
> +	fs_unlock(dev, 0);
> +
> +	return count;
> +}

<...>