[PATCH ethtool-next v4 0/2] add netlink support for rss get

[PATCH ethtool-next v4 0/2] add netlink support for rss get

[Sudheer Mogilappagari]

These patches add netlink based handler to fetch RSS information
using "ethtool -x <eth> [context %d]" command.

Output without --json option
$ethtool -x eno2
RX flow hash indirection table for eno2 with 8 RX ring(s):
    0:      0     0     0     0     0     0     0     0
    8:      1     1     1     1     1     1     1     1
   ...skip similar lines...
  120:      7     7     7     7     7     7     7     7
RSS hash key:
be:c3:13:a6:59:9a:c3:c5:d8:60:75:2b:4c:b2:12:cc:5c:4e:34:
8a:f9:ab:16:c7:19:5d:ab:1d:b5:c1:c7:57:c7:a2:e1:2b:e3:ea:
02:60:88:8e:96:ef:2d:64:d2:de:2c:16:72:b6
RSS hash function:
    toeplitz: on
    xor: off
    crc32: off

Sample output with json option:
$ethtool --json -x eno2
[ {
    "ifname": "eno2",
    "rss-indirection-table": [ 0,0,0,0,0,0,0,0,1,1,1,1,1,1,1,
    ...skip similar lines...
    7,7,7,7,7,7,7,7 ],
    "rss-hash-key": [ 190,195,19,166,..]
    "rss-hash-function": {
            "toeplitz": true,
            "xor": false,
            "crc32": false
        }
    } ]

Signed-off-by: Sudheer Mogilappagari <sudheer.mogilappagari@intel.com>
---

v4: 
-Fixed hash function values in example output of commit message.

v3:
-Made hash key as an array of ints.
-Skip json field when not supported.
-Made hash function values as true/false instead of on/off
-Formatted key strings as per review comments. 

v2:
-Added json support
---
Sudheer Mogilappagari (2):
  Move code that print rss info into common file
  netlink: add netlink handler for get rss (-x)

 Makefile.am            |   2 +-
 common.c               |  43 ++++++++
 common.h               |   7 ++
 ethtool.c              |  46 +-------
 netlink/desc-ethtool.c |  11 ++
 netlink/extapi.h       |   2 +
 netlink/rss.c          | 241 +++++++++++++++++++++++++++++++++++++++++
 7 files changed, 311 insertions(+), 41 deletions(-)
 create mode 100644 netlink/rss.c

-- 
2.31.1

[PATCH ethtool-next v4 1/2] Move code that print rss info into common file

[Sudheer Mogilappagari]

Move functions that prints rss indirection table and hash key
into common file for use by both netlink and ioctl interface.
Changed function argument to be ring count instead of structure.

Signed-off-by: Sudheer Mogilappagari <sudheer.mogilappagari@intel.com>
---
 common.c  | 43 +++++++++++++++++++++++++++++++++++++++++++
 common.h  |  7 +++++++
 ethtool.c | 44 ++++----------------------------------------
 3 files changed, 54 insertions(+), 40 deletions(-)

diff --git a/common.c b/common.c
index 2630e73..0f2d8c0 100644
--- a/common.c
+++ b/common.c
@@ -173,3 +173,46 @@ void dump_mdix(u8 mdix, u8 mdix_ctrl)
 		fprintf(stdout, "\n");
 	}
 }
+
+void print_indir_table(struct cmd_context *ctx, u64 ring_count,
+		       u32 indir_size, u32 *indir)
+{
+	u32 i;
+
+	printf("RX flow hash indirection table for %s with %llu RX ring(s):\n",
+	       ctx->devname, ring_count);
+
+	if (!indir_size)
+		printf("Operation not supported\n");
+
+	for (i = 0; i < indir_size; i++) {
+		if (i % 8 == 0)
+			printf("%5u: ", i);
+		printf(" %5u", indir[i]);
+		if (i % 8 == 7 || i == indir_size - 1)
+			fputc('\n', stdout);
+	}
+}
+
+void print_rss_info(struct cmd_context *ctx, u64 ring_count,
+		    struct ethtool_rxfh *rss)
+{
+	u32 i, indir_bytes;
+	char *hkey;
+
+	print_indir_table(ctx, ring_count, rss->indir_size, rss->rss_config);
+
+	indir_bytes = rss->indir_size * sizeof(rss->rss_config[0]);
+	hkey = ((char *)rss->rss_config + indir_bytes);
+
+	printf("RSS hash key:\n");
+	if (!rss->key_size)
+		printf("Operation not supported\n");
+
+	for (i = 0; i < rss->key_size; i++) {
+		if (i == (rss->key_size - 1))
+			printf("%02x\n", (u8)hkey[i]);
+		else
+			printf("%02x:", (u8)hkey[i]);
+	}
+}
diff --git a/common.h b/common.h
index b74fdfa..8589714 100644
--- a/common.h
+++ b/common.h
@@ -8,6 +8,8 @@
 #define ETHTOOL_COMMON_H__
 
 #include "internal.h"
+#include <stddef.h>
+#include <errno.h>
 
 #define KERNEL_VERSION(a, b, c) (((a) << 16) + ((b) << 8) + (c))
 
@@ -41,5 +43,10 @@ extern const struct off_flag_def off_flag_def[OFF_FLAG_DEF_SIZE];
 void print_flags(const struct flag_info *info, unsigned int n_info, u32 value);
 int dump_wol(struct ethtool_wolinfo *wol);
 void dump_mdix(u8 mdix, u8 mdix_ctrl);
+void print_indir_table(struct cmd_context *ctx, u64 ring_count,
+		       u32 indir_size, u32 *indir);
+
+void print_rss_info(struct cmd_context *ctx, u64 ring_count,
+		    struct ethtool_rxfh *rss);
 
 #endif /* ETHTOOL_COMMON_H__ */
