[dpdk-dev] [PATCH] net/failsafe: improve stats accuracy

[dpdk-dev] [PATCH] net/failsafe: improve stats accuracy

[Matan Azrad]

The stats_get API was changed to return error in case of failure at
stats getting process time.
By this way, failsafe can get stats snapshot in removal process for
each PMD which can give stats after removal event.

This patch implements ultimate stats snapshot on removal time by
trying to get the removed device stats before calling to dev_close.

Signed-off-by: Matan Azrad <matan@mellanox.com>
---
 drivers/net/failsafe/failsafe_ether.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/failsafe/failsafe_ether.c b/drivers/net/failsafe/failsafe_ether.c
index f4db423..2758d4c 100644
--- a/drivers/net/failsafe/failsafe_ether.c
+++ b/drivers/net/failsafe/failsafe_ether.c
@@ -312,8 +312,12 @@
 static void
 fs_dev_stats_save(struct sub_device *sdev)
 {
+	struct rte_eth_stats stats;
+
+	/* Get stats now or take it from last snapshot. */
 	failsafe_stats_increment(&PRIV(sdev->fs_dev)->stats_accumulator,
-			&sdev->stats_snapshot);
+		rte_eth_stats_get(PORT_ID(sdev), &stats) ?
+		&sdev->stats_snapshot : &stats);
 	memset(&sdev->stats_snapshot, 0, sizeof(struct rte_eth_stats));
 }
 
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH] net/failsafe: improve stats accuracy

[Gaëtan Rivet]

Hey Matan,

Thanks for the patch. Here are some remarks:

On Mon, Oct 16, 2017 at 07:41:50AM +0000, Matan Azrad wrote:
> The stats_get API was changed to return error in case of failure at
> stats getting process time.
> By this way, failsafe can get stats snapshot in removal process for
> each PMD which can give stats after removal event.
> 
> This patch implements ultimate stats snapshot on removal time by
> trying to get the removed device stats before calling to dev_close.
> 

I would reformulate the commit log as such:

The stats_get API has changed to signal a potential failure to read
stats. Furthermore, some PMDs are able to provide statistics even
after a removal event occured.

Considering this, the fail-safe can try to access the latest statistics
of a PMD to improve statistics readings.

Attempt an ultimate statistics read on removal time;
if that fails, use the latest recorded snapshot.

> Signed-off-by: Matan Azrad <matan@mellanox.com>
> ---
>  drivers/net/failsafe/failsafe_ether.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/failsafe/failsafe_ether.c b/drivers/net/failsafe/failsafe_ether.c
> index f4db423..2758d4c 100644
> --- a/drivers/net/failsafe/failsafe_ether.c
> +++ b/drivers/net/failsafe/failsafe_ether.c
> @@ -312,8 +312,12 @@
>  static void
>  fs_dev_stats_save(struct sub_device *sdev)
>  {
> +	struct rte_eth_stats stats;
> +
> +	/* Get stats now or take it from last snapshot. */
>  	failsafe_stats_increment(&PRIV(sdev->fs_dev)->stats_accumulator,
> -			&sdev->stats_snapshot);
> +		rte_eth_stats_get(PORT_ID(sdev), &stats) ?
> +		&sdev->stats_snapshot : &stats);

The conditional is awkward within the parameter list here.
Something like:

    struct rte_eth_stats stats;
    int err;

    /* Attempt to read current stats. */
    err = rte_eth_stats_get(PORT_ID(sdev), &stats);
    if (err)
        WARN("Could not access latest statistics from sub-device %d, using latest snapshot",
             SUB_ID(sdev));
    /* Use either current stats or latest snapshot. */
    failsafe_stats_increment(&PRIV(sdev->fs_dev)->stats_accumulator,
                             err ? &sdev->stats_snapshot : &stats);

Seems easier to read and allows warning the user of potential inaccuracies.
On that matter, it may be interesting to measure the time since the last snapshot
and display it as well.

>  	memset(&sdev->stats_snapshot, 0, sizeof(struct rte_eth_stats));
>  }
>  
> -- 
> 1.8.3.1
> 

-- 
Gaëtan Rivet
6WIND

Re: [dpdk-dev] [PATCH] net/failsafe: improve stats accuracy

[Matan Azrad]

