* [RFC net-next 1/6] docs: networking: extend the statistics documentation
2021-04-14 20:23 [RFC net-next 0/6] ethtool: add uAPI for reading standard stats Jakub Kicinski
@ 2021-04-14 20:23 ` Jakub Kicinski
2021-04-14 20:23 ` [RFC net-next 2/6] docs: ethtool: document standard statistics Jakub Kicinski
` (5 subsequent siblings)
6 siblings, 0 replies; 14+ messages in thread
From: Jakub Kicinski @ 2021-04-14 20:23 UTC (permalink / raw)
To: netdev; +Cc: davem, andrew, mkubecek, idosch, Jakub Kicinski
Make the lack of expectations for switching NICs explicit,
describe the new stats.
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
Documentation/networking/statistics.rst | 44 +++++++++++++++++++++++--
1 file changed, 42 insertions(+), 2 deletions(-)
diff --git a/Documentation/networking/statistics.rst b/Documentation/networking/statistics.rst
index b748fe44ee02..5aaa66bba971 100644
--- a/Documentation/networking/statistics.rst
+++ b/Documentation/networking/statistics.rst
@@ -44,8 +44,27 @@ If `-s` is specified once the detailed errors won't be shown.
Protocol-specific statistics
----------------------------
-Some of the interfaces used for configuring devices are also able
-to report related statistics. For example ethtool interface used
+Protocol-specific statistics are exposed via relevant interfaces,
+the same interfaces used to configure them.
+
+ethtool
+~~~~~~~
+
+Ethtool exposes common low-level statistics.
+All the standard statistics are expected to be maintained
+by the device, not the driver (as opposed to driver-defined stats
+described in the next section which mix software and hardware stats).
+For devices which contain unmanaged
+switches (e.g. legacy SR-IOV or multi-host NICs) the events counted
+may not pertain exclusively to the packets destined to
+the local host interface. In other words the events may
+be counted at the network port (MAC/PHY blocks) without separation
+for different host side (PCIe) devices. Such ambiguity must not
+be present when internal switch is managed by Linux (so called
+switchdev mode for NICs).
+
+Standard ethtool statistics can be accessed via the interfaces used
+for configuration. For example ethtool interface used
to configure pause frames can report corresponding hardware counters::
$ ethtool --include-statistics -a eth0
@@ -57,6 +76,27 @@ to report related statistics. For example ethtool interface used
tx_pause_frames: 1
rx_pause_frames: 1
+General Ethernet statistics not associated with any particular
+functionality are exposed via ``ethtool -S $ifc`` by specifying
+the ``--groups`` parameter::
+
+ $ ethtool -S eth0 --groups eth-phy eth-mac eth-ctrl rmon
+ Stats for eth0:
+ eth-phy-SymbolErrorDuringCarrier: 0
+ eth-mac-FramesTransmittedOK: 1
+ eth-mac-FrameTooLongErrors: 1
+ eth-ctrl-MACControlFramesTransmitted: 1
+ eth-ctrl-MACControlFramesReceived: 0
+ eth-ctrl-UnsupportedOpcodesReceived: 1
+ rmon-etherStatsUndersizePkts: 1
+ rmon-etherStatsJabbers: 0
+ rmon-rx-etherStatsPkts64Octets: 1
+ rmon-rx-etherStatsPkts65to127Octets: 0
+ rmon-rx-etherStatsPkts128to255Octets: 0
+ rmon-tx-etherStatsPkts64Octets: 2
+ rmon-tx-etherStatsPkts65to127Octets: 3
+ rmon-tx-etherStatsPkts128to255Octets: 0
+
Driver-defined statistics
-------------------------
--
2.30.2
^ permalink raw reply related [flat|nested] 14+ messages in thread* [RFC net-next 2/6] docs: ethtool: document standard statistics
2021-04-14 20:23 [RFC net-next 0/6] ethtool: add uAPI for reading standard stats Jakub Kicinski
2021-04-14 20:23 ` [RFC net-next 1/6] docs: networking: extend the statistics documentation Jakub Kicinski
@ 2021-04-14 20:23 ` Jakub Kicinski
2021-04-14 20:23 ` [RFC net-next 3/6] ethtool: add a new command for reading standard stats Jakub Kicinski
` (4 subsequent siblings)
6 siblings, 0 replies; 14+ messages in thread
From: Jakub Kicinski @ 2021-04-14 20:23 UTC (permalink / raw)
To: netdev; +Cc: davem, andrew, mkubecek, idosch, Jakub Kicinski
Add documentation for ETHTOOL_MSG_STATS_GET.
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
Documentation/networking/ethtool-netlink.rst | 74 ++++++++++++++++++++
1 file changed, 74 insertions(+)
diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
index f8219e2f489e..086a80eb17be 100644
--- a/Documentation/networking/ethtool-netlink.rst
+++ b/Documentation/networking/ethtool-netlink.rst
@@ -210,6 +210,7 @@ All constants identifying message types use ``ETHTOOL_CMD_`` prefix and suffix
``ETHTOOL_MSG_TUNNEL_INFO_GET`` get tunnel offload info
``ETHTOOL_MSG_FEC_GET`` get FEC settings
``ETHTOOL_MSG_FEC_SET`` set FEC settings
+ ``ETHTOOL_MSG_STATS_GET`` get standard statistics
===================================== ================================
Kernel to userspace:
@@ -246,6 +247,7 @@ All constants identifying message types use ``ETHTOOL_CMD_`` prefix and suffix
``ETHTOOL_MSG_TUNNEL_INFO_GET_REPLY`` tunnel offload info
``ETHTOOL_MSG_FEC_GET_REPLY`` FEC settings
``ETHTOOL_MSG_FEC_NTF`` FEC settings
+ ``ETHTOOL_MSG_STATS_GET_REPLY`` standard statistics
===================================== =================================
``GET`` requests are sent by userspace applications to retrieve device
@@ -1391,6 +1393,78 @@ accessible.
``ETHTOOL_A_MODULE_EEPROM_DATA`` has an attribute length equal to the amount of
bytes driver actually read.
+STATS_GET
+=========
+
+Get standard statistics for the interface. Note that this is not
+a re-implementation of ``ETHTOOL_GSTATS`` which exposed driver-defined
+stats.
+
+Request contents:
+
+ ======================================= ====== ==========================
+ ``ETHTOOL_A_STATS_HEADER`` nested request header
+ ``ETHTOOL_A_STATS_GROUPS`` bitset requested groups of stats
+ ======================================= ====== ==========================
+
+Kernel response contents:
+
+ +---------------------------------+--------+----------------------------------+
+ | ``ETHTOOL_A_STATS_HEADER`` | nested | reply header |
+ +---------------------------------+--------+----------------------------------+
+ | ``ETHTOOL_A_STATS_GRP`` | nested | one or more groups of statistics |
+ +-+-------------------------------+--------+----------------------------------+
+ | | ``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_*`` | u64 | actual statistics |
+ +-+-------------------------------+--------+----------------------------------+
+
+Users specify which groups of statistics they are requesting via
+the ``ETHTOOL_A_STATS_GROUPS`` bitset. Currently defined values are:
+
+ ====================== ======== ===============================================
+ ETHTOOL_STATS_ETH_MAC eth-mac Basic IEEE 802.3 MAC statistics (30.3.1.1.*)
+ 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
+ ====================== ======== ===============================================
+
+Each group should have a corresponding ``ETHTOOL_A_STATS_GRP`` in the reply.
+``ETHTOOL_A_STATS_GRP_ID`` identifies which group's statistics nest contains.
+``ETHTOOL_A_STATS_GRP_SS_ID`` identifies the string set ID for the names of
+the statistics in the group, if available.
+
+Statistics are added directly to the ``ETHTOOL_A_STATS_GRP`` nest. Each group
+has its own interpretation of attribute IDs above
+``ETHTOOL_A_STATS_GRP_FIRST_ATTR``. 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 directly in
+``ETHTOOL_A_STATS_GRP`` and do not have a string defined in the string set.
+
+RMON "histogram" counters count number of packets within given size range.
+Because RFC does not specify the ranges beyond the standard 1518 MTU devices
+differ in definition of buckets. For this reason the definition of packet ranges
+is left to each driver.
+
+ ================================= ====== ===================================
+ ETHTOOL_A_STATS_RMON_HIST nested contains the attributes below
+ ETHTOOL_A_STATS_RMON_HIST_BKT_LOW u32 low bound of the packet size bucket
+ ETHTOOL_A_STATS_RMON_HIST_BKT_HI u32 high bound of the bucket
+ ETHTOOL_A_STATS_RMON_HIST_VAL u64 packet counter
+ ETHTOOL_A_STATS_RMON_HIST_TX nested same as HIST but counting Tx pkts
+ ================================= ====== ===================================
+
+Low and high bounds are inclusive, for example:
+
+ ============================= ==== ====
+ RFC statistic low high
+ ============================= ==== ====
+ etherStatsPkts64Octets 0 64
+ etherStatsPkts512to1023Octets 512 1023
+ ============================= ==== ====
+
Request translation
===================
--
2.30.2
^ permalink raw reply related [flat|nested] 14+ messages in thread* [RFC net-next 3/6] ethtool: add a new command for reading standard stats
2021-04-14 20:23 [RFC net-next 0/6] ethtool: add uAPI for reading standard stats Jakub Kicinski
2021-04-14 20:23 ` [RFC net-next 1/6] docs: networking: extend the statistics documentation Jakub Kicinski
2021-04-14 20:23 ` [RFC net-next 2/6] docs: ethtool: document standard statistics Jakub Kicinski
@ 2021-04-14 20:23 ` Jakub Kicinski
2021-04-14 20:23 ` [RFC net-next 4/6] ethtool: add interface to read standard MAC stats Jakub Kicinski
` (3 subsequent siblings)
6 siblings, 0 replies; 14+ messages in thread
From: Jakub Kicinski @ 2021-04-14 20:23 UTC (permalink / raw)
To: netdev; +Cc: davem, andrew, mkubecek, idosch, Jakub Kicinski
Add an interface for reading standard stats, including
stats which don't have a corresponding control interface.
Start with IEEE 802.3 PHY stats. There seems to be only
one stat to expose there.
Define API to not require user space changes when new
stats or groups are added. Groups are based on bitset,
stats have a string set associated.
Place the stats directly in the group nest. This halves
the space, and while we could go for more encapsulations
it seems unlikely to be needed in practice.
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
include/linux/ethtool.h | 10 ++
include/uapi/linux/ethtool.h | 4 +
include/uapi/linux/ethtool_netlink.h | 44 +++++++
net/ethtool/Makefile | 2 +-
net/ethtool/netlink.c | 10 ++
net/ethtool/netlink.h | 5 +
net/ethtool/stats.c | 166 +++++++++++++++++++++++++++
net/ethtool/strset.c | 10 ++
8 files changed, 250 insertions(+), 1 deletion(-)
create mode 100644 net/ethtool/stats.c
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 112a85b57f1f..2d5455eedbf4 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -250,6 +250,13 @@ static inline void ethtool_stats_init(u64 *stats, unsigned int n)
stats[n] = ETHTOOL_STAT_NOT_SET;
}
+/* Basic IEEE 802.3 PHY statistics (30.3.2.1.*), not otherwise exposed
+ * via a more targeted API.
+ */
+struct ethtool_eth_phy_stats {
+ u64 SymbolErrorDuringCarrier;
+};
+
/**
* struct ethtool_pause_stats - statistics for IEEE 802.3x pause frames
* @tx_pause_frames: transmitted pause frame count. Reported to user space
@@ -487,6 +494,7 @@ struct ethtool_module_eeprom {
* @get_module_eeprom_by_page: Get a region of plug-in module EEPROM data from
* specified page. Returns a negative error code or the amount of bytes
* read.
+ * @get_eth_phy_stats: Query some of the IEEE 802.3 PHY statistics.
*
* All operations are optional (i.e. the function pointer may be set
* to %NULL) and callers must take this into account. Callers must
@@ -597,6 +605,8 @@ struct ethtool_ops {
int (*get_module_eeprom_by_page)(struct net_device *dev,
const struct ethtool_module_eeprom *page,
struct netlink_ext_ack *extack);
+ void (*get_eth_phy_stats)(struct net_device *dev,
+ struct ethtool_eth_phy_stats *phy_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 f91e079e3108..190ae6e03918 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -669,6 +669,8 @@ enum ethtool_link_ext_substate_cable_issue {
* @ETH_SS_TS_TX_TYPES: timestamping Tx types
* @ETH_SS_TS_RX_FILTERS: timestamping Rx filters
* @ETH_SS_UDP_TUNNEL_TYPES: UDP tunnel types
+ * @ETH_SS_STATS_STD: standardized stats
+ * @ETH_SS_STATS_ETH_PHY: names of IEEE 802.3 PHY statistics
*
* @ETH_SS_COUNT: number of defined string sets
*/
@@ -689,6 +691,8 @@ enum ethtool_stringset {
ETH_SS_TS_TX_TYPES,
ETH_SS_TS_RX_FILTERS,
ETH_SS_UDP_TUNNEL_TYPES,
+ ETH_SS_STATS_STD,
+ ETH_SS_STATS_ETH_PHY,
/* 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 3a2b31ccbc5b..e6a473a3a5f1 100644
--- a/include/uapi/linux/ethtool_netlink.h
+++ b/include/uapi/linux/ethtool_netlink.h
@@ -45,6 +45,7 @@ enum {
ETHTOOL_MSG_FEC_GET,
ETHTOOL_MSG_FEC_SET,
ETHTOOL_MSG_MODULE_EEPROM_GET,
+ ETHTOOL_MSG_STATS_GET,
/* add new constants above here */
__ETHTOOL_MSG_USER_CNT,
@@ -86,6 +87,7 @@ enum {
ETHTOOL_MSG_FEC_GET_REPLY,
ETHTOOL_MSG_FEC_NTF,
ETHTOOL_MSG_MODULE_EEPROM_GET_REPLY,
+ ETHTOOL_MSG_STATS_GET_REPLY,
/* add new constants above here */
__ETHTOOL_MSG_KERNEL_CNT,
@@ -679,6 +681,48 @@ enum {
ETHTOOL_A_MODULE_EEPROM_MAX = (__ETHTOOL_A_MODULE_EEPROM_CNT - 1)
};
+/* STATS */
+
+enum {
+ ETHTOOL_A_STATS_UNSPEC,
+ ETHTOOL_A_STATS_PAD,
+ ETHTOOL_A_STATS_HEADER, /* nest - _A_HEADER_* */
+ ETHTOOL_A_STATS_GROUPS, /* bitset */
+
+ ETHTOOL_A_STATS_GRP, /* nest - _A_STATS_GRP_* */
+
+ /* add new constants above here */
+ __ETHTOOL_A_STATS_CNT,
+ ETHTOOL_A_STATS_MAX = (__ETHTOOL_A_STATS_CNT - 1)
+};
+
+enum {
+ ETHTOOL_STATS_ETH_PHY,
+
+ /* add new constants above here */
+ __ETHTOOL_STATS_CNT
+};
+
+enum {
+ ETHTOOL_A_STATS_GRP_UNSPEC,
+ ETHTOOL_A_STATS_GRP_PAD,
+
+ ETHTOOL_A_STATS_GRP_ID, /* u32 */
+ ETHTOOL_A_STATS_GRP_SS_ID, /* u32 */
+
+ /* add new constants above here */
+ ETHTOOL_A_STATS_GRP_FIRST_ATTR = 64,
+};
+
+enum {
+ /* 30.3.2.1.5 aSymbolErrorDuringCarrier */
+ ETHTOOL_A_STATS_ETH_PHY_5_SYM_ERR = ETHTOOL_A_STATS_GRP_FIRST_ATTR,
+
+ /* add new constants above here */
+ __ETHTOOL_A_STATS_ETH_PHY_CNT,
+ ETHTOOL_A_STATS_ETH_PHY_MAX = (__ETHTOOL_A_STATS_ETH_PHY_CNT - 1)
+};
+
/* generic netlink info */
#define ETHTOOL_GENL_NAME "ethtool"
#define ETHTOOL_GENL_VERSION 1
diff --git a/net/ethtool/Makefile b/net/ethtool/Makefile
index 83842685fd8c..723c9a8a8cdf 100644
--- a/net/ethtool/Makefile
+++ b/net/ethtool/Makefile
@@ -7,4 +7,4 @@ obj-$(CONFIG_ETHTOOL_NETLINK) += ethtool_nl.o
ethtool_nl-y := netlink.o bitset.o strset.o linkinfo.o linkmodes.o \
linkstate.o debug.o wol.o features.o privflags.o rings.o \
channels.o coalesce.o pause.o eee.o tsinfo.o cabletest.o \
- tunnels.o fec.o eeprom.o
+ tunnels.o fec.o eeprom.o stats.o
diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index 5f5d7c4b3d4a..290012d0d11d 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -247,6 +247,7 @@ ethnl_default_requests[__ETHTOOL_MSG_USER_CNT] = {
[ETHTOOL_MSG_FEC_GET] = ðnl_fec_request_ops,
[ETHTOOL_MSG_TSINFO_GET] = ðnl_tsinfo_request_ops,
[ETHTOOL_MSG_MODULE_EEPROM_GET] = ðnl_module_eeprom_request_ops,
+ [ETHTOOL_MSG_STATS_GET] = ðnl_stats_request_ops,
};
static struct ethnl_dump_ctx *ethnl_dump_context(struct netlink_callback *cb)
@@ -942,6 +943,15 @@ static const struct genl_ops ethtool_genl_ops[] = {
.policy = ethnl_module_eeprom_get_policy,
.maxattr = ARRAY_SIZE(ethnl_module_eeprom_get_policy) - 1,
},
+ {
+ .cmd = ETHTOOL_MSG_STATS_GET,
+ .doit = ethnl_default_doit,
+ .start = ethnl_default_start,
+ .dumpit = ethnl_default_dumpit,
+ .done = ethnl_default_done,
+ .policy = ethnl_stats_get_policy,
+ .maxattr = ARRAY_SIZE(ethnl_stats_get_policy) - 1,
+ },
};
static const struct genl_multicast_group ethtool_nl_mcgrps[] = {
diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h
index 3f93b3c41c31..79631792313e 100644
--- a/net/ethtool/netlink.h
+++ b/net/ethtool/netlink.h
@@ -346,6 +346,7 @@ extern const struct ethnl_request_ops ethnl_eee_request_ops;
extern const struct ethnl_request_ops ethnl_tsinfo_request_ops;
extern const struct ethnl_request_ops ethnl_fec_request_ops;
extern const struct ethnl_request_ops ethnl_module_eeprom_request_ops;
+extern const struct ethnl_request_ops ethnl_stats_request_ops;
extern const struct nla_policy ethnl_header_policy[ETHTOOL_A_HEADER_FLAGS + 1];
extern const struct nla_policy ethnl_header_policy_stats[ETHTOOL_A_HEADER_FLAGS + 1];
@@ -380,6 +381,7 @@ extern const struct nla_policy ethnl_tunnel_info_get_policy[ETHTOOL_A_TUNNEL_INF
extern const struct nla_policy ethnl_fec_get_policy[ETHTOOL_A_FEC_HEADER + 1];
extern const struct nla_policy ethnl_fec_set_policy[ETHTOOL_A_FEC_AUTO + 1];
extern const struct nla_policy ethnl_module_eeprom_get_policy[ETHTOOL_A_MODULE_EEPROM_DATA + 1];
+extern const struct nla_policy ethnl_stats_get_policy[ETHTOOL_A_STATS_GROUPS + 1];
int ethnl_set_linkinfo(struct sk_buff *skb, struct genl_info *info);
int ethnl_set_linkmodes(struct sk_buff *skb, struct genl_info *info);
@@ -399,4 +401,7 @@ int ethnl_tunnel_info_start(struct netlink_callback *cb);
int ethnl_tunnel_info_dumpit(struct sk_buff *skb, struct netlink_callback *cb);
int ethnl_set_fec(struct sk_buff *skb, struct genl_info *info);
+extern const char stats_std_names[__ETHTOOL_STATS_CNT][ETH_GSTRING_LEN];
+extern const char stats_eth_phy_names[__ETHTOOL_A_STATS_ETH_PHY_CNT][ETH_GSTRING_LEN];
+
#endif /* _NET_ETHTOOL_NETLINK_H */
diff --git a/net/ethtool/stats.c b/net/ethtool/stats.c
new file mode 100644
index 000000000000..68bf6a7614fe
--- /dev/null
+++ b/net/ethtool/stats.c
@@ -0,0 +1,166 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include "netlink.h"
+#include "common.h"
+#include "bitset.h"
+
+struct stats_req_info {
+ struct ethnl_req_info base;
+ DECLARE_BITMAP(stat_mask, __ETHTOOL_STATS_CNT);
+};
+
+#define STATS_REQINFO(__req_base) \
+ container_of(__req_base, struct stats_req_info, base)
+
+struct stats_reply_data {
+ struct ethnl_reply_data base;
+ struct ethtool_eth_phy_stats phy_stats;
+};
+
+#define STATS_REPDATA(__reply_base) \
+ container_of(__reply_base, struct stats_reply_data, base)
+
+const char stats_std_names[__ETHTOOL_STATS_CNT][ETH_GSTRING_LEN] = {
+ [ETHTOOL_STATS_ETH_PHY] = "eth-phy",
+};
+
+const char stats_eth_phy_names[__ETHTOOL_A_STATS_ETH_PHY_CNT][ETH_GSTRING_LEN] = {
+ [ETHTOOL_A_STATS_ETH_PHY_5_SYM_ERR] = "SymbolErrorDuringCarrier",
+};
+
+const struct nla_policy ethnl_stats_get_policy[ETHTOOL_A_STATS_GROUPS + 1] = {
+ [ETHTOOL_A_STATS_HEADER] =
+ NLA_POLICY_NESTED(ethnl_header_policy),
+ [ETHTOOL_A_STATS_GROUPS] = { .type = NLA_NESTED },
+};
+
+static int stats_parse_request(struct ethnl_req_info *req_base,
+ struct nlattr **tb,
+ struct netlink_ext_ack *extack)
+{
+ struct stats_req_info *req_info = STATS_REQINFO(req_base);
+ bool mod = false;
+ int err;
+
+ err = ethnl_update_bitset(req_info->stat_mask, __ETHTOOL_STATS_CNT,
+ tb[ETHTOOL_A_STATS_GROUPS], stats_std_names,
+ extack, &mod);
+ if (err)
+ return err;
+
+ if (!mod) {
+ NL_SET_ERR_MSG(extack, "no stats requested");
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int stats_prepare_data(const struct ethnl_req_info *req_base,
+ struct ethnl_reply_data *reply_base,
+ struct genl_info *info)
+{
+ 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;
+ int ret;
+
+ ret = ethnl_ops_begin(dev);
+ if (ret < 0)
+ return ret;
+
+ memset(&data->phy_stats, 0xff, sizeof(data->phy_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);
+
+ ethnl_ops_complete(dev);
+ return 0;
+}
+
+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);
+ int len = 0;
+
+ if (test_bit(ETHTOOL_STATS_ETH_PHY, req_info->stat_mask))
+ len += nla_total_size(0) +
+ sizeof(struct ethtool_eth_phy_stats) / sizeof(u64) *
+ nla_total_size_64bit(sizeof(u64));
+
+ return len;
+}
+
+static int stat_put(struct sk_buff *skb, u16 attrtype, u64 val)
+{
+ if (val == ETHTOOL_STAT_NOT_SET)
+ return 0;
+ return nla_put_u64_64bit(skb, attrtype, val, ETHTOOL_A_STATS_GRP_PAD);
+}
+
+static int stats_put_phy_stats(struct sk_buff *skb,
+ const struct stats_reply_data *data)
+{
+ if (stat_put(skb, ETHTOOL_A_STATS_ETH_PHY_5_SYM_ERR,
+ data->phy_stats.SymbolErrorDuringCarrier))
+ return -EMSGSIZE;
+ return 0;
+}
+
+static int stats_put_stats(struct sk_buff *skb,
+ const struct stats_reply_data *data,
+ u32 id, u32 ss_id,
+ int (*cb)(struct sk_buff *skb,
+ const struct stats_reply_data *data))
+{
+ struct nlattr *nest;
+
+ nest = nla_nest_start(skb, ETHTOOL_A_STATS_GRP);
+ if (!nest)
+ return -EMSGSIZE;
+
+ if (nla_put_u32(skb, ETHTOOL_A_STATS_GRP_ID, id) ||
+ nla_put_u32(skb, ETHTOOL_A_STATS_GRP_SS_ID, ss_id))
+ goto err_cancel;
+
+ if (cb(skb, data))
+ goto err_cancel;
+
+ nla_nest_end(skb, nest);
+ return 0;
+
+err_cancel:
+ nla_nest_cancel(skb, nest);
+ return -EMSGSIZE;
+}
+
+static int stats_fill_reply(struct sk_buff *skb,
+ 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);
+ int ret = 0;
+
+ if (!ret && test_bit(ETHTOOL_STATS_ETH_PHY, req_info->stat_mask))
+ ret = stats_put_stats(skb, data, ETHTOOL_STATS_ETH_PHY,
+ ETH_SS_STATS_ETH_PHY,
+ stats_put_phy_stats);
+
+ return ret;
+}
+
+const struct ethnl_request_ops ethnl_stats_request_ops = {
+ .request_cmd = ETHTOOL_MSG_STATS_GET,
+ .reply_cmd = ETHTOOL_MSG_STATS_GET_REPLY,
+ .hdr_attr = ETHTOOL_A_STATS_HEADER,
+ .req_info_size = sizeof(struct stats_req_info),
+ .reply_data_size = sizeof(struct stats_reply_data),
+
+ .parse_request = stats_parse_request,
+ .prepare_data = stats_prepare_data,
+ .reply_size = stats_reply_size,
+ .fill_reply = stats_fill_reply,
+};
diff --git a/net/ethtool/strset.c b/net/ethtool/strset.c
index c3a5489964cd..5f3c73587ff4 100644
--- a/net/ethtool/strset.c
+++ b/net/ethtool/strset.c
@@ -80,6 +80,16 @@ static const struct strset_info info_template[] = {
.count = __ETHTOOL_UDP_TUNNEL_TYPE_CNT,
.strings = udp_tunnel_type_names,
},
+ [ETH_SS_STATS_STD] = {
+ .per_dev = false,
+ .count = __ETHTOOL_STATS_CNT,
+ .strings = stats_std_names,
+ },
+ [ETH_SS_STATS_ETH_PHY] = {
+ .per_dev = false,
+ .count = __ETHTOOL_A_STATS_ETH_PHY_CNT,
+ .strings = stats_eth_phy_names,
+ },
};
struct strset_req_info {
--
2.30.2
^ permalink raw reply related [flat|nested] 14+ messages in thread* [RFC net-next 4/6] ethtool: add interface to read standard MAC stats
2021-04-14 20:23 [RFC net-next 0/6] ethtool: add uAPI for reading standard stats Jakub Kicinski
` (2 preceding siblings ...)
2021-04-14 20:23 ` [RFC net-next 3/6] ethtool: add a new command for reading standard stats Jakub Kicinski
@ 2021-04-14 20:23 ` Jakub Kicinski
2021-04-15 6:12 ` Saeed Mahameed
2021-04-14 20:23 ` [RFC net-next 5/6] ethtool: add interface to read standard MAC Ctrl stats Jakub Kicinski
` (2 subsequent siblings)
6 siblings, 1 reply; 14+ messages in thread
From: Jakub Kicinski @ 2021-04-14 20:23 UTC (permalink / raw)
To: netdev; +Cc: davem, andrew, mkubecek, idosch, Jakub Kicinski
Most of the MAC statistics are included in
struct rtnl_link_stats64, but some fields
are aggregated. Besides it's good to expose
these clearly hardware stats separately.
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
include/linux/ethtool.h | 31 ++++++++++
include/uapi/linux/ethtool.h | 2 +
include/uapi/linux/ethtool_netlink.h | 53 ++++++++++++++++
net/ethtool/netlink.h | 1 +
net/ethtool/stats.c | 90 ++++++++++++++++++++++++++++
net/ethtool/strset.c | 5 ++
6 files changed, 182 insertions(+)
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 2d5455eedbf4..3c689a13e679 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -250,6 +250,34 @@ static inline void ethtool_stats_init(u64 *stats, unsigned int n)
stats[n] = ETHTOOL_STAT_NOT_SET;
}
+/* Basic IEEE 802.3 MAC statistics (30.3.1.1.*), not otherwise exposed
+ * via a more targeted API.
+ */
+struct ethtool_eth_mac_stats {
+ u64 FramesTransmittedOK;
+ u64 SingleCollisionFrames;
+ u64 MultipleCollisionFrames;
+ u64 FramesReceivedOK;
+ u64 FrameCheckSequenceErrors;
+ u64 AlignmentErrors;
+ u64 OctetsTransmittedOK;
+ u64 FramesWithDeferredXmissions;
+ u64 LateCollisions;
+ u64 FramesAbortedDueToXSColls;
+ u64 FramesLostDueToIntMACXmitError;
+ u64 CarrierSenseErrors;
+ u64 OctetsReceivedOK;
+ u64 FramesLostDueToIntMACRcvError;
+ u64 MulticastFramesXmittedOK;
+ u64 BroadcastFramesXmittedOK;
+ u64 FramesWithExcessiveDeferral;
+ u64 MulticastFramesReceivedOK;
+ u64 BroadcastFramesReceivedOK;
+ u64 InRangeLengthErrors;
+ u64 OutOfRangeLengthField;
+ u64 FrameTooLongErrors;
+};
+
/* Basic IEEE 802.3 PHY statistics (30.3.2.1.*), not otherwise exposed
* via a more targeted API.
*/
@@ -495,6 +523,7 @@ struct ethtool_module_eeprom {
* specified page. Returns a negative error code or the amount of bytes
* read.
* @get_eth_phy_stats: Query some of the IEEE 802.3 PHY statistics.
+ * @get_eth_mac_stats: Query some of the IEEE 802.3 MAC statistics.
*
* All operations are optional (i.e. the function pointer may be set
* to %NULL) and callers must take this into account. Callers must
@@ -607,6 +636,8 @@ struct ethtool_ops {
struct netlink_ext_ack *extack);
void (*get_eth_phy_stats)(struct net_device *dev,
struct ethtool_eth_phy_stats *phy_stats);
+ void (*get_eth_mac_stats)(struct net_device *dev,
+ struct ethtool_eth_mac_stats *mac_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 190ae6e03918..c227376d811a 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -671,6 +671,7 @@ enum ethtool_link_ext_substate_cable_issue {
* @ETH_SS_UDP_TUNNEL_TYPES: UDP tunnel types
* @ETH_SS_STATS_STD: standardized stats
* @ETH_SS_STATS_ETH_PHY: names of IEEE 802.3 PHY statistics
+ * @ETH_SS_STATS_ETH_MAC: names of IEEE 802.3 MAC statistics
*
* @ETH_SS_COUNT: number of defined string sets
*/
@@ -693,6 +694,7 @@ enum ethtool_stringset {
ETH_SS_UDP_TUNNEL_TYPES,
ETH_SS_STATS_STD,
ETH_SS_STATS_ETH_PHY,
+ ETH_SS_STATS_ETH_MAC,
/* 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 e6a473a3a5f1..7ef6fbe237d9 100644
--- a/include/uapi/linux/ethtool_netlink.h
+++ b/include/uapi/linux/ethtool_netlink.h
@@ -698,6 +698,7 @@ enum {
enum {
ETHTOOL_STATS_ETH_PHY,
+ ETHTOOL_STATS_ETH_MAC,
/* add new constants above here */
__ETHTOOL_STATS_CNT
@@ -723,6 +724,58 @@ enum {
ETHTOOL_A_STATS_ETH_PHY_MAX = (__ETHTOOL_A_STATS_ETH_PHY_CNT - 1)
};
+enum {
+ /* 30.3.1.1.2 aFramesTransmittedOK */
+ ETHTOOL_A_STATS_ETH_MAC_2_TX_PKT = ETHTOOL_A_STATS_GRP_FIRST_ATTR,
+ /* 30.3.1.1.3 aSingleCollisionFrames */
+ ETHTOOL_A_STATS_ETH_MAC_3_SINGLE_COL,
+ /* 30.3.1.1.4 aMultipleCollisionFrames */
+ ETHTOOL_A_STATS_ETH_MAC_4_MULTI_COL,
+ /* 30.3.1.1.5 aFramesReceivedOK */
+ ETHTOOL_A_STATS_ETH_MAC_5_RX_PKT,
+ /* 30.3.1.1.6 aFrameCheckSequenceErrors */
+ ETHTOOL_A_STATS_ETH_MAC_6_FCS_ERR,
+ /* 30.3.1.1.7 aAlignmentErrors */
+ ETHTOOL_A_STATS_ETH_MAC_7_ALIGN_ERR,
+ /* 30.3.1.1.8 aOctetsTransmittedOK */
+ ETHTOOL_A_STATS_ETH_MAC_8_TX_BYTES,
+ /* 30.3.1.1.9 aFramesWithDeferredXmissions */
+ ETHTOOL_A_STATS_ETH_MAC_9_TX_DEFER,
+ /* 30.3.1.1.10 aLateCollisions */
+ ETHTOOL_A_STATS_ETH_MAC_10_LATE_COL,
+ /* 30.3.1.1.11 aFramesAbortedDueToXSColls */
+ ETHTOOL_A_STATS_ETH_MAC_11_XS_COL,
+ /* 30.3.1.1.12 aFramesLostDueToIntMACXmitError */
+ ETHTOOL_A_STATS_ETH_MAC_12_TX_INT_ERR,
+ /* 30.3.1.1.13 aCarrierSenseErrors */
+ ETHTOOL_A_STATS_ETH_MAC_13_CS_ERR,
+ /* 30.3.1.1.14 aOctetsReceivedOK */
+ ETHTOOL_A_STATS_ETH_MAC_14_RX_BYTES,
+ /* 30.3.1.1.15 aFramesLostDueToIntMACRcvError */
+ ETHTOOL_A_STATS_ETH_MAC_15_RX_INT_ERR,
+
+ /* 30.3.1.1.18 aMulticastFramesXmittedOK */
+ ETHTOOL_A_STATS_ETH_MAC_18_TX_MCAST,
+ /* 30.3.1.1.19 aBroadcastFramesXmittedOK */
+ ETHTOOL_A_STATS_ETH_MAC_19_TX_BCAST,
+ /* 30.3.1.1.20 aFramesWithExcessiveDeferral */
+ ETHTOOL_A_STATS_ETH_MAC_20_XS_DEFER,
+ /* 30.3.1.1.21 aMulticastFramesReceivedOK */
+ ETHTOOL_A_STATS_ETH_MAC_21_RX_MCAST,
+ /* 30.3.1.1.22 aBroadcastFramesReceivedOK */
+ ETHTOOL_A_STATS_ETH_MAC_22_RX_BCAST,
+ /* 30.3.1.1.23 aInRangeLengthErrors */
+ ETHTOOL_A_STATS_ETH_MAC_23_IR_LEN_ERR,
+ /* 30.3.1.1.24 aOutOfRangeLengthField */
+ ETHTOOL_A_STATS_ETH_MAC_24_OOR_LEN,
+ /* 30.3.1.1.25 aFrameTooLongErrors */
+ ETHTOOL_A_STATS_ETH_MAC_25_TOO_LONG_ERR,
+
+ /* add new constants above here */
+ __ETHTOOL_A_STATS_ETH_MAC_CNT,
+ ETHTOOL_A_STATS_ETH_MAC_MAX = (__ETHTOOL_A_STATS_ETH_MAC_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 79631792313e..9c5f6ee71864 100644
--- a/net/ethtool/netlink.h
+++ b/net/ethtool/netlink.h
@@ -403,5 +403,6 @@ int ethnl_set_fec(struct sk_buff *skb, struct genl_info *info);
extern const char stats_std_names[__ETHTOOL_STATS_CNT][ETH_GSTRING_LEN];
extern const char stats_eth_phy_names[__ETHTOOL_A_STATS_ETH_PHY_CNT][ETH_GSTRING_LEN];
+extern const char stats_eth_mac_names[__ETHTOOL_A_STATS_ETH_MAC_CNT][ETH_GSTRING_LEN];
#endif /* _NET_ETHTOOL_NETLINK_H */
diff --git a/net/ethtool/stats.c b/net/ethtool/stats.c
index 68bf6a7614fe..e0395d5c0f9d 100644
--- a/net/ethtool/stats.c
+++ b/net/ethtool/stats.c
@@ -15,6 +15,7 @@ struct stats_req_info {
struct stats_reply_data {
struct ethnl_reply_data base;
struct ethtool_eth_phy_stats phy_stats;
+ struct ethtool_eth_mac_stats mac_stats;
};
#define STATS_REPDATA(__reply_base) \
@@ -22,12 +23,38 @@ struct stats_reply_data {
const char stats_std_names[__ETHTOOL_STATS_CNT][ETH_GSTRING_LEN] = {
[ETHTOOL_STATS_ETH_PHY] = "eth-phy",
+ [ETHTOOL_STATS_ETH_MAC] = "eth-mac",
};
const char stats_eth_phy_names[__ETHTOOL_A_STATS_ETH_PHY_CNT][ETH_GSTRING_LEN] = {
[ETHTOOL_A_STATS_ETH_PHY_5_SYM_ERR] = "SymbolErrorDuringCarrier",
};
+const char stats_eth_mac_names[__ETHTOOL_A_STATS_ETH_MAC_CNT][ETH_GSTRING_LEN] = {
+ [ETHTOOL_A_STATS_ETH_MAC_2_TX_PKT] = "FramesTransmittedOK",
+ [ETHTOOL_A_STATS_ETH_MAC_3_SINGLE_COL] = "SingleCollisionFrames",
+ [ETHTOOL_A_STATS_ETH_MAC_4_MULTI_COL] = "MultipleCollisionFrames",
+ [ETHTOOL_A_STATS_ETH_MAC_5_RX_PKT] = "FramesReceivedOK",
+ [ETHTOOL_A_STATS_ETH_MAC_6_FCS_ERR] = "FrameCheckSequenceErrors",
+ [ETHTOOL_A_STATS_ETH_MAC_7_ALIGN_ERR] = "AlignmentErrors",
+ [ETHTOOL_A_STATS_ETH_MAC_8_TX_BYTES] = "OctetsTransmittedOK",
+ [ETHTOOL_A_STATS_ETH_MAC_9_TX_DEFER] = "FramesWithDeferredXmissions",
+ [ETHTOOL_A_STATS_ETH_MAC_10_LATE_COL] = "LateCollisions",
+ [ETHTOOL_A_STATS_ETH_MAC_11_XS_COL] = "FramesAbortedDueToXSColls",
+ [ETHTOOL_A_STATS_ETH_MAC_12_TX_INT_ERR] = "FramesLostDueToIntMACXmitError",
+ [ETHTOOL_A_STATS_ETH_MAC_13_CS_ERR] = "CarrierSenseErrors",
+ [ETHTOOL_A_STATS_ETH_MAC_14_RX_BYTES] = "OctetsReceivedOK",
+ [ETHTOOL_A_STATS_ETH_MAC_15_RX_INT_ERR] = "FramesLostDueToIntMACRcvError",
+ [ETHTOOL_A_STATS_ETH_MAC_18_TX_MCAST] = "MulticastFramesXmittedOK",
+ [ETHTOOL_A_STATS_ETH_MAC_19_TX_BCAST] = "BroadcastFramesXmittedOK",
+ [ETHTOOL_A_STATS_ETH_MAC_20_XS_DEFER] = "FramesWithExcessiveDeferral",
+ [ETHTOOL_A_STATS_ETH_MAC_21_RX_MCAST] = "MulticastFramesReceivedOK",
+ [ETHTOOL_A_STATS_ETH_MAC_22_RX_BCAST] = "BroadcastFramesReceivedOK",
+ [ETHTOOL_A_STATS_ETH_MAC_23_IR_LEN_ERR] = "InRangeLengthErrors",
+ [ETHTOOL_A_STATS_ETH_MAC_24_OOR_LEN] = "OutOfRangeLengthField",
+ [ETHTOOL_A_STATS_ETH_MAC_25_TOO_LONG_ERR] = "FrameTooLongErrors",
+};
+
const struct nla_policy ethnl_stats_get_policy[ETHTOOL_A_STATS_GROUPS + 1] = {
[ETHTOOL_A_STATS_HEADER] =
NLA_POLICY_NESTED(ethnl_header_policy),
@@ -70,10 +97,14 @@ static int stats_prepare_data(const struct ethnl_req_info *req_base,
return ret;
memset(&data->phy_stats, 0xff, sizeof(data->phy_stats));
+ memset(&data->mac_stats, 0xff, sizeof(data->mac_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);
+ 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);
ethnl_ops_complete(dev);
return 0;
@@ -89,6 +120,10 @@ static int stats_reply_size(const struct ethnl_req_info *req_base,
len += nla_total_size(0) +
sizeof(struct ethtool_eth_phy_stats) / sizeof(u64) *
nla_total_size_64bit(sizeof(u64));
+ if (test_bit(ETHTOOL_STATS_ETH_MAC, req_info->stat_mask))
+ len += nla_total_size(0) +
+ sizeof(struct ethtool_eth_mac_stats) / sizeof(u64) *
+ nla_total_size_64bit(sizeof(u64));
return len;
}
@@ -109,6 +144,57 @@ static int stats_put_phy_stats(struct sk_buff *skb,
return 0;
}
+static int stats_put_mac_stats(struct sk_buff *skb,
+ const struct stats_reply_data *data)
+{
+ if (stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_2_TX_PKT,
+ data->mac_stats.FramesTransmittedOK) ||
+ stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_3_SINGLE_COL,
+ data->mac_stats.SingleCollisionFrames) ||
+ stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_4_MULTI_COL,
+ data->mac_stats.MultipleCollisionFrames) ||
+ stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_5_RX_PKT,
+ data->mac_stats.FramesReceivedOK) ||
+ stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_6_FCS_ERR,
+ data->mac_stats.FrameCheckSequenceErrors) ||
+ stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_7_ALIGN_ERR,
+ data->mac_stats.AlignmentErrors) ||
+ stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_8_TX_BYTES,
+ data->mac_stats.OctetsTransmittedOK) ||
+ stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_9_TX_DEFER,
+ data->mac_stats.FramesWithDeferredXmissions) ||
+ stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_10_LATE_COL,
+ data->mac_stats.LateCollisions) ||
+ stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_11_XS_COL,
+ data->mac_stats.FramesAbortedDueToXSColls) ||
+ stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_12_TX_INT_ERR,
+ data->mac_stats.FramesLostDueToIntMACXmitError) ||
+ stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_13_CS_ERR,
+ data->mac_stats.CarrierSenseErrors) ||
+ stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_14_RX_BYTES,
+ data->mac_stats.OctetsReceivedOK) ||
+ stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_15_RX_INT_ERR,
+ data->mac_stats.FramesLostDueToIntMACRcvError) ||
+ stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_18_TX_MCAST,
+ data->mac_stats.MulticastFramesXmittedOK) ||
+ stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_19_TX_BCAST,
+ data->mac_stats.BroadcastFramesXmittedOK) ||
+ stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_20_XS_DEFER,
+ data->mac_stats.FramesWithExcessiveDeferral) ||
+ stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_21_RX_MCAST,
+ data->mac_stats.MulticastFramesReceivedOK) ||
+ stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_22_RX_BCAST,
+ data->mac_stats.BroadcastFramesReceivedOK) ||
+ stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_23_IR_LEN_ERR,
+ data->mac_stats.InRangeLengthErrors) ||
+ stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_24_OOR_LEN,
+ data->mac_stats.OutOfRangeLengthField) ||
+ stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_25_TOO_LONG_ERR,
+ data->mac_stats.FrameTooLongErrors))
+ return -EMSGSIZE;
+ return 0;
+}
+
static int stats_put_stats(struct sk_buff *skb,
const struct stats_reply_data *data,
u32 id, u32 ss_id,
@@ -148,6 +234,10 @@ static int stats_fill_reply(struct sk_buff *skb,
ret = stats_put_stats(skb, data, ETHTOOL_STATS_ETH_PHY,
ETH_SS_STATS_ETH_PHY,
stats_put_phy_stats);
+ if (!ret && test_bit(ETHTOOL_STATS_ETH_MAC, req_info->stat_mask))
+ ret = stats_put_stats(skb, data, ETHTOOL_STATS_ETH_MAC,
+ ETH_SS_STATS_ETH_MAC,
+ stats_put_mac_stats);
return ret;
}
diff --git a/net/ethtool/strset.c b/net/ethtool/strset.c
index 5f3c73587ff4..a8aac7bcfcc9 100644
--- a/net/ethtool/strset.c
+++ b/net/ethtool/strset.c
@@ -90,6 +90,11 @@ static const struct strset_info info_template[] = {
.count = __ETHTOOL_A_STATS_ETH_PHY_CNT,
.strings = stats_eth_phy_names,
},
+ [ETH_SS_STATS_ETH_MAC] = {
+ .per_dev = false,
+ .count = __ETHTOOL_A_STATS_ETH_MAC_CNT,
+ .strings = stats_eth_mac_names,
+ },
};
struct strset_req_info {
--
2.30.2
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [RFC net-next 4/6] ethtool: add interface to read standard MAC stats
2021-04-14 20:23 ` [RFC net-next 4/6] ethtool: add interface to read standard MAC stats Jakub Kicinski
@ 2021-04-15 6:12 ` Saeed Mahameed
2021-04-15 15:38 ` Jakub Kicinski
0 siblings, 1 reply; 14+ messages in thread
From: Saeed Mahameed @ 2021-04-15 6:12 UTC (permalink / raw)
To: Jakub Kicinski, netdev; +Cc: davem, andrew, mkubecek, idosch
On Wed, 2021-04-14 at 13:23 -0700, Jakub Kicinski wrote:
> Most of the MAC statistics are included in
> struct rtnl_link_stats64, but some fields
> are aggregated. Besides it's good to expose
> these clearly hardware stats separately.
>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> include/linux/ethtool.h | 31 ++++++++++
> include/uapi/linux/ethtool.h | 2 +
> include/uapi/linux/ethtool_netlink.h | 53 ++++++++++++++++
> net/ethtool/netlink.h | 1 +
> net/ethtool/stats.c | 90 ++++++++++++++++++++++++++++
> net/ethtool/strset.c | 5 ++
> 6 files changed, 182 insertions(+)
>
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index 2d5455eedbf4..3c689a13e679 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -250,6 +250,34 @@ static inline void ethtool_stats_init(u64 *stats,
> unsigned int n)
> stats[n] = ETHTOOL_STAT_NOT_SET;
> }
>
> +/* Basic IEEE 802.3 MAC statistics (30.3.1.1.*), not otherwise exposed
> + * via a more targeted API.
> + */
> +struct ethtool_eth_mac_stats {
> + u64 FramesTransmittedOK;
> + u64 SingleCollisionFrames;
> + u64 MultipleCollisionFrames;
> + u64 FramesReceivedOK;
> + u64 FrameCheckSequenceErrors;
> + u64 AlignmentErrors;
> + u64 OctetsTransmittedOK;
> + u64 FramesWithDeferredXmissions;
> + u64 LateCollisions;
> + u64 FramesAbortedDueToXSColls;
> + u64 FramesLostDueToIntMACXmitError;
> + u64 CarrierSenseErrors;
> + u64 OctetsReceivedOK;
> + u64 FramesLostDueToIntMACRcvError;
> + u64 MulticastFramesXmittedOK;
> + u64 BroadcastFramesXmittedOK;
> + u64 FramesWithExcessiveDeferral;
> + u64 MulticastFramesReceivedOK;
> + u64 BroadcastFramesReceivedOK;
> + u64 InRangeLengthErrors;
> + u64 OutOfRangeLengthField;
> + u64 FrameTooLongErrors;
> +};
> +
> /* Basic IEEE 802.3 PHY statistics (30.3.2.1.*), not otherwise exposed
> * via a more targeted API.
> */
> @@ -495,6 +523,7 @@ struct ethtool_module_eeprom {
> * specified page. Returns a negative error code or the amount of
> bytes
> * read.
> * @get_eth_phy_stats: Query some of the IEEE 802.3 PHY statistics.
> + * @get_eth_mac_stats: Query some of the IEEE 802.3 MAC statistics.
> *
> * All operations are optional (i.e. the function pointer may be set
> * to %NULL) and callers must take this into account. Callers must
> @@ -607,6 +636,8 @@ struct ethtool_ops {
> struct netlink_ext_ack
> *extack);
> void (*get_eth_phy_stats)(struct net_device *dev,
> struct ethtool_eth_phy_stats
> *phy_stats);
> + void (*get_eth_mac_stats)(struct net_device *dev,
> + struct ethtool_eth_mac_stats
> *mac_stats);
too many callbacks.. I understand the point of having explicit structs
per stats group, but it can be achievable with one generic ethtool
calback function with the help of a flexible struct:
void (*get_std_stats)(struct net_device *dev, struct *std_stats)
union stats_groups {
struct ethtool_eth_phy_stats eth_phy;
struct ethtool_eth_mac_stats eth_mac;
...
}
struct std_stats {
u16 type;
union stats_groups stats[0];
}
where std_stats.stats is allocated dynamically according to
std_stats.type
and driver can just access the corresponding stats according to type
e.g:
std_stats.stats.eth_phy
> };
>
> int ethtool_check_ops(const struct ethtool_ops *ops);
> diff --git a/include/uapi/linux/ethtool.h
> b/include/uapi/linux/ethtool.h
> index 190ae6e03918..c227376d811a 100644
> --- a/include/uapi/linux/ethtool.h
> +++ b/include/uapi/linux/ethtool.h
> @@ -671,6 +671,7 @@ enum ethtool_link_ext_substate_cable_issue {
> * @ETH_SS_UDP_TUNNEL_TYPES: UDP tunnel types
> * @ETH_SS_STATS_STD: standardized stats
> * @ETH_SS_STATS_ETH_PHY: names of IEEE 802.3 PHY statistics
> + * @ETH_SS_STATS_ETH_MAC: names of IEEE 802.3 MAC statistics
> *
> * @ETH_SS_COUNT: number of defined string sets
> */
> @@ -693,6 +694,7 @@ enum ethtool_stringset {
> ETH_SS_UDP_TUNNEL_TYPES,
> ETH_SS_STATS_STD,
> ETH_SS_STATS_ETH_PHY,
> + ETH_SS_STATS_ETH_MAC,
>
> /* 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 e6a473a3a5f1..7ef6fbe237d9 100644
> --- a/include/uapi/linux/ethtool_netlink.h
> +++ b/include/uapi/linux/ethtool_netlink.h
> @@ -698,6 +698,7 @@ enum {
>
> enum {
> ETHTOOL_STATS_ETH_PHY,
> + ETHTOOL_STATS_ETH_MAC,
>
> /* add new constants above here */
> __ETHTOOL_STATS_CNT
> @@ -723,6 +724,58 @@ enum {
> ETHTOOL_A_STATS_ETH_PHY_MAX = (__ETHTOOL_A_STATS_ETH_PHY_CNT -
> 1)
> };
>
> +enum {
> + /* 30.3.1.1.2 aFramesTransmittedOK */
> + ETHTOOL_A_STATS_ETH_MAC_2_TX_PKT =
> ETHTOOL_A_STATS_GRP_FIRST_ATTR,
> + /* 30.3.1.1.3 aSingleCollisionFrames */
> + ETHTOOL_A_STATS_ETH_MAC_3_SINGLE_COL,
> + /* 30.3.1.1.4 aMultipleCollisionFrames */
> + ETHTOOL_A_STATS_ETH_MAC_4_MULTI_COL,
> + /* 30.3.1.1.5 aFramesReceivedOK */
> + ETHTOOL_A_STATS_ETH_MAC_5_RX_PKT,
> + /* 30.3.1.1.6 aFrameCheckSequenceErrors */
> + ETHTOOL_A_STATS_ETH_MAC_6_FCS_ERR,
> + /* 30.3.1.1.7 aAlignmentErrors */
> + ETHTOOL_A_STATS_ETH_MAC_7_ALIGN_ERR,
> + /* 30.3.1.1.8 aOctetsTransmittedOK */
> + ETHTOOL_A_STATS_ETH_MAC_8_TX_BYTES,
> + /* 30.3.1.1.9 aFramesWithDeferredXmissions */
> + ETHTOOL_A_STATS_ETH_MAC_9_TX_DEFER,
> + /* 30.3.1.1.10 aLateCollisions */
> + ETHTOOL_A_STATS_ETH_MAC_10_LATE_COL,
> + /* 30.3.1.1.11 aFramesAbortedDueToXSColls */
> + ETHTOOL_A_STATS_ETH_MAC_11_XS_COL,
> + /* 30.3.1.1.12 aFramesLostDueToIntMACXmitError */
> + ETHTOOL_A_STATS_ETH_MAC_12_TX_INT_ERR,
> + /* 30.3.1.1.13 aCarrierSenseErrors */
> + ETHTOOL_A_STATS_ETH_MAC_13_CS_ERR,
> + /* 30.3.1.1.14 aOctetsReceivedOK */
> + ETHTOOL_A_STATS_ETH_MAC_14_RX_BYTES,
> + /* 30.3.1.1.15 aFramesLostDueToIntMACRcvError */
> + ETHTOOL_A_STATS_ETH_MAC_15_RX_INT_ERR,
> +
> + /* 30.3.1.1.18 aMulticastFramesXmittedOK */
> + ETHTOOL_A_STATS_ETH_MAC_18_TX_MCAST,
> + /* 30.3.1.1.19 aBroadcastFramesXmittedOK */
> + ETHTOOL_A_STATS_ETH_MAC_19_TX_BCAST,
> + /* 30.3.1.1.20 aFramesWithExcessiveDeferral */
> + ETHTOOL_A_STATS_ETH_MAC_20_XS_DEFER,
> + /* 30.3.1.1.21 aMulticastFramesReceivedOK */
> + ETHTOOL_A_STATS_ETH_MAC_21_RX_MCAST,
> + /* 30.3.1.1.22 aBroadcastFramesReceivedOK */
> + ETHTOOL_A_STATS_ETH_MAC_22_RX_BCAST,
> + /* 30.3.1.1.23 aInRangeLengthErrors */
> + ETHTOOL_A_STATS_ETH_MAC_23_IR_LEN_ERR,
> + /* 30.3.1.1.24 aOutOfRangeLengthField */
> + ETHTOOL_A_STATS_ETH_MAC_24_OOR_LEN,
> + /* 30.3.1.1.25 aFrameTooLongErrors */
> + ETHTOOL_A_STATS_ETH_MAC_25_TOO_LONG_ERR,
> +
> + /* add new constants above here */
> + __ETHTOOL_A_STATS_ETH_MAC_CNT,
> + ETHTOOL_A_STATS_ETH_MAC_MAX = (__ETHTOOL_A_STATS_ETH_MAC_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 79631792313e..9c5f6ee71864 100644
> --- a/net/ethtool/netlink.h
> +++ b/net/ethtool/netlink.h
> @@ -403,5 +403,6 @@ int ethnl_set_fec(struct sk_buff *skb, struct
> genl_info *info);
>
> extern const char
> stats_std_names[__ETHTOOL_STATS_CNT][ETH_GSTRING_LEN];
> extern const char
> stats_eth_phy_names[__ETHTOOL_A_STATS_ETH_PHY_CNT][ETH_GSTRING_LEN];
> +extern const char
> stats_eth_mac_names[__ETHTOOL_A_STATS_ETH_MAC_CNT][ETH_GSTRING_LEN];
>
> #endif /* _NET_ETHTOOL_NETLINK_H */
> diff --git a/net/ethtool/stats.c b/net/ethtool/stats.c
> index 68bf6a7614fe..e0395d5c0f9d 100644
> --- a/net/ethtool/stats.c
> +++ b/net/ethtool/stats.c
> @@ -15,6 +15,7 @@ struct stats_req_info {
> struct stats_reply_data {
> struct ethnl_reply_data base;
> struct ethtool_eth_phy_stats phy_stats;
> + struct ethtool_eth_mac_stats mac_stats;
> };
>
> #define STATS_REPDATA(__reply_base) \
> @@ -22,12 +23,38 @@ struct stats_reply_data {
>
> const char stats_std_names[__ETHTOOL_STATS_CNT][ETH_GSTRING_LEN] = {
> [ETHTOOL_STATS_ETH_PHY] = "eth-phy",
> + [ETHTOOL_STATS_ETH_MAC] = "eth-mac",
> };
>
> const char
> stats_eth_phy_names[__ETHTOOL_A_STATS_ETH_PHY_CNT][ETH_GSTRING_LEN] = {
> [ETHTOOL_A_STATS_ETH_PHY_5_SYM_ERR] =
> "SymbolErrorDuringCarrier",
> };
>
> +const char
> stats_eth_mac_names[__ETHTOOL_A_STATS_ETH_MAC_CNT][ETH_GSTRING_LEN] = {
> + [ETHTOOL_A_STATS_ETH_MAC_2_TX_PKT] =
> "FramesTransmittedOK",
> + [ETHTOOL_A_STATS_ETH_MAC_3_SINGLE_COL] =
> "SingleCollisionFrames",
> + [ETHTOOL_A_STATS_ETH_MAC_4_MULTI_COL] =
> "MultipleCollisionFrames",
> + [ETHTOOL_A_STATS_ETH_MAC_5_RX_PKT] = "FramesReceivedOK",
> + [ETHTOOL_A_STATS_ETH_MAC_6_FCS_ERR] =
> "FrameCheckSequenceErrors",
> + [ETHTOOL_A_STATS_ETH_MAC_7_ALIGN_ERR] = "AlignmentErrors",
> + [ETHTOOL_A_STATS_ETH_MAC_8_TX_BYTES] =
> "OctetsTransmittedOK",
> + [ETHTOOL_A_STATS_ETH_MAC_9_TX_DEFER] =
> "FramesWithDeferredXmissions",
> + [ETHTOOL_A_STATS_ETH_MAC_10_LATE_COL] = "LateCollisions",
> + [ETHTOOL_A_STATS_ETH_MAC_11_XS_COL] =
> "FramesAbortedDueToXSColls",
> + [ETHTOOL_A_STATS_ETH_MAC_12_TX_INT_ERR] =
> "FramesLostDueToIntMACXmitError",
> + [ETHTOOL_A_STATS_ETH_MAC_13_CS_ERR] = "CarrierSenseErrors",
> + [ETHTOOL_A_STATS_ETH_MAC_14_RX_BYTES] = "OctetsReceivedOK",
> + [ETHTOOL_A_STATS_ETH_MAC_15_RX_INT_ERR] =
> "FramesLostDueToIntMACRcvError",
> + [ETHTOOL_A_STATS_ETH_MAC_18_TX_MCAST] =
> "MulticastFramesXmittedOK",
> + [ETHTOOL_A_STATS_ETH_MAC_19_TX_BCAST] =
> "BroadcastFramesXmittedOK",
> + [ETHTOOL_A_STATS_ETH_MAC_20_XS_DEFER] =
> "FramesWithExcessiveDeferral",
> + [ETHTOOL_A_STATS_ETH_MAC_21_RX_MCAST] =
> "MulticastFramesReceivedOK",
> + [ETHTOOL_A_STATS_ETH_MAC_22_RX_BCAST] =
> "BroadcastFramesReceivedOK",
> + [ETHTOOL_A_STATS_ETH_MAC_23_IR_LEN_ERR] =
> "InRangeLengthErrors",
> + [ETHTOOL_A_STATS_ETH_MAC_24_OOR_LEN] =
> "OutOfRangeLengthField",
> + [ETHTOOL_A_STATS_ETH_MAC_25_TOO_LONG_ERR] =
> "FrameTooLongErrors",
> +};
> +
> const struct nla_policy ethnl_stats_get_policy[ETHTOOL_A_STATS_GROUPS
> + 1] = {
> [ETHTOOL_A_STATS_HEADER] =
> NLA_POLICY_NESTED(ethnl_header_policy),
> @@ -70,10 +97,14 @@ static int stats_prepare_data(const struct
> ethnl_req_info *req_base,
> return ret;
>
> memset(&data->phy_stats, 0xff, sizeof(data->phy_stats));
> + memset(&data->mac_stats, 0xff, sizeof(data->mac_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);
> + 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);
>
> ethnl_ops_complete(dev);
> return 0;
> @@ -89,6 +120,10 @@ static int stats_reply_size(const struct
> ethnl_req_info *req_base,
> len += nla_total_size(0) +
> sizeof(struct ethtool_eth_phy_stats) /
> sizeof(u64) *
> nla_total_size_64bit(sizeof(u64));
> + if (test_bit(ETHTOOL_STATS_ETH_MAC, req_info->stat_mask))
> + len += nla_total_size(0) +
> + sizeof(struct ethtool_eth_mac_stats) /
> sizeof(u64) *
> + nla_total_size_64bit(sizeof(u64));
>
> return len;
> }
> @@ -109,6 +144,57 @@ static int stats_put_phy_stats(struct sk_buff
> *skb,
> return 0;
> }
>
> +static int stats_put_mac_stats(struct sk_buff *skb,
> + const struct stats_reply_data *data)
> +{
> + if (stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_2_TX_PKT,
> + data->mac_stats.FramesTransmittedOK) ||
> + stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_3_SINGLE_COL,
> + data->mac_stats.SingleCollisionFrames) ||
> + stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_4_MULTI_COL,
> + data->mac_stats.MultipleCollisionFrames) ||
> + stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_5_RX_PKT,
> + data->mac_stats.FramesReceivedOK) ||
> + stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_6_FCS_ERR,
> + data->mac_stats.FrameCheckSequenceErrors) ||
> + stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_7_ALIGN_ERR,
> + data->mac_stats.AlignmentErrors) ||
> + stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_8_TX_BYTES,
> + data->mac_stats.OctetsTransmittedOK) ||
> + stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_9_TX_DEFER,
> + data->mac_stats.FramesWithDeferredXmissions) ||
> + stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_10_LATE_COL,
> + data->mac_stats.LateCollisions) ||
> + stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_11_XS_COL,
> + data->mac_stats.FramesAbortedDueToXSColls) ||
> + stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_12_TX_INT_ERR,
> + data->mac_stats.FramesLostDueToIntMACXmitError) ||
> + stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_13_CS_ERR,
> + data->mac_stats.CarrierSenseErrors) ||
> + stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_14_RX_BYTES,
> + data->mac_stats.OctetsReceivedOK) ||
> + stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_15_RX_INT_ERR,
> + data->mac_stats.FramesLostDueToIntMACRcvError) ||
> + stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_18_TX_MCAST,
> + data->mac_stats.MulticastFramesXmittedOK) ||
> + stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_19_TX_BCAST,
> + data->mac_stats.BroadcastFramesXmittedOK) ||
> + stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_20_XS_DEFER,
> + data->mac_stats.FramesWithExcessiveDeferral) ||
> + stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_21_RX_MCAST,
> + data->mac_stats.MulticastFramesReceivedOK) ||
> + stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_22_RX_BCAST,
> + data->mac_stats.BroadcastFramesReceivedOK) ||
> + stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_23_IR_LEN_ERR,
> + data->mac_stats.InRangeLengthErrors) ||
> + stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_24_OOR_LEN,
> + data->mac_stats.OutOfRangeLengthField) ||
> + stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_25_TOO_LONG_ERR,
> + data->mac_stats.FrameTooLongErrors))
> + return -EMSGSIZE;
lots of repetition, someone might forget to add the new stat in one of
these places ..
best practice here is to centralize all the data structures and
information definitions in one place, you define the stat id, string,
and value offset, then a generic loop can generate the strset and fill
up values in the correct offset.
similar implementation is already in mlx5:
see pport_802_3_stats_desc:
https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c#L682
the "pport_802_3_stats_desc" has a description of the strings and
offsets of all stats in this stats group
and the fill/put functions are very simple and they just iterate over
the array/group and fill up according to the descriptor.
> + return 0;
> +}
> +
> static int stats_put_stats(struct sk_buff *skb,
> const struct stats_reply_data *data,
> u32 id, u32 ss_id,
> @@ -148,6 +234,10 @@ static int stats_fill_reply(struct sk_buff *skb,
> ret = stats_put_stats(skb, data, ETHTOOL_STATS_ETH_PHY,
> ETH_SS_STATS_ETH_PHY,
> stats_put_phy_stats);
> + if (!ret && test_bit(ETHTOOL_STATS_ETH_MAC, req_info-
> >stat_mask))
> + ret = stats_put_stats(skb, data, ETHTOOL_STATS_ETH_MAC,
> + ETH_SS_STATS_ETH_MAC,
> + stats_put_mac_stats);
>
> return ret;
> }
> diff --git a/net/ethtool/strset.c b/net/ethtool/strset.c
> index 5f3c73587ff4..a8aac7bcfcc9 100644
> --- a/net/ethtool/strset.c
> +++ b/net/ethtool/strset.c
> @@ -90,6 +90,11 @@ static const struct strset_info info_template[] = {
> .count = __ETHTOOL_A_STATS_ETH_PHY_CNT,
> .strings = stats_eth_phy_names,
> },
> + [ETH_SS_STATS_ETH_MAC] = {
> + .per_dev = false,
> + .count = __ETHTOOL_A_STATS_ETH_MAC_CNT,
> + .strings = stats_eth_mac_names,
> + },
> };
>
> struct strset_req_info {
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [RFC net-next 4/6] ethtool: add interface to read standard MAC stats
2021-04-15 6:12 ` Saeed Mahameed
@ 2021-04-15 15:38 ` Jakub Kicinski
2021-04-15 22:46 ` Saeed Mahameed
0 siblings, 1 reply; 14+ messages in thread
From: Jakub Kicinski @ 2021-04-15 15:38 UTC (permalink / raw)
To: Saeed Mahameed; +Cc: netdev, davem, andrew, mkubecek, idosch
On Wed, 14 Apr 2021 23:12:52 -0700 Saeed Mahameed wrote:
> On Wed, 2021-04-14 at 13:23 -0700, Jakub Kicinski wrote:
> > Most of the MAC statistics are included in
> > struct rtnl_link_stats64, but some fields
> > are aggregated. Besides it's good to expose
> > these clearly hardware stats separately.
> >
> > Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> > +/* Basic IEEE 802.3 MAC statistics (30.3.1.1.*), not otherwise exposed
> > + * via a more targeted API.
> > + */
> > +struct ethtool_eth_mac_stats {
> > + u64 FramesTransmittedOK;
> > + u64 SingleCollisionFrames;
> > + u64 MultipleCollisionFrames;
> > + u64 FramesReceivedOK;
> > + u64 FrameCheckSequenceErrors;
> > + u64 AlignmentErrors;
> > + u64 OctetsTransmittedOK;
> > + u64 FramesWithDeferredXmissions;
> > + u64 LateCollisions;
> > + u64 FramesAbortedDueToXSColls;
> > + u64 FramesLostDueToIntMACXmitError;
> > + u64 CarrierSenseErrors;
> > + u64 OctetsReceivedOK;
> > + u64 FramesLostDueToIntMACRcvError;
> > + u64 MulticastFramesXmittedOK;
> > + u64 BroadcastFramesXmittedOK;
> > + u64 FramesWithExcessiveDeferral;
> > + u64 MulticastFramesReceivedOK;
> > + u64 BroadcastFramesReceivedOK;
> > + u64 InRangeLengthErrors;
> > + u64 OutOfRangeLengthField;
> > + u64 FrameTooLongErrors;
> > +};
> > +
> > /* Basic IEEE 802.3 PHY statistics (30.3.2.1.*), not otherwise exposed
> > * via a more targeted API.
> > */
> > @@ -495,6 +523,7 @@ struct ethtool_module_eeprom {
> > * specified page. Returns a negative error code or the amount of
> > bytes
> > * read.
> > * @get_eth_phy_stats: Query some of the IEEE 802.3 PHY statistics.
> > + * @get_eth_mac_stats: Query some of the IEEE 802.3 MAC statistics.
> > *
> > * All operations are optional (i.e. the function pointer may be set
> > * to %NULL) and callers must take this into account. Callers must
> > @@ -607,6 +636,8 @@ struct ethtool_ops {
> > struct netlink_ext_ack
> > *extack);
> > void (*get_eth_phy_stats)(struct net_device *dev,
> > struct ethtool_eth_phy_stats
> > *phy_stats);
> > + void (*get_eth_mac_stats)(struct net_device *dev,
> > + struct ethtool_eth_mac_stats
> > *mac_stats);
>
> too many callbacks.. I understand the point of having explicit structs
> per stats group, but it can be achievable with one generic ethtool
> calback function with the help of a flexible struct:
>
> void (*get_std_stats)(struct net_device *dev, struct *std_stats)
>
>
> union stats_groups {
> struct ethtool_eth_phy_stats eth_phy;
> struct ethtool_eth_mac_stats eth_mac;
> ...
> }
>
> struct std_stats {
> u16 type;
> union stats_groups stats[0];
> }
>
> where std_stats.stats is allocated dynamically according to
> std_stats.type
>
> and driver can just access the corresponding stats according to type
>
> e.g:
> std_stats.stats.eth_phy
Kinda expected you'd say this :) The mux make life simpler for drivers
with a lot of layers of abstraction. Separate ops make life simpler for
simpler drivers.
Basic Ethernet driver goes from this:
get_mac_stats()
{
priv = netdev_priv()
stat->x = readl(priv->regs + REG_X);
stat->z = readl(priv->regs + REG_Y);
stat->y = readl(priv->regs + REG_Z);
}
to:
get_std_stats()
{
priv = netdev_priv();
switch (stats->type) {
case MAC:
stat->x = readl(priv->regs + REG_X);
stat->z = readl(priv->regs + REG_Y);
stat->y = readl(priv->regs + REG_Z);
break;
}
}
or likely:
get_std_stats()
{
priv = netdev_priv();
switch (stats->type) {
case MAC:
return get_mac_stats(priv..);
}
}
I prefer to keep the callbacks separate, there isn't that many of them.
> > +static int stats_put_mac_stats(struct sk_buff *skb,
> > + const struct stats_reply_data *data)
> > +{
> > + if (stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_2_TX_PKT,
> > + data->mac_stats.FramesTransmittedOK) ||
> > + stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_3_SINGLE_COL,
> > + data->mac_stats.SingleCollisionFrames) ||
> > + stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_4_MULTI_COL,
> > + data->mac_stats.MultipleCollisionFrames) ||
> > + stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_5_RX_PKT,
> > + data->mac_stats.FramesReceivedOK) ||
> > + stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_6_FCS_ERR,
> > + data->mac_stats.FrameCheckSequenceErrors) ||
> > + stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_7_ALIGN_ERR,
> > + data->mac_stats.AlignmentErrors) ||
> > + stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_8_TX_BYTES,
> > + data->mac_stats.OctetsTransmittedOK) ||
> > + stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_9_TX_DEFER,
> > + data->mac_stats.FramesWithDeferredXmissions) ||
> > + stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_10_LATE_COL,
> > + data->mac_stats.LateCollisions) ||
> > + stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_11_XS_COL,
> > + data->mac_stats.FramesAbortedDueToXSColls) ||
> > + stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_12_TX_INT_ERR,
> > + data->mac_stats.FramesLostDueToIntMACXmitError) ||
> > + stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_13_CS_ERR,
> > + data->mac_stats.CarrierSenseErrors) ||
> > + stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_14_RX_BYTES,
> > + data->mac_stats.OctetsReceivedOK) ||
> > + stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_15_RX_INT_ERR,
> > + data->mac_stats.FramesLostDueToIntMACRcvError) ||
> > + stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_18_TX_MCAST,
> > + data->mac_stats.MulticastFramesXmittedOK) ||
> > + stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_19_TX_BCAST,
> > + data->mac_stats.BroadcastFramesXmittedOK) ||
> > + stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_20_XS_DEFER,
> > + data->mac_stats.FramesWithExcessiveDeferral) ||
> > + stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_21_RX_MCAST,
> > + data->mac_stats.MulticastFramesReceivedOK) ||
> > + stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_22_RX_BCAST,
> > + data->mac_stats.BroadcastFramesReceivedOK) ||
> > + stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_23_IR_LEN_ERR,
> > + data->mac_stats.InRangeLengthErrors) ||
> > + stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_24_OOR_LEN,
> > + data->mac_stats.OutOfRangeLengthField) ||
> > + stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_25_TOO_LONG_ERR,
> > + data->mac_stats.FrameTooLongErrors))
> > + return -EMSGSIZE;
>
> lots of repetition, someone might forget to add the new stat in one of
> these places ..
If someone forgets to add a stat to the place they are dumped?
They will immediately realize it's not getting dumped...
> best practice here is to centralize all the data structures and
> information definitions in one place, you define the stat id, string,
> and value offset, then a generic loop can generate the strset and fill
> up values in the correct offset.
>
> similar implementation is already in mlx5:
>
> see pport_802_3_stats_desc:
> https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c#L682
>
> the "pport_802_3_stats_desc" has a description of the strings and
> offsets of all stats in this stats group
> and the fill/put functions are very simple and they just iterate over
> the array/group and fill up according to the descriptor.
We can maybe save 60 lines if we generate stats_eth_mac_names
in a initcall, is it really worth it? I prefer the readability
/ grepability.
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [RFC net-next 4/6] ethtool: add interface to read standard MAC stats
2021-04-15 15:38 ` Jakub Kicinski
@ 2021-04-15 22:46 ` Saeed Mahameed
2021-04-15 23:05 ` Jakub Kicinski
0 siblings, 1 reply; 14+ messages in thread
From: Saeed Mahameed @ 2021-04-15 22:46 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: netdev, davem, andrew, mkubecek, idosch
On Thu, 2021-04-15 at 08:38 -0700, Jakub Kicinski wrote:
> On Wed, 14 Apr 2021 23:12:52 -0700 Saeed Mahameed wrote:
> > On Wed, 2021-04-14 at 13:23 -0700, Jakub Kicinski wrote:
> > > Most of the MAC statistics are included in
> > > struct rtnl_link_stats64, but some fields
> > > are aggregated. Besides it's good to expose
> > > these clearly hardware stats separately.
> > >
> > > Signed-off-by: Jakub Kicinski <kuba@kernel.org>
>
> > > +/* Basic IEEE 802.3 MAC statistics (30.3.1.1.*), not otherwise
> > > exposed
> > > + * via a more targeted API.
> > > + */
> > > +struct ethtool_eth_mac_stats {
> > > + u64 FramesTransmittedOK;
> > > + u64 SingleCollisionFrames;
> > > + u64 MultipleCollisionFrames;
> > > + u64 FramesReceivedOK;
> > > + u64 FrameCheckSequenceErrors;
> > > + u64 AlignmentErrors;
> > > + u64 OctetsTransmittedOK;
> > > + u64 FramesWithDeferredXmissions;
> > > + u64 LateCollisions;
> > > + u64 FramesAbortedDueToXSColls;
> > > + u64 FramesLostDueToIntMACXmitError;
> > > + u64 CarrierSenseErrors;
> > > + u64 OctetsReceivedOK;
> > > + u64 FramesLostDueToIntMACRcvError;
> > > + u64 MulticastFramesXmittedOK;
> > > + u64 BroadcastFramesXmittedOK;
> > > + u64 FramesWithExcessiveDeferral;
> > > + u64 MulticastFramesReceivedOK;
> > > + u64 BroadcastFramesReceivedOK;
> > > + u64 InRangeLengthErrors;
> > > + u64 OutOfRangeLengthField;
> > > + u64 FrameTooLongErrors;
> > > +};
> > > +
> > > /* Basic IEEE 802.3 PHY statistics (30.3.2.1.*), not otherwise
> > > exposed
> > > * via a more targeted API.
> > > */
> > > @@ -495,6 +523,7 @@ struct ethtool_module_eeprom {
> > > * specified page. Returns a negative error code or the
> > > amount of
> > > bytes
> > > * read.
> > > * @get_eth_phy_stats: Query some of the IEEE 802.3 PHY
> > > statistics.
> > > + * @get_eth_mac_stats: Query some of the IEEE 802.3 MAC
> > > statistics.
> > > *
> > > * All operations are optional (i.e. the function pointer may be
> > > set
> > > * to %NULL) and callers must take this into account. Callers
> > > must
> > > @@ -607,6 +636,8 @@ struct ethtool_ops {
> > > struct
> > > netlink_ext_ack
> > > *extack);
> > > void (*get_eth_phy_stats)(struct net_device *dev,
> > > struct ethtool_eth_phy_stats
> > > *phy_stats);
> > > + void (*get_eth_mac_stats)(struct net_device *dev,
> > > + struct ethtool_eth_mac_stats
> > > *mac_stats);
> >
> > too many callbacks.. I understand the point of having explicit
> > structs
> > per stats group, but it can be achievable with one generic ethtool
> > calback function with the help of a flexible struct:
> >
> > void (*get_std_stats)(struct net_device *dev, struct *std_stats)
> >
> >
> > union stats_groups {
> > struct ethtool_eth_phy_stats eth_phy;
> > struct ethtool_eth_mac_stats eth_mac;
> > ...
> > }
> >
> > struct std_stats {
> > u16 type;
> > union stats_groups stats[0];
> > }
> >
> > where std_stats.stats is allocated dynamically according to
> > std_stats.type
> >
> > and driver can just access the corresponding stats according to
> > type
> >
> > e.g:
> > std_stats.stats.eth_phy
>
> Kinda expected you'd say this :) The mux make life simpler for
> drivers
> with a lot of layers of abstraction. Separate ops make life simpler
> for
> simpler drivers.
>
> Basic Ethernet driver goes from this:
>
> get_mac_stats()
> {
> priv = netdev_priv()
>
> stat->x = readl(priv->regs + REG_X);
> stat->z = readl(priv->regs + REG_Y);
> stat->y = readl(priv->regs + REG_Z);
> }
>
> to:
>
> get_std_stats()
> {
> priv = netdev_priv();
>
> switch (stats->type) {
> case MAC:
> stat->x = readl(priv->regs + REG_X);
> stat->z = readl(priv->regs + REG_Y);
> stat->y = readl(priv->regs + REG_Z);
> break;
> }
> }
>
> or likely:
>
> get_std_stats()
> {
> priv = netdev_priv();
>
> switch (stats->type) {
> case MAC:
> return get_mac_stats(priv..);
> }
> }
>
> I prefer to keep the callbacks separate, there isn't that many of
> them.
>
Ack, i don't like switch cases, but i also don't like long structs with
pages and pages of callbacks..
> > > +static int stats_put_mac_stats(struct sk_buff *skb,
> > > + const struct stats_reply_data
> > > *data)
> > > +{
> > > + if (stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_2_TX_PKT,
> > > + data->mac_stats.FramesTransmittedOK) ||
> > > + stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_3_SINGLE_COL,
> > > + data->mac_stats.SingleCollisionFrames) ||
> > > + stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_4_MULTI_COL,
> > > + data->mac_stats.MultipleCollisionFrames) ||
> > > + stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_5_RX_PKT,
> > > + data->mac_stats.FramesReceivedOK) ||
> > > + stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_6_FCS_ERR,
> > > + data->mac_stats.FrameCheckSequenceErrors) ||
> > > + stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_7_ALIGN_ERR,
> > > + data->mac_stats.AlignmentErrors) ||
> > > + stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_8_TX_BYTES,
> > > + data->mac_stats.OctetsTransmittedOK) ||
> > > + stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_9_TX_DEFER,
> > > + data->mac_stats.FramesWithDeferredXmissions)
> > > ||
> > > + stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_10_LATE_COL,
> > > + data->mac_stats.LateCollisions) ||
> > > + stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_11_XS_COL,
> > > + data->mac_stats.FramesAbortedDueToXSColls)
> > > ||
> > > + stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_12_TX_INT_ERR,
> > > + data-
> > > >mac_stats.FramesLostDueToIntMACXmitError) ||
> > > + stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_13_CS_ERR,
> > > + data->mac_stats.CarrierSenseErrors) ||
> > > + stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_14_RX_BYTES,
> > > + data->mac_stats.OctetsReceivedOK) ||
> > > + stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_15_RX_INT_ERR,
> > > + data-
> > > >mac_stats.FramesLostDueToIntMACRcvError) ||
> > > + stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_18_TX_MCAST,
> > > + data->mac_stats.MulticastFramesXmittedOK) ||
> > > + stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_19_TX_BCAST,
> > > + data->mac_stats.BroadcastFramesXmittedOK) ||
> > > + stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_20_XS_DEFER,
> > > + data->mac_stats.FramesWithExcessiveDeferral)
> > > ||
> > > + stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_21_RX_MCAST,
> > > + data->mac_stats.MulticastFramesReceivedOK)
> > > ||
> > > + stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_22_RX_BCAST,
> > > + data->mac_stats.BroadcastFramesReceivedOK)
> > > ||
> > > + stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_23_IR_LEN_ERR,
> > > + data->mac_stats.InRangeLengthErrors) ||
> > > + stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_24_OOR_LEN,
> > > + data->mac_stats.OutOfRangeLengthField) ||
> > > + stat_put(skb,
> > > ETHTOOL_A_STATS_ETH_MAC_25_TOO_LONG_ERR,
> > > + data->mac_stats.FrameTooLongErrors))
> > > + return -EMSGSIZE;
> >
> > lots of repetition, someone might forget to add the new stat in one
> > of
> > these places ..
>
> If someone forgets to add a stat to the place they are dumped?
> They will immediately realize it's not getting dumped...
>
kinda my point, I wouldn't count on this..
> > best practice here is to centralize all the data structures and
> > information definitions in one place, you define the stat id,
> > string,
> > and value offset, then a generic loop can generate the strset and
> > fill
> > up values in the correct offset.
> >
> > similar implementation is already in mlx5:
> >
> > see pport_802_3_stats_desc:
> >
> > https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c#L682
> >
> > the "pport_802_3_stats_desc" has a description of the strings and
> > offsets of all stats in this stats group
> > and the fill/put functions are very simple and they just iterate
> > over
> > the array/group and fill up according to the descriptor.
>
> We can maybe save 60 lines if we generate stats_eth_mac_names
> in a initcall, is it really worth it? I prefer the readability
> / grepability.
I don't think readability will be an issue if the infrastructure is
generic enough..
This just a preference, of course you can go with the current code.
My point is that someone doesn't need to change multiple places and
possibly files every time they need to expose a new stat, you just
update some central database of the new data you want to expose.
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [RFC net-next 4/6] ethtool: add interface to read standard MAC stats
2021-04-15 22:46 ` Saeed Mahameed
@ 2021-04-15 23:05 ` Jakub Kicinski
2021-04-15 23:52 ` Saeed Mahameed
0 siblings, 1 reply; 14+ messages in thread
From: Jakub Kicinski @ 2021-04-15 23:05 UTC (permalink / raw)
To: Saeed Mahameed; +Cc: netdev, davem, andrew, mkubecek, idosch
On Thu, 15 Apr 2021 15:46:52 -0700 Saeed Mahameed wrote:
> > > best practice here is to centralize all the data structures and
> > > information definitions in one place, you define the stat id,
> > > string,
> > > and value offset, then a generic loop can generate the strset and
> > > fill
> > > up values in the correct offset.
> > >
> > > similar implementation is already in mlx5:
> > >
> > > see pport_802_3_stats_desc:
> > >
> > > https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c#L682
> > >
> > > the "pport_802_3_stats_desc" has a description of the strings and
> > > offsets of all stats in this stats group
> > > and the fill/put functions are very simple and they just iterate
> > > over
> > > the array/group and fill up according to the descriptor.
> >
> > We can maybe save 60 lines if we generate stats_eth_mac_names
> > in a initcall, is it really worth it? I prefer the readability
> > / grepability.
>
> I don't think readability will be an issue if the infrastructure is
> generic enough..
>
> This just a preference, of course you can go with the current code.
> My point is that someone doesn't need to change multiple places and
> possibly files every time they need to expose a new stat, you just
> update some central database of the new data you want to expose.
Understood, I've written those table-based generators for ethtool stats
in the drivers in the past as well, but here we can only generate the
dumping and the names. We'll need to manually fill in defines/enums in
uAPI, struct members and the generator table. I'd rather stick to
real struct members in the core<->driver API than indexing an array
with an enums. So the savings are 4 places => 3 places?
Unless I'm missing some clever, yet robust and readable ways of coding
this up..
Can we leave as is as starting point and see where we go from here?
So far MAC stats are the only sizable ones were we'd see noticeable
gain.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC net-next 4/6] ethtool: add interface to read standard MAC stats
2021-04-15 23:05 ` Jakub Kicinski
@ 2021-04-15 23:52 ` Saeed Mahameed
0 siblings, 0 replies; 14+ messages in thread
From: Saeed Mahameed @ 2021-04-15 23:52 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: netdev, davem, andrew, mkubecek, idosch
On Thu, 2021-04-15 at 16:05 -0700, Jakub Kicinski wrote:
> On Thu, 15 Apr 2021 15:46:52 -0700 Saeed Mahameed wrote:
> > > > best practice here is to centralize all the data structures and
> > > > information definitions in one place, you define the stat id,
> > > > string,
> > > > and value offset, then a generic loop can generate the strset
> > > > and
> > > > fill
> > > > up values in the correct offset.
> > > >
> > > > similar implementation is already in mlx5:
> > > >
> > > > see pport_802_3_stats_desc:
> > > >
> > > > https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c#L682
> > > >
> > > > the "pport_802_3_stats_desc" has a description of the strings
> > > > and
> > > > offsets of all stats in this stats group
> > > > and the fill/put functions are very simple and they just
> > > > iterate
> > > > over
> > > > the array/group and fill up according to the descriptor.
> > >
> > > We can maybe save 60 lines if we generate stats_eth_mac_names
> > > in a initcall, is it really worth it? I prefer the readability
> > > / grepability.
> >
> > I don't think readability will be an issue if the infrastructure is
> > generic enough..
> >
> > This just a preference, of course you can go with the current code.
> > My point is that someone doesn't need to change multiple places and
> > possibly files every time they need to expose a new stat, you just
> > update some central database of the new data you want to expose.
>
> Understood, I've written those table-based generators for ethtool
> stats
> in the drivers in the past as well, but here we can only generate the
> dumping and the names. We'll need to manually fill in defines/enums
> in
> uAPI, struct members and the generator table. I'd rather stick to
> real struct members in the core<->driver API than indexing an array
> with an enums. So the savings are 4 places => 3 places?
>
> Unless I'm missing some clever, yet robust and readable ways of
> coding
> this up..
>
> Can we leave as is as starting point and see where we go from here?
> So far MAC stats are the only sizable ones were we'd see noticeable
> gain.
Sure ! if we see it is exploding with stats and long if conditions, we
can reconsider :) ..
^ permalink raw reply [flat|nested] 14+ messages in thread
* [RFC net-next 5/6] ethtool: add interface to read standard MAC Ctrl stats
2021-04-14 20:23 [RFC net-next 0/6] ethtool: add uAPI for reading standard stats Jakub Kicinski
` (3 preceding siblings ...)
2021-04-14 20:23 ` [RFC net-next 4/6] ethtool: add interface to read standard MAC stats Jakub Kicinski
@ 2021-04-14 20:23 ` Jakub Kicinski
2021-04-14 20:23 ` [RFC net-next 6/6] ethtool: add interface to read RMON stats Jakub Kicinski
2021-04-15 5:51 ` [RFC net-next 0/6] ethtool: add uAPI for reading standard stats Saeed Mahameed
6 siblings, 0 replies; 14+ messages in thread
From: Jakub Kicinski @ 2021-04-14 20:23 UTC (permalink / raw)
To: netdev; +Cc: davem, andrew, mkubecek, idosch, Jakub Kicinski
Number of devices maintains the standard-based MAC control
counters for control frames. Add a API for those.
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
include/linux/ethtool.h | 11 ++++++++++
include/uapi/linux/ethtool.h | 2 ++
include/uapi/linux/ethtool_netlink.h | 14 ++++++++++++
net/ethtool/netlink.h | 1 +
net/ethtool/stats.c | 33 ++++++++++++++++++++++++++++
net/ethtool/strset.c | 5 +++++
6 files changed, 66 insertions(+)
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 3c689a13e679..22bab13c5729 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -285,6 +285,15 @@ struct ethtool_eth_phy_stats {
u64 SymbolErrorDuringCarrier;
};
+/* Basic IEEE 802.3 MAC Ctrl statistics (30.3.3.*), not otherwise exposed
+ * via a more targeted API.
+ */
+struct ethtool_eth_ctrl_stats {
+ u64 MACControlFramesTransmitted;
+ u64 MACControlFramesReceived;
+ u64 UnsupportedOpcodesReceived;
+};
+
/**
* struct ethtool_pause_stats - statistics for IEEE 802.3x pause frames
* @tx_pause_frames: transmitted pause frame count. Reported to user space
@@ -638,6 +647,8 @@ struct ethtool_ops {
struct ethtool_eth_phy_stats *phy_stats);
void (*get_eth_mac_stats)(struct net_device *dev,
struct ethtool_eth_mac_stats *mac_stats);
+ void (*get_eth_ctrl_stats)(struct net_device *dev,
+ struct ethtool_eth_ctrl_stats *ctrl_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 c227376d811a..9cb8df89d4f2 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -672,6 +672,7 @@ enum ethtool_link_ext_substate_cable_issue {
* @ETH_SS_STATS_STD: standardized stats
* @ETH_SS_STATS_ETH_PHY: names of IEEE 802.3 PHY statistics
* @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_COUNT: number of defined string sets
*/
@@ -695,6 +696,7 @@ enum ethtool_stringset {
ETH_SS_STATS_STD,
ETH_SS_STATS_ETH_PHY,
ETH_SS_STATS_ETH_MAC,
+ ETH_SS_STATS_ETH_CTRL,
/* 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 7ef6fbe237d9..7c061dfc3b67 100644
--- a/include/uapi/linux/ethtool_netlink.h
+++ b/include/uapi/linux/ethtool_netlink.h
@@ -699,6 +699,7 @@ enum {
enum {
ETHTOOL_STATS_ETH_PHY,
ETHTOOL_STATS_ETH_MAC,
+ ETHTOOL_STATS_ETH_CTRL,
/* add new constants above here */
__ETHTOOL_STATS_CNT
@@ -776,6 +777,19 @@ enum {
ETHTOOL_A_STATS_ETH_MAC_MAX = (__ETHTOOL_A_STATS_ETH_MAC_CNT - 1)
};
+enum {
+ /* 30.3.3.3 aMACControlFramesTransmitted */
+ ETHTOOL_A_STATS_ETH_CTRL_3_TX = ETHTOOL_A_STATS_GRP_FIRST_ATTR,
+ /* 30.3.3.4 aMACControlFramesReceived */
+ ETHTOOL_A_STATS_ETH_CTRL_4_RX,
+ /* 30.3.3.5 aUnsupportedOpcodesReceived */
+ ETHTOOL_A_STATS_ETH_CTRL_5_RX_UNSUP,
+
+ /* add new constants above here */
+ __ETHTOOL_A_STATS_ETH_CTRL_CNT,
+ ETHTOOL_A_STATS_ETH_CTRL_MAX = (__ETHTOOL_A_STATS_ETH_CTRL_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 9c5f6ee71864..bd96eb57c07c 100644
--- a/net/ethtool/netlink.h
+++ b/net/ethtool/netlink.h
@@ -404,5 +404,6 @@ int ethnl_set_fec(struct sk_buff *skb, struct genl_info *info);
extern const char stats_std_names[__ETHTOOL_STATS_CNT][ETH_GSTRING_LEN];
extern const char stats_eth_phy_names[__ETHTOOL_A_STATS_ETH_PHY_CNT][ETH_GSTRING_LEN];
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];
#endif /* _NET_ETHTOOL_NETLINK_H */
diff --git a/net/ethtool/stats.c b/net/ethtool/stats.c
index e0395d5c0f9d..5feb6aba26c8 100644
--- a/net/ethtool/stats.c
+++ b/net/ethtool/stats.c
@@ -16,6 +16,7 @@ struct stats_reply_data {
struct ethnl_reply_data base;
struct ethtool_eth_phy_stats phy_stats;
struct ethtool_eth_mac_stats mac_stats;
+ struct ethtool_eth_ctrl_stats ctrl_stats;
};
#define STATS_REPDATA(__reply_base) \
@@ -24,6 +25,7 @@ struct stats_reply_data {
const char stats_std_names[__ETHTOOL_STATS_CNT][ETH_GSTRING_LEN] = {
[ETHTOOL_STATS_ETH_PHY] = "eth-phy",
[ETHTOOL_STATS_ETH_MAC] = "eth-mac",
+ [ETHTOOL_STATS_ETH_CTRL] = "eth-ctrl",
};
const char stats_eth_phy_names[__ETHTOOL_A_STATS_ETH_PHY_CNT][ETH_GSTRING_LEN] = {
@@ -55,6 +57,12 @@ const char stats_eth_mac_names[__ETHTOOL_A_STATS_ETH_MAC_CNT][ETH_GSTRING_LEN] =
[ETHTOOL_A_STATS_ETH_MAC_25_TOO_LONG_ERR] = "FrameTooLongErrors",
};
+const char stats_eth_ctrl_names[__ETHTOOL_A_STATS_ETH_CTRL_CNT][ETH_GSTRING_LEN] = {
+ [ETHTOOL_A_STATS_ETH_CTRL_3_TX] = "MACControlFramesTransmitted",
+ [ETHTOOL_A_STATS_ETH_CTRL_4_RX] = "MACControlFramesReceived",
+ [ETHTOOL_A_STATS_ETH_CTRL_5_RX_UNSUP] = "UnsupportedOpcodesReceived",
+};
+
const struct nla_policy ethnl_stats_get_policy[ETHTOOL_A_STATS_GROUPS + 1] = {
[ETHTOOL_A_STATS_HEADER] =
NLA_POLICY_NESTED(ethnl_header_policy),
@@ -98,6 +106,7 @@ static int stats_prepare_data(const struct ethnl_req_info *req_base,
memset(&data->phy_stats, 0xff, sizeof(data->phy_stats));
memset(&data->mac_stats, 0xff, sizeof(data->mac_stats));
+ memset(&data->ctrl_stats, 0xff, sizeof(data->mac_stats));
if (test_bit(ETHTOOL_STATS_ETH_PHY, req_info->stat_mask) &&
dev->ethtool_ops->get_eth_phy_stats)
@@ -105,6 +114,9 @@ static int stats_prepare_data(const struct ethnl_req_info *req_base,
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);
+ 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);
ethnl_ops_complete(dev);
return 0;
@@ -124,6 +136,10 @@ static int stats_reply_size(const struct ethnl_req_info *req_base,
len += nla_total_size(0) +
sizeof(struct ethtool_eth_mac_stats) / sizeof(u64) *
nla_total_size_64bit(sizeof(u64));
+ if (test_bit(ETHTOOL_STATS_ETH_CTRL, req_info->stat_mask))
+ len += nla_total_size(0) +
+ sizeof(struct ethtool_eth_ctrl_stats) / sizeof(u64) *
+ nla_total_size_64bit(sizeof(u64));
return len;
}
@@ -195,6 +211,19 @@ static int stats_put_mac_stats(struct sk_buff *skb,
return 0;
}
+static int stats_put_ctrl_stats(struct sk_buff *skb,
+ const struct stats_reply_data *data)
+{
+ if (stat_put(skb, ETHTOOL_A_STATS_ETH_CTRL_3_TX,
+ data->ctrl_stats.MACControlFramesTransmitted) ||
+ stat_put(skb, ETHTOOL_A_STATS_ETH_CTRL_4_RX,
+ data->ctrl_stats.MACControlFramesReceived) ||
+ stat_put(skb, ETHTOOL_A_STATS_ETH_CTRL_5_RX_UNSUP,
+ data->ctrl_stats.UnsupportedOpcodesReceived))
+ return -EMSGSIZE;
+ return 0;
+}
+
static int stats_put_stats(struct sk_buff *skb,
const struct stats_reply_data *data,
u32 id, u32 ss_id,
@@ -238,6 +267,10 @@ static int stats_fill_reply(struct sk_buff *skb,
ret = stats_put_stats(skb, data, ETHTOOL_STATS_ETH_MAC,
ETH_SS_STATS_ETH_MAC,
stats_put_mac_stats);
+ if (!ret && test_bit(ETHTOOL_STATS_ETH_CTRL, req_info->stat_mask))
+ ret = stats_put_stats(skb, data, ETHTOOL_STATS_ETH_CTRL,
+ ETH_SS_STATS_ETH_CTRL,
+ stats_put_ctrl_stats);
return ret;
}
diff --git a/net/ethtool/strset.c b/net/ethtool/strset.c
index a8aac7bcfcc9..a33c603a7a02 100644
--- a/net/ethtool/strset.c
+++ b/net/ethtool/strset.c
@@ -95,6 +95,11 @@ static const struct strset_info info_template[] = {
.count = __ETHTOOL_A_STATS_ETH_MAC_CNT,
.strings = stats_eth_mac_names,
},
+ [ETH_SS_STATS_ETH_CTRL] = {
+ .per_dev = false,
+ .count = __ETHTOOL_A_STATS_ETH_CTRL_CNT,
+ .strings = stats_eth_ctrl_names,
+ },
};
struct strset_req_info {
--
2.30.2
^ permalink raw reply related [flat|nested] 14+ messages in thread* [RFC net-next 6/6] ethtool: add interface to read RMON stats
2021-04-14 20:23 [RFC net-next 0/6] ethtool: add uAPI for reading standard stats Jakub Kicinski
` (4 preceding siblings ...)
2021-04-14 20:23 ` [RFC net-next 5/6] ethtool: add interface to read standard MAC Ctrl stats Jakub Kicinski
@ 2021-04-14 20:23 ` Jakub Kicinski
2021-04-15 5:51 ` [RFC net-next 0/6] ethtool: add uAPI for reading standard stats Saeed Mahameed
6 siblings, 0 replies; 14+ messages in thread
From: Jakub Kicinski @ 2021-04-14 20:23 UTC (permalink / raw)
To: netdev; +Cc: davem, andrew, mkubecek, idosch, Jakub Kicinski
Most devices maintain RMON (RFC 2819) stats - particularly
the "histogram" of packets received by size. Unlike other
RFCs which duplicate IEEE stats, the short/oversized frame
counters in RMON don't seem to match IEEE stats 1-to-1 either,
so expose those, too. Do not expose basic packet, CRC errors
etc - those are already otherwise covered.
Because standard defines packet ranges only up to 1518, and
everything above that should theoretically be "oversized"
- devices often create their own ranges.
Going beyond what the RFC defines - expose the "histogram"
in the Tx direction (assume for now that the ranges will
be the same).
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
include/linux/ethtool.h | 43 +++++++++++++++
include/uapi/linux/ethtool.h | 2 +
include/uapi/linux/ethtool_netlink.h | 23 ++++++++
net/ethtool/netlink.h | 1 +
net/ethtool/stats.c | 82 ++++++++++++++++++++++++++++
net/ethtool/strset.c | 5 ++
6 files changed, 156 insertions(+)
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 22bab13c5729..f633c561eeef 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -371,6 +371,44 @@ struct ethtool_module_eeprom {
__u8 *data;
};
+/**
+ * struct ethtool_rmon_hist_range - byte range for histogram statistics
+ * @low: low bound of the bucket (inclusive)
+ * @high: high bound of the bucket (inclusive)
+ */
+struct ethtool_rmon_hist_range {
+ u16 low;
+ u16 high;
+};
+
+#define ETHTOOL_RMON_HIST_MAX 10
+
+/**
+ * struct ethtool_rmon_stats - selected RMON (RFC 2819) statistics
+ * @undersize_pkts: Equivalent to `etherStatsUndersizePkts` from the RFC.
+ * @oversize_pkts: Equivalent to `etherStatsOversizePkts` from the RFC.
+ * @fragments: Equivalent to `etherStatsFragments` from the RFC.
+ * @jabbers: Equivalent to `etherStatsJabbers` from the RFC.
+ * @hist: Packet counter for packet length buckets (e.g.
+ * `etherStatsPkts128to255Octets` from the RFC).
+ * @hist_tx: Tx counters in similar form to @hist, not defined in the RFC.
+ *
+ * Selection of RMON (RFC 2819) statistics which are not exposed via different
+ * APIs, primarily the packet-length-based counters.
+ * Unfortunately different designs choose different buckets beyond
+ * the 1024B mark (jumbo frame teritory), so the definition of the bucket
+ * ranges is left to the driver.
+ */
+struct ethtool_rmon_stats {
+ u64 undersize_pkts;
+ u64 oversize_pkts;
+ u64 fragments;
+ u64 jabbers;
+
+ u64 hist[ETHTOOL_RMON_HIST_MAX];
+ u64 hist_tx[ETHTOOL_RMON_HIST_MAX];
+};
+
/**
* struct ethtool_ops - optional netdev operations
* @cap_link_lanes_supported: indicates if the driver supports lanes
@@ -533,6 +571,8 @@ struct ethtool_module_eeprom {
* read.
* @get_eth_phy_stats: Query some of the IEEE 802.3 PHY statistics.
* @get_eth_mac_stats: Query some of the IEEE 802.3 MAC statistics.
+ * @get_rmon_stats: Query some of the RMON (RFC 2819) statistics.
+ * Set %ranges to a pointer to zero-terminated array of byte ranges.
*
* All operations are optional (i.e. the function pointer may be set
* to %NULL) and callers must take this into account. Callers must
@@ -649,6 +689,9 @@ struct ethtool_ops {
struct ethtool_eth_mac_stats *mac_stats);
void (*get_eth_ctrl_stats)(struct net_device *dev,
struct ethtool_eth_ctrl_stats *ctrl_stats);
+ void (*get_rmon_stats)(struct net_device *dev,
+ struct ethtool_rmon_stats *rmon_stats,
+ const struct ethtool_rmon_hist_range **ranges);
};
int ethtool_check_ops(const struct ethtool_ops *ops);
diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index 9cb8df89d4f2..cfef6b08169a 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -673,6 +673,7 @@ enum ethtool_link_ext_substate_cable_issue {
* @ETH_SS_STATS_ETH_PHY: names of IEEE 802.3 PHY statistics
* @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_COUNT: number of defined string sets
*/
@@ -697,6 +698,7 @@ enum ethtool_stringset {
ETH_SS_STATS_ETH_PHY,
ETH_SS_STATS_ETH_MAC,
ETH_SS_STATS_ETH_CTRL,
+ ETH_SS_STATS_RMON,
/* 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 7c061dfc3b67..d38733d83bfb 100644
--- a/include/uapi/linux/ethtool_netlink.h
+++ b/include/uapi/linux/ethtool_netlink.h
@@ -700,6 +700,7 @@ enum {
ETHTOOL_STATS_ETH_PHY,
ETHTOOL_STATS_ETH_MAC,
ETHTOOL_STATS_ETH_CTRL,
+ ETHTOOL_STATS_RMON,
/* add new constants above here */
__ETHTOOL_STATS_CNT
@@ -790,6 +791,28 @@ enum {
ETHTOOL_A_STATS_ETH_CTRL_MAX = (__ETHTOOL_A_STATS_ETH_CTRL_CNT - 1)
};
+enum {
+ ETHTOOL_A_STATS_RMON_HIST = ETHTOOL_A_STATS_GRP_FIRST_ATTR,
+ ETHTOOL_A_STATS_RMON_HIST_BKT_LOW, /* u32 */
+ ETHTOOL_A_STATS_RMON_HIST_BKT_HI, /* u32 */
+ ETHTOOL_A_STATS_RMON_HIST_VAL,
+
+ ETHTOOL_A_STATS_RMON_HIST_TX,
+
+ /* etherStatsUndersizePkts */
+ ETHTOOL_A_STATS_RMON_UNDERSIZE,
+ /* etherStatsOversizePkts */
+ ETHTOOL_A_STATS_RMON_OVERSIZE,
+ /* etherStatsFragments */
+ ETHTOOL_A_STATS_RMON_FRAG,
+ /* etherStatsJabbers */
+ ETHTOOL_A_STATS_RMON_JABBER,
+
+ /* add new constants above here */
+ __ETHTOOL_A_STATS_RMON_CNT,
+ ETHTOOL_A_STATS_RMON_MAX = (__ETHTOOL_A_STATS_RMON_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 bd96eb57c07c..8abcbc10796c 100644
--- a/net/ethtool/netlink.h
+++ b/net/ethtool/netlink.h
@@ -405,5 +405,6 @@ extern const char stats_std_names[__ETHTOOL_STATS_CNT][ETH_GSTRING_LEN];
extern const char stats_eth_phy_names[__ETHTOOL_A_STATS_ETH_PHY_CNT][ETH_GSTRING_LEN];
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];
#endif /* _NET_ETHTOOL_NETLINK_H */
diff --git a/net/ethtool/stats.c b/net/ethtool/stats.c
index 5feb6aba26c8..2ce1326299d9 100644
--- a/net/ethtool/stats.c
+++ b/net/ethtool/stats.c
@@ -17,6 +17,8 @@ struct stats_reply_data {
struct ethtool_eth_phy_stats phy_stats;
struct ethtool_eth_mac_stats mac_stats;
struct ethtool_eth_ctrl_stats ctrl_stats;
+ struct ethtool_rmon_stats rmon_stats;
+ const struct ethtool_rmon_hist_range *rmon_ranges;
};
#define STATS_REPDATA(__reply_base) \
@@ -26,6 +28,7 @@ const char stats_std_names[__ETHTOOL_STATS_CNT][ETH_GSTRING_LEN] = {
[ETHTOOL_STATS_ETH_PHY] = "eth-phy",
[ETHTOOL_STATS_ETH_MAC] = "eth-mac",
[ETHTOOL_STATS_ETH_CTRL] = "eth-ctrl",
+ [ETHTOOL_STATS_RMON] = "rmon",
};
const char stats_eth_phy_names[__ETHTOOL_A_STATS_ETH_PHY_CNT][ETH_GSTRING_LEN] = {
@@ -63,6 +66,13 @@ const char stats_eth_ctrl_names[__ETHTOOL_A_STATS_ETH_CTRL_CNT][ETH_GSTRING_LEN]
[ETHTOOL_A_STATS_ETH_CTRL_5_RX_UNSUP] = "UnsupportedOpcodesReceived",
};
+const char stats_rmon_names[__ETHTOOL_A_STATS_RMON_CNT][ETH_GSTRING_LEN] = {
+ [ETHTOOL_A_STATS_RMON_UNDERSIZE] = "etherStatsUndersizePkts",
+ [ETHTOOL_A_STATS_RMON_OVERSIZE] = "etherStatsOversizePkts",
+ [ETHTOOL_A_STATS_RMON_FRAG] = "etherStatsFragments",
+ [ETHTOOL_A_STATS_RMON_JABBER] = "etherStatsJabbers",
+};
+
const struct nla_policy ethnl_stats_get_policy[ETHTOOL_A_STATS_GROUPS + 1] = {
[ETHTOOL_A_STATS_HEADER] =
NLA_POLICY_NESTED(ethnl_header_policy),
@@ -107,6 +117,7 @@ static int stats_prepare_data(const struct ethnl_req_info *req_base,
memset(&data->phy_stats, 0xff, sizeof(data->phy_stats));
memset(&data->mac_stats, 0xff, sizeof(data->mac_stats));
memset(&data->ctrl_stats, 0xff, sizeof(data->mac_stats));
+ 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)
@@ -117,6 +128,10 @@ static int stats_prepare_data(const struct ethnl_req_info *req_base,
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);
+ 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);
ethnl_ops_complete(dev);
return 0;
@@ -140,6 +155,17 @@ static int stats_reply_size(const struct ethnl_req_info *req_base,
len += nla_total_size(0) +
sizeof(struct ethtool_eth_ctrl_stats) / sizeof(u64) *
nla_total_size_64bit(sizeof(u64));
+ if (test_bit(ETHTOOL_STATS_RMON, req_info->stat_mask)) {
+ len += nla_total_size(0) +
+ sizeof(struct ethtool_rmon_stats) / sizeof(u64) *
+ nla_total_size_64bit(sizeof(u64));
+ /* Above includes the space for _A_STATS_RMON_HIST_VALs */
+
+ len += (nla_total_size(0) + /* _A_STATS_RMON_HIST */
+ nla_total_size(4) + /* _A_STATS_RMON_HIST_BKT_LOW */
+ nla_total_size(4)) * /* _A_STATS_RMON_HIST_BKT_HI */
+ ETHTOOL_RMON_HIST_MAX * 2;
+ }
return len;
}
@@ -224,6 +250,59 @@ static int stats_put_ctrl_stats(struct sk_buff *skb,
return 0;
}
+static int stats_put_rmon_hist(struct sk_buff *skb, u32 attr, const u64 *hist,
+ const struct ethtool_rmon_hist_range *ranges)
+{
+ struct nlattr *nest;
+ int i;
+
+ for (i = 0; i < ETHTOOL_RMON_HIST_MAX; i++) {
+ if (!ranges[i].low && !ranges[i].high)
+ break;
+ if (hist[i] == ETHTOOL_STAT_NOT_SET)
+ continue;
+
+ nest = nla_nest_start(skb, attr);
+
+ if (nla_put_u32(skb, ETHTOOL_A_STATS_RMON_HIST_BKT_LOW,
+ ranges[i].low) ||
+ nla_put_u32(skb, ETHTOOL_A_STATS_RMON_HIST_BKT_HI,
+ ranges[i].high) ||
+ stat_put(skb, ETHTOOL_A_STATS_RMON_HIST_VAL, hist[i]))
+ goto err_cancel_hist;
+
+ nla_nest_end(skb, nest);
+ }
+
+ return 0;
+
+err_cancel_hist:
+ nla_nest_cancel(skb, nest);
+ return -EMSGSIZE;
+}
+
+static int stats_put_rmon_stats(struct sk_buff *skb,
+ const struct stats_reply_data *data)
+{
+ if (stats_put_rmon_hist(skb, ETHTOOL_A_STATS_RMON_HIST,
+ data->rmon_stats.hist, data->rmon_ranges) ||
+ stats_put_rmon_hist(skb, ETHTOOL_A_STATS_RMON_HIST_TX,
+ data->rmon_stats.hist_tx, data->rmon_ranges))
+ return -EMSGSIZE;
+
+ if (stat_put(skb, ETHTOOL_A_STATS_RMON_UNDERSIZE,
+ data->rmon_stats.undersize_pkts) ||
+ stat_put(skb, ETHTOOL_A_STATS_RMON_OVERSIZE,
+ data->rmon_stats.oversize_pkts) ||
+ stat_put(skb, ETHTOOL_A_STATS_RMON_FRAG,
+ data->rmon_stats.fragments) ||
+ stat_put(skb, ETHTOOL_A_STATS_RMON_JABBER,
+ data->rmon_stats.jabbers))
+ return -EMSGSIZE;
+
+ return 0;
+}
+
static int stats_put_stats(struct sk_buff *skb,
const struct stats_reply_data *data,
u32 id, u32 ss_id,
@@ -271,6 +350,9 @@ static int stats_fill_reply(struct sk_buff *skb,
ret = stats_put_stats(skb, data, ETHTOOL_STATS_ETH_CTRL,
ETH_SS_STATS_ETH_CTRL,
stats_put_ctrl_stats);
+ 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);
return ret;
}
diff --git a/net/ethtool/strset.c b/net/ethtool/strset.c
index a33c603a7a02..b3029fff715d 100644
--- a/net/ethtool/strset.c
+++ b/net/ethtool/strset.c
@@ -100,6 +100,11 @@ static const struct strset_info info_template[] = {
.count = __ETHTOOL_A_STATS_ETH_CTRL_CNT,
.strings = stats_eth_ctrl_names,
},
+ [ETH_SS_STATS_RMON] = {
+ .per_dev = false,
+ .count = __ETHTOOL_A_STATS_RMON_CNT,
+ .strings = stats_rmon_names,
+ },
};
struct strset_req_info {
--
2.30.2
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [RFC net-next 0/6] ethtool: add uAPI for reading standard stats
2021-04-14 20:23 [RFC net-next 0/6] ethtool: add uAPI for reading standard stats Jakub Kicinski
` (5 preceding siblings ...)
2021-04-14 20:23 ` [RFC net-next 6/6] ethtool: add interface to read RMON stats Jakub Kicinski
@ 2021-04-15 5:51 ` Saeed Mahameed
2021-04-15 15:45 ` Jakub Kicinski
6 siblings, 1 reply; 14+ messages in thread
From: Saeed Mahameed @ 2021-04-15 5:51 UTC (permalink / raw)
To: Jakub Kicinski, netdev; +Cc: davem, andrew, mkubecek, idosch, Ariel Almog
On Wed, 2021-04-14 at 13:23 -0700, Jakub Kicinski wrote:
> This series adds a new ethtool command to read well defined
> device statistics. There is nothing clever here, just a netlink
> API for dumping statistics defined by standards and RFCs which
> today end up in ethtool -S under infinite variations of names.
>
> This series adds basic IEEE stats (for PHY, MAC, Ctrl frames)
> and RMON stats. AFAICT other RFCs only duplicate the IEEE
> stats.
>
> This series does _not_ add a netlink API to read driver-defined
> stats. There seems to be little to gain from moving that part
> to netlink.
>
> The netlink message format is very simple, and aims to allow
> adding stats and groups with no changes to user tooling (which
> IIUC is expected for ethtool). Stats are dumped directly
> into netlink with netlink attributes used as IDs. This is
> perhaps where the biggest question mark is. We could instead
> pack the stats into individual wrappers:
>
> [grp]
> [stat] // nest
> [id] // u32
> [value] // u64
> [stat] // nest
> [id] // u32
> [value] // u64
>
> which would increase the message size 2x but allow
> to ID the stats from 0, saving strset space as well as
don't you need to translate such ids to strs in userspace ?
I am not fond of upgrading userspace every time we add new stat..
Just throwing crazy ideas.. BTF might be a useful tool here! :))
> allow seamless adding of legacy stats to this API
which legacy stats ?
> (which are IDed from 0).
>
> On user space side we can re-use -S, and make it dump
> standard stats if --groups are defined.
>
> $ ethtool -S eth0 --groups eth-phy eth-mac rmon eth-ctrl
Deja-vu, I honestly remember someone in mlnx suggsting this exact
command a couple of years ago.. :)
> Stats for eth0:
> eth-phy-SymbolErrorDuringCarrier: 0
> eth-mac-FramesTransmittedOK: 0
> eth-mac-FrameTooLongErrors: 0
> eth-ctrl-MACControlFramesTransmitted: 0
> eth-ctrl-MACControlFramesReceived: 1
> eth-ctrl-UnsupportedOpcodesReceived: 0
> rmon-etherStatsUndersizePkts: 0
> rmon-etherStatsJabbers: 0
> rmon-rx-etherStatsPkts64Octets: 1
> rmon-rx-etherStatsPkts128to255Octets: 0
> rmon-rx-etherStatsPkts1024toMaxOctets: 1
> rmon-tx-etherStatsPkts64Octets: 1
> rmon-tx-etherStatsPkts128to255Octets: 0
> rmon-tx-etherStatsPkts1024toMaxOctets: 1
>
> Jakub Kicinski (6):
> docs: networking: extend the statistics documentation
> docs: ethtool: document standard statistics
> ethtool: add a new command for reading standard stats
> ethtool: add interface to read standard MAC stats
> ethtool: add interface to read standard MAC Ctrl stats
> ethtool: add interface to read RMON stats
>
> Documentation/networking/ethtool-netlink.rst | 74 ++++
> Documentation/networking/statistics.rst | 44 ++-
> include/linux/ethtool.h | 95 +++++
> include/uapi/linux/ethtool.h | 10 +
> include/uapi/linux/ethtool_netlink.h | 134 +++++++
> net/ethtool/Makefile | 2 +-
> net/ethtool/netlink.c | 10 +
> net/ethtool/netlink.h | 8 +
> net/ethtool/stats.c | 374
> +++++++++++++++++++
> net/ethtool/strset.c | 25 ++
> 10 files changed, 773 insertions(+), 3 deletions(-)
> create mode 100644 net/ethtool/stats.c
>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [RFC net-next 0/6] ethtool: add uAPI for reading standard stats
2021-04-15 5:51 ` [RFC net-next 0/6] ethtool: add uAPI for reading standard stats Saeed Mahameed
@ 2021-04-15 15:45 ` Jakub Kicinski
0 siblings, 0 replies; 14+ messages in thread
From: Jakub Kicinski @ 2021-04-15 15:45 UTC (permalink / raw)
To: Saeed Mahameed; +Cc: netdev, davem, andrew, mkubecek, idosch, Ariel Almog
On Wed, 14 Apr 2021 22:51:39 -0700 Saeed Mahameed wrote:
> On Wed, 2021-04-14 at 13:23 -0700, Jakub Kicinski wrote:
> > This series adds a new ethtool command to read well defined
> > device statistics. There is nothing clever here, just a netlink
> > API for dumping statistics defined by standards and RFCs which
> > today end up in ethtool -S under infinite variations of names.
> >
> > This series adds basic IEEE stats (for PHY, MAC, Ctrl frames)
> > and RMON stats. AFAICT other RFCs only duplicate the IEEE
> > stats.
> >
> > This series does _not_ add a netlink API to read driver-defined
> > stats. There seems to be little to gain from moving that part
> > to netlink.
> >
> > The netlink message format is very simple, and aims to allow
> > adding stats and groups with no changes to user tooling (which
> > IIUC is expected for ethtool). Stats are dumped directly
> > into netlink with netlink attributes used as IDs. This is
> > perhaps where the biggest question mark is. We could instead
> > pack the stats into individual wrappers:
> >
> > [grp]
> > [stat] // nest
> > [id] // u32
> > [value] // u64
> > [stat] // nest
> > [id] // u32
> > [value] // u64
> >
> > which would increase the message size 2x but allow
> > to ID the stats from 0, saving strset space as well as
>
> don't you need to translate such ids to strs in userspace ?
> I am not fond of upgrading userspace every time we add new stat..
No, no, the question was only whether we keep stat ids in the same
attribute space as other group attributes (like string set ID) or
whether they are nested somewhere deeper and have their own ID space.
I went ahead and nested them yesterday. I had to engage in a little
bit of black magic for pad, but it feels more right to nest..
static int stat_put(struct sk_buff *skb, u16 attrtype, u64 val)
{
struct nlattr *nest;
int ret;
if (val == ETHTOOL_STAT_NOT_SET)
return 0;
/* We want to start stats attr types from 0, so we don't have a type
* for pad inside ETHTOOL_A_STATS_GRP_STAT. Pad things on the outside
* of ETHTOOL_A_STATS_GRP_STAT. Since we're one nest away from the
* actual attr we're 4B off - nla_need_padding_for_64bit() & co.
* can't be used.
*/
#ifndef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
if (!IS_ALIGNED((unsigned long)skb_tail_pointer(skb), 8))
if (!nla_reserve(skb, ETHTOOL_A_STATS_GRP_PAD, 0))
return -EMSGSIZE;
#endif
nest = nla_nest_start(skb, ETHTOOL_A_STATS_GRP_STAT);
if (!nest)
return -EMSGSIZE;
ret = nla_put_u64_64bit(skb, attrtype, val, -1 /* not used */);
if (ret) {
nla_nest_cancel(skb, nest);
return ret;
}
nla_nest_end(skb, nest);
return 0;
}
> Just throwing crazy ideas.. BTF might be a useful tool here! :))
>
> > allow seamless adding of legacy stats to this API
> which legacy stats ?
The ones from get_ethtool_stats.
> > (which are IDed from 0).
> >
> > On user space side we can re-use -S, and make it dump
> > standard stats if --groups are defined.
> >
> > $ ethtool -S eth0 --groups eth-phy eth-mac rmon eth-ctrl
>
> Deja-vu, I honestly remember someone in mlnx suggsting this exact
> command a couple of years ago.. :)
Hah! I hope it wasn't shot down as a bad idea :)
Thanks a lot for the reviews!
^ permalink raw reply [flat|nested] 14+ messages in thread