diff mbox series

[net-next,3/5] ethtool: add interface to read representor Rx statistics

Message ID 20240404173357.123307-4-tariqt@nvidia.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series mlx5e rc2 misc patches | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 2589 this patch: 2589
netdev/build_tools success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers warning 7 maintainers not CCed: kory.maincent@bootlin.com jiri@resnulli.us hayatake396@gmail.com andrew@lunn.ch ahmed.zaki@intel.com corbet@lwn.net maxime.chevallier@bootlin.com
netdev/build_clang success Errors and warnings before: 1022 this patch: 1022
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 2769 this patch: 2769
netdev/checkpatch warning CHECK: Alignment should match open parenthesis WARNING: line length of 83 exceeds 80 columns WARNING: line length of 84 exceeds 80 columns WARNING: line length of 86 exceeds 80 columns WARNING: line length of 87 exceeds 80 columns WARNING: line length of 88 exceeds 80 columns WARNING: line length of 96 exceeds 80 columns
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc fail Errors and warnings before: 7 this patch: 8
netdev/source_inline success Was 0 now: 0
netdev/contest fail net-next-2024-04-05--15-00 (tests: 956)

Commit Message

Tariq Toukan April 4, 2024, 5:33 p.m. UTC
From: Carolina Jubran <cjubran@nvidia.com>

Implement common representor port statistics in
a rep_port_stats struct_group, introducing a new
'out of buffer' stats for when packets are dropped
due to a lack of receive buffers in RX queue.

The port statistics represent the statistics of the
function with which the representor is associated.

Print the representor port stats when the
--groups rep-port or --all-groups are used.

Signed-off-by: Carolina Jubran <cjubran@nvidia.com>
Reviewed-by: Rahul Rameshbabu <rrameshbabu@nvidia.com>
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---
 include/linux/ethtool.h              | 16 ++++++++++++++
 include/uapi/linux/ethtool.h         |  2 ++
 include/uapi/linux/ethtool_netlink.h | 10 +++++++++
 net/ethtool/netlink.h                |  1 +
 net/ethtool/stats.c                  | 31 ++++++++++++++++++++++++++++
 net/ethtool/strset.c                 |  5 +++++
 6 files changed, 65 insertions(+)

Comments

Jakub Kicinski April 6, 2024, 4:53 a.m. UTC | #1
On Thu, 4 Apr 2024 20:33:55 +0300 Tariq Toukan wrote:
> Implement common representor port statistics in
> a rep_port_stats struct_group, introducing a new
> 'out of buffer' stats for when packets are dropped
> due to a lack of receive buffers in RX queue.
> 
> The port statistics represent the statistics of the
> function with which the representor is associated.
> 
> Print the representor port stats when the
> --groups rep-port or --all-groups are used.

I re-read what Tariq said on v1 and I clearly missed the point,
sorry about that. Looking that his patch makes it pretty obvious.

With the netdev netlink family in place I was hoping we would
only put in ethtool stats for functionality already configured
via ethtool or clearly related to Ethernet.

But before we go to netdev - can you think of more such error stats 
that we may need to add? Since it's the equivalent of rtnl rx_missed 
I think we should consider putting it in netdev_offload_xstats. Maybe
following the same definition as packet/byte counters? Report sum by
default and CPU ones under IFLA_OFFLOAD_XSTATS_CPU_HIT? Or new enum
entry?

Simon, WDYT?

