All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC ethtool 0/6] ethtool: add FEC & std stats support
@ 2021-04-16 16:02 Jakub Kicinski
  2021-04-16 16:02 ` [RFC ethtool 1/6] update UAPI header copies Jakub Kicinski
                   ` (5 more replies)
  0 siblings, 6 replies; 11+ messages in thread
From: Jakub Kicinski @ 2021-04-16 16:02 UTC (permalink / raw)
  To: netdev, idosch; +Cc: mkubecek, Jakub Kicinski

Sending these as RFC for testing of the kernel side.

I need to look through them again to double check for bugs,
also the header update (patch 1) refers to a hash from my
local tree.

Jakub Kicinski (6):
  update UAPI header copies
  json: simplify array print API
  netlink: add FEC support
  netlink: fec: support displaying statistics
  ethtool: add nlchk for redirecting to netlink
  netlink: add support for standard stats

 Makefile.am                  |   3 +-
 ethtool.8.in                 |  11 ++
 ethtool.c                    |  12 +-
 json_print.c                 |  20 +-
 json_print.h                 |   4 +-
 netlink/desc-ethtool.c       |  51 +++++
 netlink/extapi.h             |  13 +-
 netlink/fec.c                | 356 +++++++++++++++++++++++++++++++++++
 netlink/monitor.c            |   4 +
 netlink/netlink.c            |   9 +-
 netlink/netlink.h            |   1 +
 netlink/stats.c              | 242 ++++++++++++++++++++++++
 uapi/linux/ethtool.h         | 111 +++++++----
 uapi/linux/ethtool_netlink.h | 188 ++++++++++++++++++
 uapi/linux/if_link.h         |   9 +-
 uapi/linux/netlink.h         |   2 +-
 uapi/linux/rtnetlink.h       |  33 +++-
 17 files changed, 1009 insertions(+), 60 deletions(-)
 create mode 100644 netlink/fec.c
 create mode 100644 netlink/stats.c

-- 
2.30.2


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [RFC ethtool 1/6] update UAPI header copies
  2021-04-16 16:02 [RFC ethtool 0/6] ethtool: add FEC & std stats support Jakub Kicinski
@ 2021-04-16 16:02 ` Jakub Kicinski
  2021-04-16 16:02 ` [RFC ethtool 2/6] json: simplify array print API Jakub Kicinski
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 11+ messages in thread
From: Jakub Kicinski @ 2021-04-16 16:02 UTC (permalink / raw)
  To: netdev, idosch; +Cc: mkubecek, Jakub Kicinski

Update to kernel commit 6917719f1b85.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 uapi/linux/ethtool.h         | 111 +++++++++++++++------
 uapi/linux/ethtool_netlink.h | 188 +++++++++++++++++++++++++++++++++++
 uapi/linux/if_link.h         |   9 +-
 uapi/linux/netlink.h         |   2 +-
 uapi/linux/rtnetlink.h       |  33 +++++-
 5 files changed, 302 insertions(+), 41 deletions(-)

diff --git a/uapi/linux/ethtool.h b/uapi/linux/ethtool.h
index 052689bcc90c..c6ec1111ffa3 100644
--- a/uapi/linux/ethtool.h
+++ b/uapi/linux/ethtool.h
@@ -14,7 +14,7 @@
 #ifndef _LINUX_ETHTOOL_H
 #define _LINUX_ETHTOOL_H
 
-#include <linux/kernel.h>
+#include <linux/const.h>
 #include <linux/types.h>
 #include <linux/if_ether.h>
 
@@ -24,6 +24,14 @@
  * have the same layout for 32-bit and 64-bit userland.
  */
 
