[PATCH ethtool-next v2 0/6] pause frame stats

[PATCH ethtool-next v2 0/6] pause frame stats

[Jakub Kicinski]

Hi!

This set adds support for pause frame statistics.

First pause frame info is extended to support --json.

Pause stats are first of this kind of statistics so add
support for a new flag (--include-statistics).

Next add support for dumping policy to check if the statistics
flag is supported for a given subcommand.

Last but not least - display statistics.

Jakub Kicinski (6):
  update UAPI header copies
  pause: add --json support
  separate FLAGS out in -h
  add support for stats in subcommands
  netlink: use policy dumping to check if stats flag is supported
  pause: add support for dumping statistics

 ethtool.8.in           |   7 ++
 ethtool.c              |  17 ++++-
 internal.h             |   1 +
 netlink/coalesce.c     |   6 +-
 netlink/msgbuff.h      |   6 ++
 netlink/netlink.c      | 151 +++++++++++++++++++++++++++++++++++++++++
 netlink/netlink.h      |  23 +++++--
 netlink/pause.c        | 111 ++++++++++++++++++++++++++----
 uapi/linux/genetlink.h |  11 +++
 uapi/linux/netlink.h   |   2 +
 10 files changed, 311 insertions(+), 24 deletions(-)

-- 
2.26.2

[PATCH ethtool-next v2 1/6] update UAPI header copies

[Jakub Kicinski]

Update to kernel commit 9faebeb2d800.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 uapi/linux/genetlink.h | 11 +++++++++++
 uapi/linux/netlink.h   |  2 ++
 2 files changed, 13 insertions(+)

diff --git a/uapi/linux/genetlink.h b/uapi/linux/genetlink.h
index 7c6c390c48ee..9fa720ee87ae 100644
--- a/uapi/linux/genetlink.h
+++ b/uapi/linux/genetlink.h
@@ -64,6 +64,8 @@ enum {
 	CTRL_ATTR_OPS,
 	CTRL_ATTR_MCAST_GROUPS,
 	CTRL_ATTR_POLICY,
+	CTRL_ATTR_OP_POLICY,
+	CTRL_ATTR_OP,
 	__CTRL_ATTR_MAX,
 };
 
@@ -85,6 +87,15 @@ enum {
 	__CTRL_ATTR_MCAST_GRP_MAX,
 };
 
+enum {
+	CTRL_ATTR_POLICY_UNSPEC,
+	CTRL_ATTR_POLICY_DO,
+	CTRL_ATTR_POLICY_DUMP,
+
+	__CTRL_ATTR_POLICY_DUMP_MAX,
+	CTRL_ATTR_POLICY_DUMP_MAX = __CTRL_ATTR_POLICY_DUMP_MAX - 1
+};
+
 #define CTRL_ATTR_MCAST_GRP_MAX (__CTRL_ATTR_MCAST_GRP_MAX - 1)
 
 
