[PATCH net-next v2 0/8] ethtool: add pause frame stats

[PATCH net-next v2 0/8] ethtool: add pause frame stats

[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.

v2:
 - netdevsim: add missing static
 - bnxt: fix sparse warning
 - mlx5: address Saeed's comments

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
  ixgbe: add pause frame stats
  mlx5: 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 |  17 +++
 .../net/ethernet/intel/ixgbe/ixgbe_ethtool.c  |  11 ++
 .../net/ethernet/mellanox/mlx4/en_ethtool.c   |  19 +++
 .../net/ethernet/mellanox/mlx4/mlx4_stats.h   |  12 ++
 .../ethernet/mellanox/mlx5/core/en_ethtool.c  |   9 ++
 .../net/ethernet/mellanox/mlx5/core/en_rep.c  |   9 ++
 .../ethernet/mellanox/mlx5/core/en_stats.c    |  29 +++++
 .../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 ++++++++++++++++++
 18 files changed, 456 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

[PATCH net-next v2 1/8] ethtool: add standard pause stats

[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

[PATCH net-next v2 2/8] docs: net: include the new ethtool pause stats in the stats doc

[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

[PATCH net-next v2 3/8] netdevsim: add pause frame stats

[Jakub Kicinski]

Add minimal ethtool interface for testing ethtool pause stats.

v2: add missing static on nsim_ethtool_ops

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..7a4c779b4c89
--- /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;
+}
+
+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,
+};
+
+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

[PATCH net-next v2 4/8] selftests: add a test for ethtool pause stats

[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

[PATCH net-next v2 5/8] bnxt: add pause frame stats

[Jakub Kicinski]

These stats are already reported in ethtool -S.
Michael confirms they are equivalent to standard stats.

v2: - fix sparse warning about endian by using the macro
    - use u64 for pointer type

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

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
index d0928334bdc8..5a65f28ef771 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
@@ -1778,6 +1778,22 @@ 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);
+	u64 *rx, *tx;
+
+	if (BNXT_VF(bp) || !(bp->flags & BNXT_FLAG_PORT_STATS))
+		return;
+
+	rx = bp->port_stats.sw_stats;
+	tx = bp->port_stats.sw_stats + BNXT_TX_PORT_STATS_BYTE_OFFSET / 8;
+
+	epstat->rx_pause_frames = BNXT_GET_RX_PORT_STATS64(rx, rx_pause_frames);
+	epstat->tx_pause_frames = BNXT_GET_TX_PORT_STATS64(tx, tx_pause_frames);
+}
+
 static int bnxt_set_pauseparam(struct net_device *dev,
 			       struct ethtool_pauseparam *epause)
 {
@@ -3645,6 +3661,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

[PATCH net-next v2 6/8] ixgbe: add pause frame stats

[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>
Reviewed-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
---
 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

[PATCH net-next v2 7/8] mlx5: add pause frame stats

[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 (and Saeed confirms it).

v2:
 - drop the ethool helper and call stats directly
 - don't pass 0 as initialized to in buffer
 - use local buffer

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 .../ethernet/mellanox/mlx5/core/en_ethtool.c  |  9 ++++++
 .../net/ethernet/mellanox/mlx5/core/en_rep.c  |  9 ++++++
 .../ethernet/mellanox/mlx5/core/en_stats.c    | 29 +++++++++++++++++++
 .../ethernet/mellanox/mlx5/core/en_stats.h    |  3 ++
 4 files changed, 50 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
index 5cb1e4839eb7..9a6078e89587 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
@@ -1341,6 +1341,14 @@ static int mlx5e_set_tunable(struct net_device *dev,
 	return err;
 }
 
+static void mlx5e_get_pause_stats(struct net_device *netdev,
+				  struct ethtool_pause_stats *pause_stats)
+{
+	struct mlx5e_priv *priv = netdev_priv(netdev);
+
+	mlx5e_stats_pause_get(priv, pause_stats);
+}
+
 void mlx5e_ethtool_get_pauseparam(struct mlx5e_priv *priv,
 				  struct ethtool_pauseparam *pauseparam)
 {
@@ -2033,6 +2041,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..33c21ed7169a 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_stats_pause_get(priv, pause_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..6d5e54b964c0 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
@@ -677,6 +677,35 @@ 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)
+{
+	u32 ppcnt_ieee_802_3[MLX5_ST_SZ_DW(ppcnt_reg)];
+	struct mlx5_core_dev *mdev = priv->mdev;
+	u32 in[MLX5_ST_SZ_DW(ppcnt_reg)] = {};
+	int sz = MLX5_ST_SZ_BYTES(ppcnt_reg);
+
+	if (!MLX5_BASIC_PPCNT_SUPPORTED(mdev))
+		return;
+
+	MLX5_SET(ppcnt_reg, in, local_port, 1);
+	MLX5_SET(ppcnt_reg, in, grp, MLX5_IEEE_802_3_COUNTERS_GROUP);
+	mlx5_core_access_reg(mdev, in, sz, ppcnt_ieee_802_3,
+			     sz, MLX5_REG_PPCNT, 0, 0);
+
+	pause_stats->tx_pause_frames =
+		MLX5E_READ_CTR64_BE_F(ppcnt_ieee_802_3,
+				      a_pause_mac_ctrl_frames_transmitted);
+	pause_stats->rx_pause_frames =
+		MLX5E_READ_CTR64_BE_F(ppcnt_ieee_802_3,
+				      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

[PATCH net-next v2 8/8] mlx4: add pause frame stats

[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

Re: [PATCH net-next v2 0/8] ethtool: add pause frame stats

[Vladimir Oltean]

On Fri, Sep 11, 2020 at 04:28:45PM -0700, Jakub Kicinski wrote:
> 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.
> 
> v2:
>  - netdevsim: add missing static
>  - bnxt: fix sparse warning
>  - mlx5: address Saeed's comments
> 
> 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
>   ixgbe: add pause frame stats
>   mlx5: 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 |  17 +++
>  .../net/ethernet/intel/ixgbe/ixgbe_ethtool.c  |  11 ++
>  .../net/ethernet/mellanox/mlx4/en_ethtool.c   |  19 +++
>  .../net/ethernet/mellanox/mlx4/mlx4_stats.h   |  12 ++
>  .../ethernet/mellanox/mlx5/core/en_ethtool.c  |   9 ++
>  .../net/ethernet/mellanox/mlx5/core/en_rep.c  |   9 ++
>  .../ethernet/mellanox/mlx5/core/en_stats.c    |  29 +++++
>  .../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 ++++++++++++++++++
>  18 files changed, 456 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
> 

DSA used to override the "ethtool -S" callback of the host port, and
append its own CPU port counters to that.

So you could actually see pause frames transmitted by the host port and
received by the switch's CPU port:

# ethtool -S eno2 | grep pause
MAC rx valid pause frames: 1339603152
MAC tx valid pause frames: 0
p04_rx_pause: 0
p04_tx_pause: 1339603152

With this new command what's the plan?

Thanks,
-Vladimir

Re: [PATCH net-next v2 0/8] ethtool: add pause frame stats

[Jakub Kicinski]

On Sat, 12 Sep 2020 02:49:32 +0300 Vladimir Oltean wrote:
> On Fri, Sep 11, 2020 at 04:28:45PM -0700, Jakub Kicinski wrote:
> > 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.
> > 
> > v2:
> >  - netdevsim: add missing static
> >  - bnxt: fix sparse warning
> >  - mlx5: address Saeed's comments
> 
> DSA used to override the "ethtool -S" callback of the host port, and
> append its own CPU port counters to that.
> 
> So you could actually see pause frames transmitted by the host port and
> received by the switch's CPU port:
> 
> # ethtool -S eno2 | grep pause
> MAC rx valid pause frames: 1339603152
> MAC tx valid pause frames: 0
> p04_rx_pause: 0
> p04_tx_pause: 1339603152
> 
> With this new command what's the plan?

Sounds like something for DSA folks to decide :)

What does ethtool -A $cpu_port control? 
The stats should match what the interface controls.

Re: [PATCH net-next v2 0/8] ethtool: add pause frame stats

[Vladimir Oltean]

On Fri, Sep 11, 2020 at 05:07:24PM -0700, Jakub Kicinski wrote:
> On Sat, 12 Sep 2020 02:49:32 +0300 Vladimir Oltean wrote:
> > On Fri, Sep 11, 2020 at 04:28:45PM -0700, Jakub Kicinski wrote:
> > > 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.
> > >
> > > v2:
> > >  - netdevsim: add missing static
> > >  - bnxt: fix sparse warning
> > >  - mlx5: address Saeed's comments
> >
> > DSA used to override the "ethtool -S" callback of the host port, and
> > append its own CPU port counters to that.
> >
> > So you could actually see pause frames transmitted by the host port and
> > received by the switch's CPU port:
> >
> > # ethtool -S eno2 | grep pause
> > MAC rx valid pause frames: 1339603152
> > MAC tx valid pause frames: 0
> > p04_rx_pause: 0
> > p04_tx_pause: 1339603152
> >
> > With this new command what's the plan?
>
> Sounds like something for DSA folks to decide :)
>
> What does ethtool -A $cpu_port control?
> The stats should match what the interface controls.

Error: $cpu_port: undefined variable.
With DSA switches, the CPU port is a physical Ethernet port mostly like
any other, except that its orientation is inwards towards the system
rather than outwards. So there is no network interface registered for
it, since I/O from the network stack would have to literally loop back
into the system to fulfill the request of sending a packet to that
interface.
The ethtool -S framework was nice because you could append to the
counters of the master interface while not losing them.
As for "ethtool -A", those parameters are fixed as part of the
fixed-link device tree node corresponding to the CPU port.

Re: [PATCH net-next v2 0/8] ethtool: add pause frame stats

[Jakub Kicinski]

On Sat, 12 Sep 2020 03:15:42 +0300 Vladimir Oltean wrote:
> On Fri, Sep 11, 2020 at 05:07:24PM -0700, Jakub Kicinski wrote:
> > On Sat, 12 Sep 2020 02:49:32 +0300 Vladimir Oltean wrote:  
> > > On Fri, Sep 11, 2020 at 04:28:45PM -0700, Jakub Kicinski wrote:  
> > > > 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.
> > > >
> > > > v2:
> > > >  - netdevsim: add missing static
> > > >  - bnxt: fix sparse warning
> > > >  - mlx5: address Saeed's comments  
> > >
> > > DSA used to override the "ethtool -S" callback of the host port, and
> > > append its own CPU port counters to that.
> > >
> > > So you could actually see pause frames transmitted by the host port and
> > > received by the switch's CPU port:
> > >
> > > # ethtool -S eno2 | grep pause
> > > MAC rx valid pause frames: 1339603152
> > > MAC tx valid pause frames: 0
> > > p04_rx_pause: 0
> > > p04_tx_pause: 1339603152
> > >
> > > With this new command what's the plan?  
> >
> > Sounds like something for DSA folks to decide :)
> >
> > What does ethtool -A $cpu_port control?
> > The stats should match what the interface controls.  
> 
> Error: $cpu_port: undefined variable.
> With DSA switches, the CPU port is a physical Ethernet port mostly like
> any other, except that its orientation is inwards towards the system
> rather than outwards. So there is no network interface registered for
> it, since I/O from the network stack would have to literally loop back
> into the system to fulfill the request of sending a packet to that
> interface.

Sorry, perhaps I should have said $MAC, but that kind of in itself
answers the question.

> The ethtool -S framework was nice because you could append to the
> counters of the master interface while not losing them.
> As for "ethtool -A", those parameters are fixed as part of the
> fixed-link device tree node corresponding to the CPU port.

I think I'm missing the problem you're trying to describe.
Are you making a general comment / argument on ethtool stats?

Pause stats are symmetrical - as can be seen in your quote
what's RX for the CPU is TX for the switch, and vice versa.

Since ethtool -A $cpu_mac controls whether CPU netdev generates
and accepts pause frames, correspondingly the direction and meaning
of pause statistics on that interface is well defined.

You can still append your custom CPU port stats to ethtool -S or
debugfs or whatnot, but those are only useful for validating that 
the configuration of the CPU port is not completely broken. Otherwise
the counters are symmetrical. A day-to-day user of the device doesn't
need to see both of them.

Re: [PATCH net-next v2 7/8] mlx5: add pause frame stats

[Jakub Kicinski]

On Fri, 11 Sep 2020 16:28:52 -0700 Jakub Kicinski wrote:
> +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_stats_pause_get(priv, pause_stats);

s/pause_stats/stats/

I'll give people time to review and post v3 on Monday.

Re: [PATCH net-next v2 0/8] ethtool: add pause frame stats

[Florian Fainelli]

On 9/11/2020 5:42 PM, Jakub Kicinski wrote:
> On Sat, 12 Sep 2020 03:15:42 +0300 Vladimir Oltean wrote:
>> On Fri, Sep 11, 2020 at 05:07:24PM -0700, Jakub Kicinski wrote:
>>> On Sat, 12 Sep 2020 02:49:32 +0300 Vladimir Oltean wrote:
>>>> On Fri, Sep 11, 2020 at 04:28:45PM -0700, Jakub Kicinski wrote:
>>>>> 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.
>>>>>
>>>>> v2:
>>>>>   - netdevsim: add missing static
>>>>>   - bnxt: fix sparse warning
>>>>>   - mlx5: address Saeed's comments
>>>>
>>>> DSA used to override the "ethtool -S" callback of the host port, and
>>>> append its own CPU port counters to that.
>>>>
>>>> So you could actually see pause frames transmitted by the host port and
>>>> received by the switch's CPU port:
>>>>
>>>> # ethtool -S eno2 | grep pause
>>>> MAC rx valid pause frames: 1339603152
>>>> MAC tx valid pause frames: 0
>>>> p04_rx_pause: 0
>>>> p04_tx_pause: 1339603152
>>>>
>>>> With this new command what's the plan?
>>>
>>> Sounds like something for DSA folks to decide :)
>>>
>>> What does ethtool -A $cpu_port control?
>>> The stats should match what the interface controls.
>>
>> Error: $cpu_port: undefined variable.
>> With DSA switches, the CPU port is a physical Ethernet port mostly like
>> any other, except that its orientation is inwards towards the system
>> rather than outwards. So there is no network interface registered for
>> it, since I/O from the network stack would have to literally loop back
>> into the system to fulfill the request of sending a packet to that
>> interface.
> 
> Sorry, perhaps I should have said $MAC, but that kind of in itself
> answers the question.
> 
>> The ethtool -S framework was nice because you could append to the
>> counters of the master interface while not losing them.
>> As for "ethtool -A", those parameters are fixed as part of the
>> fixed-link device tree node corresponding to the CPU port.
> 
> I think I'm missing the problem you're trying to describe.
> Are you making a general comment / argument on ethtool stats?
> 
> Pause stats are symmetrical - as can be seen in your quote
> what's RX for the CPU is TX for the switch, and vice versa.
> 
> Since ethtool -A $cpu_mac controls whether CPU netdev generates
> and accepts pause frames, correspondingly the direction and meaning
> of pause statistics on that interface is well defined.
> 
> You can still append your custom CPU port stats to ethtool -S or
> debugfs or whatnot, but those are only useful for validating that
> the configuration of the CPU port is not completely broken. Otherwise
> the counters are symmetrical. A day-to-day user of the device doesn't
> need to see both of them.

It would be a lot easier to append the stats if there was not an 
additional ndo introduce to fetch the pause statistics because DSA 
overlay ndo on a function by function basis. Alternatively we should 
consider ethtool netlink operations over a devlink port at some point so 
we can get rid of the ugly ndo and ethtool_ops overlay that DSA does.

Can we consider using get_ethtool_stats and ETH_SS_PAUSE_STATS as a 
stringset identifier? That way there is a single point within driver to 
fetch stats.
--
Florian

Re: [PATCH net-next v2 0/8] ethtool: add pause frame stats

[Vladimir Oltean]

On Fri, Sep 11, 2020 at 05:42:46PM -0700, Jakub Kicinski wrote:
> > The ethtool -S framework was nice because you could append to the
> > counters of the master interface while not losing them.
> > As for "ethtool -A", those parameters are fixed as part of the
> > fixed-link device tree node corresponding to the CPU port.
>
> I think I'm missing the problem you're trying to describe.
> Are you making a general comment / argument on ethtool stats?

No, it appears to me that you're trying to standardize some things out
of ethtool -S. I just want to make sure that, while doing so, you are
aware of some of the more subtle uses of that interface.

> Pause stats are symmetrical - as can be seen in your quote
> what's RX for the CPU is TX for the switch, and vice versa.

If things work, yes. If they don't, no.

> Since ethtool -A $cpu_mac controls whether CPU netdev generates
> and accepts pause frames, correspondingly the direction and meaning
> of pause statistics on that interface is well defined.

Well, with a fixed-link, there's not a lot you can change with "ethtool
--pause" (the link is managed with phylink). Up until now, I have mostly
only heard of links between a DSA master and the CPU port that are not
fixed-link. In theory (and in limited practice), phylink can even drive
a PHY on the CPU port, so in that case, it may make sense for DSA to try
to automatically apply the mirror pause frame configuration of the
master. However, that wasn't supported before, either, and was not what
I was talking about.

> You can still append your custom CPU port stats to ethtool -S or
> debugfs or whatnot,

I don't understand, so you're saying that DSA can keep pause stats
reporting in "ethtool -S", but the rest of devices should move to
"ethtool -a"? You know that a typical switching chip will report the
same statistics counters on all ports, including the ones that do have a
net_device, right?  So DSA gets a waiver from implementing
.get_pause_stats()?

> but those are only useful for validating that the configuration of the
> CPU port is not completely broken. Otherwise the counters are
> symmetrical. A day-to-day user of the device doesn't need to see both
> of them.

A day-to-day user shouldn't need to look at ethtool -S or any other
statistics for that matter, either. If they need to look at flow control
on the CPU port they'd better get the full story rather than half of it.

Sorry for the non-constructive answer. Like Florian said, it would be
nice to have some built-in mechanism for this new ndo that DSA could use
to keep annotating its own stats.

Thanks,
-Vladimir

Re: [PATCH net-next v2 7/8] mlx5: add pause frame stats

[kernel test robot]

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

Hi Jakub,

I love your patch! Yet something to improve:

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

url:    https://github.com/0day-ci/linux/commits/Jakub-Kicinski/ethtool-add-pause-frame-stats/20200912-073106
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 9984c0bb22dcae688ef8588e2621133850ff49bc
config: alpha-allyesconfig (attached as .config)
compiler: alpha-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=alpha 

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

All errors (new ones prefixed by >>):

   drivers/net/ethernet/mellanox/mlx5/core/en_rep.c: In function 'mlx5e_uplink_rep_get_pause_stats':
>> drivers/net/ethernet/mellanox/mlx5/core/en_rep.c:296:30: error: 'pause_stats' undeclared (first use in this function)
     296 |  mlx5e_stats_pause_get(priv, pause_stats);
         |                              ^~~~~~~~~~~
   drivers/net/ethernet/mellanox/mlx5/core/en_rep.c:296:30: note: each undeclared identifier is reported only once for each function it appears in

# https://github.com/0day-ci/linux/commit/17559603fedf69b369c0ebe597dc2e6ac5369167
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Jakub-Kicinski/ethtool-add-pause-frame-stats/20200912-073106
git checkout 17559603fedf69b369c0ebe597dc2e6ac5369167
vim +/pause_stats +296 drivers/net/ethernet/mellanox/mlx5/core/en_rep.c

   290	
   291	static void mlx5e_uplink_rep_get_pause_stats(struct net_device *netdev,
   292						     struct ethtool_pause_stats *stats)
   293	{
   294		struct mlx5e_priv *priv = netdev_priv(netdev);
   295	
 > 296		mlx5e_stats_pause_get(priv, pause_stats);
   297	}
   298	

---
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: 65778 bytes --]

Re: [PATCH net-next v2 7/8] mlx5: add pause frame stats

[kernel test robot]

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

Hi Jakub,

I love your patch! Yet something to improve:

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

url:    https://github.com/0day-ci/linux/commits/Jakub-Kicinski/ethtool-add-pause-frame-stats/20200912-073106
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 9984c0bb22dcae688ef8588e2621133850ff49bc
config: alpha-allyesconfig (attached as .config)
compiler: alpha-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=alpha 

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

All errors (new ones prefixed by >>):

   drivers/net/ethernet/mellanox/mlx5/core/en_rep.c: In function 'mlx5e_uplink_rep_get_pause_stats':
>> drivers/net/ethernet/mellanox/mlx5/core/en_rep.c:296:30: error: 'pause_stats' undeclared (first use in this function)
     296 |  mlx5e_stats_pause_get(priv, pause_stats);
         |                              ^~~~~~~~~~~
   drivers/net/ethernet/mellanox/mlx5/core/en_rep.c:296:30: note: each undeclared identifier is reported only once for each function it appears in

# https://github.com/0day-ci/linux/commit/17559603fedf69b369c0ebe597dc2e6ac5369167
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Jakub-Kicinski/ethtool-add-pause-frame-stats/20200912-073106
git checkout 17559603fedf69b369c0ebe597dc2e6ac5369167
vim +/pause_stats +296 drivers/net/ethernet/mellanox/mlx5/core/en_rep.c

   290	
   291	static void mlx5e_uplink_rep_get_pause_stats(struct net_device *netdev,
   292						     struct ethtool_pause_stats *stats)
   293	{
   294		struct mlx5e_priv *priv = netdev_priv(netdev);
   295	
 > 296		mlx5e_stats_pause_get(priv, pause_stats);
   297	}
   298	

---
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: 65778 bytes --]

Re: [PATCH net-next v2 1/8] ethtool: add standard pause stats

[Andrew Lunn]

>  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));

