-----Original Message-----
From: Jakub Kicinski <kuba@kernel.org>
Sent: Friday, January 6, 2023 1:42 PM
To: Mogilappagari, Sudheer <redacted>
Cc: netdev@vger.kernel.org; mkubecek@suse.cz; andrew@lunn.ch;
Samudrala, Sridhar [off-list ref]; Nguyen, Anthony L
[off-list ref]
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:

quoted

quoted

quoted

+	rss_hfunc = mnl_attr_get_u32(tb[ETHTOOL_A_RSS_HFUNC]);
+
+	indir_bytes =

mnl_attr_get_payload_len(tb[ETHTOOL_A_RSS_INDIR]);

quoted

quoted

quoted

+	indir_table = mnl_attr_get_str(tb[ETHTOOL_A_RSS_INDIR]);
+
+	hkey_bytes =

mnl_attr_get_payload_len(tb[ETHTOOL_A_RSS_HKEY]);

quoted

quoted

quoted

+	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

quoted

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-

quoted

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-

quoted

hkey)))

		return -EMSGSIZE;

I think we should fix the kernel side.

Will do this. 

quoted

quoted

quoted

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

quoted

quoted

+	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);

quoted

quoted

quoted

+	if (ret < 0)
+		return err_ret;
+	nlctx->devname =

get_dev_name(tb[ETHTOOL_A_CHANNELS_HEADER]);

quoted

quoted

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.

quoted

quoted

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

before.

quoted

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.

quoted

Slightly unrelated, where can I find the reason behind using combined

+ rx?

quoted

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

quoted

quoted

+	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);