Re: [PATCH v2] mlx5: Report imissed stat

[Shahaf Shuler <shahafs@mellanox.com>]

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