Sorry, i missed v1 of these patches. Maybe this has been commented?

Filling with 0xff is odd. I don't know of any other code doing this.

	Andrew

Re: [PATCH net-next v2 0/8] ethtool: add pause frame stats

[Andrew Lunn]

> DSA used to override the "ethtool -S" callback of the host port, and
> append its own CPU port counters to that.

That was always a hack. It was bound to break sooner or later.

Ido planned to add statistics to devlink. I hope we can make use of
that to replace the CPU port statistics, and also add DSA port
statistics, since these interfaces do exist in devlink.

      Andrew

Re: [PATCH net-next v2 1/8] ethtool: add standard pause stats

[Jakub Kicinski]

On Mon, 14 Sep 2020 03:48:40 +0200 Andrew Lunn wrote:
> >  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));  
> 
> Sorry, i missed v1 of these patches. Maybe this has been commented?
> 
> Filling with 0xff is odd. I don't know of any other code doing this.

Are you saying it'd be clearer to assign ETHTOOL_STAT_NOT_SET in a loop?

Or do you think the mechanism of using ~0 as "not reported" is not good?

Re: [PATCH net-next v2 0/8] ethtool: add pause frame stats

[Jakub Kicinski]

On Fri, 11 Sep 2020 19:54:11 -0700 Florian Fainelli wrote:
> > I think I'm missing the problem you're trying to describe.
> > Are you making a general comment / argument on ethtool stats?
> > 
> > Pause stats are symmetrical - as can be seen in your quote
> > what's RX for the CPU is TX for the switch, and vice versa.
> > 
> > Since ethtool -A $cpu_mac controls whether CPU netdev generates
> > and accepts pause frames, correspondingly the direction and meaning
> > of pause statistics on that interface is well defined.
> > 
> > You can still append your custom CPU port stats to ethtool -S or
> > debugfs or whatnot, but those are only useful for validating that
> > the configuration of the CPU port is not completely broken. Otherwise
> > the counters are symmetrical. A day-to-day user of the device doesn't
> > need to see both of them.  
> 
> It would be a lot easier to append the stats if there was not an 
> additional ndo introduce to fetch the pause statistics because DSA 
> overlay ndo on a function by function basis. Alternatively we should 
> consider ethtool netlink operations over a devlink port at some point so 
> we can get rid of the ugly ndo and ethtool_ops overlay that DSA does.