Hi Geatan
> -----Original Message-----
> From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com]
> Sent: Monday, October 16, 2017 11:28 AM
> To: Matan Azrad <matan@mellanox.com>
> Cc: dev@dpdk.org
> Subject: Re: [PATCH] net/failsafe: improve stats accuracy
> 
> Hey Matan,
> 
> Thanks for the patch. Here are some remarks:
> 
> On Mon, Oct 16, 2017 at 07:41:50AM +0000, Matan Azrad wrote:
> > The stats_get API was changed to return error in case of failure at
> > stats getting process time.
> > By this way, failsafe can get stats snapshot in removal process for
> > each PMD which can give stats after removal event.
> >
> > This patch implements ultimate stats snapshot on removal time by
> > trying to get the removed device stats before calling to dev_close.
> >
> 
> I would reformulate the commit log as such:
> 
> The stats_get API has changed to signal a potential failure to read stats.
> Furthermore, some PMDs are able to provide statistics even after a removal
> event occured.
> 
> Considering this, the fail-safe can try to access the latest statistics of a PMD to
> improve statistics readings.
> 
> Attempt an ultimate statistics read on removal time; if that fails, use the
> latest recorded snapshot.
> 
Looks ok for me, thanks.

> > Signed-off-by: Matan Azrad <matan@mellanox.com>
> > ---
> >  drivers/net/failsafe/failsafe_ether.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/failsafe/failsafe_ether.c
> > b/drivers/net/failsafe/failsafe_ether.c
> > index f4db423..2758d4c 100644
> > --- a/drivers/net/failsafe/failsafe_ether.c
> > +++ b/drivers/net/failsafe/failsafe_ether.c
> > @@ -312,8 +312,12 @@
> >  static void
> >  fs_dev_stats_save(struct sub_device *sdev)  {
> > +	struct rte_eth_stats stats;
> > +
> > +	/* Get stats now or take it from last snapshot. */
> >  	failsafe_stats_increment(&PRIV(sdev->fs_dev)->stats_accumulator,
> > -			&sdev->stats_snapshot);
> > +		rte_eth_stats_get(PORT_ID(sdev), &stats) ?
> > +		&sdev->stats_snapshot : &stats);
> 
> The conditional is awkward within the parameter list here.
> Something like:
> 
>     struct rte_eth_stats stats;
>     int err;
> 
>     /* Attempt to read current stats. */
>     err = rte_eth_stats_get(PORT_ID(sdev), &stats);
>     if (err)
>         WARN("Could not access latest statistics from sub-device %d, using latest
> snapshot",
>              SUB_ID(sdev));
>     /* Use either current stats or latest snapshot. */
>     failsafe_stats_increment(&PRIV(sdev->fs_dev)->stats_accumulator,
>                              err ? &sdev->stats_snapshot : &stats);
> 
> Seems easier to read and allows warning the user of potential inaccuracies.
> On that matter, it may be interesting to measure the time since the last
> snapshot and display it as well.
>
OK, I think it is good idea, I will try to add this here.
 
> >  	memset(&sdev->stats_snapshot, 0, sizeof(struct rte_eth_stats));  }
> >
> > --
> > 1.8.3.1
> >
> 
> --
> Gaëtan Rivet
> 6WIND

[dpdk-dev] [PATCH v2] net/failsafe: improve stats accuracy

[Matan Azrad]

The stats_get API was changed to signal a potential failure to read
stats. Furthermore, some PMDs are able to provide statistics even after
a removal event occurred.

Considering this, the fail-safe can try to access the latest statistics
of a PMD to improve statistics accuracy.

Attempt an ultimate statistics read on removal time; if that fails, use
the latest recorded snapshot.

Signed-off-by: Matan Azrad <matan@mellanox.com>
---
 drivers/net/failsafe/failsafe_ether.c   | 19 +++++++++++++++++--
 drivers/net/failsafe/failsafe_ops.c     | 10 ++++++++--
 drivers/net/failsafe/failsafe_private.h |  7 ++++++-
 3 files changed, 31 insertions(+), 5 deletions(-)

diff --git a/drivers/net/failsafe/failsafe_ether.c b/drivers/net/failsafe/failsafe_ether.c
index f4db423..df38360 100644
--- a/drivers/net/failsafe/failsafe_ether.c
+++ b/drivers/net/failsafe/failsafe_ether.c
@@ -35,6 +35,7 @@
 
 #include <rte_flow.h>
 #include <rte_flow_driver.h>