> +/**
> + * struct ethtool_rep_port_stats - representor port statistics
> + * @rep_port_stats: struct group for representor port

In more trivial remarks - kernel-doc script apparently doesn't want
the group to be documented (any more?)

> + *	@out_of_buf: Number of packets were dropped due to buffer exhaustion.
> + */
> +struct ethtool_rep_port_stats {
> +	struct_group(rep_port_stats,
> +		u64 out_of_buf;
> +	);
> +};
> +
Rahul Rameshbabu April 6, 2024, 5:25 a.m. UTC | #2
On Fri, 05 Apr, 2024 21:53:35 -0700 Jakub Kicinski <kuba@kernel.org> wrote:
> On Thu, 4 Apr 2024 20:33:55 +0300 Tariq Toukan wrote:
>> +/**
>> + * struct ethtool_rep_port_stats - representor port statistics
>> + * @rep_port_stats: struct group for representor port
>
> In more trivial remarks - kernel-doc script apparently doesn't want
> the group to be documented (any more?)
>

I took a look, and I believe the behavior of kernel-doc has remained the
same since the struct_group() helper macro was introduced. That said, I
think allowing the documentation of struct_group() would be a reasonable
choice/maybe worth updating the script.

  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=50d7bd38c3aafc4749e05e8d7fcb616979143602

>> + *	@out_of_buf: Number of packets were dropped due to buffer exhaustion.
>> + */
>> +struct ethtool_rep_port_stats {
>> +	struct_group(rep_port_stats,
>> +		u64 out_of_buf;
>> +	);
>> +};
>> +
Jakub Kicinski April 6, 2024, 5:46 a.m. UTC | #3
On Fri, 05 Apr 2024 22:25:21 -0700 Rahul Rameshbabu wrote:
> > In more trivial remarks - kernel-doc script apparently doesn't want
> > the group to be documented (any more?)
> 
> I took a look, and I believe the behavior of kernel-doc has remained the
> same since the struct_group() helper macro was introduced. That said, I
> think allowing the documentation of struct_group() would be a reasonable
> choice/maybe worth updating the script.

Thanks for investigating! Dunno why this started getting hit so much out
of the sudden.
diff mbox series

Patch

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 9901e563f706..6f2a6c78d41d 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -480,6 +480,17 @@  struct ethtool_rmon_stats {
 	);
 };
 
+/**
+ * struct ethtool_rep_port_stats - representor port statistics
+ * @rep_port_stats: struct group for representor port
+ *	@out_of_buf: Number of packets were dropped due to buffer exhaustion.
+ */
+struct ethtool_rep_port_stats {
+	struct_group(rep_port_stats,
+		u64 out_of_buf;
+	);
+};
+
 #define ETH_MODULE_EEPROM_PAGE_LEN	128
 #define ETH_MODULE_MAX_I2C_ADDRESS	0x7f
 
