[dpdk-dev] [PATCH v2] mlx5: Report imissed stat

[dpdk-dev] [PATCH v2] mlx5: Report imissed stat

[Tom Barbette]

The imissed counters (number of packets dropped because the queues were
full) were actually reported through xstats as "rx_out_of_buffer"
but was not reported through stats.

Following a recent discussion on the ML, as there is no way to tell the
user if a counter is implemented or not, this should be considered a
bug. Eg, user looking at imissed will think the packets are lost before
reaching the device.

As for xstats, I added a base counter to be able to "reset" imissed.

v2:
 - Add rx_discards_phy xstats counter
 - Add documentation
 - Follow suggestions from Shahaf Shuler <shahafs@mellanox.com> as per
   v1 comments

Signed-off-by: Tom Barbette <barbette@kth.se>
---
 doc/guides/nics/mlx5.rst        |  2 +-
 drivers/net/mlx5/mlx5.h         |  8 ++++++-
 drivers/net/mlx5/mlx5_stats.c   | 53 ++++++++++++++++++++++++++++-------------
 drivers/net/mlx5/mlx5_trigger.c |  2 +-
 4 files changed, 46 insertions(+), 19 deletions(-)

diff --git a/doc/guides/nics/mlx5.rst b/doc/guides/nics/mlx5.rst
index 52e1213..ddc0fdd 100644
--- a/doc/guides/nics/mlx5.rst
+++ b/doc/guides/nics/mlx5.rst
@@ -141,7 +141,7 @@ Statistics
 
 MLX5 supports various of methods to report statistics:
 
-Port statistics can be queried using ``rte_eth_stats_get()``. The port statistics are through SW only and counts the number of packets received or sent successfully by the PMD.
+Port statistics can be queried using ``rte_eth_stats_get()``. The received and sent statistics are through SW only and counts the number of packets received or sent successfully by the PMD. The imissed counter is the amount of packets that could not be delivered to SW because a queue was full. Packets not received due to congestion in the bus or on the NIC can be queried via the rx_discards_phy xstats counter.
 
 Extended statistics can be queried using ``rte_eth_xstats_get()``. The extended statistics expose a wider set of counters counted by the device. The extended port statistics counts the number of packets received or sent successfully by the port. As Mellanox NICs are using the :ref:`Bifurcated Linux Driver <linux_gsg_linux_drivers>` those counters counts also packet received or sent by the Linux kernel. The counters with ``_phy`` suffix counts the total events on the physical port, therefore not valid for VF.
 
diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h
index a3a34cf..1aaaafe 100644
--- a/drivers/net/mlx5/mlx5.h
+++ b/drivers/net/mlx5/mlx5.h
@@ -79,6 +79,11 @@ struct mlx5_xstats_ctrl {
 	uint64_t base[MLX5_MAX_XSTATS];
 };
 
+struct mlx5_stats_ctrl {
+	/* Base for imissed counter. */
+	uint64_t imissed_base;
+};
+
 /* Flow list . */
 TAILQ_HEAD(mlx5_flows, rte_flow);
 