diff --git a/ethtool.c b/ethtool.c
index 60da8af..209dbd1 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -3900,27 +3900,6 @@ static int do_grxclass(struct cmd_context *ctx)
 	return err ? 1 : 0;
 }
 
-static void print_indir_table(struct cmd_context *ctx,
-			      struct ethtool_rxnfc *ring_count,
-			      u32 indir_size, u32 *indir)
-{
-	u32 i;
-
-	printf("RX flow hash indirection table for %s with %llu RX ring(s):\n",
-	       ctx->devname, ring_count->data);
-
-	if (!indir_size)
-		printf("Operation not supported\n");
-
-	for (i = 0; i < indir_size; i++) {
-		if (i % 8 == 0)
-			printf("%5u: ", i);
-		printf(" %5u", indir[i]);
-		if (i % 8 == 7 || i == indir_size - 1)
-			fputc('\n', stdout);
-	}
-}
-
 static int do_grxfhindir(struct cmd_context *ctx,
 			 struct ethtool_rxnfc *ring_count)
 {
@@ -3952,7 +3931,8 @@ static int do_grxfhindir(struct cmd_context *ctx,
 		return 1;
 	}
 
-	print_indir_table(ctx, ring_count, indir->size, indir->ring_index);
+	print_indir_table(ctx, ring_count->data, indir->size,
+			  indir->ring_index);
 
 	free(indir);
 	return 0;
@@ -3965,9 +3945,7 @@ static int do_grxfh(struct cmd_context *ctx)
 	struct ethtool_rxnfc ring_count;
 	struct ethtool_rxfh *rss;
 	u32 rss_context = 0;
-	u32 i, indir_bytes;
-	unsigned int arg_num = 0;
-	char *hkey;
+	unsigned int arg_num = 0, i;
 	int err;
 
 	while (arg_num < ctx->argc) {
@@ -4017,21 +3995,7 @@ static int do_grxfh(struct cmd_context *ctx)
 		return 1;
 	}
 
-	print_indir_table(ctx, &ring_count, rss->indir_size, rss->rss_config);
-
-	indir_bytes = rss->indir_size * sizeof(rss->rss_config[0]);
-	hkey = ((char *)rss->rss_config + indir_bytes);
-
-	printf("RSS hash key:\n");
-	if (!rss->key_size)
-		printf("Operation not supported\n");
-
-	for (i = 0; i < rss->key_size; i++) {
-		if (i == (rss->key_size - 1))
-			printf("%02x\n", (u8) hkey[i]);
-		else
-			printf("%02x:", (u8) hkey[i]);
-	}
+	print_rss_info(ctx, ring_count.data, rss);
 
 	printf("RSS hash function:\n");
 	if (!rss->hfunc) {
-- 
2.31.1

[PATCH ethtool-next v4 2/2] netlink: add netlink handler for get rss (-x)

[Sudheer Mogilappagari]

Add support for netlink based "ethtool -x <dev>" command using
ETHTOOL_MSG_RSS_GET netlink message. It implements same functionality
provided by traditional ETHTOOL_GRSSH subcommand. This displays RSS
table, hash key and hash function along with JSON support.

Sample output with json option:
$ethtool --json -x eno2
[ {
    "ifname": "eno2",
    "rss-indirection-table": [ 0,0,0,0,0,0,0,0,1,1,1,1,1,1,1,
    ...skip similar lines...
    7,7,7,7,7,7,7,7 ],
    "rss-hash-key": [ 190,195,19,166,..]
    "rss-hash-function": {
            "toeplitz": true,
            "xor": false,
            "crc32": false
        }
    } ]

Signed-off-by: Sudheer Mogilappagari <sudheer.mogilappagari@intel.com>
---
 Makefile.am            |   2 +-
 ethtool.c              |   2 +
 netlink/desc-ethtool.c |  11 ++
 netlink/extapi.h       |   2 +
 netlink/rss.c          | 241 +++++++++++++++++++++++++++++++++++++++++
 5 files changed, 257 insertions(+), 1 deletion(-)
 create mode 100644 netlink/rss.c

diff --git a/Makefile.am b/Makefile.am
index 663f40a..c3e7401 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -39,7 +39,7 @@ ethtool_SOURCES += \
 		  netlink/eee.c netlink/tsinfo.c netlink/fec.c \
 		  netlink/stats.c \
 		  netlink/desc-ethtool.c netlink/desc-genlctrl.c \
-		  netlink/module-eeprom.c netlink/module.c \
+		  netlink/module-eeprom.c netlink/module.c netlink/rss.c \
 		  netlink/desc-rtnl.c netlink/cable_test.c netlink/tunnels.c \
 		  uapi/linux/ethtool_netlink.h \
 		  uapi/linux/netlink.h uapi/linux/genetlink.h \
diff --git a/ethtool.c b/ethtool.c
index 209dbd1..5c16b10 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -5850,7 +5850,9 @@ static const struct option args[] = {
 	},
 	{
 		.opts	= "-x|--show-rxfh-indir|--show-rxfh",
+		.json	= true,
 		.func	= do_grxfh,
+		.nlfunc	= nl_grss,
 		.help	= "Show Rx flow hash indirection table and/or RSS hash key",
 		.xhelp	= "		[ context %d ]\n"
 	},
diff --git a/netlink/desc-ethtool.c b/netlink/desc-ethtool.c
index b3ac64d..ed83dae 100644
--- a/netlink/desc-ethtool.c
+++ b/netlink/desc-ethtool.c
@@ -442,6 +442,15 @@ static const struct pretty_nla_desc __pse_desc[] = {
 	NLATTR_DESC_U32_ENUM(ETHTOOL_A_PODL_PSE_PW_D_STATUS, pse_pw_d_status),
 };
 
+static const struct pretty_nla_desc __rss_desc[] = {
+	NLATTR_DESC_INVALID(ETHTOOL_A_MODULE_UNSPEC),
+	NLATTR_DESC_NESTED(ETHTOOL_A_MODULE_HEADER, header),
+	NLATTR_DESC_U32(ETHTOOL_A_RSS_CONTEXT),
+	NLATTR_DESC_U32(ETHTOOL_A_RSS_HFUNC),
+	NLATTR_DESC_BINARY(ETHTOOL_A_RSS_INDIR),
+	NLATTR_DESC_BINARY(ETHTOOL_A_RSS_HKEY),
+};
+
 const struct pretty_nlmsg_desc ethnl_umsg_desc[] = {
 	NLMSG_DESC_INVALID(ETHTOOL_MSG_USER_NONE),
 	NLMSG_DESC(ETHTOOL_MSG_STRSET_GET, strset),
@@ -481,6 +490,7 @@ const struct pretty_nlmsg_desc ethnl_umsg_desc[] = {
 	NLMSG_DESC(ETHTOOL_MSG_MODULE_SET, module),
 	NLMSG_DESC(ETHTOOL_MSG_PSE_GET, pse),
 	NLMSG_DESC(ETHTOOL_MSG_PSE_SET, pse),
+	NLMSG_DESC(ETHTOOL_MSG_RSS_GET, rss),
 };
 
 const unsigned int ethnl_umsg_n_desc = ARRAY_SIZE(ethnl_umsg_desc);
@@ -524,6 +534,7 @@ const struct pretty_nlmsg_desc ethnl_kmsg_desc[] = {
 	NLMSG_DESC(ETHTOOL_MSG_MODULE_GET_REPLY, module),
 	NLMSG_DESC(ETHTOOL_MSG_MODULE_NTF, module),
 	NLMSG_DESC(ETHTOOL_MSG_PSE_GET_REPLY, pse),
+	NLMSG_DESC(ETHTOOL_MSG_RSS_GET_REPLY, rss),
 };
 
 const unsigned int ethnl_kmsg_n_desc = ARRAY_SIZE(ethnl_kmsg_desc);
diff --git a/netlink/extapi.h b/netlink/extapi.h
index 1bb580a..9b6dd1a 100644
--- a/netlink/extapi.h
+++ b/netlink/extapi.h
@@ -47,6 +47,7 @@ int nl_gmodule(struct cmd_context *ctx);
 int nl_smodule(struct cmd_context *ctx);
 int nl_monitor(struct cmd_context *ctx);
 int nl_getmodule(struct cmd_context *ctx);
+int nl_grss(struct cmd_context *ctx);
 
 void nl_monitor_usage(void);
 
@@ -114,6 +115,7 @@ nl_get_eeprom_page(struct cmd_context *ctx __maybe_unused,
 #define nl_getmodule		NULL
 #define nl_gmodule		NULL
 #define nl_smodule		NULL
+#define nl_grss			NULL
 
 #endif /* ETHTOOL_ENABLE_NETLINK */
 
diff --git a/netlink/rss.c b/netlink/rss.c
new file mode 100644
index 0000000..f52c2e7
--- /dev/null
+++ b/netlink/rss.c
@@ -0,0 +1,241 @@
+/*
+ * rss.c - netlink implementation of RSS context commands
+ *
+ * Implementation of "ethtool -x <dev>"
+ */
+
+#include <errno.h>
+#include <string.h>
+#include <stdio.h>
+
+#include "../internal.h"
+#include "../common.h"
+#include "netlink.h"
+#include "strset.h"
+#include "parser.h"
+
+struct cb_args {
+	struct nl_context	*nlctx;
+	u32			num_rings;
+};
+
+void dump_json_rss_info(struct cmd_context *ctx, struct ethtool_rxfh *rss,
+			const struct stringset *hash_funcs)
+{
+	unsigned int indir_bytes = rss->indir_size * sizeof(rss->rss_config[0]);
+	unsigned int i;
+
+	open_json_object(NULL);
+	print_string(PRINT_JSON, "ifname", NULL, ctx->devname);
+	if (rss->indir_size) {
+		open_json_array("rss-indirection-table", NULL);
+		for (i = 0; i < rss->indir_size; i++)
+			print_uint(PRINT_JSON, NULL, NULL, rss->rss_config[i]);
+		close_json_array("\n");
+	}
+
+	if (rss->key_size) {
+		const char *hkey = ((char *)rss->rss_config + indir_bytes);
+
+		open_json_array("rss-hash-key", NULL);
+		for (i = 0; i < rss->key_size; i++)
+			print_uint(PRINT_JSON, NULL, NULL, (u8)hkey[i]);
+		close_json_array("\n");
+	}
+
+	if (rss->hfunc) {
+		open_json_object("rss-hash-function");
+		for (i = 0; i < get_count(hash_funcs); i++)
+			print_bool(PRINT_JSON, get_string(hash_funcs, i), NULL,
+				   (rss->hfunc & (1 << i)));
+		close_json_object();
+	}
+
+	close_json_object();
+}
+
+int rss_reply_cb(const struct nlmsghdr *nlhdr, void *data)
+{
+	const struct nlattr *tb[ETHTOOL_A_RSS_MAX + 1] = {};
+	unsigned int indir_bytes = 0, hkey_bytes = 0;
+	DECLARE_ATTR_TB_INFO(tb);
+	struct cb_args *args = data;
+	struct nl_context *nlctx =  args->nlctx;
+	const struct stringset *hash_funcs;
+	u32 rss_config_size, rss_hfunc;
+	const char *indir_table, *hkey;
+
+	struct ethtool_rxfh *rss;
+	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_RSS_HEADER]);
+	if (!dev_ok(nlctx))
+		return err_ret;
+
+	if (silent)
+		putchar('\n');
+
+	rss_hfunc = mnl_attr_get_u32(tb[ETHTOOL_A_RSS_HFUNC]);
+
+	indir_bytes = mnl_attr_get_payload_len(tb[ETHTOOL_A_RSS_INDIR]);
+	indir_table = mnl_attr_get_str(tb[ETHTOOL_A_RSS_INDIR]);
+
+	hkey_bytes = mnl_attr_get_payload_len(tb[ETHTOOL_A_RSS_HKEY]);
+	hkey = mnl_attr_get_str(tb[ETHTOOL_A_RSS_HKEY]);
+
+	rss_config_size = indir_bytes + hkey_bytes;
+
+	rss = calloc(1, sizeof(*rss) + rss_config_size);
+	if (!rss) {
+		perror("Cannot allocate memory for RX flow hash config");
+		return 1;
+	}
+
+	rss->indir_size = indir_bytes / sizeof(rss->rss_config[0]);
+	rss->key_size = hkey_bytes;
+	rss->hfunc = rss_hfunc;
+
+	memcpy(rss->rss_config, indir_table, indir_bytes);
+	memcpy(rss->rss_config + rss->indir_size, hkey, hkey_bytes);
+
+	/* Fetch RSS hash functions and their status and print */
+
+	if (!nlctx->is_monitor) {
+		ret = netlink_init_ethnl2_socket(nlctx);
+		if (ret < 0)
+			return MNL_CB_ERROR;
+	}
+	hash_funcs = global_stringset(ETH_SS_RSS_HASH_FUNCS,
+				      nlctx->ethnl2_socket);
+
+	ret = mnl_attr_parse(nlhdr, GENL_HDRLEN, attr_cb, &tb_info);
+	if (ret < 0)
+		return silent ? MNL_CB_OK : MNL_CB_ERROR;
+	nlctx->devname = get_dev_name(tb[ETHTOOL_A_RSS_HEADER]);
+	if (!dev_ok(nlctx))
+		return MNL_CB_OK;
+
+	if (is_json_context()) {
+		dump_json_rss_info(nlctx->ctx, rss, hash_funcs);
+	} else {
+		print_rss_info(nlctx->ctx, args->num_rings, rss);
+		printf("RSS hash function:\n");
+		if (!rss_hfunc) {
+			printf("    Operation not supported\n");
+			return 0;
+		}
+		for (unsigned int i = 0; i < get_count(hash_funcs); i++) {
+			printf("    %s: %s\n", get_string(hash_funcs, i),
+			       (rss_hfunc & (1 << i)) ? "on" : "off");
+		}
+	}
+
+	free(rss);
+	return MNL_CB_OK;
+}
+
+/* RSS_GET */
+static const struct param_parser grss_params[] = {
+	{
+		.arg		= "context",
+		.type		= ETHTOOL_A_RSS_CONTEXT,
+		.handler	= nl_parse_direct_u32,
+		.min_argc	= 1,
+	},
+	{}
+};
+
+int get_channels_cb(const struct nlmsghdr *nlhdr, void *data)
+{
+	const struct nlattr *tb[ETHTOOL_A_CHANNELS_MAX + 1] = {};
+	DECLARE_ATTR_TB_INFO(tb);
+	struct cb_args *args = data;
+	struct nl_context *nlctx =  args->nlctx;
+	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_CHANNELS_HEADER]);
+	if (!dev_ok(nlctx))
+		return err_ret;
+
+	args->num_rings = mnl_attr_get_u8(tb[ETHTOOL_A_CHANNELS_COMBINED_COUNT]);
+	return MNL_CB_OK;
+}
+
+int nl_grss(struct cmd_context *ctx)
+{
+	struct nl_context *nlctx = ctx->nlctx;
+	struct nl_socket *nlsk = nlctx->ethnl_socket;
+	struct nl_msg_buff *msgbuff;
+	struct cb_args args;
+	int ret;
+
+	nlctx->cmd = "-x";
+	nlctx->argp = ctx->argp;
+	nlctx->argc = ctx->argc;
+	nlctx->devname = ctx->devname;
+	nlsk = nlctx->ethnl_socket;
+	msgbuff = &nlsk->msgbuff;
+
+	if (netlink_cmd_check(ctx, ETHTOOL_MSG_RSS_GET, true))
+		return -EOPNOTSUPP;
+
+	/* save rings information into args.num_rings */
+	if (netlink_cmd_check(ctx, ETHTOOL_MSG_CHANNELS_GET, true))
+		return -EOPNOTSUPP;
+
+	ret = nlsock_prep_get_request(nlsk, ETHTOOL_MSG_CHANNELS_GET,
+				      ETHTOOL_A_CHANNELS_HEADER, 0);
+	if (ret < 0)
+		goto err;
+
+	ret = nlsock_sendmsg(nlsk, NULL);
+	if (ret < 0)
+		goto err;
+
+	args.nlctx = nlsk->nlctx;
+	ret = nlsock_process_reply(nlsk, get_channels_cb, &args);
+	if (ret < 0)
+		goto err;
+
+	ret = msg_init(nlctx, msgbuff, ETHTOOL_MSG_RSS_GET,
+		       NLM_F_REQUEST | NLM_F_ACK);
+	if (ret < 0)
+		return 1;
+	if (ethnla_fill_header(msgbuff, ETHTOOL_A_RSS_HEADER,
+			       ctx->devname, 0))
+		return -EMSGSIZE;
+
+	ret = nl_parser(nlctx, grss_params, NULL, PARSER_GROUP_NONE, NULL);
+	if (ret < 0)
+		goto err;
+
+	new_json_obj(ctx->json);
+	ret = nlsock_sendmsg(nlsk, NULL);
+	if (ret < 0)
+		goto err;
+
+	args.nlctx = nlctx;
+	ret = nlsock_process_reply(nlsk, rss_reply_cb, &args);
+	delete_json_obj();
+
+	if (ret == 0)
+		return 0;
+
+err:
+	return nlctx->exit_code ?: 1;
+}
+
-- 
2.31.1

