[PATCH net-next 00/21] ethtool, stats: introduce and use standard XDP stats

[PATCH net-next 00/21] ethtool, stats: introduce and use standard XDP stats

[Alexander Lobakin]

This series follows the Jakub's work on standard statistics and
unifies XDP statistics across [most of] the drivers.
The only driver left unconverted is mlx5 -- it has rather complex
statistics, so I believe it would be better to leave this up to
its developers.

The stats itself consists of 12 counters:
 - packets: number of frames passed to bpf_prog_run_xdp();
 - errors: number of general XDP errors, if driver has one unified counter;
 - aborted: number of XDP_ABORTED returns;
 - drop: number of XDP_DROP returns;
 - invalid: number of returns of unallowed values (i.e. not XDP_*);
 - pass: number of XDP_PASS returns;
 - redirect: number of successfully performed XDP_REDIRECT requests;
 - redirect_errors: number of failed XDP_REDIRECT requests;
 - tx: number of successfully performed XDP_TX requests;
 - tx_errors: number of failed XDP_TX requests;
 - xmit: number of xdp_frames successfully transmitted via .ndo_xdp_xmit();
 - xmit_drops: number of frames dropped from .ndo_xdp_xmit().

As most drivers stores them on a per-channel basis, Ethtool standard
stats infra has been expanded to support this. A new nested
attribute has been added which indicated that the fields enclosed
in this block are related to one particular channel. If Ethtool
utility is older than the kernel, those blocks will just be skipped
with no errors.
When the stats are not per-channel, Ethtool core treats them as
regular and so does Ethtool utility display them. Otherwise,
the example output looks like:

$ ./ethtool -S enp175s0f0 --all-groups
Standard stats for enp175s0f0:
[ snip ]
channel0-xdp-aborted: 1
channel0-xdp-drop: 2
channel0-xdp-illegal: 3
channel0-xdp-pass: 4
channel0-xdp-redirect: 5
[ snip ]

...and the JSON output looks like:

[ snip ]
        "xdp": {
            "per-channel": [
                "channel0": {
                    "aborted": 1,
                    "drop": 2,
                    "illegal": 3,
                    "pass": 4,
                    "redirect": 5,
[ snip ]
                } ]
        }
[ snip ]

Rouhly half of the commits are present to unify XDP stats logics
across the drivers, and the first two are preparatory/housekeeping.

This set is also available here: [0]

[0] https://github.com/alobakin/linux/tree/xdp_stats

Alexander Lobakin (21):
  ethtool, stats: use a shorthand pointer in stats_prepare_data()
  ethtool, stats: add compile-time checks for standard stats
  ethtool, stats: introduce standard XDP statistics
  ethernet, dpaa2: simplify per-channel Ethtool stats counting
  ethernet, dpaa2: convert to standard XDP stats
  ethernet, ena: constify src and syncp args of ena_safe_update_stat()
  ethernet, ena: convert to standard XDP stats
  ethernet, enetc: convert to standard XDP stats
  ethernet, mvneta: rename xdp_xmit_err to xdp_xmit_drops
  ethernet, mvneta: convert to standard XDP stats
  ethernet, mvpp2: rename xdp_xmit_err to xdp_xmit_drops
  ethernet, mvpp2: convert to standard XDP stats
  ethernet, sfc: convert to standard XDP stats
  veth: rename rx_drops to xdp_errors
  veth: rename xdp_xmit_errors to xdp_xmit_drops
  veth: rename drop xdp_ suffix from packets and bytes stats
  veth: convert to standard XDP stats
  virtio-net: rename xdp_tx{,__drops} SQ stats to xdp_xmit{,__drops}
  virtio-net: don't mix error-caused drops with XDP_DROP cases
  virtio-net: convert to standard XDP stats
  Documentation, ethtool-netlink: update standard statistics
    documentation

 Documentation/networking/ethtool-netlink.rst  |  45 +++--
 drivers/net/ethernet/amazon/ena/ena_ethtool.c |  50 +++++-
 .../net/ethernet/freescale/dpaa2/dpaa2-eth.h  |   7 +-
 .../ethernet/freescale/dpaa2/dpaa2-ethtool.c  |  38 +++-
 .../ethernet/freescale/enetc/enetc_ethtool.c  |  58 ++++--
 drivers/net/ethernet/marvell/mvneta.c         | 112 ++++++------
 drivers/net/ethernet/marvell/mvpp2/mvpp2.h    |   2 +-
 .../net/ethernet/marvell/mvpp2/mvpp2_main.c   |  96 +++-------
 drivers/net/ethernet/sfc/ef100_ethtool.c      |   2 +
 drivers/net/ethernet/sfc/ethtool.c            |   2 +
 drivers/net/ethernet/sfc/ethtool_common.c     |  35 +++-
 drivers/net/ethernet/sfc/ethtool_common.h     |   3 +
 drivers/net/veth.c                            | 167 ++++++++++--------
 drivers/net/virtio_net.c                      |  76 ++++++--
 include/linux/ethtool.h                       |  36 ++++
 include/uapi/linux/ethtool.h                  |   2 +
 include/uapi/linux/ethtool_netlink.h          |  34 ++++
 net/ethtool/netlink.h                         |   1 +
 net/ethtool/stats.c                           | 163 +++++++++++++++--
 net/ethtool/strset.c                          |   5 +
 20 files changed, 659 insertions(+), 275 deletions(-)

-- 
2.31.1

[PATCH net-next 01/21] ethtool, stats: use a shorthand pointer in stats_prepare_data()

[Alexander Lobakin]

Just place dev->ethtool_ops on the stack and use it instead of
dereferencing the former a bunch of times to improve code
readability.

Signed-off-by: Alexander Lobakin <alexandr.lobakin@intel.com>
Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
---
 net/ethtool/stats.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/net/ethtool/stats.c b/net/ethtool/stats.c
index ec07f5765e03..e35c87206b4c 100644
--- a/net/ethtool/stats.c
+++ b/net/ethtool/stats.c
@@ -108,12 +108,15 @@ static int stats_prepare_data(const struct ethnl_req_info *req_base,
 	const struct stats_req_info *req_info = STATS_REQINFO(req_base);
 	struct stats_reply_data *data = STATS_REPDATA(reply_base);
 	struct net_device *dev = reply_base->dev;
+	const struct ethtool_ops *ops;
 	int ret;
 
 	ret = ethnl_ops_begin(dev);
 	if (ret < 0)
 		return ret;
 
+	ops = dev->ethtool_ops;
+
 	/* Mark all stats as unset (see ETHTOOL_STAT_NOT_SET) to prevent them
 	 * from being reported to user space in case driver did not set them.
 	 */
@@ -123,18 +126,17 @@ static int stats_prepare_data(const struct ethnl_req_info *req_base,
 	memset(&data->rmon_stats, 0xff, sizeof(data->rmon_stats));
 
 	if (test_bit(ETHTOOL_STATS_ETH_PHY, req_info->stat_mask) &&
-	    dev->ethtool_ops->get_eth_phy_stats)
-		dev->ethtool_ops->get_eth_phy_stats(dev, &data->phy_stats);
+	    ops->get_eth_phy_stats)
+		ops->get_eth_phy_stats(dev, &data->phy_stats);
 	if (test_bit(ETHTOOL_STATS_ETH_MAC, req_info->stat_mask) &&
-	    dev->ethtool_ops->get_eth_mac_stats)
-		dev->ethtool_ops->get_eth_mac_stats(dev, &data->mac_stats);
+	    ops->get_eth_mac_stats)
+		ops->get_eth_mac_stats(dev, &data->mac_stats);
 	if (test_bit(ETHTOOL_STATS_ETH_CTRL, req_info->stat_mask) &&
-	    dev->ethtool_ops->get_eth_ctrl_stats)
-		dev->ethtool_ops->get_eth_ctrl_stats(dev, &data->ctrl_stats);
+	    ops->get_eth_ctrl_stats)
+		ops->get_eth_ctrl_stats(dev, &data->ctrl_stats);
 	if (test_bit(ETHTOOL_STATS_RMON, req_info->stat_mask) &&
-	    dev->ethtool_ops->get_rmon_stats)
-		dev->ethtool_ops->get_rmon_stats(dev, &data->rmon_stats,
-						 &data->rmon_ranges);
+	    ops->get_rmon_stats)
+		ops->get_rmon_stats(dev, &data->rmon_stats, &data->rmon_ranges);
 
 	ethnl_ops_complete(dev);
 	return 0;
-- 
2.31.1

[PATCH net-next 02/21] ethtool, stats: add compile-time checks for standard stats

[Alexander Lobakin]

Make sure that the number of counters inside stats structures is
with the corresponding Ethtool Netlink definitions.
RMON stats is a special case -- don't take histogram fields into
account.

Signed-off-by: Alexander Lobakin <alexandr.lobakin@intel.com>
Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
---
 net/ethtool/stats.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/net/ethtool/stats.c b/net/ethtool/stats.c