I'm trying to target the 99.9% of users here, so in all honesty I'm
concerned that if we try to cater to strange corner cases too much 
the entire interface will suffer. Hence I decided not to go with
devlink, but extend the API people already know and use. It's the most
logical and obvious place to me.

> Can we consider using get_ethtool_stats and ETH_SS_PAUSE_STATS as a 
> stringset identifier? That way there is a single point within driver to 
> fetch stats.

Can you say more? There are no strings reported in this patch set.

Re: [PATCH net-next v2 0/8] ethtool: add pause frame stats

[Jakub Kicinski]

On Sat, 12 Sep 2020 10:16:12 +0300 Vladimir Oltean wrote:
> > You can still append your custom CPU port stats to ethtool -S or
> > debugfs or whatnot,  
> 
> I don't understand, so you're saying that DSA can keep pause stats
> reporting in "ethtool -S", but the rest of devices should move to
> "ethtool -a"?

I'm saying report pause stats via the new interface on normal ports
which have a netdev (external switch ports, CPU MAC).

Deal with the weird CPU port in other, correspondingly weird, ways, 
like ethtool -S :)

> You know that a typical switching chip will report the
> same statistics counters on all ports, including the ones that do have a
> net_device, right?  

I never used a DSA device. But I was under the impression they were
supposed to be modeled like separate NICs..