Re: [PATCH ethtool-next v4 2/2] netlink: add netlink handler for get rss (-x)

[Jakub Kicinski]

On Wed, 28 Dec 2022 17:12:43 -0800 Sudheer Mogilappagari wrote:
> Add support for netlink based "ethtool -x <dev>" command using
> ETHTOOL_MSG_RSS_GET netlink message. It implements same functionality
> provided by traditional ETHTOOL_GRSSH subcommand. This displays RSS
> table, hash key and hash function along with JSON support.

> +void dump_json_rss_info(struct cmd_context *ctx, struct ethtool_rxfh *rss,
> +			const struct stringset *hash_funcs)
> +{
> +	unsigned int indir_bytes = rss->indir_size * sizeof(rss->rss_config[0]);
> +	unsigned int i;
> +
> +	open_json_object(NULL);
> +	print_string(PRINT_JSON, "ifname", NULL, ctx->devname);
> +	if (rss->indir_size) {
> +		open_json_array("rss-indirection-table", NULL);
> +		for (i = 0; i < rss->indir_size; i++)
> +			print_uint(PRINT_JSON, NULL, NULL, rss->rss_config[i]);
> +		close_json_array("\n");
> +	}
> +
> +	if (rss->key_size) {
> +		const char *hkey = ((char *)rss->rss_config + indir_bytes);
> +
> +		open_json_array("rss-hash-key", NULL);
> +		for (i = 0; i < rss->key_size; i++)
> +			print_uint(PRINT_JSON, NULL, NULL, (u8)hkey[i]);
> +		close_json_array("\n");
> +	}
> +
> +	if (rss->hfunc) {
> +		open_json_object("rss-hash-function");
> +		for (i = 0; i < get_count(hash_funcs); i++)
> +			print_bool(PRINT_JSON, get_string(hash_funcs, i), NULL,
> +				   (rss->hfunc & (1 << i)));
> +		close_json_object();
> +	}

I believe there can only be a single bit set here, so why not print:

	 "rss-hash-function": "toeplitz"

rather than:

	  "rss-hash-function": {
            "toeplitz": true,
            "xor": false,
            "crc32": false
          }

We can make the kernel ensure only one bit is set by checking 
hweight() == 1 on the value returned by the driver.

> +	close_json_object();
> +}
> +
> +int rss_reply_cb(const struct nlmsghdr *nlhdr, void *data)
> +{
> +	const struct nlattr *tb[ETHTOOL_A_RSS_MAX + 1] = {};
> +	unsigned int indir_bytes = 0, hkey_bytes = 0;
> +	DECLARE_ATTR_TB_INFO(tb);
> +	struct cb_args *args = data;
> +	struct nl_context *nlctx =  args->nlctx;

double space

> +	const struct stringset *hash_funcs;
> +	u32 rss_config_size, rss_hfunc;
> +	const char *indir_table, *hkey;
> +
> +	struct ethtool_rxfh *rss;
> +	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_RSS_HEADER]);
> +	if (!dev_ok(nlctx))
> +		return err_ret;
> +
> +	if (silent)
> +		putchar('\n');

