All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net-next 0/7] ethtool: add pause frame stats
@ 2020-09-11 19:52 Jakub Kicinski
  2020-09-11 19:52 ` [PATCH net-next 1/8] ethtool: add standard pause stats Jakub Kicinski
                   ` (7 more replies)
  0 siblings, 8 replies; 23+ messages in thread
From: Jakub Kicinski @ 2020-09-11 19:52 UTC (permalink / raw)
  To: davem
  Cc: netdev, mkubecek, michael.chan, tariqt, saeedm, alexander.duyck,
	andrew, Jakub Kicinski

Hi!

This is the first (small) series which exposes some stats via
the corresponding ethtool interface. Here (thanks to the
excitability of netlink) we expose pause frame stats via
the same interfaces as ethtool -a / -A.

In particular the following stats from the standard:
 - 30.3.4.2 aPAUSEMACCtrlFramesTransmitted
 - 30.3.4.3 aPAUSEMACCtrlFramesReceived

4 real drivers are converted, hopefully the semantics match
the standard.

Jakub Kicinski (8):
  ethtool: add standard pause stats
  docs: net: include the new ethtool pause stats in the stats doc
  netdevsim: add pause frame stats
  selftests: add a test for ethtool pause stats
  bnxt: add pause frame stats
  mlx5: add pause frame stats
  ixgbe: add pause frame stats
  mlx4: add pause frame stats

 Documentation/networking/ethtool-netlink.rst  |  11 ++
 Documentation/networking/statistics.rst       |  57 ++++++++-
 .../net/ethernet/broadcom/bnxt/bnxt_ethtool.c |  19 +++
 .../net/ethernet/intel/ixgbe/ixgbe_ethtool.c  |  11 ++
 .../net/ethernet/mellanox/mlx4/en_ethtool.c   |  19 +++
 .../net/ethernet/mellanox/mlx4/mlx4_stats.h   |  12 ++
 drivers/net/ethernet/mellanox/mlx5/core/en.h  |   2 +
 .../ethernet/mellanox/mlx5/core/en_ethtool.c  |  15 +++
 .../net/ethernet/mellanox/mlx5/core/en_rep.c  |   9 ++
 .../ethernet/mellanox/mlx5/core/en_stats.c    |  30 +++++
 .../ethernet/mellanox/mlx5/core/en_stats.h    |   3 +
 drivers/net/netdevsim/Makefile                |   2 +-
 drivers/net/netdevsim/ethtool.c               |  64 +++++++++++
 drivers/net/netdevsim/netdev.c                |   1 +
 drivers/net/netdevsim/netdevsim.h             |  11 ++
 include/linux/ethtool.h                       |  26 +++++
 include/uapi/linux/ethtool_netlink.h          |  18 ++-
 net/ethtool/pause.c                           |  57 ++++++++-
 .../drivers/net/netdevsim/ethtool-pause.sh    | 108 ++++++++++++++++++
 19 files changed, 467 insertions(+), 8 deletions(-)
 create mode 100644 drivers/net/netdevsim/ethtool.c
 create mode 100755 tools/testing/selftests/drivers/net/netdevsim/ethtool-pause.sh

-- 
2.26.2


^ permalink raw reply	[flat|nested] 23+ messages in thread
* [PATCH net-next 1/8] ethtool: add standard pause stats
  2020-09-11 19:52 [PATCH net-next 0/7] ethtool: add pause frame stats Jakub Kicinski
@ 2020-09-11 19:52 ` Jakub Kicinski
  2020-09-11 19:52 ` [PATCH net-next 2/8] docs: net: include the new ethtool pause stats in the stats doc Jakub Kicinski
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 23+ messages in thread
From: Jakub Kicinski @ 2020-09-11 19:52 UTC (permalink / raw)
  To: davem
  Cc: netdev, mkubecek, michael.chan, tariqt, saeedm, alexander.duyck,
	andrew, Jakub Kicinski

Currently drivers have to report their pause frames statistics
via ethtool -S, and there is a wide variety of names used for
these statistics.

Add the two statistics defined in IEEE 802.3x to the standard
API. Create a new ethtool request header flag for including
statistics in the response to GET commands.

Always create the ETHTOOL_A_PAUSE_STATS nest in replies when
flag is set. Testing if driver declares the op is not a reliable
way of checking if any stats will actually be included and therefore
we don't want to give the impression that presence of
ETHTOOL_A_PAUSE_STATS indicates driver support.

Note that this patch does not include PFC counters, which may fit
better in dcbnl? But mostly I don't need them/have a setup to test
them so I haven't looked deeply into exposing them :)

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 Documentation/networking/ethtool-netlink.rst | 11 ++++
 include/linux/ethtool.h                      | 26 +++++++++
 include/uapi/linux/ethtool_netlink.h         | 18 ++++++-
 net/ethtool/pause.c                          | 57 +++++++++++++++++++-
 4 files changed, 110 insertions(+), 2 deletions(-)

diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
index d53bcb31645a..2c8e0ddf548e 100644
--- a/Documentation/networking/ethtool-netlink.rst
+++ b/Documentation/networking/ethtool-netlink.rst
@@ -68,6 +68,7 @@ types. The interpretation of these flags is the same for all request types but
   =================================  ===================================
   ``ETHTOOL_FLAG_COMPACT_BITSETS``   use compact format bitsets in reply
   ``ETHTOOL_FLAG_OMIT_REPLY``        omit optional reply (_SET and _ACT)
+  ``ETHTOOL_FLAG_STATS``             include optional device statistics
   =================================  ===================================
 
 New request flags should follow the general idea that if the flag is not set,
@@ -989,8 +990,18 @@ Gets channel counts like ``ETHTOOL_GPAUSE`` ioctl request.
   ``ETHTOOL_A_PAUSE_AUTONEG``            bool    pause autonegotiation
   ``ETHTOOL_A_PAUSE_RX``                 bool    receive pause frames
   ``ETHTOOL_A_PAUSE_TX``                 bool    transmit pause frames
+  ``ETHTOOL_A_PAUSE_STATS``              nested  pause statistics
   =====================================  ======  ==========================
 
+``ETHTOOL_A_PAUSE_STATS`` are reported if ``ETHTOOL_FLAG_STATS`` was set
+in ``ETHTOOL_A_HEADER_FLAGS``.
+It will be empty if driver did not report any statistics. Drivers fill in
+the statistics in the following structure:
+
+.. kernel-doc:: include/linux/ethtool.h
+    :identifiers: ethtool_pause_stats
+
+Each member has a corresponding attribute defined.
 
 PAUSE_SET
 ============
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 969a80211df6..060b20f0b20f 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -241,6 +241,27 @@ bool ethtool_convert_link_mode_to_legacy_u32(u32 *legacy_u32,
 	 ETHTOOL_COALESCE_PKT_RATE_LOW | ETHTOOL_COALESCE_PKT_RATE_HIGH | \
 	 ETHTOOL_COALESCE_RATE_SAMPLE_INTERVAL)
 
+#define ETHTOOL_STAT_NOT_SET	(~0ULL)
+
+/**
+ * struct ethtool_pause_stats - statistics for IEEE 802.3x pause frames
+ * @tx_pause_frames: transmitted pause frame count. Reported to user space
+ *	as %ETHTOOL_A_PAUSE_STAT_TX_FRAMES.
+ *
+ *	Equivalent to `30.3.4.2 aPAUSEMACCtrlFramesTransmitted`
+ *	from the standard.
+ *
+ * @rx_pause_frames: received pause frame count. Reported to user space
+ *	as %ETHTOOL_A_PAUSE_STAT_RX_FRAMES. Equivalent to:
+ *
+ *	Equivalent to `30.3.4.3 aPAUSEMACCtrlFramesReceived`
+ *	from the standard.
+ */
+struct ethtool_pause_stats {
+	u64 tx_pause_frames;
+	u64 rx_pause_frames;
+};
+
 /**
  * struct ethtool_ops - optional netdev operations
  * @supported_coalesce_params: supported types of interrupt coalescing.
@@ -282,6 +303,9 @@ bool ethtool_convert_link_mode_to_legacy_u32(u32 *legacy_u32,
  *	Returns a negative error code or zero.
  * @get_ringparam: Report ring sizes
  * @set_ringparam: Set ring sizes.  Returns a negative error code or zero.
+ * @get_pause_stats: Report pause frame statistics. Drivers must not zero
+ *	statistics which they don't report. The stats structure is initialized
+ *	to ETHTOOL_STAT_NOT_SET indicating driver does not report statistics.
  * @get_pauseparam: Report pause parameters
  * @set_pauseparam: Set pause parameters.  Returns a negative error code
  *	or zero.
@@ -418,6 +442,8 @@ struct ethtool_ops {
 				 struct ethtool_ringparam *);
 	int	(*set_ringparam)(struct net_device *,
 				 struct ethtool_ringparam *);
+	void	(*get_pause_stats)(struct net_device *dev,
+				   struct ethtool_pause_stats *pause_stats);
 	void	(*get_pauseparam)(struct net_device *,
 				  struct ethtool_pauseparam*);
 	int	(*set_pauseparam)(struct net_device *,
diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
index 5dcd24cb33ea..9cee6df01a10 100644
--- a/include/uapi/linux/ethtool_netlink.h
+++ b/include/uapi/linux/ethtool_netlink.h
@@ -91,9 +91,12 @@ enum {
 #define ETHTOOL_FLAG_COMPACT_BITSETS	(1 << 0)
 /* provide optional reply for SET or ACT requests */
 #define ETHTOOL_FLAG_OMIT_REPLY	(1 << 1)
+/* request statistics, if supported by the driver */
+#define ETHTOOL_FLAG_STATS		(1 << 2)
 
 #define ETHTOOL_FLAG_ALL (ETHTOOL_FLAG_COMPACT_BITSETS | \
