All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net-next] ethtool: add netlink support for rss set
@ 2023-03-09 22:05 Sudheer Mogilappagari
  2023-03-10  7:21 ` Jakub Kicinski
  0 siblings, 1 reply; 10+ messages in thread
From: Sudheer Mogilappagari @ 2023-03-09 22:05 UTC (permalink / raw)
  To: netdev; +Cc: kuba, mkubecek, sridhar.samudrala, anthony.l.nguyen

Add netlink based support for "ethtool -X <dev>" command by
implementing ETHTOOL_MSG_RSS_SET netlink message. This is
equivalent to functionality provided via ETHTOOL_SRSSH in
ioctl path. It allows creation and deletion of RSS context
and modifying RSS table, hash key and hash function of an
interface.

Functionality is backward compatible with the one available
in ioctl path but enables addition of new RSS context based
parameters in future.

Signed-off-by: Sudheer Mogilappagari <sudheer.mogilappagari@intel.com>
---
 Documentation/networking/ethtool-netlink.rst |  31 ++++
 include/uapi/linux/ethtool_netlink.h         |   3 +
 net/ethtool/netlink.c                        |   7 +
 net/ethtool/netlink.h                        |   2 +
 net/ethtool/rss.c                            | 155 +++++++++++++++++++
 5 files changed, 198 insertions(+)

diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
index e1bc6186d7ea..e03228978d1a 100644
--- a/Documentation/networking/ethtool-netlink.rst
+++ b/Documentation/networking/ethtool-netlink.rst
@@ -223,6 +223,7 @@ Userspace to kernel:
   ``ETHTOOL_MSG_PSE_SET``               set PSE parameters
   ``ETHTOOL_MSG_PSE_GET``               get PSE parameters
   ``ETHTOOL_MSG_RSS_GET``               get RSS settings
+  ``ETHTOOL_MSG_RSS_SET``               set RSS settings
   ``ETHTOOL_MSG_MM_GET``                get MAC merge layer state
   ``ETHTOOL_MSG_MM_SET``                set MAC merge layer parameters
   ===================================== =================================
@@ -1756,6 +1757,36 @@ being used. Current supported options are toeplitz, xor or crc32.
 ETHTOOL_A_RSS_INDIR attribute returns RSS indrection table where each byte
 indicates queue number.
 
+RSS_SET
+=======
+
+Update indirection table, hash key and hash function of a RSS context.
+similar to ``ETHTOOL_SRSSH`` ioctl request.
+
+Request contents:
+
+=====================================  ======  ==========================
+  ``ETHTOOL_A_RSS_HEADER``             nested  request header
+  ``ETHTOOL_A_RSS_CONTEXT``            u32     context number
+  ``ETHTOOL_A_RSS_HFUNC``              u32     RSS hash func
+  ``ETHTOOL_A_RSS_INDIR``              binary  Indir table bytes
+  ``ETHTOOL_A_RSS_HKEY``               binary  Hash key bytes
+  ``ETHTOOL_A_RSS_DELETE``             u8      context delete flag
+=====================================  ======  ==========================
+
+Kernel response contents:
+
+=====================================  ======  ==========================
+  ``ETHTOOL_A_RSS_HEADER``             nested  reply header
+  ``ETHTOOL_A_RSS_CONTEXT``            u32     context number created
+=====================================  ======  ==========================
+
+RSS context value of ETH_RXFH_CONTEXT_ALLOC indicates creation of new
+context. Response contains newly created context number or same context
+number as request. ETHTOOL_A_RSS_HFUNC attribute is bitmap indicating
+the hash function being used. Current supported options are toeplitz,
+xor or crc32.
+
 PLCA_GET_CFG
 ============
 
diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
index d39ce21381c5..56c4e8570dc6 100644
--- a/include/uapi/linux/ethtool_netlink.h
+++ b/include/uapi/linux/ethtool_netlink.h
@@ -52,6 +52,7 @@ enum {
 	ETHTOOL_MSG_PSE_GET,
 	ETHTOOL_MSG_PSE_SET,
 	ETHTOOL_MSG_RSS_GET,
+	ETHTOOL_MSG_RSS_SET,
 	ETHTOOL_MSG_PLCA_GET_CFG,
 	ETHTOOL_MSG_PLCA_SET_CFG,
 	ETHTOOL_MSG_PLCA_GET_STATUS,
@@ -104,6 +105,7 @@ enum {
 	ETHTOOL_MSG_MODULE_NTF,
 	ETHTOOL_MSG_PSE_GET_REPLY,
 	ETHTOOL_MSG_RSS_GET_REPLY,
+	ETHTOOL_MSG_RSS_SET_REPLY,
 	ETHTOOL_MSG_PLCA_GET_CFG_REPLY,
 	ETHTOOL_MSG_PLCA_GET_STATUS_REPLY,
 	ETHTOOL_MSG_PLCA_NTF,
@@ -906,6 +908,7 @@ enum {
 	ETHTOOL_A_RSS_HFUNC,		/* u32 */
 	ETHTOOL_A_RSS_INDIR,		/* binary */
 	ETHTOOL_A_RSS_HKEY,		/* binary */
+	ETHTOOL_A_RSS_DELETE,		/* u8 */
 
 	__ETHTOOL_A_RSS_CNT,
 	ETHTOOL_A_RSS_MAX = (__ETHTOOL_A_RSS_CNT - 1),
diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index 08120095cc68..22177883438b 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -1115,6 +1115,13 @@ static const struct genl_ops ethtool_genl_ops[] = {
 		.policy = ethnl_rss_get_policy,
 		.maxattr = ARRAY_SIZE(ethnl_rss_get_policy) - 1,
 	},
+	{
+		.cmd	= ETHTOOL_MSG_RSS_SET,
+		.flags	= GENL_UNS_ADMIN_PERM,
+		.doit	= ethnl_set_rss,
+		.policy = ethnl_rss_set_policy,
+		.maxattr = ARRAY_SIZE(ethnl_rss_set_policy) - 1,
+	},
 	{
 		.cmd	= ETHTOOL_MSG_PLCA_GET_CFG,
 		.doit	= ethnl_default_doit,
diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h
index f7b189ed96b2..67d7e4e5b916 100644
--- a/net/ethtool/netlink.h
+++ b/net/ethtool/netlink.h
@@ -436,6 +436,7 @@ extern const struct nla_policy ethnl_module_set_policy[ETHTOOL_A_MODULE_POWER_MO
 extern const struct nla_policy ethnl_pse_get_policy[ETHTOOL_A_PSE_HEADER + 1];
 extern const struct nla_policy ethnl_pse_set_policy[ETHTOOL_A_PSE_MAX + 1];
 extern const struct nla_policy ethnl_rss_get_policy[ETHTOOL_A_RSS_CONTEXT + 1];
+extern const struct nla_policy ethnl_rss_set_policy[ETHTOOL_A_RSS_MAX + 1];
 extern const struct nla_policy ethnl_plca_get_cfg_policy[ETHTOOL_A_PLCA_HEADER + 1];
 extern const struct nla_policy ethnl_plca_set_cfg_policy[ETHTOOL_A_PLCA_MAX + 1];
 extern const struct nla_policy ethnl_plca_get_status_policy[ETHTOOL_A_PLCA_HEADER + 1];
@@ -443,6 +444,7 @@ extern const struct nla_policy ethnl_mm_get_policy[ETHTOOL_A_MM_HEADER + 1];
 extern const struct nla_policy ethnl_mm_set_policy[ETHTOOL_A_MM_MAX + 1];
 
 int ethnl_set_features(struct sk_buff *skb, struct genl_info *info);
+int ethnl_set_rss(struct sk_buff *skb, struct genl_info *info);
 int ethnl_act_cable_test(struct sk_buff *skb, struct genl_info *info);
 int ethnl_act_cable_test_tdr(struct sk_buff *skb, struct genl_info *info);
 int ethnl_tunnel_info_doit(struct sk_buff *skb, struct genl_info *info);
diff --git a/net/ethtool/rss.c b/net/ethtool/rss.c
index be260ab34e58..f19e4baa83e2 100644
--- a/net/ethtool/rss.c
+++ b/net/ethtool/rss.c
@@ -154,3 +154,158 @@ const struct ethnl_request_ops ethnl_rss_request_ops = {
 	.fill_reply		= rss_fill_reply,
 	.cleanup_data		= rss_cleanup_data,
 };
+
+/* RSS_SET */
+
+const struct nla_policy ethnl_rss_set_policy[] = {
+	[ETHTOOL_A_RSS_HEADER] = NLA_POLICY_NESTED(ethnl_header_policy),
+	[ETHTOOL_A_RSS_CONTEXT] = { .type = NLA_U32 },
+	[ETHTOOL_A_RSS_HFUNC]	= { .type = NLA_U32 },
+	[ETHTOOL_A_RSS_INDIR]	= { .type = NLA_BINARY },
+	[ETHTOOL_A_RSS_HKEY]	= { .type = NLA_BINARY },
+	[ETHTOOL_A_RSS_DELETE]	= { .type = NLA_U8 },
+};
+
+static int srss_send_reply(struct net_device *dev, struct genl_info *info,
+			   u32 rss_context)
+{
+	struct sk_buff *rskb;
+	void *reply_payload;
+	int reply_len;
+	int ret;
+
+	reply_len = ethnl_reply_header_size() +
+		    nla_total_size(sizeof(u32)); /* RSS_CONTEXT */
+
+	rskb = ethnl_reply_init(reply_len, dev, ETHTOOL_MSG_RSS_SET_REPLY,
+				ETHTOOL_A_RSS_HEADER, info, &reply_payload);
+
+	ret = nla_put_u32(rskb, ETHTOOL_A_RSS_CONTEXT, rss_context);
+	if (ret < 0)
+		goto nla_put_failure;
+
+	genlmsg_end(rskb, reply_payload);
+	ret = genlmsg_reply(rskb, info);
+	return ret;
+
+nla_put_failure:
+	nlmsg_free(rskb);
+	WARN_ONCE(1, "calculated message payload length (%d) not sufficient\n",
+		  reply_len);
+	GENL_SET_ERR_MSG(info, "failed to send reply message");
+	return ret;
+}
+
+int ethnl_set_rss(struct sk_buff *skb, struct genl_info *info)
+{
+	struct ethnl_req_info req_info = {};
+	u32 rss_hfunc = 0, rss_context = 0;
+	u32 hkey_bytes = 0, indir_size = 0;
+	struct nlattr **tb = info->attrs;
+	const struct ethtool_ops *ops;
+	struct ethtool_rxnfc rx_rings;
+	struct net_device *dev;
+	u32 *rss_indir = NULL;
+	u8 *rss_hkey = NULL;
+	u32 delete = false;
+	bool mod = false;
+	int ret, i;
+
+	ret = ethnl_parse_header_dev_get(&req_info,
+					 tb[ETHTOOL_A_RSS_HEADER],
+					 genl_info_net(info), info->extack,
+					 true);
+	if (ret < 0)
+		return ret;
+
+	dev = req_info.dev;
+	ops = dev->ethtool_ops;
+	if (!ops->get_rxnfc || !ops->set_rxfh) {
+		ret = -EOPNOTSUPP;
+		goto out_dev;
+	}
+
+	rtnl_lock();
+	ret = ethnl_ops_begin(dev);
+	if (ret < 0)
+		goto out_rtnl;
+
+	ethnl_update_u32(&rss_context, tb[ETHTOOL_A_RSS_CONTEXT], &mod);
+	if (rss_context && !ops->set_rxfh_context) {
+		ret = -EOPNOTSUPP;
+		goto out_ops;
+	}
+
+	ethnl_update_bool32(&delete, tb[ETHTOOL_A_RSS_DELETE], &mod);
+	ethnl_update_u32(&rss_hfunc, tb[ETHTOOL_A_RSS_HFUNC], &mod);
+
+	if (tb[ETHTOOL_A_RSS_HKEY]) {
+		if (ops->get_rxfh_key_size)
+			hkey_bytes = ops->get_rxfh_key_size(dev);
+
+		if (!hkey_bytes ||
+		    nla_len(tb[ETHTOOL_A_RSS_HKEY]) != hkey_bytes) {
+			ret = -EINVAL;
+			goto out_free;
+		}
+
+		rss_hkey = kzalloc(hkey_bytes, GFP_KERNEL);
+		if (!rss_hkey) {
+			ret = -ENOMEM;
+			goto out_free;
+		}
+		ethnl_update_binary(rss_hkey, hkey_bytes,
+				    tb[ETHTOOL_A_RSS_HKEY], &mod);
+	}
+
+	if (tb[ETHTOOL_A_RSS_INDIR]) {
+		u32 indir_bytes;
+
+		if (ops->get_rxfh_indir_size)
+			indir_size = ops->get_rxfh_indir_size(dev);
+
+		indir_bytes = indir_size * sizeof(u32);
+		if (!indir_bytes ||
+		    nla_len(tb[ETHTOOL_A_RSS_INDIR]) != indir_bytes) {
+			ret = -EINVAL;
+			goto out_free;
+		}
+
+		rss_indir = kzalloc(indir_bytes, GFP_KERNEL);
+		if (!rss_indir) {
+			ret = -ENOMEM;
+			goto out_free;
+		}
+		ethnl_update_binary(rss_indir, indir_bytes,
+				    tb[ETHTOOL_A_RSS_INDIR], &mod);
+
+		/* Validate ring indices */
+		rx_rings.cmd = ETHTOOL_GRXRINGS;
+		ret = ops->get_rxnfc(dev, &rx_rings, NULL);
+		if (ret)
+			goto out_free;
+
+		for (i = 0; i < indir_size; i++)
+			if (rss_indir[i] >= rx_rings.data)
+				goto out_free;
+	}
+
+	if (rss_context)
+		ret = ops->set_rxfh_context(dev, rss_indir, rss_hkey, rss_hfunc,
+					    &rss_context, delete);
+	else
+		ret = ops->set_rxfh(dev, rss_indir, rss_hkey, rss_hfunc);
+
+	srss_send_reply(dev, info, rss_context);
+
+out_free:
+	kfree(rss_hkey);
+	kfree(rss_indir);
+out_ops:
+	ethnl_ops_complete(dev);
+out_rtnl:
+	rtnl_unlock();
+out_dev:
+	ethnl_parse_header_dev_put(&req_info);
+	return ret;
+}
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH net-next] ethtool: add netlink support for rss set
  2023-03-09 22:05 [PATCH net-next] ethtool: add netlink support for rss set Sudheer Mogilappagari
@ 2023-03-10  7:21 ` Jakub Kicinski
  2023-03-13 22:34   ` Mogilappagari, Sudheer
  0 siblings, 1 reply; 10+ messages in thread
From: Jakub Kicinski @ 2023-03-10  7:21 UTC (permalink / raw)
  To: Sudheer Mogilappagari
  Cc: netdev, mkubecek, sridhar.samudrala, anthony.l.nguyen

On Thu,  9 Mar 2023 14:05:44 -0800 Sudheer Mogilappagari wrote:
> Add netlink based support for "ethtool -X <dev>" command by
> implementing ETHTOOL_MSG_RSS_SET netlink message. This is
> equivalent to functionality provided via ETHTOOL_SRSSH in
> ioctl path. It allows creation and deletion of RSS context
> and modifying RSS table, hash key and hash function of an
> interface.
> 
> Functionality is backward compatible with the one available
> in ioctl path but enables addition of new RSS context based
> parameters in future.

RSS contexts are somewhat under-defined, so I'd prefer to wait
until we actually need to extend the API before going to netlink.
I think I told you as much when you posted initial code for RSS?

> diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
> index d39ce21381c5..56c4e8570dc6 100644
> --- a/include/uapi/linux/ethtool_netlink.h
> +++ b/include/uapi/linux/ethtool_netlink.h
> @@ -52,6 +52,7 @@ enum {
>  	ETHTOOL_MSG_PSE_GET,
>  	ETHTOOL_MSG_PSE_SET,
>  	ETHTOOL_MSG_RSS_GET,
> +	ETHTOOL_MSG_RSS_SET,
>  	ETHTOOL_MSG_PLCA_GET_CFG,
>  	ETHTOOL_MSG_PLCA_SET_CFG,
>  	ETHTOOL_MSG_PLCA_GET_STATUS,

You certainly can't add entries half way thru an enum in uAPI..

^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH net-next] ethtool: add netlink support for rss set
  2023-03-10  7:21 ` Jakub Kicinski