show_cr()

> +	rss_hfunc = mnl_attr_get_u32(tb[ETHTOOL_A_RSS_HFUNC]);
> +
> +	indir_bytes = mnl_attr_get_payload_len(tb[ETHTOOL_A_RSS_INDIR]);
> +	indir_table = mnl_attr_get_str(tb[ETHTOOL_A_RSS_INDIR]);
> +
> +	hkey_bytes = mnl_attr_get_payload_len(tb[ETHTOOL_A_RSS_HKEY]);
> +	hkey = mnl_attr_get_str(tb[ETHTOOL_A_RSS_HKEY]);

All elements of tb can be NULL, AFAIU.

> +	rss_config_size = indir_bytes + hkey_bytes;
> +
> +	rss = calloc(1, sizeof(*rss) + rss_config_size);
> +	if (!rss) {
> +		perror("Cannot allocate memory for RX flow hash config");
> +		return 1;
> +	}
> +
> +	rss->indir_size = indir_bytes / sizeof(rss->rss_config[0]);
> +	rss->key_size = hkey_bytes;
> +	rss->hfunc = rss_hfunc;
> +
> +	memcpy(rss->rss_config, indir_table, indir_bytes);
> +	memcpy(rss->rss_config + rss->indir_size, hkey, hkey_bytes);