+/* Note on reserved space.
+ * Reserved fields must not be accessed directly by user space because
+ * they may be replaced by a different field in the future. They must
+ * be initialized to zero before making the request, e.g. via memset
+ * of the entire structure or implicitly by not being set in a structure
+ * initializer.
+ */
+
 /**
  * struct ethtool_cmd - DEPRECATED, link control and status
  * This structure is DEPRECATED, please use struct ethtool_link_settings.
@@ -65,6 +73,7 @@
  *	and other link features that the link partner advertised
  *	through autonegotiation; 0 if unknown or not applicable.
  *	Read-only.
+ * @reserved: Reserved for future use; see the note on reserved space.
  *
  * The link speed in Mbps is split between @speed and @speed_hi.  Use
  * the ethtool_cmd_speed() and ethtool_cmd_speed_set() functions to
@@ -153,6 +162,7 @@ static __inline__ __u32 ethtool_cmd_speed(const struct ethtool_cmd *ep)
  * @bus_info: Device bus address.  This should match the dev_name()
  *	string for the underlying bus device, if there is one.  May be
  *	an empty string.
+ * @reserved2: Reserved for future use; see the note on reserved space.
  * @n_priv_flags: Number of flags valid for %ETHTOOL_GPFLAGS and
  *	%ETHTOOL_SPFLAGS commands; also the number of strings in the
  *	%ETH_SS_PRIV_FLAGS set
@@ -354,6 +364,7 @@ struct ethtool_eeprom {
  * @tx_lpi_timer: Time in microseconds the interface delays prior to asserting
  *	its tx lpi (after reaching 'idle' state). Effective only when eee
  *	was negotiated and tx_lpi_enabled was set.
+ * @reserved: Reserved for future use; see the note on reserved space.
  */
 struct ethtool_eee {
 	__u32	cmd;
@@ -372,6 +383,7 @@ struct ethtool_eee {
  * @cmd: %ETHTOOL_GMODULEINFO
  * @type: Standard the module information conforms to %ETH_MODULE_SFF_xxxx
  * @eeprom_len: Length of the eeprom
+ * @reserved: Reserved for future use; see the note on reserved space.
  *
  * This structure is used to return the information to
  * properly size memory for a subsequent call to %ETHTOOL_GMODULEEEPROM.
@@ -577,9 +589,7 @@ struct ethtool_pauseparam {
 	__u32	tx_pause;
 };
 
-/**
- * enum ethtool_link_ext_state - link extended state
- */
+/* Link extended state */
 enum ethtool_link_ext_state {
 	ETHTOOL_LINK_EXT_STATE_AUTONEG,
 	ETHTOOL_LINK_EXT_STATE_LINK_TRAINING_FAILURE,
@@ -593,10 +603,7 @@ enum ethtool_link_ext_state {
 	ETHTOOL_LINK_EXT_STATE_OVERHEAT,
 };
 
-/**
- * enum ethtool_link_ext_substate_autoneg - more information in addition to
- * ETHTOOL_LINK_EXT_STATE_AUTONEG.
- */
+/* More information in addition to ETHTOOL_LINK_EXT_STATE_AUTONEG. */
 enum ethtool_link_ext_substate_autoneg {
 	ETHTOOL_LINK_EXT_SUBSTATE_AN_NO_PARTNER_DETECTED = 1,
 	ETHTOOL_LINK_EXT_SUBSTATE_AN_ACK_NOT_RECEIVED,
@@ -606,9 +613,7 @@ enum ethtool_link_ext_substate_autoneg {
 	ETHTOOL_LINK_EXT_SUBSTATE_AN_NO_HCD,
 };
 
-/**
- * enum ethtool_link_ext_substate_link_training - more information in addition to
- * ETHTOOL_LINK_EXT_STATE_LINK_TRAINING_FAILURE.
+/* More information in addition to ETHTOOL_LINK_EXT_STATE_LINK_TRAINING_FAILURE.
  */
 enum ethtool_link_ext_substate_link_training {
 	ETHTOOL_LINK_EXT_SUBSTATE_LT_KR_FRAME_LOCK_NOT_ACQUIRED = 1,
@@ -617,9 +622,7 @@ enum ethtool_link_ext_substate_link_training {
 	ETHTOOL_LINK_EXT_SUBSTATE_LT_REMOTE_FAULT,
 };
 
-/**
- * enum ethtool_link_ext_substate_logical_mismatch - more information in addition
- * to ETHTOOL_LINK_EXT_STATE_LINK_LOGICAL_MISMATCH.
+/* More information in addition to ETHTOOL_LINK_EXT_STATE_LINK_LOGICAL_MISMATCH.
  */
 enum ethtool_link_ext_substate_link_logical_mismatch {
 	ETHTOOL_LINK_EXT_SUBSTATE_LLM_PCS_DID_NOT_ACQUIRE_BLOCK_LOCK = 1,
@@ -629,19 +632,14 @@ enum ethtool_link_ext_substate_link_logical_mismatch {
 	ETHTOOL_LINK_EXT_SUBSTATE_LLM_RS_FEC_IS_NOT_LOCKED,
 };
 
-/**
- * enum ethtool_link_ext_substate_bad_signal_integrity - more information in
- * addition to ETHTOOL_LINK_EXT_STATE_BAD_SIGNAL_INTEGRITY.
+/* More information in addition to ETHTOOL_LINK_EXT_STATE_BAD_SIGNAL_INTEGRITY.
  */
 enum ethtool_link_ext_substate_bad_signal_integrity {
 	ETHTOOL_LINK_EXT_SUBSTATE_BSI_LARGE_NUMBER_OF_PHYSICAL_ERRORS = 1,
 	ETHTOOL_LINK_EXT_SUBSTATE_BSI_UNSUPPORTED_RATE,
 };
 
-/**
- * enum ethtool_link_ext_substate_cable_issue - more information in
- * addition to ETHTOOL_LINK_EXT_STATE_CABLE_ISSUE.
- */
+/* More information in addition to ETHTOOL_LINK_EXT_STATE_CABLE_ISSUE. */
 enum ethtool_link_ext_substate_cable_issue {
 	ETHTOOL_LINK_EXT_SUBSTATE_CI_UNSUPPORTED_CABLE = 1,
 	ETHTOOL_LINK_EXT_SUBSTATE_CI_CABLE_TEST_FAILURE,
@@ -659,6 +657,7 @@ enum ethtool_link_ext_substate_cable_issue {
  *	now deprecated
  * @ETH_SS_FEATURES: Device feature names
  * @ETH_SS_RSS_HASH_FUNCS: RSS hush function names
+ * @ETH_SS_TUNABLES: tunable names
  * @ETH_SS_PHY_STATS: Statistic names, for use with %ETHTOOL_GPHYSTATS
  * @ETH_SS_PHY_TUNABLES: PHY tunable names
  * @ETH_SS_LINK_MODES: link mode names
@@ -668,6 +667,13 @@ 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_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
  */
 enum ethtool_stringset {
 	ETH_SS_TEST		= 0,
@@ -686,6 +692,11 @@ 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,
+	ETH_SS_STATS_ETH_MAC,
+	ETH_SS_STATS_ETH_CTRL,
+	ETH_SS_STATS_RMON,
 
 	/* add new constants above here */
 	ETH_SS_COUNT
@@ -713,6 +724,7 @@ struct ethtool_gstrings {
 /**
  * struct ethtool_sset_info - string set information
  * @cmd: Command number = %ETHTOOL_GSSET_INFO
+ * @reserved: Reserved for future use; see the note on reserved space.
  * @sset_mask: On entry, a bitmask of string sets to query, with bits
  *	numbered according to &enum ethtool_stringset.  On return, a
  *	bitmask of those string sets queried that are supported.
@@ -757,6 +769,7 @@ enum ethtool_test_flags {
  * @flags: A bitmask of flags from &enum ethtool_test_flags.  Some
  *	flags may be set by the user on entry; others may be set by
  *	the driver on return.
+ * @reserved: Reserved for future use; see the note on reserved space.
  * @len: On return, the number of test results
  * @data: Array of test results
  *
@@ -957,6 +970,7 @@ union ethtool_flow_union {
  * @vlan_etype: VLAN EtherType
  * @vlan_tci: VLAN tag control information
  * @data: user defined data
+ * @padding: Reserved for future use; see the note on reserved space.
  *
  * Note, @vlan_etype, @vlan_tci, and @data are only valid if %FLOW_EXT
  * is set in &struct ethtool_rx_flow_spec @flow_type.
@@ -1132,7 +1146,8 @@ struct ethtool_rxfh_indir {
  *	hardware hash key.
  * @hfunc: Defines the current RSS hash function used by HW (or to be set to).
  *	Valid values are one of the %ETH_RSS_HASH_*.
- * @rsvd:	Reserved for future extensions.
+ * @rsvd8: Reserved for future use; see the note on reserved space.
+ * @rsvd32: Reserved for future use; see the note on reserved space.
  * @rss_config: RX ring/queue index for each hash value i.e., indirection table
  *	of @indir_size __u32 elements, followed by hash key of @key_size
  *	bytes.
@@ -1300,7 +1315,9 @@ struct ethtool_sfeatures {
  * @so_timestamping: bit mask of the sum of the supported SO_TIMESTAMPING flags
  * @phc_index: device index of the associated PHC, or -1 if there is none
  * @tx_types: bit mask of the supported hwtstamp_tx_types enumeration values
+ * @tx_reserved: Reserved for future use; see the note on reserved space.
  * @rx_filters: bit mask of the supported hwtstamp_rx_filters enumeration values
+ * @rx_reserved: Reserved for future use; see the note on reserved space.
  *
  * The bits in the 'tx_types' and 'rx_filters' fields correspond to
  * the 'hwtstamp_tx_types' and 'hwtstamp_rx_filters' enumeration values,
@@ -1374,15 +1391,33 @@ struct ethtool_per_queue_op {
 };
 
 /**
- * struct ethtool_fecparam - Ethernet forward error correction(fec) parameters
+ * struct ethtool_fecparam - Ethernet Forward Error Correction parameters
  * @cmd: Command number = %ETHTOOL_GFECPARAM or %ETHTOOL_SFECPARAM
- * @active_fec: FEC mode which is active on porte
- * @fec: Bitmask of supported/configured FEC modes
- * @rsvd: Reserved for future extensions. i.e FEC bypass feature.
+ * @active_fec: FEC mode which is active on the port, single bit set, GET only.
+ * @fec: Bitmask of configured FEC modes.
+ * @reserved: Reserved for future extensions, ignore on GET, write 0 for SET.
  *
- * Drivers should reject a non-zero setting of @autoneg when
- * autoneogotiation is disabled (or not supported) for the link.
+ * Note that @reserved was never validated on input and ethtool user space
+ * left it uninitialized when calling SET. Hence going forward it can only be
+ * used to return a value to userspace with GET.
+ *
+ * FEC modes supported by the device can be read via %ETHTOOL_GLINKSETTINGS.
+ * FEC settings are configured by link autonegotiation whenever it's enabled.
+ * With autoneg on %ETHTOOL_GFECPARAM can be used to read the current mode.
+ *
+ * When autoneg is disabled %ETHTOOL_SFECPARAM controls the FEC settings.
+ * It is recommended that drivers only accept a single bit set in @fec.
+ * When multiple bits are set in @fec drivers may pick mode in an implementation
+ * dependent way. Drivers should reject mixing %ETHTOOL_FEC_AUTO_BIT with other
+ * FEC modes, because it's unclear whether in this case other modes constrain
+ * AUTO or are independent choices.
+ * Drivers must reject SET requests if they support none of the requested modes.
+ *
+ * If device does not support FEC drivers may use %ETHTOOL_FEC_NONE instead
+ * of returning %EOPNOTSUPP from %ETHTOOL_GFECPARAM.
  *
+ * See enum ethtool_fec_config_bits for definition of valid bits for both
+ * @fec and @active_fec.
  */
 struct ethtool_fecparam {
 	__u32   cmd;
@@ -1394,11 +1429,16 @@ struct ethtool_fecparam {
 
 /**
  * enum ethtool_fec_config_bits - flags definition of ethtool_fec_configuration
- * @ETHTOOL_FEC_NONE: FEC mode configuration is not supported
- * @ETHTOOL_FEC_AUTO: Default/Best FEC mode provided by driver
- * @ETHTOOL_FEC_OFF: No FEC Mode
- * @ETHTOOL_FEC_RS: Reed-Solomon Forward Error Detection mode
- * @ETHTOOL_FEC_BASER: Base-R/Reed-Solomon Forward Error Detection mode
+ * @ETHTOOL_FEC_NONE_BIT: FEC mode configuration is not supported. Should not
+ *			be used together with other bits. GET only.
+ * @ETHTOOL_FEC_AUTO_BIT: Select default/best FEC mode automatically, usually
+ *			based link mode and SFP parameters read from module's
+ *			EEPROM. This bit does _not_ mean autonegotiation.
+ * @ETHTOOL_FEC_OFF_BIT: No FEC Mode
+ * @ETHTOOL_FEC_RS_BIT: Reed-Solomon FEC Mode
+ * @ETHTOOL_FEC_BASER_BIT: Base-R/Reed-Solomon FEC Mode
+ * @ETHTOOL_FEC_LLRS_BIT: Low Latency Reed Solomon FEC Mode (25G/50G Ethernet
+ *			Consortium)
  */
 enum ethtool_fec_config_bits {
 	ETHTOOL_FEC_NONE_BIT,
@@ -1956,6 +1996,11 @@ enum ethtool_reset_flags {
  *	autonegotiation; 0 if unknown or not applicable.  Read-only.
  * @transceiver: Used to distinguish different possible PHY types,
  *	reported consistently by PHYLIB.  Read-only.
+ * @master_slave_cfg: Master/slave port mode.
+ * @master_slave_state: Master/slave port state.
+ * @reserved: Reserved for future use; see the note on reserved space.
+ * @reserved1: Reserved for future use; see the note on reserved space.
+ * @link_mode_masks: Variable length bitmaps.
  *
  * If autonegotiation is disabled, the speed and @duplex represent the
  * fixed link mode and are writable if the driver supports multiple
diff --git a/uapi/linux/ethtool_netlink.h b/uapi/linux/ethtool_netlink.h
index c022883cdb22..4653c4c79972 100644
--- a/uapi/linux/ethtool_netlink.h
+++ b/uapi/linux/ethtool_netlink.h
@@ -42,6 +42,10 @@ enum {
 	ETHTOOL_MSG_CABLE_TEST_ACT,
 	ETHTOOL_MSG_CABLE_TEST_TDR_ACT,
 	ETHTOOL_MSG_TUNNEL_INFO_GET,
+	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,
@@ -80,6 +84,10 @@ enum {
 	ETHTOOL_MSG_CABLE_TEST_NTF,
 	ETHTOOL_MSG_CABLE_TEST_TDR_NTF,
 	ETHTOOL_MSG_TUNNEL_INFO_GET_REPLY,
+	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,
@@ -227,6 +235,7 @@ enum {
 	ETHTOOL_A_LINKMODES_DUPLEX,		/* u8 */
 	ETHTOOL_A_LINKMODES_MASTER_SLAVE_CFG,	/* u8 */
 	ETHTOOL_A_LINKMODES_MASTER_SLAVE_STATE,	/* u8 */
+	ETHTOOL_A_LINKMODES_LANES,		/* u32 */
 
 	/* add new constants above here */
 	__ETHTOOL_A_LINKMODES_CNT,
@@ -628,6 +637,185 @@ enum {
 	ETHTOOL_A_TUNNEL_INFO_MAX = (__ETHTOOL_A_TUNNEL_INFO_CNT - 1)
 };
 
+/* FEC */
+
+enum {
+	ETHTOOL_A_FEC_UNSPEC,
+	ETHTOOL_A_FEC_HEADER,				/* nest - _A_HEADER_* */
+	ETHTOOL_A_FEC_MODES,				/* bitset */
+	ETHTOOL_A_FEC_AUTO,				/* u8 */
+	ETHTOOL_A_FEC_ACTIVE,				/* u32 */
+	ETHTOOL_A_FEC_STATS,				/* nest - _A_FEC_STAT */
+
+	__ETHTOOL_A_FEC_CNT,
+	ETHTOOL_A_FEC_MAX = (__ETHTOOL_A_FEC_CNT - 1)
+};
+
+enum {
+	ETHTOOL_A_FEC_STAT_UNSPEC,
+	ETHTOOL_A_FEC_STAT_PAD,
+
+	ETHTOOL_A_FEC_STAT_CORRECTED,			/* array, u64 */
+	ETHTOOL_A_FEC_STAT_UNCORR,			/* array, u64 */
+	ETHTOOL_A_FEC_STAT_CORR_BITS,			/* array, u64 */
+
+	/* add new constants above here */
+	__ETHTOOL_A_FEC_STAT_CNT,
+	ETHTOOL_A_FEC_STAT_MAX = (__ETHTOOL_A_FEC_STAT_CNT - 1)
+};
+
+/* MODULE EEPROM */
+
+enum {
+	ETHTOOL_A_MODULE_EEPROM_UNSPEC,
+	ETHTOOL_A_MODULE_EEPROM_HEADER,			/* nest - _A_HEADER_* */
+
+	ETHTOOL_A_MODULE_EEPROM_OFFSET,			/* u32 */
+	ETHTOOL_A_MODULE_EEPROM_LENGTH,			/* u32 */
+	ETHTOOL_A_MODULE_EEPROM_PAGE,			/* u8 */
+	ETHTOOL_A_MODULE_EEPROM_BANK,			/* u8 */
+	ETHTOOL_A_MODULE_EEPROM_I2C_ADDRESS,		/* u8 */
+	ETHTOOL_A_MODULE_EEPROM_DATA,			/* nested */
+
+	__ETHTOOL_A_MODULE_EEPROM_CNT,
+	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,
+	ETHTOOL_STATS_ETH_MAC,
+	ETHTOOL_STATS_ETH_CTRL,
+	ETHTOOL_STATS_RMON,
+
+	/* 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 */
+
+	ETHTOOL_A_STATS_GRP_STAT,		/* nest */
+
+	ETHTOOL_A_STATS_GRP_HIST_RX,		/* nest */
+	ETHTOOL_A_STATS_GRP_HIST_TX,		/* nest */
+
+	ETHTOOL_A_STATS_GRP_HIST_BKT_LOW,	/* u32 */
+	ETHTOOL_A_STATS_GRP_HIST_BKT_HI,	/* u32 */
+	ETHTOOL_A_STATS_GRP_HIST_VAL,		/* u64 */
+
+	/* add new constants above here */
+	__ETHTOOL_A_STATS_GRP_CNT,
+	ETHTOOL_A_STATS_GRP_MAX = (__ETHTOOL_A_STATS_CNT - 1)
+};
+
+enum {
+	/* 30.3.2.1.5 aSymbolErrorDuringCarrier */
+	ETHTOOL_A_STATS_ETH_PHY_5_SYM_ERR,
+
+	/* add new constants above here */
+	__ETHTOOL_A_STATS_ETH_PHY_CNT,
+	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,
+	/* 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)
+};
+
+enum {
+	/* 30.3.3.3 aMACControlFramesTransmitted */
+	ETHTOOL_A_STATS_ETH_CTRL_3_TX,
+	/* 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)
+};
+
+enum {
+	/* 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/uapi/linux/if_link.h b/uapi/linux/if_link.h
index 307e5c245e9f..50193377e5e4 100644
--- a/uapi/linux/if_link.h
+++ b/uapi/linux/if_link.h
@@ -75,8 +75,9 @@ struct rtnl_link_stats {
  *
  * @rx_dropped: Number of packets received but not processed,
  *   e.g. due to lack of resources or unsupported protocol.
- *   For hardware interfaces this counter should not include packets
- *   dropped by the device which are counted separately in
+ *   For hardware interfaces this counter may include packets discarded
+ *   due to L2 address filtering but should not include packets dropped
+ *   by the device due to buffer exhaustion which are counted separately in
  *   @rx_missed_errors (since procfs folds those two counters together).
  *
  * @tx_dropped: Number of packets dropped on their way to transmission,
@@ -522,6 +523,8 @@ enum {
 	IFLA_BRPORT_BACKUP_PORT,
 	IFLA_BRPORT_MRP_RING_OPEN,
 	IFLA_BRPORT_MRP_IN_OPEN,
+	IFLA_BRPORT_MCAST_EHT_HOSTS_LIMIT,
+	IFLA_BRPORT_MCAST_EHT_HOSTS_CNT,
 	__IFLA_BRPORT_MAX
 };
 #define IFLA_BRPORT_MAX (__IFLA_BRPORT_MAX - 1)
@@ -586,6 +589,8 @@ enum {
 	IFLA_MACVLAN_MACADDR,
 	IFLA_MACVLAN_MACADDR_DATA,
 	IFLA_MACVLAN_MACADDR_COUNT,
+	IFLA_MACVLAN_BC_QUEUE_LEN,
+	IFLA_MACVLAN_BC_QUEUE_LEN_USED,
 	__IFLA_MACVLAN_MAX,
 };
 
diff --git a/uapi/linux/netlink.h b/uapi/linux/netlink.h
index dfef006be9f9..5024c5435749 100644
--- a/uapi/linux/netlink.h
+++ b/uapi/linux/netlink.h
@@ -2,7 +2,7 @@
 #ifndef __LINUX_NETLINK_H
 #define __LINUX_NETLINK_H
 
-#include <linux/kernel.h>
+#include <linux/const.h>
 #include <linux/socket.h> /* for __kernel_sa_family_t */
 #include <linux/types.h>
 
diff --git a/uapi/linux/rtnetlink.h b/uapi/linux/rtnetlink.h
index 5ad84e663d01..e01efa281bdc 100644
--- a/uapi/linux/rtnetlink.h
+++ b/uapi/linux/rtnetlink.h
@@ -178,6 +178,13 @@ enum {
 	RTM_GETVLAN,
 #define RTM_GETVLAN	RTM_GETVLAN
 
+	RTM_NEWNEXTHOPBUCKET = 116,
+#define RTM_NEWNEXTHOPBUCKET	RTM_NEWNEXTHOPBUCKET
+	RTM_DELNEXTHOPBUCKET,
+#define RTM_DELNEXTHOPBUCKET	RTM_DELNEXTHOPBUCKET
+	RTM_GETNEXTHOPBUCKET,
+#define RTM_GETNEXTHOPBUCKET	RTM_GETNEXTHOPBUCKET
+
 	__RTM_MAX,
 #define RTM_MAX		(((__RTM_MAX + 3) & ~3) - 1)
 };
@@ -283,6 +290,7 @@ enum {
 #define RTPROT_MROUTED		17	/* Multicast daemon */
 #define RTPROT_KEEPALIVED	18	/* Keepalived daemon */
 #define RTPROT_BABEL		42	/* Babel daemon */
+#define RTPROT_OPENR		99	/* Open Routing (Open/R) Routes */
 #define RTPROT_BGP		186	/* BGP Routes */
 #define RTPROT_ISIS		187	/* ISIS Routes */
 #define RTPROT_OSPF		188	/* OSPF Routes */
@@ -319,6 +327,11 @@ enum rt_scope_t {
 #define RTM_F_FIB_MATCH	        0x2000	/* return full fib lookup match */
 #define RTM_F_OFFLOAD		0x4000	/* route is offloaded */
 #define RTM_F_TRAP		0x8000	/* route is trapping packets */
+#define RTM_F_OFFLOAD_FAILED	0x20000000 /* route offload failed, this value
+					    * is chosen to avoid conflicts with
+					    * other flags defined in
+					    * include/uapi/linux/ipv6_route.h
+					    */
 
 /* Reserved table identifiers */
 
@@ -396,11 +409,13 @@ struct rtnexthop {
 #define RTNH_F_DEAD		1	/* Nexthop is dead (used by multipath)	*/
 #define RTNH_F_PERVASIVE	2	/* Do recursive gateway lookup	*/
 #define RTNH_F_ONLINK		4	/* Gateway is forced on link	*/
-#define RTNH_F_OFFLOAD		8	/* offloaded route */
+#define RTNH_F_OFFLOAD		8	/* Nexthop is offloaded */
 #define RTNH_F_LINKDOWN		16	/* carrier-down on nexthop */
 #define RTNH_F_UNRESOLVED	32	/* The entry is unresolved (ipmr) */
+#define RTNH_F_TRAP		64	/* Nexthop is trapping packets */
 
-#define RTNH_COMPARE_MASK	(RTNH_F_DEAD | RTNH_F_LINKDOWN | RTNH_F_OFFLOAD)
+#define RTNH_COMPARE_MASK	(RTNH_F_DEAD | RTNH_F_LINKDOWN | \
+				 RTNH_F_OFFLOAD | RTNH_F_TRAP)
 
 /* Macros to handle hexthops */
 
@@ -764,12 +779,18 @@ enum {
 #define TA_PAYLOAD(n) NLMSG_PAYLOAD(n,sizeof(struct tcamsg))
 /* tcamsg flags stored in attribute TCA_ROOT_FLAGS
  *
- * TCA_FLAG_LARGE_DUMP_ON user->kernel to request for larger than TCA_ACT_MAX_PRIO
- * actions in a dump. All dump responses will contain the number of actions
- * being dumped stored in for user app's consumption in TCA_ROOT_COUNT
+ * TCA_ACT_FLAG_LARGE_DUMP_ON user->kernel to request for larger than
+ * TCA_ACT_MAX_PRIO actions in a dump. All dump responses will contain the
+ * number of actions being dumped stored in for user app's consumption in
+ * TCA_ROOT_COUNT
+ *
+ * TCA_ACT_FLAG_TERSE_DUMP user->kernel to request terse (brief) dump that only
+ * includes essential action info (kind, index, etc.)
  *
  */
 #define TCA_FLAG_LARGE_DUMP_ON		(1 << 0)
+#define TCA_ACT_FLAG_LARGE_DUMP_ON	TCA_FLAG_LARGE_DUMP_ON
+#define TCA_ACT_FLAG_TERSE_DUMP		(1 << 1)
 
 /* New extended info filters for IFLA_EXT_MASK */
 #define RTEXT_FILTER_VF		(1 << 0)
@@ -777,6 +798,8 @@ enum {
 #define RTEXT_FILTER_BRVLAN_COMPRESSED	(1 << 2)
 #define	RTEXT_FILTER_SKIP_STATS	(1 << 3)
 #define RTEXT_FILTER_MRP	(1 << 4)
+#define RTEXT_FILTER_CFM_CONFIG	(1 << 5)
+#define RTEXT_FILTER_CFM_STATUS	(1 << 6)
 
 /* End of information exported to user level */
 
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [RFC ethtool 2/6] json: simplify array print API
  2021-04-16 16:02 [RFC ethtool 0/6] ethtool: add FEC & std stats support Jakub Kicinski
  2021-04-16 16:02 ` [RFC ethtool 1/6] update UAPI header copies Jakub Kicinski
@ 2021-04-16 16:02 ` Jakub Kicinski
  2021-04-16 16:02 ` [RFC ethtool 3/6] netlink: add FEC support Jakub Kicinski
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 11+ messages in thread
From: Jakub Kicinski @ 2021-04-16 16:02 UTC (permalink / raw)
  To: netdev, idosch; +Cc: mkubecek, Jakub Kicinski

In ethtool when we print an array we usually have a label (non-JSON)
and a key (JSON), because arrays are most often printed on a single
line (unlike iproute2 where the output has multiple params
on a line to cater to multi-interface dumps well).

Build this into the json array API.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 json_print.c | 20 ++++++++++----------
 json_print.h |  4 ++--
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/json_print.c b/json_print.c
index 56d5b4337e49..4f62767bdbc9 100644
--- a/json_print.c
+++ b/json_print.c
@@ -73,15 +73,15 @@ void close_json_object(void)
 /*
  * Start json array or string array using
  * the provided string as json key (if not null)
- * or as array delimiter in non-json context.
+ * or array delimiter in non-json context.
  */
-void open_json_array(enum output_type type, const char *str)
+void open_json_array(const char *key, const char *str)
 {
-	if (_IS_JSON_CONTEXT(type)) {
-		if (str)
-			jsonw_name(_jw, str);
+	if (is_json_context()) {
+		if (key)
+			jsonw_name(_jw, key);
 		jsonw_start_array(_jw);
-	} else if (_IS_FP_CONTEXT(type)) {
+	} else {
 		printf("%s", str);
 	}
 }
@@ -89,12 +89,12 @@ void open_json_array(enum output_type type, const char *str)
 /*
  * End json array or string array
  */
-void close_json_array(enum output_type type, const char *str)
+void close_json_array(const char *delim)
 {
-	if (_IS_JSON_CONTEXT(type))
+	if (is_json_context())
 		jsonw_end_array(_jw);
-	else if (_IS_FP_CONTEXT(type))
-		printf("%s", str);
+	else
+		printf("%s", delim);
 }
 
 /*
diff --git a/json_print.h b/json_print.h
index cc0c2ea19b59..df15314bafe2 100644
--- a/json_print.h
+++ b/json_print.h
@@ -37,8 +37,8 @@ void fflush_fp(void);
 
 void open_json_object(const char *str);
 void close_json_object(void);
-void open_json_array(enum output_type type, const char *delim);
-void close_json_array(enum output_type type, const char *delim);
+void open_json_array(const char *key, const char *str);
+void close_json_array(const char *delim);
 
 void print_nl(void);
 
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [RFC ethtool 3/6] netlink: add FEC support
  2021-04-16 16:02 [RFC ethtool 0/6] ethtool: add FEC & std stats support Jakub Kicinski
  2021-04-16 16:02 ` [RFC ethtool 1/6] update UAPI header copies Jakub Kicinski
  2021-04-16 16:02 ` [RFC ethtool 2/6] json: simplify array print API Jakub Kicinski
@ 2021-04-16 16:02 ` Jakub Kicinski
  2021-04-16 16:02 ` [RFC ethtool 4/6] netlink: fec: support displaying statistics Jakub Kicinski
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 11+ messages in thread
From: Jakub Kicinski @ 2021-04-16 16:02 UTC (permalink / raw)
  To: netdev, idosch; +Cc: mkubecek, Jakub Kicinski

Add support for FEC get/set over netlink.

Output should be identical but for ordering of RS vs BaseR.
Since the new output is based on link modes RS comes before
BaseR in "Configured FEC encodings". Seems pretty unlikely
someone depends on the ordering there.

Since old API was case insensitive we need to carefully
manage the bit names. Luckily for now most of the modes
are all uppercase.

Beyond that we need to cover up the discrepancy between
the meanings of None and Off. Kernel API uses the link
mode definition, but ethtool needs to maintain the old
interpretation of None meaning "not supported".

Because of those deviations existing link mode parsers
can't be directly reused.

Allocate error code 83 for FEC errors.

JSON support included.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 Makefile.am            |   2 +-
 ethtool.c              |   2 +
 netlink/desc-ethtool.c |  12 ++
 netlink/extapi.h       |   4 +
 netlink/fec.c          | 273 +++++++++++++++++++++++++++++++++++++++++
 netlink/monitor.c      |   4 +
 netlink/netlink.h      |   1 +
 7 files changed, 297 insertions(+), 1 deletion(-)
 create mode 100644 netlink/fec.c

diff --git a/Makefile.am b/Makefile.am
index e3e311d4b274..f643a24af97a 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -35,7 +35,7 @@ ethtool_SOURCES += \
 		  netlink/permaddr.c netlink/prettymsg.c netlink/prettymsg.h \
 		  netlink/features.c netlink/privflags.c netlink/rings.c \
 		  netlink/channels.c netlink/coalesce.c netlink/pause.c \
-		  netlink/eee.c netlink/tsinfo.c \
+		  netlink/eee.c netlink/tsinfo.c netlink/fec.c \
 		  netlink/desc-ethtool.c netlink/desc-genlctrl.c \
 		  netlink/desc-rtnl.c netlink/cable_test.c netlink/tunnels.c \
 		  uapi/linux/ethtool_netlink.h \
diff --git a/ethtool.c b/ethtool.c
index fb90e9e456b9..0933bc02ce5e 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -5963,11 +5963,13 @@ static const struct option args[] = {
 	{
 		.opts	= "--show-fec",
 		.func	= do_gfec,
+		.nlfunc	= nl_gfec,
 		.help	= "Show FEC settings",
 	},
 	{
 		.opts	= "--set-fec",
 		.func	= do_sfec,
+		.nlfunc	= nl_sfec,
 		.help	= "Set FEC settings",
 		.xhelp	= "		[ encoding auto|off|rs|baser|llrs [...]]\n"
 	},
diff --git a/netlink/desc-ethtool.c b/netlink/desc-ethtool.c
index 96291b9bdd3b..9da052a5b8aa 100644
--- a/netlink/desc-ethtool.c
+++ b/netlink/desc-ethtool.c
@@ -317,6 +317,14 @@ const struct pretty_nla_desc __tunnel_info_desc[] = {
 	NLATTR_DESC_NESTED(ETHTOOL_A_TUNNEL_INFO_UDP_PORTS, tunnel_udp),
 };
 
+static const struct pretty_nla_desc __fec_desc[] = {
+	NLATTR_DESC_INVALID(ETHTOOL_A_FEC_UNSPEC),
+	NLATTR_DESC_NESTED(ETHTOOL_A_FEC_HEADER, header),
+	NLATTR_DESC_NESTED(ETHTOOL_A_FEC_MODES, bitset),
+	NLATTR_DESC_BOOL(ETHTOOL_A_FEC_AUTO),
+	NLATTR_DESC_U32(ETHTOOL_A_FEC_ACTIVE),
+};
+
 const struct pretty_nlmsg_desc ethnl_umsg_desc[] = {
 	NLMSG_DESC_INVALID(ETHTOOL_MSG_USER_NONE),
 	NLMSG_DESC(ETHTOOL_MSG_STRSET_GET, strset),
@@ -347,6 +355,8 @@ const struct pretty_nlmsg_desc ethnl_umsg_desc[] = {
 	NLMSG_DESC(ETHTOOL_MSG_CABLE_TEST_ACT, cable_test),
 	NLMSG_DESC(ETHTOOL_MSG_CABLE_TEST_TDR_ACT, cable_test_tdr),
 	NLMSG_DESC(ETHTOOL_MSG_TUNNEL_INFO_GET, tunnel_info),
+	NLMSG_DESC(ETHTOOL_MSG_FEC_GET, fec),
+	NLMSG_DESC(ETHTOOL_MSG_FEC_SET, fec),
 };
 
 const unsigned int ethnl_umsg_n_desc = ARRAY_SIZE(ethnl_umsg_desc);
@@ -382,6 +392,8 @@ const struct pretty_nlmsg_desc ethnl_kmsg_desc[] = {
 	NLMSG_DESC(ETHTOOL_MSG_CABLE_TEST_NTF, cable_test_ntf),
 	NLMSG_DESC(ETHTOOL_MSG_CABLE_TEST_TDR_NTF, cable_test_tdr_ntf),
 	NLMSG_DESC(ETHTOOL_MSG_TUNNEL_INFO_GET_REPLY, tunnel_info),
+	NLMSG_DESC(ETHTOOL_MSG_FEC_GET_REPLY, fec),
+	NLMSG_DESC(ETHTOOL_MSG_FEC_NTF, fec),
 };
 
 const unsigned int ethnl_kmsg_n_desc = ARRAY_SIZE(ethnl_kmsg_desc);
diff --git a/netlink/extapi.h b/netlink/extapi.h
index 761cafb97855..5cadacce08e8 100644
--- a/netlink/extapi.h
+++ b/netlink/extapi.h
@@ -38,6 +38,8 @@ int nl_tsinfo(struct cmd_context *ctx);
 int nl_cable_test(struct cmd_context *ctx);
 int nl_cable_test_tdr(struct cmd_context *ctx);
 int nl_gtunnels(struct cmd_context *ctx);
+int nl_gfec(struct cmd_context *ctx);
+int nl_sfec(struct cmd_context *ctx);
 int nl_monitor(struct cmd_context *ctx);
 
 void nl_monitor_usage(void);
@@ -87,6 +89,8 @@ static inline void nl_monitor_usage(void)
 #define nl_cable_test		NULL
 #define nl_cable_test_tdr	NULL
 #define nl_gtunnels		NULL
+#define nl_gfec			NULL
+#define nl_sfec			NULL
 
 #endif /* ETHTOOL_ENABLE_NETLINK */
 
diff --git a/netlink/fec.c b/netlink/fec.c
new file mode 100644
index 000000000000..9d15832db98a
--- /dev/null
+++ b/netlink/fec.c
@@ -0,0 +1,273 @@
+/*
+ * fec.c - netlink implementation of FEC commands
+ *
+ * Implementation of "ethtool --show-fec <dev>" and
+ * "ethtool --set-fec <dev> ..."
+ */
+
+#include <errno.h>
+#include <ctype.h>
+#include <string.h>
+#include <stdio.h>
+
+#include "../internal.h"
+#include "../common.h"
+#include "netlink.h"
+#include "bitset.h"
+#include "parser.h"
+
+/* FEC_GET */
+
+static void
+fec_mode_walk(unsigned int idx, const char *name, bool val, void *data)
+{
+	bool *empty = data;
+
+	if (!val)
+		return;
+	if (empty)
+		*empty = false;
+
+	/* Rename None to Off - in legacy ioctl None means "not supported"
+	 * rather than supported but disabled.
+	 */
+	if (idx == ETHTOOL_LINK_MODE_FEC_NONE_BIT)
+		name = "Off";
+	/* Rename to match the ioctl letter case */
+	else if (idx == ETHTOOL_LINK_MODE_FEC_BASER_BIT)
+		name = "BaseR";
+
+	print_string(PRINT_ANY, NULL, " %s", name);
+}
+
+int fec_reply_cb(const struct nlmsghdr *nlhdr, void *data)
+{
+	const struct nlattr *tb[ETHTOOL_A_FEC_MAX + 1] = {};
+	DECLARE_ATTR_TB_INFO(tb);
+	struct nl_context *nlctx = data;
+	const struct stringset *lm_strings;
+	const char *name;
+	bool fa, empty;
+	bool silent;
+	int err_ret;
+	u32 active;
+	int ret;
+
+	silent = nlctx->is_dump || nlctx->is_monitor;
+	err_ret = silent ? MNL_CB_OK : MNL_CB_ERROR;
+	ret = mnl_attr_parse(nlhdr, GENL_HDRLEN, attr_cb, &tb_info);
+	if (ret < 0)
+		return err_ret;
+	nlctx->devname = get_dev_name(tb[ETHTOOL_A_FEC_HEADER]);
+	if (!dev_ok(nlctx))
+		return err_ret;
+
+	ret = netlink_init_ethnl2_socket(nlctx);
+	if (ret < 0)
+		return err_ret;
+	lm_strings = global_stringset(ETH_SS_LINK_MODES, nlctx->ethnl2_socket);
+
+	active = 0;
+	if (tb[ETHTOOL_A_FEC_ACTIVE])
+		active = mnl_attr_get_u32(tb[ETHTOOL_A_FEC_ACTIVE]);
+
+	if (silent)
+		print_nl();
+
+	open_json_object(NULL);
+
+	print_string(PRINT_ANY, "ifname", "FEC parameters for %s:\n",
+		     nlctx->devname);
+
+	open_json_array("config", "Configured FEC encodings:");
+	fa = tb[ETHTOOL_A_FEC_AUTO] && mnl_attr_get_u8(tb[ETHTOOL_A_FEC_AUTO]);
+	if (fa)
+		print_string(PRINT_ANY, NULL, " %s", "Auto");
+	empty = !fa;
+
+	ret = walk_bitset(tb[ETHTOOL_A_FEC_MODES], lm_strings, fec_mode_walk,
+			  &empty);
+	if (ret < 0)
+		goto err_close_dev;
+	if (empty)
+		print_string(PRINT_ANY, NULL, " %s", "None");
+	close_json_array("\n");
+
+	open_json_array("active", "Active FEC encoding:");
+	if (active) {
+		name = get_string(lm_strings, active);
+		if (name)
+			/* Take care of renames */
+			fec_mode_walk(active, name, true, NULL);
+		else
+			print_uint(PRINT_ANY, NULL, " BIT%u", active);
+	} else {
+		print_string(PRINT_ANY, NULL, " %s", "None");
+	}
+	close_json_array("\n");
+
+	close_json_object();
+
+	return MNL_CB_OK;
+
+err_close_dev:
+	close_json_object();
+	return err_ret;
+}
+
+int nl_gfec(struct cmd_context *ctx)
+{
+	struct nl_context *nlctx = ctx->nlctx;
+	struct nl_socket *nlsk = nlctx->ethnl_socket;
+	int ret;
+
+	if (netlink_cmd_check(ctx, ETHTOOL_MSG_FEC_GET, true))
+		return -EOPNOTSUPP;
+	if (ctx->argc > 0) {
+		fprintf(stderr, "ethtool: unexpected parameter '%s'\n",
+			*ctx->argp);
+		return 1;
+	}
+
+	ret = nlsock_prep_get_request(nlsk, ETHTOOL_MSG_FEC_GET,
+				      ETHTOOL_A_FEC_HEADER, 0);
+	if (ret < 0)
+		return ret;
+
+	new_json_obj(ctx->json);
+	ret = nlsock_send_get_request(nlsk, fec_reply_cb);
+	delete_json_obj();
+	return ret;
+}
+
+/* FEC_SET */
+
+static void strupc(char *dst, const char *src)
+{
+	while (*src)
+		*dst++ = toupper(*src++);
+	*dst = '\0';
+}
+
+static int fec_parse_bitset(struct nl_context *nlctx, uint16_t type,
+			    const void *data __maybe_unused,
+			    struct nl_msg_buff *msgbuff, void *dest)
+{
+	struct nlattr *bitset_attr;
+	struct nlattr *bits_attr;
+	struct nlattr *bit_attr;
+	char upper[ETH_GSTRING_LEN];
+	bool fec_auto = false;
+	int ret;
+
+	if (!type || dest) {
+		fprintf(stderr, "ethtool (%s): internal error parsing '%s'\n",
+			nlctx->cmd, nlctx->param);
+		return -EFAULT;
+	}
+
+	bitset_attr = ethnla_nest_start(msgbuff, type);
+	if (!bitset_attr)
+		return -EMSGSIZE;
+	ret = -EMSGSIZE;
+	if (ethnla_put_flag(msgbuff, ETHTOOL_A_BITSET_NOMASK, true))
+		goto err;
+	bits_attr = ethnla_nest_start(msgbuff, ETHTOOL_A_BITSET_BITS);
+	if (!bits_attr)
+		goto err;
+
+	while (nlctx->argc > 0) {
+		const char *name = *nlctx->argp;
+
+		if (!strcmp(name, "--"))
+			goto next;
+
+		if (!strcasecmp(name, "auto")) {
+			fec_auto = true;
+			goto next;
+		}
+		if (!strcasecmp(name, "off")) {
+			name = "None";
+		} else {
+			strupc(upper, name);
+			name = upper;
+		}
+
+		ret = -EMSGSIZE;
+		bit_attr = ethnla_nest_start(msgbuff,
+					     ETHTOOL_A_BITSET_BITS_BIT);
+		if (!bit_attr)
+			goto err;
+		if (ethnla_put_strz(msgbuff, ETHTOOL_A_BITSET_BIT_NAME, name))
+			goto err;
+		ethnla_nest_end(msgbuff, bit_attr);
+
+next:
+		nlctx->argp++;
+		nlctx->argc--;
+	}
+
+	ethnla_nest_end(msgbuff, bits_attr);
+	ethnla_nest_end(msgbuff, bitset_attr);
+
+	if (ethnla_put_u8(msgbuff, ETHTOOL_A_FEC_AUTO, fec_auto))
+		goto err;
+
+	return 0;
+err:
+	ethnla_nest_cancel(msgbuff, bitset_attr);
+	return ret;
+}
+
+static const struct param_parser sfec_params[] = {
+	{
+		.arg		= "encoding",
+		.type		= ETHTOOL_A_FEC_MODES,
+		.handler	= fec_parse_bitset,
+		.min_argc	= 1,
+	},
+	{}
+};
+
+int nl_sfec(struct cmd_context *ctx)
+{
+	struct nl_context *nlctx = ctx->nlctx;
+	struct nl_msg_buff *msgbuff;
+	struct nl_socket *nlsk;
+	int ret;
+
+	if (netlink_cmd_check(ctx, ETHTOOL_MSG_FEC_SET, false))
+		return -EOPNOTSUPP;
+	if (!ctx->argc) {
+		fprintf(stderr, "ethtool (--set-fec): parameters missing\n");
+		return 1;
+	}
+
+	nlctx->cmd = "--set-fec";
+	nlctx->argp = ctx->argp;
+	nlctx->argc = ctx->argc;
+	nlctx->devname = ctx->devname;
+	nlsk = nlctx->ethnl_socket;
+	msgbuff = &nlsk->msgbuff;
+
+	ret = msg_init(nlctx, msgbuff, ETHTOOL_MSG_FEC_SET,
+		       NLM_F_REQUEST | NLM_F_ACK);
+	if (ret < 0)
+		return 2;
+	if (ethnla_fill_header(msgbuff, ETHTOOL_A_FEC_HEADER,
+			       ctx->devname, 0))
+		return -EMSGSIZE;
+
+	ret = nl_parser(nlctx, sfec_params, NULL, PARSER_GROUP_NONE, NULL);
+	if (ret < 0)
+		return 1;
+
+	ret = nlsock_sendmsg(nlsk, NULL);
+	if (ret < 0)
+		return 83;
+	ret = nlsock_process_reply(nlsk, nomsg_reply_cb, nlctx);
+	if (ret == 0)
+		return 0;
+	else
+		return nlctx->exit_code ?: 83;
+}
diff --git a/netlink/monitor.c b/netlink/monitor.c
index 19f991fce3e4..0c4df9e78ee3 100644
--- a/netlink/monitor.c
+++ b/netlink/monitor.c
@@ -67,6 +67,10 @@ static struct {
 		.cmd	= ETHTOOL_MSG_CABLE_TEST_TDR_NTF,
 		.cb	= cable_test_tdr_ntf_cb,
 	},
+	{
+		.cmd	= ETHTOOL_MSG_FEC_NTF,
+		.cb	= fec_reply_cb,
+	},
 };
 
 static void clear_filter(struct nl_context *nlctx)
diff --git a/netlink/netlink.h b/netlink/netlink.h
index c02558540218..70fa666b20e5 100644
--- a/netlink/netlink.h
+++ b/netlink/netlink.h
@@ -90,6 +90,7 @@ int cable_test_reply_cb(const struct nlmsghdr *nlhdr, void *data);
 int cable_test_ntf_cb(const struct nlmsghdr *nlhdr, void *data);
 int cable_test_tdr_reply_cb(const struct nlmsghdr *nlhdr, void *data);
 int cable_test_tdr_ntf_cb(const struct nlmsghdr *nlhdr, void *data);
+int fec_reply_cb(const struct nlmsghdr *nlhdr, void *data);
 
 /* dump helpers */
 
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [RFC ethtool 4/6] netlink: fec: support displaying statistics
  2021-04-16 16:02 [RFC ethtool 0/6] ethtool: add FEC & std stats support Jakub Kicinski
                   ` (2 preceding siblings ...)
  2021-04-16 16:02 ` [RFC ethtool 3/6] netlink: add FEC support Jakub Kicinski
@ 2021-04-16 16:02 ` Jakub Kicinski
  2021-04-16 16:02 ` [RFC ethtool 5/6] ethtool: add nlchk for redirecting to netlink Jakub Kicinski
  2021-04-16 16:02 ` [RFC ethtool 6/6] netlink: add support for standard stats Jakub Kicinski
  5 siblings, 0 replies; 11+ messages in thread
From: Jakub Kicinski @ 2021-04-16 16:02 UTC (permalink / raw)
  To: netdev, idosch; +Cc: mkubecek, Jakub Kicinski

 # ethtool  -I --show-fec eth0
FEC parameters for eth0:
Configured FEC encodings: None
Active FEC encoding: None
Statistics:
  corrected_blocks: 256
    Lane 0: 255
    Lane 1: 1
  uncorrectable_blocks: 145
    Lane 0: 128
    Lane 1: 17
 # ethtool --json -I --show-fec eth0
[ {
        "ifname": "eth0",
        "config": [ "None" ],
        "active": [ "None" ],
        "statistics": {
            "corrected_blocks": {
                "total": 256,
                "lanes": [ 255,1 ]
            },
            "uncorrectable_blocks": {
                "total": 145,
                "lanes": [ 128,17 ]
            }
        }
    } ]

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 netlink/fec.c | 85 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 84 insertions(+), 1 deletion(-)

diff --git a/netlink/fec.c b/netlink/fec.c
index 9d15832db98a..1efd18d38142 100644
--- a/netlink/fec.c
+++ b/netlink/fec.c
@@ -7,6 +7,7 @@
 
 #include <errno.h>
 #include <ctype.h>
+#include <inttypes.h>
 #include <string.h>
 #include <stdio.h>
 
@@ -40,6 +41,79 @@ fec_mode_walk(unsigned int idx, const char *name, bool val, void *data)
 	print_string(PRINT_ANY, NULL, " %s", name);
 }
 
+static int fec_show_stats(const struct nlattr *nest)
+{
+	const struct nlattr *tb[ETHTOOL_A_FEC_STAT_MAX + 1] = {};
+	DECLARE_ATTR_TB_INFO(tb);
+	static const struct {
+		unsigned int attr;
+		char *name;
+	} stats[] = {
+		{ ETHTOOL_A_FEC_STAT_CORRECTED, "corrected_blocks" },
+		{ ETHTOOL_A_FEC_STAT_UNCORR, "uncorrectable_blocks" },
+		{ ETHTOOL_A_FEC_STAT_CORR_BITS, "corrected_bits" },
+	};
+	bool header = false;
+	unsigned int i;
+	int ret;
+
+	ret = mnl_attr_parse_nested(nest, attr_cb, &tb_info);
+	if (ret < 0)
+		return ret;
+
+	open_json_object("statistics");
+	for (i = 0; i < ARRAY_SIZE(stats); i++) {
+		uint64_t *vals;
+		int lanes, l;
+
+		if (!tb[stats[i].attr] ||
+		    !mnl_attr_get_payload_len(tb[stats[i].attr]))
+			continue;
+
+		if (!header && !is_json_context()) {
+			printf("Statistics:\n");
+			header = true;
+		}
+
+		if (mnl_attr_get_payload_len(tb[stats[i].attr]) % 8) {
+			fprintf(stderr, "malformed netlink message (statistic)\n");
+			goto err_close_stats;
+		}
+
+		vals = mnl_attr_get_payload(tb[stats[i].attr]);
+		lanes = mnl_attr_get_payload_len(tb[stats[i].attr]) / 8 - 1;
+
+		if (!is_json_context()) {
+			fprintf(stdout, "  %s: %" PRIu64 "\n",
+				stats[i].name, *vals++);
+		} else {
+			open_json_object(stats[i].name);
+			print_u64(PRINT_JSON, "total", NULL, *vals++);
+		}
+
+		if (lanes)
+			open_json_array("lanes", "");
+		for (l = 0; l < lanes; l++) {
+			if (!is_json_context())
+				fprintf(stdout, "    Lane %d: %" PRIu64 "\n",
+					l, *vals++);
+			else
+				print_u64(PRINT_JSON, NULL, NULL, *vals++);
+		}
+		if (lanes)
+			close_json_array("");
+
+		close_json_object();
+	}
+	close_json_object();
+
+	return 0;
+
+err_close_stats:
+	close_json_object();
+	return -1;
+}
+
 int fec_reply_cb(const struct nlmsghdr *nlhdr, void *data)
 {
 	const struct nlattr *tb[ETHTOOL_A_FEC_MAX + 1] = {};
@@ -106,6 +180,12 @@ int fec_reply_cb(const struct nlmsghdr *nlhdr, void *data)
 	}
 	close_json_array("\n");
 
+	if (tb[ETHTOOL_A_FEC_STATS]) {
+		ret = fec_show_stats(tb[ETHTOOL_A_FEC_STATS]);
+		if (ret < 0)
+			goto err_close_dev;
+	}
+
 	close_json_object();
 
 	return MNL_CB_OK;
@@ -119,6 +199,7 @@ int nl_gfec(struct cmd_context *ctx)
 {
 	struct nl_context *nlctx = ctx->nlctx;
 	struct nl_socket *nlsk = nlctx->ethnl_socket;
+	u32 flags;
 	int ret;
 
 	if (netlink_cmd_check(ctx, ETHTOOL_MSG_FEC_GET, true))
@@ -129,8 +210,10 @@ int nl_gfec(struct cmd_context *ctx)
 		return 1;
 	}
 
+	flags = get_stats_flag(nlctx, ETHTOOL_MSG_FEC_GET,
+			       ETHTOOL_A_FEC_HEADER);
 	ret = nlsock_prep_get_request(nlsk, ETHTOOL_MSG_FEC_GET,
-				      ETHTOOL_A_FEC_HEADER, 0);
+				      ETHTOOL_A_FEC_HEADER, flags);
 	if (ret < 0)
 		return ret;
 
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [RFC ethtool 5/6] ethtool: add nlchk for redirecting to netlink
  2021-04-16 16:02 [RFC ethtool 0/6] ethtool: add FEC & std stats support Jakub Kicinski
                   ` (3 preceding siblings ...)
  2021-04-16 16:02 ` [RFC ethtool 4/6] netlink: fec: support displaying statistics Jakub Kicinski
@ 2021-04-16 16:02 ` Jakub Kicinski
  2021-04-16 16:02 ` [RFC ethtool 6/6] netlink: add support for standard stats Jakub Kicinski
  5 siblings, 0 replies; 11+ messages in thread
From: Jakub Kicinski @ 2021-04-16 16:02 UTC (permalink / raw)
  To: netdev, idosch; +Cc: mkubecek, Jakub Kicinski

To support commands which differ from the ioctl implementation
add a new callback which can check if the arguments on the command
line indicate that the request should be sent over netlink.
The decision should be inferred from the arguments, rather
than an explicit --netlink argument.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 ethtool.c         | 5 ++++-
 netlink/extapi.h  | 5 +++--
 netlink/netlink.c | 9 +++++++--
 3 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/ethtool.c b/ethtool.c
index 0933bc02ce5e..b07fd9292d77 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -5608,6 +5608,7 @@ struct option {
 	const char	*opts;
 	bool		no_dev;
 	int		(*func)(struct cmd_context *);
+	nl_chk_t	nlchk;
 	nl_func_t	nlfunc;
 	const char	*help;
 	const char	*xhelp;
@@ -6269,6 +6270,7 @@ int main(int argc, char **argp)
 	int (*func)(struct cmd_context *);
 	struct cmd_context ctx = {};
 	nl_func_t nlfunc = NULL;
+	nl_chk_t nlchk = NULL;
 	bool no_dev;
 	int ret;
 	int k;
@@ -6328,6 +6330,7 @@ int main(int argc, char **argp)
 		argc--;
 		func = args[k].func;
 		nlfunc = args[k].nlfunc;
+		nlchk = args[k].nlchk;
 		no_dev = args[k].no_dev;
 		goto opt_found;
 	}
@@ -6347,7 +6350,7 @@ int main(int argc, char **argp)
 	}
 	ctx.argc = argc;
 	ctx.argp = argp;
-	netlink_run_handler(&ctx, nlfunc, !func);
+	netlink_run_handler(&ctx, nlchk, nlfunc, !func);
 
 	ret = ioctl_init(&ctx, no_dev);
 	if (ret)
diff --git a/netlink/extapi.h b/netlink/extapi.h
index 5cadacce08e8..d6036a39e920 100644
--- a/netlink/extapi.h
+++ b/netlink/extapi.h
@@ -11,11 +11,12 @@ struct cmd_context;
 struct nl_context;
 
 typedef int (*nl_func_t)(struct cmd_context *);
+typedef bool (*nl_chk_t)(struct cmd_context *);
 
 #ifdef ETHTOOL_ENABLE_NETLINK
 
-void netlink_run_handler(struct cmd_context *ctx, nl_func_t nlfunc,
-			 bool no_fallback);
+void netlink_run_handler(struct cmd_context *ctx, nl_chk_t nlchk,
+			 nl_func_t nlfunc, bool no_fallback);
 
 int nl_gset(struct cmd_context *ctx);
 int nl_sset(struct cmd_context *ctx);
diff --git a/netlink/netlink.c b/netlink/netlink.c
index ffe06339f099..4cee9b23b28f 100644
--- a/netlink/netlink.c
+++ b/netlink/netlink.c
@@ -452,14 +452,15 @@ static void netlink_done(struct cmd_context *ctx)
 /**
  * netlink_run_handler() - run netlink handler for subcommand
  * @ctx:         command context
+ * @nlchk:       netlink capability check
  * @nlfunc:      subcommand netlink handler to call
  * @no_fallback: there is no ioctl fallback handler
  *
  * This function returns only if ioctl() handler should be run as fallback.
  * Otherwise it exits with appropriate return code.
  */
-void netlink_run_handler(struct cmd_context *ctx, nl_func_t nlfunc,
-			 bool no_fallback)
+void netlink_run_handler(struct cmd_context *ctx, nl_chk_t nlchk,
+			 nl_func_t nlfunc, bool no_fallback)
 {
 	bool wildcard = ctx->devname && !strcmp(ctx->devname, WILDCARD_DEVNAME);
 	bool wildcard_unsupported, ioctl_fallback;
@@ -467,6 +468,10 @@ void netlink_run_handler(struct cmd_context *ctx, nl_func_t nlfunc,
 	const char *reason;
 	int ret;
 
+	if (nlchk && !nlchk(ctx)) {
+		reason = "ioctl-only request";
+		goto no_support;
+	}
 	if (ctx->devname && strlen(ctx->devname) >= ALTIFNAMSIZ) {
 		fprintf(stderr, "device name '%s' longer than %u characters\n",
 			ctx->devname, ALTIFNAMSIZ - 1);
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [RFC ethtool 6/6] netlink: add support for standard stats
  2021-04-16 16:02 [RFC ethtool 0/6] ethtool: add FEC & std stats support Jakub Kicinski
                   ` (4 preceding siblings ...)
  2021-04-16 16:02 ` [RFC ethtool 5/6] ethtool: add nlchk for redirecting to netlink Jakub Kicinski
@ 2021-04-16 16:02 ` Jakub Kicinski
  2021-04-17 18:14   ` Ido Schimmel
  5 siblings, 1 reply; 11+ messages in thread
From: Jakub Kicinski @ 2021-04-16 16:02 UTC (permalink / raw)
  To: netdev, idosch; +Cc: mkubecek, Jakub Kicinski

 # ethtool -S eth0 --groups eth-phy eth-mac rmon
Stats for eth0:
eth-phy-SymbolErrorDuringCarrier: 1
eth-mac-FramesTransmittedOK: 1
eth-mac-FrameTooLongErrors: 1
rmon-etherStatsUndersizePkts: 1
rmon-etherStatsJabbers: 1
rmon-etherStatsPkts64Octets: 1
rmon-etherStatsPkts128to255Octets: 1
rmon-etherStatsPkts1024toMaxOctets: 0

 # ethtool --json -S eth0 --groups eth-phy eth-mac rmon | jq
 [
  {
    "ifname": "eth0",
    "eth-phy": {
      "SymbolErrorDuringCarrier": 1
    },
    "eth-mac": {
      "FramesTransmittedOK": 1,
      "FrameTooLongErrors": 0
    },
    "rmon": {
      "etherStatsUndersizePkts": 1,
      "etherStatsJabbers": 0,
      "pktsNtoM": [
        {
          "low": 0,
          "high": 64,
          "val": 1
        },
        {
          "low": 128,
          "high": 255,
          "val": 1
        },
        {
          "low": 1024,
          "high": 0,
          "val": 0
        }
      ]
    }
  }
 ]

 # ethtool --json -S eth0 --groups eth-phy eth-mac rmon | \
     jq '.[].rmon.pktsNtoM | map(select(.low >= 128)) | map(.val) | add'
 1

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 Makefile.am            |   1 +
 ethtool.8.in           |  11 ++
 ethtool.c              |   5 +-
 netlink/desc-ethtool.c |  39 +++++++
 netlink/extapi.h       |   4 +
 netlink/stats.c        | 242 +++++++++++++++++++++++++++++++++++++++++
 6 files changed, 301 insertions(+), 1 deletion(-)
 create mode 100644 netlink/stats.c

diff --git a/Makefile.am b/Makefile.am
index f643a24af97a..75c245653cda 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -36,6 +36,7 @@ ethtool_SOURCES += \
 		  netlink/features.c netlink/privflags.c netlink/rings.c \
 		  netlink/channels.c netlink/coalesce.c netlink/pause.c \
 		  netlink/eee.c netlink/tsinfo.c netlink/fec.c \
+		  netlink/stats.c \
 		  netlink/desc-ethtool.c netlink/desc-genlctrl.c \
 		  netlink/desc-rtnl.c netlink/cable_test.c netlink/tunnels.c \
 		  uapi/linux/ethtool_netlink.h \
diff --git a/ethtool.8.in b/ethtool.8.in
index ba4e245cdc61..0ca11595158a 100644
--- a/ethtool.8.in
+++ b/ethtool.8.in
@@ -240,6 +240,12 @@ ethtool \- query or control network driver and hardware settings
 .HP
 .B ethtool \-S|\-\-statistics
 .I devname
+.RB [\fB--groups
+.RB [\fBeth\-phy\fP]
+.RB [\fBeth\-mac\fP]
+.RB [\fBeth\-ctrl\fP]
+.RN [\fBrmon\fP]
+.RB ]
 .HP
 .B ethtool \-\-phy\-statistics
 .I devname
@@ -653,6 +659,11 @@ auto-negotiation is enabled.
 .B \-S \-\-statistics
 Queries the specified network device for NIC- and driver-specific
 statistics.
+.RS 4
+.TP
+.B \fB--groups [\fBeth\-phy\fP] [\fBeth\-mac\fP] [\fBeth\-ctrl\fP] [\fBrmon\fP]
+Request standard device statistics of a specific group.
+.RE
 .TP
 .B \-\-phy\-statistics
 Queries the specified network device for PHY specific statistics.
diff --git a/ethtool.c b/ethtool.c
index b07fd9292d77..1b5690f424c8 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -5760,7 +5760,10 @@ static const struct option args[] = {
 	{
 		.opts	= "-S|--statistics",
 		.func	= do_gnicstats,
-		.help	= "Show adapter statistics"
+		.nlchk	= nl_gstats_chk,
+		.nlfunc	= nl_gstats,
+		.help	= "Show adapter statistics",
+		.xhelp	= "               [ --groups [eth-phy] [eth-mac] [eth-ctrl] [rmon] ]\n"
 	},
 	{
 		.opts	= "--phy-statistics",
diff --git a/netlink/desc-ethtool.c b/netlink/desc-ethtool.c
index 9da052a5b8aa..63e91ee7ffcd 100644
--- a/netlink/desc-ethtool.c
+++ b/netlink/desc-ethtool.c
@@ -325,6 +325,43 @@ static const struct pretty_nla_desc __fec_desc[] = {
 	NLATTR_DESC_U32(ETHTOOL_A_FEC_ACTIVE),
 };
 
+static const struct pretty_nla_desc __stats_grp_stat_desc[] = {
+	NLATTR_DESC_U64(0),  NLATTR_DESC_U64(1),  NLATTR_DESC_U64(2),
+	NLATTR_DESC_U64(3),  NLATTR_DESC_U64(4),  NLATTR_DESC_U64(5),
+	NLATTR_DESC_U64(6),  NLATTR_DESC_U64(7),  NLATTR_DESC_U64(8),
+	NLATTR_DESC_U64(9),  NLATTR_DESC_U64(10), NLATTR_DESC_U64(11),
+	NLATTR_DESC_U64(12), NLATTR_DESC_U64(13), NLATTR_DESC_U64(14),
+	NLATTR_DESC_U64(15), NLATTR_DESC_U64(16), NLATTR_DESC_U64(17),
+	NLATTR_DESC_U64(18), NLATTR_DESC_U64(19), NLATTR_DESC_U64(20),
+	NLATTR_DESC_U64(21), NLATTR_DESC_U64(22), NLATTR_DESC_U64(23),
+	NLATTR_DESC_U64(24), NLATTR_DESC_U64(25), NLATTR_DESC_U64(26),
+	NLATTR_DESC_U64(27), NLATTR_DESC_U64(28), NLATTR_DESC_U64(29),
+};
+
+static const struct pretty_nla_desc __stats_grp_hist_desc[] = {
+	NLATTR_DESC_U32(ETHTOOL_A_STATS_GRP_HIST_BKT_LOW),
+	NLATTR_DESC_U32(ETHTOOL_A_STATS_GRP_HIST_BKT_HI),
+	NLATTR_DESC_U64(ETHTOOL_A_STATS_GRP_HIST_VAL),
+};
+
+static const struct pretty_nla_desc __stats_grp_desc[] = {
+	NLATTR_DESC_INVALID(ETHTOOL_A_STATS_GRP_UNSPEC),
+	NLATTR_DESC_INVALID(ETHTOOL_A_STATS_GRP_PAD),
+	NLATTR_DESC_U32(ETHTOOL_A_STATS_GRP_ID),
+	NLATTR_DESC_U32(ETHTOOL_A_STATS_GRP_SS_ID),
+	NLATTR_DESC_NESTED(ETHTOOL_A_STATS_GRP_STAT, stats_grp_stat),
+	NLATTR_DESC_NESTED(ETHTOOL_A_STATS_GRP_HIST_RX, stats_grp_hist),
+	NLATTR_DESC_NESTED(ETHTOOL_A_STATS_GRP_HIST_TX, stats_grp_hist),
+};
+
+static const struct pretty_nla_desc __stats_desc[] = {
+	NLATTR_DESC_INVALID(ETHTOOL_A_STATS_UNSPEC),
+	NLATTR_DESC_INVALID(ETHTOOL_A_STATS_PAD),
+	NLATTR_DESC_NESTED(ETHTOOL_A_STATS_HEADER, header),
+	NLATTR_DESC_NESTED(ETHTOOL_A_STATS_GROUPS, bitset),
+	NLATTR_DESC_NESTED(ETHTOOL_A_STATS_GRP, stats_grp),
+};
+
 const struct pretty_nlmsg_desc ethnl_umsg_desc[] = {
 	NLMSG_DESC_INVALID(ETHTOOL_MSG_USER_NONE),
 	NLMSG_DESC(ETHTOOL_MSG_STRSET_GET, strset),
@@ -357,6 +394,7 @@ const struct pretty_nlmsg_desc ethnl_umsg_desc[] = {
 	NLMSG_DESC(ETHTOOL_MSG_TUNNEL_INFO_GET, tunnel_info),
 	NLMSG_DESC(ETHTOOL_MSG_FEC_GET, fec),
 	NLMSG_DESC(ETHTOOL_MSG_FEC_SET, fec),
+	NLMSG_DESC(ETHTOOL_MSG_STATS_GET, stats),
 };
 
 const unsigned int ethnl_umsg_n_desc = ARRAY_SIZE(ethnl_umsg_desc);
@@ -394,6 +432,7 @@ const struct pretty_nlmsg_desc ethnl_kmsg_desc[] = {
 	NLMSG_DESC(ETHTOOL_MSG_TUNNEL_INFO_GET_REPLY, tunnel_info),
 	NLMSG_DESC(ETHTOOL_MSG_FEC_GET_REPLY, fec),
 	NLMSG_DESC(ETHTOOL_MSG_FEC_NTF, fec),
+	NLMSG_DESC(ETHTOOL_MSG_STATS_GET_REPLY, stats),
 };
 
 const unsigned int ethnl_kmsg_n_desc = ARRAY_SIZE(ethnl_kmsg_desc);
diff --git a/netlink/extapi.h b/netlink/extapi.h
index d6036a39e920..97113eb98ca0 100644
--- a/netlink/extapi.h
+++ b/netlink/extapi.h
@@ -41,6 +41,8 @@ int nl_cable_test_tdr(struct cmd_context *ctx);
 int nl_gtunnels(struct cmd_context *ctx);
 int nl_gfec(struct cmd_context *ctx);
 int nl_sfec(struct cmd_context *ctx);
+bool nl_gstats_chk(struct cmd_context *ctx);
+int nl_gstats(struct cmd_context *ctx);
 int nl_monitor(struct cmd_context *ctx);
 
 void nl_monitor_usage(void);
@@ -92,6 +94,8 @@ static inline void nl_monitor_usage(void)
 #define nl_gtunnels		NULL
 #define nl_gfec			NULL
 #define nl_sfec			NULL
+#define nl_gstats_chk		NULL
+#define nl_gstats		NULL
 
 #endif /* ETHTOOL_ENABLE_NETLINK */
 
diff --git a/netlink/stats.c b/netlink/stats.c
new file mode 100644
index 000000000000..6c0b37a9aa4a
--- /dev/null
+++ b/netlink/stats.c
@@ -0,0 +1,242 @@
+/*
+ * stats.c - netlink implementation of stats
+ *
+ * Implementation of "ethtool -S <dev> <types>" and
+ */
+
+#include <errno.h>
+#include <ctype.h>
+#include <inttypes.h>
+#include <string.h>
+#include <stdio.h>
+
+#include "../internal.h"
+#include "../common.h"
+#include "netlink.h"
+#include "parser.h"
+#include "strset.h"
+
+static int parse_rmon_hist(const struct nlattr *hist)
+{
+	const struct nlattr *tb[ETHTOOL_A_STATS_GRP_HIST_VAL + 1] = {};
+	DECLARE_ATTR_TB_INFO(tb);
+	unsigned long long val;
+	unsigned int low, hi;
+	int ret;
+
+	ret = mnl_attr_parse_nested(hist, attr_cb, &tb_info);
+	if (ret < 0) {
+		fprintf(stderr, "invalid kernel response - malformed histogram entry\n");
+		return 1;
+	}
+
+	if (!tb[ETHTOOL_A_STATS_GRP_HIST_BKT_LOW] ||
+	    !tb[ETHTOOL_A_STATS_GRP_HIST_BKT_HI] ||
+	    !tb[ETHTOOL_A_STATS_GRP_HIST_VAL]) {
+		fprintf(stderr, "invalid kernel response - histogram entry missing attributes\n");
+		return 1;
+	}
+
+	low = mnl_attr_get_u32(tb[ETHTOOL_A_STATS_GRP_HIST_BKT_LOW]);
+	hi = mnl_attr_get_u32(tb[ETHTOOL_A_STATS_GRP_HIST_BKT_HI]);
+	val = mnl_attr_get_u64(tb[ETHTOOL_A_STATS_GRP_HIST_VAL]);
+
+	if (!is_json_context()) {
+		fprintf(stdout, "rmon-%s-etherStatsPkts",
+			mnl_attr_get_type(hist) == ETHTOOL_A_STATS_GRP_HIST_RX ?
+			"rx" : "tx");
+
+		if (low && hi) {
+			fprintf(stdout, "%uto%uOctets: %llu\n", low, hi, val);
+		} else if (hi) {
+			fprintf(stdout, "%uOctets: %llu\n", hi, val);
+		} else if (low) {
+			fprintf(stdout, "%utoMaxOctets: %llu\n", low, val);
+		} else {
+			fprintf(stderr, "invalid kernel response - bad histogram entry bounds\n");
+			return 1;
+		}
+	} else {
+		open_json_object(NULL);
+		print_uint(PRINT_JSON, "low", NULL, low);
+		print_uint(PRINT_JSON, "high", NULL, hi);
+		print_u64(PRINT_JSON, "val", NULL, val);
+		close_json_object();
+	}
+
+	return 0;
+}
+
+static int parse_grp(struct nl_context *nlctx, const struct nlattr *grp,
+		     const struct stringset *std_str)
+{
+	const struct nlattr *tb[ETHTOOL_A_STATS_GRP_SS_ID + 1] = {};
+	DECLARE_ATTR_TB_INFO(tb);
+	const struct stringset *stat_str;
+	const struct nlattr *attr, *stat;
+	const char *std_name, *name;
+	unsigned int ss_id, id, s;
+	unsigned long long val;
+	int ret;
+
+	ret = mnl_attr_parse_nested(grp, attr_cb, &tb_info);
+	if (ret < 0)
+		return 1;
+
+	if (!tb[ETHTOOL_A_STATS_GRP_ID])
+		return 1;
+	if (!tb[ETHTOOL_A_STATS_GRP_SS_ID])
+		return 0;
+
+	id = mnl_attr_get_u32(tb[ETHTOOL_A_STATS_GRP_ID]);
+	ss_id = mnl_attr_get_u32(tb[ETHTOOL_A_STATS_GRP_SS_ID]);
+
+	stat_str = global_stringset(ss_id, nlctx->ethnl2_socket);
+
+	std_name = get_string(std_str, id);
+	open_json_object(std_name);
+
+	mnl_attr_for_each_nested(attr, grp) {
+		if (mnl_attr_get_type(attr) != ETHTOOL_A_STATS_GRP_STAT)
+			continue;
+		stat = mnl_attr_get_payload(attr);
+		ret = mnl_attr_validate(stat, MNL_TYPE_U64);
+		if (ret) {
+			fprintf(stderr, "invalid kernel response - bad statistic entry\n");
+			goto err_close_grp;
+		}
+		s = mnl_attr_get_type(stat);
+		name = get_string(stat_str, s);
+		if (!name || !name[0])
+			continue;
+
+		if (!is_json_context())
+			fprintf(stdout, "%s-%s: ", std_name, name);
+
+		val = mnl_attr_get_u64(stat);
+		print_u64(PRINT_ANY, name, "%llu\n", val);
+	}
+
+	if (id == ETHTOOL_STATS_RMON) {
+		open_json_array("pktsNtoM", "");
+
+		mnl_attr_for_each_nested(attr, grp) {
+			s = mnl_attr_get_type(attr);
+			if (s != ETHTOOL_A_STATS_GRP_HIST_RX &&
+			    s != ETHTOOL_A_STATS_GRP_HIST_TX)
+				continue;
+
+			if (parse_rmon_hist(attr))
+				goto err_close_rmon;
+		}
+		close_json_array("");
+	}
+
+	close_json_object();
+
+	return 0;
+
+err_close_rmon:
+	close_json_array("");
+err_close_grp:
+	close_json_object();
+	return 1;
+}
+
+static int stats_reply_cb(const struct nlmsghdr *nlhdr, void *data)
+{
+	const struct nlattr *tb[ETHTOOL_A_STATS_MAX + 1] = {};
+	DECLARE_ATTR_TB_INFO(tb);
+	struct nl_context *nlctx = data;
+	const struct stringset *std_str;
+	const struct nlattr *attr;
+	bool silent;
+	int err_ret;
+	int ret;
+
+	silent = nlctx->is_dump || nlctx->is_monitor;
+	err_ret = silent ? MNL_CB_OK : MNL_CB_ERROR;
+	ret = mnl_attr_parse(nlhdr, GENL_HDRLEN, attr_cb, &tb_info);
+	if (ret < 0)
+		return err_ret;
+	nlctx->devname = get_dev_name(tb[ETHTOOL_A_STATS_HEADER]);
+	if (!dev_ok(nlctx))
+		return err_ret;
+
+	ret = netlink_init_ethnl2_socket(nlctx);
+	if (ret < 0)
+		return err_ret;
+	std_str = global_stringset(ETH_SS_STATS_STD, nlctx->ethnl2_socket);
+
+	if (silent)
+		print_nl();
+
+	open_json_object(NULL);
+
+	print_string(PRINT_ANY, "ifname", "Standard stats for %s:\n",
+		     nlctx->devname);
+
+	mnl_attr_for_each(attr, nlhdr, GENL_HDRLEN) {
+		if (mnl_attr_get_type(attr) == ETHTOOL_A_STATS_GRP) {
+			ret = parse_grp(nlctx, attr, std_str);
+			if (ret)
+				goto err_close_dev;
+		}
+	}
+
+	close_json_object();
+
+	return MNL_CB_OK;
+
+err_close_dev:
+	close_json_object();
+	return err_ret;
+}
+
+static const struct bitset_parser_data stats_parser_data = {
+	.no_mask	= true,
+	.force_hex	= false,
+};
+
+static const struct param_parser stats_params[] = {
+	{
+		.arg		= "--groups",
+		.type		= ETHTOOL_A_STATS_GROUPS,
+		.handler	= nl_parse_bitset,
+		.handler_data	= &stats_parser_data,
+		.min_argc	= 1,
+	},
+	{}
+};
+
+int nl_gstats(struct cmd_context *ctx)
+{
+	struct nl_context *nlctx = ctx->nlctx;
+	struct nl_socket *nlsk = nlctx->ethnl_socket;
+	int ret;
+
+	ret = nlsock_prep_get_request(nlsk, ETHTOOL_MSG_STATS_GET,
+				      ETHTOOL_A_STATS_HEADER, 0);
+	if (ret < 0)
+		return ret;
+
+	nlctx->cmd = "-S";
+	nlctx->argp = ctx->argp;
+	nlctx->argc = ctx->argc;
+	nlctx->devname = ctx->devname;
+	nlsk = nlctx->ethnl_socket;
+
+	ret = nl_parser(nlctx, stats_params, NULL, PARSER_GROUP_NONE, NULL);
+	if (ret < 0)
+		return 1;
+
+	new_json_obj(ctx->json);
+	ret = nlsock_send_get_request(nlsk, stats_reply_cb);
+	delete_json_obj();
+	return ret;
+}
+
+bool nl_gstats_chk(struct cmd_context *ctx)
+{
+	return ctx->argc;
+}
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [RFC ethtool 6/6] netlink: add support for standard stats
  2021-04-16 16:02 ` [RFC ethtool 6/6] netlink: add support for standard stats Jakub Kicinski
@ 2021-04-17 18:14   ` Ido Schimmel
  2021-04-17 18:47     ` Jakub Kicinski
  0 siblings, 1 reply; 11+ messages in thread
From: Ido Schimmel @ 2021-04-17 18:14 UTC (permalink / raw)
  To: Jakub Kicinski; +Cc: netdev, idosch, mkubecek

On Fri, Apr 16, 2021 at 09:02:52AM -0700, Jakub Kicinski wrote:
>  # ethtool -S eth0 --groups eth-phy eth-mac rmon
> Stats for eth0:
> eth-phy-SymbolErrorDuringCarrier: 1
> eth-mac-FramesTransmittedOK: 1
> eth-mac-FrameTooLongErrors: 1
> rmon-etherStatsUndersizePkts: 1
> rmon-etherStatsJabbers: 1
> rmon-etherStatsPkts64Octets: 1
> rmon-etherStatsPkts128to255Octets: 1
> rmon-etherStatsPkts1024toMaxOctets: 0
> 
>  # ethtool --json -S eth0 --groups eth-phy eth-mac rmon | jq
>  [
>   {
>     "ifname": "eth0",
>     "eth-phy": {
>       "SymbolErrorDuringCarrier": 1
>     },
>     "eth-mac": {
>       "FramesTransmittedOK": 1,
>       "FrameTooLongErrors": 0
>     },
>     "rmon": {
>       "etherStatsUndersizePkts": 1,
>       "etherStatsJabbers": 0,
>       "pktsNtoM": [
>         {
>           "low": 0,
>           "high": 64,
>           "val": 1
>         },
>         {
>           "low": 128,
>           "high": 255,
>           "val": 1
>         },
>         {
>           "low": 1024,
>           "high": 0,
>           "val": 0
>         }
>       ]
>     }
>   }
>  ]
> 
>  # ethtool --json -S eth0 --groups eth-phy eth-mac rmon | \
>      jq '.[].rmon.pktsNtoM | map(select(.low >= 128)) | map(.val) | add'
>  1
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  Makefile.am            |   1 +
>  ethtool.8.in           |  11 ++
>  ethtool.c              |   5 +-
>  netlink/desc-ethtool.c |  39 +++++++
>  netlink/extapi.h       |   4 +
>  netlink/stats.c        | 242 +++++++++++++++++++++++++++++++++++++++++
>  6 files changed, 301 insertions(+), 1 deletion(-)
>  create mode 100644 netlink/stats.c
> 
> diff --git a/Makefile.am b/Makefile.am
> index f643a24af97a..75c245653cda 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -36,6 +36,7 @@ ethtool_SOURCES += \
>  		  netlink/features.c netlink/privflags.c netlink/rings.c \
>  		  netlink/channels.c netlink/coalesce.c netlink/pause.c \
>  		  netlink/eee.c netlink/tsinfo.c netlink/fec.c \
> +		  netlink/stats.c \
>  		  netlink/desc-ethtool.c netlink/desc-genlctrl.c \
>  		  netlink/desc-rtnl.c netlink/cable_test.c netlink/tunnels.c \
>  		  uapi/linux/ethtool_netlink.h \
> diff --git a/ethtool.8.in b/ethtool.8.in
> index ba4e245cdc61..0ca11595158a 100644
> --- a/ethtool.8.in
> +++ b/ethtool.8.in
> @@ -240,6 +240,12 @@ ethtool \- query or control network driver and hardware settings
>  .HP
>  .B ethtool \-S|\-\-statistics
>  .I devname
> +.RB [\fB--groups
> +.RB [\fBeth\-phy\fP]
> +.RB [\fBeth\-mac\fP]
> +.RB [\fBeth\-ctrl\fP]
> +.RN [\fBrmon\fP]
> +.RB ]
>  .HP
>  .B ethtool \-\-phy\-statistics
>  .I devname
> @@ -653,6 +659,11 @@ auto-negotiation is enabled.
>  .B \-S \-\-statistics
>  Queries the specified network device for NIC- and driver-specific
>  statistics.
> +.RS 4
> +.TP
> +.B \fB--groups [\fBeth\-phy\fP] [\fBeth\-mac\fP] [\fBeth\-ctrl\fP] [\fBrmon\fP]
> +Request standard device statistics of a specific group.
> +.RE
>  .TP
>  .B \-\-phy\-statistics
>  Queries the specified network device for PHY specific statistics.
> diff --git a/ethtool.c b/ethtool.c
> index b07fd9292d77..1b5690f424c8 100644
> --- a/ethtool.c
> +++ b/ethtool.c
> @@ -5760,7 +5760,10 @@ static const struct option args[] = {
>  	{
>  		.opts	= "-S|--statistics",
>  		.func	= do_gnicstats,
> -		.help	= "Show adapter statistics"
> +		.nlchk	= nl_gstats_chk,
> +		.nlfunc	= nl_gstats,
> +		.help	= "Show adapter statistics",
> +		.xhelp	= "               [ --groups [eth-phy] [eth-mac] [eth-ctrl] [rmon] ]\n"
>  	},
>  	{
>  		.opts	= "--phy-statistics",
> diff --git a/netlink/desc-ethtool.c b/netlink/desc-ethtool.c
> index 9da052a5b8aa..63e91ee7ffcd 100644
> --- a/netlink/desc-ethtool.c
> +++ b/netlink/desc-ethtool.c
> @@ -325,6 +325,43 @@ static const struct pretty_nla_desc __fec_desc[] = {
>  	NLATTR_DESC_U32(ETHTOOL_A_FEC_ACTIVE),
>  };
>  
> +static const struct pretty_nla_desc __stats_grp_stat_desc[] = {
> +	NLATTR_DESC_U64(0),  NLATTR_DESC_U64(1),  NLATTR_DESC_U64(2),
> +	NLATTR_DESC_U64(3),  NLATTR_DESC_U64(4),  NLATTR_DESC_U64(5),
> +	NLATTR_DESC_U64(6),  NLATTR_DESC_U64(7),  NLATTR_DESC_U64(8),
> +	NLATTR_DESC_U64(9),  NLATTR_DESC_U64(10), NLATTR_DESC_U64(11),
> +	NLATTR_DESC_U64(12), NLATTR_DESC_U64(13), NLATTR_DESC_U64(14),
> +	NLATTR_DESC_U64(15), NLATTR_DESC_U64(16), NLATTR_DESC_U64(17),
> +	NLATTR_DESC_U64(18), NLATTR_DESC_U64(19), NLATTR_DESC_U64(20),
> +	NLATTR_DESC_U64(21), NLATTR_DESC_U64(22), NLATTR_DESC_U64(23),
> +	NLATTR_DESC_U64(24), NLATTR_DESC_U64(25), NLATTR_DESC_U64(26),
> +	NLATTR_DESC_U64(27), NLATTR_DESC_U64(28), NLATTR_DESC_U64(29),
> +};
> +
> +static const struct pretty_nla_desc __stats_grp_hist_desc[] = {
> +	NLATTR_DESC_U32(ETHTOOL_A_STATS_GRP_HIST_BKT_LOW),
> +	NLATTR_DESC_U32(ETHTOOL_A_STATS_GRP_HIST_BKT_HI),
> +	NLATTR_DESC_U64(ETHTOOL_A_STATS_GRP_HIST_VAL),
> +};
> +
> +static const struct pretty_nla_desc __stats_grp_desc[] = {
> +	NLATTR_DESC_INVALID(ETHTOOL_A_STATS_GRP_UNSPEC),
> +	NLATTR_DESC_INVALID(ETHTOOL_A_STATS_GRP_PAD),
> +	NLATTR_DESC_U32(ETHTOOL_A_STATS_GRP_ID),
> +	NLATTR_DESC_U32(ETHTOOL_A_STATS_GRP_SS_ID),
> +	NLATTR_DESC_NESTED(ETHTOOL_A_STATS_GRP_STAT, stats_grp_stat),
> +	NLATTR_DESC_NESTED(ETHTOOL_A_STATS_GRP_HIST_RX, stats_grp_hist),
> +	NLATTR_DESC_NESTED(ETHTOOL_A_STATS_GRP_HIST_TX, stats_grp_hist),
> +};
> +
> +static const struct pretty_nla_desc __stats_desc[] = {
> +	NLATTR_DESC_INVALID(ETHTOOL_A_STATS_UNSPEC),
> +	NLATTR_DESC_INVALID(ETHTOOL_A_STATS_PAD),
> +	NLATTR_DESC_NESTED(ETHTOOL_A_STATS_HEADER, header),
> +	NLATTR_DESC_NESTED(ETHTOOL_A_STATS_GROUPS, bitset),
> +	NLATTR_DESC_NESTED(ETHTOOL_A_STATS_GRP, stats_grp),
> +};
> +
>  const struct pretty_nlmsg_desc ethnl_umsg_desc[] = {
>  	NLMSG_DESC_INVALID(ETHTOOL_MSG_USER_NONE),
>  	NLMSG_DESC(ETHTOOL_MSG_STRSET_GET, strset),
> @@ -357,6 +394,7 @@ const struct pretty_nlmsg_desc ethnl_umsg_desc[] = {
>  	NLMSG_DESC(ETHTOOL_MSG_TUNNEL_INFO_GET, tunnel_info),
>  	NLMSG_DESC(ETHTOOL_MSG_FEC_GET, fec),
>  	NLMSG_DESC(ETHTOOL_MSG_FEC_SET, fec),
> +	NLMSG_DESC(ETHTOOL_MSG_STATS_GET, stats),
>  };
>  
>  const unsigned int ethnl_umsg_n_desc = ARRAY_SIZE(ethnl_umsg_desc);
> @@ -394,6 +432,7 @@ const struct pretty_nlmsg_desc ethnl_kmsg_desc[] = {
>  	NLMSG_DESC(ETHTOOL_MSG_TUNNEL_INFO_GET_REPLY, tunnel_info),
>  	NLMSG_DESC(ETHTOOL_MSG_FEC_GET_REPLY, fec),
>  	NLMSG_DESC(ETHTOOL_MSG_FEC_NTF, fec),
> +	NLMSG_DESC(ETHTOOL_MSG_STATS_GET_REPLY, stats),
>  };
>  
>  const unsigned int ethnl_kmsg_n_desc = ARRAY_SIZE(ethnl_kmsg_desc);
> diff --git a/netlink/extapi.h b/netlink/extapi.h
> index d6036a39e920..97113eb98ca0 100644
> --- a/netlink/extapi.h
> +++ b/netlink/extapi.h
> @@ -41,6 +41,8 @@ int nl_cable_test_tdr(struct cmd_context *ctx);
>  int nl_gtunnels(struct cmd_context *ctx);
>  int nl_gfec(struct cmd_context *ctx);
>  int nl_sfec(struct cmd_context *ctx);
> +bool nl_gstats_chk(struct cmd_context *ctx);
> +int nl_gstats(struct cmd_context *ctx);
>  int nl_monitor(struct cmd_context *ctx);
>  
>  void nl_monitor_usage(void);
> @@ -92,6 +94,8 @@ static inline void nl_monitor_usage(void)
>  #define nl_gtunnels		NULL
>  #define nl_gfec			NULL
>  #define nl_sfec			NULL
> +#define nl_gstats_chk		NULL
> +#define nl_gstats		NULL
>  
>  #endif /* ETHTOOL_ENABLE_NETLINK */
>  
> diff --git a/netlink/stats.c b/netlink/stats.c
> new file mode 100644
> index 000000000000..6c0b37a9aa4a
> --- /dev/null
> +++ b/netlink/stats.c
> @@ -0,0 +1,242 @@
> +/*
> + * stats.c - netlink implementation of stats
> + *
> + * Implementation of "ethtool -S <dev> <types>" and
> + */
> +
> +#include <errno.h>
> +#include <ctype.h>
> +#include <inttypes.h>
> +#include <string.h>
> +#include <stdio.h>
> +
> +#include "../internal.h"
> +#include "../common.h"
> +#include "netlink.h"
> +#include "parser.h"
> +#include "strset.h"
> +
> +static int parse_rmon_hist(const struct nlattr *hist)
> +{
> +	const struct nlattr *tb[ETHTOOL_A_STATS_GRP_HIST_VAL + 1] = {};
> +	DECLARE_ATTR_TB_INFO(tb);
> +	unsigned long long val;
> +	unsigned int low, hi;
> +	int ret;
> +
> +	ret = mnl_attr_parse_nested(hist, attr_cb, &tb_info);
> +	if (ret < 0) {
> +		fprintf(stderr, "invalid kernel response - malformed histogram entry\n");
> +		return 1;
> +	}
> +
> +	if (!tb[ETHTOOL_A_STATS_GRP_HIST_BKT_LOW] ||
> +	    !tb[ETHTOOL_A_STATS_GRP_HIST_BKT_HI] ||
> +	    !tb[ETHTOOL_A_STATS_GRP_HIST_VAL]) {
> +		fprintf(stderr, "invalid kernel response - histogram entry missing attributes\n");
> +		return 1;
> +	}
> +
> +	low = mnl_attr_get_u32(tb[ETHTOOL_A_STATS_GRP_HIST_BKT_LOW]);
> +	hi = mnl_attr_get_u32(tb[ETHTOOL_A_STATS_GRP_HIST_BKT_HI]);
> +	val = mnl_attr_get_u64(tb[ETHTOOL_A_STATS_GRP_HIST_VAL]);
> +
> +	if (!is_json_context()) {
> +		fprintf(stdout, "rmon-%s-etherStatsPkts",
> +			mnl_attr_get_type(hist) == ETHTOOL_A_STATS_GRP_HIST_RX ?
> +			"rx" : "tx");
> +
> +		if (low && hi) {
> +			fprintf(stdout, "%uto%uOctets: %llu\n", low, hi, val);
> +		} else if (hi) {
> +			fprintf(stdout, "%uOctets: %llu\n", hi, val);
> +		} else if (low) {
> +			fprintf(stdout, "%utoMaxOctets: %llu\n", low, val);
> +		} else {
> +			fprintf(stderr, "invalid kernel response - bad histogram entry bounds\n");
> +			return 1;
> +		}
> +	} else {
> +		open_json_object(NULL);
> +		print_uint(PRINT_JSON, "low", NULL, low);
> +		print_uint(PRINT_JSON, "high", NULL, hi);
> +		print_u64(PRINT_JSON, "val", NULL, val);

In the non-JSON output you distinguish between Rx/Tx, but it's missing
from the JSON output as can be seen in your example:

```
       "pktsNtoM": [
         {
           "low": 0,
           "high": 64,
           "val": 1
         },
         {
           "low": 128,
           "high": 255,
           "val": 1
         },
         {
           "low": 1024,
           "high": 0,
           "val": 0
         }
       ]
```

I see that mlxsw and mlx5 only support Rx, but it's going to be
confusing with bnxt that supports both Rx and Tx.

Made me think about the structure of these attributes. Currently you
have:

ETHTOOL_A_STATS_GRP_HIST_RX
	ETHTOOL_A_STATS_GRP_HIST_BKT_LOW
	ETHTOOL_A_STATS_GRP_HIST_BKT_HI
	ETHTOOL_A_STATS_GRP_HIST_VAL

ETHTOOL_A_STATS_GRP_HIST_TX
	ETHTOOL_A_STATS_GRP_HIST_BKT_LOW
	ETHTOOL_A_STATS_GRP_HIST_BKT_HI
	ETHTOOL_A_STATS_GRP_HIST_VAL

Did you consider:

ETHTOOL_A_STATS_GRP_HIST
	ETHTOOL_A_STATS_GRP_HIST_BKT_LOW
	ETHTOOL_A_STATS_GRP_HIST_BKT_HI
	ETHTOOL_A_STATS_GRP_HIST_VAL
	ETHTOOL_A_STATS_GRP_HIST_BKT_UNITS
	ETHTOOL_A_STATS_GRP_HIST_TYPE

So you will have something like:

ETHTOOL_A_STATS_GRP_HIST_BKT_UNITS_BYTES
ETHTOOL_A_STATS_GRP_HIST_VAL_TYPE_RX_PACKETS
ETHTOOL_A_STATS_GRP_HIST_VAL_TYPE_TX_PACKETS

And it will allow you to get rid of the special casing of the RMON stuff
below:

```
	if (id == ETHTOOL_STATS_RMON) {
		open_json_array("pktsNtoM", "");

		mnl_attr_for_each_nested(attr, grp) {
			s = mnl_attr_get_type(attr);
			if (s != ETHTOOL_A_STATS_GRP_HIST_RX &&
			    s != ETHTOOL_A_STATS_GRP_HIST_TX)
				continue;

			if (parse_rmon_hist(attr))
				goto err_close_rmon;
		}
		close_json_array("");
	}
```

I don't know how many histograms we are going to have as part of RFCs,
but at least mlxsw also supports histograms of the Tx queue depth and
latency. Not to be exposed by this interface, but shows the importance
of encoding the units.

> +		close_json_object();
> +	}
> +
> +	return 0;
> +}
> +
> +static int parse_grp(struct nl_context *nlctx, const struct nlattr *grp,
> +		     const struct stringset *std_str)
> +{
> +	const struct nlattr *tb[ETHTOOL_A_STATS_GRP_SS_ID + 1] = {};
> +	DECLARE_ATTR_TB_INFO(tb);
> +	const struct stringset *stat_str;
> +	const struct nlattr *attr, *stat;
> +	const char *std_name, *name;
> +	unsigned int ss_id, id, s;
> +	unsigned long long val;
> +	int ret;
> +
> +	ret = mnl_attr_parse_nested(grp, attr_cb, &tb_info);
> +	if (ret < 0)
> +		return 1;
> +
> +	if (!tb[ETHTOOL_A_STATS_GRP_ID])
> +		return 1;
> +	if (!tb[ETHTOOL_A_STATS_GRP_SS_ID])
> +		return 0;
> +
> +	id = mnl_attr_get_u32(tb[ETHTOOL_A_STATS_GRP_ID]);
> +	ss_id = mnl_attr_get_u32(tb[ETHTOOL_A_STATS_GRP_SS_ID]);
> +
> +	stat_str = global_stringset(ss_id, nlctx->ethnl2_socket);
> +
> +	std_name = get_string(std_str, id);
> +	open_json_object(std_name);
> +
> +	mnl_attr_for_each_nested(attr, grp) {
> +		if (mnl_attr_get_type(attr) != ETHTOOL_A_STATS_GRP_STAT)
> +			continue;
> +		stat = mnl_attr_get_payload(attr);
> +		ret = mnl_attr_validate(stat, MNL_TYPE_U64);
> +		if (ret) {
> +			fprintf(stderr, "invalid kernel response - bad statistic entry\n");
> +			goto err_close_grp;
> +		}
> +		s = mnl_attr_get_type(stat);
> +		name = get_string(stat_str, s);
> +		if (!name || !name[0])
> +			continue;
> +
> +		if (!is_json_context())
> +			fprintf(stdout, "%s-%s: ", std_name, name);
> +
> +		val = mnl_attr_get_u64(stat);
> +		print_u64(PRINT_ANY, name, "%llu\n", val);
> +	}
> +
> +	if (id == ETHTOOL_STATS_RMON) {
> +		open_json_array("pktsNtoM", "");
> +
> +		mnl_attr_for_each_nested(attr, grp) {
> +			s = mnl_attr_get_type(attr);
> +			if (s != ETHTOOL_A_STATS_GRP_HIST_RX &&
> +			    s != ETHTOOL_A_STATS_GRP_HIST_TX)
> +				continue;
> +
> +			if (parse_rmon_hist(attr))
> +				goto err_close_rmon;
> +		}
> +		close_json_array("");
> +	}
> +
> +	close_json_object();
> +
> +	return 0;
> +
> +err_close_rmon:
> +	close_json_array("");
> +err_close_grp:
> +	close_json_object();
> +	return 1;
> +}
> +
> +static int stats_reply_cb(const struct nlmsghdr *nlhdr, void *data)
> +{
> +	const struct nlattr *tb[ETHTOOL_A_STATS_MAX + 1] = {};
> +	DECLARE_ATTR_TB_INFO(tb);
> +	struct nl_context *nlctx = data;
> +	const struct stringset *std_str;
> +	const struct nlattr *attr;
> +	bool silent;
> +	int err_ret;
> +	int ret;
> +
> +	silent = nlctx->is_dump || nlctx->is_monitor;
> +	err_ret = silent ? MNL_CB_OK : MNL_CB_ERROR;
> +	ret = mnl_attr_parse(nlhdr, GENL_HDRLEN, attr_cb, &tb_info);
> +	if (ret < 0)
> +		return err_ret;
> +	nlctx->devname = get_dev_name(tb[ETHTOOL_A_STATS_HEADER]);
> +	if (!dev_ok(nlctx))
> +		return err_ret;
> +
> +	ret = netlink_init_ethnl2_socket(nlctx);
> +	if (ret < 0)
> +		return err_ret;
> +	std_str = global_stringset(ETH_SS_STATS_STD, nlctx->ethnl2_socket);
> +
> +	if (silent)
> +		print_nl();
> +
> +	open_json_object(NULL);
> +
> +	print_string(PRINT_ANY, "ifname", "Standard stats for %s:\n",
> +		     nlctx->devname);
> +
> +	mnl_attr_for_each(attr, nlhdr, GENL_HDRLEN) {
> +		if (mnl_attr_get_type(attr) == ETHTOOL_A_STATS_GRP) {
> +			ret = parse_grp(nlctx, attr, std_str);
> +			if (ret)
> +				goto err_close_dev;
> +		}
> +	}
> +
> +	close_json_object();
> +
> +	return MNL_CB_OK;
> +
> +err_close_dev:
> +	close_json_object();
> +	return err_ret;
> +}
> +
> +static const struct bitset_parser_data stats_parser_data = {
> +	.no_mask	= true,
> +	.force_hex	= false,
> +};
> +
> +static const struct param_parser stats_params[] = {
> +	{
> +		.arg		= "--groups",
> +		.type		= ETHTOOL_A_STATS_GROUPS,
> +		.handler	= nl_parse_bitset,
> +		.handler_data	= &stats_parser_data,
> +		.min_argc	= 1,
> +	},
> +	{}
> +};
> +
> +int nl_gstats(struct cmd_context *ctx)
> +{
> +	struct nl_context *nlctx = ctx->nlctx;
> +	struct nl_socket *nlsk = nlctx->ethnl_socket;
> +	int ret;
> +
> +	ret = nlsock_prep_get_request(nlsk, ETHTOOL_MSG_STATS_GET,
> +				      ETHTOOL_A_STATS_HEADER, 0);
> +	if (ret < 0)
> +		return ret;
> +
> +	nlctx->cmd = "-S";
> +	nlctx->argp = ctx->argp;
> +	nlctx->argc = ctx->argc;
> +	nlctx->devname = ctx->devname;
> +	nlsk = nlctx->ethnl_socket;
> +
> +	ret = nl_parser(nlctx, stats_params, NULL, PARSER_GROUP_NONE, NULL);
> +	if (ret < 0)
> +		return 1;
> +
> +	new_json_obj(ctx->json);
> +	ret = nlsock_send_get_request(nlsk, stats_reply_cb);
> +	delete_json_obj();
> +	return ret;
> +}
> +
> +bool nl_gstats_chk(struct cmd_context *ctx)
> +{
> +	return ctx->argc;
> +}
> -- 
> 2.30.2
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFC ethtool 6/6] netlink: add support for standard stats
  2021-04-17 18:14   ` Ido Schimmel
@ 2021-04-17 18:47     ` Jakub Kicinski
  2021-04-17 19:22       ` Ido Schimmel
  0 siblings, 1 reply; 11+ messages in thread
From: Jakub Kicinski @ 2021-04-17 18:47 UTC (permalink / raw)
  To: Ido Schimmel; +Cc: netdev, idosch, mkubecek

On Sat, 17 Apr 2021 21:14:40 +0300 Ido Schimmel wrote:
> > +	if (!is_json_context()) {
> > +		fprintf(stdout, "rmon-%s-etherStatsPkts",
> > +			mnl_attr_get_type(hist) == ETHTOOL_A_STATS_GRP_HIST_RX ?
> > +			"rx" : "tx");
> > +
> > +		if (low && hi) {
> > +			fprintf(stdout, "%uto%uOctets: %llu\n", low, hi, val);
> > +		} else if (hi) {
> > +			fprintf(stdout, "%uOctets: %llu\n", hi, val);
> > +		} else if (low) {
> > +			fprintf(stdout, "%utoMaxOctets: %llu\n", low, val);
> > +		} else {
> > +			fprintf(stderr, "invalid kernel response - bad histogram entry bounds\n");
> > +			return 1;
> > +		}
> > +	} else {
> > +		open_json_object(NULL);
> > +		print_uint(PRINT_JSON, "low", NULL, low);
> > +		print_uint(PRINT_JSON, "high", NULL, hi);
> > +		print_u64(PRINT_JSON, "val", NULL, val);  
> 
> In the non-JSON output you distinguish between Rx/Tx, but it's missing
> from the JSON output as can be seen in your example:
> 
> ```
>        "pktsNtoM": [
>          {
>            "low": 0,
>            "high": 64,
>            "val": 1
>          },
>          {
>            "low": 128,
>            "high": 255,
>            "val": 1
>          },
>          {
>            "low": 1024,
>            "high": 0,
>            "val": 0
>          }
>        ]
> ```
> 
> I see that mlxsw and mlx5 only support Rx, but it's going to be
> confusing with bnxt that supports both Rx and Tx.

Good catch! I added Tx last minute (even though it's non standard).
I'll split split into two arrays - "rx-pktsNtoM" and "tx-pktsNtoM",
sounds good? Or we can add a layer: ["pktsNtoM"]["rx"] etc.

> Made me think about the structure of these attributes. Currently you
> have:
> 
> ETHTOOL_A_STATS_GRP_HIST_RX
> 	ETHTOOL_A_STATS_GRP_HIST_BKT_LOW
> 	ETHTOOL_A_STATS_GRP_HIST_BKT_HI
> 	ETHTOOL_A_STATS_GRP_HIST_VAL
> 
> ETHTOOL_A_STATS_GRP_HIST_TX
> 	ETHTOOL_A_STATS_GRP_HIST_BKT_LOW
> 	ETHTOOL_A_STATS_GRP_HIST_BKT_HI
> 	ETHTOOL_A_STATS_GRP_HIST_VAL
> 
> Did you consider:
> 
> ETHTOOL_A_STATS_GRP_HIST
> 	ETHTOOL_A_STATS_GRP_HIST_BKT_LOW
> 	ETHTOOL_A_STATS_GRP_HIST_BKT_HI
> 	ETHTOOL_A_STATS_GRP_HIST_VAL
> 	ETHTOOL_A_STATS_GRP_HIST_BKT_UNITS
> 	ETHTOOL_A_STATS_GRP_HIST_TYPE

I went back and forth on that. The reason I put the direction in the
type is that normal statistics don't have an extra _TYPE or direction.

It will also be easier to break the stats out to arrays if they are
typed on the outside, see below.

> So you will have something like:
> 
> ETHTOOL_A_STATS_GRP_HIST_BKT_UNITS_BYTES

Histogram has two dimensions, what's the second dimension for bytes?
Time? Packet arrival?

> ETHTOOL_A_STATS_GRP_HIST_VAL_TYPE_RX_PACKETS
> ETHTOOL_A_STATS_GRP_HIST_VAL_TYPE_TX_PACKETS
> 
> And it will allow you to get rid of the special casing of the RMON stuff
> below:
> 
> ```
> 	if (id == ETHTOOL_STATS_RMON) {
> 		open_json_array("pktsNtoM", "");
> 
> 		mnl_attr_for_each_nested(attr, grp) {
> 			s = mnl_attr_get_type(attr);
> 			if (s != ETHTOOL_A_STATS_GRP_HIST_RX &&
> 			    s != ETHTOOL_A_STATS_GRP_HIST_TX)
> 				continue;
> 
> 			if (parse_rmon_hist(attr))
> 				goto err_close_rmon;
> 		}
> 		close_json_array("");
> 	}
> ```

We can drop the if, but we still need a separate for() loop
to be able to place those entries in a JSON array.

> I don't know how many histograms we are going to have as part of RFCs,
> but at least mlxsw also supports histograms of the Tx queue depth and
> latency. Not to be exposed by this interface, but shows the importance
> of encoding the units.

TBH I hope we'll never use the hist for anything else. Sadly the
bucketing of various drivers is really different (at least 6
variants). But the overarching goal is a common interface for common
port stats.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFC ethtool 6/6] netlink: add support for standard stats
  2021-04-17 18:47     ` Jakub Kicinski
@ 2021-04-17 19:22       ` Ido Schimmel
  2021-04-17 19:44         ` Jakub Kicinski
  0 siblings, 1 reply; 11+ messages in thread
From: Ido Schimmel @ 2021-04-17 19:22 UTC (permalink / raw)
  To: Jakub Kicinski; +Cc: netdev, idosch, mkubecek

On Sat, Apr 17, 2021 at 11:47:28AM -0700, Jakub Kicinski wrote:
> On Sat, 17 Apr 2021 21:14:40 +0300 Ido Schimmel wrote:
> > > +	if (!is_json_context()) {
> > > +		fprintf(stdout, "rmon-%s-etherStatsPkts",
> > > +			mnl_attr_get_type(hist) == ETHTOOL_A_STATS_GRP_HIST_RX ?
> > > +			"rx" : "tx");
> > > +
> > > +		if (low && hi) {
> > > +			fprintf(stdout, "%uto%uOctets: %llu\n", low, hi, val);
> > > +		} else if (hi) {
> > > +			fprintf(stdout, "%uOctets: %llu\n", hi, val);
> > > +		} else if (low) {
> > > +			fprintf(stdout, "%utoMaxOctets: %llu\n", low, val);
> > > +		} else {
> > > +			fprintf(stderr, "invalid kernel response - bad histogram entry bounds\n");
> > > +			return 1;
> > > +		}
> > > +	} else {
> > > +		open_json_object(NULL);
> > > +		print_uint(PRINT_JSON, "low", NULL, low);
> > > +		print_uint(PRINT_JSON, "high", NULL, hi);
> > > +		print_u64(PRINT_JSON, "val", NULL, val);  
> > 
> > In the non-JSON output you distinguish between Rx/Tx, but it's missing
> > from the JSON output as can be seen in your example:
> > 
> > ```
> >        "pktsNtoM": [
> >          {
> >            "low": 0,
> >            "high": 64,
> >            "val": 1
> >          },
> >          {
> >            "low": 128,
> >            "high": 255,
> >            "val": 1
> >          },
> >          {
> >            "low": 1024,
> >            "high": 0,
> >            "val": 0
> >          }
> >        ]
> > ```
> > 
> > I see that mlxsw and mlx5 only support Rx, but it's going to be
> > confusing with bnxt that supports both Rx and Tx.
> 
> Good catch! I added Tx last minute (even though it's non standard).
> I'll split split into two arrays - "rx-pktsNtoM" and "tx-pktsNtoM",
> sounds good? Or we can add a layer: ["pktsNtoM"]["rx"] etc.

I'm fine with both, but I think the first form will be easier when
working with jq to extract Rx/Tx. It is also more inline with the
current nesting of the netlink attributes.

> 
> > Made me think about the structure of these attributes. Currently you
> > have:
> > 
> > ETHTOOL_A_STATS_GRP_HIST_RX
> > 	ETHTOOL_A_STATS_GRP_HIST_BKT_LOW
> > 	ETHTOOL_A_STATS_GRP_HIST_BKT_HI
> > 	ETHTOOL_A_STATS_GRP_HIST_VAL
> > 
> > ETHTOOL_A_STATS_GRP_HIST_TX
> > 	ETHTOOL_A_STATS_GRP_HIST_BKT_LOW
> > 	ETHTOOL_A_STATS_GRP_HIST_BKT_HI
> > 	ETHTOOL_A_STATS_GRP_HIST_VAL
> > 
> > Did you consider:
> > 
> > ETHTOOL_A_STATS_GRP_HIST
> > 	ETHTOOL_A_STATS_GRP_HIST_BKT_LOW
> > 	ETHTOOL_A_STATS_GRP_HIST_BKT_HI
> > 	ETHTOOL_A_STATS_GRP_HIST_VAL
> > 	ETHTOOL_A_STATS_GRP_HIST_BKT_UNITS
> > 	ETHTOOL_A_STATS_GRP_HIST_TYPE
> 
> I went back and forth on that. The reason I put the direction in the
> type is that normal statistics don't have an extra _TYPE or direction.
> 
> It will also be easier to break the stats out to arrays if they are
> typed on the outside, see below.
> 
> > So you will have something like:
> > 
> > ETHTOOL_A_STATS_GRP_HIST_BKT_UNITS_BYTES
> 
> Histogram has two dimensions, what's the second dimension for bytes?
> Time? Packet arrival?

Not sure what you mean. Here you are counting how many Rx/Tx packets are
between N to M bytes in length. I meant to add two attributes. One that
tells user space that you are counting Rx/Tx packets and the second that
N to M are in bytes.

But given your comment below about this histogram probably being a one
time thing, I think maybe staying with the current attributes is OK.
There is no need to over-engineer it if we don't see ourselves adding
new histograms.

Anyway, these histograms are under ETHTOOL_A_STATS_GRP that should give
user space all the context about what is being counted.

> 
> > ETHTOOL_A_STATS_GRP_HIST_VAL_TYPE_RX_PACKETS
> > ETHTOOL_A_STATS_GRP_HIST_VAL_TYPE_TX_PACKETS
> > 
> > And it will allow you to get rid of the special casing of the RMON stuff
> > below:
> > 
> > ```
> > 	if (id == ETHTOOL_STATS_RMON) {
> > 		open_json_array("pktsNtoM", "");
> > 
> > 		mnl_attr_for_each_nested(attr, grp) {
> > 			s = mnl_attr_get_type(attr);
> > 			if (s != ETHTOOL_A_STATS_GRP_HIST_RX &&
> > 			    s != ETHTOOL_A_STATS_GRP_HIST_TX)
> > 				continue;
> > 
> > 			if (parse_rmon_hist(attr))
> > 				goto err_close_rmon;
> > 		}
> > 		close_json_array("");
> > 	}
> > ```
> 
> We can drop the if, but we still need a separate for() loop
> to be able to place those entries in a JSON array.
> 
> > I don't know how many histograms we are going to have as part of RFCs,
> > but at least mlxsw also supports histograms of the Tx queue depth and
> > latency. Not to be exposed by this interface, but shows the importance
> > of encoding the units.
> 
> TBH I hope we'll never use the hist for anything else. Sadly the
> bucketing of various drivers is really different (at least 6
> variants). But the overarching goal is a common interface for common
> port stats.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFC ethtool 6/6] netlink: add support for standard stats
  2021-04-17 19:22       ` Ido Schimmel
@ 2021-04-17 19:44         ` Jakub Kicinski
  0 siblings, 0 replies; 11+ messages in thread
From: Jakub Kicinski @ 2021-04-17 19:44 UTC (permalink / raw)
  To: Ido Schimmel; +Cc: netdev, idosch, mkubecek

On Sat, 17 Apr 2021 22:22:57 +0300 Ido Schimmel wrote:
> > > So you will have something like:
> > > 
> > > ETHTOOL_A_STATS_GRP_HIST_BKT_UNITS_BYTES  
> > 
> > Histogram has two dimensions, what's the second dimension for bytes?
> > Time? Packet arrival?  
> 
> Not sure what you mean. Here you are counting how many Rx/Tx packets are
> between N to M bytes in length. I meant to add two attributes. One that
> tells user space that you are counting Rx/Tx packets and the second that
> N to M are in bytes.

Ah, these were not part of the same enum, I get it now.

I thought maybe bytes were trying to cater to the queue length use case
and I wasn't sure how that histogram is constructed.

> But given your comment below about this histogram probably being a one
> time thing, I think maybe staying with the current attributes is OK.
> There is no need to over-engineer it if we don't see ourselves adding
> new histograms.
> 
> Anyway, these histograms are under ETHTOOL_A_STATS_GRP that should give
> user space all the context about what is being counted.


^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2021-04-17 19:44 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-16 16:02 [RFC ethtool 0/6] ethtool: add FEC & std stats support Jakub Kicinski
2021-04-16 16:02 ` [RFC ethtool 1/6] update UAPI header copies Jakub Kicinski
2021-04-16 16:02 ` [RFC ethtool 2/6] json: simplify array print API Jakub Kicinski
2021-04-16 16:02 ` [RFC ethtool 3/6] netlink: add FEC support Jakub Kicinski
2021-04-16 16:02 ` [RFC ethtool 4/6] netlink: fec: support displaying statistics Jakub Kicinski
2021-04-16 16:02 ` [RFC ethtool 5/6] ethtool: add nlchk for redirecting to netlink Jakub Kicinski
2021-04-16 16:02 ` [RFC ethtool 6/6] netlink: add support for standard stats Jakub Kicinski
2021-04-17 18:14   ` Ido Schimmel
2021-04-17 18:47     ` Jakub Kicinski
2021-04-17 19:22       ` Ido Schimmel
2021-04-17 19:44         ` Jakub Kicinski

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