> So DSA gets a waiver from implementing .get_pause_stats()?
> 
> > but those are only useful for validating that the configuration of the
> > CPU port is not completely broken. Otherwise the counters are
> > symmetrical. A day-to-day user of the device doesn't need to see both
> > of them.  
> 
> A day-to-day user shouldn't need to look at ethtool -S or any other
> statistics for that matter, either. If they need to look at flow control
> on the CPU port they'd better get the full story rather than half of it.

Flow control stats are a really important piece of data on how 
the network operates. And they are part of normal operation of 
the network.

Stats on the "CPU port" should be symmetrical with the CPU MAC.

> Sorry for the non-constructive answer. Like Florian said, it would be
> nice to have some built-in mechanism for this new ndo that DSA could use
> to keep annotating its own stats.

I do sympathize with the challenges of DSA. I never had any good ideas
on how to help with it, TBH. I feel like this is a larger challenge,
adding stats to the already existing (and problematic from DSA
perspective) interface to configure pause frames is not changing the
situation all that much.

Re: [PATCH net-next v2 0/8] ethtool: add pause frame stats

[Florian Fainelli]

On 9/14/2020 8:53 AM, Jakub Kicinski wrote:
> On Fri, 11 Sep 2020 19:54:11 -0700 Florian Fainelli wrote:
>>> I think I'm missing the problem you're trying to describe.
>>> Are you making a general comment / argument on ethtool stats?
>>>
>>> Pause stats are symmetrical - as can be seen in your quote
>>> what's RX for the CPU is TX for the switch, and vice versa.
>>>
>>> Since ethtool -A $cpu_mac controls whether CPU netdev generates
>>> and accepts pause frames, correspondingly the direction and meaning
>>> of pause statistics on that interface is well defined.
>>>
>>> You can still append your custom CPU port stats to ethtool -S or
>>> debugfs or whatnot, but those are only useful for validating that
>>> the configuration of the CPU port is not completely broken. Otherwise
>>> the counters are symmetrical. A day-to-day user of the device doesn't
>>> need to see both of them.
>>
>> It would be a lot easier to append the stats if there was not an
>> additional ndo introduce to fetch the pause statistics because DSA
>> overlay ndo on a function by function basis. Alternatively we should
>> consider ethtool netlink operations over a devlink port at some point so
>> we can get rid of the ugly ndo and ethtool_ops overlay that DSA does.
> 
> I'm trying to target the 99.9% of users here, so in all honesty I'm
> concerned that if we try to cater to strange corner cases too much
> the entire interface will suffer. Hence I decided not to go with
> devlink, but extend the API people already know and use. It's the most
> logical and obvious place to me.
> 
>> Can we consider using get_ethtool_stats and ETH_SS_PAUSE_STATS as a
>> stringset identifier? That way there is a single point within driver to
>> fetch stats.
> 
> Can you say more? There are no strings reported in this patch set.