diff --git a/uapi/linux/netlink.h b/uapi/linux/netlink.h
index 695c88e3c29d..f774920506b3 100644
--- a/uapi/linux/netlink.h
+++ b/uapi/linux/netlink.h
@@ -327,6 +327,7 @@ enum netlink_attribute_type {
  *	the index, if limited inside the nesting (U32)
  * @NL_POLICY_TYPE_ATTR_BITFIELD32_MASK: valid mask for the
  *	bitfield32 type (U32)
+ * @NL_POLICY_TYPE_ATTR_MASK: mask of valid bits for unsigned integers (U64)
  * @NL_POLICY_TYPE_ATTR_PAD: pad attribute for 64-bit alignment
  */
 enum netlink_policy_type_attr {
@@ -342,6 +343,7 @@ enum netlink_policy_type_attr {
 	NL_POLICY_TYPE_ATTR_POLICY_MAXTYPE,
 	NL_POLICY_TYPE_ATTR_BITFIELD32_MASK,
 	NL_POLICY_TYPE_ATTR_PAD,
+	NL_POLICY_TYPE_ATTR_MASK,
 
 	/* keep last */
 	__NL_POLICY_TYPE_ATTR_MAX,
-- 
2.26.2

[PATCH ethtool-next v2 2/6] pause: add --json support

[Jakub Kicinski]

No change in normal text output:

 # ./ethtool  -a eth0
Pause parameters for eth0:
Autonegotiate:	on
RX:		on
TX:		on
RX negotiated: on
TX negotiated: on

JSON:

 # ./ethtool --json -a eth0
[ {
        "ifname": "eth0",
        "autonegotiate": true,
        "rx": true,
        "tx": true,
        "negotiated": {
            "rx": true,
            "tx": true
        }
    } ]

v2:
 - restructure show_bool() so we can use its logic for show_bool_val()

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 netlink/coalesce.c |  6 +++---
 netlink/netlink.h  | 21 ++++++++++++++++-----
 netlink/pause.c    | 44 ++++++++++++++++++++++++++++++++------------
 3 files changed, 51 insertions(+), 20 deletions(-)

diff --git a/netlink/coalesce.c b/netlink/coalesce.c
index 65f75cf9a8dd..07a92d04b7a1 100644
--- a/netlink/coalesce.c
+++ b/netlink/coalesce.c
@@ -36,9 +36,9 @@ int coalesce_reply_cb(const struct nlmsghdr *nlhdr, void *data)
 	if (silent)
 		putchar('\n');
 	printf("Coalesce parameters for %s:\n", nlctx->devname);
-	printf("Adaptive RX: %s  TX: %s\n",
-	       u8_to_bool(tb[ETHTOOL_A_COALESCE_USE_ADAPTIVE_RX]),
-	       u8_to_bool(tb[ETHTOOL_A_COALESCE_USE_ADAPTIVE_TX]));
+	show_bool("rx", "Adaptive RX: %s  ",
+		  tb[ETHTOOL_A_COALESCE_USE_ADAPTIVE_RX]);
+	show_bool("tx", "TX: %s\n", tb[ETHTOOL_A_COALESCE_USE_ADAPTIVE_TX]);
 	show_u32(tb[ETHTOOL_A_COALESCE_STATS_BLOCK_USECS],
 		 "stats-block-usecs: ");
 	show_u32(tb[ETHTOOL_A_COALESCE_RATE_SAMPLE_INTERVAL],
diff --git a/netlink/netlink.h b/netlink/netlink.h
index dd4a02bcc916..1012e8e32cd8 100644
--- a/netlink/netlink.h
+++ b/netlink/netlink.h
@@ -98,17 +98,28 @@ static inline void show_u32(const struct nlattr *attr, const char *label)
 		printf("%sn/a\n", label);
 }
 
-static inline const char *u8_to_bool(const struct nlattr *attr)
+static inline const char *u8_to_bool(const uint8_t *val)
 {
-	if (attr)
-		return mnl_attr_get_u8(attr) ? "on" : "off";
+	if (val)
+		return *val ? "on" : "off";
 	else
 		return "n/a";
 }
 
-static inline void show_bool(const struct nlattr *attr, const char *label)
+static inline void show_bool_val(const char *key, const char *fmt, uint8_t *val)
+{
+	if (is_json_context()) {
+		if (val)
+			print_bool(PRINT_JSON, key, NULL, val);
+	} else {
+		print_string(PRINT_FP, NULL, fmt, u8_to_bool(val));
+	}
+}
+
+static inline void show_bool(const char *key, const char *fmt,
+			     const struct nlattr *attr)
 {
-	printf("%s%s\n", label, u8_to_bool(attr));
+	show_bool_val(key, fmt, attr ? mnl_attr_get_payload(attr) : NULL);
 }
 
 /* misc */
diff --git a/netlink/pause.c b/netlink/pause.c
index 7b6b3a1d2c10..c960c82cba5f 100644
--- a/netlink/pause.c
+++ b/netlink/pause.c
@@ -40,8 +40,8 @@ static int pause_autoneg_cb(const struct nlmsghdr *nlhdr, void *data)
 	struct pause_autoneg_status ours = {};
 	struct pause_autoneg_status peer = {};
 	struct nl_context *nlctx = data;
-	bool rx_status = false;
-	bool tx_status = false;
+	uint8_t rx_status = false;
+	uint8_t tx_status = false;
 	bool silent;
 	int err_ret;
 	int ret;
@@ -72,8 +72,11 @@ static int pause_autoneg_cb(const struct nlmsghdr *nlhdr, void *data)
 		else if (peer.pause)
 			tx_status = true;
 	}
-	printf("RX negotiated: %s\nTX negotiated: %s\n",
-	       rx_status ? "on" : "off", tx_status ? "on" : "off");
+
+	open_json_object("negotiated");
+	show_bool_val("rx", "RX negotiated: %s\n", &rx_status);
+	show_bool_val("tx", "TX negotiated: %s\n", &tx_status);
+	close_json_object();
 
 	return MNL_CB_OK;
 }
@@ -121,21 +124,34 @@ int pause_reply_cb(const struct nlmsghdr *nlhdr, void *data)
 		return err_ret;
 
 	if (silent)
-		putchar('\n');
-	printf("Pause parameters for %s:\n", nlctx->devname);
-	show_bool(tb[ETHTOOL_A_PAUSE_AUTONEG], "Autonegotiate:\t");
-	show_bool(tb[ETHTOOL_A_PAUSE_RX], "RX:\t\t");
-	show_bool(tb[ETHTOOL_A_PAUSE_TX], "TX:\t\t");
+		print_nl();
+
+	open_json_object(NULL);
+
+	print_string(PRINT_ANY, "ifname", "Pause parameters for %s:\n",
+		     nlctx->devname);
+
+	show_bool("autonegotiate", "Autonegotiate:\t%s\n",
+		  tb[ETHTOOL_A_PAUSE_AUTONEG]);
+	show_bool("rx", "RX:\t\t%s\n", tb[ETHTOOL_A_PAUSE_RX]);
+	show_bool("tx", "TX:\t\t%s\n", tb[ETHTOOL_A_PAUSE_TX]);
+
 	if (!nlctx->is_monitor && tb[ETHTOOL_A_PAUSE_AUTONEG] &&
 	    mnl_attr_get_u8(tb[ETHTOOL_A_PAUSE_AUTONEG])) {
 		ret = show_pause_autoneg_status(nlctx);
 		if (ret < 0)
-			return err_ret;
+			goto err_close_dev;
 	}
 	if (!silent)
-		putchar('\n');
+		print_nl();
+
+	close_json_object();
 
 	return MNL_CB_OK;
+
+err_close_dev:
+	close_json_object();
+	return err_ret;
 }
 
 int nl_gpause(struct cmd_context *ctx)