Do you only perform this coalescing to reuse the existing print
helpers? With a bit of extra refactoring this seems avoidable...

> +	/* Fetch RSS hash functions and their status and print */
> +
> +	if (!nlctx->is_monitor) {
> +		ret = netlink_init_ethnl2_socket(nlctx);
> +		if (ret < 0)
> +			return MNL_CB_ERROR;
> +	}
> +	hash_funcs = global_stringset(ETH_SS_RSS_HASH_FUNCS,
> +				      nlctx->ethnl2_socket);
> +
> +	ret = mnl_attr_parse(nlhdr, GENL_HDRLEN, attr_cb, &tb_info);
> +	if (ret < 0)
> +		return silent ? MNL_CB_OK : MNL_CB_ERROR;
> +	nlctx->devname = get_dev_name(tb[ETHTOOL_A_RSS_HEADER]);
> +	if (!dev_ok(nlctx))
> +		return MNL_CB_OK;
> +
> +	if (is_json_context()) {
> +		dump_json_rss_info(nlctx->ctx, rss, hash_funcs);
> +	} else {
> +		print_rss_info(nlctx->ctx, args->num_rings, rss);
> +		printf("RSS hash function:\n");
> +		if (!rss_hfunc) {
> +			printf("    Operation not supported\n");
> +			return 0;
> +		}
> +		for (unsigned int i = 0; i < get_count(hash_funcs); i++) {
> +			printf("    %s: %s\n", get_string(hash_funcs, i),
> +			       (rss_hfunc & (1 << i)) ? "on" : "off");
> +		}
> +	}
> +
> +	free(rss);
> +	return MNL_CB_OK;
> +}