@@ -214,6 +219,7 @@ struct priv {
 	LIST_HEAD(ind_tables, mlx5_ind_table_ibv) ind_tbls;
 	uint32_t link_speed_capa; /* Link speed capabilities. */
 	struct mlx5_xstats_ctrl xstats_ctrl; /* Extended stats control. */
+	struct mlx5_stats_ctrl stats_ctrl; /* Stats control. */
 	int primary_socket; /* Unix socket for primary process. */
 	void *uar_base; /* Reserved address space for UAR mapping */
 	struct rte_intr_handle intr_handle_socket; /* Interrupt handler. */
@@ -309,7 +315,7 @@ void mlx5_allmulticast_disable(struct rte_eth_dev *dev);
 
 /* mlx5_stats.c */
 
-void mlx5_xstats_init(struct rte_eth_dev *dev);
+void mlx5_stats_init(struct rte_eth_dev *dev);
 int mlx5_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats);
 void mlx5_stats_reset(struct rte_eth_dev *dev);
 int mlx5_xstats_get(struct rte_eth_dev *dev, struct rte_eth_xstat *stats,
diff --git a/drivers/net/mlx5/mlx5_stats.c b/drivers/net/mlx5/mlx5_stats.c
index 91f3d47..323f8fb 100644
--- a/drivers/net/mlx5/mlx5_stats.c
+++ b/drivers/net/mlx5/mlx5_stats.c
@@ -108,6 +108,14 @@ static const struct mlx5_counter_ctrl mlx5_counters_init[] = {
 		.ctr_name = "rx_packets_phy",
 	},
 	{
+		.dpdk_name = "tx_discards_phy",
+		.ctr_name = "tx_discards_phy",
+	},
+	{
+		.dpdk_name = "rx_discards_phy",
+		.ctr_name = "rx_discards_phy",
+	},
+	{
 		.dpdk_name = "tx_bytes_phy",
 		.ctr_name = "tx_bytes_phy",
 	},
@@ -119,6 +127,24 @@ static const struct mlx5_counter_ctrl mlx5_counters_init[] = {
 
 static const unsigned int xstats_n = RTE_DIM(mlx5_counters_init);
 
+static inline void
+mlx5_read_ib_stat(struct priv *priv, const char *ctr_name, uint64_t *stat)
+{
+	FILE *file;
+	MKSTR(path, "%s/ports/1/hw_counters/%s",
+		  priv->ibdev_path,
+		  ctr_name);
+
+	file = fopen(path, "rb");
+	if (file) {
+		int n = fscanf(file, "%" SCNu64, stat);
+
+		fclose(file);
+		if (n != 1)
+			stat = 0;
+	}
+}
+
 /**
  * Read device counters table.
  *
@@ -155,19 +181,8 @@ mlx5_read_dev_counters(struct rte_eth_dev *dev, uint64_t *stats)
 	}
 	for (i = 0; i != xstats_n; ++i) {
 		if (mlx5_counters_init[i].ib) {
-			FILE *file;
-			MKSTR(path, "%s/ports/1/hw_counters/%s",
-			      priv->ibdev_path,
-			      mlx5_counters_init[i].ctr_name);
-
-			file = fopen(path, "rb");
-			if (file) {
-				int n = fscanf(file, "%" SCNu64, &stats[i]);
-
-				fclose(file);
-				if (n != 1)
-					stats[i] = 0;
-			}
+			mlx5_read_ib_stat(priv, mlx5_counters_init[i].ctr_name,
+				&stats[i]);
 		} else {
 			stats[i] = (uint64_t)
 				et_stats->data[xstats_ctrl->dev_table_idx[i]];
@@ -210,10 +225,11 @@ mlx5_ethtool_get_stats_n(struct rte_eth_dev *dev) {
  *   Pointer to Ethernet device.
  */
 void
-mlx5_xstats_init(struct rte_eth_dev *dev)
+mlx5_stats_init(struct rte_eth_dev *dev)
 {
 	struct priv *priv = dev->data->dev_private;
 	struct mlx5_xstats_ctrl *xstats_ctrl = &priv->xstats_ctrl;
+	struct mlx5_stats_ctrl *stats_ctrl = &priv->stats_ctrl;
 	unsigned int i;
 	unsigned int j;
 	struct ifreq ifr;
@@ -281,6 +297,7 @@ mlx5_xstats_init(struct rte_eth_dev *dev)
 	if (ret)
 		DRV_LOG(ERR, "port %u cannot read device counters: %s",
 			dev->data->port_id, strerror(rte_errno));
+	mlx5_read_ib_stat(priv, "out_of_buffer", &stats_ctrl->imissed_base);
 free:
 	rte_free(strings);
 }
@@ -316,7 +333,7 @@ mlx5_xstats_get(struct rte_eth_dev *dev, struct rte_eth_xstat *stats,
 		if (stats_n < 0)
 			return stats_n;
 		if (xstats_ctrl->stats_n != stats_n)
-			mlx5_xstats_init(dev);
+			mlx5_stats_init(dev);
 		ret = mlx5_read_dev_counters(dev, counters);
 		if (ret)
 			return ret;
@@ -389,6 +406,8 @@ mlx5_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
 #endif
 		tmp.oerrors += txq->stats.oerrors;
 	}
+	mlx5_read_ib_stat(priv, "out_of_buffer", &tmp.imissed);
+	tmp.imissed -= priv->stats_ctrl.imissed_base;
 #ifndef MLX5_PMD_SOFT_COUNTERS
 	/* FIXME: retrieve and add hardware counters. */
 #endif
@@ -406,6 +425,7 @@ void
 mlx5_stats_reset(struct rte_eth_dev *dev)
 {
 	struct priv *priv = dev->data->dev_private;
+	struct mlx5_stats_ctrl *stats_ctrl = &priv->stats_ctrl;
 	unsigned int i;
 	unsigned int idx;
 
@@ -423,6 +443,7 @@ mlx5_stats_reset(struct rte_eth_dev *dev)
 		(*priv->txqs)[i]->stats =
 			(struct mlx5_txq_stats){ .idx = idx };
 	}
+	mlx5_read_ib_stat(priv, "out_of_buffer", &stats_ctrl->imissed_base);
 #ifndef MLX5_PMD_SOFT_COUNTERS
 	/* FIXME: reset hardware counters. */
 #endif
@@ -452,7 +473,7 @@ mlx5_xstats_reset(struct rte_eth_dev *dev)
 		return;
 	}
 	if (xstats_ctrl->stats_n != stats_n)
-		mlx5_xstats_init(dev);
+		mlx5_stats_init(dev);
 	ret = mlx5_read_dev_counters(dev, counters);
 	if (ret) {
 		DRV_LOG(ERR, "port %u cannot read device counters: %s",
diff --git a/drivers/net/mlx5/mlx5_trigger.c b/drivers/net/mlx5/mlx5_trigger.c
index e2a9bb7..3015edd 100644
--- a/drivers/net/mlx5/mlx5_trigger.c
+++ b/drivers/net/mlx5/mlx5_trigger.c
@@ -181,7 +181,7 @@ mlx5_dev_start(struct rte_eth_dev *dev)
 			dev->data->port_id);
 		goto error;
 	}
-	mlx5_xstats_init(dev);
+	mlx5_stats_init(dev);
 	ret = mlx5_traffic_enable(dev);
 	if (ret) {
 		DRV_LOG(DEBUG, "port %u failed to set defaults flows",
-- 
2.7.4

Re: [dpdk-dev] [PATCH v2] mlx5: Report imissed stat

[Shahaf Shuler]

Friday, November 23, 2018 10:04 AM, Friday, November 23, 2018:
> Subject: [PATCH v2] mlx5: Report imissed stat
> 
> The imissed counters (number of packets dropped because the queues were
> full) were actually reported through xstats as "rx_out_of_buffer"
> but was not reported through stats.
> 
> Following a recent discussion on the ML, as there is no way to tell the user if a
> counter is implemented or not, this should be considered a bug. Eg, user
> looking at imissed will think the packets are lost before reaching the device.
> 
> As for xstats, I added a base counter to be able to "reset" imissed.
> 
> v2:
>  - Add rx_discards_phy xstats counter
>  - Add documentation
>  - Follow suggestions from Shahaf Shuler <shahafs@mellanox.com> as per
>    v1 comments
> 
> Signed-off-by: Tom Barbette <barbette@kth.se>

Applied to next-net-mlx, with small modifications due to rebase issues.
Tom - please have a second look and confirm.

> ---
>  doc/guides/nics/mlx5.rst        |  2 +-
>  drivers/net/mlx5/mlx5.h         |  8 ++++++-
>  drivers/net/mlx5/mlx5_stats.c   | 53 ++++++++++++++++++++++++++++----
> ---------
>  drivers/net/mlx5/mlx5_trigger.c |  2 +-
>  4 files changed, 46 insertions(+), 19 deletions(-)
> 
> diff --git a/doc/guides/nics/mlx5.rst b/doc/guides/nics/mlx5.rst index
> 52e1213..ddc0fdd 100644
> --- a/doc/guides/nics/mlx5.rst
> +++ b/doc/guides/nics/mlx5.rst
> @@ -141,7 +141,7 @@ Statistics
> 
>  MLX5 supports various of methods to report statistics:
> 
> -Port statistics can be queried using ``rte_eth_stats_get()``. The port statistics
> are through SW only and counts the number of packets received or sent
> successfully by the PMD.
> +Port statistics can be queried using ``rte_eth_stats_get()``. The received
> and sent statistics are through SW only and counts the number of packets
> received or sent successfully by the PMD. The imissed counter is the amount
> of packets that could not be delivered to SW because a queue was full.
> Packets not received due to congestion in the bus or on the NIC can be
> queried via the rx_discards_phy xstats counter.
> 
>  Extended statistics can be queried using ``rte_eth_xstats_get()``. The
> extended statistics expose a wider set of counters counted by the device.
> The extended port statistics counts the number of packets received or sent
> successfully by the port. As Mellanox NICs are using the :ref:`Bifurcated Linux
> Driver <linux_gsg_linux_drivers>` those counters counts also packet received
> or sent by the Linux kernel. The counters with ``_phy`` suffix counts the total
> events on the physical port, therefore not valid for VF.
> 
> diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h index
> a3a34cf..1aaaafe 100644
> --- a/drivers/net/mlx5/mlx5.h
> +++ b/drivers/net/mlx5/mlx5.h
> @@ -79,6 +79,11 @@ struct mlx5_xstats_ctrl {
>  	uint64_t base[MLX5_MAX_XSTATS];
>  };
> 
> +struct mlx5_stats_ctrl {
> +	/* Base for imissed counter. */
> +	uint64_t imissed_base;
> +};
> +
>  /* Flow list . */
>  TAILQ_HEAD(mlx5_flows, rte_flow);
> 
> @@ -214,6 +219,7 @@ struct priv {
>  	LIST_HEAD(ind_tables, mlx5_ind_table_ibv) ind_tbls;
>  	uint32_t link_speed_capa; /* Link speed capabilities. */
>  	struct mlx5_xstats_ctrl xstats_ctrl; /* Extended stats control. */
> +	struct mlx5_stats_ctrl stats_ctrl; /* Stats control. */
>  	int primary_socket; /* Unix socket for primary process. */
>  	void *uar_base; /* Reserved address space for UAR mapping */
>  	struct rte_intr_handle intr_handle_socket; /* Interrupt handler. */
> @@ -309,7 +315,7 @@ void mlx5_allmulticast_disable(struct rte_eth_dev
> *dev);
> 
>  /* mlx5_stats.c */
> 
> -void mlx5_xstats_init(struct rte_eth_dev *dev);
> +void mlx5_stats_init(struct rte_eth_dev *dev);
>  int mlx5_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats);
> void mlx5_stats_reset(struct rte_eth_dev *dev);  int mlx5_xstats_get(struct
> rte_eth_dev *dev, struct rte_eth_xstat *stats, diff --git
> a/drivers/net/mlx5/mlx5_stats.c b/drivers/net/mlx5/mlx5_stats.c index
> 91f3d47..323f8fb 100644
> --- a/drivers/net/mlx5/mlx5_stats.c
> +++ b/drivers/net/mlx5/mlx5_stats.c
> @@ -108,6 +108,14 @@ static const struct mlx5_counter_ctrl
> mlx5_counters_init[] = {
>  		.ctr_name = "rx_packets_phy",
>  	},
>  	{
> +		.dpdk_name = "tx_discards_phy",
> +		.ctr_name = "tx_discards_phy",
> +	},
> +	{
> +		.dpdk_name = "rx_discards_phy",
> +		.ctr_name = "rx_discards_phy",
> +	},
> +	{
>  		.dpdk_name = "tx_bytes_phy",
>  		.ctr_name = "tx_bytes_phy",
>  	},
> @@ -119,6 +127,24 @@ static const struct mlx5_counter_ctrl
> mlx5_counters_init[] = {
> 
>  static const unsigned int xstats_n = RTE_DIM(mlx5_counters_init);
> 
> +static inline void
> +mlx5_read_ib_stat(struct priv *priv, const char *ctr_name, uint64_t
> +*stat) {
> +	FILE *file;
> +	MKSTR(path, "%s/ports/1/hw_counters/%s",
> +		  priv->ibdev_path,
> +		  ctr_name);
> +
> +	file = fopen(path, "rb");
> +	if (file) {
> +		int n = fscanf(file, "%" SCNu64, stat);
> +
> +		fclose(file);
> +		if (n != 1)
> +			stat = 0;
> +	}
> +}
> +
>  /**
>   * Read device counters table.
>   *
> @@ -155,19 +181,8 @@ mlx5_read_dev_counters(struct rte_eth_dev *dev,
> uint64_t *stats)
>  	}
>  	for (i = 0; i != xstats_n; ++i) {
>  		if (mlx5_counters_init[i].ib) {
> -			FILE *file;
> -			MKSTR(path, "%s/ports/1/hw_counters/%s",
> -			      priv->ibdev_path,
> -			      mlx5_counters_init[i].ctr_name);
> -
> -			file = fopen(path, "rb");
> -			if (file) {
> -				int n = fscanf(file, "%" SCNu64, &stats[i]);
> -
> -				fclose(file);
> -				if (n != 1)
> -					stats[i] = 0;
> -			}
> +			mlx5_read_ib_stat(priv,
> mlx5_counters_init[i].ctr_name,
> +				&stats[i]);
>  		} else {
>  			stats[i] = (uint64_t)
>  				et_stats->data[xstats_ctrl-
> >dev_table_idx[i]];
> @@ -210,10 +225,11 @@ mlx5_ethtool_get_stats_n(struct rte_eth_dev
> *dev) {
>   *   Pointer to Ethernet device.
>   */
>  void
> -mlx5_xstats_init(struct rte_eth_dev *dev)
> +mlx5_stats_init(struct rte_eth_dev *dev)
>  {
>  	struct priv *priv = dev->data->dev_private;
>  	struct mlx5_xstats_ctrl *xstats_ctrl = &priv->xstats_ctrl;
> +	struct mlx5_stats_ctrl *stats_ctrl = &priv->stats_ctrl;
>  	unsigned int i;
>  	unsigned int j;
>  	struct ifreq ifr;
> @@ -281,6 +297,7 @@ mlx5_xstats_init(struct rte_eth_dev *dev)
>  	if (ret)
>  		DRV_LOG(ERR, "port %u cannot read device counters: %s",
>  			dev->data->port_id, strerror(rte_errno));
> +	mlx5_read_ib_stat(priv, "out_of_buffer", &stats_ctrl-
> >imissed_base);
>  free:
>  	rte_free(strings);
>  }
> @@ -316,7 +333,7 @@ mlx5_xstats_get(struct rte_eth_dev *dev, struct
> rte_eth_xstat *stats,
>  		if (stats_n < 0)
>  			return stats_n;
>  		if (xstats_ctrl->stats_n != stats_n)
> -			mlx5_xstats_init(dev);
> +			mlx5_stats_init(dev);
>  		ret = mlx5_read_dev_counters(dev, counters);
>  		if (ret)
>  			return ret;
> @@ -389,6 +406,8 @@ mlx5_stats_get(struct rte_eth_dev *dev, struct
> rte_eth_stats *stats)  #endif
>  		tmp.oerrors += txq->stats.oerrors;
>  	}
> +	mlx5_read_ib_stat(priv, "out_of_buffer", &tmp.imissed);
> +	tmp.imissed -= priv->stats_ctrl.imissed_base;
>  #ifndef MLX5_PMD_SOFT_COUNTERS
>  	/* FIXME: retrieve and add hardware counters. */  #endif @@ -406,6
> +425,7 @@ void  mlx5_stats_reset(struct rte_eth_dev *dev)  {
>  	struct priv *priv = dev->data->dev_private;
> +	struct mlx5_stats_ctrl *stats_ctrl = &priv->stats_ctrl;
>  	unsigned int i;
>  	unsigned int idx;
> 
> @@ -423,6 +443,7 @@ mlx5_stats_reset(struct rte_eth_dev *dev)
>  		(*priv->txqs)[i]->stats =
>  			(struct mlx5_txq_stats){ .idx = idx };
>  	}
> +	mlx5_read_ib_stat(priv, "out_of_buffer", &stats_ctrl-
> >imissed_base);
>  #ifndef MLX5_PMD_SOFT_COUNTERS
>  	/* FIXME: reset hardware counters. */
>  #endif
> @@ -452,7 +473,7 @@ mlx5_xstats_reset(struct rte_eth_dev *dev)
>  		return;
>  	}
>  	if (xstats_ctrl->stats_n != stats_n)
> -		mlx5_xstats_init(dev);
> +		mlx5_stats_init(dev);
>  	ret = mlx5_read_dev_counters(dev, counters);
>  	if (ret) {
>  		DRV_LOG(ERR, "port %u cannot read device counters: %s",
> diff --git a/drivers/net/mlx5/mlx5_trigger.c
> b/drivers/net/mlx5/mlx5_trigger.c index e2a9bb7..3015edd 100644
> --- a/drivers/net/mlx5/mlx5_trigger.c
> +++ b/drivers/net/mlx5/mlx5_trigger.c
> @@ -181,7 +181,7 @@ mlx5_dev_start(struct rte_eth_dev *dev)
>  			dev->data->port_id);
>  		goto error;
>  	}
> -	mlx5_xstats_init(dev);
> +	mlx5_stats_init(dev);
>  	ret = mlx5_traffic_enable(dev);
>  	if (ret) {
>  		DRV_LOG(DEBUG, "port %u failed to set defaults flows",
> --
> 2.7.4

Re: [dpdk-dev] [PATCH v2] mlx5: Report imissed stat

[Tom Barbette]

> Tom - please have a second look and confirm.

Looks good. I tried next-net-mlx and it works fine.  My imissed increases as expected, rx_discards_phy also (sadly).

Thanks,

Tom