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

[Jakub Kicinski <kuba@kernel.org>]

On Wed, 21 Dec 2022 16:13: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.
> 
> Sample output with json option:
> $ethtool --json -x eno2
> [ {
>     "ifname": "eno2",
>     "RSS indirection table": [ 0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,

"indirection-table"

>     1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,2,2,2,2,2,2,2,2,2,2,2,2,2,2
>     ...skip similar lines...
>     7,7,7,7,7,7,7,7,7,7,7,7,7,7,7,7 ],
>     "RSS hash Key": "be:c3:13:a6:59:9a:c3:c5:d8:60:75:2b:4c:

again, better key name, and please use an array of ints:

"hash-key": [ 190, 195, 19, ...

(Or binary encoded string (if it's compliant with JSON):

"hash-key": "\xbe\xc3\x13\xa6...

but I think array is easier to deal with.)

>     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"

Please use true / false.

>         }
>     } ]


> +void dump_rss_info(struct cmd_context *ctx, struct ethtool_rxfh *rss,
> +		   const struct stringset *hash_funcs)
> +{
> +	unsigned int indir_bytes = rss->indir_size * sizeof(u32);
> +	char *indir_str = NULL;
> +	char *hkey_str = NULL;
> +	unsigned int i;
> +
> +	open_json_object(NULL);
> +	print_string(PRINT_JSON, "ifname", NULL, ctx->devname);
> +
> +	if (rss->indir_size) {
> +		indir_str = calloc(1, indir_bytes * 3);
> +		if (!indir_str) {

where is this used?

> +			perror("Cannot allocate memory for RSS config");
> +			goto err;
> +		}
> +
> +		open_json_array("RSS indirection table", NULL);
> +		for (i = 0; i < rss->indir_size; i++)
> +			print_uint(PRINT_ANY, NULL, "%u", rss->rss_config[i]);
> +		close_json_array("\n");
> +	} else {
> +		print_string(PRINT_JSON, "RSS indirection table", NULL,
> +			     "not supported");

Why not skip the field? In non-JSON output I think we use "n/a" when not
supported.

> +	}