@ 2023-03-13 22:34   ` Mogilappagari, Sudheer
  2023-03-13 22:53     ` Jakub Kicinski
  0 siblings, 1 reply; 10+ messages in thread
From: Mogilappagari, Sudheer @ 2023-03-13 22:34 UTC (permalink / raw)
  To: Jakub Kicinski; +Cc: netdev, mkubecek, Samudrala, Sridhar, Nguyen, Anthony L



> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Thursday, March 9, 2023 11:21 PM
> To: Mogilappagari, Sudheer <sudheer.mogilappagari@intel.com>
> Cc: netdev@vger.kernel.org; mkubecek@suse.cz; Samudrala, Sridhar
> <sridhar.samudrala@intel.com>; Nguyen, Anthony L
> <anthony.l.nguyen@intel.com>
> Subject: Re: [PATCH net-next] ethtool: add netlink support for rss set
> 
> On Thu,  9 Mar 2023 14:05:44 -0800 Sudheer Mogilappagari wrote:
> > Add netlink based support for "ethtool -X <dev>" command by
> > implementing ETHTOOL_MSG_RSS_SET netlink message. This is equivalent
> > to functionality provided via ETHTOOL_SRSSH in ioctl path. It allows
> > creation and deletion of RSS context and modifying RSS table, hash key
> > and hash function of an interface.
> >
> > Functionality is backward compatible with the one available in ioctl
> > path but enables addition of new RSS context based parameters in
> > future.
> 
> RSS contexts are somewhat under-defined, so I'd prefer to wait until we
> actually need to extend the API before going to netlink.
> I think I told you as much when you posted initial code for RSS?
> 

