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 <step...@networkplumber.org>
> Acked-by: Gaetan Rivet <gaetan.ri...@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;
> +}
<...>