> +int get_channels_cb(const struct nlmsghdr *nlhdr, void *data)
> +{
> +	const struct nlattr *tb[ETHTOOL_A_CHANNELS_MAX + 1] = {};
> +	DECLARE_ATTR_TB_INFO(tb);
> +	struct cb_args *args = data;
> +	struct nl_context *nlctx =  args->nlctx;
> +	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_CHANNELS_HEADER]);

We need to check that the kernel has filled in the attrs before accessing them, no?

> +	if (!dev_ok(nlctx))
> +		return err_ret;
> +
> +	args->num_rings = mnl_attr_get_u8(tb[ETHTOOL_A_CHANNELS_COMBINED_COUNT]);

u32, not u8

The correct value is combined + rx, I think I mentioned this before.

> +	return MNL_CB_OK;

I'm also not sure if fetching the channel info shouldn't be done over
the nl2 socket, like the string set. If we are in monitor mode we may
confuse ourselves trying to parse the wrong messages.

RE: [PATCH ethtool-next v4 2/2] netlink: add netlink handler for get rss (-x)

[Mogilappagari, Sudheer]

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>


> I believe there can only be a single bit set here, so why not print:
> 
> 	 "rss-hash-function": "toeplitz"
>
> rather than:
> 
> 	  "rss-hash-function": {
>             "toeplitz": true,
>             "xor": false,
>             "crc32": false
>           }
> 

Was following similar output format as current one. Changed in next version. 

> > +	rss_hfunc = mnl_attr_get_u32(tb[ETHTOOL_A_RSS_HFUNC]);
> > +
> > +	indir_bytes = mnl_attr_get_payload_len(tb[ETHTOOL_A_RSS_INDIR]);
> > +	indir_table = mnl_attr_get_str(tb[ETHTOOL_A_RSS_INDIR]);
> > +
> > +	hkey_bytes = mnl_attr_get_payload_len(tb[ETHTOOL_A_RSS_HKEY]);
> > +	hkey = mnl_attr_get_str(tb[ETHTOOL_A_RSS_HKEY]);
> 
> All elements of tb can be NULL, AFAIU.

Didn't get this. Do you mean the variables need to be checked 
for NULL here? If yes, am checking before printing later on.  
 
> > +	rss->indir_size = indir_bytes / sizeof(rss->rss_config[0]);
> > +	rss->key_size = hkey_bytes;
> > +	rss->hfunc = rss_hfunc;
> > +
> > +	memcpy(rss->rss_config, indir_table, indir_bytes);
> > +	memcpy(rss->rss_config + rss->indir_size, hkey, hkey_bytes);
> 
> Do you only perform this coalescing to reuse the existing print
> helpers? With a bit of extra refactoring this seems avoidable...

Yes. Was reusing print helpers. Have refactored to avoid need for
coalescing.

> > +int get_channels_cb(const struct nlmsghdr *nlhdr, void *data) {

> > +	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_CHANNELS_HEADER]);
> 
> We need to check that the kernel has filled in the attrs before
> accessing them, no?

Didn't get this one either. similar code isn't doing any checks 
like you suggested.

> > +	if (!dev_ok(nlctx))
> > +		return err_ret;
> > +
> > +	args->num_rings =
> > +mnl_attr_get_u8(tb[ETHTOOL_A_CHANNELS_COMBINED_COUNT]);
> 
> u32, not u8

Fixed.

> The correct value is combined + rx, I think I mentioned this before.

Have changed it to include rx too like below. 
args->num_rings = mnl_attr_get_u32(tb[ETHTOOL_A_CHANNELS_COMBINED_COUNT]);
args->num_rings += mnl_attr_get_u32(tb[ETHTOOL_A_CHANNELS_RX_COUNT]);

Slightly unrelated, where can I find the reason behind using combined + rx?
Guess it was discussed in mailing list but not able to find it.

> +	return MNL_CB_OK;
> 
> I'm also not sure if fetching the channel info shouldn't be done over
> the nl2 socket, like the string set. If we are in monitor mode we may
> confuse ourselves trying to parse the wrong messages.

Are you suggesting we need to use ioctl for fetching ring info to avoid
mix-up. Is there alternative way to do it ?  

Re: [PATCH ethtool-next v4 2/2] netlink: add netlink handler for get rss (-x)

[Jakub Kicinski]