@@ -156,7 +172,11 @@ int nl_gpause(struct cmd_context *ctx)
 				      ETHTOOL_A_PAUSE_HEADER, 0);
 	if (ret < 0)
 		return ret;
-	return nlsock_send_get_request(nlsk, pause_reply_cb);
+
+	new_json_obj(ctx->json);
+	ret = nlsock_send_get_request(nlsk, pause_reply_cb);
+	delete_json_obj();
+	return ret;
 }
 
 /* PAUSE_SET */
-- 
2.26.2

[PATCH ethtool-next v2 3/6] separate FLAGS out in -h

[Jakub Kicinski]

Help output is quite crowded already with every command
being prefixed by --debug and --json options, and we're
about to add a third one.

Add an indirection.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 ethtool.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/ethtool.c b/ethtool.c
index 2a7de97c51bb..403616bb7fa0 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -6015,10 +6015,10 @@ static int show_usage(struct cmd_context *ctx __maybe_unused)
 	fprintf(stdout, PACKAGE " version " VERSION "\n");
 	fprintf(stdout,
 		"Usage:\n"
-		"        ethtool [ --debug MASK ][ --json ] DEVNAME\t"
+		"        ethtool [ FLAGS ] DEVNAME\t"
 		"Display standard information about device\n");
 	for (i = 0; args[i].opts; i++) {
-		fputs("        ethtool [ --debug MASK ][ --json ] ", stdout);
+		fputs("        ethtool [ FLAGS ] ", stdout);
 		fprintf(stdout, "%s %s\t%s\n",
 			args[i].opts,
 			args[i].no_dev ? "\t" : "DEVNAME",
@@ -6027,7 +6027,10 @@ static int show_usage(struct cmd_context *ctx __maybe_unused)
 			fputs(args[i].xhelp, stdout);
 	}
 	nl_monitor_usage();
-	fprintf(stdout, "Not all options support JSON output\n");
+	fprintf(stdout, "\n");
+	fprintf(stdout, "FLAGS:\n");
+	fprintf(stdout, "	--debug MASK	turn on debugging messages\n");
+	fprintf(stdout, "	--json		enable JSON output format (not supported by all commands)\n");
 
 	return 0;
 }