What I am suggesting is that we have a central and unique method for 
drivers to be called for all ethtool statisitcs to be obtained, and not 
create another ethtool operation specifically for pause stats.

Today we have get_ethtool_stats and a stringset argument that tells you 
which type of statistic to return. I am not suggesting that we return 
strings or that it should be necessary for fetching pause stats.
-- 
Florian

Re: [PATCH net-next v2 0/8] ethtool: add pause frame stats

[Jakub Kicinski]

On Mon, 14 Sep 2020 04:08:14 +0200 Andrew Lunn wrote:
> > DSA used to override the "ethtool -S" callback of the host port, and
> > append its own CPU port counters to that.  
> 
> That was always a hack. It was bound to break sooner or later.
> 
> Ido planned to add statistics to devlink. I hope we can make use of
> that to replace the CPU port statistics, and also add DSA port
> statistics, since these interfaces do exist in devlink.

I considered devlink but it really doesn't make much sense to me to
configure something via ethtool and have its stats in devlink. If
devlink was the way to go then the config interface should have been
added there, too. And it wasn't (we just merged ethtool-nl for pause 
a couple of releases ago). Besides, doesn't it go against our "Linux 
is in control policy" to facilitate ports that don't have netdevs?
Especially making a precedent like this for completely symmetrical
pause frame config and stats does not seem like the right trade off 
to me.