Hi Jakub, we are making these changes based on below discussion:
https://lore.kernel.org/netdev/0402fc4f-21c9-eded-bed7-fd82a069ca70@intel.com/
Our thinking was to move existing functionality to netlink first and then
add new parameter (inline-flow-steering). Hence the reason for sending RSS_GET 
first and now RSS_SET. Are you suggesting that new parameter changes be sent
together with this patch-set ? 

> > diff --git a/include/uapi/linux/ethtool_netlink.h
> > b/include/uapi/linux/ethtool_netlink.h
> > index d39ce21381c5..56c4e8570dc6 100644
> > --- a/include/uapi/linux/ethtool_netlink.h
> > +++ b/include/uapi/linux/ethtool_netlink.h
> > @@ -52,6 +52,7 @@ enum {
> >  	ETHTOOL_MSG_PSE_GET,
> >  	ETHTOOL_MSG_PSE_SET,
> >  	ETHTOOL_MSG_RSS_GET,
> > +	ETHTOOL_MSG_RSS_SET,
> >  	ETHTOOL_MSG_PLCA_GET_CFG,
> >  	ETHTOOL_MSG_PLCA_SET_CFG,
> >  	ETHTOOL_MSG_PLCA_GET_STATUS,
> 
> You certainly can't add entries half way thru an enum in uAPI..

Will fix this.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH net-next] ethtool: add netlink support for rss set
  2023-03-13 22:34   ` Mogilappagari, Sudheer
@ 2023-03-13 22:53     ` Jakub Kicinski
  2023-03-14 13:34       ` Edward Cree
  0 siblings, 1 reply; 10+ messages in thread
From: Jakub Kicinski @ 2023-03-13 22:53 UTC (permalink / raw)
  To: Mogilappagari, Sudheer
  Cc: netdev, mkubecek, Samudrala, Sridhar, Nguyen, Anthony L

On Mon, 13 Mar 2023 22:34:17 +0000 Mogilappagari, Sudheer wrote:
> > RSS contexts are somewhat under-defined, so I'd prefer to wait until we
> > actually need to extend the API before going to netlink.
> > I think I told you as much when you posted initial code for RSS?
> 
> Hi Jakub, we are making these changes based on below discussion:
> https://lore.kernel.org/netdev/0402fc4f-21c9-eded-bed7-fd82a069ca70@intel.com/
> Our thinking was to move existing functionality to netlink first and then
> add new parameter (inline-flow-steering). Hence the reason for sending RSS_GET 
> first and now RSS_SET. Are you suggesting that new parameter changes be sent
> together with this patch-set ? 

Ah, so you do have a feature. Yes, it would be somewhat helpful but my
larger concern remains. We skipped the dump implementation when
implementing GET. The admin still has no way of knowing what / how many
RSS contexts had been created. With the context ID being an unbounded
integer just going from 0 until ENOENT is not even an option.

So we need to start tracking the contexts. Add a pointer to struct
netdevice to hold an "ethtool_settings" struct. In the ethtool settings
struct add a list head. Put an object for each created RSS context on
that list.

Then implement dump, then the netlink SET. (All one series.)

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH net-next] ethtool: add netlink support for rss set
  2023-03-13 22:53     ` Jakub Kicinski