-			  ETHTOOL_FLAG_OMIT_REPLY)
+			  ETHTOOL_FLAG_OMIT_REPLY | \
+			  ETHTOOL_FLAG_STATS)
 
 enum {
 	ETHTOOL_A_HEADER_UNSPEC,
@@ -376,12 +379,25 @@ enum {
 	ETHTOOL_A_PAUSE_AUTONEG,			/* u8 */
 	ETHTOOL_A_PAUSE_RX,				/* u8 */
 	ETHTOOL_A_PAUSE_TX,				/* u8 */
+	ETHTOOL_A_PAUSE_STATS,				/* nest - _PAUSE_STAT_* */
 
 	/* add new constants above here */
 	__ETHTOOL_A_PAUSE_CNT,
 	ETHTOOL_A_PAUSE_MAX = (__ETHTOOL_A_PAUSE_CNT - 1)
 };
 
+enum {
+	ETHTOOL_A_PAUSE_STAT_UNSPEC,
+	ETHTOOL_A_PAUSE_STAT_PAD,
+
+	ETHTOOL_A_PAUSE_STAT_TX_FRAMES,
+	ETHTOOL_A_PAUSE_STAT_RX_FRAMES,
+
+	/* add new constants above here */
+	__ETHTOOL_A_PAUSE_STAT_CNT,
+	ETHTOOL_A_PAUSE_STAT_MAX = (__ETHTOOL_A_PAUSE_STAT_CNT - 1)
+};
+
 /* EEE */
 
 enum {
diff --git a/net/ethtool/pause.c b/net/ethtool/pause.c
index 7aea35d1e8a5..501cb8e7269b 100644
--- a/net/ethtool/pause.c
+++ b/net/ethtool/pause.c
@@ -10,6 +10,7 @@ struct pause_req_info {
 struct pause_reply_data {
 	struct ethnl_reply_data		base;
 	struct ethtool_pauseparam	pauseparam;
+	struct ethtool_pause_stats	pausestat;
 };
 
 #define PAUSE_REPDATA(__reply_base) \
@@ -22,6 +23,7 @@ pause_get_policy[ETHTOOL_A_PAUSE_MAX + 1] = {
 	[ETHTOOL_A_PAUSE_AUTONEG]		= { .type = NLA_REJECT },
 	[ETHTOOL_A_PAUSE_RX]			= { .type = NLA_REJECT },
 	[ETHTOOL_A_PAUSE_TX]			= { .type = NLA_REJECT },
+	[ETHTOOL_A_PAUSE_STATS]			= { .type = NLA_REJECT },
 };
 
 static int pause_prepare_data(const struct ethnl_req_info *req_base,
@@ -34,10 +36,17 @@ static int pause_prepare_data(const struct ethnl_req_info *req_base,
 
 	if (!dev->ethtool_ops->get_pauseparam)
 		return -EOPNOTSUPP;
+
 	ret = ethnl_ops_begin(dev);
 	if (ret < 0)
 		return ret;
 	dev->ethtool_ops->get_pauseparam(dev, &data->pauseparam);
+	if (req_base->flags & ETHTOOL_FLAG_STATS &&
+	    dev->ethtool_ops->get_pause_stats) {
+		memset(&data->pausestat, 0xff,
+		       sizeof(struct ethtool_pause_stats));
+		dev->ethtool_ops->get_pause_stats(dev, &data->pausestat);
+	}
 	ethnl_ops_complete(dev);
 
 	return 0;
@@ -46,9 +55,50 @@ static int pause_prepare_data(const struct ethnl_req_info *req_base,
 static int pause_reply_size(const struct ethnl_req_info *req_base,
 			    const struct ethnl_reply_data *reply_base)
 {
-	return nla_total_size(sizeof(u8)) +	/* _PAUSE_AUTONEG */
+	int n = nla_total_size(sizeof(u8)) +	/* _PAUSE_AUTONEG */
 		nla_total_size(sizeof(u8)) +	/* _PAUSE_RX */
 		nla_total_size(sizeof(u8));	/* _PAUSE_TX */
+
+	if (req_base->flags & ETHTOOL_FLAG_STATS)
+		n += nla_total_size(0) +	/* _PAUSE_STATS */
+			nla_total_size_64bit(sizeof(u64)) *
+				(ETHTOOL_A_PAUSE_STAT_MAX - 2);
+	return n;
+}
+
+static int ethtool_put_stat(struct sk_buff *skb, u64 val, u16 attrtype,
+			    u16 padtype)
+{
+	if (val == ETHTOOL_STAT_NOT_SET)
+		return 0;
+	if (nla_put_u64_64bit(skb, attrtype, val, padtype))
+		return -EMSGSIZE;
+
+	return 0;
+}
+
+static int pause_put_stats(struct sk_buff *skb,
+			   const struct ethtool_pause_stats *pause_stats)
+{
+	const u16 pad = ETHTOOL_A_PAUSE_STAT_PAD;
+	struct nlattr *nest;
+
+	nest = nla_nest_start(skb, ETHTOOL_A_PAUSE_STATS);
+	if (!nest)
+		return -EMSGSIZE;
+
+	if (ethtool_put_stat(skb, pause_stats->tx_pause_frames,
+			     ETHTOOL_A_PAUSE_STAT_TX_FRAMES, pad) ||
+	    ethtool_put_stat(skb, pause_stats->rx_pause_frames,
+			     ETHTOOL_A_PAUSE_STAT_RX_FRAMES, pad))
+		goto err_cancel;
+
+	nla_nest_end(skb, nest);
+	return 0;
+
+err_cancel:
+	nla_nest_cancel(skb, nest);
+	return -EMSGSIZE;
 }
 
 static int pause_fill_reply(struct sk_buff *skb,
@@ -63,6 +113,10 @@ static int pause_fill_reply(struct sk_buff *skb,
 	    nla_put_u8(skb, ETHTOOL_A_PAUSE_TX, !!pauseparam->tx_pause))
 		return -EMSGSIZE;
 
+	if (req_base->flags & ETHTOOL_FLAG_STATS &&
+	    pause_put_stats(skb, &data->pausestat))
+		return -EMSGSIZE;
+
 	return 0;
 }
 
@@ -89,6 +143,7 @@ pause_set_policy[ETHTOOL_A_PAUSE_MAX + 1] = {
 	[ETHTOOL_A_PAUSE_AUTONEG]		= { .type = NLA_U8 },
 	[ETHTOOL_A_PAUSE_RX]			= { .type = NLA_U8 },
 	[ETHTOOL_A_PAUSE_TX]			= { .type = NLA_U8 },
+	[ETHTOOL_A_PAUSE_STATS]			= { .type = NLA_REJECT },
 };
 
 int ethnl_set_pause(struct sk_buff *skb, struct genl_info *info)
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 23+ messages in thread
* [PATCH net-next 2/8] docs: net: include the new ethtool pause stats in the stats doc
  2020-09-11 19:52 [PATCH net-next 0/7] ethtool: add pause frame stats Jakub Kicinski
  2020-09-11 19:52 ` [PATCH net-next 1/8] ethtool: add standard pause stats Jakub Kicinski
@ 2020-09-11 19:52 ` Jakub Kicinski
  2020-09-11 19:52 ` [PATCH net-next 3/8] netdevsim: add pause frame stats Jakub Kicinski
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 23+ messages in thread
From: Jakub Kicinski @ 2020-09-11 19:52 UTC (permalink / raw)
  To: davem
  Cc: netdev, mkubecek, michael.chan, tariqt, saeedm, alexander.duyck,
	andrew, Jakub Kicinski

Tell people that there now is an interface for querying pause frames.
A little bit of restructuring is needed given this is a first source
of such statistics.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 Documentation/networking/statistics.rst | 57 ++++++++++++++++++++++---
 1 file changed, 52 insertions(+), 5 deletions(-)

diff --git a/Documentation/networking/statistics.rst b/Documentation/networking/statistics.rst
index d490b535cd14..8e15bc98830b 100644
--- a/Documentation/networking/statistics.rst
+++ b/Documentation/networking/statistics.rst
@@ -4,16 +4,23 @@
 Interface statistics
 ====================
 
+Overview
+========
+
 This document is a guide to Linux network interface statistics.
 
-There are two main sources of interface statistics in Linux:
+There are three main sources of interface statistics in Linux:
 
  - standard interface statistics based on
-   :c:type:`struct rtnl_link_stats64 <rtnl_link_stats64>`; and
+   :c:type:`struct rtnl_link_stats64 <rtnl_link_stats64>`;
+ - protocol-specific statistics; and
  - driver-defined statistics available via ethtool.
 
-There are multiple interfaces to reach the former. Most commonly used
-is the `ip` command from `iproute2`::
+Standard interface statistics
+-----------------------------
+
+There are multiple interfaces to reach the standard statistics.
+Most commonly used is the `ip` command from `iproute2`::
 
   $ ip -s -s link show dev ens4u1u1
   6: ens4u1u1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc fq_codel state UP mode DEFAULT group default qlen 1000
@@ -34,7 +41,26 @@ If `-s` is specified once the detailed errors won't be shown.
 
 `ip` supports JSON formatting via the `-j` option.
 
-Ethtool statistics can be dumped using `ethtool -S $ifc`, e.g.::
+Protocol-specific statistics
+----------------------------
+
+Some of the interfaces used for configuring devices are also able
+to report related statistics. For example ethtool interface used
+to configure pause frames can report corresponding hardware counters::
+
+  $ ethtool --include-statistics -a eth0
+  Pause parameters for eth0:
+  Autonegotiate:	on
+  RX:			on
+  TX:			on
+  Statistics:
+    tx_pause_frames: 1
+    rx_pause_frames: 1
+
+Driver-defined statistics
+-------------------------
+
+Driver-defined ethtool statistics can be dumped using `ethtool -S $ifc`, e.g.::
 
   $ ethtool -S ens4u1u1
   NIC statistics:
@@ -94,6 +120,17 @@ Identifiers via `ETHTOOL_GSTRINGS` with `string_set` set to `ETH_SS_STATS`,
 and values via `ETHTOOL_GSTATS`. User space should use `ETHTOOL_GDRVINFO`
 to retrieve the number of statistics (`.n_stats`).
 
+ethtool-netlink
+---------------
+
+Ethtool netlink is a replacement for the older IOCTL interface.
+
+Protocol-related statistics can be requested in get commands by setting
+the `ETHTOOL_FLAG_STATS` flag in `ETHTOOL_A_HEADER_FLAGS`. Currently
+statistics are supported in the following commands:
+
+  - `ETHTOOL_MSG_PAUSE_GET`
+
 debugfs
 -------
 
@@ -130,3 +167,13 @@ user space trying to read them.
 
 Statistics must persist across routine operations like bringing the interface
 down and up.
+
+Kernel-internal data structures
+-------------------------------
+
+The following structures are internal to the kernel, their members are
+translated to netlink attributes when dumped. Drivers must not overwrite
+the statistics they don't report with 0.
+
+.. kernel-doc:: include/linux/ethtool.h
+    :identifiers: ethtool_pause_stats
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 23+ messages in thread
* [PATCH net-next 3/8] netdevsim: add pause frame stats
  2020-09-11 19:52 [PATCH net-next 0/7] ethtool: add pause frame stats Jakub Kicinski
  2020-09-11 19:52 ` [PATCH net-next 1/8] ethtool: add standard pause stats Jakub Kicinski
  2020-09-11 19:52 ` [PATCH net-next 2/8] docs: net: include the new ethtool pause stats in the stats doc Jakub Kicinski
@ 2020-09-11 19:52 ` Jakub Kicinski
  2020-09-12 14:38     ` kernel test robot
  2020-09-12 14:38   ` [RFC PATCH] netdevsim: nsim_ethtool_ops can be static kernel test robot
  2020-09-11 19:52 ` [PATCH net-next 4/8] selftests: add a test for ethtool pause stats Jakub Kicinski
                   ` (4 subsequent siblings)
  7 siblings, 2 replies; 23+ messages in thread
From: Jakub Kicinski @ 2020-09-11 19:52 UTC (permalink / raw)
  To: davem
  Cc: netdev, mkubecek, michael.chan, tariqt, saeedm, alexander.duyck,
	andrew, Jakub Kicinski

Add minimal ethtool interface for testing ethtool pause stats.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 drivers/net/netdevsim/Makefile    |  2 +-
 drivers/net/netdevsim/ethtool.c   | 64 +++++++++++++++++++++++++++++++
 drivers/net/netdevsim/netdev.c    |  1 +
 drivers/net/netdevsim/netdevsim.h | 11 ++++++
 4 files changed, 77 insertions(+), 1 deletion(-)
 create mode 100644 drivers/net/netdevsim/ethtool.c

diff --git a/drivers/net/netdevsim/Makefile b/drivers/net/netdevsim/Makefile
index 4dfb389dbfd8..ade086eed955 100644
--- a/drivers/net/netdevsim/Makefile
+++ b/drivers/net/netdevsim/Makefile
@@ -3,7 +3,7 @@
 obj-$(CONFIG_NETDEVSIM) += netdevsim.o
 
 netdevsim-objs := \
-	netdev.o dev.o fib.o bus.o health.o udp_tunnels.o
+	netdev.o dev.o ethtool.o fib.o bus.o health.o udp_tunnels.o
 
 ifeq ($(CONFIG_BPF_SYSCALL),y)
 netdevsim-objs += \
diff --git a/drivers/net/netdevsim/ethtool.c b/drivers/net/netdevsim/ethtool.c
new file mode 100644
index 000000000000..7c016edfbcee
--- /dev/null
+++ b/drivers/net/netdevsim/ethtool.c
@@ -0,0 +1,64 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2020 Facebook
+
+#include <linux/debugfs.h>
+#include <linux/ethtool.h>
+#include <linux/random.h>
+
+#include "netdevsim.h"
+
+static void
+nsim_get_pause_stats(struct net_device *dev,
+		     struct ethtool_pause_stats *pause_stats)
+{
+	struct netdevsim *ns = netdev_priv(dev);
+
+	if (ns->ethtool.report_stats_rx)
+		pause_stats->rx_pause_frames = 1;
+	if (ns->ethtool.report_stats_tx)
+		pause_stats->tx_pause_frames = 2;
+}
+
+static void
+nsim_get_pauseparam(struct net_device *dev, struct ethtool_pauseparam *pause)
+{
+	struct netdevsim *ns = netdev_priv(dev);
+
+	pause->autoneg = 0; /* We don't support ksettings, so can't pretend */
+	pause->rx_pause = ns->ethtool.rx;
+	pause->tx_pause = ns->ethtool.tx;
+}
+
+static int
+nsim_set_pauseparam(struct net_device *dev, struct ethtool_pauseparam *pause)
+{
+	struct netdevsim *ns = netdev_priv(dev);
+
+	if (pause->autoneg)
+		return -EINVAL;
+
+	ns->ethtool.rx = pause->rx_pause;
+	ns->ethtool.tx = pause->tx_pause;
+	return 0;
+}
+
+const struct ethtool_ops nsim_ethtool_ops = {
+	.get_pause_stats	= nsim_get_pause_stats,
+	.get_pauseparam		= nsim_get_pauseparam,
+	.set_pauseparam		= nsim_set_pauseparam,
+};
+
+void nsim_ethtool_init(struct netdevsim *ns)
+{
+	struct dentry *ethtool, *dir;
+
+	ns->netdev->ethtool_ops = &nsim_ethtool_ops;
+
+	ethtool = debugfs_create_dir("ethtool", ns->nsim_dev->ddir);
+
+	dir = debugfs_create_dir("pause", ethtool);
+	debugfs_create_bool("report_stats_rx", 0600, dir,
+			    &ns->ethtool.report_stats_rx);
+	debugfs_create_bool("report_stats_tx", 0600, dir,
+			    &ns->ethtool.report_stats_tx);
+}
diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
index 97cfb015a50b..7178468302c8 100644
--- a/drivers/net/netdevsim/netdev.c
+++ b/drivers/net/netdevsim/netdev.c
@@ -301,6 +301,7 @@ nsim_create(struct nsim_dev *nsim_dev, struct nsim_dev_port *nsim_dev_port)
 	ns->nsim_bus_dev = nsim_dev->nsim_bus_dev;
 	SET_NETDEV_DEV(dev, &ns->nsim_bus_dev->dev);
 	dev->netdev_ops = &nsim_netdev_ops;
+	nsim_ethtool_init(ns);
 
 	err = nsim_udp_tunnels_info_create(nsim_dev, dev);
 	if (err)
diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h
index 284f7092241d..0c86561e6d8d 100644
--- a/drivers/net/netdevsim/netdevsim.h
+++ b/drivers/net/netdevsim/netdevsim.h
@@ -50,6 +50,13 @@ struct nsim_ipsec {
 	u32 ok;
 };
 
+struct nsim_ethtool {
+	bool rx;
+	bool tx;
+	bool report_stats_rx;
+	bool report_stats_tx;
+};
+
 struct netdevsim {
 	struct net_device *netdev;
 	struct nsim_dev *nsim_dev;
@@ -80,12 +87,16 @@ struct netdevsim {
 		u32 ports[2][NSIM_UDP_TUNNEL_N_PORTS];
 		struct debugfs_u32_array dfs_ports[2];
 	} udp_ports;
+
+	struct nsim_ethtool ethtool;
 };
 
 struct netdevsim *
 nsim_create(struct nsim_dev *nsim_dev, struct nsim_dev_port *nsim_dev_port);
 void nsim_destroy(struct netdevsim *ns);
 
+void nsim_ethtool_init(struct netdevsim *ns);
+
 void nsim_udp_tunnels_debugfs_create(struct nsim_dev *nsim_dev);
 int nsim_udp_tunnels_info_create(struct nsim_dev *nsim_dev,
 				 struct net_device *dev);
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 23+ messages in thread
* [PATCH net-next 4/8] selftests: add a test for ethtool pause stats
  2020-09-11 19:52 [PATCH net-next 0/7] ethtool: add pause frame stats Jakub Kicinski
                   ` (2 preceding siblings ...)
  2020-09-11 19:52 ` [PATCH net-next 3/8] netdevsim: add pause frame stats Jakub Kicinski
@ 2020-09-11 19:52 ` Jakub Kicinski
  2020-09-11 19:52 ` [PATCH net-next 5/8] bnxt: add pause frame stats Jakub Kicinski
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 23+ messages in thread
From: Jakub Kicinski @ 2020-09-11 19:52 UTC (permalink / raw)
  To: davem
  Cc: netdev, mkubecek, michael.chan, tariqt, saeedm, alexander.duyck,
	andrew, Jakub Kicinski

Make sure the empty nest is reported even without stats.
Make sure reporting only selected stats works fine.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 .../drivers/net/netdevsim/ethtool-pause.sh    | 108 ++++++++++++++++++
 1 file changed, 108 insertions(+)
 create mode 100755 tools/testing/selftests/drivers/net/netdevsim/ethtool-pause.sh

diff --git a/tools/testing/selftests/drivers/net/netdevsim/ethtool-pause.sh b/tools/testing/selftests/drivers/net/netdevsim/ethtool-pause.sh
new file mode 100755
index 000000000000..dd6ad6501c9c
--- /dev/null
+++ b/tools/testing/selftests/drivers/net/netdevsim/ethtool-pause.sh
@@ -0,0 +1,108 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0-only
+
+NSIM_ID=$((RANDOM % 1024))
+NSIM_DEV_SYS=/sys/bus/netdevsim/devices/netdevsim$NSIM_ID
+NSIM_DEV_DFS=/sys/kernel/debug/netdevsim/netdevsim$NSIM_ID
+NSIM_NETDEV=
+num_passes=0
+num_errors=0
+
+function cleanup_nsim {
+    if [ -e $NSIM_DEV_SYS ]; then
+	echo $NSIM_ID > /sys/bus/netdevsim/del_device
+    fi
+}
+
+function cleanup {
+    cleanup_nsim
+}
+
+trap cleanup EXIT
+
+function get_netdev_name {
+    local -n old=$1
+
+    new=$(ls /sys/class/net)
+
+    for netdev in $new; do
+	for check in $old; do
+            [ $netdev == $check ] && break
+	done
+
+	if [ $netdev != $check ]; then
+	    echo $netdev
+	    break
+	fi
+    done
+}
+
+function check {
+    local code=$1
+    local str=$2
+    local exp_str=$3
+
+    if [ $code -ne 0 ]; then
+	((num_errors++))
+	return
+    fi
+
+    if [ "$str" != "$exp_str"  ]; then
+	echo -e "Expected: '$exp_str', got '$str'"
+	((num_errors++))
+	return
+    fi
+
+    ((num_passes++))
+}
+
+# Bail if ethtool is too old
+if ! ethtool -h | grep include-stat 2>&1 >/dev/null; then
+    echo "SKIP: No --include-statistics support in ethtool"
+    exit 4
+fi
+
+# Make a netdevsim
+old_netdevs=$(ls /sys/class/net)
+
+modprobe netdevsim
+echo $NSIM_ID > /sys/bus/netdevsim/new_device
+
+NSIM_NETDEV=`get_netdev_name old_netdevs`
+
+set -o pipefail
+
+echo n > $NSIM_DEV_DFS/ethtool/pause/report_stats_tx
+echo n > $NSIM_DEV_DFS/ethtool/pause/report_stats_rx
+
+s=$(ethtool --json -a $NSIM_NETDEV | jq '.[].statistics')
+check $? "$s" "null"
+
+s=$(ethtool -I --json -a $NSIM_NETDEV | jq '.[].statistics')
+check $? "$s" "{}"
+
+echo y > $NSIM_DEV_DFS/ethtool/pause/report_stats_tx
+
+s=$(ethtool -I --json -a $NSIM_NETDEV | jq '.[].statistics | length')
+check $? "$s" "1"
+
+s=$(ethtool -I --json -a $NSIM_NETDEV | jq '.[].statistics.tx_pause_frames')
+check $? "$s" "2"
+
+echo y > $NSIM_DEV_DFS/ethtool/pause/report_stats_rx
+
+s=$(ethtool -I --json -a $NSIM_NETDEV | jq '.[].statistics | length')
+check $? "$s" "2"
+
+s=$(ethtool -I --json -a $NSIM_NETDEV | jq '.[].statistics.rx_pause_frames')
+check $? "$s" "1"
+s=$(ethtool -I --json -a $NSIM_NETDEV | jq '.[].statistics.tx_pause_frames')
+check $? "$s" "2"
+
+if [ $num_errors -eq 0 ]; then
+    echo "PASSED all $((num_passes)) checks"
+    exit 0
+else
+    echo "FAILED $num_errors/$((num_errors+num_passes)) checks"
+    exit 1
+fi
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 23+ messages in thread
* [PATCH net-next 5/8] bnxt: add pause frame stats
  2020-09-11 19:52 [PATCH net-next 0/7] ethtool: add pause frame stats Jakub Kicinski
                   ` (3 preceding siblings ...)
  2020-09-11 19:52 ` [PATCH net-next 4/8] selftests: add a test for ethtool pause stats Jakub Kicinski
@ 2020-09-11 19:52 ` Jakub Kicinski
  2020-09-11 22:34   ` Michael Chan
  2020-09-12 15:57   ` kernel test robot
  2020-09-11 19:52 ` [PATCH net-next 6/8] mlx5: " Jakub Kicinski
                   ` (2 subsequent siblings)
  7 siblings, 2 replies; 23+ messages in thread
From: Jakub Kicinski @ 2020-09-11 19:52 UTC (permalink / raw)
  To: davem
  Cc: netdev, mkubecek, michael.chan, tariqt, saeedm, alexander.duyck,
	andrew, Jakub Kicinski

These stats are already reported in ethtool -S.
Hopefully they are equivalent to standard stats?

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 .../net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
index d0928334bdc8..b5de242766e3 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
@@ -1778,6 +1778,24 @@ static void bnxt_get_pauseparam(struct net_device *dev,
 	epause->tx_pause = !!(link_info->req_flow_ctrl & BNXT_LINK_PAUSE_TX);
 }
 
+static void bnxt_get_pause_stats(struct net_device *dev,
+				 struct ethtool_pause_stats *epstat)
+{
+	struct bnxt *bp = netdev_priv(dev);
+	struct rx_port_stats *rx_stats;
+	struct tx_port_stats *tx_stats;
+
+	if (BNXT_VF(bp) || !(bp->flags & BNXT_FLAG_PORT_STATS))
+		return;
+
+	rx_stats = (void *)bp->port_stats.sw_stats;
+	tx_stats = (void *)((unsigned long)bp->port_stats.sw_stats +
+			    BNXT_TX_PORT_STATS_BYTE_OFFSET);
+
+	epstat->rx_pause_frames = rx_stats->rx_pause_frames;
+	epstat->tx_pause_frames = tx_stats->tx_pause_frames;
+}
+
 static int bnxt_set_pauseparam(struct net_device *dev,
 			       struct ethtool_pauseparam *epause)
 {
@@ -3645,6 +3663,7 @@ const struct ethtool_ops bnxt_ethtool_ops = {
 				     ETHTOOL_COALESCE_USE_ADAPTIVE_RX,
 	.get_link_ksettings	= bnxt_get_link_ksettings,
 	.set_link_ksettings	= bnxt_set_link_ksettings,
+	.get_pause_stats	= bnxt_get_pause_stats,
 	.get_pauseparam		= bnxt_get_pauseparam,
 	.set_pauseparam		= bnxt_set_pauseparam,
 	.get_drvinfo		= bnxt_get_drvinfo,
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 23+ messages in thread
* [PATCH net-next 6/8] mlx5: add pause frame stats
  2020-09-11 19:52 [PATCH net-next 0/7] ethtool: add pause frame stats Jakub Kicinski
                   ` (4 preceding siblings ...)
  2020-09-11 19:52 ` [PATCH net-next 5/8] bnxt: add pause frame stats Jakub Kicinski
@ 2020-09-11 19:52 ` Jakub Kicinski
  2020-09-11 21:49   ` Saeed Mahameed
  2020-09-11 19:52 ` [PATCH net-next 7/8] ixgbe: " Jakub Kicinski
  2020-09-11 19:52 ` [PATCH net-next 8/8] mlx4: " Jakub Kicinski
  7 siblings, 1 reply; 23+ messages in thread
From: Jakub Kicinski @ 2020-09-11 19:52 UTC (permalink / raw)
  To: davem
  Cc: netdev, mkubecek, michael.chan, tariqt, saeedm, alexander.duyck,
	andrew, Jakub Kicinski

Plumb through all the indirection and copy some code from
ethtool -S. The names of the group indicate that these are
the stats we are after.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 drivers/net/ethernet/mellanox/mlx5/core/en.h  |  2 ++
 .../ethernet/mellanox/mlx5/core/en_ethtool.c  | 15 ++++++++++
 .../net/ethernet/mellanox/mlx5/core/en_rep.c  |  9 ++++++
 .../ethernet/mellanox/mlx5/core/en_stats.c    | 30 +++++++++++++++++++
 .../ethernet/mellanox/mlx5/core/en_stats.h    |  3 ++
 5 files changed, 59 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en.h b/drivers/net/ethernet/mellanox/mlx5/core/en.h
index 4f33658da25a..615ac0dc248e 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en.h
@@ -1053,6 +1053,8 @@ int mlx5e_ethtool_get_ts_info(struct mlx5e_priv *priv,
 			      struct ethtool_ts_info *info);
 int mlx5e_ethtool_flash_device(struct mlx5e_priv *priv,
 			       struct ethtool_flash *flash);
+void mlx5e_ethtool_get_pause_stats(struct mlx5e_priv *priv,
+				   struct ethtool_pause_stats *pause_stats);
 void mlx5e_ethtool_get_pauseparam(struct mlx5e_priv *priv,
 				  struct ethtool_pauseparam *pauseparam);
 int mlx5e_ethtool_set_pauseparam(struct mlx5e_priv *priv,
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
index 5cb1e4839eb7..c9fb3c018b96 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
@@ -1341,6 +1341,20 @@ static int mlx5e_set_tunable(struct net_device *dev,
 	return err;
 }
 
+void mlx5e_ethtool_get_pause_stats(struct mlx5e_priv *priv,
+				   struct ethtool_pause_stats *pause_stats)
+{
+	mlx5e_stats_pause_get(priv, pause_stats);
+}
+
+static void mlx5e_get_pause_stats(struct net_device *netdev,
+				  struct ethtool_pause_stats *pause_stats)
+{
+	struct mlx5e_priv *priv = netdev_priv(netdev);
+
+	mlx5e_ethtool_get_pause_stats(priv, pause_stats);
+}
+
 void mlx5e_ethtool_get_pauseparam(struct mlx5e_priv *priv,
 				  struct ethtool_pauseparam *pauseparam)
 {
@@ -2033,6 +2047,7 @@ const struct ethtool_ops mlx5e_ethtool_ops = {
 	.set_rxnfc         = mlx5e_set_rxnfc,
 	.get_tunable       = mlx5e_get_tunable,
 	.set_tunable       = mlx5e_set_tunable,
+	.get_pause_stats   = mlx5e_get_pause_stats,
 	.get_pauseparam    = mlx5e_get_pauseparam,
 	.set_pauseparam    = mlx5e_set_pauseparam,
 	.get_ts_info       = mlx5e_get_ts_info,
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c b/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c
index 135ee26881c9..856ae4c8cb25 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c
@@ -288,6 +288,14 @@ static u32 mlx5e_rep_get_rxfh_indir_size(struct net_device *netdev)
 	return mlx5e_ethtool_get_rxfh_indir_size(priv);
 }
 
+static void mlx5e_uplink_rep_get_pause_stats(struct net_device *netdev,
+					     struct ethtool_pause_stats *stats)
+{
+	struct mlx5e_priv *priv = netdev_priv(netdev);
+
+	mlx5e_ethtool_get_pause_stats(priv, stats);
+}
+
 static void mlx5e_uplink_rep_get_pauseparam(struct net_device *netdev,
 					    struct ethtool_pauseparam *pauseparam)
 {
@@ -362,6 +370,7 @@ static const struct ethtool_ops mlx5e_uplink_rep_ethtool_ops = {
 	.set_rxfh          = mlx5e_set_rxfh,
 	.get_rxnfc         = mlx5e_get_rxnfc,
 	.set_rxnfc         = mlx5e_set_rxnfc,
+	.get_pause_stats   = mlx5e_uplink_rep_get_pause_stats,
 	.get_pauseparam    = mlx5e_uplink_rep_get_pauseparam,
 	.set_pauseparam    = mlx5e_uplink_rep_set_pauseparam,
 };
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
index e3b2f59408e6..97b03aa7535f 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
@@ -677,6 +677,36 @@ static MLX5E_DECLARE_STATS_GRP_OP_UPDATE_STATS(802_3)
 	mlx5_core_access_reg(mdev, in, sz, out, sz, MLX5_REG_PPCNT, 0, 0);
 }
 
+#define MLX5E_READ_CTR64_BE_F(ptr, c)			\
+	be64_to_cpu(*(__be64 *)((char *)ptr +		\
+		MLX5_BYTE_OFF(ppcnt_reg,		\
+			counter_set.eth_802_3_cntrs_grp_data_layout.c##_high)))
+
+void mlx5e_stats_pause_get(struct mlx5e_priv *priv,
+			   struct ethtool_pause_stats *pause_stats)
+{
+	struct mlx5e_pport_stats *pstats = &priv->stats.pport;
+	struct mlx5_core_dev *mdev = priv->mdev;
+	u32 in[MLX5_ST_SZ_DW(ppcnt_reg)] = {0};
+	int sz = MLX5_ST_SZ_BYTES(ppcnt_reg);
+	void *out;
+
+	if (!MLX5_BASIC_PPCNT_SUPPORTED(mdev))
+		return;
+
+	MLX5_SET(ppcnt_reg, in, local_port, 1);
+	out = pstats->IEEE_802_3_counters;
+	MLX5_SET(ppcnt_reg, in, grp, MLX5_IEEE_802_3_COUNTERS_GROUP);
+	mlx5_core_access_reg(mdev, in, sz, out, sz, MLX5_REG_PPCNT, 0, 0);
+
+	pause_stats->tx_pause_frames =
+		MLX5E_READ_CTR64_BE_F(&priv->stats.pport.IEEE_802_3_counters,
+				      a_pause_mac_ctrl_frames_transmitted);
+	pause_stats->rx_pause_frames =
+		MLX5E_READ_CTR64_BE_F(&priv->stats.pport.IEEE_802_3_counters,
+				      a_pause_mac_ctrl_frames_received);
+}
+
 #define PPORT_2863_OFF(c) \
 	MLX5_BYTE_OFF(ppcnt_reg, \
 		      counter_set.eth_2863_cntrs_grp_data_layout.c##_high)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.h b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.h
index 2e1cca1923b9..9d9ee269a041 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.h
@@ -104,6 +104,9 @@ void mlx5e_stats_update(struct mlx5e_priv *priv);
 void mlx5e_stats_fill(struct mlx5e_priv *priv, u64 *data, int idx);
 void mlx5e_stats_fill_strings(struct mlx5e_priv *priv, u8 *data);
 
+void mlx5e_stats_pause_get(struct mlx5e_priv *priv,
+			   struct ethtool_pause_stats *pause_stats);
+
 /* Concrete NIC Stats */
 
 struct mlx5e_sw_stats {
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 23+ messages in thread
* [PATCH net-next 7/8] ixgbe: add pause frame stats
  2020-09-11 19:52 [PATCH net-next 0/7] ethtool: add pause frame stats Jakub Kicinski
                   ` (5 preceding siblings ...)
  2020-09-11 19:52 ` [PATCH net-next 6/8] mlx5: " Jakub Kicinski
@ 2020-09-11 19:52 ` Jakub Kicinski
  2020-09-11 21:12   ` Alexander Duyck
  2020-09-11 19:52 ` [PATCH net-next 8/8] mlx4: " Jakub Kicinski
  7 siblings, 1 reply; 23+ messages in thread
From: Jakub Kicinski @ 2020-09-11 19:52 UTC (permalink / raw)
  To: davem
  Cc: netdev, mkubecek, michael.chan, tariqt, saeedm, alexander.duyck,
	andrew, Jakub Kicinski

Report standard pause frame stats. They are already aggregated
in struct ixgbe_hw_stats.

The combination of the registers is suggested as equivalent to
PAUSEMACCtrlFramesTransmitted / PAUSEMACCtrlFramesReceived
by the Intel 82576EB datasheet, I could not find any information
in the HW actually supported by ixgbe.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
index 71ec908266a6..a280aa34ca1d 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
@@ -531,6 +531,16 @@ static int ixgbe_set_link_ksettings(struct net_device *netdev,
 	return err;
 }
 
+static void ixgbe_get_pause_stats(struct net_device *netdev,
+				  struct ethtool_pause_stats *stats)
+{
+	struct ixgbe_adapter *adapter = netdev_priv(netdev);
+	struct ixgbe_hw_stats *hwstats = &adapter->stats;
+
+	stats->tx_pause_frames = hwstats->lxontxc + hwstats->lxofftxc;
+	stats->rx_pause_frames = hwstats->lxonrxc + hwstats->lxoffrxc;
+}
+
 static void ixgbe_get_pauseparam(struct net_device *netdev,
 				 struct ethtool_pauseparam *pause)
 {
@@ -3546,6 +3556,7 @@ static const struct ethtool_ops ixgbe_ethtool_ops = {
 	.set_eeprom             = ixgbe_set_eeprom,
 	.get_ringparam          = ixgbe_get_ringparam,
 	.set_ringparam          = ixgbe_set_ringparam,
+	.get_pause_stats	= ixgbe_get_pause_stats,
 	.get_pauseparam         = ixgbe_get_pauseparam,
 	.set_pauseparam         = ixgbe_set_pauseparam,
 	.get_msglevel           = ixgbe_get_msglevel,
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 23+ messages in thread
* [PATCH net-next 8/8] mlx4: add pause frame stats
  2020-09-11 19:52 [PATCH net-next 0/7] ethtool: add pause frame stats Jakub Kicinski
                   ` (6 preceding siblings ...)
  2020-09-11 19:52 ` [PATCH net-next 7/8] ixgbe: " Jakub Kicinski
@ 2020-09-11 19:52 ` Jakub Kicinski
  7 siblings, 0 replies; 23+ messages in thread
From: Jakub Kicinski @ 2020-09-11 19:52 UTC (permalink / raw)
  To: davem
  Cc: netdev, mkubecek, michael.chan, tariqt, saeedm, alexander.duyck,
	andrew, Jakub Kicinski

Check if the pause stats are reported by HW by checking the bitmap.
Calculation is based on the order of strings in main_strings from
ethtool -S. Hopefully the semantics of these stats match the standard..

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 .../net/ethernet/mellanox/mlx4/en_ethtool.c   | 19 +++++++++++++++++++
 .../net/ethernet/mellanox/mlx4/mlx4_stats.h   | 12 ++++++++++++
 2 files changed, 31 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
index b816154bc79a..23849f2b9c25 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
@@ -1106,6 +1106,24 @@ static int mlx4_en_set_pauseparam(struct net_device *dev,
 	return err;
 }
 
+static void mlx4_en_get_pause_stats(struct net_device *dev,
+				    struct ethtool_pause_stats *stats)
+{
+	struct mlx4_en_priv *priv = netdev_priv(dev);
+	struct bitmap_iterator it;
+
+	bitmap_iterator_init(&it, priv->stats_bitmap.bitmap, NUM_ALL_STATS);
+
+	spin_lock_bh(&priv->stats_lock);
+	if (test_bit(FLOW_PRIORITY_STATS_IDX_TX_FRAMES,
+		     priv->stats_bitmap.bitmap))
+		stats->tx_pause_frames = priv->tx_flowstats.tx_pause;
+	if (test_bit(FLOW_PRIORITY_STATS_IDX_RX_FRAMES,
+		     priv->stats_bitmap.bitmap))
+		stats->rx_pause_frames = priv->rx_flowstats.rx_pause;
+	spin_unlock_bh(&priv->stats_lock);
+}
+
 static void mlx4_en_get_pauseparam(struct net_device *dev,
 				 struct ethtool_pauseparam *pause)
 {
@@ -2138,6 +2156,7 @@ const struct ethtool_ops mlx4_en_ethtool_ops = {
 	.set_msglevel = mlx4_en_set_msglevel,
 	.get_coalesce = mlx4_en_get_coalesce,
 	.set_coalesce = mlx4_en_set_coalesce,
+	.get_pause_stats = mlx4_en_get_pause_stats,
 	.get_pauseparam = mlx4_en_get_pauseparam,
 	.set_pauseparam = mlx4_en_set_pauseparam,
 	.get_ringparam = mlx4_en_get_ringparam,
diff --git a/drivers/net/ethernet/mellanox/mlx4/mlx4_stats.h b/drivers/net/ethernet/mellanox/mlx4/mlx4_stats.h
index 86b6051da8ec..51d4eaab6a2f 100644
--- a/drivers/net/ethernet/mellanox/mlx4/mlx4_stats.h
+++ b/drivers/net/ethernet/mellanox/mlx4/mlx4_stats.h
@@ -84,6 +84,11 @@ struct mlx4_en_flow_stats_rx {
 					 MLX4_NUM_PRIORITIES)
 };
 
+#define FLOW_PRIORITY_STATS_IDX_RX_FRAMES	(NUM_MAIN_STATS +	\
+						 NUM_PORT_STATS +	\
+						 NUM_PF_STATS +		\
+						 NUM_FLOW_PRIORITY_STATS_RX)
+
 struct mlx4_en_flow_stats_tx {
 	u64 tx_pause;
 	u64 tx_pause_duration;
@@ -93,6 +98,13 @@ struct mlx4_en_flow_stats_tx {
 					 MLX4_NUM_PRIORITIES)
 };
 
+#define FLOW_PRIORITY_STATS_IDX_TX_FRAMES	(NUM_MAIN_STATS +	\
+						 NUM_PORT_STATS +	\
+						 NUM_PF_STATS +		\
+						 NUM_FLOW_PRIORITY_STATS_RX + \
+						 NUM_FLOW_STATS_RX +	\
+						 NUM_FLOW_PRIORITY_STATS_TX)
+
 #define NUM_FLOW_STATS (NUM_FLOW_STATS_RX + NUM_FLOW_STATS_TX + \
 			NUM_FLOW_PRIORITY_STATS_TX + \
 			NUM_FLOW_PRIORITY_STATS_RX)
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 23+ messages in thread
* Re: [PATCH net-next 7/8] ixgbe: add pause frame stats
  2020-09-11 19:52 ` [PATCH net-next 7/8] ixgbe: " Jakub Kicinski
@ 2020-09-11 21:12   ` Alexander Duyck
  2020-09-11 22:13     ` Jakub Kicinski
  0 siblings, 1 reply; 23+ messages in thread
From: Alexander Duyck @ 2020-09-11 21:12 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: David Miller, Netdev, Michal Kubecek, Michael Chan, tariqt,
	saeedm, Andrew Lunn

On Fri, Sep 11, 2020 at 12:53 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> Report standard pause frame stats. They are already aggregated
> in struct ixgbe_hw_stats.
>
> The combination of the registers is suggested as equivalent to
> PAUSEMACCtrlFramesTransmitted / PAUSEMACCtrlFramesReceived
> by the Intel 82576EB datasheet, I could not find any information
> in the HW actually supported by ixgbe.
>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
> index 71ec908266a6..a280aa34ca1d 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
> @@ -531,6 +531,16 @@ static int ixgbe_set_link_ksettings(struct net_device *netdev,
>         return err;
>  }
>
> +static void ixgbe_get_pause_stats(struct net_device *netdev,
> +                                 struct ethtool_pause_stats *stats)
> +{
> +       struct ixgbe_adapter *adapter = netdev_priv(netdev);
> +       struct ixgbe_hw_stats *hwstats = &adapter->stats;
> +
> +       stats->tx_pause_frames = hwstats->lxontxc + hwstats->lxofftxc;
> +       stats->rx_pause_frames = hwstats->lxonrxc + hwstats->lxoffrxc;
> +}
> +
>  static void ixgbe_get_pauseparam(struct net_device *netdev,
>                                  struct ethtool_pauseparam *pause)
>  {
> @@ -3546,6 +3556,7 @@ static const struct ethtool_ops ixgbe_ethtool_ops = {
>         .set_eeprom             = ixgbe_set_eeprom,
>         .get_ringparam          = ixgbe_get_ringparam,
>         .set_ringparam          = ixgbe_set_ringparam,
> +       .get_pause_stats        = ixgbe_get_pause_stats,
>         .get_pauseparam         = ixgbe_get_pauseparam,
>         .set_pauseparam         = ixgbe_set_pauseparam,
>         .get_msglevel           = ixgbe_get_msglevel,

So the count for this is simpler in igb than it is for ixgbe. I'm
assuming you want just standard link flow control frames. If so then
this patch is correct. Otherwise if you are wanting to capture
priority flow control data then those are a seperate array of stats
prefixed with a "p" instead of an "l". Otherwise this looks fine to
me.

Reviewed-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>

^ permalink raw reply	[flat|nested] 23+ messages in thread
* Re: [PATCH net-next 6/8] mlx5: add pause frame stats
  2020-09-11 19:52 ` [PATCH net-next 6/8] mlx5: " Jakub Kicinski
@ 2020-09-11 21:49   ` Saeed Mahameed
  0 siblings, 0 replies; 23+ messages in thread
From: Saeed Mahameed @ 2020-09-11 21:49 UTC (permalink / raw)
  To: kuba, davem
  Cc: mkubecek, Tariq Toukan, michael.chan, netdev, alexander.duyck, andrew

On Fri, 2020-09-11 at 12:52 -0700, Jakub Kicinski wrote:
> Plumb through all the indirection and copy some code from
> ethtool -S. The names of the group indicate that these are
> the stats we are after.
> 

Yes they are.

> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  drivers/net/ethernet/mellanox/mlx5/core/en.h  |  2 ++
>  .../ethernet/mellanox/mlx5/core/en_ethtool.c  | 15 ++++++++++
>  .../net/ethernet/mellanox/mlx5/core/en_rep.c  |  9 ++++++
>  .../ethernet/mellanox/mlx5/core/en_stats.c    | 30
> +++++++++++++++++++
>  .../ethernet/mellanox/mlx5/core/en_stats.h    |  3 ++
>  5 files changed, 59 insertions(+)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en.h
> b/drivers/net/ethernet/mellanox/mlx5/core/en.h
> index 4f33658da25a..615ac0dc248e 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en.h
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en.h
> @@ -1053,6 +1053,8 @@ int mlx5e_ethtool_get_ts_info(struct mlx5e_priv
> *priv,
>  			      struct ethtool_ts_info *info);
>  int mlx5e_ethtool_flash_device(struct mlx5e_priv *priv,
>  			       struct ethtool_flash *flash);
> +void mlx5e_ethtool_get_pause_stats(struct mlx5e_priv *priv,
> +				   struct ethtool_pause_stats
> *pause_stats);
>  void mlx5e_ethtool_get_pauseparam(struct mlx5e_priv *priv,
>  				  struct ethtool_pauseparam
> *pauseparam);
>  int mlx5e_ethtool_set_pauseparam(struct mlx5e_priv *priv,
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
> b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
> index 5cb1e4839eb7..c9fb3c018b96 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
> @@ -1341,6 +1341,20 @@ static int mlx5e_set_tunable(struct net_device
> *dev,
>  	return err;
>  }
>  
> +void mlx5e_ethtool_get_pause_stats(struct mlx5e_priv *priv,
> +				   struct ethtool_pause_stats
> *pause_stats)
> +{
> +	mlx5e_stats_pause_get(priv, pause_stats);
this function is only being called here, I would just unfold it here
and skip the redundant definition in en_stat.c and declaration in
en_stats.h.

> +}
> +
> +static void mlx5e_get_pause_stats(struct net_device *netdev,
> +				  struct ethtool_pause_stats
> *pause_stats)
> +{
> +	struct mlx5e_priv *priv = netdev_priv(netdev);
> +
> +	mlx5e_ethtool_get_pause_stats(priv, pause_stats);
> +}
> +
>  void mlx5e_ethtool_get_pauseparam(struct mlx5e_priv *priv,
>  				  struct ethtool_pauseparam
> *pauseparam)
>  {
> @@ -2033,6 +2047,7 @@ const struct ethtool_ops mlx5e_ethtool_ops = {
>  	.set_rxnfc         = mlx5e_set_rxnfc,
>  	.get_tunable       = mlx5e_get_tunable,
>  	.set_tunable       = mlx5e_set_tunable,
> +	.get_pause_stats   = mlx5e_get_pause_stats,
>  	.get_pauseparam    = mlx5e_get_pauseparam,
>  	.set_pauseparam    = mlx5e_set_pauseparam,
>  	.get_ts_info       = mlx5e_get_ts_info,
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c
> b/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c
> index 135ee26881c9..856ae4c8cb25 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c
> @@ -288,6 +288,14 @@ static u32 mlx5e_rep_get_rxfh_indir_size(struct
> net_device *netdev)
>  	return mlx5e_ethtool_get_rxfh_indir_size(priv);
>  }
>  
> +static void mlx5e_uplink_rep_get_pause_stats(struct net_device
> *netdev,
> +					     struct ethtool_pause_stats
> *stats)
> +{
> +	struct mlx5e_priv *priv = netdev_priv(netdev);
> +
> +	mlx5e_ethtool_get_pause_stats(priv, stats);
> +}
> +
>  static void mlx5e_uplink_rep_get_pauseparam(struct net_device
> *netdev,
>  					    struct ethtool_pauseparam
> *pauseparam)
>  {
> @@ -362,6 +370,7 @@ static const struct ethtool_ops
> mlx5e_uplink_rep_ethtool_ops = {
>  	.set_rxfh          = mlx5e_set_rxfh,
>  	.get_rxnfc         = mlx5e_get_rxnfc,
>  	.set_rxnfc         = mlx5e_set_rxnfc,
> +	.get_pause_stats   = mlx5e_uplink_rep_get_pause_stats,
>  	.get_pauseparam    = mlx5e_uplink_rep_get_pauseparam,
>  	.set_pauseparam    = mlx5e_uplink_rep_set_pauseparam,
>  };
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
> b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
> index e3b2f59408e6..97b03aa7535f 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
> @@ -677,6 +677,36 @@ static
> MLX5E_DECLARE_STATS_GRP_OP_UPDATE_STATS(802_3)
>  	mlx5_core_access_reg(mdev, in, sz, out, sz, MLX5_REG_PPCNT, 0,
> 0);
>  }
>  
> +#define MLX5E_READ_CTR64_BE_F(ptr, c)			\
> +	be64_to_cpu(*(__be64 *)((char *)ptr +		\
> +		MLX5_BYTE_OFF(ppcnt_reg,		\
> +			counter_set.eth_802_3_cntrs_grp_data_layout.c##
> _high)))
> +
> +void mlx5e_stats_pause_get(struct mlx5e_priv *priv,
> +			   struct ethtool_pause_stats *pause_stats)
> +{
> +	struct mlx5e_pport_stats *pstats = &priv->stats.pport;
> +	struct mlx5_core_dev *mdev = priv->mdev;
> +	u32 in[MLX5_ST_SZ_DW(ppcnt_reg)] = {0};
     no need for explicit array initializer ^^ just {} should be good
enough.

> +	int sz = MLX5_ST_SZ_BYTES(ppcnt_reg);
> +	void *out;
> +
> +	if (!MLX5_BASIC_PPCNT_SUPPORTED(mdev))
> +		return;
> +
> +	MLX5_SET(ppcnt_reg, in, local_port, 1);
> +	out = pstats->IEEE_802_3_counters;

you are reading the outbox buffer into a shared buffer, and this could
lead to weird race conditions with ethtool stats, maybe both ethtool
stats and pause_stats are protected with rtnl_lock, but let's not make
any assumption here and use a local buffer for this flow anyway.

just define "out" to be
u32 ppcnt_ieee_802_3[MLX5_ST_SZ_DW(ppcnt_reg)];

I hope this is not too big for the stack.

> +	MLX5_SET(ppcnt_reg, in, grp, MLX5_IEEE_802_3_COUNTERS_GROUP);
> +	mlx5_core_access_reg(mdev, in, sz, out, sz, MLX5_REG_PPCNT, 0, 
                        replace "out" here ^^^ with ppcnt_ieee_802_3

> 0);
> +
> +	pause_stats->tx_pause_frames =
> +		MLX5E_READ_CTR64_BE_F(&priv-
> >stats.pport.IEEE_802_3_counters,
> +				      a_pause_mac_ctrl_frames_transmitt
> ed);
> +	pause_stats->rx_pause_frames =
> +		MLX5E_READ_CTR64_BE_F(&priv-
> >stats.pport.IEEE_802_3_counters,
> +				      a_pause_mac_ctrl_frames_received)
> ;

in the above 2 lines you could have just used "out" instead of 
&priv->stats.pport.IEEE_802_3_counters, but with my suggestion above
just use the local buffer.



> +}
> +
>  #define PPORT_2863_OFF(c) \
>  	MLX5_BYTE_OFF(ppcnt_reg, \
>  		      counter_set.eth_2863_cntrs_grp_data_layout.c##_hi
> gh)
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.h
> b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.h
> index 2e1cca1923b9..9d9ee269a041 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.h
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.h
> @@ -104,6 +104,9 @@ void mlx5e_stats_update(struct mlx5e_priv *priv);
>  void mlx5e_stats_fill(struct mlx5e_priv *priv, u64 *data, int idx);
>  void mlx5e_stats_fill_strings(struct mlx5e_priv *priv, u8 *data);
>  
> +void mlx5e_stats_pause_get(struct mlx5e_priv *priv,
> +			   struct ethtool_pause_stats *pause_stats);
> +
>  /* Concrete NIC Stats */
>  
>  struct mlx5e_sw_stats {

^ permalink raw reply	[flat|nested] 23+ messages in thread
* Re: [PATCH net-next 7/8] ixgbe: add pause frame stats
  2020-09-11 21:12   ` Alexander Duyck
@ 2020-09-11 22:13     ` Jakub Kicinski
  2020-09-13  9:14       ` Ido Schimmel
  0 siblings, 1 reply; 23+ messages in thread
From: Jakub Kicinski @ 2020-09-11 22:13 UTC (permalink / raw)
  To: Alexander Duyck
  Cc: David Miller, Netdev, Michal Kubecek, Michael Chan, tariqt,
	saeedm, Andrew Lunn, Ido Schimmel

On Fri, 11 Sep 2020 14:12:50 -0700 Alexander Duyck wrote:
> On Fri, Sep 11, 2020 at 12:53 PM Jakub Kicinski <kuba@kernel.org> wrote:
> > @@ -3546,6 +3556,7 @@ static const struct ethtool_ops ixgbe_ethtool_ops = {
> >         .set_eeprom             = ixgbe_set_eeprom,
> >         .get_ringparam          = ixgbe_get_ringparam,
> >         .set_ringparam          = ixgbe_set_ringparam,
> > +       .get_pause_stats        = ixgbe_get_pause_stats,
> >         .get_pauseparam         = ixgbe_get_pauseparam,
> >         .set_pauseparam         = ixgbe_set_pauseparam,
> >         .get_msglevel           = ixgbe_get_msglevel,  
> 
> So the count for this is simpler in igb than it is for ixgbe. I'm
> assuming you want just standard link flow control frames. If so then
> this patch is correct. Otherwise if you are wanting to capture
> priority flow control data then those are a seperate array of stats
> prefixed with a "p" instead of an "l". Otherwise this looks fine to
> me.

That's my interpretation, although I haven't found any place the
standard would address this directly. Non-PFC pause has a different
opcode, so I'm reasonably certain this makes sense.

BTW I'm not entirely clear on what "global PFC pause" is either.

Maybe someone can clarify? Mellanox folks?

> Reviewed-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>

Thanks!


^ permalink raw reply	[flat|nested] 23+ messages in thread
* Re: [PATCH net-next 5/8] bnxt: add pause frame stats
  2020-09-11 19:52 ` [PATCH net-next 5/8] bnxt: add pause frame stats Jakub Kicinski
@ 2020-09-11 22:34   ` Michael Chan
  2020-09-11 22:43     ` Jakub Kicinski
  2020-09-12 15:57   ` kernel test robot
  1 sibling, 1 reply; 23+ messages in thread
From: Michael Chan @ 2020-09-11 22:34 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: David Miller, Netdev, mkubecek, tariqt, saeedm, Alexander Duyck,
	Andrew Lunn

On Fri, Sep 11, 2020 at 12:53 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> These stats are already reported in ethtool -S.
> Hopefully they are equivalent to standard stats?

Yes.

>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  .../net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> index d0928334bdc8..b5de242766e3 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> @@ -1778,6 +1778,24 @@ static void bnxt_get_pauseparam(struct net_device *dev,
>         epause->tx_pause = !!(link_info->req_flow_ctrl & BNXT_LINK_PAUSE_TX);
>  }
>
> +static void bnxt_get_pause_stats(struct net_device *dev,
> +                                struct ethtool_pause_stats *epstat)
> +{
> +       struct bnxt *bp = netdev_priv(dev);
> +       struct rx_port_stats *rx_stats;
> +       struct tx_port_stats *tx_stats;
> +
> +       if (BNXT_VF(bp) || !(bp->flags & BNXT_FLAG_PORT_STATS))
> +               return;
> +
> +       rx_stats = (void *)bp->port_stats.sw_stats;
> +       tx_stats = (void *)((unsigned long)bp->port_stats.sw_stats +
> +                           BNXT_TX_PORT_STATS_BYTE_OFFSET);
> +
> +       epstat->rx_pause_frames = rx_stats->rx_pause_frames;
> +       epstat->tx_pause_frames = tx_stats->tx_pause_frames;

This will work, but the types on the 2 sides don't match.  On the
right hand side, since you are casting to the hardware struct
rx_port_stats and tx_port_stats, the types are __le64.

If rx_stats and tx_stats are *u64 and you use these macros:

BNXT_GET_RX_PORT_STATS64(rx_stats, rx_pause_frames)
BNXT_GET_TX_PORT_STATS64(tx_stats, tx_pause_frames)

the results will be the same with native CPU u64 types.

Thanks.

^ permalink raw reply	[flat|nested] 23+ messages in thread
* Re: [PATCH net-next 5/8] bnxt: add pause frame stats
  2020-09-11 22:34   ` Michael Chan
@ 2020-09-11 22:43     ` Jakub Kicinski
  2020-09-11 22:46       ` Jakub Kicinski
  2020-09-11 22:53       ` Michael Chan
  0 siblings, 2 replies; 23+ messages in thread
From: Jakub Kicinski @ 2020-09-11 22:43 UTC (permalink / raw)
  To: Michael Chan
  Cc: David Miller, Netdev, mkubecek, tariqt, saeedm, Alexander Duyck,
	Andrew Lunn

On Fri, 11 Sep 2020 15:34:24 -0700 Michael Chan wrote:
> On Fri, Sep 11, 2020 at 12:53 PM Jakub Kicinski <kuba@kernel.org> wrote:
> >
> > These stats are already reported in ethtool -S.
> > Hopefully they are equivalent to standard stats?  
> 
> Yes.
> 
> >
> > Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> > ---
> >  .../net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 19 +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> > index d0928334bdc8..b5de242766e3 100644
> > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> > @@ -1778,6 +1778,24 @@ static void bnxt_get_pauseparam(struct net_device *dev,
> >         epause->tx_pause = !!(link_info->req_flow_ctrl & BNXT_LINK_PAUSE_TX);
> >  }
> >
> > +static void bnxt_get_pause_stats(struct net_device *dev,
> > +                                struct ethtool_pause_stats *epstat)
> > +{
> > +       struct bnxt *bp = netdev_priv(dev);
> > +       struct rx_port_stats *rx_stats;
> > +       struct tx_port_stats *tx_stats;
> > +
> > +       if (BNXT_VF(bp) || !(bp->flags & BNXT_FLAG_PORT_STATS))
> > +               return;
> > +
> > +       rx_stats = (void *)bp->port_stats.sw_stats;
> > +       tx_stats = (void *)((unsigned long)bp->port_stats.sw_stats +
> > +                           BNXT_TX_PORT_STATS_BYTE_OFFSET);
> > +
> > +       epstat->rx_pause_frames = rx_stats->rx_pause_frames;
> > +       epstat->tx_pause_frames = tx_stats->tx_pause_frames;  
> 
> This will work, but the types on the 2 sides don't match.  On the
> right hand side, since you are casting to the hardware struct
> rx_port_stats and tx_port_stats, the types are __le64.
> 
> If rx_stats and tx_stats are *u64 and you use these macros:
> 
> BNXT_GET_RX_PORT_STATS64(rx_stats, rx_pause_frames)
> BNXT_GET_TX_PORT_STATS64(tx_stats, tx_pause_frames)
> 
> the results will be the same with native CPU u64 types.

Thanks! My build bot just poked me about this as well.

I don't see any byte swaps in bnxt_get_ethtool_stats() - 
are they not needed there? I'm slightly confused.

^ permalink raw reply	[flat|nested] 23+ messages in thread
* Re: [PATCH net-next 5/8] bnxt: add pause frame stats
  2020-09-11 22:43     ` Jakub Kicinski
@ 2020-09-11 22:46       ` Jakub Kicinski
  2020-09-11 22:53       ` Michael Chan
  1 sibling, 0 replies; 23+ messages in thread
From: Jakub Kicinski @ 2020-09-11 22:46 UTC (permalink / raw)
  To: Michael Chan
  Cc: David Miller, Netdev, mkubecek, tariqt, saeedm, Alexander Duyck,
	Andrew Lunn

On Fri, 11 Sep 2020 15:43:43 -0700 Jakub Kicinski wrote:
> > > +       struct bnxt *bp = netdev_priv(dev);
> > > +       struct rx_port_stats *rx_stats;
> > > +       struct tx_port_stats *tx_stats;
> > > +
> > > +       if (BNXT_VF(bp) || !(bp->flags & BNXT_FLAG_PORT_STATS))
> > > +               return;
> > > +
> > > +       rx_stats = (void *)bp->port_stats.sw_stats;
> > > +       tx_stats = (void *)((unsigned long)bp->port_stats.sw_stats +
> > > +                           BNXT_TX_PORT_STATS_BYTE_OFFSET);
> > > +
> > > +       epstat->rx_pause_frames = rx_stats->rx_pause_frames;
> > > +       epstat->tx_pause_frames = tx_stats->tx_pause_frames;    
> > 
> > This will work, but the types on the 2 sides don't match.  On the
> > right hand side, since you are casting to the hardware struct
> > rx_port_stats and tx_port_stats, the types are __le64.
> > 
> > If rx_stats and tx_stats are *u64 and you use these macros:
> > 
> > BNXT_GET_RX_PORT_STATS64(rx_stats, rx_pause_frames)
> > BNXT_GET_TX_PORT_STATS64(tx_stats, tx_pause_frames)
> > 
> > the results will be the same with native CPU u64 types.  
> 
> Thanks! My build bot just poked me about this as well.
> 
> I don't see any byte swaps in bnxt_get_ethtool_stats() - 
> are they not needed there? I'm slightly confused.

Oof those macros don't byte swap either, even more confused now :)

^ permalink raw reply	[flat|nested] 23+ messages in thread
* Re: [PATCH net-next 5/8] bnxt: add pause frame stats
  2020-09-11 22:43     ` Jakub Kicinski
  2020-09-11 22:46       ` Jakub Kicinski
@ 2020-09-11 22:53       ` Michael Chan
  2020-09-11 22:58         ` Jakub Kicinski
  1 sibling, 1 reply; 23+ messages in thread
From: Michael Chan @ 2020-09-11 22:53 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: David Miller, Netdev, mkubecek, tariqt, saeedm, Alexander Duyck,
	Andrew Lunn

On Fri, Sep 11, 2020 at 3:43 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Fri, 11 Sep 2020 15:34:24 -0700 Michael Chan wrote:
> > On Fri, Sep 11, 2020 at 12:53 PM Jakub Kicinski <kuba@kernel.org> wrote:
> > >
> > > These stats are already reported in ethtool -S.
> > > Hopefully they are equivalent to standard stats?
> >
> > Yes.
> >
> > >
> > > Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> > > ---
> > >  .../net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 19 +++++++++++++++++++
> > >  1 file changed, 19 insertions(+)
> > >
> > > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> > > index d0928334bdc8..b5de242766e3 100644
> > > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> > > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> > > @@ -1778,6 +1778,24 @@ static void bnxt_get_pauseparam(struct net_device *dev,
> > >         epause->tx_pause = !!(link_info->req_flow_ctrl & BNXT_LINK_PAUSE_TX);
> > >  }
> > >
> > > +static void bnxt_get_pause_stats(struct net_device *dev,
> > > +                                struct ethtool_pause_stats *epstat)
> > > +{
> > > +       struct bnxt *bp = netdev_priv(dev);
> > > +       struct rx_port_stats *rx_stats;
> > > +       struct tx_port_stats *tx_stats;
> > > +
> > > +       if (BNXT_VF(bp) || !(bp->flags & BNXT_FLAG_PORT_STATS))
> > > +               return;
> > > +
> > > +       rx_stats = (void *)bp->port_stats.sw_stats;
> > > +       tx_stats = (void *)((unsigned long)bp->port_stats.sw_stats +
> > > +                           BNXT_TX_PORT_STATS_BYTE_OFFSET);
> > > +
> > > +       epstat->rx_pause_frames = rx_stats->rx_pause_frames;
> > > +       epstat->tx_pause_frames = tx_stats->tx_pause_frames;
> >
> > This will work, but the types on the 2 sides don't match.  On the
> > right hand side, since you are casting to the hardware struct
> > rx_port_stats and tx_port_stats, the types are __le64.
> >
> > If rx_stats and tx_stats are *u64 and you use these macros:
> >
> > BNXT_GET_RX_PORT_STATS64(rx_stats, rx_pause_frames)
> > BNXT_GET_TX_PORT_STATS64(tx_stats, tx_pause_frames)
> >
> > the results will be the same with native CPU u64 types.
>
> Thanks! My build bot just poked me about this as well.
>
> I don't see any byte swaps in bnxt_get_ethtool_stats() -
> are they not needed there? I'm slightly confused.

No, swapping is not needed since we are referencing the sw_stats.
Every counter has already been swapped when we did the copy and
overflow check from the hw struct to sw_stats.  sw_stats is exactly
the same as the hw struct except that every counter is already swapped
into native CPU u64 and properly adjusted for overflow.

^ permalink raw reply	[flat|nested] 23+ messages in thread
* Re: [PATCH net-next 5/8] bnxt: add pause frame stats
  2020-09-11 22:53       ` Michael Chan
@ 2020-09-11 22:58         ` Jakub Kicinski
  0 siblings, 0 replies; 23+ messages in thread
From: Jakub Kicinski @ 2020-09-11 22:58 UTC (permalink / raw)
  To: Michael Chan
  Cc: David Miller, Netdev, mkubecek, tariqt, saeedm, Alexander Duyck,
	Andrew Lunn

On Fri, 11 Sep 2020 15:53:24 -0700 Michael Chan wrote:
> > > This will work, but the types on the 2 sides don't match.  On the
> > > right hand side, since you are casting to the hardware struct
> > > rx_port_stats and tx_port_stats, the types are __le64.
> > >
> > > If rx_stats and tx_stats are *u64 and you use these macros:
> > >
> > > BNXT_GET_RX_PORT_STATS64(rx_stats, rx_pause_frames)
> > > BNXT_GET_TX_PORT_STATS64(tx_stats, tx_pause_frames)
> > >
> > > the results will be the same with native CPU u64 types.  
> >
> > Thanks! My build bot just poked me about this as well.
> >
> > I don't see any byte swaps in bnxt_get_ethtool_stats() -
> > are they not needed there? I'm slightly confused.  
> 
> No, swapping is not needed since we are referencing the sw_stats.
> Every counter has already been swapped when we did the copy and
> overflow check from the hw struct to sw_stats.  sw_stats is exactly
> the same as the hw struct except that every counter is already swapped
> into native CPU u64 and properly adjusted for overflow.

I see, I'll change the pointer types to u64 * as well. Thanks!

^ permalink raw reply	[flat|nested] 23+ messages in thread
* Re: [PATCH net-next 3/8] netdevsim: add pause frame stats
  2020-09-11 19:52 ` [PATCH net-next 3/8] netdevsim: add pause frame stats Jakub Kicinski
@ 2020-09-12 14:38     ` kernel test robot
  2020-09-12 14:38   ` [RFC PATCH] netdevsim: nsim_ethtool_ops can be static kernel test robot
  1 sibling, 0 replies; 23+ messages in thread
From: kernel test robot @ 2020-09-12 14:38 UTC (permalink / raw)
  To: Jakub Kicinski, davem
  Cc: kbuild-all, netdev, mkubecek, michael.chan, tariqt, saeedm,
	alexander.duyck, andrew, Jakub Kicinski

[-- Attachment #1: Type: text/plain, Size: 1115 bytes --]

Hi Jakub,

I love your patch! Perhaps something to improve:

[auto build test WARNING on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Jakub-Kicinski/ethtool-add-standard-pause-stats/20200912-045257
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 9984c0bb22dcae688ef8588e2621133850ff49bc
config: x86_64-randconfig-s022-20200911 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.2-191-g10164920-dirty
        # save the attached .config to linux build tree
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)

>> drivers/net/netdevsim/ethtool.c:45:26: sparse: sparse: symbol 'nsim_ethtool_ops' was not declared. Should it be static?

Please review and possibly fold the followup patch.

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 42678 bytes --]

^ permalink raw reply	[flat|nested] 23+ messages in thread
* Re: [PATCH net-next 3/8] netdevsim: add pause frame stats
@ 2020-09-12 14:38     ` kernel test robot
  0 siblings, 0 replies; 23+ messages in thread
From: kernel test robot @ 2020-09-12 14:38 UTC (permalink / raw)
  To: kbuild-all

[-- Attachment #1: Type: text/plain, Size: 1146 bytes --]

Hi Jakub,

I love your patch! Perhaps something to improve:

[auto build test WARNING on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Jakub-Kicinski/ethtool-add-standard-pause-stats/20200912-045257
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 9984c0bb22dcae688ef8588e2621133850ff49bc
config: x86_64-randconfig-s022-20200911 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.2-191-g10164920-dirty
        # save the attached .config to linux build tree
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)

>> drivers/net/netdevsim/ethtool.c:45:26: sparse: sparse: symbol 'nsim_ethtool_ops' was not declared. Should it be static?

Please review and possibly fold the followup patch.

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 42678 bytes --]

^ permalink raw reply	[flat|nested] 23+ messages in thread
* [RFC PATCH] netdevsim: nsim_ethtool_ops can be static
  2020-09-11 19:52 ` [PATCH net-next 3/8] netdevsim: add pause frame stats Jakub Kicinski
  2020-09-12 14:38     ` kernel test robot
@ 2020-09-12 14:38   ` kernel test robot
  1 sibling, 0 replies; 23+ messages in thread
From: kernel test robot @ 2020-09-12 14:38 UTC (permalink / raw)
  To: kbuild-all

[-- Attachment #1: Type: text/plain, Size: 680 bytes --]


Signed-off-by: kernel test robot <lkp@intel.com>
---
 ethtool.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/netdevsim/ethtool.c b/drivers/net/netdevsim/ethtool.c
index 7c016edfbcee37..7a4c779b4c895c 100644
--- a/drivers/net/netdevsim/ethtool.c
+++ b/drivers/net/netdevsim/ethtool.c
@@ -42,7 +42,7 @@ nsim_set_pauseparam(struct net_device *dev, struct ethtool_pauseparam *pause)
 	return 0;
 }
 
-const struct ethtool_ops nsim_ethtool_ops = {
+static const struct ethtool_ops nsim_ethtool_ops = {
 	.get_pause_stats	= nsim_get_pause_stats,
 	.get_pauseparam		= nsim_get_pauseparam,
 	.set_pauseparam		= nsim_set_pauseparam,

^ permalink raw reply related	[flat|nested] 23+ messages in thread
* Re: [PATCH net-next 5/8] bnxt: add pause frame stats
  2020-09-11 19:52 ` [PATCH net-next 5/8] bnxt: add pause frame stats Jakub Kicinski
  2020-09-11 22:34   ` Michael Chan
@ 2020-09-12 15:57   ` kernel test robot
  1 sibling, 0 replies; 23+ messages in thread
From: kernel test robot @ 2020-09-12 15:57 UTC (permalink / raw)
  To: kbuild-all

[-- Attachment #1: Type: text/plain, Size: 3027 bytes --]

Hi Jakub,

I love your patch! Perhaps something to improve:

[auto build test WARNING on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Jakub-Kicinski/ethtool-add-standard-pause-stats/20200912-045257
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 9984c0bb22dcae688ef8588e2621133850ff49bc
config: x86_64-randconfig-s022-20200911 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.2-191-g10164920-dirty
        # save the attached .config to linux build tree
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)

>> drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c:1795:33: sparse: sparse: incorrect type in assignment (different base types) @@     expected unsigned long long [usertype] rx_pause_frames @@     got restricted __le64 [usertype] rx_pause_frames @@
>> drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c:1795:33: sparse:     expected unsigned long long [usertype] rx_pause_frames
>> drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c:1795:33: sparse:     got restricted __le64 [usertype] rx_pause_frames
>> drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c:1796:33: sparse: sparse: incorrect type in assignment (different base types) @@     expected unsigned long long [usertype] tx_pause_frames @@     got restricted __le64 [usertype] tx_pause_frames @@
>> drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c:1796:33: sparse:     expected unsigned long long [usertype] tx_pause_frames
>> drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c:1796:33: sparse:     got restricted __le64 [usertype] tx_pause_frames

# https://github.com/0day-ci/linux/commit/c5c1d08b72fbfe88b0bb991fb4dded1f9d230280
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Jakub-Kicinski/ethtool-add-standard-pause-stats/20200912-045257
git checkout c5c1d08b72fbfe88b0bb991fb4dded1f9d230280
vim +1795 drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c

  1780	
  1781	static void bnxt_get_pause_stats(struct net_device *dev,
  1782					 struct ethtool_pause_stats *epstat)
  1783	{
  1784		struct bnxt *bp = netdev_priv(dev);
  1785		struct rx_port_stats *rx_stats;
  1786		struct tx_port_stats *tx_stats;
  1787	
  1788		if (BNXT_VF(bp) || !(bp->flags & BNXT_FLAG_PORT_STATS))
  1789			return;
  1790	
  1791		rx_stats = (void *)bp->port_stats.sw_stats;
  1792		tx_stats = (void *)((unsigned long)bp->port_stats.sw_stats +
  1793				    BNXT_TX_PORT_STATS_BYTE_OFFSET);
  1794	
> 1795		epstat->rx_pause_frames = rx_stats->rx_pause_frames;
> 1796		epstat->tx_pause_frames = tx_stats->tx_pause_frames;
  1797	}
  1798	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 42678 bytes --]

^ permalink raw reply	[flat|nested] 23+ messages in thread
* Re: [PATCH net-next 7/8] ixgbe: add pause frame stats
  2020-09-11 22:13     ` Jakub Kicinski
@ 2020-09-13  9:14       ` Ido Schimmel
  2020-09-14 16:20         ` Jakub Kicinski
  0 siblings, 1 reply; 23+ messages in thread
From: Ido Schimmel @ 2020-09-13  9:14 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: Alexander Duyck, David Miller, Netdev, Michal Kubecek,
	Michael Chan, tariqt, saeedm, Andrew Lunn

On Fri, Sep 11, 2020 at 03:13:43PM -0700, Jakub Kicinski wrote:
> On Fri, 11 Sep 2020 14:12:50 -0700 Alexander Duyck wrote:
> > On Fri, Sep 11, 2020 at 12:53 PM Jakub Kicinski <kuba@kernel.org> wrote:
> > > @@ -3546,6 +3556,7 @@ static const struct ethtool_ops ixgbe_ethtool_ops = {
> > >         .set_eeprom             = ixgbe_set_eeprom,
> > >         .get_ringparam          = ixgbe_get_ringparam,
> > >         .set_ringparam          = ixgbe_set_ringparam,
> > > +       .get_pause_stats        = ixgbe_get_pause_stats,
> > >         .get_pauseparam         = ixgbe_get_pauseparam,
> > >         .set_pauseparam         = ixgbe_set_pauseparam,
> > >         .get_msglevel           = ixgbe_get_msglevel,  
> > 
> > So the count for this is simpler in igb than it is for ixgbe. I'm
> > assuming you want just standard link flow control frames. If so then
> > this patch is correct. Otherwise if you are wanting to capture
> > priority flow control data then those are a seperate array of stats
> > prefixed with a "p" instead of an "l". Otherwise this looks fine to
> > me.
> 
> That's my interpretation, although I haven't found any place the
> standard would address this directly. Non-PFC pause has a different
> opcode, so I'm reasonably certain this makes sense.
> 
> BTW I'm not entirely clear on what "global PFC pause" is either.
> 
> Maybe someone can clarify? Mellanox folks?

I checked IEEE 802.1Qaz and could not find anything relevant. My only
guess is that it might be a PFC frame with all the priorities set.

Where did you see it?

> 
> > Reviewed-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> 
> Thanks!
> 

^ permalink raw reply	[flat|nested] 23+ messages in thread
* Re: [PATCH net-next 7/8] ixgbe: add pause frame stats
  2020-09-13  9:14       ` Ido Schimmel
@ 2020-09-14 16:20         ` Jakub Kicinski
  0 siblings, 0 replies; 23+ messages in thread
From: Jakub Kicinski @ 2020-09-14 16:20 UTC (permalink / raw)
  To: Ido Schimmel
  Cc: Alexander Duyck, David Miller, Netdev, Michal Kubecek,
	Michael Chan, tariqt, saeedm, Andrew Lunn

On Sun, 13 Sep 2020 12:14:14 +0300 Ido Schimmel wrote:
> On Fri, Sep 11, 2020 at 03:13:43PM -0700, Jakub Kicinski wrote:
> > On Fri, 11 Sep 2020 14:12:50 -0700 Alexander Duyck wrote:  
> > > On Fri, Sep 11, 2020 at 12:53 PM Jakub Kicinski <kuba@kernel.org> wrote:  
> > > > @@ -3546,6 +3556,7 @@ static const struct ethtool_ops ixgbe_ethtool_ops = {
> > > >         .set_eeprom             = ixgbe_set_eeprom,
> > > >         .get_ringparam          = ixgbe_get_ringparam,
> > > >         .set_ringparam          = ixgbe_set_ringparam,
> > > > +       .get_pause_stats        = ixgbe_get_pause_stats,
> > > >         .get_pauseparam         = ixgbe_get_pauseparam,
> > > >         .set_pauseparam         = ixgbe_set_pauseparam,
> > > >         .get_msglevel           = ixgbe_get_msglevel,    
> > > 
> > > So the count for this is simpler in igb than it is for ixgbe. I'm
> > > assuming you want just standard link flow control frames. If so then
> > > this patch is correct. Otherwise if you are wanting to capture
> > > priority flow control data then those are a seperate array of stats
> > > prefixed with a "p" instead of an "l". Otherwise this looks fine to
> > > me.  
> > 
> > That's my interpretation, although I haven't found any place the
> > standard would address this directly. Non-PFC pause has a different
> > opcode, so I'm reasonably certain this makes sense.
> > 
> > BTW I'm not entirely clear on what "global PFC pause" is either.
> > 
> > Maybe someone can clarify? Mellanox folks?  
> 
> I checked IEEE 802.1Qaz and could not find anything relevant. My only
> guess is that it might be a PFC frame with all the priorities set.
> 
> Where did you see it?

I think I saw it in MLX5 and I thought it's something
implementation-specific. But then I noticed 802.1-2018 
has a ieee8021PfcGlobalReqdGroup group.

^ permalink raw reply	[flat|nested] 23+ messages in thread
end of thread, other threads:[~2020-09-14 16:22 UTC | newest]

Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-11 19:52 [PATCH net-next 0/7] ethtool: add pause frame stats Jakub Kicinski
2020-09-11 19:52 ` [PATCH net-next 1/8] ethtool: add standard pause stats Jakub Kicinski
2020-09-11 19:52 ` [PATCH net-next 2/8] docs: net: include the new ethtool pause stats in the stats doc Jakub Kicinski
2020-09-11 19:52 ` [PATCH net-next 3/8] netdevsim: add pause frame stats Jakub Kicinski
2020-09-12 14:38   ` kernel test robot
2020-09-12 14:38     ` kernel test robot
2020-09-12 14:38   ` [RFC PATCH] netdevsim: nsim_ethtool_ops can be static kernel test robot
2020-09-11 19:52 ` [PATCH net-next 4/8] selftests: add a test for ethtool pause stats Jakub Kicinski
2020-09-11 19:52 ` [PATCH net-next 5/8] bnxt: add pause frame stats Jakub Kicinski
2020-09-11 22:34   ` Michael Chan
2020-09-11 22:43     ` Jakub Kicinski
2020-09-11 22:46       ` Jakub Kicinski
2020-09-11 22:53       ` Michael Chan
2020-09-11 22:58         ` Jakub Kicinski
2020-09-12 15:57   ` kernel test robot
2020-09-11 19:52 ` [PATCH net-next 6/8] mlx5: " Jakub Kicinski
2020-09-11 21:49   ` Saeed Mahameed
2020-09-11 19:52 ` [PATCH net-next 7/8] ixgbe: " Jakub Kicinski
2020-09-11 21:12   ` Alexander Duyck
2020-09-11 22:13     ` Jakub Kicinski
2020-09-13  9:14       ` Ido Schimmel
2020-09-14 16:20         ` Jakub Kicinski
2020-09-11 19:52 ` [PATCH net-next 8/8] mlx4: " Jakub Kicinski

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.