+#include <rte_cycles.h>
 
 #include "failsafe_private.h"
 
@@ -312,9 +313,23 @@
 static void
 fs_dev_stats_save(struct sub_device *sdev)
 {
+	struct rte_eth_stats stats;
+	int err;
+
+	/* Attempt to read current stats. */
+	err = rte_eth_stats_get(PORT_ID(sdev), &stats);
+	if (err) {
+		uint64_t cycles = sdev->stats_snapshot.cycles;
+
+		WARN("Could not access latest statistics from sub-device %d.\n",
+			 SUB_ID(sdev));
+		if (cycles != 0)
+			WARN("Using latest snapshot taken before %lu seconds.\n",
+				 (rte_rdtsc() - cycles) / rte_get_tsc_hz());
+	}
 	failsafe_stats_increment(&PRIV(sdev->fs_dev)->stats_accumulator,
-			&sdev->stats_snapshot);
-	memset(&sdev->stats_snapshot, 0, sizeof(struct rte_eth_stats));
+			err ? &sdev->stats_snapshot.stats : &stats);
+	memset(&sdev->stats_snapshot, 0, sizeof(sdev->stats_snapshot));
 }
 
 static inline int
diff --git a/drivers/net/failsafe/failsafe_ops.c b/drivers/net/failsafe/failsafe_ops.c
index d360965..818f12d 100644
--- a/drivers/net/failsafe/failsafe_ops.c
+++ b/drivers/net/failsafe/failsafe_ops.c
@@ -38,6 +38,7 @@
 #include <rte_ethdev.h>
 #include <rte_malloc.h>
 #include <rte_flow.h>
+#include <rte_cycles.h>
 
 #include "failsafe_private.h"
 
@@ -592,13 +593,18 @@
 
 	rte_memcpy(stats, &PRIV(dev)->stats_accumulator, sizeof(*stats));
 	FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) {
-		ret = rte_eth_stats_get(PORT_ID(sdev), &sdev->stats_snapshot);
+		struct rte_eth_stats *snapshot = &sdev->stats_snapshot.stats;
+		uint64_t *cycles = &sdev->stats_snapshot.cycles;
+
+		ret = rte_eth_stats_get(PORT_ID(sdev), snapshot);
 		if (ret) {
 			ERROR("Operation rte_eth_stats_get failed for sub_device %d with error %d",
 				  i, ret);
+			*cycles = 0;
 			return ret;
 		}
-		failsafe_stats_increment(stats, &sdev->stats_snapshot);
+		*cycles = rte_rdtsc();
+		failsafe_stats_increment(stats, snapshot);
 	}
 	return 0;
 }
diff --git a/drivers/net/failsafe/failsafe_private.h b/drivers/net/failsafe/failsafe_private.h
index d343ebf..1df52f4 100644
--- a/drivers/net/failsafe/failsafe_private.h
+++ b/drivers/net/failsafe/failsafe_private.h
@@ -93,6 +93,11 @@ enum dev_state {
 	DEV_STARTED,
 };
 
