[dpdk-dev] [PATCH 0/2] failsafe: add xstats

[dpdk-dev] [PATCH 0/2] failsafe: add xstats

[Stephen Hemminger]

A useful feature of netvsc PMD is the ability to see how many packets
were processed through the VF device. This patch set adds a similar
(but more limited) capability to failsafe driver.

Since failsafe doesn't have top level xstats, this set uses the generic
xstats that exist already as a base then adds the sub-device xstats
to that.

Stephen Hemminger (2):
  ethdev: expose basic xstats for driver use
  failsafe: implement xstats

 drivers/net/failsafe/failsafe_ops.c      | 144 +++++++++++++++++++++++
 lib/librte_ethdev/rte_ethdev.c           |  17 ++-
 lib/librte_ethdev/rte_ethdev_core.h      |  14 +++
 lib/librte_ethdev/rte_ethdev_version.map |   3 +
 4 files changed, 169 insertions(+), 9 deletions(-)

-- 
2.20.1

[dpdk-dev] [PATCH 1/2] ethdev: expose basic xstats for driver use

[Stephen Hemminger]

From: Stephen Hemminger <sthemmin@microsoft.com>

Avoid duplication by having generic basic xstats available
for use by drivers. A later patch uses this for failsafe
driver.

Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
---
 lib/librte_ethdev/rte_ethdev.c           | 17 ++++++++---------
 lib/librte_ethdev/rte_ethdev_core.h      | 14 ++++++++++++++
 lib/librte_ethdev/rte_ethdev_version.map |  3 +++
 3 files changed, 25 insertions(+), 9 deletions(-)

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 8ac301608b9c..a83e9727c144 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -1996,8 +1996,8 @@ rte_eth_stats_reset(uint16_t port_id)
 	return 0;
 }
 
-static inline int
-get_xstats_basic_count(struct rte_eth_dev *dev)
+int
+rte_eth_basic_stats_count(struct rte_eth_dev *dev)
 {
 	uint16_t nb_rxqs, nb_txqs;
 	int count;
@@ -2034,7 +2034,7 @@ get_xstats_count(uint16_t port_id)
 		count = 0;
 
 
-	count += get_xstats_basic_count(dev);
+	count += rte_eth_basic_stats_count(dev);
 
 	return count;
 }
@@ -2084,7 +2084,7 @@ rte_eth_xstats_get_id_by_name(uint16_t port_id, const char *xstat_name,
 }
 
 /* retrieve basic stats names */