Re: [PATCH net-next v2 0/8] ethtool: add pause frame stats

[Jakub Kicinski]

On Mon, 14 Sep 2020 09:25:44 -0700 Florian Fainelli wrote:
> >> Can we consider using get_ethtool_stats and ETH_SS_PAUSE_STATS as a
> >> stringset identifier? That way there is a single point within driver to
> >> fetch stats.  
> > 
> > Can you say more? There are no strings reported in this patch set.  
> 
> What I am suggesting is that we have a central and unique method for 
> drivers to be called for all ethtool statisitcs to be obtained, and not 
> create another ethtool operation specifically for pause stats.

That won't work for statistics which correspond to a non-singleton
object, like queue stats.

> Today we have get_ethtool_stats and a stringset argument that tells you 
> which type of statistic to return. I am not suggesting that we return 
> strings or that it should be necessary for fetching pause stats.

A multiplexer call or a call that dumps everything and then core picks
out what it needs?

Re: [PATCH net-next v2 1/8] ethtool: add standard pause stats

[Andrew Lunn]

On Mon, Sep 14, 2020 at 08:48:10AM -0700, Jakub Kicinski wrote:
> On Mon, 14 Sep 2020 03:48:40 +0200 Andrew Lunn wrote:
> > >  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));  
> > 
> > Sorry, i missed v1 of these patches. Maybe this has been commented?
> > 
> > Filling with 0xff is odd. I don't know of any other code doing this.
> 
> Are you saying it'd be clearer to assign ETHTOOL_STAT_NOT_SET in a loop?