+struct fs_stats {
+	struct rte_eth_stats stats;
+	uint64_t cycles;
+};
+
 struct sub_device {
 	/* Exhaustive DPDK device description */
 	struct rte_devargs devargs;
@@ -103,7 +108,7 @@ struct sub_device {
 	/* Device state machine */
 	enum dev_state state;
 	/* Last stats snapshot passed to user */
-	struct rte_eth_stats stats_snapshot;
+	struct fs_stats stats_snapshot;
 	/* Some device are defined as a command line */
 	char *cmdline;
 	/* fail-safe device backreference */
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH v2] net/failsafe: improve stats accuracy

[Gaëtan Rivet]

Hello Matan,

Adding the time delta should have been done in a separate commit.
Can you please divide this patch in two?

The first one will only attempt the ultimate stat read, the second one
would add the delay warning.

Small nit below for your v3.

On Thu, Oct 19, 2017 at 02:31:54PM +0000, Matan Azrad wrote:
> The stats_get API was changed to signal a potential failure to read
> stats. Furthermore, some PMDs are able to provide statistics even after
> a removal event occurred.
> 
> Considering this, the fail-safe can try to access the latest statistics
> of a PMD to improve statistics accuracy.
> 
> Attempt an ultimate statistics read on removal time; if that fails, use
> the latest recorded snapshot.
> 
> Signed-off-by: Matan Azrad <matan@mellanox.com>
> ---
>  drivers/net/failsafe/failsafe_ether.c   | 19 +++++++++++++++++--
>  drivers/net/failsafe/failsafe_ops.c     | 10 ++++++++--
>  drivers/net/failsafe/failsafe_private.h |  7 ++++++-
>  3 files changed, 31 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/failsafe/failsafe_ether.c b/drivers/net/failsafe/failsafe_ether.c
> index f4db423..df38360 100644
> --- a/drivers/net/failsafe/failsafe_ether.c
> +++ b/drivers/net/failsafe/failsafe_ether.c
> @@ -35,6 +35,7 @@
>  
>  #include <rte_flow.h>
>  #include <rte_flow_driver.h>
> +#include <rte_cycles.h>
>  
>  #include "failsafe_private.h"
>  
> @@ -312,9 +313,23 @@
>  static void
>  fs_dev_stats_save(struct sub_device *sdev)
>  {
> +	struct rte_eth_stats stats;
> +	int err;
> +
> +	/* Attempt to read current stats. */
> +	err = rte_eth_stats_get(PORT_ID(sdev), &stats);
> +	if (err) {
> +		uint64_t cycles = sdev->stats_snapshot.cycles;
> +
> +		WARN("Could not access latest statistics from sub-device %d.\n",
> +			 SUB_ID(sdev));
> +		if (cycles != 0)
> +			WARN("Using latest snapshot taken before %lu seconds.\n",
> +				 (rte_rdtsc() - cycles) / rte_get_tsc_hz());
> +	}
>  	failsafe_stats_increment(&PRIV(sdev->fs_dev)->stats_accumulator,
> -			&sdev->stats_snapshot);
> -	memset(&sdev->stats_snapshot, 0, sizeof(struct rte_eth_stats));
> +			err ? &sdev->stats_snapshot.stats : &stats);
> +	memset(&sdev->stats_snapshot, 0, sizeof(sdev->stats_snapshot));
>  }
>  
>  static inline int
> diff --git a/drivers/net/failsafe/failsafe_ops.c b/drivers/net/failsafe/failsafe_ops.c
> index d360965..818f12d 100644
> --- a/drivers/net/failsafe/failsafe_ops.c
> +++ b/drivers/net/failsafe/failsafe_ops.c
> @@ -38,6 +38,7 @@
>  #include <rte_ethdev.h>
>  #include <rte_malloc.h>
>  #include <rte_flow.h>
> +#include <rte_cycles.h>
>  
>  #include "failsafe_private.h"
>  
> @@ -592,13 +593,18 @@
>  
>  	rte_memcpy(stats, &PRIV(dev)->stats_accumulator, sizeof(*stats));
>  	FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) {
> -		ret = rte_eth_stats_get(PORT_ID(sdev), &sdev->stats_snapshot);
> +		struct rte_eth_stats *snapshot = &sdev->stats_snapshot.stats;
> +		uint64_t *cycles = &sdev->stats_snapshot.cycles;
> +
> +		ret = rte_eth_stats_get(PORT_ID(sdev), snapshot);
>  		if (ret) {
>  			ERROR("Operation rte_eth_stats_get failed for sub_device %d with error %d",
>  				  i, ret);
> +			*cycles = 0;
>  			return ret;
>  		}
> -		failsafe_stats_increment(stats, &sdev->stats_snapshot);
> +		*cycles = rte_rdtsc();
> +		failsafe_stats_increment(stats, snapshot);
>  	}
>  	return 0;
>  }
> diff --git a/drivers/net/failsafe/failsafe_private.h b/drivers/net/failsafe/failsafe_private.h
> index d343ebf..1df52f4 100644
> --- a/drivers/net/failsafe/failsafe_private.h
> +++ b/drivers/net/failsafe/failsafe_private.h
> @@ -93,6 +93,11 @@ enum dev_state {
>  	DEV_STARTED,
>  };
>  
> +struct fs_stats {
> +	struct rte_eth_stats stats;
> +	uint64_t cycles;

What do you think of the name "timestamp" for this field?
It would be more descriptive of its use.

> +};
> +
>  struct sub_device {
>  	/* Exhaustive DPDK device description */
>  	struct rte_devargs devargs;
> @@ -103,7 +108,7 @@ struct sub_device {
>  	/* Device state machine */
>  	enum dev_state state;
>  	/* Last stats snapshot passed to user */
> -	struct rte_eth_stats stats_snapshot;
> +	struct fs_stats stats_snapshot;
>  	/* Some device are defined as a command line */
>  	char *cmdline;
>  	/* fail-safe device backreference */
> -- 
> 1.8.3.1
> 

Thanks,

-- 
Gaëtan Rivet
6WIND

[dpdk-dev] [PATCH v3 1/2] net/failsafe: improve stats accuracy

[Matan Azrad]

The stats_get API was changed to signal a potential failure to read
stats. Furthermore, some PMDs are able to provide statistics even
after a removal event occurred.

Considering this, the fail-safe can try to access the latest
statistics of a PMD to improve statistics accuracy.

Attempt an ultimate statistics read on removal time; if that
fails, use the latest recorded snapshot.

Signed-off-by: Matan Azrad <matan@mellanox.com>
---
 drivers/net/failsafe/failsafe_ether.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

V2:
Improve commit message.
Add warning massage when using stats snapshot.
Add time report from last snapshot.

V3:
Separate patch.
Replace "cycles" by "timestamp".

diff --git a/drivers/net/failsafe/failsafe_ether.c b/drivers/net/failsafe/failsafe_ether.c
index f4db423..0282891 100644
--- a/drivers/net/failsafe/failsafe_ether.c
+++ b/drivers/net/failsafe/failsafe_ether.c
@@ -312,8 +312,16 @@
 static void
 fs_dev_stats_save(struct sub_device *sdev)
 {
+	struct rte_eth_stats stats;
+	int err;
+
+	/* Attempt to read current stats. */
+	err = rte_eth_stats_get(PORT_ID(sdev), &stats);
+	if (err)
+		WARN("Could not access latest statistics from sub-device %d,"
+			 " using latest snapshot.\n", SUB_ID(sdev));
 	failsafe_stats_increment(&PRIV(sdev->fs_dev)->stats_accumulator,
-			&sdev->stats_snapshot);
+			err ? &sdev->stats_snapshot : &stats);
 	memset(&sdev->stats_snapshot, 0, sizeof(struct rte_eth_stats));
 }
 
-- 
1.8.3.1

[dpdk-dev] [PATCH v3 2/2] net/failsafe: add timestamp to stats snapshot

[Matan Azrad]

Fail-safe attempts to read an ultimate statistics on removal time; if
that fails, it uses the latest recorded snapshot.

This patch adds timestamp for each stats snapshot to allow a time report
since the last snapshot in case of the above failure.

By this way, the user can estimate the stats read accuracy.

Signed-off-by: Matan Azrad <matan@mellanox.com>
---
 drivers/net/failsafe/failsafe_ether.c   | 17 ++++++++++++-----
 drivers/net/failsafe/failsafe_ops.c     | 10 ++++++++--
 drivers/net/failsafe/failsafe_private.h |  7 ++++++-
 3 files changed, 26 insertions(+), 8 deletions(-)

diff --git a/drivers/net/failsafe/failsafe_ether.c b/drivers/net/failsafe/failsafe_ether.c
index 0282891..6cb435f 100644
--- a/drivers/net/failsafe/failsafe_ether.c
+++ b/drivers/net/failsafe/failsafe_ether.c
@@ -35,6 +35,7 @@
 
 #include <rte_flow.h>
 #include <rte_flow_driver.h>
+#include <rte_cycles.h>
 
 #include "failsafe_private.h"
 
@@ -317,12 +318,18 @@
 
 	/* Attempt to read current stats. */
 	err = rte_eth_stats_get(PORT_ID(sdev), &stats);
-	if (err)
-		WARN("Could not access latest statistics from sub-device %d,"
-			 " using latest snapshot.\n", SUB_ID(sdev));
+	if (err) {
+		uint64_t timestamp = sdev->stats_snapshot.timestamp;
+
+		WARN("Could not access latest statistics from sub-device %d.\n",
+			 SUB_ID(sdev));
+		if (timestamp != 0)
+			WARN("Using latest snapshot taken before %lu seconds.\n",
+				 (rte_rdtsc() - timestamp) / rte_get_tsc_hz());
+	}
 	failsafe_stats_increment(&PRIV(sdev->fs_dev)->stats_accumulator,
-			err ? &sdev->stats_snapshot : &stats);
-	memset(&sdev->stats_snapshot, 0, sizeof(struct rte_eth_stats));
+			err ? &sdev->stats_snapshot.stats : &stats);
+	memset(&sdev->stats_snapshot, 0, sizeof(sdev->stats_snapshot));
 }
 
 static inline int