@ 2023-03-14 13:34       ` Edward Cree
  2023-03-14 23:51         ` Mogilappagari, Sudheer
  2023-03-15  4:23         ` Jakub Kicinski
  0 siblings, 2 replies; 10+ messages in thread
From: Edward Cree @ 2023-03-14 13:34 UTC (permalink / raw)
  To: Jakub Kicinski, Mogilappagari, Sudheer
  Cc: netdev, mkubecek, Samudrala, Sridhar, Nguyen, Anthony L

On 13/03/2023 22:53, Jakub Kicinski wrote:
> Ah, so you do have a feature. Yes, it would be somewhat helpful but my
> larger concern remains. We skipped the dump implementation when
> implementing GET. The admin still has no way of knowing what / how many
> RSS contexts had been created. With the context ID being an unbounded
> integer just going from 0 until ENOENT is not even an option.
> 
> So we need to start tracking the contexts.

Hi Jakub, as the original author of custom RSS contexts I feel like I
 owe this bit of work; I can take a swing at it, unless Sudheer would
 rather do it himself.

> Add a pointer to struct
> netdevice to hold an "ethtool_settings" struct. In the ethtool settings
> struct add a list head. Put an object for each created RSS context on
> that list.
Would an IDR not be appropriate here, rather than a list?
AFAICT every driver that supports contexts either treats the context
 ID as an opaque handle or as an index into a fixed-size array, so as
 long as the driver reports its max context ID to the core somehow,
 the specific ID values chosen are arbitrary and the driver doesn't
 need to do the choosing, it can just take what comes out of the IDR.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH net-next] ethtool: add netlink support for rss set
  2023-03-14 13:34       ` Edward Cree