@@ -804,6 +815,8 @@  struct ethtool_rxfh_param {
  * @get_eth_ctrl_stats: Query some of the IEEE 802.3 MAC Ctrl statistics.
  * @get_rmon_stats: Query some of the RMON (RFC 2819) statistics.
  *	Set %ranges to a pointer to zero-terminated array of byte ranges.
+ * @get_rep_port_stats: Query the representor port statistics.
+ *	Returns zero on success.
  * @get_module_power_mode: Get the power mode policy for the plug-in module
  *	used by the network device and its operational power mode, if
  *	plugged-in.
@@ -940,6 +953,9 @@  struct ethtool_ops {
 	void	(*get_rmon_stats)(struct net_device *dev,
 				  struct ethtool_rmon_stats *rmon_stats,
 				  const struct ethtool_rmon_hist_range **ranges);
+	int    (*get_rep_port_stats)(struct net_device *dev,
+				     struct ethtool_rep_port_stats *rep_port_stats,
+				     struct netlink_ext_ack *extack);
 	int	(*get_module_power_mode)(struct net_device *dev,
 					 struct ethtool_module_power_mode_params *params,
 					 struct netlink_ext_ack *extack);
diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index 11fc18988bc2..a3bc96e7e958 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -681,6 +681,7 @@  enum ethtool_link_ext_substate_module {
  * @ETH_SS_STATS_ETH_MAC: names of IEEE 802.3 MAC statistics
  * @ETH_SS_STATS_ETH_CTRL: names of IEEE 802.3 MAC Control statistics
  * @ETH_SS_STATS_RMON: names of RMON statistics
+ * @ETH_SS_STATS_REP_PORT: names of representor port statistics
  *
  * @ETH_SS_COUNT: number of defined string sets
  */
@@ -706,6 +707,7 @@  enum ethtool_stringset {
 	ETH_SS_STATS_ETH_MAC,
 	ETH_SS_STATS_ETH_CTRL,
 	ETH_SS_STATS_RMON,
+	ETH_SS_STATS_REP_PORT,
 
 	/* add new constants above here */
 	ETH_SS_COUNT
diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
index accbb1a231df..0257103a001a 100644
--- a/include/uapi/linux/ethtool_netlink.h
+++ b/include/uapi/linux/ethtool_netlink.h
@@ -764,6 +764,7 @@  enum {
 	ETHTOOL_STATS_ETH_MAC,
 	ETHTOOL_STATS_ETH_CTRL,
 	ETHTOOL_STATS_RMON,
+	ETHTOOL_STATS_REP_PORT,
 
 	/* add new constants above here */
 	__ETHTOOL_STATS_CNT
@@ -879,6 +880,15 @@  enum {
 	ETHTOOL_A_STATS_RMON_MAX = (__ETHTOOL_A_STATS_RMON_CNT - 1)
 };
 
+enum {
+	/* out_of_buf */
+	ETHTOOL_A_STATS_REP_PORT_OUT_OF_BUF,
+
+	/* add new constants above here */
+	__ETHTOOL_A_STATS_REP_PORT_CNT,
+	ETHTOOL_A_STATS_REP_PORT_MAX = (__ETHTOOL_A_STATS_REP_PORT_CNT - 1)
+};
+
 /* MODULE */
 
 enum {
diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h
index 9a333a8d04c1..f1568fa788f2 100644
--- a/net/ethtool/netlink.h
+++ b/net/ethtool/netlink.h
@@ -454,5 +454,6 @@  extern const char stats_eth_phy_names[__ETHTOOL_A_STATS_ETH_PHY_CNT][ETH_GSTRING
 extern const char stats_eth_mac_names[__ETHTOOL_A_STATS_ETH_MAC_CNT][ETH_GSTRING_LEN];
 extern const char stats_eth_ctrl_names[__ETHTOOL_A_STATS_ETH_CTRL_CNT][ETH_GSTRING_LEN];
 extern const char stats_rmon_names[__ETHTOOL_A_STATS_RMON_CNT][ETH_GSTRING_LEN];
+extern const char stats_rep_port_names[__ETHTOOL_A_STATS_REP_PORT_CNT][ETH_GSTRING_LEN];
 
 #endif /* _NET_ETHTOOL_NETLINK_H */
diff --git a/net/ethtool/stats.c b/net/ethtool/stats.c
index 912f0c4fff2f..84b05d9a2fc1 100644
--- a/net/ethtool/stats.c
+++ b/net/ethtool/stats.c
@@ -20,6 +20,7 @@  struct stats_reply_data {
 		struct ethtool_eth_mac_stats	mac_stats;
 		struct ethtool_eth_ctrl_stats	ctrl_stats;
 		struct ethtool_rmon_stats	rmon_stats;
+		struct ethtool_rep_port_stats	rep_port_stats;
 	);
 	const struct ethtool_rmon_hist_range	*rmon_ranges;
 };
@@ -32,6 +33,7 @@  const char stats_std_names[__ETHTOOL_STATS_CNT][ETH_GSTRING_LEN] = {
 	[ETHTOOL_STATS_ETH_MAC]			= "eth-mac",
 	[ETHTOOL_STATS_ETH_CTRL]		= "eth-ctrl",
 	[ETHTOOL_STATS_RMON]			= "rmon",
+	[ETHTOOL_STATS_REP_PORT]		= "rep-port",
 };
 
 const char stats_eth_phy_names[__ETHTOOL_A_STATS_ETH_PHY_CNT][ETH_GSTRING_LEN] = {
@@ -76,6 +78,10 @@  const char stats_rmon_names[__ETHTOOL_A_STATS_RMON_CNT][ETH_GSTRING_LEN] = {
 	[ETHTOOL_A_STATS_RMON_JABBER]		= "etherStatsJabbers",
 };
 
+const char stats_rep_port_names[__ETHTOOL_A_STATS_REP_PORT_CNT][ETH_GSTRING_LEN] = {
+	[ETHTOOL_A_STATS_REP_PORT_OUT_OF_BUF]	= "out_of_buf",
+};
+
 const struct nla_policy ethnl_stats_get_policy[ETHTOOL_A_STATS_SRC + 1] = {
 	[ETHTOOL_A_STATS_HEADER]	=
 		NLA_POLICY_NESTED(ethnl_header_policy),
@@ -158,6 +164,15 @@  static int stats_prepare_data(const struct ethnl_req_info *req_base,
 	    dev->ethtool_ops->get_rmon_stats)
 		dev->ethtool_ops->get_rmon_stats(dev, &data->rmon_stats,
 						 &data->rmon_ranges);
+	if (test_bit(ETHTOOL_STATS_REP_PORT, req_info->stat_mask) &&
+	    dev->ethtool_ops->get_rep_port_stats) {
+		ret = dev->ethtool_ops->get_rep_port_stats(dev, &data->rep_port_stats,
+							   info->extack);
+		if (ret) {
+			ethnl_ops_complete(dev);
+			return ret;
+		}
+	}
 
 	ethnl_ops_complete(dev);
 	return 0;
@@ -194,6 +209,10 @@  static int stats_reply_size(const struct ethnl_req_info *req_base,
 			nla_total_size(4)) *	/* _A_STATS_GRP_HIST_BKT_HI */
 			ETHTOOL_RMON_HIST_MAX * 2;
 	}
+	if (test_bit(ETHTOOL_STATS_REP_PORT, req_info->stat_mask)) {
+		n_stats += sizeof(struct ethtool_rep_port_stats) / sizeof(u64);
+		n_grps++;
+	}
 
 	len += n_grps * (nla_total_size(0) + /* _A_STATS_GRP */
 			 nla_total_size(4) + /* _A_STATS_GRP_ID */
@@ -370,6 +389,15 @@  static int stats_put_rmon_stats(struct sk_buff *skb,
 	return 0;
 }
 
+static int stats_put_rep_port_stats(struct sk_buff *skb,
+				    const struct stats_reply_data *data)
+{
+	if (stat_put(skb, ETHTOOL_A_STATS_REP_PORT_OUT_OF_BUF, data->rep_port_stats.out_of_buf))
+		return -EMSGSIZE;
+
+	return 0;
+}
+
 static int stats_put_stats(struct sk_buff *skb,
 			   const struct stats_reply_data *data,
 			   u32 id, u32 ss_id,
@@ -423,6 +451,9 @@  static int stats_fill_reply(struct sk_buff *skb,
 	if (!ret && test_bit(ETHTOOL_STATS_RMON, req_info->stat_mask))
 		ret = stats_put_stats(skb, data, ETHTOOL_STATS_RMON,
 				      ETH_SS_STATS_RMON, stats_put_rmon_stats);
+	if (!ret && test_bit(ETHTOOL_STATS_REP_PORT, req_info->stat_mask))
+		ret = stats_put_stats(skb, data, ETHTOOL_STATS_REP_PORT,
+				      ETH_SS_STATS_REP_PORT, stats_put_rep_port_stats);
 
 	return ret;
 }
diff --git a/net/ethtool/strset.c b/net/ethtool/strset.c
index c678b484a079..01d7a6fd9471 100644
--- a/net/ethtool/strset.c
+++ b/net/ethtool/strset.c
@@ -105,6 +105,11 @@  static const struct strset_info info_template[] = {
 		.count		= __ETHTOOL_A_STATS_RMON_CNT,
 		.strings	= stats_rmon_names,
 	},
+	[ETH_SS_STATS_REP_PORT] = {
+		.per_dev	= false,
+		.count		= __ETHTOOL_A_STATS_REP_PORT_CNT,
+		.strings	= stats_rep_port_names,
+	},
 };
 
 struct strset_req_info {