Yes. In the end i figured out this is what you intended. I knew there
had to be more to it than what i was seeing. It would be much more
readable to just set the two values to ETHTOOL_STAT_NOT_SET. And i
doubt it makes any difference to the compile, it is probably rolling
the loop and just doing two assignments anyway.

    Andrew

Re: [PATCH net-next v2 0/8] ethtool: add pause frame stats

[Andrew Lunn]

On Mon, Sep 14, 2020 at 09:15:18AM -0700, Jakub Kicinski wrote:
> On Sat, 12 Sep 2020 10:16:12 +0300 Vladimir Oltean wrote:
> I never used a DSA device. But I was under the impression they were
> supposed to be modeled like separate NICs..

The front panel ports are. However there are other types of ports as
well. You have at least one port of the switch connected to the SoC,
so the SoC can send/receive frames. This is the so called CPU port of
the switch. And Marvell switches support connecting switch ports
together to form a cluster of switches. These are the so called DSA
ports of the switch. Neither CPU nor DSA ports have a netdev, since
they are internal plumbing.

> Stats on the "CPU port" should be symmetrical with the CPU MAC.

If things are working as expected. But pause is configurable per
MAC. It could be one end has been configured to asym pause, and the
other to pause. It could be one end is configured to asym pause, and
the other end is failing to autoneg, etc. Just seeing that the stats
are significantly different is a good clue something is up.

    Andrew

Re: [PATCH net-next v2 0/8] ethtool: add pause frame stats

[Andrew Lunn]

> > Can we consider using get_ethtool_stats and ETH_SS_PAUSE_STATS as a 
> > stringset identifier? That way there is a single point within driver to 
> > fetch stats.
> 
> Can you say more? There are no strings reported in this patch set.

Let me ask another question. Is pause stats the end of the story? Or
do you plan to add more use case specific statistics?

ethtool -T|--show-time-stamping could show statistics for PTP frames
sent/received?

ethtool --show-eee could show statistics for sleep/wake cycles?

ethtool --show-rxfh-indir could show RSS statistics?

Would you add a new ethtool op for each of these? Or maybe we should
duplex them all through get_ethtool_stats()?

       Andrew

Re: [PATCH net-next v2 1/8] ethtool: add standard pause stats

[Jakub Kicinski]

On Mon, 14 Sep 2020 19:18:08 +0200 Andrew Lunn wrote:
> On Mon, Sep 14, 2020 at 08:48:10AM -0700, Jakub Kicinski wrote:
> > On Mon, 14 Sep 2020 03:48:40 +0200 Andrew Lunn wrote:  
> > > >  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));    
> > > 
> > > Sorry, i missed v1 of these patches. Maybe this has been commented?
> > > 
> > > Filling with 0xff is odd. I don't know of any other code doing this.  
> > 
> > Are you saying it'd be clearer to assign ETHTOOL_STAT_NOT_SET in a loop?  
> 
> Yes. In the end i figured out this is what you intended. I knew there
> had to be more to it than what i was seeing. It would be much more
> readable to just set the two values to ETHTOOL_STAT_NOT_SET. And i
> doubt it makes any difference to the compile, it is probably rolling
> the loop and just doing two assignments anyway.

Good point, thanks!

Re: [PATCH net-next v2 0/8] ethtool: add pause frame stats

[Jakub Kicinski]

On Mon, 14 Sep 2020 19:36:50 +0200 Andrew Lunn wrote:
> > > Can we consider using get_ethtool_stats and ETH_SS_PAUSE_STATS as a 
> > > stringset identifier? That way there is a single point within driver to 
> > > fetch stats.  
> > 
> > Can you say more? There are no strings reported in this patch set.  
> 
> Let me ask another question. Is pause stats the end of the story? Or
> do you plan to add more use case specific statistics?
> 
> ethtool -T|--show-time-stamping could show statistics for PTP frames
> sent/received?
> 
> ethtool --show-eee could show statistics for sleep/wake cycles?
> 
> ethtool --show-rxfh-indir could show RSS statistics?

I don't have a need for any of these. But they may make sense.

I'll add FEC stats next:

  30.5.1.1.17 aFECCorrectedBlocks
  30.5.1.1.18 aFECUncorrectableBlocks

I was tempted to add RMON stats, cause a lot of MACs expose those,
but I don't actually have a use for them personally, so it's lower 
prio.

> Would you add a new ethtool op for each of these? Or maybe we should
> duplex them all through get_ethtool_stats()?