@ 2023-03-14 23:51         ` Mogilappagari, Sudheer
  2023-03-15  4:24           ` Jakub Kicinski
  2023-03-15  4:23         ` Jakub Kicinski
  1 sibling, 1 reply; 10+ messages in thread
From: Mogilappagari, Sudheer @ 2023-03-14 23:51 UTC (permalink / raw)
  To: Edward Cree, Jakub Kicinski
  Cc: netdev, mkubecek, Samudrala, Sridhar, Nguyen, Anthony L



> -----Original Message-----
> From: Edward Cree <ecree.xilinx@gmail.com>
> Sent: Tuesday, March 14, 2023 6:35 AM
> To: Jakub Kicinski <kuba@kernel.org>; Mogilappagari, Sudheer
> <sudheer.mogilappagari@intel.com>
> Cc: netdev@vger.kernel.org; mkubecek@suse.cz; Samudrala, Sridhar
> <sridhar.samudrala@intel.com>; Nguyen, Anthony L
> <anthony.l.nguyen@intel.com>
> Subject: Re: [PATCH net-next] ethtool: add netlink support for rss set
> 
> On 13/03/2023 22:53, Jakub Kicinski wrote:
> > Ah, so you do have a feature. Yes, it would be somewhat helpful but my
> > larger concern remains. We skipped the dump implementation when
> > implementing GET. The admin still has no way of knowing what / how
> > many RSS contexts had been created. With the context ID being an
> > unbounded integer just going from 0 until ENOENT is not even an option.
> >
> > So we need to start tracking the contexts.
> 
> Hi Jakub, as the original author of custom RSS contexts I feel like I  owe
> this bit of work; I can take a swing at it, unless Sudheer would  rather do
> it himself.

Hi Edward, would be great if you can add the support. I will modify the RSS_SET
patch accordingly based on your changes.

Got a question wrt dumpit. How to instrument dumpit functionality? ethtool doesn't 
seem to have an explicit option. In ethtool userspace code, NLM_F_DUMP is set 
in below functions 

 - nlsock_prep_get_request: 
	if (devname && !strcmp(devname, WILDCARD_DEVNAME)) {
	       devname = NULL;
	       nlm_flags |= NLM_F_DUMP;
	}
 - stringset_load_request : called with is_dump=false in ethtool functions

How to get devname to be WILDCARD_DEVNAME ?

> 
> > Add a pointer to struct
> > netdevice to hold an "ethtool_settings" struct. In the ethtool
> > settings struct add a list head. Put an object for each created RSS
> > context on that list.

> Would an IDR not be appropriate here, rather than a list?
> AFAICT every driver that supports contexts either treats the context  ID as
> an opaque handle or as an index into a fixed-size array, so as  long as the
> driver reports its max context ID to the core somehow,  the specific ID
> values chosen are arbitrary and the driver doesn't  need to do the choosing,
> it can just take what comes out of the IDR.


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH net-next] ethtool: add netlink support for rss set
  2023-03-14 13:34       ` Edward Cree
  2023-03-14 23:51         ` Mogilappagari, Sudheer
@ 2023-03-15  4:23         ` Jakub Kicinski
  1 sibling, 0 replies; 10+ messages in thread