On Fri, 6 Jan 2023 17:41:39 +0000 Mogilappagari, Sudheer wrote:
> > > +	rss_hfunc = mnl_attr_get_u32(tb[ETHTOOL_A_RSS_HFUNC]);
> > > +
> > > +	indir_bytes = mnl_attr_get_payload_len(tb[ETHTOOL_A_RSS_INDIR]);
> > > +	indir_table = mnl_attr_get_str(tb[ETHTOOL_A_RSS_INDIR]);
> > > +
> > > +	hkey_bytes = mnl_attr_get_payload_len(tb[ETHTOOL_A_RSS_HKEY]);
> > > +	hkey = mnl_attr_get_str(tb[ETHTOOL_A_RSS_HKEY]);  
> > 
> > All elements of tb can be NULL, AFAIU.  
> 
> Didn't get this. Do you mean the variables need to be checked 
> for NULL here? If yes, am checking before printing later on.  

tb[ETHTOOL_A_RSS_HKEY] can be NULL. I just realized now that the kernel
always fills in the attrs:

	if (nla_put_u32(skb, ETHTOOL_A_RSS_HFUNC, data->hfunc) ||
	    nla_put(skb, ETHTOOL_A_RSS_INDIR,
		    sizeof(u32) * data->indir_size, data->indir_table) ||
	    nla_put(skb, ETHTOOL_A_RSS_HKEY, data->hkey_size, data->hkey))
		return -EMSGSIZE;

but that's not a great use of netlink. If there is nothing to report
the attribute should simply be skipped, like this:

	if (nla_put_u32(skb, ETHTOOL_A_RSS_HFUNC, data->hfunc)) ||
	    (data->indir_size && 
	     nla_put(skb, ETHTOOL_A_RSS_INDIR,
		     sizeof(u32) * data->indir_size, data->indir_table)) ||
	    (data->hkey_size &&
	     nla_put(skb, ETHTOOL_A_RSS_HKEY, data->hkey_size, data->hkey)))
		return -EMSGSIZE;

I think we should fix the kernel side.

> > > +int get_channels_cb(const struct nlmsghdr *nlhdr, void *data) {  
> 
> > > +	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_CHANNELS_HEADER]);  
> > 
> > We need to check that the kernel has filled in the attrs before
> > accessing them, no?  
> 
> Didn't get this one either. similar code isn't doing any checks 
> like you suggested.

Same point, really. Even if the kernel always populates the attribute
today, according to netlink rules it's not guaranteed to do so in the
future, so any tb[] access should be NULL-checked.

> > The correct value is combined + rx, I think I mentioned this before.  
> 
> Have changed it to include rx too like below. 
> args->num_rings = mnl_attr_get_u32(tb[ETHTOOL_A_CHANNELS_COMBINED_COUNT]);
> args->num_rings += mnl_attr_get_u32(tb[ETHTOOL_A_CHANNELS_RX_COUNT]);

Related to previous comments - take a look at channels_fill_reply().
tb[ETHTOOL_A_CHANNELS_RX_COUNT] will be NULL for most devices.
mnl_attr_get_u32(NULL) will crash.

> Slightly unrelated, where can I find the reason behind using combined + rx?
> Guess it was discussed in mailing list but not able to find it.

Yes, it's been discussed on the list multiple times but man ethtool 
is the only source of documentation I know of.

The reason is that the channels API refers to interrupts more than
queues. So rx means an _irq_ dedicated to Rx processing, not an Rx
queue. Unfortunately the author came up with the term "channel" which
most people take to mean "queue" :(

> > +	return MNL_CB_OK;
> > 
> > I'm also not sure if fetching the channel info shouldn't be done over
> > the nl2 socket, like the string set. If we are in monitor mode we may
> > confuse ourselves trying to parse the wrong messages.  
> 
> Are you suggesting we need to use ioctl for fetching ring info to avoid
> mix-up. Is there alternative way to do it ?  

No no, look how the strings for hfunc names are fetched - they are
fetched over a different socket, right?

RE: [PATCH ethtool-next v4 2/2] netlink: add netlink handler for get rss (-x)

[Mogilappagari, Sudheer]

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Friday, January 6, 2023 1:42 PM
> To: Mogilappagari, Sudheer <sudheer.mogilappagari@intel.com>
> Cc: netdev@vger.kernel.org; mkubecek@suse.cz; andrew@lunn.ch;
> Samudrala, Sridhar <sridhar.samudrala@intel.com>; Nguyen, Anthony L
> <anthony.l.nguyen@intel.com>
> Subject: Re: [PATCH ethtool-next v4 2/2] netlink: add netlink handler
> for get rss (-x)
> 
> On Fri, 6 Jan 2023 17:41:39 +0000 Mogilappagari, Sudheer wrote:
> > > > +	rss_hfunc = mnl_attr_get_u32(tb[ETHTOOL_A_RSS_HFUNC]);
> > > > +
> > > > +	indir_bytes =
> mnl_attr_get_payload_len(tb[ETHTOOL_A_RSS_INDIR]);
> > > > +	indir_table = mnl_attr_get_str(tb[ETHTOOL_A_RSS_INDIR]);
> > > > +
> > > > +	hkey_bytes =
> mnl_attr_get_payload_len(tb[ETHTOOL_A_RSS_HKEY]);
> > > > +	hkey = mnl_attr_get_str(tb[ETHTOOL_A_RSS_HKEY]);
> > >
> > > All elements of tb can be NULL, AFAIU.
> >
> > Didn't get this. Do you mean the variables need to be checked for
> NULL
> > here? If yes, am checking before printing later on.
> 
> tb[ETHTOOL_A_RSS_HKEY] can be NULL. I just realized now that the kernel
> always fills in the attrs:
> 
> 	if (nla_put_u32(skb, ETHTOOL_A_RSS_HFUNC, data->hfunc) ||
> 	    nla_put(skb, ETHTOOL_A_RSS_INDIR,
> 		    sizeof(u32) * data->indir_size, data->indir_table) ||
> 	    nla_put(skb, ETHTOOL_A_RSS_HKEY, data->hkey_size, data-
> >hkey))
> 		return -EMSGSIZE;
> 
> but that's not a great use of netlink. If there is nothing to report
> the attribute should simply be skipped, like this:
> 
> 	if (nla_put_u32(skb, ETHTOOL_A_RSS_HFUNC, data->hfunc)) ||
> 	    (data->indir_size &&
> 	     nla_put(skb, ETHTOOL_A_RSS_INDIR,
> 		     sizeof(u32) * data->indir_size, data->indir_table))
> ||
> 	    (data->hkey_size &&
> 	     nla_put(skb, ETHTOOL_A_RSS_HKEY, data->hkey_size, data-
> >hkey)))
> 		return -EMSGSIZE;
> 
> I think we should fix the kernel side.