index e35c87206b4c..8b5c27e034f9 100644
--- a/net/ethtool/stats.c
+++ b/net/ethtool/stats.c
@@ -117,6 +117,15 @@ static int stats_prepare_data(const struct ethnl_req_info *req_base,
 
 	ops = dev->ethtool_ops;
 
+	BUILD_BUG_ON(sizeof(data->phy_stats) / sizeof(u64) !=
+		     __ETHTOOL_A_STATS_ETH_PHY_CNT);
+	BUILD_BUG_ON(sizeof(data->mac_stats) / sizeof(u64) !=
+		     __ETHTOOL_A_STATS_ETH_MAC_CNT);
+	BUILD_BUG_ON(sizeof(data->ctrl_stats) / sizeof(u64) !=
+		     __ETHTOOL_A_STATS_ETH_CTRL_CNT);
+	BUILD_BUG_ON(offsetof(typeof(data->rmon_stats), hist) / sizeof(u64) !=
+		     __ETHTOOL_A_STATS_RMON_CNT);
+
 	/* Mark all stats as unset (see ETHTOOL_STAT_NOT_SET) to prevent them
 	 * from being reported to user space in case driver did not set them.
 	 */
-- 
2.31.1

[PATCH net-next 03/21] ethtool, stats: introduce standard XDP statistics

[Alexander Lobakin]

Most of the driver-side XDP enabled drivers provide some statistics
on XDP programs runs and different actions taken (number of passes,
drops, redirects etc.).
Regarding that it's almost pretty the same across all the drivers
(which is obvious), we can implement some sort of "standardized"
statistics using Ethtool standard stats infra to eliminate a lot
of code and stringsets duplication, different approaches to count
these stats and so on.
These new 12 fields provided by the standard XDP stats should cover
most, if not all, stats that might be interesting for collecting and
tracking.
Note that most NIC drivers keep XDP statistics on a per-channel
basis, so this also introduces a new callback for getting a number
of channels which a driver will provide stats for. If it's not
implemented or returns 0, it means stats are global/device-wide.

Signed-off-by: Alexander Lobakin <alexandr.lobakin@intel.com>
Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
---
 include/linux/ethtool.h              |  36 +++++++
 include/uapi/linux/ethtool.h         |   2 +
 include/uapi/linux/ethtool_netlink.h |  34 +++++++
 net/ethtool/netlink.h                |   1 +
 net/ethtool/stats.c                  | 134 ++++++++++++++++++++++++++-
 net/ethtool/strset.c                 |   5 +
 6 files changed, 211 insertions(+), 1 deletion(-)

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 4711b96dae0c..62d617ad9f50 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -380,6 +380,36 @@ struct ethtool_rmon_stats {
 	u64 hist_tx[ETHTOOL_RMON_HIST_MAX];
 };
 
+/**
+ * struct ethtool_xdp_stats - standard driver-side XDP statistics
+ * @packets: number of frames passed to bpf_prog_run_xdp().
+ * @errors: number of general XDP errors, if driver has one unified counter.
+ * @aborted: number of %XDP_ABORTED returns.
+ * @drop: number of %XDP_DROP returns.
+ * @invalid: number of returns of unallowed values (i.e. not XDP_*).
+ * @pass: number of %XDP_PASS returns.
+ * @redirect: number of successfully performed %XDP_REDIRECT requests.
+ * @redirect_errors: number of failed %XDP_REDIRECT requests.
+ * @tx: number of successfully performed %XDP_TX requests.
+ * @tx_errors: number of failed %XDP_TX requests.
+ * @xmit: number of xdp_frames successfully transmitted via .ndo_xdp_xmit().
+ * @xmit_drops: number of frames dropped from .ndo_xdp_xmit().
+ */
+struct ethtool_xdp_stats {
+	u64	packets;
+	u64	errors;
+	u64	aborted;
+	u64	drop;
+	u64	invalid;
+	u64	pass;
+	u64	redirect;
+	u64	redirect_errors;
+	u64	tx;
+	u64	tx_errors;
+	u64	xmit;
+	u64	xmit_drops;
+};
+
 #define ETH_MODULE_EEPROM_PAGE_LEN	128
 #define ETH_MODULE_MAX_I2C_ADDRESS	0x7f
 
@@ -570,6 +600,9 @@ struct ethtool_module_eeprom {
  * @get_eth_ctrl_stats: Query some of the IEEE 802.3 MAC Ctrl statistics.
  * @get_rmon_stats: Query some of the RMON (RFC 2819) statistics.
  *	Set %ranges to a pointer to zero-terminated array of byte ranges.
+ * @get_std_stats_channels: Get the number of channels which get_*_stats will
+ *	return statistics for.
+ * @get_xdp_stats: Query some XDP statistics.
  *
  * All operations are optional (i.e. the function pointer may be set
  * to %NULL) and callers must take this into account.  Callers must
@@ -689,6 +722,9 @@ struct ethtool_ops {
 	void	(*get_rmon_stats)(struct net_device *dev,
 				  struct ethtool_rmon_stats *rmon_stats,
 				  const struct ethtool_rmon_hist_range **ranges);
+	int	(*get_std_stats_channels)(struct net_device *dev, u32 sset);
+	void	(*get_xdp_stats)(struct net_device *dev,
+				 struct ethtool_xdp_stats *xdp_stats);
 };
 
 int ethtool_check_ops(const struct ethtool_ops *ops);
diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index 67aa7134b301..c3f1f3cde739 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -674,6 +674,7 @@ enum ethtool_link_ext_substate_cable_issue {
  * @ETH_SS_STATS_ETH_MAC: names of IEEE 802.3 MAC statistics
  * @ETH_SS_STATS_ETH_CTRL: names of IEEE 802.3 MAC Control statistics
  * @ETH_SS_STATS_RMON: names of RMON statistics
+ * @ETH_SS_STATS_XDP: names of XDP statistics
  *
  * @ETH_SS_COUNT: number of defined string sets
  */
@@ -699,6 +700,7 @@ enum ethtool_stringset {
 	ETH_SS_STATS_ETH_MAC,
 	ETH_SS_STATS_ETH_CTRL,
 	ETH_SS_STATS_RMON,
+	ETH_SS_STATS_XDP,
 
 	/* add new constants above here */
 	ETH_SS_COUNT
diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
index b3b93710eff7..f9d19cfa9397 100644
--- a/include/uapi/linux/ethtool_netlink.h
+++ b/include/uapi/linux/ethtool_netlink.h
@@ -716,6 +716,7 @@ enum {
 	ETHTOOL_STATS_ETH_MAC,
 	ETHTOOL_STATS_ETH_CTRL,
 	ETHTOOL_STATS_RMON,
+	ETHTOOL_STATS_XDP,
 
 	/* add new constants above here */
 	__ETHTOOL_STATS_CNT
@@ -737,6 +738,8 @@ enum {
 	ETHTOOL_A_STATS_GRP_HIST_BKT_HI,	/* u32 */
 	ETHTOOL_A_STATS_GRP_HIST_VAL,		/* u64 */
 
+	ETHTOOL_A_STATS_GRP_STAT_BLOCK,		/* nest */
+
 	/* add new constants above here */
 	__ETHTOOL_A_STATS_GRP_CNT,
 	ETHTOOL_A_STATS_GRP_MAX = (__ETHTOOL_A_STATS_CNT - 1)
@@ -831,6 +834,37 @@ enum {
 	ETHTOOL_A_STATS_RMON_MAX = (__ETHTOOL_A_STATS_RMON_CNT - 1)
 };
 
+enum {
+	/* Number of frames passed to bpf_prog_run_xdp() */
+	ETHTOOL_A_STATS_XDP_PACKETS,
+	/* Number o general XDP errors if driver counts them together */
+	ETHTOOL_A_STATS_XDP_ERRORS,
+	/* Number of %XDP_ABORTED returns */
+	ETHTOOL_A_STATS_XDP_ABORTED,
+	/* Number of %XDP_DROP returns */
+	ETHTOOL_A_STATS_XDP_DROP,
+	/* Number of returns of unallowed values (i.e. not XDP_*) */
+	ETHTOOL_A_STATS_XDP_INVALID,
+	/* Number of %XDP_PASS returns */
+	ETHTOOL_A_STATS_XDP_PASS,
+	/* Number of successfully performed %XDP_REDIRECT requests */
+	ETHTOOL_A_STATS_XDP_REDIRECT,
+	/* Number of failed %XDP_REDIRECT requests */
+	ETHTOOL_A_STATS_XDP_REDIRECT_ERRORS,
+	/* Number of successfully performed %XDP_TX requests */
+	ETHTOOL_A_STATS_XDP_TX,
+	/* Number of failed %XDP_TX requests */
+	ETHTOOL_A_STATS_XDP_TX_ERRORS,
+	/* Number of xdp_frames successfully transmitted via .ndo_xdp_xmit() */
+	ETHTOOL_A_STATS_XDP_XMIT,
+	/* Number of frames dropped from .ndo_xdp_xmit() */
+	ETHTOOL_A_STATS_XDP_XMIT_DROPS,
+
+	/* Add new constants above here */
+	__ETHTOOL_A_STATS_XDP_CNT,
+	ETHTOOL_A_STATS_XDP_MAX = (__ETHTOOL_A_STATS_XDP_CNT - 1)
+};
+
 /* generic netlink info */
 #define ETHTOOL_GENL_NAME "ethtool"
 #define ETHTOOL_GENL_VERSION 1
diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h
index 077aac3929a8..c7983bba0b43 100644
--- a/net/ethtool/netlink.h
+++ b/net/ethtool/netlink.h
@@ -397,5 +397,6 @@ extern const char stats_eth_phy_names[__ETHTOOL_A_STATS_ETH_PHY_CNT][ETH_GSTRING
 extern const char stats_eth_mac_names[__ETHTOOL_A_STATS_ETH_MAC_CNT][ETH_GSTRING_LEN];
 extern const char stats_eth_ctrl_names[__ETHTOOL_A_STATS_ETH_CTRL_CNT][ETH_GSTRING_LEN];
 extern const char stats_rmon_names[__ETHTOOL_A_STATS_RMON_CNT][ETH_GSTRING_LEN];
+extern const char stats_xdp_names[__ETHTOOL_A_STATS_XDP_CNT][ETH_GSTRING_LEN];
 
 #endif /* _NET_ETHTOOL_NETLINK_H */
diff --git a/net/ethtool/stats.c b/net/ethtool/stats.c
index 8b5c27e034f9..76f2a78e8d02 100644
--- a/net/ethtool/stats.c
+++ b/net/ethtool/stats.c
@@ -19,6 +19,8 @@ struct stats_reply_data {
 	struct ethtool_eth_ctrl_stats	ctrl_stats;
 	struct ethtool_rmon_stats	rmon_stats;
 	const struct ethtool_rmon_hist_range	*rmon_ranges;
+	u32				xdp_stats_channels;
+	struct ethtool_xdp_stats	*xdp_stats;
 };
 
 #define STATS_REPDATA(__reply_base) \
@@ -29,6 +31,7 @@ const char stats_std_names[__ETHTOOL_STATS_CNT][ETH_GSTRING_LEN] = {
 	[ETHTOOL_STATS_ETH_MAC]			= "eth-mac",
 	[ETHTOOL_STATS_ETH_CTRL]		= "eth-ctrl",
 	[ETHTOOL_STATS_RMON]			= "rmon",
+	[ETHTOOL_STATS_XDP]			= "xdp",
 };
 
 const char stats_eth_phy_names[__ETHTOOL_A_STATS_ETH_PHY_CNT][ETH_GSTRING_LEN] = {
@@ -73,6 +76,21 @@ const char stats_rmon_names[__ETHTOOL_A_STATS_RMON_CNT][ETH_GSTRING_LEN] = {
 	[ETHTOOL_A_STATS_RMON_JABBER]		= "etherStatsJabbers",
 };
 
+const char stats_xdp_names[__ETHTOOL_A_STATS_XDP_CNT][ETH_GSTRING_LEN] = {
+	[ETHTOOL_A_STATS_XDP_PACKETS]		= "packets",
+	[ETHTOOL_A_STATS_XDP_ERRORS]		= "errors",
+	[ETHTOOL_A_STATS_XDP_ABORTED]		= "aborted",
+	[ETHTOOL_A_STATS_XDP_DROP]		= "drop",
+	[ETHTOOL_A_STATS_XDP_INVALID]		= "invalid",
+	[ETHTOOL_A_STATS_XDP_PASS]		= "pass",
+	[ETHTOOL_A_STATS_XDP_REDIRECT]		= "redirect",
+	[ETHTOOL_A_STATS_XDP_REDIRECT_ERRORS]	= "redirect-errors",
+	[ETHTOOL_A_STATS_XDP_TX]		= "tx",
+	[ETHTOOL_A_STATS_XDP_TX_ERRORS]		= "tx-errors",
+	[ETHTOOL_A_STATS_XDP_XMIT]		= "xmit",
+	[ETHTOOL_A_STATS_XDP_XMIT_DROPS]	= "xmit-drops",
+};
+
 const struct nla_policy ethnl_stats_get_policy[ETHTOOL_A_STATS_GROUPS + 1] = {
 	[ETHTOOL_A_STATS_HEADER]	=
 		NLA_POLICY_NESTED(ethnl_header_policy),
@@ -101,6 +119,37 @@ static int stats_parse_request(struct ethnl_req_info *req_base,
 	return 0;
 }
 
+static int stats_prepare_data_xdp(struct net_device *dev,
+				  struct stats_reply_data *data)
+{
+	const struct ethtool_ops *ops = dev->ethtool_ops;
+	size_t size;
+	int ret;
+
+	/* Zero means stats are global/device-wide */
+	data->xdp_stats_channels = 0;
+
+	if (ops->get_std_stats_channels) {
+		ret = ops->get_std_stats_channels(dev, ETH_SS_STATS_XDP);
+		if (ret > 0)
+			data->xdp_stats_channels = ret;
+	}
+
+	size = array_size(min_not_zero(data->xdp_stats_channels, 1U),
+			  sizeof(*data->xdp_stats));
+	if (unlikely(size == SIZE_MAX))
+		return -EOVERFLOW;
+
+	data->xdp_stats = kvmalloc(size, GFP_KERNEL);
+	if (!data->xdp_stats)
+		return -ENOMEM;
+
+	memset(data->xdp_stats, 0xff, size);
+	ops->get_xdp_stats(dev, data->xdp_stats);
+
+	return 0;
+}
+
 static int stats_prepare_data(const struct ethnl_req_info *req_base,
 			      struct ethnl_reply_data *reply_base,
 			      struct genl_info *info)
@@ -125,6 +174,8 @@ static int stats_prepare_data(const struct ethnl_req_info *req_base,
 		     __ETHTOOL_A_STATS_ETH_CTRL_CNT);
 	BUILD_BUG_ON(offsetof(typeof(data->rmon_stats), hist) / sizeof(u64) !=
 		     __ETHTOOL_A_STATS_RMON_CNT);
+	BUILD_BUG_ON(sizeof(*data->xdp_stats) / sizeof(u64) !=
+		     __ETHTOOL_A_STATS_XDP_CNT);
 
 	/* Mark all stats as unset (see ETHTOOL_STAT_NOT_SET) to prevent them
 	 * from being reported to user space in case driver did not set them.
@@ -146,15 +197,19 @@ static int stats_prepare_data(const struct ethnl_req_info *req_base,
 	if (test_bit(ETHTOOL_STATS_RMON, req_info->stat_mask) &&
 	    ops->get_rmon_stats)
 		ops->get_rmon_stats(dev, &data->rmon_stats, &data->rmon_ranges);
+	if (test_bit(ETHTOOL_STATS_XDP, req_info->stat_mask) &&
+	    ops->get_xdp_stats)
+		ret = stats_prepare_data_xdp(dev, data);
 
 	ethnl_ops_complete(dev);
-	return 0;
+	return ret;
 }
 
 static int stats_reply_size(const struct ethnl_req_info *req_base,
 			    const struct ethnl_reply_data *reply_base)
 {
 	const struct stats_req_info *req_info = STATS_REQINFO(req_base);
+	const struct stats_reply_data *data = STATS_REPDATA(reply_base);
 	unsigned int n_grps = 0, n_stats = 0;
 	int len = 0;
 
@@ -180,6 +235,14 @@ static int stats_reply_size(const struct ethnl_req_info *req_base,
 			nla_total_size(4)) *	/* _A_STATS_GRP_HIST_BKT_HI */
 			ETHTOOL_RMON_HIST_MAX * 2;
 	}
+	if (test_bit(ETHTOOL_STATS_XDP, req_info->stat_mask)) {
+		n_stats += min_not_zero(data->xdp_stats_channels, 1U) *
+			   sizeof(*data->xdp_stats) / sizeof(u64);
+		n_grps++;
+
+		len += (nla_total_size(0) *	/* _A_STATS_GRP_STAT_BLOCK */
+			data->xdp_stats_channels);
+	}
 
 	len += n_grps * (nla_total_size(0) + /* _A_STATS_GRP */
 			 nla_total_size(4) + /* _A_STATS_GRP_ID */
@@ -356,6 +419,64 @@ static int stats_put_rmon_stats(struct sk_buff *skb,
 	return 0;
 }
 
+static int stats_put_xdp_stats_one(struct sk_buff *skb,
+				   const struct ethtool_xdp_stats *xdp_stats)
+{
+	if (stat_put(skb, ETHTOOL_A_STATS_XDP_PACKETS,
+		     xdp_stats->packets) ||
+	    stat_put(skb, ETHTOOL_A_STATS_XDP_ABORTED,
+		     xdp_stats->aborted) ||
+	    stat_put(skb, ETHTOOL_A_STATS_XDP_DROP,
+		     xdp_stats->drop) ||
+	    stat_put(skb, ETHTOOL_A_STATS_XDP_INVALID,
+		     xdp_stats->invalid) ||
+	    stat_put(skb, ETHTOOL_A_STATS_XDP_PASS,
+		     xdp_stats->pass) ||
+	    stat_put(skb, ETHTOOL_A_STATS_XDP_REDIRECT,
+		     xdp_stats->redirect) ||
+	    stat_put(skb, ETHTOOL_A_STATS_XDP_REDIRECT_ERRORS,
+		     xdp_stats->redirect_errors) ||
+	    stat_put(skb, ETHTOOL_A_STATS_XDP_TX,
+		     xdp_stats->tx) ||
+	    stat_put(skb, ETHTOOL_A_STATS_XDP_TX_ERRORS,
+		     xdp_stats->tx_errors) ||
+	    stat_put(skb, ETHTOOL_A_STATS_XDP_XMIT,
+		     xdp_stats->xmit) ||
+	    stat_put(skb, ETHTOOL_A_STATS_XDP_XMIT_DROPS,
+		     xdp_stats->xmit_drops))
+		return -EMSGSIZE;
+
+	return 0;
+}
+
+static int stats_put_xdp_stats(struct sk_buff *skb,
+			       const struct stats_reply_data *data)
+{
+	struct nlattr *nest;
+	int i;
+
+	if (!data->xdp_stats_channels)
+		return stats_put_xdp_stats_one(skb, data->xdp_stats);
+
+	for (i = 0; i < data->xdp_stats_channels; i++) {
+		nest = nla_nest_start(skb, ETHTOOL_A_STATS_GRP_STAT_BLOCK);
+		if (!nest)
+			return -EMSGSIZE;
+
+		if (stats_put_xdp_stats_one(skb, data->xdp_stats + i))
+			goto err_cancel_xdp;
+
+		nla_nest_end(skb, nest);
+	}
+
+	return 0;
+
+err_cancel_xdp:
+	nla_nest_cancel(skb, nest);
+
+	return -EMSGSIZE;
+}
+
 static int stats_put_stats(struct sk_buff *skb,
 			   const struct stats_reply_data *data,
 			   u32 id, u32 ss_id,
@@ -406,10 +527,20 @@ static int stats_fill_reply(struct sk_buff *skb,
 	if (!ret && test_bit(ETHTOOL_STATS_RMON, req_info->stat_mask))
 		ret = stats_put_stats(skb, data, ETHTOOL_STATS_RMON,
 				      ETH_SS_STATS_RMON, stats_put_rmon_stats);
+	if (!ret && test_bit(ETHTOOL_STATS_XDP, req_info->stat_mask))
+		ret = stats_put_stats(skb, data, ETHTOOL_STATS_XDP,
+				      ETH_SS_STATS_XDP, stats_put_xdp_stats);
 
 	return ret;
 }
 
+static void stats_cleanup_data(struct ethnl_reply_data *reply_data)
+{
+	struct stats_reply_data *data = STATS_REPDATA(reply_data);
+
+	kvfree(data->xdp_stats);
+}
+
 const struct ethnl_request_ops ethnl_stats_request_ops = {
 	.request_cmd		= ETHTOOL_MSG_STATS_GET,
 	.reply_cmd		= ETHTOOL_MSG_STATS_GET_REPLY,
@@ -421,4 +552,5 @@ const struct ethnl_request_ops ethnl_stats_request_ops = {
 	.prepare_data		= stats_prepare_data,
 	.reply_size		= stats_reply_size,
 	.fill_reply		= stats_fill_reply,
+	.cleanup_data		= stats_cleanup_data,
 };
diff --git a/net/ethtool/strset.c b/net/ethtool/strset.c
index 2d51b7ab4dc5..a149048f9c34 100644
--- a/net/ethtool/strset.c
+++ b/net/ethtool/strset.c
@@ -105,6 +105,11 @@ static const struct strset_info info_template[] = {
 		.count		= __ETHTOOL_A_STATS_RMON_CNT,
 		.strings	= stats_rmon_names,
 	},
+	[ETH_SS_STATS_XDP] = {
+		.per_dev	= false,
+		.count		= __ETHTOOL_A_STATS_XDP_CNT,
+		.strings	= stats_xdp_names,
+	},
 };
 
 struct strset_req_info {
-- 
2.31.1

Re: [PATCH net-next 03/21] ethtool, stats: introduce standard XDP statistics

[Jakub Kicinski]

On Tue,  3 Aug 2021 18:36:23 +0200 Alexander Lobakin wrote:
> Most of the driver-side XDP enabled drivers provide some statistics
> on XDP programs runs and different actions taken (number of passes,
> drops, redirects etc.).

Could you please share the statistics to back that statement up?
Having uAPI for XDP stats is pretty much making the recommendation 
that drivers should implement such stats. The recommendation from
Alexei and others back in the day (IIRC) was that XDP programs should
implement stats, not the drivers, to avoid duplication.

> Regarding that it's almost pretty the same across all the drivers
> (which is obvious), we can implement some sort of "standardized"
> statistics using Ethtool standard stats infra to eliminate a lot
> of code and stringsets duplication, different approaches to count
> these stats and so on.

I'm not 100% sold on the fact that these should be ethtool stats. 
Why not rtnl_fill_statsinfo() stats? Current ethtool std stats are 
all pretty Ethernet specific, and all HW stats. Mixing HW and SW stats
is what we're trying to get away from.

> These new 12 fields provided by the standard XDP stats should cover
> most, if not all, stats that might be interesting for collecting and
> tracking.
> Note that most NIC drivers keep XDP statistics on a per-channel
> basis, so this also introduces a new callback for getting a number
> of channels which a driver will provide stats for. If it's not
> implemented or returns 0, it means stats are global/device-wide.

Per-channel stats via std ethtool stats are not a good idea. Per queue
stats must be via the queue netlink interface we keep talking about for
ever but which doesn't seem to materialize. When stats are reported via
a different interface than objects they pertain to matching stats,
objects and their lifetime becomes very murky.

Re: [PATCH net-next 03/21] ethtool, stats: introduce standard XDP statistics

[Saeed Mahameed]

On Tue, 2021-08-03 at 13:49 -0700, Jakub Kicinski wrote:
> On Tue,  3 Aug 2021 18:36:23 +0200 Alexander Lobakin wrote:
> > Most of the driver-side XDP enabled drivers provide some statistics
> > on XDP programs runs and different actions taken (number of passes,
> > drops, redirects etc.).
> 
> Could you please share the statistics to back that statement up?
> Having uAPI for XDP stats is pretty much making the recommendation 
> that drivers should implement such stats. The recommendation from
> Alexei and others back in the day (IIRC) was that XDP programs should
> implement stats, not the drivers, to avoid duplication.
> 

There are stats "mainly errors*"  that are not even visible or reported
to the user prog, for that i had an idea in the past to attach an
exception_bpf_prog provided by the user, where driver/stack will report
errors to this special exception_prog.

> > Regarding that it's almost pretty the same across all the drivers
> > (which is obvious), we can implement some sort of "standardized"
> > statistics using Ethtool standard stats infra to eliminate a lot
> > of code and stringsets duplication, different approaches to count
> > these stats and so on.
> 
> I'm not 100% sold on the fact that these should be ethtool stats. 
> Why not rtnl_fill_statsinfo() stats? Current ethtool std stats are 
> all pretty Ethernet specific, and all HW stats. Mixing HW and SW
> stats
> is what we're trying to get away from.
> 

XDP is going to always be eBPF based ! why not just report such stats
to a special BPF_MAP ? BPF stack can collect the stats from the driver
and report them to this special MAP upon user request.

> > These new 12 fields provided by the standard XDP stats should cover
> > most, if not all, stats that might be interesting for collecting
> > and
> > tracking.
> > Note that most NIC drivers keep XDP statistics on a per-channel
> > basis, so this also introduces a new callback for getting a number
> > of channels which a driver will provide stats for. If it's not
> > implemented or returns 0, it means stats are global/device-wide.
> 
> Per-channel stats via std ethtool stats are not a good idea. Per
> queue
> stats must be via the queue netlink interface we keep talking about
> for
> ever but which doesn't seem to materialize. When stats are reported
> via
> a different interface than objects they pertain to matching stats,
> objects and their lifetime becomes very murky.

Re: [PATCH net-next 03/21] ethtool, stats: introduce standard XDP statistics

[Jakub Kicinski]

On Tue, 03 Aug 2021 16:57:22 -0700 Saeed Mahameed wrote:
> On Tue, 2021-08-03 at 13:49 -0700, Jakub Kicinski wrote:
> > On Tue,  3 Aug 2021 18:36:23 +0200 Alexander Lobakin wrote:  
> > > Most of the driver-side XDP enabled drivers provide some statistics
> > > on XDP programs runs and different actions taken (number of passes,
> > > drops, redirects etc.).  
> > 
> > Could you please share the statistics to back that statement up?
> > Having uAPI for XDP stats is pretty much making the recommendation 
> > that drivers should implement such stats. The recommendation from
> > Alexei and others back in the day (IIRC) was that XDP programs should
> > implement stats, not the drivers, to avoid duplication.
> 
> There are stats "mainly errors*"  that are not even visible or reported
> to the user prog, 

Fair point, exceptions should not be performance critical.

> for that i had an idea in the past to attach an
> exception_bpf_prog provided by the user, where driver/stack will report
> errors to this special exception_prog.

Or maybe we should turn trace_xdp_exception() into a call which
unconditionally collects exception stats? I think we can reasonably
expect the exception_bpf_prog to always be attached, right?

> > > Regarding that it's almost pretty the same across all the drivers
> > > (which is obvious), we can implement some sort of "standardized"
> > > statistics using Ethtool standard stats infra to eliminate a lot
> > > of code and stringsets duplication, different approaches to count
> > > these stats and so on.  
> > 
> > I'm not 100% sold on the fact that these should be ethtool stats. 
> > Why not rtnl_fill_statsinfo() stats? Current ethtool std stats are 
> > all pretty Ethernet specific, and all HW stats. Mixing HW and SW
> > stats
> > is what we're trying to get away from.
> 
> XDP is going to always be eBPF based ! why not just report such stats
> to a special BPF_MAP ? BPF stack can collect the stats from the driver
> and report them to this special MAP upon user request.

Do you mean replacing the ethtool-netlink / rtnetlink etc. with
a new BPF_MAP? I don't think adding another category of uAPI thru 
which netdevice stats are exposed would do much good :( Plus it 
doesn't address the "yet another cacheline" concern.

To my understanding the need for stats recognizes the fact that (in
large organizations) fleet monitoring is done by different teams than
XDP development. So XDP team may have all the stats they need, but the
team doing fleet monitoring has no idea how to get to them.

To bridge the two worlds we need a way for the infra team to ask the
XDP for well-defined stats. Maybe we should take a page from the BPF
iterators book and create a program type for bridging the two worlds?
Called by networking core when duping stats to extract from the
existing BPF maps all the relevant stats and render them into a well
known struct? Users' XDP design can still use a single per-cpu map with
all the stats if they so choose, but there's a way to implement more
optimal designs and still expose well-defined stats.

Maybe that's too complex, IDK.

Re: [PATCH net-next 03/21] ethtool, stats: introduce standard XDP statistics

[Alexander Lobakin]

From: Jakub Kicinski <kuba@kernel.org>
Date: Wed, 4 Aug 2021 05:36:50 -0700

> On Tue, 03 Aug 2021 16:57:22 -0700 Saeed Mahameed wrote:
> > On Tue, 2021-08-03 at 13:49 -0700, Jakub Kicinski wrote:
> > > On Tue,  3 Aug 2021 18:36:23 +0200 Alexander Lobakin wrote:  
> > > > Most of the driver-side XDP enabled drivers provide some statistics
> > > > on XDP programs runs and different actions taken (number of passes,
> > > > drops, redirects etc.).  
> > > 
> > > Could you please share the statistics to back that statement up?
> > > Having uAPI for XDP stats is pretty much making the recommendation 
> > > that drivers should implement such stats. The recommendation from
> > > Alexei and others back in the day (IIRC) was that XDP programs should
> > > implement stats, not the drivers, to avoid duplication.

Well, 20+ patches in the series with at least half of them is
drivers conversion. Plus mlx5. Plus we'll about to land XDP
statistics for all Intel drivers, just firstly need to get a
common infra for them (the purpose of this series).

Also, introducing IEEE and rmon stats didn't make a statement that
all drivers should really expose them, right?

> > There are stats "mainly errors*"  that are not even visible or reported
> > to the user prog, 

Not really. Many drivers like to count the number of redirects,
xdp_xmits and stuff (incl. mlx5). Nevertheless, these stats aren't
the same as something you can get from inside an XDP prog, right.

> Fair point, exceptions should not be performance critical.
> 
> > for that i had an idea in the past to attach an
> > exception_bpf_prog provided by the user, where driver/stack will report
> > errors to this special exception_prog.
> 
> Or maybe we should turn trace_xdp_exception() into a call which
> unconditionally collects exception stats? I think we can reasonably
> expect the exception_bpf_prog to always be attached, right?

trace_xdp_exception() is again a error path, and would restrict us
to have only "bad" statistics.

> > > > Regarding that it's almost pretty the same across all the drivers
> > > > (which is obvious), we can implement some sort of "standardized"
> > > > statistics using Ethtool standard stats infra to eliminate a lot
> > > > of code and stringsets duplication, different approaches to count
> > > > these stats and so on.  
> > > 
> > > I'm not 100% sold on the fact that these should be ethtool stats. 
> > > Why not rtnl_fill_statsinfo() stats? Current ethtool std stats are 
> > > all pretty Ethernet specific, and all HW stats. Mixing HW and SW
> > > stats
> > > is what we're trying to get away from.

I was trying to introduce as few functional changes as possible,
including that all the current drivers expose XDP stats through
Ethtool.
I don't say it's a 100% optimal way, but lots of different scripts
and monitoring tools are already based on this fact and there can
be some negative impact. There'll be for sure due to that std stats
is a bit different thing and different drivers count and name XDP
stats differently (breh).

BTW, I'm fine with rtnl xstats. A nice reminder, thanks. If there
won't be much cons like "don't touch our Ethtool stats", I would
prefer this one instead of Ethtool standard stats way.

> > XDP is going to always be eBPF based ! why not just report such stats
> > to a special BPF_MAP ? BPF stack can collect the stats from the driver
> > and report them to this special MAP upon user request.
> 
> Do you mean replacing the ethtool-netlink / rtnetlink etc. with
> a new BPF_MAP? I don't think adding another category of uAPI thru 
> which netdevice stats are exposed would do much good :( Plus it 
> doesn't address the "yet another cacheline" concern.

+ this makes obtaining/tracking the statistics much harder. For now,
all you need is `ethtool -S devname` (mainline) or
`ethtool -S devname --groups xdp` (this series), and obtaining rtnl
xstats is just a different command to invoke. BPF_MAP-based stats
are a completely different story then.

> To my understanding the need for stats recognizes the fact that (in
> large organizations) fleet monitoring is done by different teams than
> XDP development. So XDP team may have all the stats they need, but the
> team doing fleet monitoring has no idea how to get to them.
> 
> To bridge the two worlds we need a way for the infra team to ask the
> XDP for well-defined stats. Maybe we should take a page from the BPF
> iterators book and create a program type for bridging the two worlds?
> Called by networking core when duping stats to extract from the
> existing BPF maps all the relevant stats and render them into a well
> known struct? Users' XDP design can still use a single per-cpu map with
> all the stats if they so choose, but there's a way to implement more
> optimal designs and still expose well-defined stats.
> 
> Maybe that's too complex, IDK.

Thanks,
Al

Re: [PATCH net-next 03/21] ethtool, stats: introduce standard XDP statistics

[Jakub Kicinski]

On Wed,  4 Aug 2021 17:53:27 +0200 Alexander Lobakin wrote:
> From: Jakub Kicinski <kuba@kernel.org>
> Date: Wed, 4 Aug 2021 05:36:50 -0700
> 
> > On Tue, 03 Aug 2021 16:57:22 -0700 Saeed Mahameed wrote:  
> > > On Tue, 2021-08-03 at 13:49 -0700, Jakub Kicinski wrote:  
> > > > On Tue,  3 Aug 2021 18:36:23 +0200 Alexander Lobakin wrote:    
> > > > > Most of the driver-side XDP enabled drivers provide some statistics
> > > > > on XDP programs runs and different actions taken (number of passes,
> > > > > drops, redirects etc.).    
> > > > 
> > > > Could you please share the statistics to back that statement up?
> > > > Having uAPI for XDP stats is pretty much making the recommendation 
> > > > that drivers should implement such stats. The recommendation from
> > > > Alexei and others back in the day (IIRC) was that XDP programs should
> > > > implement stats, not the drivers, to avoid duplication.  
> 
> Well, 20+ patches in the series with at least half of them is
> drivers conversion. Plus mlx5. Plus we'll about to land XDP
> statistics for all Intel drivers, just firstly need to get a
> common infra for them (the purpose of this series).

Great, do you have impact of the stats on Intel drivers?
(Preferably from realistic scenarios where CPU cache is actually 
under pressure, not { return XDP_PASS; }). Numbers win arguments.

> Also, introducing IEEE and rmon stats didn't make a statement that
> all drivers should really expose them, right?

That's not relevant. IEEE and RMON stats are read from HW, they have 
no impact on the SW fast path.

> > > There are stats "mainly errors*"  that are not even visible or reported
> > > to the user prog,   
> 
> Not really. Many drivers like to count the number of redirects,
> xdp_xmits and stuff (incl. mlx5). Nevertheless, these stats aren't
> the same as something you can get from inside an XDP prog, right.
> 
> > Fair point, exceptions should not be performance critical.
> >   
> > > for that i had an idea in the past to attach an
> > > exception_bpf_prog provided by the user, where driver/stack will report
> > > errors to this special exception_prog.  
> > 
> > Or maybe we should turn trace_xdp_exception() into a call which
> > unconditionally collects exception stats? I think we can reasonably
> > expect the exception_bpf_prog to always be attached, right?  
> 
> trace_xdp_exception() is again a error path, and would restrict us
> to have only "bad" statistics.
> 
> > > > > Regarding that it's almost pretty the same across all the drivers
> > > > > (which is obvious), we can implement some sort of "standardized"
> > > > > statistics using Ethtool standard stats infra to eliminate a lot
> > > > > of code and stringsets duplication, different approaches to count
> > > > > these stats and so on.    
> > > > 
> > > > I'm not 100% sold on the fact that these should be ethtool stats. 
> > > > Why not rtnl_fill_statsinfo() stats? Current ethtool std stats are 
> > > > all pretty Ethernet specific, and all HW stats. Mixing HW and SW
> > > > stats
> > > > is what we're trying to get away from.  
> 
> I was trying to introduce as few functional changes as possible,
> including that all the current drivers expose XDP stats through
> Ethtool.

You know this, but for the benefit of others - ethtool -S does not 
dump standard stats from the netlink API, and ethtool -S --goups does
not dump "old" stats. So users will need to use different commands
to get to the two, anyway.

> I don't say it's a 100% optimal way, but lots of different scripts
> and monitoring tools are already based on this fact and there can
> be some negative impact. There'll be for sure due to that std stats
> is a bit different thing and different drivers count and name XDP
> stats differently (breh).

That's concerning. I'd much rather you didn't convert all the drivers
than convert them before someone makes 100% sure the meaning of the
stats is equivalent.

> BTW, I'm fine with rtnl xstats. A nice reminder, thanks. If there
> won't be much cons like "don't touch our Ethtool stats", I would
> prefer this one instead of Ethtool standard stats way.

You'll have to leave the ethtool -S ones in place anyway, right?
New drivers would not include them but I don't think there's much
we can (or should) do for the existing ones.

> > > XDP is going to always be eBPF based ! why not just report such stats
> > > to a special BPF_MAP ? BPF stack can collect the stats from the driver
> > > and report them to this special MAP upon user request.  
> > 
> > Do you mean replacing the ethtool-netlink / rtnetlink etc. with
> > a new BPF_MAP? I don't think adding another category of uAPI thru 
> > which netdevice stats are exposed would do much good :( Plus it 
> > doesn't address the "yet another cacheline" concern.  
> 
> + this makes obtaining/tracking the statistics much harder. For now,
> all you need is `ethtool -S devname` (mainline) or
> `ethtool -S devname --groups xdp` (this series), and obtaining rtnl
> xstats is just a different command to invoke. BPF_MAP-based stats
> are a completely different story then.
> 
> > To my understanding the need for stats recognizes the fact that (in
> > large organizations) fleet monitoring is done by different teams than
> > XDP development. So XDP team may have all the stats they need, but the
> > team doing fleet monitoring has no idea how to get to them.
> > 
> > To bridge the two worlds we need a way for the infra team to ask the
> > XDP for well-defined stats. Maybe we should take a page from the BPF
> > iterators book and create a program type for bridging the two worlds?
> > Called by networking core when duping stats to extract from the
> > existing BPF maps all the relevant stats and render them into a well
> > known struct? Users' XDP design can still use a single per-cpu map with
> > all the stats if they so choose, but there's a way to implement more
> > optimal designs and still expose well-defined stats.
> > 
> > Maybe that's too complex, IDK.  
> 
> Thanks,
> Al

Re: [PATCH net-next 03/21] ethtool, stats: introduce standard XDP statistics

[Alexander Lobakin]

From: Jakub Kicinski <kuba@kernel.org>
Date: Wed, 5 Aug 2021 09:57:16 -0700

> On Wed,  4 Aug 2021 17:53:27 +0200 Alexander Lobakin wrote:
> > From: Jakub Kicinski <kuba@kernel.org>
> > Date: Wed, 4 Aug 2021 05:36:50 -0700
> > 
> > > On Tue, 03 Aug 2021 16:57:22 -0700 Saeed Mahameed wrote:  
> > > > On Tue, 2021-08-03 at 13:49 -0700, Jakub Kicinski wrote:  
> > > > > On Tue,  3 Aug 2021 18:36:23 +0200 Alexander Lobakin wrote:    
> > > > > > Most of the driver-side XDP enabled drivers provide some statistics
> > > > > > on XDP programs runs and different actions taken (number of passes,
> > > > > > drops, redirects etc.).    
> > > > > 
> > > > > Could you please share the statistics to back that statement up?
> > > > > Having uAPI for XDP stats is pretty much making the recommendation 
> > > > > that drivers should implement such stats. The recommendation from
> > > > > Alexei and others back in the day (IIRC) was that XDP programs should
> > > > > implement stats, not the drivers, to avoid duplication.  
> > 
> > Well, 20+ patches in the series with at least half of them is
> > drivers conversion. Plus mlx5. Plus we'll about to land XDP
> > statistics for all Intel drivers, just firstly need to get a
> > common infra for them (the purpose of this series).
> 
> Great, do you have impact of the stats on Intel drivers?
> (Preferably from realistic scenarios where CPU cache is actually 
> under pressure, not { return XDP_PASS; }). Numbers win arguments.
> 
> > Also, introducing IEEE and rmon stats didn't make a statement that
> > all drivers should really expose them, right?
> 
> That's not relevant. IEEE and RMON stats are read from HW, they have 
> no impact on the SW fast path.

True, I thought about this, but after the mail was sent, sorry.

> > > > There are stats "mainly errors*"  that are not even visible or reported
> > > > to the user prog,   
> > 
> > Not really. Many drivers like to count the number of redirects,
> > xdp_xmits and stuff (incl. mlx5). Nevertheless, these stats aren't
> > the same as something you can get from inside an XDP prog, right.
> > 
> > > Fair point, exceptions should not be performance critical.
> > >   
> > > > for that i had an idea in the past to attach an
> > > > exception_bpf_prog provided by the user, where driver/stack will report
> > > > errors to this special exception_prog.  
> > > 
> > > Or maybe we should turn trace_xdp_exception() into a call which
> > > unconditionally collects exception stats? I think we can reasonably
> > > expect the exception_bpf_prog to always be attached, right?  
> > 
> > trace_xdp_exception() is again a error path, and would restrict us
> > to have only "bad" statistics.
> > 
> > > > > > Regarding that it's almost pretty the same across all the drivers
> > > > > > (which is obvious), we can implement some sort of "standardized"
> > > > > > statistics using Ethtool standard stats infra to eliminate a lot
> > > > > > of code and stringsets duplication, different approaches to count
> > > > > > these stats and so on.    
> > > > > 
> > > > > I'm not 100% sold on the fact that these should be ethtool stats. 
> > > > > Why not rtnl_fill_statsinfo() stats? Current ethtool std stats are 
> > > > > all pretty Ethernet specific, and all HW stats. Mixing HW and SW
> > > > > stats
> > > > > is what we're trying to get away from.  
> > 
> > I was trying to introduce as few functional changes as possible,
> > including that all the current drivers expose XDP stats through
> > Ethtool.
> 
> You know this, but for the benefit of others - ethtool -S does not 
> dump standard stats from the netlink API, and ethtool -S --goups does
> not dump "old" stats. So users will need to use different commands
> to get to the two, anyway.

Ditto.

> > I don't say it's a 100% optimal way, but lots of different scripts
> > and monitoring tools are already based on this fact and there can
> > be some negative impact. There'll be for sure due to that std stats
> > is a bit different thing and different drivers count and name XDP
> > stats differently (breh).
> 
> That's concerning. I'd much rather you didn't convert all the drivers
> than convert them before someone makes 100% sure the meaning of the
> stats is equivalent.
> 
> > BTW, I'm fine with rtnl xstats. A nice reminder, thanks. If there
> > won't be much cons like "don't touch our Ethtool stats", I would
> > prefer this one instead of Ethtool standard stats way.
> 
> You'll have to leave the ethtool -S ones in place anyway, right?
> New drivers would not include them but I don't think there's much
> we can (or should) do for the existing ones.

That's also a nice and fair one. So I leave a little conclusion
at the end of this message to clarify things.

> > > > XDP is going to always be eBPF based ! why not just report such stats
> > > > to a special BPF_MAP ? BPF stack can collect the stats from the driver
> > > > and report them to this special MAP upon user request.  
> > > 
> > > Do you mean replacing the ethtool-netlink / rtnetlink etc. with
> > > a new BPF_MAP? I don't think adding another category of uAPI thru 
> > > which netdevice stats are exposed would do much good :( Plus it 
> > > doesn't address the "yet another cacheline" concern.  
> > 
> > + this makes obtaining/tracking the statistics much harder. For now,
> > all you need is `ethtool -S devname` (mainline) or
> > `ethtool -S devname --groups xdp` (this series), and obtaining rtnl
> > xstats is just a different command to invoke. BPF_MAP-based stats
> > are a completely different story then.
> > 
> > > To my understanding the need for stats recognizes the fact that (in
> > > large organizations) fleet monitoring is done by different teams than
> > > XDP development. So XDP team may have all the stats they need, but the
> > > team doing fleet monitoring has no idea how to get to them.
> > > 
> > > To bridge the two worlds we need a way for the infra team to ask the
> > > XDP for well-defined stats. Maybe we should take a page from the BPF
> > > iterators book and create a program type for bridging the two worlds?
> > > Called by networking core when duping stats to extract from the
> > > existing BPF maps all the relevant stats and render them into a well
> > > known struct? Users' XDP design can still use a single per-cpu map with
> > > all the stats if they so choose, but there's a way to implement more
> > > optimal designs and still expose well-defined stats.
> > > 
> > > Maybe that's too complex, IDK.  

[ the conclusion I talk about above ]

From the concerns, ideas and thoughts gone throughout this thread,
I see the next steps as:
 - don't use Ethtool standard stats infra as it's designed for HW
   stats only;
 - expose SW XDP stats via rtnl xstats which is currently used to
   query SW/CPU-port stats from switchdev-based drivers;
 - don't remove the existing Ethtool XDP stats provided by drivers,
   just wire them up with the new API too;
 - encourage driver developers to use this new API when they want to
   provide any XDP stats, not the Ethtool stats which are already
   overburdened and mix HW, SW and whatnot in most of the complex
   drivers.

That really makes lot more sense for me than the v1 approach.

Anyways, I don't think I'll queue v2 before going on a vacation
which will happen in two days, so we have two more weeks to
discuss all this before I'll start the rework. I'll still be on
the line here and keep on tracking the thread and replying when
needed.

> > Thanks,
> > Al

Thanks everyone for the feedback, lots of precious stuff,
Al

Re: [PATCH net-next 03/21] ethtool, stats: introduce standard XDP statistics

[Jakub Kicinski]

On Thu,  5 Aug 2021 13:18:26 +0200 Alexander Lobakin wrote:
>  - encourage driver developers to use this new API when they want to
>    provide any XDP stats, not the Ethtool stats which are already
>    overburdened and mix HW, SW and whatnot in most of the complex
>    drivers.

On the question of adding the stats in general I'd still ask for 
(a) performance analysis or (b) to start with just the exception 
stats which I'd think should be uncontroversial.

Re: [PATCH net-next 03/21] ethtool, stats: introduce standard XDP statistics

[David Ahern]

On 8/4/21 6:36 AM, Jakub Kicinski wrote:
>> XDP is going to always be eBPF based ! why not just report such stats
>> to a special BPF_MAP ? BPF stack can collect the stats from the driver
>> and report them to this special MAP upon user request.
> Do you mean replacing the ethtool-netlink / rtnetlink etc. with
> a new BPF_MAP? I don't think adding another category of uAPI thru 
> which netdevice stats are exposed would do much good :( Plus it 
> doesn't address the "yet another cacheline" concern.
> 
> To my understanding the need for stats recognizes the fact that (in
> large organizations) fleet monitoring is done by different teams than
> XDP development. So XDP team may have all the stats they need, but the
> team doing fleet monitoring has no idea how to get to them.
> 
> To bridge the two worlds we need a way for the infra team to ask the
> XDP for well-defined stats. Maybe we should take a page from the BPF
> iterators book and create a program type for bridging the two worlds?
> Called by networking core when duping stats to extract from the
> existing BPF maps all the relevant stats and render them into a well
> known struct? Users' XDP design can still use a single per-cpu map with
> all the stats if they so choose, but there's a way to implement more
> optimal designs and still expose well-defined stats.
> 
> Maybe that's too complex, IDK.

I was just explaining to someone internally how to get stats at all of
the different points in the stack to track down reasons for dropped packets:

ethtool -S for h/w and driver
tc -s for drops by the qdisc
/proc/net/softnet_stat for drops at the backlog layer
netstat -s for network and transport layer

yet another command and API just adds to the nightmare of explaining and
understanding these stats.

There is real value in continuing to use ethtool API for XDP stats. Not
saying this reorg of the XDP stats is the right thing to do, only that
the existing API has real user benefits.

Does anyone have data that shows bumping a properly implemented counter
causes a noticeable performance degradation and if so by how much? You
mention 'yet another cacheline' but collecting stats on stack and
incrementing the driver structs at the end of the napi loop should not
have a huge impact versus the value the stats provide.

Re: [PATCH net-next 03/21] ethtool, stats: introduce standard XDP statistics

[Jakub Kicinski]

On Wed, 4 Aug 2021 10:17:56 -0600 David Ahern wrote:
> On 8/4/21 6:36 AM, Jakub Kicinski wrote:
> >> XDP is going to always be eBPF based ! why not just report such stats
> >> to a special BPF_MAP ? BPF stack can collect the stats from the driver
> >> and report them to this special MAP upon user request.  
> > Do you mean replacing the ethtool-netlink / rtnetlink etc. with
> > a new BPF_MAP? I don't think adding another category of uAPI thru 
> > which netdevice stats are exposed would do much good :( Plus it 
> > doesn't address the "yet another cacheline" concern.
> > 
> > To my understanding the need for stats recognizes the fact that (in
> > large organizations) fleet monitoring is done by different teams than
> > XDP development. So XDP team may have all the stats they need, but the
> > team doing fleet monitoring has no idea how to get to them.
> > 
> > To bridge the two worlds we need a way for the infra team to ask the
> > XDP for well-defined stats. Maybe we should take a page from the BPF
> > iterators book and create a program type for bridging the two worlds?
> > Called by networking core when duping stats to extract from the
> > existing BPF maps all the relevant stats and render them into a well
> > known struct? Users' XDP design can still use a single per-cpu map with
> > all the stats if they so choose, but there's a way to implement more
> > optimal designs and still expose well-defined stats.
> > 
> > Maybe that's too complex, IDK.  
> 
> I was just explaining to someone internally how to get stats at all of
> the different points in the stack to track down reasons for dropped packets:
> 
> ethtool -S for h/w and driver
> tc -s for drops by the qdisc
> /proc/net/softnet_stat for drops at the backlog layer
> netstat -s for network and transport layer
> 
> yet another command and API just adds to the nightmare of explaining and
> understanding these stats.

Are you referring to RTM_GETSTATS when you say "yet another command"?
RTM_GETSTATS exists and is used by offloads today.

I'd expect ip -s (-s) to be extended to run GETSTATS and display the xdp
stats. (Not sure why ip -s was left out of your list :))

> There is real value in continuing to use ethtool API for XDP stats. Not
> saying this reorg of the XDP stats is the right thing to do, only that
> the existing API has real user benefits.

RTM_GETSTATS is an existing API. New ethtool stats are intended to be HW
stats. I don't want to go back to ethtool being a dumping ground for all
stats because that's what the old interface encouraged.

> Does anyone have data that shows bumping a properly implemented counter
> causes a noticeable performance degradation and if so by how much? You
> mention 'yet another cacheline' but collecting stats on stack and
> incrementing the driver structs at the end of the napi loop should not
> have a huge impact versus the value the stats provide.

Not sure, maybe Jesper has some numbers. Maybe Intel folks do?

I'm just allergic to situations when there is a decision made and 
then months later patches are posted disregarding the decision, 
without analysis on why that decision was wrong. And while the
maintainer who made the decision is on vacation.

Re: [PATCH net-next 03/21] ethtool, stats: introduce standard XDP statistics

[David Ahern]

On 8/4/21 10:44 AM, Jakub Kicinski wrote:
> On Wed, 4 Aug 2021 10:17:56 -0600 David Ahern wrote:
>> On 8/4/21 6:36 AM, Jakub Kicinski wrote:
>>>> XDP is going to always be eBPF based ! why not just report such stats
>>>> to a special BPF_MAP ? BPF stack can collect the stats from the driver
>>>> and report them to this special MAP upon user request.  
>>> Do you mean replacing the ethtool-netlink / rtnetlink etc. with
>>> a new BPF_MAP? I don't think adding another category of uAPI thru 
>>> which netdevice stats are exposed would do much good :( Plus it 
>>> doesn't address the "yet another cacheline" concern.
>>>
>>> To my understanding the need for stats recognizes the fact that (in
>>> large organizations) fleet monitoring is done by different teams than
>>> XDP development. So XDP team may have all the stats they need, but the
>>> team doing fleet monitoring has no idea how to get to them.
>>>
>>> To bridge the two worlds we need a way for the infra team to ask the
>>> XDP for well-defined stats. Maybe we should take a page from the BPF
>>> iterators book and create a program type for bridging the two worlds?
>>> Called by networking core when duping stats to extract from the
>>> existing BPF maps all the relevant stats and render them into a well
>>> known struct? Users' XDP design can still use a single per-cpu map with
>>> all the stats if they so choose, but there's a way to implement more
>>> optimal designs and still expose well-defined stats.
>>>
>>> Maybe that's too complex, IDK.  
>>
>> I was just explaining to someone internally how to get stats at all of
>> the different points in the stack to track down reasons for dropped packets:
>>
>> ethtool -S for h/w and driver
>> tc -s for drops by the qdisc
>> /proc/net/softnet_stat for drops at the backlog layer
>> netstat -s for network and transport layer
>>
>> yet another command and API just adds to the nightmare of explaining and
>> understanding these stats.
> 
> Are you referring to RTM_GETSTATS when you say "yet another command"?
> RTM_GETSTATS exists and is used by offloads today.
> 
> I'd expect ip -s (-s) to be extended to run GETSTATS and display the xdp
> stats. (Not sure why ip -s was left out of your list :))

It's on my diagram, and yes, forgot to add it here.

> 
>> There is real value in continuing to use ethtool API for XDP stats. Not
>> saying this reorg of the XDP stats is the right thing to do, only that
>> the existing API has real user benefits.
> 
> RTM_GETSTATS is an existing API. New ethtool stats are intended to be HW
> stats. I don't want to go back to ethtool being a dumping ground for all
> stats because that's what the old interface encouraged.

driver stats are important too. e.g., mlx5's cache stats and per-queue
stats.

> 
>> Does anyone have data that shows bumping a properly implemented counter
>> causes a noticeable performance degradation and if so by how much? You
>> mention 'yet another cacheline' but collecting stats on stack and
>> incrementing the driver structs at the end of the napi loop should not
>> have a huge impact versus the value the stats provide.
> 
> Not sure, maybe Jesper has some numbers. Maybe Intel folks do?

I just ran some quick tests with my setup and measured about 1.2% worst
case. Certainly not exhaustive. Perhaps Intel or Mellanox can provide
numbers for their high speed nics - e.g. ConnectX-6 and a saturated host.

> 
> I'm just allergic to situations when there is a decision made and 
> then months later patches are posted disregarding the decision, 
> without analysis on why that decision was wrong. And while the
> maintainer who made the decision is on vacation.
> 

stats is one of the many sensitive topics. I have been consistent in
defending the need to use existing APIs and tooling and not relying on
XDP program writers to add the relevant stats and then provide whatever
tool is needed to extract and print them. Standardization for
fundamental analysis tools.

Re: [PATCH net-next 03/21] ethtool, stats: introduce standard XDP statistics

[Saeed Mahameed]

On Wed, 2021-08-04 at 11:28 -0600, David Ahern wrote:
> On 8/4/21 10:44 AM, Jakub Kicinski wrote:
> > On Wed, 4 Aug 2021 10:17:56 -0600 David Ahern wrote:
> > > On 8/4/21 6:36 AM, Jakub Kicinski wrote:
> > > > > XDP is going to always be eBPF based ! why not just report
> > > > > such stats
> > > > > to a special BPF_MAP ? BPF stack can collect the stats from
> > > > > the driver
> > > > > and report them to this special MAP upon user request.  
> > > > Do you mean replacing the ethtool-netlink / rtnetlink etc. with
> > > > a new BPF_MAP? I don't think adding another category of uAPI
> > > > thru 
> > > > which netdevice stats are exposed would do much good :( Plus it
> > > > doesn't address the "yet another cacheline" concern.
> > > > 
> > > > To my understanding the need for stats recognizes the fact that
> > > > (in
> > > > large organizations) fleet monitoring is done by different
> > > > teams than
> > > > XDP development. So XDP team may have all the stats they need,
> > > > but the
> > > > team doing fleet monitoring has no idea how to get to them.
> > > > 
> > > > To bridge the two worlds we need a way for the infra team to
> > > > ask the
> > > > XDP for well-defined stats. Maybe we should take a page from
> > > > the BPF
> > > > iterators book and create a program type for bridging the two
> > > > worlds?
> > > > Called by networking core when duping stats to extract from the
> > > > existing BPF maps all the relevant stats and render them into a
> > > > well
> > > > known struct? Users' XDP design can still use a single per-cpu
> > > > map with
> > > > all the stats if they so choose, but there's a way to implement
> > > > more
> > > > optimal designs and still expose well-defined stats.
> > > > 
> > > > Maybe that's too complex, IDK.  
> > > 

The main question here, do we want the prog to count or driver ? 
and the answer will lead to more questions :) :

1) will the prog/user need to access driver for driver only stats ? or
driver shall report to a special program and all the collection and
reporting is done in XDP/BPF internally .. 
2) stats per prog/queue/cpu/interface ? 
3) how to eventually report to user ethtool/ip -s/bpftool ?

too complex, IDK too .. :D


> > > I was just explaining to someone internally how to get stats at
> > > all of
> > > the different points in the stack to track down reasons for
> > > dropped packets:
> > > 
> > > ethtool -S for h/w and driver
> > > tc -s for drops by the qdisc
> > > /proc/net/softnet_stat for drops at the backlog layer
> > > netstat -s for network and transport layer
> > > 
> > > yet another command and API just adds to the nightmare of
> > > explaining and
> > > understanding these stats.
> > 
> > Are you referring to RTM_GETSTATS when you say "yet another
> > command"?
> > RTM_GETSTATS exists and is used by offloads today.
> > 
> > I'd expect ip -s (-s) to be extended to run GETSTATS and display
> > the xdp
> > stats. (Not sure why ip -s was left out of your list :))
> 
> It's on my diagram, and yes, forgot to add it here.
> 

i think ip -s is a good place for "standard" driver based xdp stats.
but as Jakub already explained, adding such driver mechanism is like
making a statement that drivers must implement this.

> > 
> > > There is real value in continuing to use ethtool API for XDP
> > > stats. Not
> > > saying this reorg of the XDP stats is the right thing to do, only
> > > that
> > > the existing API has real user benefits.
> > 
> > RTM_GETSTATS is an existing API. New ethtool stats are intended to
> > be HW
> > stats. I don't want to go back to ethtool being a dumping ground
> > for all
> > stats because that's what the old interface encouraged.
> 
> driver stats are important too. e.g., mlx5's cache stats and per-
> queue
> stats.
> 

one could claim that mlx5 cache stats should move to page_pool and
per_queue stats should move to the stack.

> > 
> > > Does anyone have data that shows bumping a properly implemented
> > > counter
> > > causes a noticeable performance degradation and if so by how
> > > much? You
> > > mention 'yet another cacheline' but collecting stats on stack and
> > > incrementing the driver structs at the end of the napi loop
> > > should not
> > > have a huge impact versus the value the stats provide.
> > 
> > Not sure, maybe Jesper has some numbers. Maybe Intel folks do?
> 

A properly implemented counter that doesn't introduce new cache misses,
will hardly show any measurable difference, the only way to measure is
via instructions per packet.

usually the way we implement counters in mlx5 is that if this is the
fastest flow that we expect then we only increment the good counters
"packet++/drop++/redirect++" any slower path should include counters to
indicate the slower path and the effect of the new "slower" counters
will still be negligible as we already are at a higher instructions per
packet hence the slower path .. 

the only time you measure a difference is when you introduce new
counting on a counter-free flow, e.g page_pool ;)

> I just ran some quick tests with my setup and measured about 1.2%
> worst

1.2% is a lot ! what was the test ? what is the change ?

> case. Certainly not exhaustive. Perhaps Intel or Mellanox can provide
> numbers for their high speed nics - e.g. ConnectX-6 and a saturated
> host.
> 

let's define what are we testing first, there are multiple places we
need to check, Tariq will be exploring transitioning mlx5 cache to
page_pool with all the counters, maybe it is a good place to measure.. 

> > 
> > I'm just allergic to situations when there is a decision made and 
> > then months later patches are posted disregarding the decision, 
> > without analysis on why that decision was wrong. And while the
> > maintainer who made the decision is on vacation.
> > 
> 
> stats is one of the many sensitive topics. I have been consistent in
> defending the need to use existing APIs and tooling and not relying
> on
> XDP program writers to add the relevant stats and then provide
> whatever
> tool is needed to extract and print them. Standardization for
> fundamental analysis tools.

Re: [PATCH net-next 03/21] ethtool, stats: introduce standard XDP statistics

[David Ahern]

On 8/4/21 12:27 PM, Saeed Mahameed wrote:
> 
>> I just ran some quick tests with my setup and measured about 1.2%
>> worst
> 
> 1.2% is a lot ! what was the test ? what is the change ?

I did say "quick test ... not exhaustive" and it was definitely
eyeballing a pps change over a small time window.

If multiple counters are bumped 20-25 million times a second (e.g. XDP
drop case), how measurable is it? I was just trying to ballpark the
overhead - 1%, 5%, more? If it is <~ 1% then there is no performance
argument in which case let's do the right thing for users - export via
existing APIs.

Re: [PATCH net-next 03/21] ethtool, stats: introduce standard XDP statistics

[Saeed Mahameed]

On Wed, 2021-08-04 at 18:43 -0600, David Ahern wrote:
> On 8/4/21 12:27 PM, Saeed Mahameed wrote:
> > 
> > > I just ran some quick tests with my setup and measured about 1.2%
> > > worst
> > 
> > 1.2% is a lot ! what was the test ? what is the change ?
> 
> I did say "quick test ... not exhaustive" and it was definitely
> eyeballing a pps change over a small time window.
> 
> If multiple counters are bumped 20-25 million times a second (e.g.
> XDP
> drop case), how measurable is it? I was just trying to ballpark the
> overhead - 1%, 5%, more? If it is <~ 1% then there is no performance
> argument in which case let's do the right thing for users - export
> via
> existing APIs.

from experience I don't believe it can be more than 1% and yes on
existing APIs !

Re: [PATCH net-next 03/21] ethtool, stats: introduce standard XDP statistics

[Jesper Dangaard Brouer]

On 04/08/2021 18.44, Jakub Kicinski wrote:
> On Wed, 4 Aug 2021 10:17:56 -0600 David Ahern wrote:
>> On 8/4/21 6:36 AM, Jakub Kicinski wrote:
> 
>> Does anyone have data that shows bumping a properly implemented counter
>> causes a noticeable performance degradation and if so by how much? You
>> mention 'yet another cacheline' but collecting stats on stack and
>> incrementing the driver structs at the end of the napi loop should not
>> have a huge impact versus the value the stats provide.
> 
> Not sure, maybe Jesper has some numbers. Maybe Intel folks do?

(sorry, behind on emails after vacation ... just partly answering inside 
this thread, not checking if you did a smart counter impl.).

I don't have exact numbers, but I hope Magnus (Intel) would be motivated 
to validate performance degradation from this patchset.  As I know Intel 
is hunting the DPDK numbers with AF_XDP-zc, where every last cycle *do* 
count.

My experience is that counters can easily hurt performance, without the 
developers noticing the small degradation's.  As Ahern sketch out above 
(stats on stack + end of napi loop update), I do believe that a smart 
counter implementation is possible to hide this overhead (hopefully 
completely in the CPUs pipeline slots).

I do highly appreciate the effort to standardize the XDP stats!
So, I do hope this can somehow move forward.

--Jesper

Re: [PATCH net-next 03/21] ethtool, stats: introduce standard XDP statistics

[Alexander Lobakin]

From: Saeed Mahameed <saeed@kernel.org>
Date: Tue, 03 Aug 2021 16:57:22 -0700

[ snip ]

> XDP is going to always be eBPF based ! why not just report such stats
> to a special BPF_MAP ? BPF stack can collect the stats from the driver
> and report them to this special MAP upon user request.

I really dig this idea now. How do you see it?
<ifindex:channel:stat_id> as a key and its value as a value or ...?

[ snip ]

Thanks,
Al

Re: [PATCH net-next 03/21] ethtool, stats: introduce standard XDP statistics

[Alexander Lobakin]

From: Alexander Lobakin <alexandr.lobakin@intel.com>
Date: Tue, 26 Oct 2021 11:23:23 +0200

> From: Saeed Mahameed <saeed@kernel.org>
> Date: Tue, 03 Aug 2021 16:57:22 -0700
> 
> [ snip ]
> 
> > XDP is going to always be eBPF based ! why not just report such stats
> > to a special BPF_MAP ? BPF stack can collect the stats from the driver
> > and report them to this special MAP upon user request.
> 
> I really dig this idea now. How do you see it?
> <ifindex:channel:stat_id> as a key and its value as a value or ...?

Ideas, suggestions, anyone?

> [ snip ]
> 
> Thanks,
> Al

Thanks,
Al

Re: [PATCH net-next 03/21] ethtool, stats: introduce standard XDP statistics

[Toke Høiland-Jørgensen]

Alexander Lobakin <alexandr.lobakin@intel.com> writes:

> From: Alexander Lobakin <alexandr.lobakin@intel.com>
> Date: Tue, 26 Oct 2021 11:23:23 +0200
>
>> From: Saeed Mahameed <saeed@kernel.org>
>> Date: Tue, 03 Aug 2021 16:57:22 -0700
>> 
>> [ snip ]
>> 
>> > XDP is going to always be eBPF based ! why not just report such stats
>> > to a special BPF_MAP ? BPF stack can collect the stats from the driver
>> > and report them to this special MAP upon user request.
>> 
>> I really dig this idea now. How do you see it?
>> <ifindex:channel:stat_id> as a key and its value as a value or ...?
>
> Ideas, suggestions, anyone?

I don't like the idea of putting statistics in a map instead of the
regular statistics counters. Sure, for bespoke things people want to put
into their XDP programs, use a map, but for regular packet/byte
counters, update the regular counters so XDP isn't "invisible".

As Jesper pointed out, batching the updates so the global counters are
only updated once per NAPI cycle is the way to avoid a huge performance
overhead of this...

-Toke

Re: [PATCH net-next 03/21] ethtool, stats: introduce standard XDP statistics

[Alexander Lobakin]

From: Toke Høiland-Jørgensen <toke@redhat.com>
Date: Mon, 08 Nov 2021 12:37:54 +0100

> Alexander Lobakin <alexandr.lobakin@intel.com> writes:
> 
> > From: Alexander Lobakin <alexandr.lobakin@intel.com>
> > Date: Tue, 26 Oct 2021 11:23:23 +0200
> >
> >> From: Saeed Mahameed <saeed@kernel.org>
> >> Date: Tue, 03 Aug 2021 16:57:22 -0700
> >> 
> >> [ snip ]
> >> 
> >> > XDP is going to always be eBPF based ! why not just report such stats
> >> > to a special BPF_MAP ? BPF stack can collect the stats from the driver
> >> > and report them to this special MAP upon user request.
> >> 
> >> I really dig this idea now. How do you see it?
> >> <ifindex:channel:stat_id> as a key and its value as a value or ...?
> >
> > Ideas, suggestions, anyone?
> 
> I don't like the idea of putting statistics in a map instead of the
> regular statistics counters. Sure, for bespoke things people want to put
> into their XDP programs, use a map, but for regular packet/byte
> counters, update the regular counters so XDP isn't "invisible".

I wanted to provide an `ip link` command for getting these stats
from maps and printing them in a usual format as well, but seems
like that's an unneeded overcomplication of things since using
maps for "regular"/"generic" XDP stats really has no reason except
for "XDP means eBPF means maps".

> As Jesper pointed out, batching the updates so the global counters are
> only updated once per NAPI cycle is the way to avoid a huge performance
> overhead of this...

That's how I do things currently, seems to work just fine.

> -Toke

Thanks,
Al

Re: [PATCH net-next 03/21] ethtool, stats: introduce standard XDP statistics

[Toke Høiland-Jørgensen]

Alexander Lobakin <alexandr.lobakin@intel.com> writes:

> From: Toke Høiland-Jørgensen <toke@redhat.com>
> Date: Mon, 08 Nov 2021 12:37:54 +0100
>
>> Alexander Lobakin <alexandr.lobakin@intel.com> writes:
>> 
>> > From: Alexander Lobakin <alexandr.lobakin@intel.com>
>> > Date: Tue, 26 Oct 2021 11:23:23 +0200
>> >
>> >> From: Saeed Mahameed <saeed@kernel.org>
>> >> Date: Tue, 03 Aug 2021 16:57:22 -0700
>> >> 
>> >> [ snip ]
>> >> 
>> >> > XDP is going to always be eBPF based ! why not just report such stats
>> >> > to a special BPF_MAP ? BPF stack can collect the stats from the driver
>> >> > and report them to this special MAP upon user request.
>> >> 
>> >> I really dig this idea now. How do you see it?
>> >> <ifindex:channel:stat_id> as a key and its value as a value or ...?
>> >
>> > Ideas, suggestions, anyone?
>> 
>> I don't like the idea of putting statistics in a map instead of the
>> regular statistics counters. Sure, for bespoke things people want to put
>> into their XDP programs, use a map, but for regular packet/byte
>> counters, update the regular counters so XDP isn't "invisible".
>
> I wanted to provide an `ip link` command for getting these stats
> from maps and printing them in a usual format as well, but seems
> like that's an unneeded overcomplication of things since using
> maps for "regular"/"generic" XDP stats really has no reason except
> for "XDP means eBPF means maps".

Yeah, don't really see why it would have to: to me, one of the benefits
of XDP is being integrated closely with the kernel so we can have a
"fast path" *without* reinventing everything...

>> As Jesper pointed out, batching the updates so the global counters are
>> only updated once per NAPI cycle is the way to avoid a huge performance
>> overhead of this...
>
> That's how I do things currently, seems to work just fine.

Awesome!

-Toke

[PATCH net-next 04/21] ethernet, dpaa2: simplify per-channel Ethtool stats counting

[Alexander Lobakin]

Don't hardcode ARRAY_SIZE() - 1, just put a placeholder and take
offsetof() from it to hide unwanted statistics from Ethtool.
Will be handy for switching to standard XDP stats.

Signed-off-by: Alexander Lobakin <alexandr.lobakin@intel.com>
Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
---
 drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h     | 7 ++++++-
 drivers/net/ethernet/freescale/dpaa2/dpaa2-ethtool.c | 2 +-
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h
index cdb623d5f2c1..8cb57f103d6b 100644
--- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h
+++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h
@@ -382,10 +382,15 @@ struct dpaa2_eth_ch_stats {
 	__u64 xdp_tx;
 	__u64 xdp_tx_err;
 	__u64 xdp_redirect;
-	/* Must be last, does not show up in ethtool stats */
+	/* The rest of the structure does not show up in ethtool stats */
+	struct { } __eth_end;
+	/* Must be last */
 	__u64 frames;
 };
 
+#define DPAA2_ETH_NUM_CH_STATS		(offsetof(struct dpaa2_eth_ch_stats, \
+						  __eth_end) / sizeof(u64))
+
 /* Maximum number of queues associated with a DPNI */
 #define DPAA2_ETH_MAX_TCS		8
 #define DPAA2_ETH_MAX_RX_QUEUES_PER_TC	16
diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-ethtool.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-ethtool.c
index ad5e374eeccf..95ae83905458 100644
--- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-ethtool.c
+++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-ethtool.c
@@ -278,7 +278,7 @@ static void dpaa2_eth_get_ethtool_stats(struct net_device *net_dev,
 	/* Per-channel stats */
 	for (k = 0; k < priv->num_channels; k++) {
 		ch_stats = &priv->channel[k]->stats;
-		for (j = 0; j < sizeof(*ch_stats) / sizeof(__u64) - 1; j++)
+		for (j = 0; j < DPAA2_ETH_NUM_CH_STATS; j++)
 			*((__u64 *)data + i + j) += *((__u64 *)ch_stats + j);
 	}
 	i += j;
-- 
2.31.1

[PATCH net-next 05/21] ethernet, dpaa2: convert to standard XDP stats

[Alexander Lobakin]

DPAA2 driver has 4 XDP counters which align just fine with the
standard XDP stats. Convert the driver to use the new approach.

Note that those counters are stored per-channel, but originally
were being given to Ethtool as sums across all channels. This
change makes them per-channel in Ethtool as well by providing
a number of channels to the standard stats infra.

Signed-off-by: Alexander Lobakin <alexandr.lobakin@intel.com>
Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
---
 .../net/ethernet/freescale/dpaa2/dpaa2-eth.h  |  4 +--
 .../ethernet/freescale/dpaa2/dpaa2-ethtool.c  | 36 ++++++++++++++++---
 2 files changed, 34 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h
index 8cb57f103d6b..fbba28fbb527 100644
--- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h
+++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h
@@ -377,13 +377,13 @@ struct dpaa2_eth_ch_stats {
 	__u64 pull_err;
 	/* Number of CDANs; useful to estimate avg NAPI len */
 	__u64 cdan;
+	/* The rest of the structure does not show up in ethtool stats */
+	struct { } __eth_end;
 	/* XDP counters */
 	__u64 xdp_drop;
 	__u64 xdp_tx;
 	__u64 xdp_tx_err;
 	__u64 xdp_redirect;
-	/* The rest of the structure does not show up in ethtool stats */
-	struct { } __eth_end;
 	/* Must be last */
 	__u64 frames;
 };
diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-ethtool.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-ethtool.c
index 95ae83905458..6529fa35c532 100644
--- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-ethtool.c
+++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-ethtool.c
@@ -53,10 +53,6 @@ static char dpaa2_ethtool_extras[][ETH_GSTRING_LEN] = {
 	"[drv] dequeue portal busy",
 	"[drv] channel pull errors",
 	"[drv] cdan",
-	"[drv] xdp drop",
-	"[drv] xdp tx",
-	"[drv] xdp tx errors",
-	"[drv] xdp redirect",
 	/* FQ stats */
 	"[qbman] rx pending frames",
 	"[qbman] rx pending bytes",
@@ -317,6 +313,36 @@ static void dpaa2_eth_get_ethtool_stats(struct net_device *net_dev,
 		dpaa2_mac_get_ethtool_stats(priv->mac, data + i);
 }
 
+static int dpaa2_eth_get_std_stats_channels(struct net_device *net_dev,
+					    u32 sset)
+{
+	const struct dpaa2_eth_priv *priv = netdev_priv(net_dev);
+
+	switch (sset) {
+	case ETH_SS_STATS_XDP:
+		return priv->num_channels;
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static void dpaa2_eth_get_xdp_stats(struct net_device *net_dev,
+				    struct ethtool_xdp_stats *xdp_stats)
+{
+	const struct dpaa2_eth_priv *priv = netdev_priv(net_dev);
+	const struct dpaa2_eth_ch_stats *ch_stats;
+	u32 i;
+
+	for (i = 0; i < priv->num_channels; i++) {
+		ch_stats = &priv->channel[i]->stats;
+
+		xdp_stats[i].drop = ch_stats->xdp_drop;
+		xdp_stats[i].redirect = ch_stats->xdp_redirect;
+		xdp_stats[i].tx = ch_stats->xdp_tx;
+		xdp_stats[i].tx_errors = ch_stats->xdp_tx_err;
+	}
+}
+
 static int dpaa2_eth_prep_eth_rule(struct ethhdr *eth_value, struct ethhdr *eth_mask,
 				   void *key, void *mask, u64 *fields)
 {
@@ -836,4 +862,6 @@ const struct ethtool_ops dpaa2_ethtool_ops = {
 	.get_ts_info = dpaa2_eth_get_ts_info,
 	.get_tunable = dpaa2_eth_get_tunable,
 	.set_tunable = dpaa2_eth_set_tunable,
+	.get_std_stats_channels = dpaa2_eth_get_std_stats_channels,
+	.get_xdp_stats = dpaa2_eth_get_xdp_stats,
 };
-- 
2.31.1

[PATCH net-next 06/21] ethernet, ena: constify src and syncp args of ena_safe_update_stat()

[Alexander Lobakin]

src and syncp pointers are being read only, and thus can be const.

Signed-off-by: Alexander Lobakin <alexandr.lobakin@intel.com>
Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
---
 drivers/net/ethernet/amazon/ena/ena_ethtool.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/amazon/ena/ena_ethtool.c b/drivers/net/ethernet/amazon/ena/ena_ethtool.c
index 27dae632efcb..851a198cec82 100644
--- a/drivers/net/ethernet/amazon/ena/ena_ethtool.c
+++ b/drivers/net/ethernet/amazon/ena/ena_ethtool.c
@@ -113,8 +113,8 @@ static const struct ena_stats ena_stats_ena_com_strings[] = {
 #define ENA_STATS_ARRAY_ENI(adapter)	\
 	(ARRAY_SIZE(ena_stats_eni_strings) * (adapter)->eni_stats_supported)
 
-static void ena_safe_update_stat(u64 *src, u64 *dst,
-				 struct u64_stats_sync *syncp)
+static void ena_safe_update_stat(const u64 *src, u64 *dst,
+				 const struct u64_stats_sync *syncp)
 {
 	unsigned int start;
 
-- 
2.31.1

[PATCH net-next 07/21] ethernet, ena: convert to standard XDP stats

[Alexander Lobakin]

Its 6 XDP per-channel counters align just fine with the standard
stats.
Drop them from the custom Ethtool statistics and expose to the
standard stats infra instead.

Signed-off-by: Alexander Lobakin <alexandr.lobakin@intel.com>
Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
---
 drivers/net/ethernet/amazon/ena/ena_ethtool.c | 46 ++++++++++++++++---
 1 file changed, 40 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/amazon/ena/ena_ethtool.c b/drivers/net/ethernet/amazon/ena/ena_ethtool.c
index 851a198cec82..1b6563641575 100644
--- a/drivers/net/ethernet/amazon/ena/ena_ethtool.c
+++ b/drivers/net/ethernet/amazon/ena/ena_ethtool.c
@@ -90,12 +90,6 @@ static const struct ena_stats ena_stats_rx_strings[] = {
 	ENA_STAT_RX_ENTRY(bad_req_id),
 	ENA_STAT_RX_ENTRY(empty_rx_ring),
 	ENA_STAT_RX_ENTRY(csum_unchecked),
-	ENA_STAT_RX_ENTRY(xdp_aborted),
-	ENA_STAT_RX_ENTRY(xdp_drop),
-	ENA_STAT_RX_ENTRY(xdp_pass),
-	ENA_STAT_RX_ENTRY(xdp_tx),
-	ENA_STAT_RX_ENTRY(xdp_invalid),
-	ENA_STAT_RX_ENTRY(xdp_redirect),
 };
 
 static const struct ena_stats ena_stats_ena_com_strings[] = {
@@ -324,6 +318,44 @@ static void ena_get_ethtool_strings(struct net_device *netdev,
 	}
 }
 
+static int ena_get_std_stats_channels(struct net_device *netdev, u32 sset)
+{
+	const struct ena_adapter *adapter = netdev_priv(netdev);
+
+	switch (sset) {
+	case ETH_SS_STATS_XDP:
+		return adapter->num_io_queues;
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static void ena_get_xdp_stats(struct net_device *netdev,
+			      struct ethtool_xdp_stats *xdp_stats)
+{
+	const struct ena_adapter *adapter = netdev_priv(netdev);
+	const struct u64_stats_sync *syncp;
+	const struct ena_stats_rx *stats;
+	struct ethtool_xdp_stats *iter;
+	u32 i;
+
+	for (i = 0; i < adapter->num_io_queues; i++) {
+		stats = &adapter->rx_ring[i].rx_stats;
+		syncp = &adapter->rx_ring[i].syncp;
+		iter = xdp_stats + i;
+
+		ena_safe_update_stat(&stats->xdp_drop, &iter->drop, syncp);
+		ena_safe_update_stat(&stats->xdp_pass, &iter->pass, syncp);
+		ena_safe_update_stat(&stats->xdp_tx, &iter->tx, syncp);
+		ena_safe_update_stat(&stats->xdp_redirect, &iter->redirect,
+				     syncp);
+		ena_safe_update_stat(&stats->xdp_aborted, &iter->aborted,
+				     syncp);
+		ena_safe_update_stat(&stats->xdp_invalid, &iter->invalid,
+				     syncp);
+	}
+}
+
 static int ena_get_link_ksettings(struct net_device *netdev,
 				  struct ethtool_link_ksettings *link_ksettings)
 {
@@ -916,6 +948,8 @@ static const struct ethtool_ops ena_ethtool_ops = {
 	.get_tunable		= ena_get_tunable,
 	.set_tunable		= ena_set_tunable,
 	.get_ts_info            = ethtool_op_get_ts_info,
+	.get_std_stats_channels	= ena_get_std_stats_channels,
+	.get_xdp_stats		= ena_get_xdp_stats,
 };
 
 void ena_set_ethtool_ops(struct net_device *netdev)
-- 
2.31.1

Re: [PATCH net-next 07/21] ethernet, ena: convert to standard XDP stats

[Shay Agroskin]

Alexander Lobakin <alexandr.lobakin@intel.com> writes:

>
>
>
> Its 6 XDP per-channel counters align just fine with the standard
> stats.
> Drop them from the custom Ethtool statistics and expose to the
> standard stats infra instead.
>
> Signed-off-by: Alexander Lobakin <alexandr.lobakin@intel.com>
> Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
> ---
>  drivers/net/ethernet/amazon/ena/ena_ethtool.c | 46 
>  ++++++++++++++++---
>  1 file changed, 40 insertions(+), 6 deletions(-)

Hi,
thanks for making this patch. I like the idea of splitting stats 
into a per-queue basis

>
> diff --git a/drivers/net/ethernet/amazon/ena/ena_ethtool.c 
> b/drivers/net/ethernet/amazon/ena/ena_ethtool.c
> index 851a198cec82..1b6563641575 100644
> --- a/drivers/net/ethernet/amazon/ena/ena_ethtool.c
> +++ b/drivers/net/ethernet/amazon/ena/ena_ethtool.c
> @@ -90,12 +90,6 @@ static const struct ena_stats 
> ena_stats_rx_strings[] = {
>         ENA_STAT_RX_ENTRY(bad_req_id),
>         ENA_STAT_RX_ENTRY(empty_rx_ring),
>         ENA_STAT_RX_ENTRY(csum_unchecked),
> -       ENA_STAT_RX_ENTRY(xdp_aborted),
> -       ENA_STAT_RX_ENTRY(xdp_drop),
> -       ENA_STAT_RX_ENTRY(xdp_pass),
> -       ENA_STAT_RX_ENTRY(xdp_tx),
> -       ENA_STAT_RX_ENTRY(xdp_invalid),
> -       ENA_STAT_RX_ENTRY(xdp_redirect),
>

The ena_stats_rx_strings array is (indirectly) accessed through 
ena_get_stats() function which is used for both fetching ethtool 
stats and
for sharing the stats with the device in case of an error (through 
ena_dump_stats_ex() function).

The latter use is broken by removing the XDP specific stats from 
ena_stats_rx_strings array.

I can submit an adaptation for the new system later (similar to 
mlx5) if you prefer

thanks,
Shay

>  };
>
>  static const struct ena_stats ena_stats_ena_com_strings[] = {
> @@ -324,6 +318,44 @@ static void ena_get_ethtool_strings(struct 
> net_device *netdev,
>         }
>  }
>
> +static int ena_get_std_stats_channels(struct net_device 
> *netdev, u32 sset)
> +{
> +       const struct ena_adapter *adapter = netdev_priv(netdev);
> +
> +       switch (sset) {
> +       case ETH_SS_STATS_XDP:
> +               return adapter->num_io_queues;
> +       default:
> +               return -EOPNOTSUPP;
> +       }
> +}
> +
> +static void ena_get_xdp_stats(struct net_device *netdev,
> +                             struct ethtool_xdp_stats 
> *xdp_stats)
> +{
> +       const struct ena_adapter *adapter = netdev_priv(netdev);
> +       const struct u64_stats_sync *syncp;
> +       const struct ena_stats_rx *stats;
> +       struct ethtool_xdp_stats *iter;
> +       u32 i;
> +
...
>  {
> @@ -916,6 +948,8 @@ static const struct ethtool_ops 
> ena_ethtool_ops = {
>         .get_tunable            = ena_get_tunable,
>         .set_tunable            = ena_set_tunable,
>         .get_ts_info            = ethtool_op_get_ts_info,
> +       .get_std_stats_channels = ena_get_std_stats_channels,
> +       .get_xdp_stats          = ena_get_xdp_stats,
>  };
>
>  void ena_set_ethtool_ops(struct net_device *netdev)

Re: [PATCH net-next 07/21] ethernet, ena: convert to standard XDP stats

[Alexander Lobakin]

From: Shay Agroskin <shayagr@amazon.com>
Date: Wed, 4 Aug 2021 16:04:59 +0300

> Alexander Lobakin <alexandr.lobakin@intel.com> writes:
> 
> >
> >
> >
> > Its 6 XDP per-channel counters align just fine with the standard
> > stats.
> > Drop them from the custom Ethtool statistics and expose to the
> > standard stats infra instead.
> >
> > Signed-off-by: Alexander Lobakin <alexandr.lobakin@intel.com>
> > Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
> > ---
> >  drivers/net/ethernet/amazon/ena/ena_ethtool.c | 46 
> >  ++++++++++++++++---
> >  1 file changed, 40 insertions(+), 6 deletions(-)
> 
> Hi,
> thanks for making this patch. I like the idea of splitting stats 
> into a per-queue basis
> 
> >
> > diff --git a/drivers/net/ethernet/amazon/ena/ena_ethtool.c 
> > b/drivers/net/ethernet/amazon/ena/ena_ethtool.c
> > index 851a198cec82..1b6563641575 100644
> > --- a/drivers/net/ethernet/amazon/ena/ena_ethtool.c
> > +++ b/drivers/net/ethernet/amazon/ena/ena_ethtool.c
> > @@ -90,12 +90,6 @@ static const struct ena_stats 
> > ena_stats_rx_strings[] = {
> >         ENA_STAT_RX_ENTRY(bad_req_id),
> >         ENA_STAT_RX_ENTRY(empty_rx_ring),
> >         ENA_STAT_RX_ENTRY(csum_unchecked),
> > -       ENA_STAT_RX_ENTRY(xdp_aborted),
> > -       ENA_STAT_RX_ENTRY(xdp_drop),
> > -       ENA_STAT_RX_ENTRY(xdp_pass),
> > -       ENA_STAT_RX_ENTRY(xdp_tx),
> > -       ENA_STAT_RX_ENTRY(xdp_invalid),
> > -       ENA_STAT_RX_ENTRY(xdp_redirect),
> >
> 
> The ena_stats_rx_strings array is (indirectly) accessed through 
> ena_get_stats() function which is used for both fetching ethtool 
> stats and
> for sharing the stats with the device in case of an error (through 
> ena_dump_stats_ex() function).
> 
> The latter use is broken by removing the XDP specific stats from 
> ena_stats_rx_strings array.
> 
> I can submit an adaptation for the new system later (similar to 
> mlx5) if you prefer

Feel free to either do that (I'll exclude this patch from that
series then) or you can give me some little tips or examples or
anything on how to improve this one, so ena would stay converted.
Both ways are fine for me.

> thanks,
> Shay

Thanks,
Al

> >  };
> >
> >  static const struct ena_stats ena_stats_ena_com_strings[] = {
> > @@ -324,6 +318,44 @@ static void ena_get_ethtool_strings(struct 
> > net_device *netdev,
> >         }
> >  }
> >
> > +static int ena_get_std_stats_channels(struct net_device 
> > *netdev, u32 sset)
> > +{
> > +       const struct ena_adapter *adapter = netdev_priv(netdev);
> > +
> > +       switch (sset) {
> > +       case ETH_SS_STATS_XDP:
> > +               return adapter->num_io_queues;
> > +       default:
> > +               return -EOPNOTSUPP;
> > +       }
> > +}
> > +
> > +static void ena_get_xdp_stats(struct net_device *netdev,
> > +                             struct ethtool_xdp_stats 
> > *xdp_stats)
> > +{
> > +       const struct ena_adapter *adapter = netdev_priv(netdev);
> > +       const struct u64_stats_sync *syncp;
> > +       const struct ena_stats_rx *stats;
> > +       struct ethtool_xdp_stats *iter;
> > +       u32 i;
> > +
> ...
> >  {
> > @@ -916,6 +948,8 @@ static const struct ethtool_ops 
> > ena_ethtool_ops = {
> >         .get_tunable            = ena_get_tunable,
> >         .set_tunable            = ena_set_tunable,
> >         .get_ts_info            = ethtool_op_get_ts_info,
> > +       .get_std_stats_channels = ena_get_std_stats_channels,
> > +       .get_xdp_stats          = ena_get_xdp_stats,
> >  };
> >
> >  void ena_set_ethtool_ops(struct net_device *netdev)
> 
> 

[PATCH net-next 08/21] ethernet, enetc: convert to standard XDP stats

[Alexander Lobakin]

This driver has 6 per-channel XDP counters. Convert them to 5
standard XDP stats (redirect_sg and redirect_failures go under
the same redirect_errors).
As this driver theoretically can have different numbers of RX
and TX rings, collect statistics separately for each direction.

Signed-off-by: Alexander Lobakin <alexandr.lobakin@intel.com>
Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
---
 .../ethernet/freescale/enetc/enetc_ethtool.c  | 58 ++++++++++++++-----
 1 file changed, 44 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c b/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c
index ebccaf02411c..1b94cb488850 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c
@@ -192,18 +192,12 @@ static const struct {
 static const char rx_ring_stats[][ETH_GSTRING_LEN] = {
 	"Rx ring %2d frames",
 	"Rx ring %2d alloc errors",
-	"Rx ring %2d XDP drops",
 	"Rx ring %2d recycles",
 	"Rx ring %2d recycle failures",
-	"Rx ring %2d redirects",
-	"Rx ring %2d redirect failures",
-	"Rx ring %2d redirect S/G",
 };
 
 static const char tx_ring_stats[][ETH_GSTRING_LEN] = {
 	"Tx ring %2d frames",
-	"Tx ring %2d XDP frames",
-	"Tx ring %2d XDP drops",
 };
 
 static int enetc_get_sset_count(struct net_device *ndev, int sset)
@@ -275,21 +269,14 @@ static void enetc_get_ethtool_stats(struct net_device *ndev,
 	for (i = 0; i < ARRAY_SIZE(enetc_si_counters); i++)
 		data[o++] = enetc_rd64(hw, enetc_si_counters[i].reg);
 
-	for (i = 0; i < priv->num_tx_rings; i++) {
+	for (i = 0; i < priv->num_tx_rings; i++)
 		data[o++] = priv->tx_ring[i]->stats.packets;
-		data[o++] = priv->tx_ring[i]->stats.xdp_tx;
-		data[o++] = priv->tx_ring[i]->stats.xdp_tx_drops;
-	}
 
 	for (i = 0; i < priv->num_rx_rings; i++) {
 		data[o++] = priv->rx_ring[i]->stats.packets;
 		data[o++] = priv->rx_ring[i]->stats.rx_alloc_errs;
-		data[o++] = priv->rx_ring[i]->stats.xdp_drops;
 		data[o++] = priv->rx_ring[i]->stats.recycles;
 		data[o++] = priv->rx_ring[i]->stats.recycle_failures;
-		data[o++] = priv->rx_ring[i]->stats.xdp_redirect;
-		data[o++] = priv->rx_ring[i]->stats.xdp_redirect_failures;
-		data[o++] = priv->rx_ring[i]->stats.xdp_redirect_sg;
 	}
 
 	if (!enetc_si_is_pf(priv->si))
@@ -299,6 +286,45 @@ static void enetc_get_ethtool_stats(struct net_device *ndev,
 		data[o++] = enetc_port_rd(hw, enetc_port_counters[i].reg);
 }
 
+static int enetc_get_std_stats_channels(struct net_device *ndev, u32 sset)
+{
+	const struct enetc_ndev_priv *priv = netdev_priv(ndev);
+
+	switch (sset) {
+	case ETH_SS_STATS_XDP:
+		return max(priv->num_rx_rings, priv->num_tx_rings);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static void enetc_get_xdp_stats(struct net_device *ndev,
+				struct ethtool_xdp_stats *xdp_stats)
+{
+	const struct enetc_ndev_priv *priv = netdev_priv(ndev);
+	const struct enetc_ring_stats *stats;
+	struct ethtool_xdp_stats *iter;
+	u32 i;
+
+	for (i = 0; i < priv->num_tx_rings; i++) {
+		stats = &priv->tx_ring[i]->stats;
+		iter = xdp_stats + i;
+
+		iter->tx = stats->xdp_tx;
+		iter->tx_errors = stats->xdp_tx_drops;
+	}
+
+	for (i = 0; i < priv->num_rx_rings; i++) {
+		stats = &priv->rx_ring[i]->stats;
+		iter = xdp_stats + i;
+
+		iter->drop = stats->xdp_drops;
+		iter->redirect = stats->xdp_redirect;
+		iter->redirect_errors = stats->xdp_redirect_failures;
+		iter->redirect_errors += stats->xdp_redirect_sg;
+	}
+}
+
 #define ENETC_RSSHASH_L3 (RXH_L2DA | RXH_VLAN | RXH_L3_PROTO | RXH_IP_SRC | \
 			  RXH_IP_DST)
 #define ENETC_RSSHASH_L4 (ENETC_RSSHASH_L3 | RXH_L4_B_0_1 | RXH_L4_B_2_3)
@@ -772,6 +798,8 @@ static const struct ethtool_ops enetc_pf_ethtool_ops = {
 	.set_wol = enetc_set_wol,
 	.get_pauseparam = enetc_get_pauseparam,
 	.set_pauseparam = enetc_set_pauseparam,
+	.get_std_stats_channels = enetc_get_std_stats_channels,
+	.get_xdp_stats = enetc_get_xdp_stats,
 };
 
 static const struct ethtool_ops enetc_vf_ethtool_ops = {
@@ -793,6 +821,8 @@ static const struct ethtool_ops enetc_vf_ethtool_ops = {
 	.set_coalesce = enetc_set_coalesce,
 	.get_link = ethtool_op_get_link,
 	.get_ts_info = enetc_get_ts_info,
+	.get_std_stats_channels = enetc_get_std_stats_channels,
+	.get_xdp_stats = enetc_get_xdp_stats,
 };
 
 void enetc_set_ethtool_ops(struct net_device *ndev)
-- 
2.31.1

[PATCH net-next 09/21] ethernet, mvneta: rename xdp_xmit_err to xdp_xmit_drops

[Alexander Lobakin]

NETA driver also doesn't separate XDP xmit errors from drops, and in
that case more general "drops" should be used.
Rename the field before converting to standard XDP stats infra.

Signed-off-by: Alexander Lobakin <alexandr.lobakin@intel.com>
Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
---
 drivers/net/ethernet/marvell/mvneta.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index ff8db311963c..f030d5b7bdee 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -351,7 +351,7 @@ enum {
 	ETHTOOL_XDP_TX,
 	ETHTOOL_XDP_TX_ERR,
 	ETHTOOL_XDP_XMIT,
-	ETHTOOL_XDP_XMIT_ERR,
+	ETHTOOL_XDP_XMIT_DROPS,
 	ETHTOOL_MAX_STATS,
 };
 
@@ -412,7 +412,7 @@ static const struct mvneta_statistic mvneta_statistics[] = {
 	{ ETHTOOL_XDP_TX, T_SW, "rx_xdp_tx", },
 	{ ETHTOOL_XDP_TX_ERR, T_SW, "rx_xdp_tx_errors", },
 	{ ETHTOOL_XDP_XMIT, T_SW, "tx_xdp_xmit", },
-	{ ETHTOOL_XDP_XMIT_ERR, T_SW, "tx_xdp_xmit_errors", },
+	{ ETHTOOL_XDP_XMIT_DROPS, T_SW, "tx_xdp_xmit_drops", },
 };
 
 struct mvneta_stats {
@@ -425,7 +425,7 @@ struct mvneta_stats {
 	u64	xdp_pass;
 	u64	xdp_drop;
 	u64	xdp_xmit;
-	u64	xdp_xmit_err;
+	u64	xdp_xmit_drops;
 	u64	xdp_tx;
 	u64	xdp_tx_err;
 };
@@ -2166,7 +2166,7 @@ mvneta_xdp_xmit(struct net_device *dev, int num_frame,
 	stats->es.ps.tx_bytes += nxmit_byte;
 	stats->es.ps.tx_packets += nxmit;
 	stats->es.ps.xdp_xmit += nxmit;
-	stats->es.ps.xdp_xmit_err += num_frame - nxmit;
+	stats->es.ps.xdp_xmit_drops += num_frame - nxmit;
 	u64_stats_update_end(&stats->syncp);
 
 	return nxmit;
@@ -4630,9 +4630,9 @@ mvneta_ethtool_update_pcpu_stats(struct mvneta_port *pp,
 	for_each_possible_cpu(cpu) {
 		struct mvneta_pcpu_stats *stats;
 		u64 skb_alloc_error;
+		u64 xdp_xmit_drops;
 		u64 refill_error;
 		u64 xdp_redirect;
-		u64 xdp_xmit_err;
 		u64 xdp_tx_err;
 		u64 xdp_pass;
 		u64 xdp_drop;
@@ -4648,7 +4648,7 @@ mvneta_ethtool_update_pcpu_stats(struct mvneta_port *pp,
 			xdp_pass = stats->es.ps.xdp_pass;
 			xdp_drop = stats->es.ps.xdp_drop;
 			xdp_xmit = stats->es.ps.xdp_xmit;
-			xdp_xmit_err = stats->es.ps.xdp_xmit_err;
+			xdp_xmit_drops = stats->es.ps.xdp_xmit_drops;
 			xdp_tx = stats->es.ps.xdp_tx;
 			xdp_tx_err = stats->es.ps.xdp_tx_err;
 		} while (u64_stats_fetch_retry_irq(&stats->syncp, start));
@@ -4659,7 +4659,7 @@ mvneta_ethtool_update_pcpu_stats(struct mvneta_port *pp,
 		es->ps.xdp_pass += xdp_pass;
 		es->ps.xdp_drop += xdp_drop;
 		es->ps.xdp_xmit += xdp_xmit;
-		es->ps.xdp_xmit_err += xdp_xmit_err;
+		es->ps.xdp_xmit_drops += xdp_xmit_drops;
 		es->ps.xdp_tx += xdp_tx;
 		es->ps.xdp_tx_err += xdp_tx_err;
 	}
@@ -4720,8 +4720,8 @@ static void mvneta_ethtool_update_stats(struct mvneta_port *pp)
 			case ETHTOOL_XDP_XMIT:
 				pp->ethtool_stats[i] = stats.ps.xdp_xmit;
 				break;
-			case ETHTOOL_XDP_XMIT_ERR:
-				pp->ethtool_stats[i] = stats.ps.xdp_xmit_err;
+			case ETHTOOL_XDP_XMIT_DROPS:
+				pp->ethtool_stats[i] = stats.ps.xdp_xmit_drops;
 				break;
 			}
 			break;
-- 
2.31.1

[PATCH net-next 10/21] ethernet, mvneta: convert to standard XDP stats

[Alexander Lobakin]

Replace custom Ethtools XDP statistics with the standard infra based
one (7 basic fields).
This driver uses global [per-cpu] stats, no other callbacks needed.

Signed-off-by: Alexander Lobakin <alexandr.lobakin@intel.com>
Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
---
 drivers/net/ethernet/marvell/mvneta.c | 108 +++++++++++++-------------
 1 file changed, 52 insertions(+), 56 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index f030d5b7bdee..9015c8b45395 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -345,13 +345,6 @@ enum {
 	ETHTOOL_STAT_EEE_WAKEUP,
 	ETHTOOL_STAT_SKB_ALLOC_ERR,
 	ETHTOOL_STAT_REFILL_ERR,
-	ETHTOOL_XDP_REDIRECT,
-	ETHTOOL_XDP_PASS,
-	ETHTOOL_XDP_DROP,
-	ETHTOOL_XDP_TX,
-	ETHTOOL_XDP_TX_ERR,
-	ETHTOOL_XDP_XMIT,
-	ETHTOOL_XDP_XMIT_DROPS,
 	ETHTOOL_MAX_STATS,
 };
 
@@ -406,13 +399,6 @@ static const struct mvneta_statistic mvneta_statistics[] = {
 	{ ETHTOOL_STAT_EEE_WAKEUP, T_SW, "eee_wakeup_errors", },
 	{ ETHTOOL_STAT_SKB_ALLOC_ERR, T_SW, "skb_alloc_errors", },
 	{ ETHTOOL_STAT_REFILL_ERR, T_SW, "refill_errors", },
-	{ ETHTOOL_XDP_REDIRECT, T_SW, "rx_xdp_redirect", },
-	{ ETHTOOL_XDP_PASS, T_SW, "rx_xdp_pass", },
-	{ ETHTOOL_XDP_DROP, T_SW, "rx_xdp_drop", },
-	{ ETHTOOL_XDP_TX, T_SW, "rx_xdp_tx", },
-	{ ETHTOOL_XDP_TX_ERR, T_SW, "rx_xdp_tx_errors", },
-	{ ETHTOOL_XDP_XMIT, T_SW, "tx_xdp_xmit", },
-	{ ETHTOOL_XDP_XMIT_DROPS, T_SW, "tx_xdp_xmit_drops", },
 };
 
 struct mvneta_stats {
@@ -4630,38 +4616,17 @@ mvneta_ethtool_update_pcpu_stats(struct mvneta_port *pp,
 	for_each_possible_cpu(cpu) {
 		struct mvneta_pcpu_stats *stats;
 		u64 skb_alloc_error;
-		u64 xdp_xmit_drops;
 		u64 refill_error;
-		u64 xdp_redirect;
-		u64 xdp_tx_err;
-		u64 xdp_pass;
-		u64 xdp_drop;
-		u64 xdp_xmit;
-		u64 xdp_tx;
 
 		stats = per_cpu_ptr(pp->stats, cpu);
 		do {
 			start = u64_stats_fetch_begin_irq(&stats->syncp);
 			skb_alloc_error = stats->es.skb_alloc_error;
 			refill_error = stats->es.refill_error;
-			xdp_redirect = stats->es.ps.xdp_redirect;
-			xdp_pass = stats->es.ps.xdp_pass;
-			xdp_drop = stats->es.ps.xdp_drop;
-			xdp_xmit = stats->es.ps.xdp_xmit;
-			xdp_xmit_drops = stats->es.ps.xdp_xmit_drops;
-			xdp_tx = stats->es.ps.xdp_tx;
-			xdp_tx_err = stats->es.ps.xdp_tx_err;
 		} while (u64_stats_fetch_retry_irq(&stats->syncp, start));
 
 		es->skb_alloc_error += skb_alloc_error;
 		es->refill_error += refill_error;
-		es->ps.xdp_redirect += xdp_redirect;
-		es->ps.xdp_pass += xdp_pass;
-		es->ps.xdp_drop += xdp_drop;
-		es->ps.xdp_xmit += xdp_xmit;
-		es->ps.xdp_xmit_drops += xdp_xmit_drops;
-		es->ps.xdp_tx += xdp_tx;
-		es->ps.xdp_tx_err += xdp_tx_err;
 	}
 }
 
@@ -4702,27 +4667,6 @@ static void mvneta_ethtool_update_stats(struct mvneta_port *pp)
 			case ETHTOOL_STAT_REFILL_ERR:
 				pp->ethtool_stats[i] = stats.refill_error;
 				break;
-			case ETHTOOL_XDP_REDIRECT:
-				pp->ethtool_stats[i] = stats.ps.xdp_redirect;
-				break;
-			case ETHTOOL_XDP_PASS:
-				pp->ethtool_stats[i] = stats.ps.xdp_pass;
-				break;
-			case ETHTOOL_XDP_DROP:
-				pp->ethtool_stats[i] = stats.ps.xdp_drop;
-				break;
-			case ETHTOOL_XDP_TX:
-				pp->ethtool_stats[i] = stats.ps.xdp_tx;
-				break;
-			case ETHTOOL_XDP_TX_ERR:
-				pp->ethtool_stats[i] = stats.ps.xdp_tx_err;
-				break;
-			case ETHTOOL_XDP_XMIT:
-				pp->ethtool_stats[i] = stats.ps.xdp_xmit;
-				break;
-			case ETHTOOL_XDP_XMIT_DROPS:
-				pp->ethtool_stats[i] = stats.ps.xdp_xmit_drops;
-				break;
 			}
 			break;
 		}
@@ -4748,6 +4692,57 @@ static int mvneta_ethtool_get_sset_count(struct net_device *dev, int sset)
 	return -EOPNOTSUPP;
 }
 
+static void mvneta_ethtool_get_xdp_stats(struct net_device *dev,
+					 struct ethtool_xdp_stats *xdp_stats)
+{
+	const struct mvneta_port *pp = netdev_priv(dev);
+	u32 cpu;
+
+	xdp_stats->drop = 0;
+	xdp_stats->pass = 0;
+	xdp_stats->redirect = 0;
+	xdp_stats->tx = 0;
+	xdp_stats->tx_errors = 0;
+	xdp_stats->xmit = 0;
+	xdp_stats->xmit_drops = 0;
+
+	for_each_possible_cpu(cpu) {
+		const struct mvneta_pcpu_stats *stats;
+		const struct mvneta_stats *ps;
+		u64 xdp_xmit_drops;
+		u64 xdp_redirect;
+		u64 xdp_tx_err;
+		u64 xdp_pass;
+		u64 xdp_drop;
+		u64 xdp_xmit;
+		u64 xdp_tx;
+		u32 start;
+
+		stats = per_cpu_ptr(pp->stats, cpu);
+		ps = &stats->es.ps;
+
+		do {
+			start = u64_stats_fetch_begin_irq(&stats->syncp);
+
+			xdp_drop = ps->xdp_drop;
+			xdp_pass = ps->xdp_pass;
+			xdp_redirect = ps->xdp_redirect;
+			xdp_tx = ps->xdp_tx;
+			xdp_tx_err = ps->xdp_tx_err;
+			xdp_xmit = ps->xdp_xmit;
+			xdp_xmit_drops = ps->xdp_xmit_drops;
+		} while (u64_stats_fetch_retry_irq(&stats->syncp, start));
+
+		xdp_stats->drop += xdp_drop;
+		xdp_stats->pass += xdp_pass;
+		xdp_stats->redirect += xdp_redirect;
+		xdp_stats->tx += xdp_tx;
+		xdp_stats->tx_errors += xdp_tx_err;
+		xdp_stats->xmit += xdp_xmit;
+		xdp_stats->xmit_drops += xdp_xmit_drops;
+	}
+}
+
 static u32 mvneta_ethtool_get_rxfh_indir_size(struct net_device *dev)
 {
 	return MVNETA_RSS_LU_TABLE_SIZE;
@@ -5025,6 +5020,7 @@ static const struct ethtool_ops mvneta_eth_tool_ops = {
 	.set_wol        = mvneta_ethtool_set_wol,
 	.get_eee	= mvneta_ethtool_get_eee,
 	.set_eee	= mvneta_ethtool_set_eee,
+	.get_xdp_stats	= mvneta_ethtool_get_xdp_stats,
 };
 
 /* Initialize hw */
-- 
2.31.1

[PATCH net-next 11/21] ethernet, mvpp2: rename xdp_xmit_err to xdp_xmit_drops

[Alexander Lobakin]

xdp_xmit_err stat is defined as num_frames - nxmit in
.ndo_xdp_xmit() callback implementation, and regarding that the
frames which weren't transmitted are treated as drops by BPF core,
give it a more fitting name in preparation for switching to standard
XDP stats.

Signed-off-by: Alexander Lobakin <alexandr.lobakin@intel.com>
Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
---
 drivers/net/ethernet/marvell/mvpp2/mvpp2.h      |  2 +-
 drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 16 ++++++++--------
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2.h b/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
index b9fbc9f000f2..2be6123be6f3 100644
--- a/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
+++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
@@ -1121,7 +1121,7 @@ struct mvpp2_pcpu_stats {
 	u64	xdp_pass;
 	u64	xdp_drop;
 	u64	xdp_xmit;
-	u64	xdp_xmit_err;
+	u64	xdp_xmit_drops;
 	u64	xdp_tx;
 	u64	xdp_tx_err;
 };
diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
index 99bd8b8aa0e2..a3aee1a6d760 100644
--- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
+++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
@@ -1831,7 +1831,7 @@ enum {
 	ETHTOOL_XDP_TX,
 	ETHTOOL_XDP_TX_ERR,
 	ETHTOOL_XDP_XMIT,
-	ETHTOOL_XDP_XMIT_ERR,
+	ETHTOOL_XDP_XMIT_DROPS,
 };
 
 struct mvpp2_ethtool_counter {
@@ -1933,7 +1933,7 @@ static const struct mvpp2_ethtool_counter mvpp2_ethtool_xdp[] = {
 	{ ETHTOOL_XDP_TX, "rx_xdp_tx", },
 	{ ETHTOOL_XDP_TX_ERR, "rx_xdp_tx_errors", },
 	{ ETHTOOL_XDP_XMIT, "tx_xdp_xmit", },
-	{ ETHTOOL_XDP_XMIT_ERR, "tx_xdp_xmit_errors", },
+	{ ETHTOOL_XDP_XMIT_DROPS, "tx_xdp_xmit_drops", },
 };
 
 #define MVPP2_N_ETHTOOL_STATS(ntxqs, nrxqs)	(ARRAY_SIZE(mvpp2_ethtool_mib_regs) + \
@@ -2000,7 +2000,7 @@ mvpp2_get_xdp_stats(struct mvpp2_port *port, struct mvpp2_pcpu_stats *xdp_stats)
 		u64	xdp_pass;
 		u64	xdp_drop;
 		u64	xdp_xmit;
-		u64	xdp_xmit_err;
+		u64	xdp_xmit_drops;
 		u64	xdp_tx;
 		u64	xdp_tx_err;
 
@@ -2011,7 +2011,7 @@ mvpp2_get_xdp_stats(struct mvpp2_port *port, struct mvpp2_pcpu_stats *xdp_stats)
 			xdp_pass   = cpu_stats->xdp_pass;
 			xdp_drop = cpu_stats->xdp_drop;
 			xdp_xmit   = cpu_stats->xdp_xmit;
-			xdp_xmit_err   = cpu_stats->xdp_xmit_err;
+			xdp_xmit_drops = cpu_stats->xdp_xmit_drops;
 			xdp_tx   = cpu_stats->xdp_tx;
 			xdp_tx_err   = cpu_stats->xdp_tx_err;
 		} while (u64_stats_fetch_retry_irq(&cpu_stats->syncp, start));
@@ -2020,7 +2020,7 @@ mvpp2_get_xdp_stats(struct mvpp2_port *port, struct mvpp2_pcpu_stats *xdp_stats)
 		xdp_stats->xdp_pass   += xdp_pass;
 		xdp_stats->xdp_drop += xdp_drop;
 		xdp_stats->xdp_xmit   += xdp_xmit;
-		xdp_stats->xdp_xmit_err   += xdp_xmit_err;
+		xdp_stats->xdp_xmit_drops += xdp_xmit_drops;
 		xdp_stats->xdp_tx   += xdp_tx;
 		xdp_stats->xdp_tx_err   += xdp_tx_err;
 	}
@@ -2083,8 +2083,8 @@ static void mvpp2_read_stats(struct mvpp2_port *port)
 		case ETHTOOL_XDP_XMIT:
 			*pstats++ = xdp_stats.xdp_xmit;
 			break;
-		case ETHTOOL_XDP_XMIT_ERR:
-			*pstats++ = xdp_stats.xdp_xmit_err;
+		case ETHTOOL_XDP_XMIT_DROPS:
+			*pstats++ = xdp_stats.xdp_xmit_drops;
 			break;
 		}
 	}
@@ -3773,7 +3773,7 @@ mvpp2_xdp_xmit(struct net_device *dev, int num_frame,
 	stats->tx_bytes += nxmit_byte;
 	stats->tx_packets += nxmit;
 	stats->xdp_xmit += nxmit;
-	stats->xdp_xmit_err += num_frame - nxmit;
+	stats->xdp_xmit_drops += num_frame - nxmit;
 	u64_stats_update_end(&stats->syncp);
 
 	return nxmit;
-- 
2.31.1

[PATCH net-next 12/21] ethernet, mvpp2: convert to standard XDP stats

[Alexander Lobakin]

Convert PPv2 driver to provide standard XDP statistics instead of
custom-defined Ethtool stats. This also allows to greatly simplify
stats filling code.
In the same fashion as mvneta, the driver uses global XDP counters.

Signed-off-by: Alexander Lobakin <alexandr.lobakin@intel.com>
Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
---
 .../net/ethernet/marvell/mvpp2/mvpp2_main.c   | 90 +++++--------------
 1 file changed, 20 insertions(+), 70 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
index a3aee1a6d760..ed34f8fefced 100644
--- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
+++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
@@ -1824,16 +1824,6 @@ static void mvpp2_port_loopback_set(struct mvpp2_port *port,
 	writel(val, port->base + MVPP2_GMAC_CTRL_1_REG);
 }
 
-enum {
-	ETHTOOL_XDP_REDIRECT,
-	ETHTOOL_XDP_PASS,
-	ETHTOOL_XDP_DROP,
-	ETHTOOL_XDP_TX,
-	ETHTOOL_XDP_TX_ERR,
-	ETHTOOL_XDP_XMIT,
-	ETHTOOL_XDP_XMIT_DROPS,
-};
-
 struct mvpp2_ethtool_counter {
 	unsigned int offset;
 	const char string[ETH_GSTRING_LEN];
@@ -1926,21 +1916,10 @@ static const struct mvpp2_ethtool_counter mvpp2_ethtool_rxq_regs[] = {
 	{ MVPP2_RX_PKTS_BM_DROP_CTR, "rxq_%d_packets_bm_drops" },
 };
 
-static const struct mvpp2_ethtool_counter mvpp2_ethtool_xdp[] = {
-	{ ETHTOOL_XDP_REDIRECT, "rx_xdp_redirect", },
-	{ ETHTOOL_XDP_PASS, "rx_xdp_pass", },
-	{ ETHTOOL_XDP_DROP, "rx_xdp_drop", },
-	{ ETHTOOL_XDP_TX, "rx_xdp_tx", },
-	{ ETHTOOL_XDP_TX_ERR, "rx_xdp_tx_errors", },
-	{ ETHTOOL_XDP_XMIT, "tx_xdp_xmit", },
-	{ ETHTOOL_XDP_XMIT_DROPS, "tx_xdp_xmit_drops", },
-};
-
 #define MVPP2_N_ETHTOOL_STATS(ntxqs, nrxqs)	(ARRAY_SIZE(mvpp2_ethtool_mib_regs) + \
 						 ARRAY_SIZE(mvpp2_ethtool_port_regs) + \
 						 (ARRAY_SIZE(mvpp2_ethtool_txq_regs) * (ntxqs)) + \
-						 (ARRAY_SIZE(mvpp2_ethtool_rxq_regs) * (nrxqs)) + \
-						 ARRAY_SIZE(mvpp2_ethtool_xdp))
+						 (ARRAY_SIZE(mvpp2_ethtool_rxq_regs) * (nrxqs)))
 
 static void mvpp2_ethtool_get_strings(struct net_device *netdev, u32 sset,
 				      u8 *data)
@@ -1979,20 +1958,23 @@ static void mvpp2_ethtool_get_strings(struct net_device *netdev, u32 sset,
 			data += ETH_GSTRING_LEN;
 		}
 	}
-
-	for (i = 0; i < ARRAY_SIZE(mvpp2_ethtool_xdp); i++) {
-		strscpy(data, mvpp2_ethtool_xdp[i].string,
-			ETH_GSTRING_LEN);
-		data += ETH_GSTRING_LEN;
-	}
 }
 
-static void
-mvpp2_get_xdp_stats(struct mvpp2_port *port, struct mvpp2_pcpu_stats *xdp_stats)
+static void mvpp2_ethtool_get_xdp_stats(struct net_device *dev,
+					struct ethtool_xdp_stats *xdp_stats)
 {
+	const struct mvpp2_port *port = netdev_priv(dev);
 	unsigned int start;
 	unsigned int cpu;
 
+	xdp_stats->redirect = 0;
+	xdp_stats->pass = 0;
+	xdp_stats->drop = 0;
+	xdp_stats->xmit = 0;
+	xdp_stats->xmit_drops = 0;
+	xdp_stats->tx = 0;
+	xdp_stats->tx_errors = 0;
+
 	/* Gather XDP Statistics */
 	for_each_possible_cpu(cpu) {
 		struct mvpp2_pcpu_stats *cpu_stats;
@@ -2016,20 +1998,18 @@ mvpp2_get_xdp_stats(struct mvpp2_port *port, struct mvpp2_pcpu_stats *xdp_stats)
 			xdp_tx_err   = cpu_stats->xdp_tx_err;
 		} while (u64_stats_fetch_retry_irq(&cpu_stats->syncp, start));
 
-		xdp_stats->xdp_redirect += xdp_redirect;
-		xdp_stats->xdp_pass   += xdp_pass;
-		xdp_stats->xdp_drop += xdp_drop;
-		xdp_stats->xdp_xmit   += xdp_xmit;
-		xdp_stats->xdp_xmit_drops += xdp_xmit_drops;
-		xdp_stats->xdp_tx   += xdp_tx;
-		xdp_stats->xdp_tx_err   += xdp_tx_err;
+		xdp_stats->redirect += xdp_redirect;
+		xdp_stats->pass += xdp_pass;
+		xdp_stats->drop += xdp_drop;
+		xdp_stats->xmit += xdp_xmit;
+		xdp_stats->xmit_drops += xdp_xmit_drops;
+		xdp_stats->tx += xdp_tx;
+		xdp_stats->tx_errors  += xdp_tx_err;
 	}
 }
 
 static void mvpp2_read_stats(struct mvpp2_port *port)
 {
-	struct mvpp2_pcpu_stats xdp_stats = {};
-	const struct mvpp2_ethtool_counter *s;
 	u64 *pstats;
 	int i, q;
 
@@ -2057,37 +2037,6 @@ static void mvpp2_read_stats(struct mvpp2_port *port)
 			*pstats++ += mvpp2_read_index(port->priv,
 						      port->first_rxq + q,
 						      mvpp2_ethtool_rxq_regs[i].offset);
-
-	/* Gather XDP Statistics */
-	mvpp2_get_xdp_stats(port, &xdp_stats);
-
-	for (i = 0, s = mvpp2_ethtool_xdp;
-		 s < mvpp2_ethtool_xdp + ARRAY_SIZE(mvpp2_ethtool_xdp);
-	     s++, i++) {
-		switch (s->offset) {
-		case ETHTOOL_XDP_REDIRECT:
-			*pstats++ = xdp_stats.xdp_redirect;
-			break;
-		case ETHTOOL_XDP_PASS:
-			*pstats++ = xdp_stats.xdp_pass;
-			break;
-		case ETHTOOL_XDP_DROP:
-			*pstats++ = xdp_stats.xdp_drop;
-			break;
-		case ETHTOOL_XDP_TX:
-			*pstats++ = xdp_stats.xdp_tx;
-			break;
-		case ETHTOOL_XDP_TX_ERR:
-			*pstats++ = xdp_stats.xdp_tx_err;
-			break;
-		case ETHTOOL_XDP_XMIT:
-			*pstats++ = xdp_stats.xdp_xmit;
-			break;
-		case ETHTOOL_XDP_XMIT_DROPS:
-			*pstats++ = xdp_stats.xdp_xmit_drops;
-			break;
-		}
-	}
 }
 
 static void mvpp2_gather_hw_statistics(struct work_struct *work)
@@ -5735,6 +5684,7 @@ static const struct ethtool_ops mvpp2_eth_tool_ops = {
 	.set_rxfh		= mvpp2_ethtool_set_rxfh,
 	.get_rxfh_context	= mvpp2_ethtool_get_rxfh_context,
 	.set_rxfh_context	= mvpp2_ethtool_set_rxfh_context,
+	.get_xdp_stats		= mvpp2_ethtool_get_xdp_stats,
 };
 
 /* Used for PPv2.1, or PPv2.2 with the old Device Tree binding that
-- 
2.31.1

[PATCH net-next 13/21] ethernet, sfc: convert to standard XDP stats

[Alexander Lobakin]

Just like DPAA2 driver, EF{100,X} store XDP stats per-channel, but
present them as the sums across all channels.
Switch to the standard per-channel XDP stats. n_rx_xdp_bad_drops
goes as "general XDP errors", because driver uses just one counter
for all kinds of errors.

Signed-off-by: Alexander Lobakin <alexandr.lobakin@intel.com>
Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
---
 drivers/net/ethernet/sfc/ef100_ethtool.c  |  2 ++
 drivers/net/ethernet/sfc/ethtool.c        |  2 ++
 drivers/net/ethernet/sfc/ethtool_common.c | 35 ++++++++++++++++++++---
 drivers/net/ethernet/sfc/ethtool_common.h |  3 ++
 4 files changed, 38 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/sfc/ef100_ethtool.c b/drivers/net/ethernet/sfc/ef100_ethtool.c
index 835c838b7dfa..c4797fefef2e 100644
--- a/drivers/net/ethernet/sfc/ef100_ethtool.c
+++ b/drivers/net/ethernet/sfc/ef100_ethtool.c
@@ -49,6 +49,8 @@ const struct ethtool_ops ef100_ethtool_ops = {
 	.get_fecparam		= efx_ethtool_get_fecparam,
 	.set_fecparam		= efx_ethtool_set_fecparam,
 	.get_ethtool_stats	= efx_ethtool_get_stats,
+	.get_std_stats_channels	= efx_ethtool_get_std_stats_channels,
+	.get_xdp_stats		= efx_ethtool_get_xdp_stats,
 	.get_rxnfc              = efx_ethtool_get_rxnfc,
 	.set_rxnfc              = efx_ethtool_set_rxnfc,
 	.reset                  = efx_ethtool_reset,
diff --git a/drivers/net/ethernet/sfc/ethtool.c b/drivers/net/ethernet/sfc/ethtool.c
index 058d9fe41d99..307724275a3e 100644
--- a/drivers/net/ethernet/sfc/ethtool.c
+++ b/drivers/net/ethernet/sfc/ethtool.c
@@ -269,4 +269,6 @@ const struct ethtool_ops efx_ethtool_ops = {
 	.get_fec_stats		= efx_ethtool_get_fec_stats,
 	.get_fecparam		= efx_ethtool_get_fecparam,
 	.set_fecparam		= efx_ethtool_set_fecparam,
+	.get_std_stats_channels	= efx_ethtool_get_std_stats_channels,
+	.get_xdp_stats		= efx_ethtool_get_xdp_stats,
 };
diff --git a/drivers/net/ethernet/sfc/ethtool_common.c b/drivers/net/ethernet/sfc/ethtool_common.c
index bf1443539a1a..4aa6792d5795 100644
--- a/drivers/net/ethernet/sfc/ethtool_common.c
+++ b/drivers/net/ethernet/sfc/ethtool_common.c
@@ -87,10 +87,6 @@ static const struct efx_sw_stat_desc efx_sw_stat_desc[] = {
 	EFX_ETHTOOL_UINT_CHANNEL_STAT(rx_frm_trunc),
 	EFX_ETHTOOL_UINT_CHANNEL_STAT(rx_merge_events),
 	EFX_ETHTOOL_UINT_CHANNEL_STAT(rx_merge_packets),
-	EFX_ETHTOOL_UINT_CHANNEL_STAT(rx_xdp_drops),
-	EFX_ETHTOOL_UINT_CHANNEL_STAT(rx_xdp_bad_drops),
-	EFX_ETHTOOL_UINT_CHANNEL_STAT(rx_xdp_tx),
-	EFX_ETHTOOL_UINT_CHANNEL_STAT(rx_xdp_redirect),
 #ifdef CONFIG_RFS_ACCEL
 	EFX_ETHTOOL_UINT_CHANNEL_STAT_NO_N(rfs_filter_count),
 	EFX_ETHTOOL_UINT_CHANNEL_STAT(rfs_succeeded),
@@ -557,6 +553,37 @@ void efx_ethtool_get_stats(struct net_device *net_dev,
 	efx_ptp_update_stats(efx, data);
 }
 
+int efx_ethtool_get_std_stats_channels(struct net_device *net_dev, u32 sset)
+{
+	const struct efx_nic *efx = netdev_priv(net_dev);
+
+	switch (sset) {
+	case ETH_SS_STATS_XDP:
+		return efx->n_channels;
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+void efx_ethtool_get_xdp_stats(struct net_device *net_dev,
+			       struct ethtool_xdp_stats *xdp_stats)
+{
+	struct efx_nic *efx = netdev_priv(net_dev);
+	const struct efx_channel *channel;
+
+	spin_lock_bh(&efx->stats_lock);
+
+	efx_for_each_channel(channel, efx) {
+		xdp_stats->drop = channel->n_rx_xdp_drops;
+		xdp_stats->errors = channel->n_rx_xdp_bad_drops;
+		xdp_stats->redirect = channel->n_rx_xdp_redirect;
+		xdp_stats->tx = channel->n_rx_xdp_tx;
+		xdp_stats++;
+	}
+
+	spin_unlock_bh(&efx->stats_lock);
+}
+
 /* This must be called with rtnl_lock held. */
 int efx_ethtool_get_link_ksettings(struct net_device *net_dev,
 				   struct ethtool_link_ksettings *cmd)
diff --git a/drivers/net/ethernet/sfc/ethtool_common.h b/drivers/net/ethernet/sfc/ethtool_common.h
index 659491932101..bb2a43873ba1 100644
--- a/drivers/net/ethernet/sfc/ethtool_common.h
+++ b/drivers/net/ethernet/sfc/ethtool_common.h
@@ -30,6 +30,9 @@ void efx_ethtool_get_strings(struct net_device *net_dev, u32 string_set,
 void efx_ethtool_get_stats(struct net_device *net_dev,
 			   struct ethtool_stats *stats __attribute__ ((unused)),
 			   u64 *data);
+int efx_ethtool_get_std_stats_channels(struct net_device *net_dev, u32 sset);
+void efx_ethtool_get_xdp_stats(struct net_device *net_dev,
+			       struct ethtool_xdp_stats *xdp_stats);
 int efx_ethtool_get_link_ksettings(struct net_device *net_dev,
 				   struct ethtool_link_ksettings *out);
 int efx_ethtool_set_link_ksettings(struct net_device *net_dev,
-- 
2.31.1

Re: [PATCH net-next 13/21] ethernet, sfc: convert to standard XDP stats

[Edward Cree]

On 03/08/2021 17:36, Alexander Lobakin wrote:
> Just like DPAA2 driver, EF{100,X} store XDP stats per-channel, but
> present them as the sums across all channels.
> Switch to the standard per-channel XDP stats. n_rx_xdp_bad_drops
> goes as "general XDP errors", because driver uses just one counter
> for all kinds of errors.
> 
> Signed-off-by: Alexander Lobakin <alexandr.lobakin@intel.com>
> Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>

Acked-by: Edward Cree <ecree.xilinx@gmail.com>

[PATCH net-next 14/21] veth: rename rx_drops to xdp_errors

[Alexander Lobakin]

rx_drops field stores the number of errors occurred on XDP path, such
as XDP_TX or XDP_REDIRECT non-zero return codes. There are no stores
of this field outside XDP code.
Give it a more fitting name which also aligns well with the standard
XDP stats.

Signed-off-by: Alexander Lobakin <alexandr.lobakin@intel.com>
Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
---
 drivers/net/veth.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index 50eb43e5bf45..914aebfbe7c4 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -38,10 +38,10 @@
 #define VETH_XDP_BATCH		16
 
 struct veth_stats {
-	u64	rx_drops;
 	/* xdp */
 	u64	xdp_packets;
 	u64	xdp_bytes;
+	u64	xdp_errors;
 	u64	xdp_redirect;
 	u64	xdp_drops;
 	u64	xdp_tx;
@@ -94,7 +94,7 @@ struct veth_q_stat_desc {
 static const struct veth_q_stat_desc veth_rq_stats_desc[] = {
 	{ "xdp_packets",	VETH_RQ_STAT(xdp_packets) },
 	{ "xdp_bytes",		VETH_RQ_STAT(xdp_bytes) },
-	{ "drops",		VETH_RQ_STAT(rx_drops) },
+	{ "xdp_errors",		VETH_RQ_STAT(xdp_errors) },
 	{ "xdp_redirect",	VETH_RQ_STAT(xdp_redirect) },
 	{ "xdp_drops",		VETH_RQ_STAT(xdp_drops) },
 	{ "xdp_tx",		VETH_RQ_STAT(xdp_tx) },
@@ -379,9 +379,9 @@ static void veth_stats_rx(struct veth_stats *result, struct net_device *dev)
 	result->xdp_packets = 0;
 	result->xdp_tx_err = 0;
 	result->xdp_bytes = 0;
-	result->rx_drops = 0;
+	result->xdp_errors = 0;
 	for (i = 0; i < dev->num_rx_queues; i++) {
-		u64 packets, bytes, drops, xdp_tx_err, peer_tq_xdp_xmit_err;
+		u64 packets, bytes, xdp_err, xdp_tx_err, peer_tq_xdp_xmit_err;
 		struct veth_rq_stats *stats = &priv->rq[i].stats;
 		unsigned int start;
 
@@ -391,13 +391,13 @@ static void veth_stats_rx(struct veth_stats *result, struct net_device *dev)
 			xdp_tx_err = stats->vs.xdp_tx_err;
 			packets = stats->vs.xdp_packets;
 			bytes = stats->vs.xdp_bytes;
-			drops = stats->vs.rx_drops;
+			xdp_err = stats->vs.xdp_errors;
 		} while (u64_stats_fetch_retry_irq(&stats->syncp, start));
 		result->peer_tq_xdp_xmit_err += peer_tq_xdp_xmit_err;
 		result->xdp_tx_err += xdp_tx_err;
 		result->xdp_packets += packets;
 		result->xdp_bytes += bytes;
-		result->rx_drops += drops;
+		result->xdp_errors += xdp_err;
 	}
 }
 
@@ -415,7 +415,7 @@ static void veth_get_stats64(struct net_device *dev,
 
 	veth_stats_rx(&rx, dev);
 	tot->tx_dropped += rx.xdp_tx_err;
-	tot->rx_dropped = rx.rx_drops + rx.peer_tq_xdp_xmit_err;
+	tot->rx_dropped = rx.xdp_errors + rx.peer_tq_xdp_xmit_err;
 	tot->rx_bytes = rx.xdp_bytes;
 	tot->rx_packets = rx.xdp_packets;
 
@@ -633,7 +633,7 @@ static struct xdp_frame *veth_xdp_rcv_one(struct veth_rq *rq,
 			if (unlikely(veth_xdp_tx(rq, &xdp, bq) < 0)) {
 				trace_xdp_exception(rq->dev, xdp_prog, act);
 				frame = &orig_frame;
-				stats->rx_drops++;
+				stats->xdp_errors++;
 				goto err_xdp;
 			}
 			stats->xdp_tx++;
@@ -644,7 +644,7 @@ static struct xdp_frame *veth_xdp_rcv_one(struct veth_rq *rq,
 			xdp.rxq->mem = frame->mem;
 			if (xdp_do_redirect(rq->dev, &xdp, xdp_prog)) {
 				frame = &orig_frame;
-				stats->rx_drops++;
+				stats->xdp_errors++;
 				goto err_xdp;
 			}
 			stats->xdp_redirect++;
@@ -683,7 +683,7 @@ static void veth_xdp_rcv_bulk_skb(struct veth_rq *rq, void **frames,
 			       GFP_ATOMIC | __GFP_ZERO) < 0) {
 		for (i = 0; i < n_xdpf; i++)
 			xdp_return_frame(frames[i]);
-		stats->rx_drops += n_xdpf;
+		stats->xdp_errors += n_xdpf;
 
 		return;
 	}
@@ -695,7 +695,7 @@ static void veth_xdp_rcv_bulk_skb(struct veth_rq *rq, void **frames,
 						 rq->dev);
 		if (!skb) {
 			xdp_return_frame(frames[i]);
-			stats->rx_drops++;
+			stats->xdp_errors++;
 			continue;
 		}
 		napi_gro_receive(&rq->xdp_napi, skb);
@@ -783,7 +783,7 @@ static struct sk_buff *veth_xdp_rcv_skb(struct veth_rq *rq,
 		xdp.rxq->mem = rq->xdp_mem;
 		if (unlikely(veth_xdp_tx(rq, &xdp, bq) < 0)) {
 			trace_xdp_exception(rq->dev, xdp_prog, act);
-			stats->rx_drops++;
+			stats->xdp_errors++;
 			goto err_xdp;
 		}
 		stats->xdp_tx++;
@@ -794,7 +794,7 @@ static struct sk_buff *veth_xdp_rcv_skb(struct veth_rq *rq,
 		consume_skb(skb);
 		xdp.rxq->mem = rq->xdp_mem;
 		if (xdp_do_redirect(rq->dev, &xdp, xdp_prog)) {
-			stats->rx_drops++;
+			stats->xdp_errors++;
 			goto err_xdp;
 		}
 		stats->xdp_redirect++;
@@ -833,7 +833,7 @@ static struct sk_buff *veth_xdp_rcv_skb(struct veth_rq *rq,
 out:
 	return skb;
 drop:
-	stats->rx_drops++;
+	stats->xdp_errors++;
 xdp_drop:
 	rcu_read_unlock();
 	kfree_skb(skb);
@@ -892,7 +892,7 @@ static int veth_xdp_rcv(struct veth_rq *rq, int budget,
 	rq->stats.vs.xdp_redirect += stats->xdp_redirect;
 	rq->stats.vs.xdp_bytes += stats->xdp_bytes;
 	rq->stats.vs.xdp_drops += stats->xdp_drops;
-	rq->stats.vs.rx_drops += stats->rx_drops;
+	rq->stats.vs.xdp_errors += stats->xdp_errors;
 	rq->stats.vs.xdp_packets += done;
 	u64_stats_update_end(&rq->stats.syncp);
 
-- 
2.31.1

[PATCH net-next 15/21] veth: rename xdp_xmit_errors to xdp_xmit_drops

[Alexander Lobakin]

veth keeps tracking of numbers of XDP frames being dropped from
inside of .ndo_xdp_xmit() callback. This counter really should
be named after drops, not errors.

Signed-off-by: Alexander Lobakin <alexandr.lobakin@intel.com>
Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
---
 drivers/net/veth.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index 914aebfbe7c4..c5079b9145c9 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -47,7 +47,7 @@ struct veth_stats {
 	u64	xdp_tx;
 	u64	xdp_tx_err;
 	u64	peer_tq_xdp_xmit;
-	u64	peer_tq_xdp_xmit_err;
+	u64	peer_tq_xdp_xmit_drops;
 };
 
 struct veth_rq_stats {
@@ -105,7 +105,7 @@ static const struct veth_q_stat_desc veth_rq_stats_desc[] = {
 
 static const struct veth_q_stat_desc veth_tq_stats_desc[] = {
 	{ "xdp_xmit",		VETH_RQ_STAT(peer_tq_xdp_xmit) },
-	{ "xdp_xmit_errors",	VETH_RQ_STAT(peer_tq_xdp_xmit_err) },
+	{ "xdp_xmit_drops",	VETH_RQ_STAT(peer_tq_xdp_xmit_drops) },
 };
 
 #define VETH_TQ_STATS_LEN	ARRAY_SIZE(veth_tq_stats_desc)
@@ -375,25 +375,25 @@ static void veth_stats_rx(struct veth_stats *result, struct net_device *dev)
 	struct veth_priv *priv = netdev_priv(dev);
 	int i;
 
-	result->peer_tq_xdp_xmit_err = 0;
+	result->peer_tq_xdp_xmit_drops = 0;
 	result->xdp_packets = 0;
 	result->xdp_tx_err = 0;
 	result->xdp_bytes = 0;
 	result->xdp_errors = 0;
 	for (i = 0; i < dev->num_rx_queues; i++) {
-		u64 packets, bytes, xdp_err, xdp_tx_err, peer_tq_xdp_xmit_err;
+		u64 packets, bytes, xdp_err, xdp_tx_err, peer_tq_xdp_xmit_drops;
 		struct veth_rq_stats *stats = &priv->rq[i].stats;
 		unsigned int start;
 
 		do {
 			start = u64_stats_fetch_begin_irq(&stats->syncp);
-			peer_tq_xdp_xmit_err = stats->vs.peer_tq_xdp_xmit_err;
+			peer_tq_xdp_xmit_drops = stats->vs.peer_tq_xdp_xmit_drops;
 			xdp_tx_err = stats->vs.xdp_tx_err;
 			packets = stats->vs.xdp_packets;
 			bytes = stats->vs.xdp_bytes;
 			xdp_err = stats->vs.xdp_errors;
 		} while (u64_stats_fetch_retry_irq(&stats->syncp, start));
-		result->peer_tq_xdp_xmit_err += peer_tq_xdp_xmit_err;
+		result->peer_tq_xdp_xmit_drops += peer_tq_xdp_xmit_drops;
 		result->xdp_tx_err += xdp_tx_err;
 		result->xdp_packets += packets;
 		result->xdp_bytes += bytes;
@@ -415,7 +415,7 @@ static void veth_get_stats64(struct net_device *dev,
 
 	veth_stats_rx(&rx, dev);
 	tot->tx_dropped += rx.xdp_tx_err;
-	tot->rx_dropped = rx.xdp_errors + rx.peer_tq_xdp_xmit_err;
+	tot->rx_dropped = rx.xdp_errors + rx.peer_tq_xdp_xmit_drops;
 	tot->rx_bytes = rx.xdp_bytes;
 	tot->rx_packets = rx.xdp_packets;
 
@@ -427,7 +427,7 @@ static void veth_get_stats64(struct net_device *dev,
 		tot->rx_packets += packets;
 
 		veth_stats_rx(&rx, peer);
-		tot->tx_dropped += rx.peer_tq_xdp_xmit_err;
+		tot->tx_dropped += rx.peer_tq_xdp_xmit_drops;
 		tot->rx_dropped += rx.xdp_tx_err;
 		tot->tx_bytes += rx.xdp_bytes;
 		tot->tx_packets += rx.xdp_packets;
@@ -515,7 +515,7 @@ static int veth_xdp_xmit(struct net_device *dev, int n,
 	if (ndo_xmit) {
 		u64_stats_update_begin(&rq->stats.syncp);
 		rq->stats.vs.peer_tq_xdp_xmit += nxmit;
-		rq->stats.vs.peer_tq_xdp_xmit_err += n - nxmit;
+		rq->stats.vs.peer_tq_xdp_xmit_drops += n - nxmit;
 		u64_stats_update_end(&rq->stats.syncp);
 	}
 
-- 
2.31.1

[PATCH net-next 16/21] veth: rename drop xdp_ suffix from packets and bytes stats

[Alexander Lobakin]

They get updated not only on XDP path. Moreover, packet counter
stores the total number of frames, not only the ones passed to
bpf_prog_run_xdp(), so it's rather confusing.
Drop the xdp_ suffix from both of them to not mix XDP-only stats
with the general ones.

Signed-off-by: Alexander Lobakin <alexandr.lobakin@intel.com>
Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
---
 drivers/net/veth.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index c5079b9145c9..d7e95f09e19d 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -38,9 +38,9 @@
 #define VETH_XDP_BATCH		16
 
 struct veth_stats {
+	u64	packets;
+	u64	bytes;
 	/* xdp */
-	u64	xdp_packets;
-	u64	xdp_bytes;
 	u64	xdp_errors;
 	u64	xdp_redirect;
 	u64	xdp_drops;
@@ -92,8 +92,8 @@ struct veth_q_stat_desc {
 #define VETH_RQ_STAT(m)	offsetof(struct veth_stats, m)
 
 static const struct veth_q_stat_desc veth_rq_stats_desc[] = {
-	{ "xdp_packets",	VETH_RQ_STAT(xdp_packets) },
-	{ "xdp_bytes",		VETH_RQ_STAT(xdp_bytes) },
+	{ "packets",		VETH_RQ_STAT(packets) },
+	{ "bytes",		VETH_RQ_STAT(bytes) },
 	{ "xdp_errors",		VETH_RQ_STAT(xdp_errors) },
 	{ "xdp_redirect",	VETH_RQ_STAT(xdp_redirect) },
 	{ "xdp_drops",		VETH_RQ_STAT(xdp_drops) },
@@ -376,9 +376,9 @@ static void veth_stats_rx(struct veth_stats *result, struct net_device *dev)
 	int i;
 
 	result->peer_tq_xdp_xmit_drops = 0;
-	result->xdp_packets = 0;
+	result->packets = 0;
 	result->xdp_tx_err = 0;
-	result->xdp_bytes = 0;
+	result->bytes = 0;
 	result->xdp_errors = 0;
 	for (i = 0; i < dev->num_rx_queues; i++) {
 		u64 packets, bytes, xdp_err, xdp_tx_err, peer_tq_xdp_xmit_drops;
@@ -389,14 +389,14 @@ static void veth_stats_rx(struct veth_stats *result, struct net_device *dev)
 			start = u64_stats_fetch_begin_irq(&stats->syncp);
 			peer_tq_xdp_xmit_drops = stats->vs.peer_tq_xdp_xmit_drops;
 			xdp_tx_err = stats->vs.xdp_tx_err;
-			packets = stats->vs.xdp_packets;
-			bytes = stats->vs.xdp_bytes;
+			packets = stats->vs.packets;
+			bytes = stats->vs.bytes;
 			xdp_err = stats->vs.xdp_errors;
 		} while (u64_stats_fetch_retry_irq(&stats->syncp, start));
 		result->peer_tq_xdp_xmit_drops += peer_tq_xdp_xmit_drops;
 		result->xdp_tx_err += xdp_tx_err;
-		result->xdp_packets += packets;
-		result->xdp_bytes += bytes;
+		result->packets += packets;
+		result->bytes += bytes;
 		result->xdp_errors += xdp_err;
 	}
 }
@@ -416,8 +416,8 @@ static void veth_get_stats64(struct net_device *dev,
 	veth_stats_rx(&rx, dev);
 	tot->tx_dropped += rx.xdp_tx_err;
 	tot->rx_dropped = rx.xdp_errors + rx.peer_tq_xdp_xmit_drops;
-	tot->rx_bytes = rx.xdp_bytes;
-	tot->rx_packets = rx.xdp_packets;
+	tot->rx_bytes = rx.bytes;
+	tot->rx_packets = rx.packets;
 
 	rcu_read_lock();
 	peer = rcu_dereference(priv->peer);
@@ -429,8 +429,8 @@ static void veth_get_stats64(struct net_device *dev,
 		veth_stats_rx(&rx, peer);
 		tot->tx_dropped += rx.peer_tq_xdp_xmit_drops;
 		tot->rx_dropped += rx.xdp_tx_err;
-		tot->tx_bytes += rx.xdp_bytes;
-		tot->tx_packets += rx.xdp_packets;
+		tot->tx_bytes += rx.bytes;
+		tot->tx_packets += rx.packets;
 	}
 	rcu_read_unlock();
 }
@@ -862,7 +862,7 @@ static int veth_xdp_rcv(struct veth_rq *rq, int budget,
 			/* ndo_xdp_xmit */
 			struct xdp_frame *frame = veth_ptr_to_xdp(ptr);
 
-			stats->xdp_bytes += frame->len;
+			stats->bytes += frame->len;
 			frame = veth_xdp_rcv_one(rq, frame, bq, stats);
 			if (frame) {
 				/* XDP_PASS */
@@ -877,7 +877,7 @@ static int veth_xdp_rcv(struct veth_rq *rq, int budget,
 			/* ndo_start_xmit */
 			struct sk_buff *skb = ptr;
 
-			stats->xdp_bytes += skb->len;
+			stats->bytes += skb->len;
 			skb = veth_xdp_rcv_skb(rq, skb, bq, stats);
 			if (skb)
 				napi_gro_receive(&rq->xdp_napi, skb);
@@ -890,10 +890,10 @@ static int veth_xdp_rcv(struct veth_rq *rq, int budget,
 
 	u64_stats_update_begin(&rq->stats.syncp);
 	rq->stats.vs.xdp_redirect += stats->xdp_redirect;
-	rq->stats.vs.xdp_bytes += stats->xdp_bytes;
+	rq->stats.vs.bytes += stats->bytes;
 	rq->stats.vs.xdp_drops += stats->xdp_drops;
 	rq->stats.vs.xdp_errors += stats->xdp_errors;
-	rq->stats.vs.xdp_packets += done;
+	rq->stats.vs.packets += done;
 	u64_stats_update_end(&rq->stats.syncp);
 
 	return done;
-- 
2.31.1

[PATCH net-next 17/21] veth: convert to standard XDP stats

[Alexander Lobakin]

veth has 7 per-channel XDP counters which could be aligned to the
standard XDP stats. Peer stats are here too as well, with the
original channel numbering logics.

Signed-off-by: Alexander Lobakin <alexandr.lobakin@intel.com>
Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
---
 drivers/net/veth.c | 91 +++++++++++++++++++++++++++++-----------------
 1 file changed, 57 insertions(+), 34 deletions(-)

diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index d7e95f09e19d..79614d8e88bd 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -94,22 +94,10 @@ struct veth_q_stat_desc {
 static const struct veth_q_stat_desc veth_rq_stats_desc[] = {
 	{ "packets",		VETH_RQ_STAT(packets) },
 	{ "bytes",		VETH_RQ_STAT(bytes) },
-	{ "xdp_errors",		VETH_RQ_STAT(xdp_errors) },
-	{ "xdp_redirect",	VETH_RQ_STAT(xdp_redirect) },
-	{ "xdp_drops",		VETH_RQ_STAT(xdp_drops) },
-	{ "xdp_tx",		VETH_RQ_STAT(xdp_tx) },
-	{ "xdp_tx_errors",	VETH_RQ_STAT(xdp_tx_err) },
 };
 
 #define VETH_RQ_STATS_LEN	ARRAY_SIZE(veth_rq_stats_desc)
 
-static const struct veth_q_stat_desc veth_tq_stats_desc[] = {
-	{ "xdp_xmit",		VETH_RQ_STAT(peer_tq_xdp_xmit) },
-	{ "xdp_xmit_drops",	VETH_RQ_STAT(peer_tq_xdp_xmit_drops) },
-};
-
-#define VETH_TQ_STATS_LEN	ARRAY_SIZE(veth_tq_stats_desc)
-
 static struct {
 	const char string[ETH_GSTRING_LEN];
 } ethtool_stats_keys[] = {
@@ -149,14 +137,6 @@ static void veth_get_strings(struct net_device *dev, u32 stringset, u8 *buf)
 				p += ETH_GSTRING_LEN;
 			}
 		}
-		for (i = 0; i < dev->real_num_tx_queues; i++) {
-			for (j = 0; j < VETH_TQ_STATS_LEN; j++) {
-				snprintf(p, ETH_GSTRING_LEN,
-					 "tx_queue_%u_%.18s",
-					 i, veth_tq_stats_desc[j].desc);
-				p += ETH_GSTRING_LEN;
-			}
-		}
 		break;
 	}
 }
@@ -166,8 +146,7 @@ static int veth_get_sset_count(struct net_device *dev, int sset)
 	switch (sset) {
 	case ETH_SS_STATS:
 		return ARRAY_SIZE(ethtool_stats_keys) +
-		       VETH_RQ_STATS_LEN * dev->real_num_rx_queues +
-		       VETH_TQ_STATS_LEN * dev->real_num_tx_queues;
+		       VETH_RQ_STATS_LEN * dev->real_num_rx_queues;
 	default:
 		return -EOPNOTSUPP;
 	}
@@ -176,8 +155,8 @@ static int veth_get_sset_count(struct net_device *dev, int sset)
 static void veth_get_ethtool_stats(struct net_device *dev,
 		struct ethtool_stats *stats, u64 *data)
 {
-	struct veth_priv *rcv_priv, *priv = netdev_priv(dev);
-	struct net_device *peer = rtnl_dereference(priv->peer);
+	const struct veth_priv *priv = netdev_priv(dev);
+	const struct net_device *peer = rtnl_dereference(priv->peer);
 	int i, j, idx;
 
 	data[0] = peer ? peer->ifindex : 0;
@@ -197,25 +176,67 @@ static void veth_get_ethtool_stats(struct net_device *dev,
 		} while (u64_stats_fetch_retry_irq(&rq_stats->syncp, start));
 		idx += VETH_RQ_STATS_LEN;
 	}
+}
+
+static int veth_get_std_stats_channels(struct net_device *dev, u32 sset)
+{
+	switch (sset) {
+	case ETH_SS_STATS_XDP:
+		return max(dev->real_num_rx_queues, dev->real_num_tx_queues);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static void veth_get_xdp_stats(struct net_device *dev,
+			       struct ethtool_xdp_stats *xdp_stats)
+{
+	const struct veth_priv *priv = netdev_priv(dev);
+	const struct net_device *peer = rtnl_dereference(priv->peer);
+	const struct veth_rq_stats *rq_stats;
+	struct ethtool_xdp_stats *iter;
+	u64 xmit, xmit_drops;
+	u32 i, start;
+
+	for (i = 0; i < dev->real_num_rx_queues; i++) {
+		rq_stats = &priv->rq[i].stats;
+		iter = xdp_stats + i;
+
+		do {
+			start = u64_stats_fetch_begin_irq(&rq_stats->syncp);
+
+			iter->errors = rq_stats->vs.xdp_errors;
+			iter->redirect = rq_stats->vs.xdp_redirect;
+			iter->drop = rq_stats->vs.xdp_drops;
+			iter->tx = rq_stats->vs.xdp_tx;
+			iter->tx_errors = rq_stats->vs.xdp_tx_err;
+		} while (u64_stats_fetch_retry_irq(&rq_stats->syncp, start));
+	}
 
 	if (!peer)
 		return;
 
-	rcv_priv = netdev_priv(peer);
+	for (i = 0; i < dev->real_num_tx_queues; i++) {
+		iter = xdp_stats + i;
+		iter->xmit = 0;
+		iter->xmit_drops = 0;
+	}
+
+	priv = netdev_priv(peer);
+
 	for (i = 0; i < peer->real_num_rx_queues; i++) {
-		const struct veth_rq_stats *rq_stats = &rcv_priv->rq[i].stats;
-		const void *base = (void *)&rq_stats->vs;
-		unsigned int start, tx_idx = idx;
-		size_t offset;
+		rq_stats = &priv->rq[i].stats;
+		iter = xdp_stats + (i % dev->real_num_tx_queues);
 
-		tx_idx += (i % dev->real_num_tx_queues) * VETH_TQ_STATS_LEN;
 		do {
 			start = u64_stats_fetch_begin_irq(&rq_stats->syncp);
-			for (j = 0; j < VETH_TQ_STATS_LEN; j++) {
-				offset = veth_tq_stats_desc[j].offset;
-				data[tx_idx + j] += *(u64 *)(base + offset);
-			}
+
+			xmit = rq_stats->vs.peer_tq_xdp_xmit;
+			xmit_drops = rq_stats->vs.peer_tq_xdp_xmit_drops;
 		} while (u64_stats_fetch_retry_irq(&rq_stats->syncp, start));
+
+		iter->xmit += xmit;
+		iter->xmit_drops += xmit_drops;
 	}
 }
 
@@ -241,6 +262,8 @@ static const struct ethtool_ops veth_ethtool_ops = {
 	.get_ts_info		= ethtool_op_get_ts_info,
 	.get_channels		= veth_get_channels,
 	.set_channels		= veth_set_channels,
+	.get_std_stats_channels	= veth_get_std_stats_channels,
+	.get_xdp_stats		= veth_get_xdp_stats,
 };
 
 /* general routines */
-- 
2.31.1

[PATCH net-next 18/21] virtio-net: rename xdp_tx{,__drops} SQ stats to xdp_xmit{,__drops}

[Alexander Lobakin]

These two are present to track the number of frames processed and
dropped by .ndo_xdp_xmit() callback, rename them accordingly to
avoid confusion with xdp_tx RQ field and XDP_TX case in general.

Signed-off-by: Alexander Lobakin <alexandr.lobakin@intel.com>
Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
---
 drivers/net/virtio_net.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 74482a52f076..8cbb4651ed75 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -77,8 +77,8 @@ struct virtnet_sq_stats {
 	struct u64_stats_sync syncp;
 	u64 packets;
 	u64 bytes;
-	u64 xdp_tx;
-	u64 xdp_tx_drops;
+	u64 xdp_xmit;
+	u64 xdp_xmit_drops;
 	u64 kicks;
 };
 
@@ -100,8 +100,8 @@ struct virtnet_rq_stats {
 static const struct virtnet_stat_desc virtnet_sq_stats_desc[] = {
 	{ "packets",		VIRTNET_SQ_STAT(packets) },
 	{ "bytes",		VIRTNET_SQ_STAT(bytes) },
-	{ "xdp_tx",		VIRTNET_SQ_STAT(xdp_tx) },
-	{ "xdp_tx_drops",	VIRTNET_SQ_STAT(xdp_tx_drops) },
+	{ "xdp_xmit",		VIRTNET_SQ_STAT(xdp_xmit) },
+	{ "xdp_xmit_drops",	VIRTNET_SQ_STAT(xdp_xmit_drops) },
 	{ "kicks",		VIRTNET_SQ_STAT(kicks) },
 };
 
@@ -619,8 +619,8 @@ static int virtnet_xdp_xmit(struct net_device *dev,
 	u64_stats_update_begin(&sq->stats.syncp);
 	sq->stats.bytes += bytes;
 	sq->stats.packets += packets;
-	sq->stats.xdp_tx += n;
-	sq->stats.xdp_tx_drops += n - nxmit;
+	sq->stats.xdp_xmit += n;
+	sq->stats.xdp_xmit_drops += n - nxmit;
 	sq->stats.kicks += kicks;
 	u64_stats_update_end(&sq->stats.syncp);
 
-- 
2.31.1

[PATCH net-next 18/21] virtio-net: rename xdp_tx{,__drops} SQ stats to xdp_xmit{,__drops}

[Alexander Lobakin]

These two are present to track the number of frames processed and
dropped by .ndo_xdp_xmit() callback, rename them accordingly to
avoid confusion with xdp_tx RQ field and XDP_TX case in general.

Signed-off-by: Alexander Lobakin <alexandr.lobakin@intel.com>
Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
---
 drivers/net/virtio_net.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 74482a52f076..8cbb4651ed75 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -77,8 +77,8 @@ struct virtnet_sq_stats {
 	struct u64_stats_sync syncp;
 	u64 packets;
 	u64 bytes;
-	u64 xdp_tx;
-	u64 xdp_tx_drops;
+	u64 xdp_xmit;
+	u64 xdp_xmit_drops;
 	u64 kicks;
 };
 
@@ -100,8 +100,8 @@ struct virtnet_rq_stats {
 static const struct virtnet_stat_desc virtnet_sq_stats_desc[] = {
 	{ "packets",		VIRTNET_SQ_STAT(packets) },
 	{ "bytes",		VIRTNET_SQ_STAT(bytes) },
-	{ "xdp_tx",		VIRTNET_SQ_STAT(xdp_tx) },
-	{ "xdp_tx_drops",	VIRTNET_SQ_STAT(xdp_tx_drops) },
+	{ "xdp_xmit",		VIRTNET_SQ_STAT(xdp_xmit) },
+	{ "xdp_xmit_drops",	VIRTNET_SQ_STAT(xdp_xmit_drops) },
 	{ "kicks",		VIRTNET_SQ_STAT(kicks) },
 };
 
@@ -619,8 +619,8 @@ static int virtnet_xdp_xmit(struct net_device *dev,
 	u64_stats_update_begin(&sq->stats.syncp);
 	sq->stats.bytes += bytes;
 	sq->stats.packets += packets;
-	sq->stats.xdp_tx += n;
-	sq->stats.xdp_tx_drops += n - nxmit;
+	sq->stats.xdp_xmit += n;
+	sq->stats.xdp_xmit_drops += n - nxmit;
 	sq->stats.kicks += kicks;
 	u64_stats_update_end(&sq->stats.syncp);
 
-- 
2.31.1

[PATCH net-next 19/21] virtio-net: don't mix error-caused drops with XDP_DROP cases

[Alexander Lobakin]

It's pretty confusing to have just one field for tracking both
XDP_DROP cases and various errors which lead to the frame being
dropped.
Add a new field, xdp_errors, to separate error paths, and leave
xdp_drops purely for counting frames with the XDP_DROP verdict.

Signed-off-by: Alexander Lobakin <alexandr.lobakin@intel.com>
Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
---
 drivers/net/virtio_net.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 8cbb4651ed75..acad099006cd 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -91,6 +91,7 @@ struct virtnet_rq_stats {
 	u64 xdp_tx;
 	u64 xdp_redirects;
 	u64 xdp_drops;
+	u64 xdp_errors;
 	u64 kicks;
 };
 
@@ -113,6 +114,7 @@ static const struct virtnet_stat_desc virtnet_rq_stats_desc[] = {
 	{ "xdp_tx",		VIRTNET_RQ_STAT(xdp_tx) },
 	{ "xdp_redirects",	VIRTNET_RQ_STAT(xdp_redirects) },
 	{ "xdp_drops",		VIRTNET_RQ_STAT(xdp_drops) },
+	{ "xdp_errors",		VIRTNET_RQ_STAT(xdp_errors) },
 	{ "kicks",		VIRTNET_RQ_STAT(kicks) },
 };
 
@@ -804,7 +806,8 @@ static struct sk_buff *receive_small(struct net_device *dev,
 			trace_xdp_exception(vi->dev, xdp_prog, act);
 			goto err_xdp;
 		case XDP_DROP:
-			goto err_xdp;
+			stats->xdp_drops++;
+			goto xdp_drop;
 		}
 	}
 	rcu_read_unlock();
@@ -828,8 +831,9 @@ static struct sk_buff *receive_small(struct net_device *dev,
 	return skb;
 
 err_xdp:
+	stats->xdp_errors++;
+xdp_drop:
 	rcu_read_unlock();
-	stats->xdp_drops++;
 err_len:
 	stats->drops++;
 	put_page(page);
@@ -1012,7 +1016,12 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
 		case XDP_DROP:
 			if (unlikely(xdp_page != page))
 				__free_pages(xdp_page, 0);
-			goto err_xdp;
+
+			if (unlikely(act != XDP_DROP))
+				goto err_xdp;
+
+			stats->xdp_drops++;
+			goto xdp_drop;
 		}
 	}
 	rcu_read_unlock();
@@ -1081,8 +1090,9 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
 	return head_skb;
 
 err_xdp:
+	stats->xdp_errors++;
+xdp_drop:
 	rcu_read_unlock();
-	stats->xdp_drops++;
 err_skb:
 	put_page(page);
 	while (num_buf-- > 1) {
-- 
2.31.1

[PATCH net-next 20/21] virtio-net: convert to standard XDP stats

[Alexander Lobakin]

Now that we have 1:1 correspondence between the driver XDP stats and
the standard XDP stats, we can go forth and convert the driver to
expose XDP statistics via our standard interface.

Signed-off-by: Alexander Lobakin <alexandr.lobakin@intel.com>
Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
---
 drivers/net/virtio_net.c | 52 ++++++++++++++++++++++++++++++++++------
 1 file changed, 45 insertions(+), 7 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index acad099006cd..df1fe36cf089 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -101,8 +101,6 @@ struct virtnet_rq_stats {
 static const struct virtnet_stat_desc virtnet_sq_stats_desc[] = {
 	{ "packets",		VIRTNET_SQ_STAT(packets) },
 	{ "bytes",		VIRTNET_SQ_STAT(bytes) },
-	{ "xdp_xmit",		VIRTNET_SQ_STAT(xdp_xmit) },
-	{ "xdp_xmit_drops",	VIRTNET_SQ_STAT(xdp_xmit_drops) },
 	{ "kicks",		VIRTNET_SQ_STAT(kicks) },
 };
 
@@ -110,11 +108,6 @@ static const struct virtnet_stat_desc virtnet_rq_stats_desc[] = {
 	{ "packets",		VIRTNET_RQ_STAT(packets) },
 	{ "bytes",		VIRTNET_RQ_STAT(bytes) },
 	{ "drops",		VIRTNET_RQ_STAT(drops) },
-	{ "xdp_packets",	VIRTNET_RQ_STAT(xdp_packets) },
-	{ "xdp_tx",		VIRTNET_RQ_STAT(xdp_tx) },
-	{ "xdp_redirects",	VIRTNET_RQ_STAT(xdp_redirects) },
-	{ "xdp_drops",		VIRTNET_RQ_STAT(xdp_drops) },
-	{ "xdp_errors",		VIRTNET_RQ_STAT(xdp_errors) },
 	{ "kicks",		VIRTNET_RQ_STAT(kicks) },
 };
 
@@ -2295,6 +2288,49 @@ static void virtnet_get_ethtool_stats(struct net_device *dev,
 	}
 }
 
+static int virtnet_get_std_stats_channels(struct net_device *dev, u32 sset)
+{
+	const struct virtnet_info *vi = netdev_priv(dev);
+
+	switch (sset) {
+	case ETH_SS_STATS_XDP:
+		return vi->curr_queue_pairs;
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static void virtnet_get_xdp_stats(struct net_device *dev,
+				  struct ethtool_xdp_stats *xdp_stats)
+{
+	const struct virtnet_info *vi = netdev_priv(dev);
+	u32 i;
+
+	for (i = 0; i < vi->curr_queue_pairs; i++) {
+		const struct virtnet_rq_stats *rqs = &vi->rq[i].stats;
+		const struct virtnet_sq_stats *sqs = &vi->sq[i].stats;
+		struct ethtool_xdp_stats *iter = xdp_stats + i;
+		u32 start;
+
+		do {
+			start = u64_stats_fetch_begin_irq(&rqs->syncp);
+
+			iter->packets = rqs->xdp_packets;
+			iter->tx = rqs->xdp_tx;
+			iter->redirect = rqs->xdp_redirects;
+			iter->drop = rqs->xdp_drops;
+			iter->errors = rqs->xdp_errors;
+		} while (u64_stats_fetch_retry_irq(&rqs->syncp, start));
+
+		do {
+			start = u64_stats_fetch_begin_irq(&sqs->syncp);
+
+			iter->xmit = sqs->xdp_xmit;
+			iter->xmit_drops = sqs->xdp_xmit_drops;
+		} while (u64_stats_fetch_retry_irq(&sqs->syncp, start));
+	}
+}
+
 static void virtnet_get_channels(struct net_device *dev,
 				 struct ethtool_channels *channels)
 {
@@ -2409,6 +2445,8 @@ static const struct ethtool_ops virtnet_ethtool_ops = {
 	.set_link_ksettings = virtnet_set_link_ksettings,
 	.set_coalesce = virtnet_set_coalesce,
 	.get_coalesce = virtnet_get_coalesce,
+	.get_std_stats_channels = virtnet_get_std_stats_channels,
+	.get_xdp_stats = virtnet_get_xdp_stats,
 };
 
 static void virtnet_freeze_down(struct virtio_device *vdev)
-- 
2.31.1

[PATCH net-next 21/21] Documentation, ethtool-netlink: update standard statistics documentation

[Alexander Lobakin]

Reflect the addition of the new standard XDP stats as well as of
a new NL attribute.

Signed-off-by: Alexander Lobakin <alexandr.lobakin@intel.com>
Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
---
 Documentation/networking/ethtool-netlink.rst | 45 +++++++++++++-------
 1 file changed, 30 insertions(+), 15 deletions(-)

diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
index c86628e6a235..d304a5361569 100644
--- a/Documentation/networking/ethtool-netlink.rst
+++ b/Documentation/networking/ethtool-netlink.rst
@@ -1415,21 +1415,26 @@ Request contents:
 
 Kernel response contents:
 
- +-----------------------------------+--------+--------------------------------+
- | ``ETHTOOL_A_STATS_HEADER``        | nested | reply header                   |
- +-----------------------------------+--------+--------------------------------+
- | ``ETHTOOL_A_STATS_GRP``           | nested | one or more group of stats     |
- +-+---------------------------------+--------+--------------------------------+
- | | ``ETHTOOL_A_STATS_GRP_ID``      | u32    | group ID - ``ETHTOOL_STATS_*`` |
- +-+---------------------------------+--------+--------------------------------+
- | | ``ETHTOOL_A_STATS_GRP_SS_ID``   | u32    | string set ID for names        |
- +-+---------------------------------+--------+--------------------------------+
- | | ``ETHTOOL_A_STATS_GRP_STAT``    | nested | nest containing a statistic    |
- +-+---------------------------------+--------+--------------------------------+
- | | ``ETHTOOL_A_STATS_GRP_HIST_RX`` | nested | histogram statistic (Rx)       |
- +-+---------------------------------+--------+--------------------------------+
- | | ``ETHTOOL_A_STATS_GRP_HIST_TX`` | nested | histogram statistic (Tx)       |
- +-+---------------------------------+--------+--------------------------------+
+ +--------------------------------------+--------+-----------------------------+
+ | ``ETHTOOL_A_STATS_HEADER``           | nested | reply header                |
+ +--------------------------------------+--------+-----------------------------+
+ | ``ETHTOOL_A_STATS_GRP``              | nested | one or more group of stats  |
+ +-+------------------------------------+--------+-----------------------------+
+ | | ``ETHTOOL_A_STATS_GRP_ID``         | u32    | group ID -                  |
+ | |                                    |        | ``ETHTOOL_STATS_*``         |
+ +-+------------------------------------+--------+-----------------------------+
+ | | ``ETHTOOL_A_STATS_GRP_SS_ID``      | u32    | string set ID for names     |
+ +-+------------------------------------+--------+-----------------------------+
+ | | ``ETHTOOL_A_STATS_GRP_STAT``       | nested | nest containing a statistic |
+ +-+------------------------------------+--------+-----------------------------+
+ | | ``ETHTOOL_A_STATS_GRP_STAT_BLOCK`` | nested | block of stats per channel  |
+ +-+-+----------------------------------+--------+-----------------------------+
+ | | | ``ETHTOOL_A_STATS_GRP_STAT``     | nested | nest containing a statistic |
+ +-+-+----------------------------------+--------+-----------------------------+
+ | | ``ETHTOOL_A_STATS_GRP_HIST_RX``    | nested | histogram statistic (Rx)    |
+ +-+------------------------------------+--------+-----------------------------+
+ | | ``ETHTOOL_A_STATS_GRP_HIST_TX``    | nested | histogram statistic (Tx)    |
+ +-+------------------------------------+--------+-----------------------------+
 
 Users specify which groups of statistics they are requesting via
 the ``ETHTOOL_A_STATS_GROUPS`` bitset. Currently defined values are:
@@ -1439,6 +1444,7 @@ the ``ETHTOOL_A_STATS_GROUPS`` bitset. Currently defined values are:
  ETHTOOL_STATS_ETH_PHY  eth-phy  Basic IEEE 802.3 PHY statistics (30.3.2.1.*)
  ETHTOOL_STATS_ETH_CTRL eth-ctrl Basic IEEE 802.3 MAC Ctrl statistics (30.3.3.*)
  ETHTOOL_STATS_RMON     rmon     RMON (RFC 2819) statistics
+ ETHTOOL_STATS_XDP      xdp      XDP statistics
  ====================== ======== ===============================================
 
 Each group should have a corresponding ``ETHTOOL_A_STATS_GRP`` in the reply.
@@ -1451,6 +1457,10 @@ Statistics are added to the ``ETHTOOL_A_STATS_GRP`` nest under
 single 8 byte (u64) attribute inside - the type of that attribute is
 the statistic ID and the value is the value of the statistic.
 Each group has its own interpretation of statistic IDs.
+Statistics can be folded into a consistent (non-broken with any other attr)
+sequence of blocks ``ETHTOOL_A_STATS_GRP_STAT_BLOCK``. This way they are
+treated by Ethtool as per-channel statistics, and are printed with the
+"channel%d-" prefix.
 Attribute IDs correspond to strings from the string set identified
 by ``ETHTOOL_A_STATS_GRP_SS_ID``. Complex statistics (such as RMON histogram
 entries) are also listed inside ``ETHTOOL_A_STATS_GRP`` and do not have
@@ -1479,6 +1489,11 @@ Low and high bounds are inclusive, for example:
  etherStatsPkts512to1023Octets 512  1023
  ============================= ==== ====
 
+Drivers which want to export global (per-device) XDP statistics should
+only implement ``get_xdp_stats`` callback. An additional one
+``get_std_stats_channels`` is needed if the driver exposes per-channel
+statistics.
+
 PHC_VCLOCKS_GET
 ===============
 
-- 
2.31.1