diff --git a/drivers/net/failsafe/failsafe_ops.c b/drivers/net/failsafe/failsafe_ops.c
index d360965..f460551 100644
--- a/drivers/net/failsafe/failsafe_ops.c
+++ b/drivers/net/failsafe/failsafe_ops.c
@@ -38,6 +38,7 @@
 #include <rte_ethdev.h>
 #include <rte_malloc.h>
 #include <rte_flow.h>
+#include <rte_cycles.h>
 
 #include "failsafe_private.h"
 
@@ -592,13 +593,18 @@
 
 	rte_memcpy(stats, &PRIV(dev)->stats_accumulator, sizeof(*stats));
 	FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) {
-		ret = rte_eth_stats_get(PORT_ID(sdev), &sdev->stats_snapshot);
+		struct rte_eth_stats *snapshot = &sdev->stats_snapshot.stats;
+		uint64_t *timestamp = &sdev->stats_snapshot.timestamp;
+
+		ret = rte_eth_stats_get(PORT_ID(sdev), snapshot);
 		if (ret) {
 			ERROR("Operation rte_eth_stats_get failed for sub_device %d with error %d",
 				  i, ret);
+			*timestamp = 0;
 			return ret;
 		}
-		failsafe_stats_increment(stats, &sdev->stats_snapshot);
+		*timestamp = rte_rdtsc();
+		failsafe_stats_increment(stats, snapshot);
 	}
 	return 0;
 }