-- 
2.26.2

[PATCH ethtool-next v2 4/6] add support for stats in subcommands

[Jakub Kicinski]

Add new parameters (-I | --include-statistics) which will
control requesting statistic dumps from the kernel.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 ethtool.8.in | 7 +++++++
 ethtool.c    | 8 ++++++++
 internal.h   | 1 +
 3 files changed, 16 insertions(+)

diff --git a/ethtool.8.in b/ethtool.8.in
index f70edb5d9d39..866b4e940dc0 100644
--- a/ethtool.8.in
+++ b/ethtool.8.in
@@ -140,6 +140,9 @@ ethtool \- query or control network driver and hardware settings
 .B ethtool [--json]
 .I args
 .HP
+.B ethtool [-I | --include-statistics]
+.I args
+.HP
 .B ethtool \-\-monitor
 [
 .I command
@@ -499,6 +502,10 @@ Output results in JavaScript Object Notation (JSON). Only a subset of
 options support this. Those which do not will continue to output
 plain text in the presence of this option.
 .TP
+.B \-I \-\-include\-statistics
+Include command-related statistics in the output. This option allows
+displaying relevant device statistics for selected get commands.
+.TP
 .B \-a \-\-show\-pause
 Queries the specified Ethernet device for pause parameter information.
 .TP
diff --git a/ethtool.c b/ethtool.c
index 403616bb7fa0..1d9067e774af 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -6031,6 +6031,7 @@ static int show_usage(struct cmd_context *ctx __maybe_unused)
 	fprintf(stdout, "FLAGS:\n");
 	fprintf(stdout, "	--debug MASK	turn on debugging messages\n");
 	fprintf(stdout, "	--json		enable JSON output format (not supported by all commands)\n");
+	fprintf(stdout, "	-I|--include-statistics		request device statistics related to the command (not supported by all commands)\n");
 
 	return 0;
 }
@@ -6291,6 +6292,13 @@ int main(int argc, char **argp)
 			argc -= 1;
 			continue;
 		}