From: Jakub Kicinski @ 2023-03-15  4:23 UTC (permalink / raw)
  To: Edward Cree
  Cc: Mogilappagari, Sudheer, netdev, mkubecek, Samudrala, Sridhar,
	Nguyen, Anthony L

On Tue, 14 Mar 2023 13:34:51 +0000 Edward Cree wrote:
> > Add a pointer to struct
> > netdevice to hold an "ethtool_settings" struct. In the ethtool settings
> > struct add a list head. Put an object for each created RSS context on
> > that list.  
> Would an IDR not be appropriate here, rather than a list?

Yup, I was too lazy how much memory IDR eats when unused, and list is
easier to explain, but let's just go for an xarray if you're doing it.

> AFAICT every driver that supports contexts either treats the context
>  ID as an opaque handle or as an index into a fixed-size array, so as
>  long as the driver reports its max context ID to the core somehow,
>  the specific ID values chosen are arbitrary and the driver doesn't
>  need to do the choosing, it can just take what comes out of the IDR.

Sounds great.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH net-next] ethtool: add netlink support for rss set
  2023-03-14 23:51         ` Mogilappagari, Sudheer
@ 2023-03-15  4:24           ` Jakub Kicinski
  2023-03-15  7:37             ` Michal Kubecek
  0 siblings, 1 reply; 10+ messages in thread
From: Jakub Kicinski @ 2023-03-15  4:24 UTC (permalink / raw)
  To: Mogilappagari, Sudheer
  Cc: Edward Cree, netdev, mkubecek, Samudrala, Sridhar, Nguyen, Anthony L

On Tue, 14 Mar 2023 23:51:00 +0000 Mogilappagari, Sudheer wrote:
> How to get devname to be WILDCARD_DEVNAME ?

Isn't it just \* ?

$ ethtool \*

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH net-next] ethtool: add netlink support for rss set
  2023-03-15  4:24           ` Jakub Kicinski