I don't see a problem with an op for each of those. It makes for 
natural querying granularity. Works quite well for drivers converted
here, IMHO. 

Re: [PATCH net-next v2 2/8] docs: net: include the new ethtool pause stats in the stats doc

[Saeed Mahameed]

On Fri, 2020-09-11 at 16:28 -0700, Jakub Kicinski wrote:
> 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
> +

this will require to access the HW twice per stats request to read both
stats and current parameters, maybe this is not a big deal, but sharp
accuracy can be important for some performance enthusiasts.

Do we need an API that only reports statistics without the current
parameters ?

Re: [PATCH net-next v2 0/8] ethtool: add pause frame stats

[Jakub Kicinski]

On Mon, 14 Sep 2020 19:28:29 +0200 Andrew Lunn wrote:
> On Mon, Sep 14, 2020 at 09:15:18AM -0700, Jakub Kicinski wrote:
> > On Sat, 12 Sep 2020 10:16:12 +0300 Vladimir Oltean wrote:
> > I never used a DSA device. But I was under the impression they were
> > supposed to be modeled like separate NICs..  
> 
> The front panel ports are. However there are other types of ports as
> well. You have at least one port of the switch connected to the SoC,
> so the SoC can send/receive frames. This is the so called CPU port of
> the switch. And Marvell switches support connecting switch ports
> together to form a cluster of switches. These are the so called DSA
> ports of the switch. Neither CPU nor DSA ports have a netdev, since
> they are internal plumbing.
> 
> > Stats on the "CPU port" should be symmetrical with the CPU MAC.  
> 
> If things are working as expected. But pause is configurable per
> MAC. It could be one end has been configured to asym pause, and the
> other to pause. It could be one end is configured to asym pause, and
> the other end is failing to autoneg, etc. Just seeing that the stats
> are significantly different is a good clue something is up.

Andrew, I appreciate DSA's complexities, but those are inherent to 
the lack of netdevs. Nobody raised an eyelid when pause config was
converted to ethtool-nl, why are the statistics a problem?

I'm adding an interface for monitoring daemons to use. sigar, zabbix,
whatever. For those being able to query pause frames or FEC errors of
real ports is important. 

Pauses on internal DSA ports are a different beast. If the monitoring
dashboard starts listing internal DSA ports alongside real netdevs
users will see them as ports, and wonder where the netdevs are.

Re: [PATCH net-next v2 2/8] docs: net: include the new ethtool pause stats in the stats doc

[Jakub Kicinski]

On Mon, 14 Sep 2020 19:33:08 +0000 Saeed Mahameed wrote:
> > +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
> > +  
> 
> this will require to access the HW twice per stats request to read both
> stats and current parameters, maybe this is not a big deal, but sharp
> accuracy can be important for some performance enthusiasts.
> 
> Do we need an API that only reports statistics without the current
> parameters ?

That crossed my mind. IDK how real this concern is if we have ethtool
-S which dumps half of the universe and nobody ever done anything
about it..

Once we start adding more interfaces (as I said elsewhere I plan to add
FEC counters) we'll also have to do multiple calls for multiple types
of stats. But I think that's fine as a starting point. We can extend
RTM_GETSTATS to provide an efficient interface when needed.

As you may recall a couple years back I posted a set with "hierarchical
stats" which as an attempt to solve all the problems at once. 
I concluded that it's a wrong approach. We should start with the simple
and obvious stuff. We can build complexity later.

Re: [PATCH net-next v2 0/8] ethtool: add pause frame stats

[Saeed Mahameed]

On Fri, 2020-09-11 at 16:28 -0700, Jakub Kicinski wrote:
> 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.
> 
> v2:
>  - netdevsim: add missing static
>  - bnxt: fix sparse warning
>  - mlx5: address Saeed's comments
> 
> 


LGTM,

Reviewed-by: Saeed Mahameed <saeedm@nvidia.com>

API for stats only reporting comment is not critical.

Re: [PATCH net-next v2 2/8] docs: net: include the new ethtool pause stats in the stats doc

[Saeed Mahameed]

On Mon, 2020-09-14 at 12:52 -0700, Jakub Kicinski wrote:
> On Mon, 14 Sep 2020 19:33:08 +0000 Saeed Mahameed wrote:
> > > +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
> > > +  
> > 
> > this will require to access the HW twice per stats request to read
> > both
> > stats and current parameters, maybe this is not a big deal, but
> > sharp
> > accuracy can be important for some performance enthusiasts.
> > 
> > Do we need an API that only reports statistics without the current
> > parameters ?
> 
> That crossed my mind. IDK how real this concern is if we have ethtool
> -S which dumps half of the universe and nobody ever done anything
> about it..
> 
> Once we start adding more interfaces (as I said elsewhere I plan to
> add
> FEC counters) we'll also have to do multiple calls for multiple types
> of stats. But I think that's fine as a starting point. We can extend
> RTM_GETSTATS to provide an efficient interface when needed.
> 
> As you may recall a couple years back I posted a set with
> "hierarchical
> stats" which as an attempt to solve all the problems at once. 
> I concluded that it's a wrong approach. We should start with the
> simple
> and obvious stuff. We can build complexity later.

Agreed.