+		if (*argp && (!strcmp(*argp, "--include-statistics") ||
+			      !strcmp(*argp, "-I"))) {
+			ctx.show_stats = true;
+			argp += 1;
+			argc -= 1;
+			continue;
+		}
 		break;
 	}
 	if (*argp && !strcmp(*argp, "--monitor")) {
diff --git a/internal.h b/internal.h
index 935ebac3ca2e..27da8eac57bb 100644
--- a/internal.h
+++ b/internal.h
@@ -225,6 +225,7 @@ struct cmd_context {
 	char **argp;		/* arguments to the sub-command */
 	unsigned long debug;	/* debugging mask */
 	bool json;		/* Output JSON, if supported */
+	bool show_stats;	/* include command-specific stats */
 #ifdef ETHTOOL_ENABLE_NETLINK
 	struct nl_context *nlctx;	/* netlink context (opaque) */
 #endif
-- 
2.26.2

[PATCH ethtool-next v2 5/6] netlink: use policy dumping to check if stats flag is supported

[Jakub Kicinski]

Older kernels don't support statistics, to avoid retries
make use of netlink policy dumps to figure out which
flags kernel actually supports.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 netlink/msgbuff.h |   6 ++
 netlink/netlink.c | 151 ++++++++++++++++++++++++++++++++++++++++++++++
 netlink/netlink.h |   2 +
 3 files changed, 159 insertions(+)

diff --git a/netlink/msgbuff.h b/netlink/msgbuff.h
index 24b99c5a28d7..7d6731fc24a3 100644
--- a/netlink/msgbuff.h
+++ b/netlink/msgbuff.h
@@ -81,6 +81,12 @@ static inline bool ethnla_put_u32(struct nl_msg_buff *msgbuff, uint16_t type,
 	return ethnla_put(msgbuff, type, sizeof(uint32_t), &data);
 }
 
+static inline bool ethnla_put_u16(struct nl_msg_buff *msgbuff, uint16_t type,
+				  uint16_t data)
+{
+	return ethnla_put(msgbuff, type, sizeof(uint16_t), &data);
+}
+
 static inline bool ethnla_put_u8(struct nl_msg_buff *msgbuff, uint16_t type,
 				 uint8_t data)
 {
diff --git a/netlink/netlink.c b/netlink/netlink.c
index e42d57076a4b..f79da12d3eee 100644
--- a/netlink/netlink.c
+++ b/netlink/netlink.c
@@ -135,6 +135,157 @@ bool netlink_cmd_check(struct cmd_context *ctx, unsigned int cmd,
 	return !(nlctx->ops_flags[cmd] & cap);
 }
 
+struct ethtool_op_policy_query_ctx {
+	struct nl_context *nlctx;
+	unsigned int op;
+	unsigned int op_hdr_attr;
+
+	bool op_policy_found;
+	bool hdr_policy_found;
+	unsigned int op_policy_idx;
+	unsigned int hdr_policy_idx;
+	uint64_t flag_mask;
+};
+
+static int family_policy_find_op(struct ethtool_op_policy_query_ctx *ctx,
+				 const struct nlattr *op_policy)
+{
+	const struct nlattr *attr;
+	unsigned int type;
+	int ret;
+
+	type = ctx->nlctx->is_dump ?
+		CTRL_ATTR_POLICY_DUMP : CTRL_ATTR_POLICY_DO;
+
+	mnl_attr_for_each_nested(attr, op_policy) {
+		const struct nlattr *tb[CTRL_ATTR_POLICY_DUMP_MAX + 1] = {};
+		DECLARE_ATTR_TB_INFO(tb);
+
+		if (mnl_attr_get_type(attr) != ctx->op)
+			continue;
+
+		ret = mnl_attr_parse_nested(attr, attr_cb, &tb_info);
+		if (ret < 0)
+			return ret;
+
+		if (!tb[type])
+			continue;
+
+		ctx->op_policy_found = true;
+		ctx->op_policy_idx = mnl_attr_get_u32(tb[type]);
+		break;
+	}
+
+	return 0;
+}
+
+static int family_policy_cb(const struct nlmsghdr *nlhdr, void *data)
+{
+	const struct nlattr *tba[NL_POLICY_TYPE_ATTR_MAX + 1] = {};
+	DECLARE_ATTR_TB_INFO(tba);
+	const struct nlattr *tb[CTRL_ATTR_MAX + 1] = {};
+	DECLARE_ATTR_TB_INFO(tb);
+	const struct nlattr *policy_attr, *attr_attr, *attr;
+	struct ethtool_op_policy_query_ctx *ctx = data;
+	unsigned int attr_idx, policy_idx;
+	int ret;
+
+	ret = mnl_attr_parse(nlhdr, GENL_HDRLEN, attr_cb, &tb_info);
+	if (ret < 0)
+		return MNL_CB_ERROR;
+
+	if (!ctx->op_policy_found) {
+		if (!tb[CTRL_ATTR_OP_POLICY]) {
+			fprintf(stderr, "Error: op policy map not present\n");
+			return MNL_CB_ERROR;
+		}
+		ret = family_policy_find_op(ctx, tb[CTRL_ATTR_OP_POLICY]);
+		return ret < 0 ? MNL_CB_ERROR : MNL_CB_OK;
+	}
+
+	if (!tb[CTRL_ATTR_POLICY])
+		return MNL_CB_OK;
+
+	policy_attr = mnl_attr_get_payload(tb[CTRL_ATTR_POLICY]);
+	policy_idx = mnl_attr_get_type(policy_attr);
+	attr_attr = mnl_attr_get_payload(policy_attr);
+	attr_idx = mnl_attr_get_type(attr_attr);
+
+	ret = mnl_attr_parse_nested(attr_attr, attr_cb, &tba_info);
+	if (ret < 0)
+		return MNL_CB_ERROR;
+
+	if (policy_idx == ctx->op_policy_idx && attr_idx == ctx->op_hdr_attr) {
+		attr = tba[NL_POLICY_TYPE_ATTR_POLICY_IDX];
+		if (!attr) {
+			fprintf(stderr,	"Error: no policy index in what was expected to be ethtool header attribute\n");
+			return MNL_CB_ERROR;
+		}
+		ctx->hdr_policy_found = true;
+		ctx->hdr_policy_idx = mnl_attr_get_u32(attr);
+	}
+
+	if (ctx->hdr_policy_found && ctx->hdr_policy_idx == policy_idx &&
+	    attr_idx == ETHTOOL_A_HEADER_FLAGS) {
+		attr = tba[NL_POLICY_TYPE_ATTR_MASK];
+		if (!attr) {
+			fprintf(stderr,	"Error: validation mask not reported for ethtool header flags\n");
+			return MNL_CB_ERROR;
+		}
+
+		ctx->flag_mask = mnl_attr_get_u64(attr);
+	}
+
+	return MNL_CB_OK;
+}
+
+static int get_flags_policy(struct nl_context *nlctx, struct nl_socket *nlsk,
+			    unsigned int nlcmd, unsigned int hdrattr)
+{
+	struct nl_msg_buff *msgbuff = &nlsk->msgbuff;
+	struct ethtool_op_policy_query_ctx ctx;
+	int ret;
+
+	memset(&ctx, 0, sizeof(ctx));
+	ctx.nlctx = nlctx;
+	ctx.op = nlcmd;
+	ctx.op_hdr_attr = hdrattr;
+
+	ret = __msg_init(msgbuff, GENL_ID_CTRL, CTRL_CMD_GETPOLICY,
+			 NLM_F_REQUEST | NLM_F_ACK | NLM_F_DUMP, 1);
+	if (ret < 0)
+		return ret;
+	ret = -EMSGSIZE;
+	if (ethnla_put_u16(msgbuff, CTRL_ATTR_FAMILY_ID, nlctx->ethnl_fam))
+		return ret;
+	if (ethnla_put_u32(msgbuff, CTRL_ATTR_OP, nlcmd))
+		return ret;
+
+	nlsock_sendmsg(nlsk, NULL);
+	nlsock_process_reply(nlsk, family_policy_cb, &ctx);
+
+	ret = ctx.flag_mask;
+	return ret;
+}
+
+u32 get_stats_flag(struct nl_context *nlctx, unsigned int nlcmd,
+		   unsigned int hdrattr)
+{
+	int ret;
+
+	if (!nlctx->ctx->show_stats)
+		return 0;
+	if (nlcmd > ETHTOOL_MSG_USER_MAX ||
+	    !(nlctx->ops_flags[nlcmd] & GENL_CMD_CAP_HASPOL))
+		return 0;
+
+	ret = get_flags_policy(nlctx, nlctx->ethnl_socket, nlcmd, hdrattr);
+	if (ret < 0)
+		return 0;
+
+	return ret & ETHTOOL_FLAG_STATS;
+}
+
 /* initialization */
 
 static int genl_read_ops(struct nl_context *nlctx,
diff --git a/netlink/netlink.h b/netlink/netlink.h
index 1012e8e32cd8..799f2556cb31 100644
--- a/netlink/netlink.h
+++ b/netlink/netlink.h
@@ -66,6 +66,8 @@ bool netlink_cmd_check(struct cmd_context *ctx, unsigned int cmd,
 		       bool allow_wildcard);
 const char *get_dev_name(const struct nlattr *nest);
 int get_dev_info(const struct nlattr *nest, int *ifindex, char *ifname);
+u32 get_stats_flag(struct nl_context *nlctx, unsigned int nlcmd,
+		   unsigned int hdrattr);
 
 int linkmodes_reply_cb(const struct nlmsghdr *nlhdr, void *data);
 int linkinfo_reply_cb(const struct nlmsghdr *nlhdr, void *data);
-- 
2.26.2

Re: [PATCH ethtool-next v2 5/6] netlink: use policy dumping to check if stats flag is supported

[Ido Schimmel]

On Tue, Oct 06, 2020 at 08:04:24AM -0700, Jakub Kicinski wrote:
> Older kernels don't support statistics, to avoid retries
> make use of netlink policy dumps to figure out which
> flags kernel actually supports.

Thanks for working on this, Jakub.

Michal, should I try something similar for the legacy flag we have been
discussing earlier this week [1]?

[1] https://lore.kernel.org/netdev/20201005074600.xkbomksbbuliuyft@lion.mk-sys.cz/#t

Re: [PATCH ethtool-next v2 5/6] netlink: use policy dumping to check if stats flag is supported

[Michal Kubecek]

On Tue, Oct 06, 2020 at 10:39:13PM +0300, Ido Schimmel wrote:
> On Tue, Oct 06, 2020 at 08:04:24AM -0700, Jakub Kicinski wrote:
> > Older kernels don't support statistics, to avoid retries
> > make use of netlink policy dumps to figure out which
> > flags kernel actually supports.
> 
> Thanks for working on this, Jakub.
> 
> Michal, should I try something similar for the legacy flag we have been
> discussing earlier this week [1]?
> 
> [1] https://lore.kernel.org/netdev/20201005074600.xkbomksbbuliuyft@lion.mk-sys.cz/#t

Yes, using the policy dump will be the way to go. I wanted to come with
something myself but we have a virtual conference this week and today
an urgent request took the spare time I had in the morning.

Michal

Re: [PATCH ethtool-next v2 5/6] netlink: use policy dumping to check if stats flag is supported

[Michal Kubecek]

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

On Tue, Oct 06, 2020 at 08:04:24AM -0700, Jakub Kicinski wrote:
> Older kernels don't support statistics, to avoid retries
> make use of netlink policy dumps to figure out which
> flags kernel actually supports.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
[...]
> +static int family_policy_cb(const struct nlmsghdr *nlhdr, void *data)
> +{
> +	const struct nlattr *tba[NL_POLICY_TYPE_ATTR_MAX + 1] = {};
> +	DECLARE_ATTR_TB_INFO(tba);
> +	const struct nlattr *tb[CTRL_ATTR_MAX + 1] = {};
> +	DECLARE_ATTR_TB_INFO(tb);
> +	const struct nlattr *policy_attr, *attr_attr, *attr;
> +	struct ethtool_op_policy_query_ctx *ctx = data;

I would rather use a different name for this variable as "ctx" is used
for struct cmd_context everywhere else.

> +	unsigned int attr_idx, policy_idx;
> +	int ret;

[...]
> +static int get_flags_policy(struct nl_context *nlctx, struct nl_socket *nlsk,
> +			    unsigned int nlcmd, unsigned int hdrattr)
> +{
> +	struct nl_msg_buff *msgbuff = &nlsk->msgbuff;
> +	struct ethtool_op_policy_query_ctx ctx;

Same here.

> +	int ret;
> +
> +	memset(&ctx, 0, sizeof(ctx));
> +	ctx.nlctx = nlctx;
> +	ctx.op = nlcmd;
> +	ctx.op_hdr_attr = hdrattr;
> +
> +	ret = __msg_init(msgbuff, GENL_ID_CTRL, CTRL_CMD_GETPOLICY,
> +			 NLM_F_REQUEST | NLM_F_ACK | NLM_F_DUMP, 1);
> +	if (ret < 0)
> +		return ret;
> +	ret = -EMSGSIZE;
> +	if (ethnla_put_u16(msgbuff, CTRL_ATTR_FAMILY_ID, nlctx->ethnl_fam))
> +		return ret;
> +	if (ethnla_put_u32(msgbuff, CTRL_ATTR_OP, nlcmd))
> +		return ret;
> +
> +	nlsock_sendmsg(nlsk, NULL);
> +	nlsock_process_reply(nlsk, family_policy_cb, &ctx);
> +
> +	ret = ctx.flag_mask;
> +	return ret;
> +}

The return value is assigned either a negative error code or a u32 flag
mask. If we run this code on a kernel supporting 32 flags, flag in bit
31 would collide with sign and successfully retrieved flag mask would be
interpreted as an error by caller.

Other than this, the series looks good to me.

Michal

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH ethtool-next v2 6/6] pause: add support for dumping statistics

[Jakub Kicinski]

Add support for requesting pause frame stats from the kernel.

 # ./ethtool -I -a eth0
Pause parameters for eth0:
Autonegotiate:	on
RX:		on
TX:		on
Statistics:
  tx_pause_frames: 1
  rx_pause_frames: 1

 # ./ethtool -I --json -a eth0
[ {
        "ifname": "eth0",
        "autonegotiate": true,
        "rx": true,
        "tx": true,
        "statistics": {
            "tx_pause_frames": 1,
            "rx_pause_frames": 1
        }
    } ]

v2: - correct print format for u64

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

diff --git a/netlink/pause.c b/netlink/pause.c
index c960c82cba5f..9bc9a301821f 100644
--- a/netlink/pause.c
+++ b/netlink/pause.c
@@ -5,6 +5,7 @@
  */
 
 #include <errno.h>
+#include <inttypes.h>
 #include <string.h>
 #include <stdio.h>
 
@@ -105,6 +106,62 @@ static int show_pause_autoneg_status(struct nl_context *nlctx)
 	return ret;
 }
 
+static int show_pause_stats(const struct nlattr *nest)
+{
+	const struct nlattr *tb[ETHTOOL_A_PAUSE_STAT_MAX + 1] = {};
+	DECLARE_ATTR_TB_INFO(tb);
+	static const struct {
+		unsigned int attr;
+		char *name;
+	} stats[] = {
+		{ ETHTOOL_A_PAUSE_STAT_TX_FRAMES, "tx_pause_frames" },
+		{ ETHTOOL_A_PAUSE_STAT_RX_FRAMES, "rx_pause_frames" },
+	};
+	bool header = false;
+	unsigned int i;
+	size_t n;
+	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++) {
+		char fmt[32];
+
+		if (!tb[stats[i].attr])
+			continue;
+
+		if (!header && !is_json_context()) {
+			printf("Statistics:\n");
+			header = true;
+		}
+
+		if (mnl_attr_validate(tb[stats[i].attr], MNL_TYPE_U64)) {
+			fprintf(stderr, "malformed netlink message (statistic)\n");
+			goto err_close_stats;
+		}
+
+		n = snprintf(fmt, sizeof(fmt), "  %s: %%" PRIu64 "\n",
+			     stats[i].name);
+		if (n >= sizeof(fmt)) {
+			fprintf(stderr, "internal error - malformed label\n");
+			goto err_close_stats;
+		}
+
+		print_u64(PRINT_ANY, stats[i].name, fmt,
+			  mnl_attr_get_u64(tb[stats[i].attr]));
+	}
+	close_json_object();
+
+	return 0;
+
+err_close_stats:
+	close_json_object();
+	return -1;
+}
+
 int pause_reply_cb(const struct nlmsghdr *nlhdr, void *data)
 {
 	const struct nlattr *tb[ETHTOOL_A_PAUSE_MAX + 1] = {};
@@ -142,6 +199,11 @@ int pause_reply_cb(const struct nlmsghdr *nlhdr, void *data)
 		if (ret < 0)
 			goto err_close_dev;
 	}
+	if (tb[ETHTOOL_A_PAUSE_STATS]) {
+		ret = show_pause_stats(tb[ETHTOOL_A_PAUSE_STATS]);
+		if (ret < 0)
+			goto err_close_dev;
+	}
 	if (!silent)
 		print_nl();
 
@@ -158,6 +220,7 @@ int nl_gpause(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_PAUSE_GET, true))
@@ -168,8 +231,10 @@ int nl_gpause(struct cmd_context *ctx)
 		return 1;
 	}
 
+	flags = get_stats_flag(nlctx, ETHTOOL_MSG_PAUSE_GET,
+			       ETHTOOL_A_PAUSE_HEADER);
 	ret = nlsock_prep_get_request(nlsk, ETHTOOL_MSG_PAUSE_GET,
-				      ETHTOOL_A_PAUSE_HEADER, 0);
+				      ETHTOOL_A_PAUSE_HEADER, flags);
 	if (ret < 0)
 		return ret;
 
-- 
2.26.2