Will do this. 

> 
> > > > +int get_channels_cb(const struct nlmsghdr *nlhdr, void *data) {
> >
> > > > +	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_CHANNELS_HEADER]);
> > >
> > > We need to check that the kernel has filled in the attrs before
> > > accessing them, no?
> >
> > Didn't get this one either. similar code isn't doing any checks like
> > you suggested.
> 
> Same point, really. Even if the kernel always populates the attribute
> today, according to netlink rules it's not guaranteed to do so in the
> future, so any tb[] access should be NULL-checked.
> 
> > > The correct value is combined + rx, I think I mentioned this
> before.
> >
> > Have changed it to include rx too like below.
> > args->num_rings =
> > args->mnl_attr_get_u32(tb[ETHTOOL_A_CHANNELS_COMBINED_COUNT]);
> > args->num_rings += mnl_attr_get_u32(tb[ETHTOOL_A_CHANNELS_RX_COUNT]);
> 
> Related to previous comments - take a look at channels_fill_reply().
> tb[ETHTOOL_A_CHANNELS_RX_COUNT] will be NULL for most devices.
> mnl_attr_get_u32(NULL) will crash.
> 
> > Slightly unrelated, where can I find the reason behind using combined
> + rx?
> > Guess it was discussed in mailing list but not able to find it.
> 
> Yes, it's been discussed on the list multiple times but man ethtool is
> the only source of documentation I know of.
> 
> The reason is that the channels API refers to interrupts more than
> queues. So rx means an _irq_ dedicated to Rx processing, not an Rx
> queue. Unfortunately the author came up with the term "channel" which
> most people take to mean "queue" :(
> 
> > > +	return MNL_CB_OK;
> > >
> > > I'm also not sure if fetching the channel info shouldn't be done
> > > over the nl2 socket, like the string set. If we are in monitor mode
> > > we may confuse ourselves trying to parse the wrong messages.
> >
> > Are you suggesting we need to use ioctl for fetching ring info to
> > avoid mix-up. Is there alternative way to do it ?
> 
> No no, look how the strings for hfunc names are fetched - they are
> fetched over a different socket, right?

global_stringset is using nlctx->ethnl2_socket. Are you suggesting use
of it for fetching channels info too ? 

ret = netlink_init_ethnl2_socket(nlctx);
...
hash_funcs = global_stringset(ETH_SS_RSS_HASH_FUNCS, nlctx->ethnl2_socket);

Re: [PATCH ethtool-next v4 2/2] netlink: add netlink handler for get rss (-x)

[Jakub Kicinski]

On Mon, 9 Jan 2023 18:07:45 +0000 Mogilappagari, Sudheer wrote:
> > > Are you suggesting we need to use ioctl for fetching ring info to
> > > avoid mix-up. Is there alternative way to do it ?  
> > 
> > No no, look how the strings for hfunc names are fetched - they are
> > fetched over a different socket, right?  
> 
> global_stringset is using nlctx->ethnl2_socket. Are you suggesting use
> of it for fetching channels info too ? 
> 
> ret = netlink_init_ethnl2_socket(nlctx);
> ...
> hash_funcs = global_stringset(ETH_SS_RSS_HASH_FUNCS, nlctx->ethnl2_socket);

Yessir.

Re: [PATCH ethtool-next v4 2/2] netlink: add netlink handler for get rss (-x)

[Michal Kubecek]

On Mon, Jan 09, 2023 at 06:07:45PM +0000, Mogilappagari, Sudheer wrote:
> > -----Original Message-----
> > From: Jakub Kicinski <kuba@kernel.org>
> > Sent: Friday, January 6, 2023 1:42 PM
> > To: Mogilappagari, Sudheer <sudheer.mogilappagari@intel.com>
> > Cc: netdev@vger.kernel.org; mkubecek@suse.cz; andrew@lunn.ch;
> > Samudrala, Sridhar <sridhar.samudrala@intel.com>; Nguyen, Anthony L
> > <anthony.l.nguyen@intel.com>
> > Subject: Re: [PATCH ethtool-next v4 2/2] netlink: add netlink handler
> > for get rss (-x)
[...]
> > No no, look how the strings for hfunc names are fetched - they are
> > fetched over a different socket, right?
> 
> global_stringset is using nlctx->ethnl2_socket. Are you suggesting use
> of it for fetching channels info too ? 
> 
> ret = netlink_init_ethnl2_socket(nlctx);
> ...
> hash_funcs = global_stringset(ETH_SS_RSS_HASH_FUNCS, nlctx->ethnl2_socket);

The purpose of the second socket is to allow sending another request
while we are still processing the data from the main request so that we
cannot reuse the primary socket.

In this case, if we do not support dumps (and do not intend to), we
could technically send both request through the same socket (one after
the other) but I think it would be easier to use ethnl2_socket for the
auxiliary request anyway.

Michal