-static int
+int
 rte_eth_basic_stats_get_names(struct rte_eth_dev *dev,
 	struct rte_eth_xstat_name *xstats_names)
 {
@@ -2140,7 +2140,7 @@ rte_eth_xstats_get_names_by_id(uint16_t port_id,
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 	dev = &rte_eth_devices[port_id];
 
-	basic_count = get_xstats_basic_count(dev);
+	basic_count = rte_eth_basic_stats_count(dev);
 	ret = get_xstats_count(port_id);
 	if (ret < 0)
 		return ret;
@@ -2268,8 +2268,7 @@ rte_eth_xstats_get_names(uint16_t port_id,
 	return cnt_used_entries;
 }
 
-
-static int
+int
 rte_eth_basic_stats_get(uint16_t port_id, struct rte_eth_xstat *xstats)
 {
 	struct rte_eth_dev *dev;
@@ -2341,7 +2340,7 @@ rte_eth_xstats_get_by_id(uint16_t port_id, const uint64_t *ids,
 	expected_entries = (uint16_t)ret;
 	struct rte_eth_xstat xstats[expected_entries];
 	dev = &rte_eth_devices[port_id];
-	basic_count = get_xstats_basic_count(dev);
+	basic_count = rte_eth_basic_stats_count(dev);
 
 	/* Return max number of stats if no ids given */
 	if (!ids) {
@@ -2355,7 +2354,7 @@ rte_eth_xstats_get_by_id(uint16_t port_id, const uint64_t *ids,
 		return -EINVAL;
 
 	if (ids && dev->dev_ops->xstats_get_by_id != NULL && size) {
-		unsigned int basic_count = get_xstats_basic_count(dev);
+		unsigned int basic_count = rte_eth_basic_stats_count(dev);
 		uint64_t ids_copy[size];
 
 		for (i = 0; i < size; i++) {
diff --git a/lib/librte_ethdev/rte_ethdev_core.h b/lib/librte_ethdev/rte_ethdev_core.h
index 2922d5b7cc95..91ce1880d1c6 100644
--- a/lib/librte_ethdev/rte_ethdev_core.h
+++ b/lib/librte_ethdev/rte_ethdev_core.h
@@ -517,6 +517,20 @@ struct eth_dev_ops {
 	/**< Test if a port supports specific mempool ops */
 };
 
+/**
+ * @internal
+ * Get basic stats for ethdev
+ */
+int __rte_experimental
+rte_eth_basic_stats_count(struct rte_eth_dev *dev);
+
+int  __rte_experimental
+rte_eth_basic_stats_get_names(struct rte_eth_dev *dev,
+			      struct rte_eth_xstat_name *xstats_names);
+
+int __rte_experimental
+rte_eth_basic_stats_get(uint16_t port_id, struct rte_eth_xstat *xstats);
+
 /**
  * @internal
  * Structure used to hold information about the callbacks to be called for a
diff --git a/lib/librte_ethdev/rte_ethdev_version.map b/lib/librte_ethdev/rte_ethdev_version.map
index df9141825c3f..949a79800cbc 100644
--- a/lib/librte_ethdev/rte_ethdev_version.map
+++ b/lib/librte_ethdev/rte_ethdev_version.map
@@ -239,6 +239,9 @@ DPDK_19.05 {
 EXPERIMENTAL {
 	global:
 
+	rte_eth_basic_stats_count;
+	rte_eth_basic_stats_get;
+	rte_eth_basic_stats_get_names;
 	rte_eth_devargs_parse;
 	rte_eth_dev_create;
 	rte_eth_dev_destroy;
-- 
2.20.1

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

[Stephen Hemminger]

From: Stephen Hemminger <sthemmin@microsoft.com>

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 | 144 ++++++++++++++++++++++++++++
 1 file changed, 144 insertions(+)

diff --git a/drivers/net/failsafe/failsafe_ops.c b/drivers/net/failsafe/failsafe_ops.c
index 96e05d4dc4b1..9ab32457983d 100644
--- a/drivers/net/failsafe/failsafe_ops.c
+++ b/drivers/net/failsafe/failsafe_ops.c
@@ -789,6 +789,147 @@ 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;
+	int r, count;
+	uint8_t i;
+
+	count = rte_eth_basic_stats_count(dev);
+
+	fs_lock(dev, 0);
+	FOREACH_SUBDEV(sdev, i, dev) {
+		if (sdev->state < DEV_ACTIVE)
+			continue;
+
+		r = rte_eth_xstats_get_names(PORT_ID(sdev), NULL, 0);
+		if (r < 0) {
+			fs_unlock(dev, 0);
+			return r;
+		}
+
+		count += r;
+	}
+	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;
+	uint8_t i;
+	char tmp[RTE_ETH_XSTATS_NAME_SIZE];
+	int j,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 (sdev->state < DEV_ACTIVE)
+			continue;
+
+		if (count >= limit)
+			break;
+
+		r = rte_eth_xstats_get_names(PORT_ID(sdev), sub_names,
+					     count < limit ? limit - count: 0);
+		if (r < 0) {
+			fs_unlock(dev, 0);
+			return r;
+		}
+
+		/* add sub_ prefix to names */
+		for (j = 0; j < r; j++) {
+			snprintf(tmp, sizeof(tmp), "sub%u_%s",
+				 i, sub_names[j].name);
+			memcpy(sub_names[j].name, tmp,
+			       RTE_ETH_XSTATS_NAME_SIZE);
+		}
+
+		count += r;
+	}
+	fs_unlock(dev, 0);
+
+	return count;
+}
+
+static int
+fs_xstats_get(struct rte_eth_dev *dev,
+	      struct rte_eth_xstat *xstats,
+	      unsigned int n)
+{
+	unsigned int nstats, j, count, scount;
+	struct sub_device *sdev;
+	uint8_t i;
+	int ret;
+
+	nstats = fs_xstats_count(dev);
+	if (n < nstats || xstats == NULL)
+		return nstats;
+
+	ret = rte_eth_basic_stats_get(dev->data->port_id, xstats);
+	if (ret < 0)
+		return ret;
+
+	count = ret;
+	for (j = 0; j < count; j++)
+		xstats[j].id = j;
+
+	fs_lock(dev, 0);
+	FOREACH_SUBDEV(sdev, i, dev) {
+		if (sdev->state < DEV_ACTIVE)
+			continue;
+
+		ret = rte_eth_xstats_get(PORT_ID(sdev),
+					 xstats + count,
+					 n > count ? n - count : 0);
+		if (ret < 0) {
+			fs_unlock(dev, 0);
+			return ret;
+		}
+		scount = ret;
+
+		/* add offset to id's from sub-device */
+		for (j = 0; j < scount; j++)
+			xstats[count + j].id += count;
+
+		count += scount;
+	}
+	fs_unlock(dev, 0);
+
+	return count;
+}
+
+static void
+fs_xstats_reset(struct rte_eth_dev *dev)
+{
+	struct sub_device *sdev;
+	uint8_t i;
+
+	rte_eth_stats_reset(dev->data->port_id);
+
+	fs_lock(dev, 0);
+	FOREACH_SUBDEV(sdev, i, dev) {
+		rte_eth_xstats_reset(PORT_ID(sdev));
+	}
+	fs_unlock(dev, 0);
+}
+
 static void
 fs_dev_merge_desc_lim(struct rte_eth_desc_lim *to,
 		      const struct rte_eth_desc_lim *from)
@@ -1233,6 +1374,9 @@ const struct eth_dev_ops failsafe_ops = {
 	.link_update = fs_link_update,
 	.stats_get = fs_stats_get,
 	.stats_reset = fs_stats_reset,
+	.xstats_get = fs_xstats_get,
+	.xstats_get_names = fs_xstats_get_names,
+	.xstats_reset = fs_xstats_reset,
 	.dev_infos_get = fs_dev_infos_get,
 	.dev_supported_ptypes_get = fs_dev_supported_ptypes_get,
 	.mtu_set = fs_mtu_set,
-- 
2.20.1

[dpdk-dev] [PATCH v2 0/2] failsafe: add xstats

[Stephen Hemminger]

A useful feature of netvsc PMD is the ability to see how many packets
were processed through the VF device. This patch set adds a similar
(but more limited) capability to failsafe driver.

Since failsafe doesn't have top level xstats, this set uses the generic
xstats that exist already as a base then adds the sub-device xstats
to that.

v2 - fix checkpatch complaints

Stephen Hemminger (2):
  ethdev: expose basic xstats for driver use
  failsafe: implement xstats

 drivers/net/failsafe/failsafe_ops.c      | 130 +++++++++++++++++++++++
 lib/librte_ethdev/rte_ethdev.c           |  17 ++-
 lib/librte_ethdev/rte_ethdev_core.h      |  14 +++
 lib/librte_ethdev/rte_ethdev_version.map |   3 +
 4 files changed, 155 insertions(+), 9 deletions(-)

-- 
2.20.1

[dpdk-dev] [PATCH v2 1/2] ethdev: expose basic xstats for driver use

[Stephen Hemminger]

Avoid duplication by having generic basic xstats available
for use by drivers. A later patch uses this for failsafe
driver.

Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
---
 lib/librte_ethdev/rte_ethdev.c           | 17 ++++++++---------
 lib/librte_ethdev/rte_ethdev_core.h      | 14 ++++++++++++++
 lib/librte_ethdev/rte_ethdev_version.map |  3 +++
 3 files changed, 25 insertions(+), 9 deletions(-)

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 8ac301608b9c..a83e9727c144 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -1996,8 +1996,8 @@ rte_eth_stats_reset(uint16_t port_id)
 	return 0;
 }
 
-static inline int
-get_xstats_basic_count(struct rte_eth_dev *dev)
+int
+rte_eth_basic_stats_count(struct rte_eth_dev *dev)
 {
 	uint16_t nb_rxqs, nb_txqs;
 	int count;
@@ -2034,7 +2034,7 @@ get_xstats_count(uint16_t port_id)
 		count = 0;
 
 
-	count += get_xstats_basic_count(dev);
+	count += rte_eth_basic_stats_count(dev);
 
 	return count;
 }
@@ -2084,7 +2084,7 @@ rte_eth_xstats_get_id_by_name(uint16_t port_id, const char *xstat_name,
 }
 
 /* retrieve basic stats names */
-static int
+int
 rte_eth_basic_stats_get_names(struct rte_eth_dev *dev,
 	struct rte_eth_xstat_name *xstats_names)
 {
@@ -2140,7 +2140,7 @@ rte_eth_xstats_get_names_by_id(uint16_t port_id,
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 	dev = &rte_eth_devices[port_id];
 
-	basic_count = get_xstats_basic_count(dev);
+	basic_count = rte_eth_basic_stats_count(dev);
 	ret = get_xstats_count(port_id);
 	if (ret < 0)
 		return ret;
@@ -2268,8 +2268,7 @@ rte_eth_xstats_get_names(uint16_t port_id,
 	return cnt_used_entries;
 }
 
-
-static int
+int
 rte_eth_basic_stats_get(uint16_t port_id, struct rte_eth_xstat *xstats)
 {
 	struct rte_eth_dev *dev;
@@ -2341,7 +2340,7 @@ rte_eth_xstats_get_by_id(uint16_t port_id, const uint64_t *ids,
 	expected_entries = (uint16_t)ret;
 	struct rte_eth_xstat xstats[expected_entries];
 	dev = &rte_eth_devices[port_id];
-	basic_count = get_xstats_basic_count(dev);
+	basic_count = rte_eth_basic_stats_count(dev);
 
 	/* Return max number of stats if no ids given */
 	if (!ids) {
@@ -2355,7 +2354,7 @@ rte_eth_xstats_get_by_id(uint16_t port_id, const uint64_t *ids,
 		return -EINVAL;
 
 	if (ids && dev->dev_ops->xstats_get_by_id != NULL && size) {
-		unsigned int basic_count = get_xstats_basic_count(dev);
+		unsigned int basic_count = rte_eth_basic_stats_count(dev);
 		uint64_t ids_copy[size];
 
 		for (i = 0; i < size; i++) {
diff --git a/lib/librte_ethdev/rte_ethdev_core.h b/lib/librte_ethdev/rte_ethdev_core.h
index 2922d5b7cc95..91ce1880d1c6 100644
--- a/lib/librte_ethdev/rte_ethdev_core.h
+++ b/lib/librte_ethdev/rte_ethdev_core.h
@@ -517,6 +517,20 @@ struct eth_dev_ops {
 	/**< Test if a port supports specific mempool ops */
 };
 
+/**
+ * @internal
+ * Get basic stats for ethdev
+ */
+int __rte_experimental
+rte_eth_basic_stats_count(struct rte_eth_dev *dev);
+
+int  __rte_experimental
+rte_eth_basic_stats_get_names(struct rte_eth_dev *dev,
+			      struct rte_eth_xstat_name *xstats_names);
+
+int __rte_experimental
+rte_eth_basic_stats_get(uint16_t port_id, struct rte_eth_xstat *xstats);
+
 /**
  * @internal
  * Structure used to hold information about the callbacks to be called for a
diff --git a/lib/librte_ethdev/rte_ethdev_version.map b/lib/librte_ethdev/rte_ethdev_version.map
index df9141825c3f..949a79800cbc 100644
--- a/lib/librte_ethdev/rte_ethdev_version.map
+++ b/lib/librte_ethdev/rte_ethdev_version.map
@@ -239,6 +239,9 @@ DPDK_19.05 {
 EXPERIMENTAL {
 	global:
 
+	rte_eth_basic_stats_count;
+	rte_eth_basic_stats_get;
+	rte_eth_basic_stats_get_names;
 	rte_eth_devargs_parse;
 	rte_eth_dev_create;
 	rte_eth_dev_destroy;
-- 
2.20.1

Re: [dpdk-dev] [PATCH v2 1/2] ethdev: expose basic xstats for driver use

[Andrew Rybchenko]

On 27.06.2019 2:33, Stephen Hemminger wrote:
> Avoid duplication by having generic basic xstats available
> for use by drivers. A later patch uses this for failsafe
> driver.
>
> Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>

[...]


> diff --git a/lib/librte_ethdev/rte_ethdev_core.h b/lib/librte_ethdev/rte_ethdev_core.h
> index 2922d5b7cc95..91ce1880d1c6 100644
> --- a/lib/librte_ethdev/rte_ethdev_core.h
> +++ b/lib/librte_ethdev/rte_ethdev_core.h
> @@ -517,6 +517,20 @@ struct eth_dev_ops {
>   	/**< Test if a port supports specific mempool ops */
>   };
>   
> +/**
> + * @internal
> + * Get basic stats for ethdev
> + */
> +int __rte_experimental
> +rte_eth_basic_stats_count(struct rte_eth_dev *dev);
> +
> +int  __rte_experimental
> +rte_eth_basic_stats_get_names(struct rte_eth_dev *dev,
> +			      struct rte_eth_xstat_name *xstats_names);
> +
> +int __rte_experimental
> +rte_eth_basic_stats_get(uint16_t port_id, struct rte_eth_xstat *xstats);
> +
>   /**
>    * @internal
>    * Structure used to hold information about the callbacks to be called for a

It conflicts with __rte_experimenal placing patch which is on the 
mailing list.
Also I've expected to see these functions in rte_ethdev_driver.h to
avoid inclusion in rte_ethdev.h. As I understand these functions are for
rte_ethdev and drivers only.

> diff --git a/lib/librte_ethdev/rte_ethdev_version.map b/lib/librte_ethdev/rte_ethdev_version.map
> index df9141825c3f..949a79800cbc 100644
> --- a/lib/librte_ethdev/rte_ethdev_version.map
> +++ b/lib/librte_ethdev/rte_ethdev_version.map
> @@ -239,6 +239,9 @@ DPDK_19.05 {
>   EXPERIMENTAL {
>   	global:
>   
> +	rte_eth_basic_stats_count;
> +	rte_eth_basic_stats_get;
> +	rte_eth_basic_stats_get_names;
>   	rte_eth_devargs_parse;
>   	rte_eth_dev_create;
>   	rte_eth_dev_destroy;

Re: [dpdk-dev] [PATCH v2 1/2] ethdev: expose basic xstats for driver use

[Stephen Hemminger]

On Mon, 1 Jul 2019 13:54:52 +0300
Andrew Rybchenko <arybchenko@solarflare.com> wrote:

> On 27.06.2019 2:33, Stephen Hemminger wrote:
> > Avoid duplication by having generic basic xstats available
> > for use by drivers. A later patch uses this for failsafe
> > driver.
> >
> > Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>  
> 
> [...]
> 
> 
> > diff --git a/lib/librte_ethdev/rte_ethdev_core.h b/lib/librte_ethdev/rte_ethdev_core.h
> > index 2922d5b7cc95..91ce1880d1c6 100644
> > --- a/lib/librte_ethdev/rte_ethdev_core.h
> > +++ b/lib/librte_ethdev/rte_ethdev_core.h
> > @@ -517,6 +517,20 @@ struct eth_dev_ops {
> >   	/**< Test if a port supports specific mempool ops */
> >   };
> >   
> > +/**
> > + * @internal
> > + * Get basic stats for ethdev
> > + */
> > +int __rte_experimental
> > +rte_eth_basic_stats_count(struct rte_eth_dev *dev);
> > +
> > +int  __rte_experimental
> > +rte_eth_basic_stats_get_names(struct rte_eth_dev *dev,
> > +			      struct rte_eth_xstat_name *xstats_names);
> > +
> > +int __rte_experimental
> > +rte_eth_basic_stats_get(uint16_t port_id, struct rte_eth_xstat *xstats);
> > +
> >   /**
> >    * @internal
> >    * Structure used to hold information about the callbacks to be called for a  
> 
> It conflicts with __rte_experimenal placing patch which is on the 
> mailing list.
> Also I've expected to see these functions in rte_ethdev_driver.h to
> avoid inclusion in rte_ethdev.h. As I understand these functions are for
> rte_ethdev and drivers only.

Yes. Will change in V3

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

[Stephen Hemminger]

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,
+					     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",
+				 i, sub_names[i].name);
+			memcpy(sub_names[i].name, tmp,
+			       RTE_ETH_XSTATS_NAME_SIZE);
+		}
+
+		count += r;
+	}
+	fs_unlock(dev, 0);
+
+	return count;
+}
+
+static int
+fs_xstats_get(struct rte_eth_dev *dev,
+	      struct rte_eth_xstat *xstats,
+	      unsigned int n)
+{
+	unsigned int nstats, j, count, scount;
+	struct sub_device *sdev;
+	uint8_t i;
+	int ret;
+
+	nstats = fs_xstats_count(dev);
+	if (n < nstats || xstats == NULL)
+		return nstats;
+
+	ret = rte_eth_basic_stats_get(dev->data->port_id, xstats);
+	if (ret < 0)
+		return ret;
+
+	count = ret;
+	for (j = 0; j < count; j++)
+		xstats[j].id = j;
+
+	fs_lock(dev, 0);
+	FOREACH_SUBDEV(sdev, i, dev) {
+		ret = rte_eth_xstats_get(PORT_ID(sdev),
+					 xstats + count,
+					 n > count ? n - count : 0);
+		if (ret < 0) {
+			fs_unlock(dev, 0);
+			return ret;
+		}
+		scount = ret;
+
+		/* add offset to id's from sub-device */
+		for (j = 0; j < scount; j++)
+			xstats[count + j].id += count;
+
+		count += scount;
+	}
+	fs_unlock(dev, 0);
+
+	return count;
+}
+
+static void
+fs_xstats_reset(struct rte_eth_dev *dev)
+{
+	struct sub_device *sdev;
+	uint8_t i;
+
+	rte_eth_stats_reset(dev->data->port_id);
+
+	fs_lock(dev, 0);
+	FOREACH_SUBDEV(sdev, i, dev) {
+		rte_eth_xstats_reset(PORT_ID(sdev));
+	}
+	fs_unlock(dev, 0);
+}
+
 static void
 fs_dev_merge_desc_lim(struct rte_eth_desc_lim *to,
 		      const struct rte_eth_desc_lim *from)
@@ -1233,6 +1360,9 @@ const struct eth_dev_ops failsafe_ops = {
 	.link_update = fs_link_update,
 	.stats_get = fs_stats_get,
 	.stats_reset = fs_stats_reset,
+	.xstats_get = fs_xstats_get,
+	.xstats_get_names = fs_xstats_get_names,
+	.xstats_reset = fs_xstats_reset,
 	.dev_infos_get = fs_dev_infos_get,
 	.dev_supported_ptypes_get = fs_dev_supported_ptypes_get,
 	.mtu_set = fs_mtu_set,
-- 
2.20.1

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

[Gaëtan Rivet]

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

[dpdk-dev] [PATCH v2 0/2] *failsafe: add xstats

[Stephen Hemminger]

A useful feature of netvsc PMD is the ability to see how many packets
were processed through the VF device. This patch set adds a similar
(but more limited) capability to failsafe driver.

Since failsafe doesn't have top level xstats, this set uses the generic
xstats that exist already as a base then adds the sub-device xstats
to that.

v2 
    move generic xstats helpers to rte_ethdev_driver.h
    change prefix on subdevice stats to sub0_XXXX

Stephen Hemminger (2):
  ethdev: expose basic xstats for driver use
  failsafe: implement xstats

 drivers/net/failsafe/failsafe_ops.c      | 135 +++++++++++++++++++++++
 lib/librte_ethdev/rte_ethdev.c           |  17 ++-
 lib/librte_ethdev/rte_ethdev_driver.h    |  65 +++++++++++
 lib/librte_ethdev/rte_ethdev_version.map |   3 +
 4 files changed, 211 insertions(+), 9 deletions(-)

-- 
2.20.1

[dpdk-dev] [PATCH v2 1/2] ethdev: expose basic xstats for driver use

[Stephen Hemminger]

Avoid duplication by having generic basic xstats available
for use by drivers. A later patch uses this for failsafe
driver.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/librte_ethdev/rte_ethdev.c           | 17 +++----
 lib/librte_ethdev/rte_ethdev_driver.h    | 65 ++++++++++++++++++++++++
 lib/librte_ethdev/rte_ethdev_version.map |  3 ++
 3 files changed, 76 insertions(+), 9 deletions(-)

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 31f02ec2ba3b..82fcac57b1a7 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -1996,8 +1996,8 @@ rte_eth_stats_reset(uint16_t port_id)
 	return 0;
 }
 
-static inline int
-get_xstats_basic_count(struct rte_eth_dev *dev)
+int
+rte_eth_basic_stats_count(struct rte_eth_dev *dev)
 {
 	uint16_t nb_rxqs, nb_txqs;
 	int count;
@@ -2034,7 +2034,7 @@ get_xstats_count(uint16_t port_id)
 		count = 0;
 
 
-	count += get_xstats_basic_count(dev);
+	count += rte_eth_basic_stats_count(dev);
 
 	return count;
 }
@@ -2084,7 +2084,7 @@ rte_eth_xstats_get_id_by_name(uint16_t port_id, const char *xstat_name,
 }
 
 /* retrieve basic stats names */
-static int
+int
 rte_eth_basic_stats_get_names(struct rte_eth_dev *dev,
 	struct rte_eth_xstat_name *xstats_names)
 {
@@ -2140,7 +2140,7 @@ rte_eth_xstats_get_names_by_id(uint16_t port_id,
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 	dev = &rte_eth_devices[port_id];
 
-	basic_count = get_xstats_basic_count(dev);
+	basic_count = rte_eth_basic_stats_count(dev);
 	ret = get_xstats_count(port_id);
 	if (ret < 0)
 		return ret;
@@ -2268,8 +2268,7 @@ rte_eth_xstats_get_names(uint16_t port_id,
 	return cnt_used_entries;
 }
 
-
-static int
+int
 rte_eth_basic_stats_get(uint16_t port_id, struct rte_eth_xstat *xstats)
 {
 	struct rte_eth_dev *dev;
@@ -2341,7 +2340,7 @@ rte_eth_xstats_get_by_id(uint16_t port_id, const uint64_t *ids,
 	expected_entries = (uint16_t)ret;
 	struct rte_eth_xstat xstats[expected_entries];
 	dev = &rte_eth_devices[port_id];
-	basic_count = get_xstats_basic_count(dev);
+	basic_count = rte_eth_basic_stats_count(dev);
 
 	/* Return max number of stats if no ids given */
 	if (!ids) {
@@ -2355,7 +2354,7 @@ rte_eth_xstats_get_by_id(uint16_t port_id, const uint64_t *ids,
 		return -EINVAL;
 
 	if (ids && dev->dev_ops->xstats_get_by_id != NULL && size) {
-		unsigned int basic_count = get_xstats_basic_count(dev);
+		unsigned int basic_count = rte_eth_basic_stats_count(dev);
 		uint64_t ids_copy[size];
 
 		for (i = 0; i < size; i++) {
diff --git a/lib/librte_ethdev/rte_ethdev_driver.h b/lib/librte_ethdev/rte_ethdev_driver.h
index 936ff8c98651..489889a72203 100644
--- a/lib/librte_ethdev/rte_ethdev_driver.h
+++ b/lib/librte_ethdev/rte_ethdev_driver.h
@@ -208,6 +208,71 @@ rte_eth_linkstatus_get(const struct rte_eth_dev *dev,
 #endif
 }
 
+/**
+ * @internal
+ * Get basic stats part of xstats for an ethernet device.
+ *
+ * @param dev
+ *  Pointer to struct rte_eth_dev.
+ */
+__rte_experimental
+int
+rte_eth_basic_stats_count(struct rte_eth_dev *dev);
+
+/**
+ * @internal
+ * @b EXPERIMENTAL: this API may change without prior notice.
+ *
+ * Retrieve the names for the basic part of extended statistics.
+ *
+ * @param dev
+ *  Pointer to struct rte_eth_dev.
+ * @param xstats_names
+ *   An rte_eth_xstat_name array of at least *size* elements to
+ *   be filled. If set to NULL, the function returns the required number
+ *   of elements.
+ * @return
+ *   - A positive value lower or equal to size: success. The return value
+ *     is the number of entries filled in the stats table.
+ *   - A positive value higher than size: error, the given statistics table
+ *     is too small. The return value corresponds to the size that should
+ *     be given to succeed. The entries in the table are not valid and
+ *     shall not be used by the caller.
+ *   - A negative value on error (invalid port id).
+ */
+__rte_experimental
+int
+rte_eth_basic_stats_get_names(struct rte_eth_dev *dev,
+			      struct rte_eth_xstat_name *xstats_names);
+
+/**
+ * @internal
+ * @b EXPERIMENTAL: this API may change without prior notice.
+ *
+ * Retrieve the basic part of the extended statistics.
+ *
+ * @param dev
+ *  Pointer to struct rte_eth_dev.
+ * @param xstats
+ *   A pointer to a table of structure of type *rte_eth_xstat*
+ *   to be filled with device statistics ids and values.
+ *   This parameter can be set to NULL if n is 0.
+ * @param n
+ *   The size of the xstats array (number of elements).
+ * @return
+ *   - A positive value lower or equal to n: success. The return value
+ *     is the number of entries filled in the stats table.
+ *   - A positive value higher than n: error, the given statistics table
+ *     is too small. The return value corresponds to the size that should
+ *     be given to succeed. The entries in the table are not valid and
+ *     shall not be used by the caller.
+ *   - A negative value on error (invalid port id).
+ */
+__rte_experimental
+int
+rte_eth_basic_stats_get(uint16_t port_id, struct rte_eth_xstat *xstats);
+
+
 /**
  * @warning
  * @b EXPERIMENTAL: this API may change without prior notice.
diff --git a/lib/librte_ethdev/rte_ethdev_version.map b/lib/librte_ethdev/rte_ethdev_version.map
index df9141825c3f..949a79800cbc 100644
--- a/lib/librte_ethdev/rte_ethdev_version.map
+++ b/lib/librte_ethdev/rte_ethdev_version.map
@@ -239,6 +239,9 @@ DPDK_19.05 {
 EXPERIMENTAL {
 	global:
 
+	rte_eth_basic_stats_count;
+	rte_eth_basic_stats_get;
+	rte_eth_basic_stats_get_names;
 	rte_eth_devargs_parse;
 	rte_eth_dev_create;
 	rte_eth_dev_destroy;
-- 
2.20.1

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

[Stephen Hemminger]

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 <stephen@networkplumber.org>
---
 drivers/net/failsafe/failsafe_ops.c | 135 ++++++++++++++++++++++++++++
 1 file changed, 135 insertions(+)

diff --git a/drivers/net/failsafe/failsafe_ops.c b/drivers/net/failsafe/failsafe_ops.c
index 96e05d4dc4b1..711fdf302f4f 100644
--- a/drivers/net/failsafe/failsafe_ops.c
+++ b/drivers/net/failsafe/failsafe_ops.c
@@ -789,6 +789,138 @@ fs_stats_reset(struct rte_eth_dev *dev)
 	fs_unlock(dev, 0);
 }
 
+/*
+ * Count how many xstats in total
+ * Include basic stats for failsafe device
+ * plus xstats for each sub-device present.
+ */
+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;
+
+	/* 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++) {
+			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;
+}
+
+static int
+fs_xstats_get(struct rte_eth_dev *dev,
+	      struct rte_eth_xstat *xstats,
+	      unsigned int n)
+{
+	unsigned int nstats, j, count, scount;
+	struct sub_device *sdev;
+	uint8_t i;
+	int ret;
+
+	nstats = fs_xstats_count(dev);
+	if (n < nstats || xstats == NULL)
+		return nstats;
+
+	ret = rte_eth_basic_stats_get(dev->data->port_id, xstats);
+	if (ret < 0)
+		return ret;
+
+	count = ret;
+	for (j = 0; j < count; j++)
+		xstats[j].id = j;
+
+	fs_lock(dev, 0);
+	FOREACH_SUBDEV(sdev, i, dev) {
+		ret = rte_eth_xstats_get(PORT_ID(sdev),
+					 xstats + count,
+					 n > count ? n - count : 0);
+		if (ret < 0) {
+			fs_unlock(dev, 0);
+			return ret;
+		}
+		scount = ret;
+
+		/* add offset to id's from sub-device */
+		for (j = 0; j < scount; j++)
+			xstats[count + j].id += count;
+
+		count += scount;
+	}
+	fs_unlock(dev, 0);
+
+	return count;
+}
+
+static void
+fs_xstats_reset(struct rte_eth_dev *dev)
+{
+	struct sub_device *sdev;
+	uint8_t i;
+
+	rte_eth_stats_reset(dev->data->port_id);
+
+	fs_lock(dev, 0);
+	FOREACH_SUBDEV(sdev, i, dev) {
+		rte_eth_xstats_reset(PORT_ID(sdev));
+	}
+	fs_unlock(dev, 0);
+}
+
 static void
 fs_dev_merge_desc_lim(struct rte_eth_desc_lim *to,
 		      const struct rte_eth_desc_lim *from)
@@ -1233,6 +1365,9 @@ const struct eth_dev_ops failsafe_ops = {
 	.link_update = fs_link_update,
 	.stats_get = fs_stats_get,
 	.stats_reset = fs_stats_reset,
+	.xstats_get = fs_xstats_get,
+	.xstats_get_names = fs_xstats_get_names,
+	.xstats_reset = fs_xstats_reset,
 	.dev_infos_get = fs_dev_infos_get,
 	.dev_supported_ptypes_get = fs_dev_supported_ptypes_get,
 	.mtu_set = fs_mtu_set,
-- 
2.20.1

Re: [dpdk-dev] [PATCH v2 0/2] *failsafe: add xstats

[Gaëtan Rivet]

On Wed, Jul 03, 2019 at 10:25:10AM -0700, Stephen Hemminger wrote:
> A useful feature of netvsc PMD is the ability to see how many packets
> were processed through the VF device. This patch set adds a similar
> (but more limited) capability to failsafe driver.
> 
> Since failsafe doesn't have top level xstats, this set uses the generic
> xstats that exist already as a base then adds the sub-device xstats
> to that.
> 
> v2 
>     move generic xstats helpers to rte_ethdev_driver.h
>     change prefix on subdevice stats to sub0_XXXX
> 

For the series:

Acked-by: Gaetan Rivet <gaetan.rivet@6wind.com>

> Stephen Hemminger (2):
>   ethdev: expose basic xstats for driver use
>   failsafe: implement xstats
> 
>  drivers/net/failsafe/failsafe_ops.c      | 135 +++++++++++++++++++++++
>  lib/librte_ethdev/rte_ethdev.c           |  17 ++-
>  lib/librte_ethdev/rte_ethdev_driver.h    |  65 +++++++++++
>  lib/librte_ethdev/rte_ethdev_version.map |   3 +
>  4 files changed, 211 insertions(+), 9 deletions(-)
> 
> -- 
> 2.20.1
> 

-- 
Gaëtan Rivet
6WIND

[dpdk-dev] [PATCH v3 0/2] failsafe: add xstats

[Stephen Hemminger]

A useful feature of netvsc PMD is the ability to see how many packets
were processed through the VF device. This patch set adds a similar
(but more limited) capability to failsafe driver.

Since failsafe doesn't have top level xstats, this set uses the generic
xstats that exist already as a base then adds the sub-device xstats
to that.

v3 - rebase to 19.08

Stephen Hemminger (2):
  ethdev: expose basic xstats for driver use
  net/failsafe: implement xstats

 drivers/net/failsafe/failsafe_ops.c      | 135 +++++++++++++++++++++++
 lib/librte_ethdev/rte_ethdev.c           |  17 ++-
 lib/librte_ethdev/rte_ethdev_driver.h    |  65 +++++++++++
 lib/librte_ethdev/rte_ethdev_version.map |  24 ++++
 4 files changed, 232 insertions(+), 9 deletions(-)

-- 
2.20.1

[dpdk-dev] [PATCH v3 1/2] ethdev: expose basic xstats for driver use

[Stephen Hemminger]

Avoid duplication by having generic basic xstats available
for use by drivers. A later patch uses this for failsafe
driver.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 lib/librte_ethdev/rte_ethdev.c           | 17 +++----
 lib/librte_ethdev/rte_ethdev_driver.h    | 65 ++++++++++++++++++++++++
 lib/librte_ethdev/rte_ethdev_version.map | 24 +++++++++
 3 files changed, 97 insertions(+), 9 deletions(-)

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 17d183e1f0ec..88e3065d06fd 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -1996,8 +1996,8 @@ rte_eth_stats_reset(uint16_t port_id)
 	return 0;
 }
 
-static inline int
-get_xstats_basic_count(struct rte_eth_dev *dev)
+int
+rte_eth_basic_stats_count(struct rte_eth_dev *dev)
 {
 	uint16_t nb_rxqs, nb_txqs;
 	int count;
@@ -2034,7 +2034,7 @@ get_xstats_count(uint16_t port_id)
 		count = 0;
 
 
-	count += get_xstats_basic_count(dev);
+	count += rte_eth_basic_stats_count(dev);
 
 	return count;
 }
@@ -2084,7 +2084,7 @@ rte_eth_xstats_get_id_by_name(uint16_t port_id, const char *xstat_name,
 }
 
 /* retrieve basic stats names */
-static int
+int
 rte_eth_basic_stats_get_names(struct rte_eth_dev *dev,
 	struct rte_eth_xstat_name *xstats_names)
 {
@@ -2140,7 +2140,7 @@ rte_eth_xstats_get_names_by_id(uint16_t port_id,
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 	dev = &rte_eth_devices[port_id];
 
-	basic_count = get_xstats_basic_count(dev);
+	basic_count = rte_eth_basic_stats_count(dev);
 	ret = get_xstats_count(port_id);
 	if (ret < 0)
 		return ret;
@@ -2268,8 +2268,7 @@ rte_eth_xstats_get_names(uint16_t port_id,
 	return cnt_used_entries;
 }
 
-
-static int
+int
 rte_eth_basic_stats_get(uint16_t port_id, struct rte_eth_xstat *xstats)
 {
 	struct rte_eth_dev *dev;
@@ -2341,7 +2340,7 @@ rte_eth_xstats_get_by_id(uint16_t port_id, const uint64_t *ids,
 	expected_entries = (uint16_t)ret;
 	struct rte_eth_xstat xstats[expected_entries];
 	dev = &rte_eth_devices[port_id];
-	basic_count = get_xstats_basic_count(dev);
+	basic_count = rte_eth_basic_stats_count(dev);
 
 	/* Return max number of stats if no ids given */
 	if (!ids) {
@@ -2355,7 +2354,7 @@ rte_eth_xstats_get_by_id(uint16_t port_id, const uint64_t *ids,
 		return -EINVAL;
 
 	if (ids && dev->dev_ops->xstats_get_by_id != NULL && size) {
-		unsigned int basic_count = get_xstats_basic_count(dev);
+		unsigned int basic_count = rte_eth_basic_stats_count(dev);
 		uint64_t ids_copy[size];
 
 		for (i = 0; i < size; i++) {
diff --git a/lib/librte_ethdev/rte_ethdev_driver.h b/lib/librte_ethdev/rte_ethdev_driver.h
index 936ff8c98651..489889a72203 100644
--- a/lib/librte_ethdev/rte_ethdev_driver.h
+++ b/lib/librte_ethdev/rte_ethdev_driver.h
@@ -208,6 +208,71 @@ rte_eth_linkstatus_get(const struct rte_eth_dev *dev,
 #endif
 }
 
+/**
+ * @internal
+ * Get basic stats part of xstats for an ethernet device.
+ *
+ * @param dev
+ *  Pointer to struct rte_eth_dev.
+ */
+__rte_experimental
+int
+rte_eth_basic_stats_count(struct rte_eth_dev *dev);
+
+/**
+ * @internal
+ * @b EXPERIMENTAL: this API may change without prior notice.
+ *
+ * Retrieve the names for the basic part of extended statistics.
+ *
+ * @param dev
+ *  Pointer to struct rte_eth_dev.
+ * @param xstats_names
+ *   An rte_eth_xstat_name array of at least *size* elements to
+ *   be filled. If set to NULL, the function returns the required number
+ *   of elements.
+ * @return
+ *   - A positive value lower or equal to size: success. The return value
+ *     is the number of entries filled in the stats table.
+ *   - A positive value higher than size: error, the given statistics table
+ *     is too small. The return value corresponds to the size that should
+ *     be given to succeed. The entries in the table are not valid and
+ *     shall not be used by the caller.
+ *   - A negative value on error (invalid port id).
+ */
+__rte_experimental
+int
+rte_eth_basic_stats_get_names(struct rte_eth_dev *dev,
+			      struct rte_eth_xstat_name *xstats_names);
+
+/**
+ * @internal
+ * @b EXPERIMENTAL: this API may change without prior notice.
+ *
+ * Retrieve the basic part of the extended statistics.
+ *
+ * @param dev
+ *  Pointer to struct rte_eth_dev.
+ * @param xstats
+ *   A pointer to a table of structure of type *rte_eth_xstat*
+ *   to be filled with device statistics ids and values.
+ *   This parameter can be set to NULL if n is 0.
+ * @param n
+ *   The size of the xstats array (number of elements).
+ * @return
+ *   - A positive value lower or equal to n: success. The return value
+ *     is the number of entries filled in the stats table.
+ *   - A positive value higher than n: error, the given statistics table
+ *     is too small. The return value corresponds to the size that should
+ *     be given to succeed. The entries in the table are not valid and
+ *     shall not be used by the caller.
+ *   - A negative value on error (invalid port id).
+ */
+__rte_experimental
+int
+rte_eth_basic_stats_get(uint16_t port_id, struct rte_eth_xstat *xstats);
+
+
 /**
  * @warning
  * @b EXPERIMENTAL: this API may change without prior notice.
diff --git a/lib/librte_ethdev/rte_ethdev_version.map b/lib/librte_ethdev/rte_ethdev_version.map
index 6df42a47b89d..f2198b0367d1 100644
--- a/lib/librte_ethdev/rte_ethdev_version.map
+++ b/lib/librte_ethdev/rte_ethdev_version.map
@@ -240,6 +240,25 @@ EXPERIMENTAL {
 	global:
 
 	# added in 17.11
+	rte_eth_devargs_parse;
+	rte_eth_dev_create;
+	rte_eth_dev_destroy;
+	rte_eth_dev_get_module_eeprom;
+	rte_eth_dev_get_module_info;
+	rte_eth_dev_is_removed;
+	rte_eth_dev_owner_delete;
+	rte_eth_dev_owner_get;
+	rte_eth_dev_owner_new;
+	rte_eth_dev_owner_set;
+	rte_eth_dev_owner_unset;
+	rte_eth_dev_rx_intr_ctl_q_get_fd;
+	rte_eth_find_next_of;
+	rte_eth_find_next_sibling;
+	rte_eth_read_clock;
+	rte_eth_switch_domain_alloc;
+	rte_eth_switch_domain_free;
+	rte_flow_conv;
+	rte_flow_expand_rss;
 	rte_mtr_capabilities_get;
 	rte_mtr_create;
 	rte_mtr_destroy;
@@ -283,4 +302,9 @@ EXPERIMENTAL {
 
 	# added in 19.08
 	rte_eth_read_clock;
+
+	# added in 19.11
+	rte_eth_basic_stats_count;
+	rte_eth_basic_stats_get;
+	rte_eth_basic_stats_get_names;
 };
-- 
2.20.1

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

[Stephen Hemminger]

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 <stephen@networkplumber.org>
Acked-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 drivers/net/failsafe/failsafe_ops.c | 135 ++++++++++++++++++++++++++++
 1 file changed, 135 insertions(+)

diff --git a/drivers/net/failsafe/failsafe_ops.c b/drivers/net/failsafe/failsafe_ops.c
index 96e05d4dc4b1..711fdf302f4f 100644
--- a/drivers/net/failsafe/failsafe_ops.c
+++ b/drivers/net/failsafe/failsafe_ops.c
@@ -789,6 +789,138 @@ fs_stats_reset(struct rte_eth_dev *dev)
 	fs_unlock(dev, 0);
 }
 
+/*
+ * Count how many xstats in total
+ * Include basic stats for failsafe device
+ * plus xstats for each sub-device present.
+ */
+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;
+
+	/* 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++) {
+			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;
+}
+
+static int
+fs_xstats_get(struct rte_eth_dev *dev,
+	      struct rte_eth_xstat *xstats,
+	      unsigned int n)
+{
+	unsigned int nstats, j, count, scount;
+	struct sub_device *sdev;
+	uint8_t i;
+	int ret;
+
+	nstats = fs_xstats_count(dev);
+	if (n < nstats || xstats == NULL)
+		return nstats;
+
+	ret = rte_eth_basic_stats_get(dev->data->port_id, xstats);
+	if (ret < 0)
+		return ret;
+
+	count = ret;
+	for (j = 0; j < count; j++)
+		xstats[j].id = j;
+
+	fs_lock(dev, 0);
+	FOREACH_SUBDEV(sdev, i, dev) {
+		ret = rte_eth_xstats_get(PORT_ID(sdev),
+					 xstats + count,
+					 n > count ? n - count : 0);
+		if (ret < 0) {
+			fs_unlock(dev, 0);
+			return ret;
+		}
+		scount = ret;
+
+		/* add offset to id's from sub-device */
+		for (j = 0; j < scount; j++)
+			xstats[count + j].id += count;
+
+		count += scount;
+	}
+	fs_unlock(dev, 0);
+
+	return count;
+}
+
+static void
+fs_xstats_reset(struct rte_eth_dev *dev)
+{
+	struct sub_device *sdev;
+	uint8_t i;
+
+	rte_eth_stats_reset(dev->data->port_id);
+
+	fs_lock(dev, 0);
+	FOREACH_SUBDEV(sdev, i, dev) {
+		rte_eth_xstats_reset(PORT_ID(sdev));
+	}
+	fs_unlock(dev, 0);
+}
+
 static void
 fs_dev_merge_desc_lim(struct rte_eth_desc_lim *to,
 		      const struct rte_eth_desc_lim *from)
@@ -1233,6 +1365,9 @@ const struct eth_dev_ops failsafe_ops = {
 	.link_update = fs_link_update,
 	.stats_get = fs_stats_get,
 	.stats_reset = fs_stats_reset,
+	.xstats_get = fs_xstats_get,
+	.xstats_get_names = fs_xstats_get_names,
+	.xstats_reset = fs_xstats_reset,
 	.dev_infos_get = fs_dev_infos_get,
 	.dev_supported_ptypes_get = fs_dev_supported_ptypes_get,
 	.mtu_set = fs_mtu_set,
-- 
2.20.1

[dpdk-dev] [PATCH v4 0/2] failsafe: add xstats

[Stephen Hemminger]

A useful feature of netvsc PMD is the ability to see how many packets
were processed through the VF device. This patch set adds a similar
(but more limited) capability to failsafe driver.

Since failsafe doesn't have top level xstats, this set uses the generic
xstats that exist already as a base then adds the sub-device xstats
to that.

v4 - rebase to 19.11-rc
v3 - rebase to 19.08

Stephen Hemminger (2):
  ethdev: expose basic xstats for driver use
  net/failsafe: implement xstats

 drivers/net/failsafe/failsafe_ops.c      | 135 +++++++++++++++++++++++
 lib/librte_ethdev/rte_ethdev.c           |  17 ++-
 lib/librte_ethdev/rte_ethdev_driver.h    |  65 +++++++++++
 lib/librte_ethdev/rte_ethdev_version.map |  24 ++++
 4 files changed, 232 insertions(+), 9 deletions(-)

-- 
2.17.1

[dpdk-dev] [PATCH 1/2] ethdev: expose basic xstats for driver use

[Stephen Hemminger]

Avoid duplication by having generic basic xstats available
for use by drivers. A later patch uses this for failsafe
driver.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 lib/librte_ethdev/rte_ethdev.c           | 17 +++----
 lib/librte_ethdev/rte_ethdev_driver.h    | 65 ++++++++++++++++++++++++
 lib/librte_ethdev/rte_ethdev_version.map | 24 +++++++++
 3 files changed, 97 insertions(+), 9 deletions(-)

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 17d183e1f0ec..88e3065d06fd 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -1996,8 +1996,8 @@ rte_eth_stats_reset(uint16_t port_id)
 	return 0;
 }
 
-static inline int
-get_xstats_basic_count(struct rte_eth_dev *dev)
+int
+rte_eth_basic_stats_count(struct rte_eth_dev *dev)
 {
 	uint16_t nb_rxqs, nb_txqs;
 	int count;
@@ -2034,7 +2034,7 @@ get_xstats_count(uint16_t port_id)
 		count = 0;
 
 
-	count += get_xstats_basic_count(dev);
+	count += rte_eth_basic_stats_count(dev);
 
 	return count;
 }
@@ -2084,7 +2084,7 @@ rte_eth_xstats_get_id_by_name(uint16_t port_id, const char *xstat_name,
 }
 
 /* retrieve basic stats names */
-static int
+int
 rte_eth_basic_stats_get_names(struct rte_eth_dev *dev,
 	struct rte_eth_xstat_name *xstats_names)
 {
@@ -2140,7 +2140,7 @@ rte_eth_xstats_get_names_by_id(uint16_t port_id,
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 	dev = &rte_eth_devices[port_id];
 
-	basic_count = get_xstats_basic_count(dev);
+	basic_count = rte_eth_basic_stats_count(dev);
 	ret = get_xstats_count(port_id);
 	if (ret < 0)
 		return ret;
@@ -2268,8 +2268,7 @@ rte_eth_xstats_get_names(uint16_t port_id,
 	return cnt_used_entries;
 }
 
-
-static int
+int
 rte_eth_basic_stats_get(uint16_t port_id, struct rte_eth_xstat *xstats)
 {
 	struct rte_eth_dev *dev;
@@ -2341,7 +2340,7 @@ rte_eth_xstats_get_by_id(uint16_t port_id, const uint64_t *ids,
 	expected_entries = (uint16_t)ret;
 	struct rte_eth_xstat xstats[expected_entries];
 	dev = &rte_eth_devices[port_id];
-	basic_count = get_xstats_basic_count(dev);
+	basic_count = rte_eth_basic_stats_count(dev);
 
 	/* Return max number of stats if no ids given */
 	if (!ids) {
@@ -2355,7 +2354,7 @@ rte_eth_xstats_get_by_id(uint16_t port_id, const uint64_t *ids,
 		return -EINVAL;
 
 	if (ids && dev->dev_ops->xstats_get_by_id != NULL && size) {
-		unsigned int basic_count = get_xstats_basic_count(dev);
+		unsigned int basic_count = rte_eth_basic_stats_count(dev);
 		uint64_t ids_copy[size];
 
 		for (i = 0; i < size; i++) {
diff --git a/lib/librte_ethdev/rte_ethdev_driver.h b/lib/librte_ethdev/rte_ethdev_driver.h
index 936ff8c98651..489889a72203 100644
--- a/lib/librte_ethdev/rte_ethdev_driver.h
+++ b/lib/librte_ethdev/rte_ethdev_driver.h
@@ -208,6 +208,71 @@ rte_eth_linkstatus_get(const struct rte_eth_dev *dev,
 #endif
 }
 
+/**
+ * @internal
+ * Get basic stats part of xstats for an ethernet device.
+ *
+ * @param dev
+ *  Pointer to struct rte_eth_dev.
+ */
+__rte_experimental
+int
+rte_eth_basic_stats_count(struct rte_eth_dev *dev);
+
+/**
+ * @internal
+ * @b EXPERIMENTAL: this API may change without prior notice.
+ *
+ * Retrieve the names for the basic part of extended statistics.
+ *
+ * @param dev
+ *  Pointer to struct rte_eth_dev.
+ * @param xstats_names
+ *   An rte_eth_xstat_name array of at least *size* elements to
+ *   be filled. If set to NULL, the function returns the required number
+ *   of elements.
+ * @return
+ *   - A positive value lower or equal to size: success. The return value
+ *     is the number of entries filled in the stats table.
+ *   - A positive value higher than size: error, the given statistics table
+ *     is too small. The return value corresponds to the size that should
+ *     be given to succeed. The entries in the table are not valid and
+ *     shall not be used by the caller.
+ *   - A negative value on error (invalid port id).
+ */
+__rte_experimental
+int
+rte_eth_basic_stats_get_names(struct rte_eth_dev *dev,
+			      struct rte_eth_xstat_name *xstats_names);
+
+/**
+ * @internal
+ * @b EXPERIMENTAL: this API may change without prior notice.
+ *
+ * Retrieve the basic part of the extended statistics.
+ *
+ * @param dev
+ *  Pointer to struct rte_eth_dev.
+ * @param xstats
+ *   A pointer to a table of structure of type *rte_eth_xstat*
+ *   to be filled with device statistics ids and values.
+ *   This parameter can be set to NULL if n is 0.
+ * @param n
+ *   The size of the xstats array (number of elements).
+ * @return
+ *   - A positive value lower or equal to n: success. The return value
+ *     is the number of entries filled in the stats table.
+ *   - A positive value higher than n: error, the given statistics table
+ *     is too small. The return value corresponds to the size that should
+ *     be given to succeed. The entries in the table are not valid and
+ *     shall not be used by the caller.
+ *   - A negative value on error (invalid port id).
+ */
+__rte_experimental
+int
+rte_eth_basic_stats_get(uint16_t port_id, struct rte_eth_xstat *xstats);
+
+
 /**
  * @warning
  * @b EXPERIMENTAL: this API may change without prior notice.
diff --git a/lib/librte_ethdev/rte_ethdev_version.map b/lib/librte_ethdev/rte_ethdev_version.map
index 6df42a47b89d..f2198b0367d1 100644
--- a/lib/librte_ethdev/rte_ethdev_version.map
+++ b/lib/librte_ethdev/rte_ethdev_version.map
@@ -240,6 +240,25 @@ EXPERIMENTAL {
 	global:
 
 	# added in 17.11
+	rte_eth_devargs_parse;
+	rte_eth_dev_create;
+	rte_eth_dev_destroy;
+	rte_eth_dev_get_module_eeprom;
+	rte_eth_dev_get_module_info;
+	rte_eth_dev_is_removed;
+	rte_eth_dev_owner_delete;
+	rte_eth_dev_owner_get;
+	rte_eth_dev_owner_new;
+	rte_eth_dev_owner_set;
+	rte_eth_dev_owner_unset;
+	rte_eth_dev_rx_intr_ctl_q_get_fd;
+	rte_eth_find_next_of;
+	rte_eth_find_next_sibling;
+	rte_eth_read_clock;
+	rte_eth_switch_domain_alloc;
+	rte_eth_switch_domain_free;
+	rte_flow_conv;
+	rte_flow_expand_rss;
 	rte_mtr_capabilities_get;
 	rte_mtr_create;
 	rte_mtr_destroy;
@@ -283,4 +302,9 @@ EXPERIMENTAL {
 
 	# added in 19.08
 	rte_eth_read_clock;
+
+	# added in 19.11
+	rte_eth_basic_stats_count;
+	rte_eth_basic_stats_get;
+	rte_eth_basic_stats_get_names;
 };
-- 
2.17.1

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

[Stephen Hemminger]

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 <stephen@networkplumber.org>
Acked-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 drivers/net/failsafe/failsafe_ops.c | 135 ++++++++++++++++++++++++++++
 1 file changed, 135 insertions(+)

diff --git a/drivers/net/failsafe/failsafe_ops.c b/drivers/net/failsafe/failsafe_ops.c
index 96e05d4dc4b1..711fdf302f4f 100644
--- a/drivers/net/failsafe/failsafe_ops.c
+++ b/drivers/net/failsafe/failsafe_ops.c
@@ -789,6 +789,138 @@ fs_stats_reset(struct rte_eth_dev *dev)
 	fs_unlock(dev, 0);
 }
 
+/*
+ * Count how many xstats in total
+ * Include basic stats for failsafe device
+ * plus xstats for each sub-device present.
+ */
+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;
+
+	/* 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++) {
+			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;
+}
+
+static int
+fs_xstats_get(struct rte_eth_dev *dev,
+	      struct rte_eth_xstat *xstats,
+	      unsigned int n)
+{
+	unsigned int nstats, j, count, scount;
+	struct sub_device *sdev;
+	uint8_t i;
+	int ret;
+
+	nstats = fs_xstats_count(dev);
+	if (n < nstats || xstats == NULL)
+		return nstats;
+
+	ret = rte_eth_basic_stats_get(dev->data->port_id, xstats);
+	if (ret < 0)
+		return ret;
+
+	count = ret;
+	for (j = 0; j < count; j++)
+		xstats[j].id = j;
+
+	fs_lock(dev, 0);
+	FOREACH_SUBDEV(sdev, i, dev) {
+		ret = rte_eth_xstats_get(PORT_ID(sdev),
+					 xstats + count,
+					 n > count ? n - count : 0);
+		if (ret < 0) {
+			fs_unlock(dev, 0);
+			return ret;
+		}
+		scount = ret;
+
+		/* add offset to id's from sub-device */
+		for (j = 0; j < scount; j++)
+			xstats[count + j].id += count;
+
+		count += scount;
+	}
+	fs_unlock(dev, 0);
+
+	return count;
+}
+
+static void
+fs_xstats_reset(struct rte_eth_dev *dev)
+{
+	struct sub_device *sdev;
+	uint8_t i;
+
+	rte_eth_stats_reset(dev->data->port_id);
+
+	fs_lock(dev, 0);
+	FOREACH_SUBDEV(sdev, i, dev) {
+		rte_eth_xstats_reset(PORT_ID(sdev));
+	}
+	fs_unlock(dev, 0);
+}
+
 static void
 fs_dev_merge_desc_lim(struct rte_eth_desc_lim *to,
 		      const struct rte_eth_desc_lim *from)
@@ -1233,6 +1365,9 @@ const struct eth_dev_ops failsafe_ops = {
 	.link_update = fs_link_update,
 	.stats_get = fs_stats_get,
 	.stats_reset = fs_stats_reset,
+	.xstats_get = fs_xstats_get,
+	.xstats_get_names = fs_xstats_get_names,
+	.xstats_reset = fs_xstats_reset,
 	.dev_infos_get = fs_dev_infos_get,
 	.dev_supported_ptypes_get = fs_dev_supported_ptypes_get,
 	.mtu_set = fs_mtu_set,
-- 
2.17.1

[dpdk-dev] [PATCH v5 0/2] failsafe: add xstats

[Stephen Hemminger]

A useful feature of netvsc PMD is the ability to see how many
packets
were processed through the VF device. This patch set adds a similar
(but more limited) capability to failsafe driver.

Since failsafe doesn't have top level xstats, this set uses the generic
xstats that exist already as a base then adds the sub-device xstats
to that.

v5 - fix ethdev map file
v4 - rebase to 19.11
v3 - rebase to 19.08

Stephen Hemminger (2):
  ethdev: expose basic xstats for driver use
  net/failsafe: implement xstats

 drivers/net/failsafe/failsafe_ops.c      | 135 +++++++++++++++++++++++
 lib/librte_ethdev/rte_ethdev.c           |  17 ++-
 lib/librte_ethdev/rte_ethdev_driver.h    |  65 +++++++++++
 lib/librte_ethdev/rte_ethdev_version.map |   5 +
 4 files changed, 213 insertions(+), 9 deletions(-)

-- 
2.17.1

[dpdk-dev] [PATCH v5 1/2] ethdev: expose basic xstats for driver use

[Stephen Hemminger]

Avoid duplication by having generic basic xstats available
for use by drivers. A later patch uses this for failsafe
driver.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 lib/librte_ethdev/rte_ethdev.c           | 17 +++----
 lib/librte_ethdev/rte_ethdev_driver.h    | 65 ++++++++++++++++++++++++
 lib/librte_ethdev/rte_ethdev_version.map |  5 ++
 3 files changed, 78 insertions(+), 9 deletions(-)

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 17d183e1f0ec..88e3065d06fd 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -1996,8 +1996,8 @@ rte_eth_stats_reset(uint16_t port_id)
 	return 0;
 }
 
-static inline int
-get_xstats_basic_count(struct rte_eth_dev *dev)
+int
+rte_eth_basic_stats_count(struct rte_eth_dev *dev)
 {
 	uint16_t nb_rxqs, nb_txqs;
 	int count;
@@ -2034,7 +2034,7 @@ get_xstats_count(uint16_t port_id)
 		count = 0;
 
 
-	count += get_xstats_basic_count(dev);
+	count += rte_eth_basic_stats_count(dev);
 
 	return count;
 }
@@ -2084,7 +2084,7 @@ rte_eth_xstats_get_id_by_name(uint16_t port_id, const char *xstat_name,
 }
 
 /* retrieve basic stats names */
-static int
+int
 rte_eth_basic_stats_get_names(struct rte_eth_dev *dev,
 	struct rte_eth_xstat_name *xstats_names)
 {
@@ -2140,7 +2140,7 @@ rte_eth_xstats_get_names_by_id(uint16_t port_id,
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 	dev = &rte_eth_devices[port_id];
 
-	basic_count = get_xstats_basic_count(dev);
+	basic_count = rte_eth_basic_stats_count(dev);
 	ret = get_xstats_count(port_id);
 	if (ret < 0)
 		return ret;
@@ -2268,8 +2268,7 @@ rte_eth_xstats_get_names(uint16_t port_id,
 	return cnt_used_entries;
 }
 
-
-static int
+int
 rte_eth_basic_stats_get(uint16_t port_id, struct rte_eth_xstat *xstats)
 {
 	struct rte_eth_dev *dev;
@@ -2341,7 +2340,7 @@ rte_eth_xstats_get_by_id(uint16_t port_id, const uint64_t *ids,
 	expected_entries = (uint16_t)ret;
 	struct rte_eth_xstat xstats[expected_entries];
 	dev = &rte_eth_devices[port_id];
-	basic_count = get_xstats_basic_count(dev);
+	basic_count = rte_eth_basic_stats_count(dev);
 
 	/* Return max number of stats if no ids given */
 	if (!ids) {
@@ -2355,7 +2354,7 @@ rte_eth_xstats_get_by_id(uint16_t port_id, const uint64_t *ids,
 		return -EINVAL;
 
 	if (ids && dev->dev_ops->xstats_get_by_id != NULL && size) {
-		unsigned int basic_count = get_xstats_basic_count(dev);
+		unsigned int basic_count = rte_eth_basic_stats_count(dev);
 		uint64_t ids_copy[size];
 
 		for (i = 0; i < size; i++) {
diff --git a/lib/librte_ethdev/rte_ethdev_driver.h b/lib/librte_ethdev/rte_ethdev_driver.h
index 936ff8c98651..489889a72203 100644
--- a/lib/librte_ethdev/rte_ethdev_driver.h
+++ b/lib/librte_ethdev/rte_ethdev_driver.h
@@ -208,6 +208,71 @@ rte_eth_linkstatus_get(const struct rte_eth_dev *dev,
 #endif
 }
 
+/**
+ * @internal
+ * Get basic stats part of xstats for an ethernet device.
+ *
+ * @param dev
+ *  Pointer to struct rte_eth_dev.
+ */
+__rte_experimental
+int
+rte_eth_basic_stats_count(struct rte_eth_dev *dev);
+
+/**
+ * @internal
+ * @b EXPERIMENTAL: this API may change without prior notice.
+ *
+ * Retrieve the names for the basic part of extended statistics.
+ *
+ * @param dev
+ *  Pointer to struct rte_eth_dev.
+ * @param xstats_names
+ *   An rte_eth_xstat_name array of at least *size* elements to
+ *   be filled. If set to NULL, the function returns the required number
+ *   of elements.
+ * @return
+ *   - A positive value lower or equal to size: success. The return value
+ *     is the number of entries filled in the stats table.
+ *   - A positive value higher than size: error, the given statistics table
+ *     is too small. The return value corresponds to the size that should
+ *     be given to succeed. The entries in the table are not valid and
+ *     shall not be used by the caller.
+ *   - A negative value on error (invalid port id).
+ */
+__rte_experimental
+int
+rte_eth_basic_stats_get_names(struct rte_eth_dev *dev,
+			      struct rte_eth_xstat_name *xstats_names);
+
+/**
+ * @internal
+ * @b EXPERIMENTAL: this API may change without prior notice.
+ *
+ * Retrieve the basic part of the extended statistics.
+ *
+ * @param dev
+ *  Pointer to struct rte_eth_dev.
+ * @param xstats
+ *   A pointer to a table of structure of type *rte_eth_xstat*
+ *   to be filled with device statistics ids and values.
+ *   This parameter can be set to NULL if n is 0.
+ * @param n
+ *   The size of the xstats array (number of elements).
+ * @return
+ *   - A positive value lower or equal to n: success. The return value
+ *     is the number of entries filled in the stats table.
+ *   - A positive value higher than n: error, the given statistics table
+ *     is too small. The return value corresponds to the size that should
+ *     be given to succeed. The entries in the table are not valid and
+ *     shall not be used by the caller.
+ *   - A negative value on error (invalid port id).
+ */
+__rte_experimental
+int
+rte_eth_basic_stats_get(uint16_t port_id, struct rte_eth_xstat *xstats);
+
+
 /**
  * @warning
  * @b EXPERIMENTAL: this API may change without prior notice.
diff --git a/lib/librte_ethdev/rte_ethdev_version.map b/lib/librte_ethdev/rte_ethdev_version.map
index 6df42a47b89d..df7e83a90603 100644
--- a/lib/librte_ethdev/rte_ethdev_version.map
+++ b/lib/librte_ethdev/rte_ethdev_version.map
@@ -283,4 +283,9 @@ EXPERIMENTAL {
 
 	# added in 19.08
 	rte_eth_read_clock;
+
+	# added in 19.11
+	rte_eth_basic_stats_count;
+	rte_eth_basic_stats_get;
+	rte_eth_basic_stats_get_names;
 };
-- 
2.17.1

Re: [dpdk-dev] [PATCH v5 1/2] ethdev: expose basic xstats for driver use

[Andrew Rybchenko]

On 9/19/19 4:17 PM, Stephen Hemminger wrote:
> Avoid duplication by having generic basic xstats available
> for use by drivers. A later patch uses this for failsafe
> driver.
>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> Acked-by: Gaetan Rivet <gaetan.rivet@6wind.com>

Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>

> diff --git a/lib/librte_ethdev/rte_ethdev_driver.h b/lib/librte_ethdev/rte_ethdev_driver.h
> index 936ff8c98651..489889a72203 100644
> --- a/lib/librte_ethdev/rte_ethdev_driver.h
> +++ b/lib/librte_ethdev/rte_ethdev_driver.h
> @@ -208,6 +208,71 @@ rte_eth_linkstatus_get(const struct rte_eth_dev *dev,
>   #endif
>   }
>   
> +/**
> + * @internal
> + * Get basic stats part of xstats for an ethernet device.
> + *
> + * @param dev
> + *  Pointer to struct rte_eth_dev.
> + */
> +__rte_experimental

Does it make sense to mark internal API as experimental?
I thought that @internal is not a part of API/ABI.

[snip]

Re: [dpdk-dev] [PATCH v5 1/2] ethdev: expose basic xstats for driver use

[Stephen Hemminger]

On Thu, 26 Sep 2019 15:46:52 +0300
Andrew Rybchenko <arybchenko@solarflare.com> wrote:

> On 9/19/19 4:17 PM, Stephen Hemminger wrote:
> > Avoid duplication by having generic basic xstats available
> > for use by drivers. A later patch uses this for failsafe
> > driver.
> >
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > Acked-by: Gaetan Rivet <gaetan.rivet@6wind.com>  
> 
> Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
> 
> > diff --git a/lib/librte_ethdev/rte_ethdev_driver.h b/lib/librte_ethdev/rte_ethdev_driver.h
> > index 936ff8c98651..489889a72203 100644
> > --- a/lib/librte_ethdev/rte_ethdev_driver.h
> > +++ b/lib/librte_ethdev/rte_ethdev_driver.h
> > @@ -208,6 +208,71 @@ rte_eth_linkstatus_get(const struct rte_eth_dev *dev,
> >   #endif
> >   }
> >   
> > +/**
> > + * @internal
> > + * Get basic stats part of xstats for an ethernet device.
> > + *
> > + * @param dev
> > + *  Pointer to struct rte_eth_dev.
> > + */
> > +__rte_experimental  
> 
> Does it make sense to mark internal API as experimental?
> I thought that @internal is not a part of API/ABI.

agree, but checkpatch doesn't understand @internal tag.

Re: [dpdk-dev] [PATCH v5 1/2] ethdev: expose basic xstats for driver use

[Stephen Hemminger]

On Thu, 26 Sep 2019 15:46:52 +0300
Andrew Rybchenko <arybchenko@solarflare.com> wrote:

> On 9/19/19 4:17 PM, Stephen Hemminger wrote:
> > Avoid duplication by having generic basic xstats available
> > for use by drivers. A later patch uses this for failsafe
> > driver.
> >
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > Acked-by: Gaetan Rivet <gaetan.rivet@6wind.com>  
> 
> Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
> 
> > diff --git a/lib/librte_ethdev/rte_ethdev_driver.h b/lib/librte_ethdev/rte_ethdev_driver.h
> > index 936ff8c98651..489889a72203 100644
> > --- a/lib/librte_ethdev/rte_ethdev_driver.h
> > +++ b/lib/librte_ethdev/rte_ethdev_driver.h
> > @@ -208,6 +208,71 @@ rte_eth_linkstatus_get(const struct rte_eth_dev *dev,
> >   #endif
> >   }
> >   
> > +/**
> > + * @internal
> > + * Get basic stats part of xstats for an ethernet device.
> > + *
> > + * @param dev
> > + *  Pointer to struct rte_eth_dev.
> > + */
> > +__rte_experimental  
> 
> Does it make sense to mark internal API as experimental?
> I thought that @internal is not a part of API/ABI.


Checkpatch picks on it

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

[Stephen Hemminger]

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 <stephen@networkplumber.org>
Acked-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 drivers/net/failsafe/failsafe_ops.c | 135 ++++++++++++++++++++++++++++
 1 file changed, 135 insertions(+)

diff --git a/drivers/net/failsafe/failsafe_ops.c b/drivers/net/failsafe/failsafe_ops.c
index 96e05d4dc4b1..711fdf302f4f 100644
--- a/drivers/net/failsafe/failsafe_ops.c
+++ b/drivers/net/failsafe/failsafe_ops.c
@@ -789,6 +789,138 @@ fs_stats_reset(struct rte_eth_dev *dev)
 	fs_unlock(dev, 0);
 }
 
+/*
+ * Count how many xstats in total
+ * Include basic stats for failsafe device
+ * plus xstats for each sub-device present.
+ */
+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;
+
+	/* 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++) {
+			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;
+}
+
+static int
+fs_xstats_get(struct rte_eth_dev *dev,
+	      struct rte_eth_xstat *xstats,
+	      unsigned int n)
+{
+	unsigned int nstats, j, count, scount;
+	struct sub_device *sdev;
+	uint8_t i;
+	int ret;
+
+	nstats = fs_xstats_count(dev);
+	if (n < nstats || xstats == NULL)
+		return nstats;
+
+	ret = rte_eth_basic_stats_get(dev->data->port_id, xstats);
+	if (ret < 0)
+		return ret;
+
+	count = ret;
+	for (j = 0; j < count; j++)
+		xstats[j].id = j;
+
+	fs_lock(dev, 0);
+	FOREACH_SUBDEV(sdev, i, dev) {
+		ret = rte_eth_xstats_get(PORT_ID(sdev),
+					 xstats + count,
+					 n > count ? n - count : 0);
+		if (ret < 0) {
+			fs_unlock(dev, 0);
+			return ret;
+		}
+		scount = ret;
+
+		/* add offset to id's from sub-device */
+		for (j = 0; j < scount; j++)
+			xstats[count + j].id += count;
+
+		count += scount;
+	}
+	fs_unlock(dev, 0);
+
+	return count;
+}
+
+static void
+fs_xstats_reset(struct rte_eth_dev *dev)
+{
+	struct sub_device *sdev;
+	uint8_t i;
+
+	rte_eth_stats_reset(dev->data->port_id);
+
+	fs_lock(dev, 0);
+	FOREACH_SUBDEV(sdev, i, dev) {
+		rte_eth_xstats_reset(PORT_ID(sdev));
+	}
+	fs_unlock(dev, 0);
+}
+
 static void
 fs_dev_merge_desc_lim(struct rte_eth_desc_lim *to,
 		      const struct rte_eth_desc_lim *from)
@@ -1233,6 +1365,9 @@ const struct eth_dev_ops failsafe_ops = {
 	.link_update = fs_link_update,
 	.stats_get = fs_stats_get,
 	.stats_reset = fs_stats_reset,
+	.xstats_get = fs_xstats_get,
+	.xstats_get_names = fs_xstats_get_names,
+	.xstats_reset = fs_xstats_reset,
 	.dev_infos_get = fs_dev_infos_get,
 	.dev_supported_ptypes_get = fs_dev_supported_ptypes_get,
 	.mtu_set = fs_mtu_set,
-- 
2.17.1

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

[Ferruh Yigit]

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;
> +}

<...>

Re: [dpdk-dev] [PATCH v5 0/2] failsafe: add xstats

[Stephen Hemminger]

On Thu, 19 Sep 2019 15:17:27 +0200
Stephen Hemminger <stephen@networkplumber.org> wrote:

> A useful feature of netvsc PMD is the ability to see how many
> packets
> were processed through the VF device. This patch set adds a similar
> (but more limited) capability to failsafe driver.
> 
> Since failsafe doesn't have top level xstats, this set uses the generic
> xstats that exist already as a base then adds the sub-device xstats
> to that.
> 
> v5 - fix ethdev map file
> v4 - rebase to 19.11
> v3 - rebase to 19.08
> 
> Stephen Hemminger (2):
>   ethdev: expose basic xstats for driver use
>   net/failsafe: implement xstats
> 
>  drivers/net/failsafe/failsafe_ops.c      | 135 +++++++++++++++++++++++
>  lib/librte_ethdev/rte_ethdev.c           |  17 ++-
>  lib/librte_ethdev/rte_ethdev_driver.h    |  65 +++++++++++
>  lib/librte_ethdev/rte_ethdev_version.map |   5 +
>  4 files changed, 213 insertions(+), 9 deletions(-)
> 

Why are these patches still not merged?

Re: [dpdk-dev] [PATCH v5 0/2] failsafe: add xstats

[Ferruh Yigit]

On 10/14/2019 6:04 PM, Stephen Hemminger wrote:
> On Thu, 19 Sep 2019 15:17:27 +0200
> Stephen Hemminger <stephen@networkplumber.org> wrote:
> 
>> A useful feature of netvsc PMD is the ability to see how many
>> packets
>> were processed through the VF device. This patch set adds a similar
>> (but more limited) capability to failsafe driver.
>>
>> Since failsafe doesn't have top level xstats, this set uses the generic
>> xstats that exist already as a base then adds the sub-device xstats
>> to that.
>>
>> v5 - fix ethdev map file
>> v4 - rebase to 19.11
>> v3 - rebase to 19.08
>>
>> Stephen Hemminger (2):
>>   ethdev: expose basic xstats for driver use
>>   net/failsafe: implement xstats
>>
>>  drivers/net/failsafe/failsafe_ops.c      | 135 +++++++++++++++++++++++
>>  lib/librte_ethdev/rte_ethdev.c           |  17 ++-
>>  lib/librte_ethdev/rte_ethdev_driver.h    |  65 +++++++++++
>>  lib/librte_ethdev/rte_ethdev_version.map |   5 +
>>  4 files changed, 213 insertions(+), 9 deletions(-)
>>
> 
> Why are these patches still not merged?
> 

Because patches are not reviewed yet, specially ethdev one.