[dpdk-dev] [PATCH] net/mlx5: support extended statistics

Adrien Mazarguil adrien.mazarguil at 6wind.com
Wed Jan 11 17:54:46 CET 2017


Hi Shahaf,

Thanks, I have a few comments, most of them relatively minor. Please see
below.

On Wed, Jan 11, 2017 at 10:55:42AM +0200, Shahaf Shuler wrote:
> Implement xstats_*() DPDK callbacks
> 
> Signed-off-by: Shahaf Shuler <shahafs at mellanox.com>
> Signed-off-by: Elad Persiko <eladpe at mellanox.com>
> Signed-off-by: Hanoch Haim <hhaim at cisco.com>
> ---
>  drivers/net/mlx5/mlx5.c         |   3 +
>  drivers/net/mlx5/mlx5.h         |  12 ++
>  drivers/net/mlx5/mlx5_defs.h    |   3 +
>  drivers/net/mlx5/mlx5_stats.c   | 342 ++++++++++++++++++++++++++++++++++++++++
>  drivers/net/mlx5/mlx5_trigger.c |   1 +
>  5 files changed, 361 insertions(+)
> 
> diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
> index 6293c1f..3359d3c 100644
> --- a/drivers/net/mlx5/mlx5.c
> +++ b/drivers/net/mlx5/mlx5.c
> @@ -199,6 +199,9 @@
>  	.link_update = mlx5_link_update,
>  	.stats_get = mlx5_stats_get,
>  	.stats_reset = mlx5_stats_reset,
> +	.xstats_get = mlx5_xstats_get,
> +	.xstats_reset = mlx5_xstats_reset,
> +	.xstats_get_names = mlx5_xstats_get_names,
>  	.dev_infos_get = mlx5_dev_infos_get,
>  	.dev_supported_ptypes_get = mlx5_dev_supported_ptypes_get,
>  	.vlan_filter_set = mlx5_vlan_filter_set,
> diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h
> index ee62e04..034a05e 100644
> --- a/drivers/net/mlx5/mlx5.h
> +++ b/drivers/net/mlx5/mlx5.h
> @@ -89,6 +89,11 @@ enum {
>  	PCI_DEVICE_ID_MELLANOX_CONNECTX5EXVF = 0x101a,
>  };
>  
> +struct mlx5_xstats_ctrl {
> +	uint64_t shadow[MLX5_MAX_XSTATS];
> +	uint16_t stats_n; /* Number of device stats. */
> +};
> +
>  struct priv {
>  	struct rte_eth_dev *dev; /* Ethernet device. */
>  	struct ibv_context *ctx; /* Verbs context. */
> @@ -142,6 +147,7 @@ struct priv {
>  	struct fdir_queue *fdir_drop_queue; /* Flow director drop queue. */
>  	LIST_HEAD(mlx5_flows, rte_flow) flows; /* RTE Flow rules. */
>  	uint32_t link_speed_capa; /* Link speed capabilities. */
> +	struct mlx5_xstats_ctrl xstats_ctrl; /* Extended stats control. */
>  	rte_spinlock_t lock; /* Lock for control functions. */
>  };
>  
> @@ -250,8 +256,14 @@ int mlx5_dev_rss_reta_update(struct rte_eth_dev *,
>  
>  /* mlx5_stats.c */
>  
> +void priv_xstats_init(struct priv *);
>  void mlx5_stats_get(struct rte_eth_dev *, struct rte_eth_stats *);
>  void mlx5_stats_reset(struct rte_eth_dev *);
> +int mlx5_xstats_get(struct rte_eth_dev *,
> +		    struct rte_eth_xstat *, unsigned int);
> +void mlx5_xstats_reset(struct rte_eth_dev *);
> +int mlx5_xstats_get_names(struct rte_eth_dev *,
> +			  struct rte_eth_xstat_name *, unsigned int);
>  
>  /* mlx5_vlan.c */
>  
> diff --git a/drivers/net/mlx5/mlx5_defs.h b/drivers/net/mlx5/mlx5_defs.h
> index b32816e..beabb70 100644
> --- a/drivers/net/mlx5/mlx5_defs.h
> +++ b/drivers/net/mlx5/mlx5_defs.h
> @@ -79,4 +79,7 @@
>  /* Alarm timeout. */
>  #define MLX5_ALARM_TIMEOUT_US 100000
>  
> +/* Maximum number of extended statistics counters. */
> +#define MLX5_MAX_XSTATS 32
> +
>  #endif /* RTE_PMD_MLX5_DEFS_H_ */
> diff --git a/drivers/net/mlx5/mlx5_stats.c b/drivers/net/mlx5/mlx5_stats.c
> index f2b5781..08a7656 100644
> --- a/drivers/net/mlx5/mlx5_stats.c
> +++ b/drivers/net/mlx5/mlx5_stats.c
> @@ -31,11 +31,16 @@
>   *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>   */
>  
> +#include <linux/sockios.h>
> +#include <linux/ethtool.h>
> +
>  /* DPDK headers don't like -pedantic. */
>  #ifdef PEDANTIC
>  #pragma GCC diagnostic ignored "-Wpedantic"
>  #endif
>  #include <rte_ethdev.h>
> +#include <rte_common.h>
> +#include <rte_malloc.h>
>  #ifdef PEDANTIC
>  #pragma GCC diagnostic error "-Wpedantic"
>  #endif
> @@ -44,6 +49,269 @@
>  #include "mlx5_rxtx.h"
>  #include "mlx5_defs.h"
>  
> +struct mlx5_counter_ctrl {
> +	/* Name of the counter for dpdk user. */

"for dpdk user" is redundant given the purpose of this API.

> +	char dpdk_name[RTE_ETH_XSTATS_NAME_SIZE];
> +	/* Name of the counter on the device table. */
> +	char ctr_name[RTE_ETH_XSTATS_NAME_SIZE];
> +	/* Index in the device counters table. */
> +	uint16_t dev_table_idx;
> +};
> +
> +static struct mlx5_counter_ctrl mlx5_counters_init[] = {
> +	{
> +		.dpdk_name = "rx_port_unicast_bytes",
> +		.ctr_name = "rx_vport_unicast_bytes",
> +		.dev_table_idx = 0

Please add comma after ".dev_table_idx = 0" (same comment applies to the
following definitions). Note you may leave it out entirely since unspecified
fields are automatically initialized to zero.

> +	},
> +	{
> +		.dpdk_name = "rx_port_multicast_bytes",
> +		.ctr_name = "rx_vport_multicast_bytes",
> +		.dev_table_idx = 0
> +	},
> +	{
> +		.dpdk_name = "rx_port_broadcast_bytes",
> +		.ctr_name = "rx_vport_broadcast_bytes",
> +		.dev_table_idx = 0
> +	},
> +	{
> +		.dpdk_name = "rx_port_unicast_packets",
> +		.ctr_name = "rx_vport_unicast_packets",
> +		.dev_table_idx = 0
> +	},
> +	{
> +		.dpdk_name = "rx_port_multicast_packets",
> +		.ctr_name = "rx_vport_multicast_packets",
> +		.dev_table_idx = 0
> +	},
> +	{
> +		.dpdk_name = "rx_port_broadcast_packets",
> +		.ctr_name = "rx_vport_broadcast_packets",
> +		.dev_table_idx = 0
> +	},
> +	{
> +		.dpdk_name = "tx_port_unicast_bytes",
> +		.ctr_name = "tx_vport_unicast_bytes",
> +		.dev_table_idx = 0
> +	},
> +	{
> +		.dpdk_name = "tx_port_multicast_bytes",
> +		.ctr_name = "tx_vport_multicast_bytes",
> +		.dev_table_idx = 0
> +	},
> +	{
> +		.dpdk_name = "tx_port_broadcast_bytes",
> +		.ctr_name = "tx_vport_broadcast_bytes",
> +		.dev_table_idx = 0
> +	},
> +	{
> +		.dpdk_name = "tx_port_unicast_packets",
> +		.ctr_name = "tx_vport_unicast_packets",
> +		.dev_table_idx = 0
> +	},
> +	{
> +		.dpdk_name = "tx_port_multicast_packets",
> +		.ctr_name = "tx_vport_multicast_packets",
> +		.dev_table_idx = 0
> +	},
> +	{
> +		.dpdk_name = "tx_port_broadcast_packets",
> +		.ctr_name = "tx_vport_broadcast_packets",
> +		.dev_table_idx = 0
> +	},
> +	{
> +		.dpdk_name = "rx_wqe_err",
> +		.ctr_name = "rx_wqe_err",
> +		.dev_table_idx = 0
> +	},
> +	{
> +		.dpdk_name = "rx_crc_errors_phy",
> +		.ctr_name = "rx_crc_errors_phy",
> +		.dev_table_idx = 0
> +	},
> +	{
> +		.dpdk_name = "rx_in_range_len_errors_phy",
> +		.ctr_name = "rx_in_range_len_errors_phy",
> +		.dev_table_idx = 0
> +	},
> +	{
> +		.dpdk_name = "rx_symbol_err_phy",
> +		.ctr_name = "rx_symbol_err_phy",
> +		.dev_table_idx = 0
> +	},
> +	{
> +		.dpdk_name = "tx_errors_phy",
> +		.ctr_name = "tx_errors_phy",
> +		.dev_table_idx = 0
> +	},
> +};
> +
> +const unsigned int xstats_n = RTE_DIM(mlx5_counters_init);

Considering this global depends on mlx5_counters_init[] and is only used in
this file, it should be static.

> +
> +/**
> + * Read device counters table.
> + *
> + * @param priv
> + *   Pointer to private structure.
> + * @param[out] stats
> + *   Counters table output buffer.
> + *
> + * @return
> + *   0 on success and stats is filled, negative on error.
> + */
> +static int
> +priv_read_dev_counters(struct priv *priv, uint64_t *stats)
> +{
> +	struct mlx5_xstats_ctrl *xstats_ctrl = &priv->xstats_ctrl;
> +	unsigned int i;
> +	struct ifreq ifr;
> +	unsigned int stats_sz = (xstats_ctrl->stats_n * sizeof(uint64_t)) +
> +				 sizeof(struct ethtool_stats);
> +	struct ethtool_stats et_stats[(stats_sz + (
> +				      sizeof(struct ethtool_stats) - 1)) /
> +				      sizeof(struct ethtool_stats)];
> +
> +	et_stats->cmd = ETHTOOL_GSTATS;
> +	et_stats->n_stats = xstats_ctrl->stats_n;
> +	ifr.ifr_data = (caddr_t)et_stats;
> +	if (priv_ifreq(priv, SIOCETHTOOL, &ifr) != 0) {
> +		WARN("unable to get statistic values");
> +		return -1;
> +	}
> +	for (i = 0; (i != xstats_n) ; ++i)
> +		stats[i] = (uint64_t)
> +			   et_stats->data[mlx5_counters_init[i].dev_table_idx];
> +	return 0;
> +}
> +
> +/**
> + * Init the strcutures to read device counters.

Typo, "strcutures".

> + *
> + * @param priv
> + *   Pointer to private structure.
> + */
> +void
> +priv_xstats_init(struct priv *priv)
> +{
> +	struct mlx5_xstats_ctrl *xstats_ctrl = &priv->xstats_ctrl;
> +	unsigned int i;
> +	unsigned int j;
> +	char ifname[IF_NAMESIZE];
> +	struct ifreq ifr;
> +	struct ethtool_drvinfo drvinfo;
> +	struct ethtool_gstrings *strings = NULL;
> +	unsigned int dev_stats_n;
> +	unsigned int str_sz;
> +
> +	if (priv_get_ifname(priv, &ifname)) {
> +		WARN("unable to get interface name");
> +		return;
> +	}
> +	/* How many statistics are available. */
> +	drvinfo.cmd = ETHTOOL_GDRVINFO;
> +	ifr.ifr_data = (caddr_t)&drvinfo;
> +	if (priv_ifreq(priv, SIOCETHTOOL, &ifr) != 0) {
> +		WARN("unable to get driver info");
> +		return;
> +	}
> +	dev_stats_n = drvinfo.n_stats;
> +	if (dev_stats_n < 1) {
> +		WARN("no extended statistics available");
> +		return;
> +	}
> +	xstats_ctrl->stats_n = dev_stats_n;
> +	/* Allocate memory to grab stat names and values. */
> +	str_sz = dev_stats_n * ETH_GSTRING_LEN;
> +	strings = (struct ethtool_gstrings *)
> +		  rte_malloc("xstats_strings",
> +			     str_sz + sizeof(struct ethtool_gstrings), 0);
> +	if (!strings) {
> +		WARN("unable to allocate memory for xstats");
> +		return;
> +	}
> +	strings->cmd = ETHTOOL_GSTRINGS;
> +	strings->string_set = ETH_SS_STATS;
> +	strings->len = dev_stats_n;
> +	ifr.ifr_data = (caddr_t)strings;
> +	if (priv_ifreq(priv, SIOCETHTOOL, &ifr) != 0) {
> +		WARN("unable to get statistic names");
> +		goto free;
> +	}
> +	for (j = 0; (j != xstats_n); ++j)
> +		mlx5_counters_init[j].dev_table_idx = dev_stats_n;

As a global, mlx5_counters_init[] affects all DPDK ports that use the mlx5
PMD simultaneously. Are you sure calling priv_xstats_init() from
mlx5_dev_start() is safe?

I think this global should be const and initialized only once.

> +	for (i = 0; (i != dev_stats_n); ++i) {
> +		const char *curr_string = (const char *)
> +			&strings->data[i * ETH_GSTRING_LEN];
> +
> +		for (j = 0; (j != xstats_n); ++j) {
> +			if (!strcmp(mlx5_counters_init[j].ctr_name,
> +				    curr_string)) {
> +				mlx5_counters_init[j].dev_table_idx = i;

The above comment also applies here. The PMD instance should allocate its
own mapping as a priv field if there is no other choice. You could perhaps
make it part of the mlx5_xstats_ctrl structure.

> +				break;
> +			}
> +		}
> +	}
> +	for (j = 0; (j != xstats_n); ++j) {
> +		if (mlx5_counters_init[j].dev_table_idx >= dev_stats_n) {
> +			WARN("Counters are not recognized");

Displaying the name of the counter that is not recognized could help users
determine what's doing on. Please make sure all info/warning/error messages
are helpful enough, e.g.:

 testpmd> show port xstats 0
 ###### NIC extended statistics for port 0 
 mlx5: Counters are not recognized
 testpmd> # what?

> +			goto free;
> +		}
> +	}
> +	/* Copy to shadow at first time. */
> +	assert(xstats_n <= MLX5_MAX_XSTATS);
> +	priv_read_dev_counters(priv, xstats_ctrl->shadow);
> +free:
> +	rte_free(strings);
> +}
> +
> +/**
> + * Get device extended statistics.
> + *
> + * @param priv
> + *   Pointer to private structure.
> + * @param[out] stats
> + *   Pointer to rte extended stats table.
> + *
> + * @return
> + *   Number of extended stats on success and stats is filled,
> + *   nagative on error.

Typo, "nagative".

> + */
> +static int
> +priv_xstats_get(struct priv *priv, struct rte_eth_xstat *stats)
> +{
> +	struct mlx5_xstats_ctrl *xstats_ctrl = &priv->xstats_ctrl;
> +	unsigned int i;
> +	uint64_t counters[xstats_n];
> +
> +	if (priv_read_dev_counters(priv, counters) < 0)
> +		return -1;
> +	for (i = 0; (i != xstats_n) ; ++i) {
> +		stats[i].id = i;
> +		stats[i].value = (uint64_t)
> +				 (counters[i] - xstats_ctrl->shadow[i]);

I understand the purpose of the shadow array in this context, however to me
"shadow" implies some sort of caching is taking place. I think "init" or
"base" (as the base value or something) would make its purpose clearer.

> +	}
> +	return xstats_n;
> +}
> +
> +/**
> + * Reset device extended statistics.
> + *
> + * @param priv
> + *   Pointer to private structure.
> + */
> +static void
> +priv_xstats_reset(struct priv *priv)
> +{
> +	struct mlx5_xstats_ctrl *xstats_ctrl = &priv->xstats_ctrl;
> +	unsigned int i;
> +	uint64_t counters[xstats_n];
> +
> +	if (priv_read_dev_counters(priv, counters) < 0)
> +		return;
> +	for (i = 0; (i != xstats_n) ; ++i)
> +		xstats_ctrl->shadow[i] = counters[i];
> +}
> +
>  /**
>   * DPDK callback to get device statistics.
>   *
> @@ -142,3 +410,77 @@
>  #endif
>  	priv_unlock(priv);
>  }
> +
> +/**
> + * DPDK callback to get extended device statistics.
> + *
> + * @param dev
> + *   Pointer to Ethernet device structure.
> + * @param[out] stats
> + *   Stats table output buffer.
> + * @param n
> + *   The size of the stats table.
> + *
> + * @return
> + *   Number of xstats on success, negative on failure.
> + */
> +int
> +mlx5_xstats_get(struct rte_eth_dev *dev,
> +		struct rte_eth_xstat *stats, unsigned int n)
> +{
> +	struct priv *priv = mlx5_get_priv(dev);
> +	int ret = xstats_n;
> +
> +	if (n >= xstats_n && stats) {
> +		priv_lock(priv);
> +		ret = priv_xstats_get(priv, stats);
> +		priv_unlock(priv);
> +	}
> +	return ret;
> +}
> +
> +/**
> + * DPDK callback to clear device extended statistics.
> + *
> + * @param dev
> + *   Pointer to Ethernet device structure.
> + */
> +void
> +mlx5_xstats_reset(struct rte_eth_dev *dev)
> +{
> +	struct priv *priv = mlx5_get_priv(dev);
> +
> +	priv_lock(priv);
> +	priv_xstats_reset(priv);
> +	priv_unlock(priv);
> +}
> +
> +/**
> + * DPDK callback to retrieve names of extended device statistics
> + *
> + * @param dev
> + *   Pointer to Ethernet device structure.
> + * @param[out] xstats_names
> + *   Block of memory to insert names into

Let's call "block of memory" a "buffer"? There is also a missing period at
the end of this sentence. 

> + * @param size
> + *   Number of names.

"num" (or "n" as in the API) would make more sense than "size" here.

> + *
> + * @return
> + *   Number of xstats names.
> + */
> +int
> +mlx5_xstats_get_names(struct rte_eth_dev *dev,
> +		struct rte_eth_xstat_name *xstats_names, unsigned int size)
> +{
> +	struct priv *priv = mlx5_get_priv(dev);
> +	unsigned int i;
> +
> +	if (size >= xstats_n && xstats_names) {
> +		priv_lock(priv);
> +		for (i = 0; (i != xstats_n) ; ++i)
> +			strcpy(xstats_names[i].name,
> +			       mlx5_counters_init[i].dpdk_name);
> +		priv_unlock(priv);
> +	}
> +	return xstats_n;
> +}
> diff --git a/drivers/net/mlx5/mlx5_trigger.c b/drivers/net/mlx5/mlx5_trigger.c
> index 2399243..30addd2 100644
> --- a/drivers/net/mlx5/mlx5_trigger.c
> +++ b/drivers/net/mlx5/mlx5_trigger.c
> @@ -91,6 +91,7 @@
>  		priv_fdir_enable(priv);
>  	priv_dev_interrupt_handler_install(priv, dev);
>  	err = priv_flow_start(priv);
> +	priv_xstats_init(priv);
>  	priv_unlock(priv);
>  	return -err;
>  }
> -- 
> 1.8.3.1

-- 
Adrien Mazarguil
6WIND


More information about the dev mailing list