diff --git a/drivers/net/failsafe/failsafe_private.h b/drivers/net/failsafe/failsafe_private.h
index d343ebf..d81cc3c 100644
--- a/drivers/net/failsafe/failsafe_private.h
+++ b/drivers/net/failsafe/failsafe_private.h
@@ -93,6 +93,11 @@ enum dev_state {
 	DEV_STARTED,
 };
 
+struct fs_stats {
+	struct rte_eth_stats stats;
+	uint64_t timestamp;
+};
+
 struct sub_device {
 	/* Exhaustive DPDK device description */
 	struct rte_devargs devargs;
@@ -103,7 +108,7 @@ struct sub_device {
 	/* Device state machine */
 	enum dev_state state;
 	/* Last stats snapshot passed to user */
-	struct rte_eth_stats stats_snapshot;
+	struct fs_stats stats_snapshot;
 	/* Some device are defined as a command line */
 	char *cmdline;
 	/* fail-safe device backreference */
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH v3 1/2] net/failsafe: improve stats accuracy

[Gaëtan Rivet]

Thanks Matan,

On Sat, Oct 21, 2017 at 08:54:45PM +0000, Matan Azrad wrote:
> The stats_get API was changed to signal a potential failure to read
> stats. Furthermore, some PMDs are able to provide statistics even
> after a removal event occurred.
> 
> Considering this, the fail-safe can try to access the latest
> statistics of a PMD to improve statistics accuracy.
> 
> Attempt an ultimate statistics read on removal time; if that
> fails, use the latest recorded snapshot.
> 
> Signed-off-by: Matan Azrad <matan@mellanox.com>
Acked-by: Gaetan Rivet <gaetan.rivet@6wind.com>
> ---
>  drivers/net/failsafe/failsafe_ether.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> V2:
> Improve commit message.
> Add warning massage when using stats snapshot.
> Add time report from last snapshot.
> 
> V3:
> Separate patch.
> Replace "cycles" by "timestamp".
> 
> diff --git a/drivers/net/failsafe/failsafe_ether.c b/drivers/net/failsafe/failsafe_ether.c
> index f4db423..0282891 100644
> --- a/drivers/net/failsafe/failsafe_ether.c
> +++ b/drivers/net/failsafe/failsafe_ether.c
> @@ -312,8 +312,16 @@
>  static void
>  fs_dev_stats_save(struct sub_device *sdev)
>  {
> +	struct rte_eth_stats stats;
> +	int err;
> +
> +	/* Attempt to read current stats. */
> +	err = rte_eth_stats_get(PORT_ID(sdev), &stats);
> +	if (err)
> +		WARN("Could not access latest statistics from sub-device %d,"
> +			 " using latest snapshot.\n", SUB_ID(sdev));
>  	failsafe_stats_increment(&PRIV(sdev->fs_dev)->stats_accumulator,
> -			&sdev->stats_snapshot);
> +			err ? &sdev->stats_snapshot : &stats);
>  	memset(&sdev->stats_snapshot, 0, sizeof(struct rte_eth_stats));
>  }
>  
> -- 
> 1.8.3.1
> 