@ 2023-03-15  7:37             ` Michal Kubecek
  2023-03-15 15:37               ` Mogilappagari, Sudheer
  0 siblings, 1 reply; 10+ messages in thread
From: Michal Kubecek @ 2023-03-15  7:37 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: Mogilappagari, Sudheer, Edward Cree, netdev, Samudrala, Sridhar,
	Nguyen, Anthony L

On Tue, Mar 14, 2023 at 09:24:27PM -0700, Jakub Kicinski wrote:
> On Tue, 14 Mar 2023 23:51:00 +0000 Mogilappagari, Sudheer wrote:
> > How to get devname to be WILDCARD_DEVNAME ?
> 
> Isn't it just \* ?
> 
> $ ethtool \*

Yes, that's how it's supposed to work:

------------------------------------------------------------------------
mike@lion:~> /usr/sbin/ethtool -l '*'

Channel parameters for eth0:
Pre-set maximums:
RX:             n/a
TX:             n/a
Other:          1
Combined:       8
Current hardware settings:
RX:             n/a
TX:             n/a
Other:          1
Combined:       8

Channel parameters for eth1:
Pre-set maximums:
RX:             n/a
TX:             n/a
Other:          1
Combined:       8
Current hardware settings:
RX:             n/a
TX:             n/a
Other:          1
Combined:       8

