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 <sthem...@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