-- 
Gaëtan Rivet
6WIND

Re: [dpdk-dev] [PATCH v3 2/2] net/failsafe: add timestamp to stats snapshot

[Gaëtan Rivet]

On Sat, Oct 21, 2017 at 08:54:46PM +0000, Matan Azrad wrote:
> Fail-safe attempts to read an ultimate statistics on removal time; if
> that fails, it uses the latest recorded snapshot.
> 
> This patch adds timestamp for each stats snapshot to allow a time report
> since the last snapshot in case of the above failure.
> 
> By this way, the user can estimate the stats read accuracy.
> 
> Signed-off-by: Matan Azrad <matan@mellanox.com>
Acked-by: Gaetan Rivet <gaetan.rivet@6wind.com>
> ---
>  drivers/net/failsafe/failsafe_ether.c   | 17 ++++++++++++-----
>  drivers/net/failsafe/failsafe_ops.c     | 10 ++++++++--
>  drivers/net/failsafe/failsafe_private.h |  7 ++++++-
>  3 files changed, 26 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/failsafe/failsafe_ether.c b/drivers/net/failsafe/failsafe_ether.c
> index 0282891..6cb435f 100644
> --- a/drivers/net/failsafe/failsafe_ether.c
> +++ b/drivers/net/failsafe/failsafe_ether.c
> @@ -35,6 +35,7 @@
>  
>  #include <rte_flow.h>
>  #include <rte_flow_driver.h>
> +#include <rte_cycles.h>
>  
>  #include "failsafe_private.h"
>  
> @@ -317,12 +318,18 @@
>  
>  	/* Attempt to read current stats. */
>  	err = rte_eth_stats_get(PORT_ID(sdev), &stats);
> -	if (err)
> -		WARN("Could not access latest statistics from sub-device %d,"
> -			 " using latest snapshot.\n", SUB_ID(sdev));
> +	if (err) {
> +		uint64_t timestamp = sdev->stats_snapshot.timestamp;
> +
> +		WARN("Could not access latest statistics from sub-device %d.\n",
> +			 SUB_ID(sdev));
> +		if (timestamp != 0)
> +			WARN("Using latest snapshot taken before %lu seconds.\n",
> +				 (rte_rdtsc() - timestamp) / rte_get_tsc_hz());
> +	}
>  	failsafe_stats_increment(&PRIV(sdev->fs_dev)->stats_accumulator,
> -			err ? &sdev->stats_snapshot : &stats);
> -	memset(&sdev->stats_snapshot, 0, sizeof(struct rte_eth_stats));
> +			err ? &sdev->stats_snapshot.stats : &stats);
> +	memset(&sdev->stats_snapshot, 0, sizeof(sdev->stats_snapshot));
>  }
>  
>  static inline int
> diff --git a/drivers/net/failsafe/failsafe_ops.c b/drivers/net/failsafe/failsafe_ops.c
> index d360965..f460551 100644
> --- a/drivers/net/failsafe/failsafe_ops.c
> +++ b/drivers/net/failsafe/failsafe_ops.c
> @@ -38,6 +38,7 @@
>  #include <rte_ethdev.h>
>  #include <rte_malloc.h>
>  #include <rte_flow.h>
> +#include <rte_cycles.h>
>  
>  #include "failsafe_private.h"
>  
> @@ -592,13 +593,18 @@
>  
>  	rte_memcpy(stats, &PRIV(dev)->stats_accumulator, sizeof(*stats));
>  	FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) {
> -		ret = rte_eth_stats_get(PORT_ID(sdev), &sdev->stats_snapshot);
> +		struct rte_eth_stats *snapshot = &sdev->stats_snapshot.stats;
> +		uint64_t *timestamp = &sdev->stats_snapshot.timestamp;
> +
> +		ret = rte_eth_stats_get(PORT_ID(sdev), snapshot);
>  		if (ret) {
>  			ERROR("Operation rte_eth_stats_get failed for sub_device %d with error %d",
>  				  i, ret);
> +			*timestamp = 0;
>  			return ret;
>  		}
> -		failsafe_stats_increment(stats, &sdev->stats_snapshot);
> +		*timestamp = rte_rdtsc();
> +		failsafe_stats_increment(stats, snapshot);
>  	}
>  	return 0;
>  }
> diff --git a/drivers/net/failsafe/failsafe_private.h b/drivers/net/failsafe/failsafe_private.h
> index d343ebf..d81cc3c 100644
> --- a/drivers/net/failsafe/failsafe_private.h
> +++ b/drivers/net/failsafe/failsafe_private.h
> @@ -93,6 +93,11 @@ enum dev_state {
>  	DEV_STARTED,
>  };
>  
> +struct fs_stats {
> +	struct rte_eth_stats stats;
> +	uint64_t timestamp;
> +};
> +
>  struct sub_device {
>  	/* Exhaustive DPDK device description */
>  	struct rte_devargs devargs;
> @@ -103,7 +108,7 @@ struct sub_device {
>  	/* Device state machine */
>  	enum dev_state state;
>  	/* Last stats snapshot passed to user */
> -	struct rte_eth_stats stats_snapshot;
> +	struct fs_stats stats_snapshot;
>  	/* Some device are defined as a command line */
>  	char *cmdline;
>  	/* fail-safe device backreference */
> -- 
> 1.8.3.1
> 

-- 
Gaëtan Rivet
6WIND

Re: [dpdk-dev] [PATCH v3 1/2] net/failsafe: improve stats accuracy

[Ferruh Yigit]

On 10/23/2017 1:46 AM, Gaëtan Rivet wrote:
> Thanks Matan,
> 
> On Sat, Oct 21, 2017 at 08:54:45PM +0000, Matan Azrad wrote:
>> The stats_get API was changed to signal a potential failure to read
>> stats. Furthermore, some PMDs are able to provide statistics even
>> after a removal event occurred.
>>
>> Considering this, the fail-safe can try to access the latest
>> statistics of a PMD to improve statistics accuracy.
>>
>> Attempt an ultimate statistics read on removal time; if that
>> fails, use the latest recorded snapshot.
>>
>> Signed-off-by: Matan Azrad <matan@mellanox.com>
> Acked-by: Gaetan Rivet <gaetan.rivet@6wind.com>

Series applied to dpdk-next-net/master, thanks.


(Fixed build error for 32bits [1] while applying, please check.)

[1]
.../dpdk/drivers/net/failsafe/failsafe_ether.c: In function ‘fs_dev_stats_save’:
.../dpdk/drivers/net/failsafe/failsafe_ether.c:328:50: error: format ‘%lu’
expects argument of type ‘long unsigned int’, but argument 4 has type ‘uint64_t
{aka long long unsigned int}’ [-Werror=format=]
      (rte_rdtsc() - timestamp) / rte_get_tsc_hz());
                                                  ^
.../dpdk/i686-native-linuxapp-gcc/include/rte_log.h:345:25: note: in definition
of macro ‘RTE_LOG’
    RTE_LOGTYPE_ ## t, # t ": " __VA_ARGS__)
                         ^
.../dpdk/drivers/net/failsafe/failsafe_private.h:299:26: note: in expansion of
macro ‘LOG__’
 #define LOG_(level, ...) LOG__(level, __VA_ARGS__, '\n')
                          ^~~~~
.../dpdk/drivers/net/failsafe/failsafe_private.h:302:19: note: in expansion of
macro ‘LOG_’
 #define WARN(...) LOG_(WARNING, __VA_ARGS__)
                   ^~~~
.../dpdk/drivers/net/failsafe/failsafe_ether.c:327:4: note: in expansion of
macro ‘WARN’
    WARN("Using latest snapshot taken before %lu seconds.\n",
    ^~~~