Channel parameters for eth2:
Pre-set maximums:
RX:             n/a
TX:             n/a
Other:          1
Combined:       2
Current hardware settings:
RX:             n/a
TX:             n/a
Other:          1
Combined:       2
------------------------------------------------------------------------

In some cases where the output for a single device can consist of
many entries it may also make sense to iterate over something else.
Listing flow rules might be an example.

Michal

^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH net-next] ethtool: add netlink support for rss set
  2023-03-15  7:37             ` Michal Kubecek
@ 2023-03-15 15:37               ` Mogilappagari, Sudheer
  0 siblings, 0 replies; 10+ messages in thread
From: Mogilappagari, Sudheer @ 2023-03-15 15:37 UTC (permalink / raw)
  To: Michal Kubecek, Jakub Kicinski
  Cc: Edward Cree, netdev, Samudrala, Sridhar, Nguyen, Anthony L


> -----Original Message-----
> From: Michal Kubecek <mkubecek@suse.cz>
> Sent: Wednesday, March 15, 2023 12:38 AM
> To: Jakub Kicinski <kuba@kernel.org>
> Cc: Mogilappagari, Sudheer <sudheer.mogilappagari@intel.com>; Edward Cree
> <ecree.xilinx@gmail.com>; netdev@vger.kernel.org; Samudrala, Sridhar
> <sridhar.samudrala@intel.com>; Nguyen, Anthony L
> <anthony.l.nguyen@intel.com>
> Subject: Re: [PATCH net-next] ethtool: add netlink support for rss set
> 
> On Tue, Mar 14, 2023 at 09:24:27PM -0700, Jakub Kicinski wrote:
> > On Tue, 14 Mar 2023 23:51:00 +0000 Mogilappagari, Sudheer wrote:
> > > How to get devname to be WILDCARD_DEVNAME ?
> >
> > Isn't it just \* ?
> >
> > $ ethtool \*
> 
> Yes, that's how it's supposed to work:
> 
Thanks. I had tried with just *

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2023-03-15 15:38 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-09 22:05 [PATCH net-next] ethtool: add netlink support for rss set Sudheer Mogilappagari
2023-03-10  7:21 ` Jakub Kicinski
2023-03-13 22:34   ` Mogilappagari, Sudheer
2023-03-13 22:53     ` Jakub Kicinski
2023-03-14 13:34       ` Edward Cree
2023-03-14 23:51         ` Mogilappagari, Sudheer
2023-03-15  4:24           ` Jakub Kicinski
2023-03-15  7:37             ` Michal Kubecek
2023-03-15 15:37               ` Mogilappagari, Sudheer
2023-03-15  4:23         ` Jakub Kicinski

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.