From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f44.google.com (mail-wm0-f44.google.com [74.125.82.44]) by dpdk.org (Postfix) with ESMTP id DA0B9DE5 for ; Mon, 16 Jan 2017 15:38:49 +0100 (CET) Received: by mail-wm0-f44.google.com with SMTP id r126so161647852wmr.0 for ; Mon, 16 Jan 2017 06:38:49 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=from:to:cc:subject:date:message-id:user-agent:in-reply-to :references:mime-version:content-transfer-encoding; bh=aKsuWJRptqUkjFqQIPrU56fMXsVQXOQ/XQ2442SsjNw=; b=qjoJ/rpoE3rK3nja2R4Ry2PQcErrVef0BRFA/yXVXKrT6o3nIDoMlz7ZZG5ibxbfyv Tc5oUCLQumapEayo6DmUtGd7dmgut3aaPbibEVBGOU1FOZSXw+ahJrDagfMK4RRd8yVE a7YueDL5KBLicTV0l+YrRrDm1/y4BHgFxcw6P9tjY+V03q9Xvq/RqgR//qeWcyuPpwWF Awq2EIpo7uuyp1XSD7d/NxWz0xve4RuIrD+ERFqYeQC3U6X4Lo9U24AaIddM8pZJjVOn Vq814V/hfv74rNnMk/b6VSjizDI9mqawRybxMtuiwxINweSR3R4oRPbwzJHDPaX58JT5 vN/w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:user-agent :in-reply-to:references:mime-version:content-transfer-encoding; bh=aKsuWJRptqUkjFqQIPrU56fMXsVQXOQ/XQ2442SsjNw=; b=A66lXEwjLJYvy+gLK+UmYfstZss+ZueXCpGJu4qatI8okaRbcZ2gLGmkvmt2hYQcbw nMVTR55pMj/vbvIx0IIZT3VjTmXVvsKT63/iURdmfb4QwqGg6/ROqClGN4phhuiHxlHh r/BVM1xBSCiMTgoqr93WV7NahnijGXx1bpFWJbRcA80rb529/GuIbwROfUWy5qSf+IsC zy2w3G9HtbFCCyBwk4BOEsYRtOVVuEx10bg/OP82/7n5497QRJun5RcbPfqH6Ku5AZam l8pnToHrzqR5MOjkCYULKS0nY3cZXbsK90CiiT/u8vNiE6XnY7Rj7GTu6zsqLFyQuEBS 1m+Q== X-Gm-Message-State: AIkVDXJ93kddt4/gCK/QP+cIrw7jnbS3DAkW9h3zA1BblUTycYIAC9V8o271/6wl6dEWFBAh X-Received: by 10.223.136.152 with SMTP id f24mr19079092wrf.187.1484577529510; Mon, 16 Jan 2017 06:38:49 -0800 (PST) Received: from xps13.localnet (184.203.134.77.rev.sfr.net. [77.134.203.184]) by smtp.gmail.com with ESMTPSA id y65sm25972576wmb.5.2017.01.16.06.38.47 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 16 Jan 2017 06:38:48 -0800 (PST) From: Thomas Monjalon To: Shahaf Shuler Cc: dev@dpdk.org, "Hanoch Haim (hhaim)" , Adrien Mazarguil , Elad Persiko Date: Mon, 16 Jan 2017 15:38:46 +0100 Message-ID: <4595489.kzA2mruRYd@xps13> User-Agent: KMail/4.14.10 (Linux/4.5.4-1-ARCH; KDE/4.14.11; x86_64; ; ) In-Reply-To: References: <1484124942-26279-1-git-send-email-shahafs@mellanox.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Subject: Re: [dpdk-dev] [PATCH] net/mlx5: support extended statistics X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 16 Jan 2017 14:38:50 -0000 Hi Shahaf, I am not specifically interested in this thread, but if I was, it is not sure I would make the effort to try to understand your message (starting with a signature, an useless original header, and some long useless listings). I won't try to find which special sign you use to mark your reply. We read a lot of mails in this mailing list so we must comply to some standard efficient formatting: - remove the useless lines - quote original lines with one more "> " - reply below the regarding section Maybe you need to tune or change your mail software, I'm sorry but it is worth to do it. Note: some other contributors have currently the same issue as you. Note 2: I said nothing about Outlook ;) ---- for reference, below ---- 2017-01-16 13:32, Shahaf Shuler: > > --Shahaf > > -----Original Message----- > From: Hanoch Haim (hhaim) [mailto:hhaim@cisco.com] > Sent: Thursday, January 12, 2017 11:57 AM > To: Adrien Mazarguil ; Shahaf Shuler > Cc: dev@dpdk.org; Elad Persiko > Subject: RE: [PATCH] net/mlx5: support extended statistics > > Hi Shahaf, > > 1) I would add *all* the hw counters to PF > > $ethtool -S enp135s0f0 > NIC statistics: > rx_packets: 54 > rx_bytes: 3240 > tx_packets: 138 > tx_bytes: 8280 > tx_tso_packets: 0 > tx_tso_bytes: 0 > tx_tso_inner_packets: 0 > tx_tso_inner_bytes: 0 > rx_lro_packets: 0 > rx_lro_bytes: 0 > rx_csum_unnecessary: 0 > rx_csum_none: 54 > rx_csum_complete: 0 > rx_csum_unnecessary_inner: 0 > tx_csum_partial: 0 > tx_csum_partial_inner: 0 > tx_csum_none: 138 > tx_queue_stopped: 0 > tx_queue_wake: 0 > tx_queue_dropped: 0 > rx_sw_lro_aggregated: 0 > rx_sw_lro_flushed: 0 > rx_sw_lro_no_desc: 0 > rx_wqe_err: 0 > rx_cqe_compress_pkts: 0 > rx_cqe_compress_blks: 0 > rx_mpwqe_filler: 0 > rx_mpwqe_frag: 0 > link_down_events_phy: 0 > rx_out_of_buffer: 0 > rx_vport_unicast_packets: 26126070627 > rx_vport_unicast_bytes: 1328543705864986 > tx_vport_unicast_packets: 687072959078 > tx_vport_unicast_bytes: 3339188884659844 > rx_vport_multicast_packets: 0 > rx_vport_multicast_bytes: 0 > tx_vport_multicast_packets: 0 > tx_vport_multicast_bytes: 0 > rx_vport_broadcast_packets: 4543553 > rx_vport_broadcast_bytes: 272614872 > tx_vport_broadcast_packets: 4543637 > tx_vport_broadcast_bytes: 272619912 > rx_vport_rdma_unicast_packets: 430134 > rx_vport_rdma_unicast_bytes: 32690184 > tx_vport_rdma_unicast_packets: 0 > tx_vport_rdma_unicast_bytes: 0 > rx_vport_rdma_multicast_packets: 0 > rx_vport_rdma_multicast_bytes: 0 > tx_vport_rdma_multicast_packets: 0 > tx_vport_rdma_multicast_bytes: 0 > tx_packets_phy: 11756775169850 > rx_packets_phy: 2246825269635 > rx_crc_errors_phy: 0 > tx_bytes_phy: 3404211507236837 > rx_bytes_phy: 1342896754459327 > tx_multicast_phy: 0 > tx_broadcast_phy: 4552688 > rx_multicast_phy: 0 > rx_broadcast_phy: 4552604 > rx_in_range_len_errors_phy: 0 > rx_out_of_range_len_phy: 0 > rx_oversize_pkts_phy: 1135778701 > rx_symbol_err_phy: 0 > tx_mac_control_phy: 16579953 > rx_mac_control_phy: 16670966 > rx_unsupported_op_phy: 0 > rx_pause_ctrl_phy: 16670966 > tx_pause_ctrl_phy: 16579953 > rx_discards_phy: 12244164648 > tx_discards_phy: 0 > tx_errors_phy: 0 > rx_undersize_pkts_phy: 0 > rx_fragments_phy: 0 > rx_jabbers_phy: 0 > rx_64_bytes_phy: 911948813404 > rx_65_to_127_bytes_phy: 335422614022 > rx_128_to_255_bytes_phy: 153383903056 > rx_256_to_511_bytes_phy: 80464779917 > rx_512_to_1023_bytes_phy: 184356224678 > rx_1024_to_1518_bytes_phy: 313733619509 > rx_1519_to_2047_bytes_phy: 235669614881 > rx_2048_to_4095_bytes_phy: 4051420173 > rx_4096_to_8191_bytes_phy: 1011852034 > rx_8192_to_10239_bytes_phy: 27710327250 > time_since_last_clear_phy: 2998621833 > symbol_errors_phy: 0 > sync_headers_errors_phy: 0 > edpl/bip_errors_lane0_phy: 0 > edpl/bip_errors_lane1_phy: 0 > edpl/bip_errors_lane2_phy: 0 > edpl/bip_errors_lane3_phy: 0 > fc_corrected_blocks_lane0_phy: 0 > fc_corrected_blocks_lane1_phy: 0 > fc_corrected_blocks_lane2_phy: 0 > fc_corrected_blocks_lane3_phy: 0 > fc_uncorrectable_lane0_phy: 0 > fc_uncorrectable_lane1_phy: 0 > fc_uncorrectable_lane2_phy: 0 > fc_uncorrectable_lane3_phy: 0 > rs_corrected_blocks_phy: 0 > rs_uncorrectable_blocks_phy: 0 > rs_no_errors_blocks_phy: 58566832662545 > rs_single_error_blocks_phy: 0 > rs_corrected_symbols_total_phy: 0 > rs_corrected_symbols_lane0_phy: 0 > rs_corrected_symbols_lane1_phy: 0 > rs_corrected_symbols_lane2_phy: 0 > rs_corrected_symbols_lane3_phy: 0 > rx_prio0_bytes: 1342896754459327 > rx_prio0_packets: 2234564434021 > tx_prio0_bytes: 3404210446119845 > tx_prio0_packets: 11756758589897 > rx_prio1_bytes: 0 > rx_prio1_packets: 0 > tx_prio1_bytes: 0 > tx_prio1_packets: 0 > rx_prio2_bytes: 0 > rx_prio2_packets: 0 > tx_prio2_bytes: 0 > tx_prio2_packets: 0 > rx_prio3_bytes: 0 > rx_prio3_packets: 0 > tx_prio3_bytes: 0 > tx_prio3_packets: 0 > rx_prio4_bytes: 0 > rx_prio4_packets: 0 > tx_prio4_bytes: 0 > tx_prio4_packets: 0 > rx_prio5_bytes: 0 > rx_prio5_packets: 0 > tx_prio5_bytes: 0 > tx_prio5_packets: 0 > rx_prio6_bytes: 0 > rx_prio6_packets: 0 > tx_prio6_bytes: 0 > tx_prio6_packets: 0 > rx_prio7_bytes: 0 > rx_prio7_packets: 0 > tx_prio7_bytes: 0 > tx_prio7_packets: 0 > rx0_packets: 54 > rx0_bytes: 3240 > rx0_csum_complete > [Shahaf Shuler] we will consider to insert new counters on future patches > > 2) Add the right HW counters to the VF > > tx28_nop: 0 > tx28_queue_stopped: 0 > tx28_queue_wake: 0 > tx28_queue_dropped: 0 > tx29_packets: 0 > tx29_bytes: 0 > tx29_tso_packets: 0 > tx29_tso_bytes: 0 > tx29_tso_inner_packets: 0 > tx29_tso_inner_bytes: 0 > tx29_csum_none: 0 > tx29_csum_partial: 0 > tx29_csum_partial_inner: 0 > tx29_nop: 0 > tx29_queue_stopped: 0 > [Shahaf Shuler] the counters are valid for both VF/PF > > 3) Implement the stats_get/reset using HW counters > > .stats_get = mlx5_stats_get, > .stats_reset = mlx5_stats_reset, > [Shahaf Shuler] it will be supported by future commits. > > > Thanks, > Hanoh > > > -----Original Message----- > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com] > Sent: Wednesday, January 11, 2017 6:55 PM > To: Shahaf Shuler > Cc: dev@dpdk.org; Elad Persiko; Hanoch Haim (hhaim) > Subject: Re: [PATCH] net/mlx5: support extended statistics > > 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 > > Signed-off-by: Elad Persiko > > Signed-off-by: Hanoch Haim > > --- > > 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 > > +#include > > + > > /* DPDK headers don't like -pedantic. */ #ifdef PEDANTIC #pragma > > GCC diagnostic ignored "-Wpedantic" > > #endif > > #include > > +#include > > +#include > > #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