netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net-next v2 0/8] net: ethtool: rss: add notifications
@ 2025-06-23 23:17 Jakub Kicinski
  2025-06-23 23:17 ` [PATCH net-next v2 1/8] netlink: specs: add the multicast group name to spec Jakub Kicinski
                   ` (9 more replies)
  0 siblings, 10 replies; 20+ messages in thread
From: Jakub Kicinski @ 2025-06-23 23:17 UTC (permalink / raw)
  To: davem
  Cc: netdev, edumazet, pabeni, andrew+netdev, horms, donald.hunter,
	maxime.chevallier, sdf, jdamato, ecree.xilinx, Jakub Kicinski

Next step on the path to moving RSS config to Netlink. With the
refactoring of the driver-facing API for ETHTOOL_GRXFH/ETHTOOL_SRXFH
out of the way we can move on to more interesting work.

Add Netlink notifications for changes in RSS configuration.

As a reminder (part) of rss-get was introduced in previous releases
when input-xfrm (symmetric hashing) was added. rss-set isn't
implemented, yet, but we can implement rss-ntf and hook it into
the changes done via the IOCTL path (same as other ethtool-nl
notifications do).

Most of the series is concerned with passing arguments to notifications.
So far none of the notifications needed to be parametrized, but RSS can
have multiple contexts per device, and since GET operates on a single
context at a time, the notification needs to also be scoped to a context.
Patches 2-5 add support for passing arguments to notifications thru
ethtool-nl generic infra.

The notification handling itself is pretty trivial, it's mostly
hooking in the right entries into the ethool-nl op tables.

v2:
 - [patch 2] fix typo in commit msg
 - [patch 5] propagate phy_index already
 - [patch 6] fix compilation with ETHTOOL_NETLINK=n
 - [patch 8] old patch dropped/applied already patch 9 becomes patch 8
v1: https://lore.kernel.org/20250621171944.2619249-1-kuba@kernel.org

Jakub Kicinski (8):
  netlink: specs: add the multicast group name to spec
  net: ethtool: dynamically allocate full req size req
  net: ethtool: call .parse_request for SET handlers
  net: ethtool: remove the data argument from ethtool_notify()
  net: ethtool: copy req_info from SET to NTF
  net: ethtool: rss: add notifications
  doc: ethtool: mark ETHTOOL_GRXFHINDIR as reimplemented
  selftests: drv-net: test RSS Netlink notifications

 Documentation/netlink/specs/ethtool.yaml      | 13 +++
 Documentation/networking/ethtool-netlink.rst  |  3 +-
 include/linux/netdevice.h                     |  5 +-
 include/uapi/linux/ethtool_netlink.h          |  2 -
 .../uapi/linux/ethtool_netlink_generated.h    |  3 +
 net/ethtool/common.h                          |  8 ++
 net/ethtool/netlink.h                         |  4 +
 net/ethtool/ioctl.c                           | 28 +++---
 net/ethtool/netlink.c                         | 47 ++++++----
 net/ethtool/rss.c                             | 11 +++
 .../selftests/drivers/net/hw/rss_api.py       | 89 +++++++++++++++++++
 11 files changed, 180 insertions(+), 33 deletions(-)
 create mode 100755 tools/testing/selftests/drivers/net/hw/rss_api.py

-- 
2.49.0


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

* [PATCH net-next v2 1/8] netlink: specs: add the multicast group name to spec
  2025-06-23 23:17 [PATCH net-next v2 0/8] net: ethtool: rss: add notifications Jakub Kicinski
@ 2025-06-23 23:17 ` Jakub Kicinski
  2025-06-23 23:17 ` [PATCH net-next v2 2/8] net: ethtool: dynamically allocate full req size req Jakub Kicinski
                   ` (8 subsequent siblings)
  9 siblings, 0 replies; 20+ messages in thread
From: Jakub Kicinski @ 2025-06-23 23:17 UTC (permalink / raw)
  To: davem
  Cc: netdev, edumazet, pabeni, andrew+netdev, horms, donald.hunter,
	maxime.chevallier, sdf, jdamato, ecree.xilinx, Jakub Kicinski

Add the multicast group's name to the YAML spec.
Without it YNL doesn't know how to subscribe to notifications.

Reviewed-by: Donald Hunter <donald.hunter@gmail.com>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 Documentation/netlink/specs/ethtool.yaml       | 6 ++++++
 include/uapi/linux/ethtool_netlink.h           | 2 --
 include/uapi/linux/ethtool_netlink_generated.h | 2 ++
 3 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/Documentation/netlink/specs/ethtool.yaml b/Documentation/netlink/specs/ethtool.yaml
index c1651e175e8b..cfe84f84ba29 100644
--- a/Documentation/netlink/specs/ethtool.yaml
+++ b/Documentation/netlink/specs/ethtool.yaml
@@ -2492,3 +2492,9 @@ c-version-name: ethtool-genl-version
         attributes:
           - header
           - events
+
+mcast-groups:
+  list:
+    -
+      name: monitor
+      c-define-name: ethtool-mcgrp-monitor-name
diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
index 09a75bdb6560..fa5d645140a4 100644
--- a/include/uapi/linux/ethtool_netlink.h
+++ b/include/uapi/linux/ethtool_netlink.h
@@ -208,6 +208,4 @@ enum {
 	ETHTOOL_A_STATS_PHY_MAX = (__ETHTOOL_A_STATS_PHY_CNT - 1)
 };
 
-#define ETHTOOL_MCGRP_MONITOR_NAME "monitor"
-
 #endif /* _UAPI_LINUX_ETHTOOL_NETLINK_H_ */
diff --git a/include/uapi/linux/ethtool_netlink_generated.h b/include/uapi/linux/ethtool_netlink_generated.h
index 4944badf9fba..859e28c8a91a 100644
--- a/include/uapi/linux/ethtool_netlink_generated.h
+++ b/include/uapi/linux/ethtool_netlink_generated.h
@@ -867,4 +867,6 @@ enum {
 	ETHTOOL_MSG_KERNEL_MAX = (__ETHTOOL_MSG_KERNEL_CNT - 1)
 };
 
+#define ETHTOOL_MCGRP_MONITOR_NAME	"monitor"
+
 #endif /* _UAPI_LINUX_ETHTOOL_NETLINK_GENERATED_H */
-- 
2.49.0


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

* [PATCH net-next v2 2/8] net: ethtool: dynamically allocate full req size req
  2025-06-23 23:17 [PATCH net-next v2 0/8] net: ethtool: rss: add notifications Jakub Kicinski
  2025-06-23 23:17 ` [PATCH net-next v2 1/8] netlink: specs: add the multicast group name to spec Jakub Kicinski
@ 2025-06-23 23:17 ` Jakub Kicinski
  2025-06-24  5:57   ` Maxime Chevallier
  2025-06-23 23:17 ` [PATCH net-next v2 3/8] net: ethtool: call .parse_request for SET handlers Jakub Kicinski
                   ` (7 subsequent siblings)
  9 siblings, 1 reply; 20+ messages in thread
From: Jakub Kicinski @ 2025-06-23 23:17 UTC (permalink / raw)
  To: davem
  Cc: netdev, edumazet, pabeni, andrew+netdev, horms, donald.hunter,
	maxime.chevallier, sdf, jdamato, ecree.xilinx, Jakub Kicinski

In preparation for using req_info to carry parameters between
SET and NTF allocate a full request info struct. Since the size
depends on the subcommand we need to allocate it on the heap.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
v2:
 - s/into/info/
---
 net/ethtool/netlink.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index 9de828df46cd..a9467b96f00c 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -863,8 +863,8 @@ static int ethnl_default_done(struct netlink_callback *cb)
 static int ethnl_default_set_doit(struct sk_buff *skb, struct genl_info *info)
 {
 	const struct ethnl_request_ops *ops;
-	struct ethnl_req_info req_info = {};
 	const u8 cmd = info->genlhdr->cmd;
+	struct ethnl_req_info *req_info;
 	struct net_device *dev;
 	int ret;
 
@@ -874,20 +874,24 @@ static int ethnl_default_set_doit(struct sk_buff *skb, struct genl_info *info)
 	if (GENL_REQ_ATTR_CHECK(info, ops->hdr_attr))
 		return -EINVAL;
 
-	ret = ethnl_parse_header_dev_get(&req_info, info->attrs[ops->hdr_attr],
+	req_info = kzalloc(ops->req_info_size, GFP_KERNEL);
+	if (!req_info)
+		return -ENOMEM;
+
+	ret = ethnl_parse_header_dev_get(req_info, info->attrs[ops->hdr_attr],
 					 genl_info_net(info), info->extack,
 					 true);
 	if (ret < 0)
-		return ret;
+		goto out_free_req;
 
 	if (ops->set_validate) {
-		ret = ops->set_validate(&req_info, info);
+		ret = ops->set_validate(req_info, info);
 		/* 0 means nothing to do */
 		if (ret <= 0)
 			goto out_dev;
 	}
 
-	dev = req_info.dev;
+	dev = req_info->dev;
 
 	rtnl_lock();
 	netdev_lock_ops(dev);
@@ -902,7 +906,7 @@ static int ethnl_default_set_doit(struct sk_buff *skb, struct genl_info *info)
 	if (ret < 0)
 		goto out_free_cfg;
 
-	ret = ops->set(&req_info, info);
+	ret = ops->set(req_info, info);
 	if (ret < 0)
 		goto out_ops;
 
@@ -921,7 +925,9 @@ static int ethnl_default_set_doit(struct sk_buff *skb, struct genl_info *info)
 	netdev_unlock_ops(dev);
 	rtnl_unlock();
 out_dev:
-	ethnl_parse_header_dev_put(&req_info);
+	ethnl_parse_header_dev_put(req_info);
+out_free_req:
+	kfree(req_info);
 	return ret;
 }
 
-- 
2.49.0


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

* [PATCH net-next v2 3/8] net: ethtool: call .parse_request for SET handlers
  2025-06-23 23:17 [PATCH net-next v2 0/8] net: ethtool: rss: add notifications Jakub Kicinski
  2025-06-23 23:17 ` [PATCH net-next v2 1/8] netlink: specs: add the multicast group name to spec Jakub Kicinski
  2025-06-23 23:17 ` [PATCH net-next v2 2/8] net: ethtool: dynamically allocate full req size req Jakub Kicinski
@ 2025-06-23 23:17 ` Jakub Kicinski
  2025-06-24  5:57   ` Maxime Chevallier
  2025-06-23 23:17 ` [PATCH net-next v2 4/8] net: ethtool: remove the data argument from ethtool_notify() Jakub Kicinski
                   ` (6 subsequent siblings)
  9 siblings, 1 reply; 20+ messages in thread
From: Jakub Kicinski @ 2025-06-23 23:17 UTC (permalink / raw)
  To: davem
  Cc: netdev, edumazet, pabeni, andrew+netdev, horms, donald.hunter,
	maxime.chevallier, sdf, jdamato, ecree.xilinx, Jakub Kicinski

In preparation for using req_info to carry parameters between SET
and NTF - call .parse_request during ethnl_default_set_doit().

The main question here is whether .parse_request is intended to be
GET-specific. Originally the SET handling was delegated to each subcommand
directly - ethnl_default_set_doit() and .set callbacks in ethnl_request_ops
did not exist. Looking at existing users does not shed much light, all
of the following subcommands use .parse_request but have no SET handler
(and no NTF):

  net/ethtool/eeprom.c
  net/ethtool/rss.c
  net/ethtool/stats.c
  net/ethtool/strset.c
  net/ethtool/tsinfo.c

There's only one which does have a SET:

  net/ethtool/pause.c

where .parse_request handling is used to select which statistics to query.
Not relevant for SET but also harmless.

Going back to RSS (which doesn't have SET today) .parse_request parses
the rss_context ID. Using the req_info struct to pass the context ID
from SET to NTF will be very useful.

Switch to ethnl_default_parse(), effectively adding the .parse_request
for SET handlers.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/ethtool/netlink.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index a9467b96f00c..c5ec3c82ab2e 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -878,9 +878,7 @@ static int ethnl_default_set_doit(struct sk_buff *skb, struct genl_info *info)
 	if (!req_info)
 		return -ENOMEM;
 
-	ret = ethnl_parse_header_dev_get(req_info, info->attrs[ops->hdr_attr],
-					 genl_info_net(info), info->extack,
-					 true);
+	ret = ethnl_default_parse(req_info, info,  ops, true);
 	if (ret < 0)
 		goto out_free_req;
 
-- 
2.49.0


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

* [PATCH net-next v2 4/8] net: ethtool: remove the data argument from ethtool_notify()
  2025-06-23 23:17 [PATCH net-next v2 0/8] net: ethtool: rss: add notifications Jakub Kicinski
                   ` (2 preceding siblings ...)
  2025-06-23 23:17 ` [PATCH net-next v2 3/8] net: ethtool: call .parse_request for SET handlers Jakub Kicinski
@ 2025-06-23 23:17 ` Jakub Kicinski
  2025-06-24  5:56   ` Maxime Chevallier
  2025-06-23 23:17 ` [PATCH net-next v2 5/8] net: ethtool: copy req_info from SET to NTF Jakub Kicinski
                   ` (5 subsequent siblings)
  9 siblings, 1 reply; 20+ messages in thread
From: Jakub Kicinski @ 2025-06-23 23:17 UTC (permalink / raw)
  To: davem
  Cc: netdev, edumazet, pabeni, andrew+netdev, horms, donald.hunter,
	maxime.chevallier, sdf, jdamato, ecree.xilinx, Jakub Kicinski

ethtool_notify() takes a const void *data argument, which presumably
was intended to pass information from the call site to the subcommand
handler. This argument currently has no users.

Expecting the data to be subcommand-specific has two complications.

Complication #1 is that its not plumbed thru any of the standardized
callbacks. It gets propagated to ethnl_default_notify() where it
remains unused. Coming from the ethnl_default_set_doit() side we pass
in NULL, because how could we have a command specific attribute in
a generic handler.

Complication #2 is that we expect the ethtool_notify() callers to
know what attribute type to pass in. Again, the data pointer is
untyped.

RSS will need to pass the context ID to the notifications.
I think it's a better design if the "subcommand" exports its own
typed interface and constructs the appropriate argument struct
(which will be req_info). Remove the unused data argument from
ethtool_notify() but retain it in a new internal helper which
subcommands can use to build a typed interface.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 include/linux/netdevice.h |  5 ++---
 net/ethtool/netlink.h     |  1 +
 net/ethtool/ioctl.c       | 24 ++++++++++++------------
 net/ethtool/netlink.c     | 11 ++++++++---
 4 files changed, 23 insertions(+), 18 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 03c26bb0fbbe..db5bfd4e7ec8 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -5138,10 +5138,9 @@ void netdev_bonding_info_change(struct net_device *dev,
 				struct netdev_bonding_info *bonding_info);
 
 #if IS_ENABLED(CONFIG_ETHTOOL_NETLINK)
-void ethtool_notify(struct net_device *dev, unsigned int cmd, const void *data);
+void ethtool_notify(struct net_device *dev, unsigned int cmd);
 #else
-static inline void ethtool_notify(struct net_device *dev, unsigned int cmd,
-				  const void *data)
+static inline void ethtool_notify(struct net_device *dev, unsigned int cmd)
 {
 }
 #endif
diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h
index 91b953924af3..4a061944a3aa 100644
--- a/net/ethtool/netlink.h
+++ b/net/ethtool/netlink.h
@@ -23,6 +23,7 @@ void *ethnl_dump_put(struct sk_buff *skb, struct netlink_callback *cb, u8 cmd);
 void *ethnl_bcastmsg_put(struct sk_buff *skb, u8 cmd);
 void *ethnl_unicast_put(struct sk_buff *skb, u32 portid, u32 seq, u8 cmd);
 int ethnl_multicast(struct sk_buff *skb, struct net_device *dev);
+void ethnl_notify(struct net_device *dev, unsigned int cmd, const void *data);
 
 /**
  * ethnl_strz_size() - calculate attribute length for fixed size string
diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index 82cde640aa87..96da9d18789b 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -617,8 +617,8 @@ static int ethtool_set_link_ksettings(struct net_device *dev,
 
 	err = dev->ethtool_ops->set_link_ksettings(dev, &link_ksettings);
 	if (err >= 0) {
-		ethtool_notify(dev, ETHTOOL_MSG_LINKINFO_NTF, NULL);
-		ethtool_notify(dev, ETHTOOL_MSG_LINKMODES_NTF, NULL);
+		ethtool_notify(dev, ETHTOOL_MSG_LINKINFO_NTF);
+		ethtool_notify(dev, ETHTOOL_MSG_LINKMODES_NTF);
 	}
 	return err;
 }
@@ -708,8 +708,8 @@ static int ethtool_set_settings(struct net_device *dev, void __user *useraddr)
 		__ETHTOOL_LINK_MODE_MASK_NU32;
 	ret = dev->ethtool_ops->set_link_ksettings(dev, &link_ksettings);
 	if (ret >= 0) {
-		ethtool_notify(dev, ETHTOOL_MSG_LINKINFO_NTF, NULL);
-		ethtool_notify(dev, ETHTOOL_MSG_LINKMODES_NTF, NULL);
+		ethtool_notify(dev, ETHTOOL_MSG_LINKINFO_NTF);
+		ethtool_notify(dev, ETHTOOL_MSG_LINKMODES_NTF);
 	}
 	return ret;
 }
@@ -1868,7 +1868,7 @@ static int ethtool_set_wol(struct net_device *dev, char __user *useraddr)
 		return ret;
 
 	dev->ethtool->wol_enabled = !!wol.wolopts;
-	ethtool_notify(dev, ETHTOOL_MSG_WOL_NTF, NULL);
+	ethtool_notify(dev, ETHTOOL_MSG_WOL_NTF);
 
 	return 0;
 }
@@ -1944,7 +1944,7 @@ static int ethtool_set_eee(struct net_device *dev, char __user *useraddr)
 	eee_to_keee(&keee, &eee);
 	ret = dev->ethtool_ops->set_eee(dev, &keee);
 	if (!ret)
-		ethtool_notify(dev, ETHTOOL_MSG_EEE_NTF, NULL);
+		ethtool_notify(dev, ETHTOOL_MSG_EEE_NTF);
 	return ret;
 }
 
@@ -2184,7 +2184,7 @@ static noinline_for_stack int ethtool_set_coalesce(struct net_device *dev,
 	ret = dev->ethtool_ops->set_coalesce(dev, &coalesce, &kernel_coalesce,
 					     NULL);
 	if (!ret)
-		ethtool_notify(dev, ETHTOOL_MSG_COALESCE_NTF, NULL);
+		ethtool_notify(dev, ETHTOOL_MSG_COALESCE_NTF);
 	return ret;
 }
 
@@ -2228,7 +2228,7 @@ static int ethtool_set_ringparam(struct net_device *dev, void __user *useraddr)
 	ret = dev->ethtool_ops->set_ringparam(dev, &ringparam,
 					      &kernel_ringparam, NULL);
 	if (!ret)
-		ethtool_notify(dev, ETHTOOL_MSG_RINGS_NTF, NULL);
+		ethtool_notify(dev, ETHTOOL_MSG_RINGS_NTF);
 	return ret;
 }
 
@@ -2295,7 +2295,7 @@ static noinline_for_stack int ethtool_set_channels(struct net_device *dev,
 
 	ret = dev->ethtool_ops->set_channels(dev, &channels);
 	if (!ret)
-		ethtool_notify(dev, ETHTOOL_MSG_CHANNELS_NTF, NULL);
+		ethtool_notify(dev, ETHTOOL_MSG_CHANNELS_NTF);
 	return ret;
 }
 
@@ -2326,7 +2326,7 @@ static int ethtool_set_pauseparam(struct net_device *dev, void __user *useraddr)
 
 	ret = dev->ethtool_ops->set_pauseparam(dev, &pauseparam);
 	if (!ret)
-		ethtool_notify(dev, ETHTOOL_MSG_PAUSE_NTF, NULL);
+		ethtool_notify(dev, ETHTOOL_MSG_PAUSE_NTF);
 	return ret;
 }
 
@@ -3328,7 +3328,7 @@ __dev_ethtool(struct net *net, struct ifreq *ifr, void __user *useraddr,
 		rc = ethtool_set_value_void(dev, useraddr,
 				       dev->ethtool_ops->set_msglevel);
 		if (!rc)
-			ethtool_notify(dev, ETHTOOL_MSG_DEBUG_NTF, NULL);
+			ethtool_notify(dev, ETHTOOL_MSG_DEBUG_NTF);
 		break;
 	case ETHTOOL_GEEE:
 		rc = ethtool_get_eee(dev, useraddr);
@@ -3392,7 +3392,7 @@ __dev_ethtool(struct net *net, struct ifreq *ifr, void __user *useraddr,
 		rc = ethtool_get_value(dev, useraddr, ethcmd,
 				       dev->ethtool_ops->get_priv_flags);
 		if (!rc)
-			ethtool_notify(dev, ETHTOOL_MSG_PRIVFLAGS_NTF, NULL);
+			ethtool_notify(dev, ETHTOOL_MSG_PRIVFLAGS_NTF);
 		break;
 	case ETHTOOL_SPFLAGS:
 		rc = ethtool_set_value(dev, useraddr,
diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index c5ec3c82ab2e..129f9d56ac65 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -911,7 +911,7 @@ static int ethnl_default_set_doit(struct sk_buff *skb, struct genl_info *info)
 	swap(dev->cfg, dev->cfg_pending);
 	if (!ret)
 		goto out_ops;
-	ethtool_notify(dev, ops->set_ntf_cmd, NULL);
+	ethtool_notify(dev, ops->set_ntf_cmd);
 
 	ret = 0;
 out_ops:
@@ -1049,7 +1049,7 @@ static const ethnl_notify_handler_t ethnl_notify_handlers[] = {
 	[ETHTOOL_MSG_MM_NTF]		= ethnl_default_notify,
 };
 
-void ethtool_notify(struct net_device *dev, unsigned int cmd, const void *data)
+void ethnl_notify(struct net_device *dev, unsigned int cmd, const void *data)
 {
 	if (unlikely(!ethnl_ok))
 		return;
@@ -1062,13 +1062,18 @@ void ethtool_notify(struct net_device *dev, unsigned int cmd, const void *data)
 		WARN_ONCE(1, "notification %u not implemented (dev=%s)\n",
 			  cmd, netdev_name(dev));
 }
+
+void ethtool_notify(struct net_device *dev, unsigned int cmd)
+{
+	ethnl_notify(dev, cmd, NULL);
+}
 EXPORT_SYMBOL(ethtool_notify);
 
 static void ethnl_notify_features(struct netdev_notifier_info *info)
 {
 	struct net_device *dev = netdev_notifier_info_to_dev(info);
 
-	ethtool_notify(dev, ETHTOOL_MSG_FEATURES_NTF, NULL);
+	ethtool_notify(dev, ETHTOOL_MSG_FEATURES_NTF);
 }
 
 static int ethnl_netdev_event(struct notifier_block *this, unsigned long event,
-- 
2.49.0


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

* [PATCH net-next v2 5/8] net: ethtool: copy req_info from SET to NTF
  2025-06-23 23:17 [PATCH net-next v2 0/8] net: ethtool: rss: add notifications Jakub Kicinski
                   ` (3 preceding siblings ...)
  2025-06-23 23:17 ` [PATCH net-next v2 4/8] net: ethtool: remove the data argument from ethtool_notify() Jakub Kicinski
@ 2025-06-23 23:17 ` Jakub Kicinski
  2025-06-24  5:55   ` Maxime Chevallier
  2025-06-23 23:17 ` [PATCH net-next v2 6/8] net: ethtool: rss: add notifications Jakub Kicinski
                   ` (4 subsequent siblings)
  9 siblings, 1 reply; 20+ messages in thread
From: Jakub Kicinski @ 2025-06-23 23:17 UTC (permalink / raw)
  To: davem
  Cc: netdev, edumazet, pabeni, andrew+netdev, horms, donald.hunter,
	maxime.chevallier, sdf, jdamato, ecree.xilinx, Jakub Kicinski

Copy information parsed for SET with .req_parse to NTF handling
and therefore the GET-equivalent that it ends up executing.
This way if the SET was on a sub-object (like RSS context)
the notification will also be appropriately scoped.

Also copy the phy_index, Maxime suggests this will help PLCA
commands generate accurate notifications as well.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
v2:
 - populate the phy_index
v1: https://lore.kernel.org/20250621171944.2619249-6-kuba@kernel.org
---
 net/ethtool/netlink.h |  5 ++++-
 net/ethtool/netlink.c | 16 +++++++++++-----
 2 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h
index 4a061944a3aa..373a8d5e86ae 100644
--- a/net/ethtool/netlink.h
+++ b/net/ethtool/netlink.h
@@ -23,7 +23,8 @@ void *ethnl_dump_put(struct sk_buff *skb, struct netlink_callback *cb, u8 cmd);
 void *ethnl_bcastmsg_put(struct sk_buff *skb, u8 cmd);
 void *ethnl_unicast_put(struct sk_buff *skb, u32 portid, u32 seq, u8 cmd);
 int ethnl_multicast(struct sk_buff *skb, struct net_device *dev);
-void ethnl_notify(struct net_device *dev, unsigned int cmd, const void *data);
+void ethnl_notify(struct net_device *dev, unsigned int cmd,
+		  const struct ethnl_req_info *req_info);
 
 /**
  * ethnl_strz_size() - calculate attribute length for fixed size string
@@ -338,6 +339,8 @@ int ethnl_sock_priv_set(struct sk_buff *skb, struct net_device *dev, u32 portid,
  *	header is already filled on entry, the rest up to @repdata_offset
  *	is zero initialized. This callback should only modify type specific
  *	request info by parsed attributes from request message.
+ *	Called for both GET and SET. Information parsed for SET will
+ *	be conveyed to the req_info used during NTF generation.
  * @prepare_data:
  *	Retrieve and prepare data needed to compose a reply message. Calls to
  *	ethtool_ops handlers are limited to this callback. Common reply data
diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index 129f9d56ac65..60b3c07507d2 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -911,7 +911,7 @@ static int ethnl_default_set_doit(struct sk_buff *skb, struct genl_info *info)
 	swap(dev->cfg, dev->cfg_pending);
 	if (!ret)
 		goto out_ops;
-	ethtool_notify(dev, ops->set_ntf_cmd);
+	ethnl_notify(dev, ops->set_ntf_cmd, req_info);
 
 	ret = 0;
 out_ops:
@@ -950,7 +950,7 @@ ethnl_default_notify_ops[ETHTOOL_MSG_KERNEL_MAX + 1] = {
 
 /* default notification handler */
 static void ethnl_default_notify(struct net_device *dev, unsigned int cmd,
-				 const void *data)
+				 const struct ethnl_req_info *orig_req_info)
 {
 	struct ethnl_reply_data *reply_data;
 	const struct ethnl_request_ops *ops;
@@ -979,6 +979,11 @@ static void ethnl_default_notify(struct net_device *dev, unsigned int cmd,
 
 	req_info->dev = dev;
 	req_info->flags |= ETHTOOL_FLAG_COMPACT_BITSETS;
+	if (orig_req_info) {
+		req_info->phy_index = orig_req_info->phy_index;
+		memcpy(&req_info[1], &orig_req_info[1],
+		       ops->req_info_size - sizeof(*req_info));
+	}
 
 	netdev_ops_assert_locked(dev);
 
@@ -1029,7 +1034,7 @@ static void ethnl_default_notify(struct net_device *dev, unsigned int cmd,
 /* notifications */
 
 typedef void (*ethnl_notify_handler_t)(struct net_device *dev, unsigned int cmd,
-				       const void *data);
+				       const struct ethnl_req_info *req_info);
 
 static const ethnl_notify_handler_t ethnl_notify_handlers[] = {
 	[ETHTOOL_MSG_LINKINFO_NTF]	= ethnl_default_notify,
@@ -1049,7 +1054,8 @@ static const ethnl_notify_handler_t ethnl_notify_handlers[] = {
 	[ETHTOOL_MSG_MM_NTF]		= ethnl_default_notify,
 };
 
-void ethnl_notify(struct net_device *dev, unsigned int cmd, const void *data)
+void ethnl_notify(struct net_device *dev, unsigned int cmd,
+		  const struct ethnl_req_info *req_info)
 {
 	if (unlikely(!ethnl_ok))
 		return;
@@ -1057,7 +1063,7 @@ void ethnl_notify(struct net_device *dev, unsigned int cmd, const void *data)
 
 	if (likely(cmd < ARRAY_SIZE(ethnl_notify_handlers) &&
 		   ethnl_notify_handlers[cmd]))
-		ethnl_notify_handlers[cmd](dev, cmd, data);
+		ethnl_notify_handlers[cmd](dev, cmd, req_info);
 	else
 		WARN_ONCE(1, "notification %u not implemented (dev=%s)\n",
 			  cmd, netdev_name(dev));
-- 
2.49.0


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

* [PATCH net-next v2 6/8] net: ethtool: rss: add notifications
  2025-06-23 23:17 [PATCH net-next v2 0/8] net: ethtool: rss: add notifications Jakub Kicinski
                   ` (4 preceding siblings ...)
  2025-06-23 23:17 ` [PATCH net-next v2 5/8] net: ethtool: copy req_info from SET to NTF Jakub Kicinski
@ 2025-06-23 23:17 ` Jakub Kicinski
  2025-06-23 23:17 ` [PATCH net-next v2 7/8] doc: ethtool: mark ETHTOOL_GRXFHINDIR as reimplemented Jakub Kicinski
                   ` (3 subsequent siblings)
  9 siblings, 0 replies; 20+ messages in thread
From: Jakub Kicinski @ 2025-06-23 23:17 UTC (permalink / raw)
  To: davem
  Cc: netdev, edumazet, pabeni, andrew+netdev, horms, donald.hunter,
	maxime.chevallier, sdf, jdamato, ecree.xilinx, Jakub Kicinski

In preparation for RSS_SET handling in ethnl introduce Netlink
notifications for RSS. Only cover modifications, not creation
and not removal of a context, because the latter may deserve
a different notification type. We should cross that bridge
when we add the support for context add / remove via Netlink.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
v2:
 - fix compilation with ETHTOOL_NETLINK=n
v1: https://lore.kernel.org/20250621171944.2619249-7-kuba@kernel.org
---
 Documentation/netlink/specs/ethtool.yaml       |  7 +++++++
 Documentation/networking/ethtool-netlink.rst   |  1 +
 include/uapi/linux/ethtool_netlink_generated.h |  1 +
 net/ethtool/common.h                           |  8 ++++++++
 net/ethtool/ioctl.c                            |  4 ++++
 net/ethtool/netlink.c                          |  2 ++
 net/ethtool/rss.c                              | 11 +++++++++++
 7 files changed, 34 insertions(+)

diff --git a/Documentation/netlink/specs/ethtool.yaml b/Documentation/netlink/specs/ethtool.yaml
index cfe84f84ba29..19a32229772a 100644
--- a/Documentation/netlink/specs/ethtool.yaml
+++ b/Documentation/netlink/specs/ethtool.yaml
@@ -2492,6 +2492,13 @@ c-version-name: ethtool-genl-version
         attributes:
           - header
           - events
+    -
+      name: rss-ntf
+      doc: |
+        Notification for change in RSS configuration.
+        For additional contexts only modifications are modified, not creation
+        or removal of the contexts.
+      notify: rss-get
 
 mcast-groups:
   list:
diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
index e45bb555e909..08abca99a6dc 100644
--- a/Documentation/networking/ethtool-netlink.rst
+++ b/Documentation/networking/ethtool-netlink.rst
@@ -281,6 +281,7 @@ All constants identifying message types use ``ETHTOOL_CMD_`` prefix and suffix
   ``ETHTOOL_MSG_MODULE_GET_REPLY``         transceiver module parameters
   ``ETHTOOL_MSG_PSE_GET_REPLY``            PSE parameters
   ``ETHTOOL_MSG_RSS_GET_REPLY``            RSS settings
+  ``ETHTOOL_MSG_RSS_NTF``                  RSS settings
   ``ETHTOOL_MSG_PLCA_GET_CFG_REPLY``       PLCA RS parameters
   ``ETHTOOL_MSG_PLCA_GET_STATUS_REPLY``    PLCA RS status
   ``ETHTOOL_MSG_PLCA_NTF``                 PLCA RS parameters
diff --git a/include/uapi/linux/ethtool_netlink_generated.h b/include/uapi/linux/ethtool_netlink_generated.h
index 859e28c8a91a..8f30ffa1cd14 100644
--- a/include/uapi/linux/ethtool_netlink_generated.h
+++ b/include/uapi/linux/ethtool_netlink_generated.h
@@ -862,6 +862,7 @@ enum {
 	ETHTOOL_MSG_TSCONFIG_GET_REPLY,
 	ETHTOOL_MSG_TSCONFIG_SET_REPLY,
 	ETHTOOL_MSG_PSE_NTF,
+	ETHTOOL_MSG_RSS_NTF,
 
 	__ETHTOOL_MSG_KERNEL_CNT,
 	ETHTOOL_MSG_KERNEL_MAX = (__ETHTOOL_MSG_KERNEL_CNT - 1)
diff --git a/net/ethtool/common.h b/net/ethtool/common.h
index b4683d286a5a..c41db1595621 100644
--- a/net/ethtool/common.h
+++ b/net/ethtool/common.h
@@ -74,4 +74,12 @@ int ethtool_get_module_eeprom_call(struct net_device *dev,
 
 bool __ethtool_dev_mm_supported(struct net_device *dev);
 
+#if IS_ENABLED(CONFIG_ETHTOOL_NETLINK)
+void ethtool_rss_notify(struct net_device *dev, u32 rss_context);
+#else
+static inline void ethtool_rss_notify(struct net_device *dev, u32 rss_context)
+{
+}
+#endif
+
 #endif /* _ETHTOOL_COMMON_H */
diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index 96da9d18789b..c34bac7bffd8 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -1502,6 +1502,7 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
 	struct ethtool_rxfh rxfh;
 	bool locked = false; /* dev->ethtool->rss_lock taken */
 	bool create = false;
+	bool mod = false;
 	u8 *rss_config;
 	int ret;
 
@@ -1688,6 +1689,7 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
 		}
 		goto out;
 	}
+	mod = !create && !rxfh_dev.rss_delete;
 
 	if (copy_to_user(useraddr + offsetof(struct ethtool_rxfh, rss_context),
 			 &rxfh_dev.rss_context, sizeof(rxfh_dev.rss_context)))
@@ -1757,6 +1759,8 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
 	if (locked)
 		mutex_unlock(&dev->ethtool->rss_lock);
 	kfree(rss_config);
+	if (mod)
+		ethtool_rss_notify(dev, rxfh.rss_context);
 	return ret;
 }
 
diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index 60b3c07507d2..09c81cc9a08f 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -946,6 +946,7 @@ ethnl_default_notify_ops[ETHTOOL_MSG_KERNEL_MAX + 1] = {
 	[ETHTOOL_MSG_MODULE_NTF]	= &ethnl_module_request_ops,
 	[ETHTOOL_MSG_PLCA_NTF]		= &ethnl_plca_cfg_request_ops,
 	[ETHTOOL_MSG_MM_NTF]		= &ethnl_mm_request_ops,
+	[ETHTOOL_MSG_RSS_NTF]		= &ethnl_rss_request_ops,
 };
 
 /* default notification handler */
@@ -1052,6 +1053,7 @@ static const ethnl_notify_handler_t ethnl_notify_handlers[] = {
 	[ETHTOOL_MSG_MODULE_NTF]	= ethnl_default_notify,
 	[ETHTOOL_MSG_PLCA_NTF]		= ethnl_default_notify,
 	[ETHTOOL_MSG_MM_NTF]		= ethnl_default_notify,
+	[ETHTOOL_MSG_RSS_NTF]		= ethnl_default_notify,
 };
 
 void ethnl_notify(struct net_device *dev, unsigned int cmd,
diff --git a/net/ethtool/rss.c b/net/ethtool/rss.c
index 6d9b1769896b..3adddca7e215 100644
--- a/net/ethtool/rss.c
+++ b/net/ethtool/rss.c
@@ -358,6 +358,17 @@ int ethnl_rss_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
 	return ret;
 }
 
+/* RSS_NTF */
+
+void ethtool_rss_notify(struct net_device *dev, u32 rss_context)
+{
+	struct rss_req_info req_info = {
+		.rss_context = rss_context,
+	};
+
+	ethnl_notify(dev, ETHTOOL_MSG_RSS_NTF, &req_info.base);
+}
+
 const struct ethnl_request_ops ethnl_rss_request_ops = {
 	.request_cmd		= ETHTOOL_MSG_RSS_GET,
 	.reply_cmd		= ETHTOOL_MSG_RSS_GET_REPLY,
-- 
2.49.0


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

* [PATCH net-next v2 7/8] doc: ethtool: mark ETHTOOL_GRXFHINDIR as reimplemented
  2025-06-23 23:17 [PATCH net-next v2 0/8] net: ethtool: rss: add notifications Jakub Kicinski
                   ` (5 preceding siblings ...)
  2025-06-23 23:17 ` [PATCH net-next v2 6/8] net: ethtool: rss: add notifications Jakub Kicinski
@ 2025-06-23 23:17 ` Jakub Kicinski
  2025-06-25  8:55   ` Donald Hunter
  2025-06-23 23:17 ` [PATCH net-next v2 8/8] selftests: drv-net: test RSS Netlink notifications Jakub Kicinski
                   ` (2 subsequent siblings)
  9 siblings, 1 reply; 20+ messages in thread
From: Jakub Kicinski @ 2025-06-23 23:17 UTC (permalink / raw)
  To: davem
  Cc: netdev, edumazet, pabeni, andrew+netdev, horms, donald.hunter,
	maxime.chevallier, sdf, jdamato, ecree.xilinx, Jakub Kicinski

The ETHTOOL_GRXFHINDIR reimplementation has been completed around
a year ago. We have been tweaking it so a bit hard to point
to a single commit that completed it, but all the fields available
in IOCTL are reported via Netlink.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 Documentation/networking/ethtool-netlink.rst | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
index 08abca99a6dc..07e9808ebd2c 100644
--- a/Documentation/networking/ethtool-netlink.rst
+++ b/Documentation/networking/ethtool-netlink.rst
@@ -2451,7 +2451,7 @@ are netlink only.
   ``ETHTOOL_SRXNTUPLE``               n/a
   ``ETHTOOL_GRXNTUPLE``               n/a
   ``ETHTOOL_GSSET_INFO``              ``ETHTOOL_MSG_STRSET_GET``
-  ``ETHTOOL_GRXFHINDIR``              n/a
+  ``ETHTOOL_GRXFHINDIR``              ``ETHTOOL_MSG_RSS_GET``
   ``ETHTOOL_SRXFHINDIR``              n/a
   ``ETHTOOL_GFEATURES``               ``ETHTOOL_MSG_FEATURES_GET``
   ``ETHTOOL_SFEATURES``               ``ETHTOOL_MSG_FEATURES_SET``
-- 
2.49.0


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

* [PATCH net-next v2 8/8] selftests: drv-net: test RSS Netlink notifications
  2025-06-23 23:17 [PATCH net-next v2 0/8] net: ethtool: rss: add notifications Jakub Kicinski
                   ` (6 preceding siblings ...)
  2025-06-23 23:17 ` [PATCH net-next v2 7/8] doc: ethtool: mark ETHTOOL_GRXFHINDIR as reimplemented Jakub Kicinski
@ 2025-06-23 23:17 ` Jakub Kicinski
  2025-06-25  9:46   ` Donald Hunter
  2025-06-24  6:00 ` [PATCH net-next v2 0/8] net: ethtool: rss: add notifications Maxime Chevallier
  2025-06-25 22:50 ` patchwork-bot+netdevbpf
  9 siblings, 1 reply; 20+ messages in thread
From: Jakub Kicinski @ 2025-06-23 23:17 UTC (permalink / raw)
  To: davem
  Cc: netdev, edumazet, pabeni, andrew+netdev, horms, donald.hunter,
	maxime.chevallier, sdf, jdamato, ecree.xilinx, Jakub Kicinski

Test that changing the RSS config generates Netlink notifications.

  # ./tools/testing/selftests/drivers/net/hw/rss_api.py
  TAP version 13
  1..2
  ok 1 rss_api.test_rxfh_indir_ntf
  ok 2 rss_api.test_rxfh_indir_ctx_ntf
  # Totals: pass:2 fail:0 xfail:0 xpass:0 skip:0 error:0

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 .../selftests/drivers/net/hw/rss_api.py       | 89 +++++++++++++++++++
 1 file changed, 89 insertions(+)
 create mode 100755 tools/testing/selftests/drivers/net/hw/rss_api.py

diff --git a/tools/testing/selftests/drivers/net/hw/rss_api.py b/tools/testing/selftests/drivers/net/hw/rss_api.py
new file mode 100755
index 000000000000..db0f723a674b
--- /dev/null
+++ b/tools/testing/selftests/drivers/net/hw/rss_api.py
@@ -0,0 +1,89 @@
+#!/usr/bin/env python3
+# SPDX-License-Identifier: GPL-2.0
+
+"""
+API level tests for RSS (mostly Netlink vs IOCTL).
+"""
+
+import glob
+from lib.py import ksft_run, ksft_exit, ksft_eq, ksft_is, ksft_ne
+from lib.py import KsftSkipEx, KsftFailEx
+from lib.py import defer, ethtool
+from lib.py import EthtoolFamily
+from lib.py import NetDrvEnv
+
+
+def _ethtool_create(cfg, act, opts):
+    output = ethtool(f"{act} {cfg.ifname} {opts}").stdout
+    # Output will be something like: "New RSS context is 1" or
+    # "Added rule with ID 7", we want the integer from the end
+    return int(output.split()[-1])
+
+
+def test_rxfh_indir_ntf(cfg):
+    """
+    Check that Netlink notifications are generated when RSS indirection
+    table was modified.
+    """
+
+    qcnt = len(glob.glob(f"/sys/class/net/{cfg.ifname}/queues/rx-*"))
+    if qcnt < 2:
+        raise KsftSkipEx(f"Local has only {qcnt} queues")
+
+    ethnl = EthtoolFamily()
+    ethnl.ntf_subscribe("monitor")
+
+    ethtool(f"--disable-netlink -X {cfg.ifname} weight 0 1")
+    reset = defer(ethtool, f"-X {cfg.ifname} default")
+
+    ntf = next(ethnl.poll_ntf(duration=0.2), None)
+    if ntf is None:
+        raise KsftFailEx("No notification received")
+    ksft_eq(ntf["name"], "rss-ntf")
+    ksft_eq(set(ntf["msg"]["indir"]), {1})
+
+    reset.exec()
+    ntf = next(ethnl.poll_ntf(duration=0.2), None)
+    if ntf is None:
+        raise KsftFailEx("No notification received after reset")
+    ksft_eq(ntf["name"], "rss-ntf")
+    ksft_is(ntf["msg"].get("context"), None)
+    ksft_ne(set(ntf["msg"]["indir"]), {1})
+
+
+def test_rxfh_indir_ctx_ntf(cfg):
+    """
+    Check that Netlink notifications are generated when RSS indirection
+    table was modified on an additional RSS context.
+    """
+
+    qcnt = len(glob.glob(f"/sys/class/net/{cfg.ifname}/queues/rx-*"))
+    if qcnt < 2:
+        raise KsftSkipEx(f"Local has only {qcnt} queues")
+
+    ctx_id = _ethtool_create(cfg, "-X", "context new")
+    defer(ethtool, f"-X {cfg.ifname} context {ctx_id} delete")
+
+    ethnl = EthtoolFamily()
+    ethnl.ntf_subscribe("monitor")
+
+    ethtool(f"--disable-netlink -X {cfg.ifname} context {ctx_id} weight 0 1")
+
+    ntf = next(ethnl.poll_ntf(duration=0.2), None)
+    if ntf is None:
+        raise KsftFailEx("No notification received")
+    ksft_eq(ntf["name"], "rss-ntf")
+    ksft_eq(ntf["msg"].get("context"), ctx_id)
+    ksft_eq(set(ntf["msg"]["indir"]), {1})
+
+
+def main() -> None:
+    """ Ksft boiler plate main """
+
+    with NetDrvEnv(__file__, nsim_test=False) as cfg:
+        ksft_run(globs=globals(), case_pfx={"test_"}, args=(cfg, ))
+    ksft_exit()
+
+
+if __name__ == "__main__":
+    main()
-- 
2.49.0


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

* Re: [PATCH net-next v2 5/8] net: ethtool: copy req_info from SET to NTF
  2025-06-23 23:17 ` [PATCH net-next v2 5/8] net: ethtool: copy req_info from SET to NTF Jakub Kicinski
@ 2025-06-24  5:55   ` Maxime Chevallier
  0 siblings, 0 replies; 20+ messages in thread
From: Maxime Chevallier @ 2025-06-24  5:55 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: davem, netdev, edumazet, pabeni, andrew+netdev, horms,
	donald.hunter, sdf, jdamato, ecree.xilinx

Hi Jakub,

On Mon, 23 Jun 2025 16:17:17 -0700
Jakub Kicinski <kuba@kernel.org> wrote:

> Copy information parsed for SET with .req_parse to NTF handling
> and therefore the GET-equivalent that it ends up executing.
> This way if the SET was on a sub-object (like RSS context)
> the notification will also be appropriately scoped.
> 
> Also copy the phy_index, Maxime suggests this will help PLCA
> commands generate accurate notifications as well.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

It works perfectly for PLCA, now the notif is generated, and for the
right PHY :) thanks !

Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
Tested-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Maxime

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

* Re: [PATCH net-next v2 4/8] net: ethtool: remove the data argument from ethtool_notify()
  2025-06-23 23:17 ` [PATCH net-next v2 4/8] net: ethtool: remove the data argument from ethtool_notify() Jakub Kicinski
@ 2025-06-24  5:56   ` Maxime Chevallier
  0 siblings, 0 replies; 20+ messages in thread
From: Maxime Chevallier @ 2025-06-24  5:56 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: davem, netdev, edumazet, pabeni, andrew+netdev, horms,
	donald.hunter, sdf, jdamato, ecree.xilinx

On Mon, 23 Jun 2025 16:17:16 -0700
Jakub Kicinski <kuba@kernel.org> wrote:

> ethtool_notify() takes a const void *data argument, which presumably
> was intended to pass information from the call site to the subcommand
> handler. This argument currently has no users.
> 
> Expecting the data to be subcommand-specific has two complications.
> 
> Complication #1 is that its not plumbed thru any of the standardized
> callbacks. It gets propagated to ethnl_default_notify() where it
> remains unused. Coming from the ethnl_default_set_doit() side we pass
> in NULL, because how could we have a command specific attribute in
> a generic handler.
> 
> Complication #2 is that we expect the ethtool_notify() callers to
> know what attribute type to pass in. Again, the data pointer is
> untyped.
> 
> RSS will need to pass the context ID to the notifications.
> I think it's a better design if the "subcommand" exports its own
> typed interface and constructs the appropriate argument struct
> (which will be req_info). Remove the unused data argument from
> ethtool_notify() but retain it in a new internal helper which
> subcommands can use to build a typed interface.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
Tested-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Maxime

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

* Re: [PATCH net-next v2 3/8] net: ethtool: call .parse_request for SET handlers
  2025-06-23 23:17 ` [PATCH net-next v2 3/8] net: ethtool: call .parse_request for SET handlers Jakub Kicinski
@ 2025-06-24  5:57   ` Maxime Chevallier
  0 siblings, 0 replies; 20+ messages in thread
From: Maxime Chevallier @ 2025-06-24  5:57 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: davem, netdev, edumazet, pabeni, andrew+netdev, horms,
	donald.hunter, sdf, jdamato, ecree.xilinx

On Mon, 23 Jun 2025 16:17:15 -0700
Jakub Kicinski <kuba@kernel.org> wrote:

> In preparation for using req_info to carry parameters between SET
> and NTF - call .parse_request during ethnl_default_set_doit().
> 
> The main question here is whether .parse_request is intended to be
> GET-specific. Originally the SET handling was delegated to each subcommand
> directly - ethnl_default_set_doit() and .set callbacks in ethnl_request_ops
> did not exist. Looking at existing users does not shed much light, all
> of the following subcommands use .parse_request but have no SET handler
> (and no NTF):
> 
>   net/ethtool/eeprom.c
>   net/ethtool/rss.c
>   net/ethtool/stats.c
>   net/ethtool/strset.c
>   net/ethtool/tsinfo.c
> 
> There's only one which does have a SET:
> 
>   net/ethtool/pause.c
> 
> where .parse_request handling is used to select which statistics to query.
> Not relevant for SET but also harmless.
> 
> Going back to RSS (which doesn't have SET today) .parse_request parses
> the rss_context ID. Using the req_info struct to pass the context ID
> from SET to NTF will be very useful.
> 
> Switch to ethnl_default_parse(), effectively adding the .parse_request
> for SET handlers.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
Tested-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Maxime

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

* Re: [PATCH net-next v2 2/8] net: ethtool: dynamically allocate full req size req
  2025-06-23 23:17 ` [PATCH net-next v2 2/8] net: ethtool: dynamically allocate full req size req Jakub Kicinski
@ 2025-06-24  5:57   ` Maxime Chevallier
  0 siblings, 0 replies; 20+ messages in thread
From: Maxime Chevallier @ 2025-06-24  5:57 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: davem, netdev, edumazet, pabeni, andrew+netdev, horms,
	donald.hunter, sdf, jdamato, ecree.xilinx

On Mon, 23 Jun 2025 16:17:14 -0700
Jakub Kicinski <kuba@kernel.org> wrote:

> In preparation for using req_info to carry parameters between
> SET and NTF allocate a full request info struct. Since the size
> depends on the subcommand we need to allocate it on the heap.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
Tested-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Maxime

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

* Re: [PATCH net-next v2 0/8] net: ethtool: rss: add notifications
  2025-06-23 23:17 [PATCH net-next v2 0/8] net: ethtool: rss: add notifications Jakub Kicinski
                   ` (7 preceding siblings ...)
  2025-06-23 23:17 ` [PATCH net-next v2 8/8] selftests: drv-net: test RSS Netlink notifications Jakub Kicinski
@ 2025-06-24  6:00 ` Maxime Chevallier
  2025-06-24 14:13   ` Jakub Kicinski
  2025-06-25 22:50 ` patchwork-bot+netdevbpf
  9 siblings, 1 reply; 20+ messages in thread
From: Maxime Chevallier @ 2025-06-24  6:00 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: davem, netdev, edumazet, pabeni, andrew+netdev, horms,
	donald.hunter, sdf, jdamato, ecree.xilinx

Hi Jakub,

On Mon, 23 Jun 2025 16:17:12 -0700
Jakub Kicinski <kuba@kernel.org> wrote:

> Next step on the path to moving RSS config to Netlink. With the
> refactoring of the driver-facing API for ETHTOOL_GRXFH/ETHTOOL_SRXFH
> out of the way we can move on to more interesting work.
> 
> Add Netlink notifications for changes in RSS configuration.
> 
> As a reminder (part) of rss-get was introduced in previous releases
> when input-xfrm (symmetric hashing) was added. rss-set isn't
> implemented, yet, but we can implement rss-ntf and hook it into
> the changes done via the IOCTL path (same as other ethtool-nl
> notifications do).
> 
> Most of the series is concerned with passing arguments to notifications.
> So far none of the notifications needed to be parametrized, but RSS can
> have multiple contexts per device, and since GET operates on a single
> context at a time, the notification needs to also be scoped to a context.
> Patches 2-5 add support for passing arguments to notifications thru
> ethtool-nl generic infra.
> 
> The notification handling itself is pretty trivial, it's mostly
> hooking in the right entries into the ethool-nl op tables.

I was able to test the PLCA path, which works fine :)

So, I reviewed and tested some of the ethnl patches in this series,
though I wasn't able to test the RSS-specific aspects.

Thanks,

Maxime

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

* Re: [PATCH net-next v2 0/8] net: ethtool: rss: add notifications
  2025-06-24  6:00 ` [PATCH net-next v2 0/8] net: ethtool: rss: add notifications Maxime Chevallier
@ 2025-06-24 14:13   ` Jakub Kicinski
  0 siblings, 0 replies; 20+ messages in thread
From: Jakub Kicinski @ 2025-06-24 14:13 UTC (permalink / raw)
  To: Maxime Chevallier
  Cc: davem, netdev, edumazet, pabeni, andrew+netdev, horms,
	donald.hunter, sdf, jdamato, ecree.xilinx

On Tue, 24 Jun 2025 08:00:04 +0200 Maxime Chevallier wrote:
> I was able to test the PLCA path, which works fine :)
> 
> So, I reviewed and tested some of the ethnl patches in this series,
> though I wasn't able to test the RSS-specific aspects.

Excellent, thank you!

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

* Re: [PATCH net-next v2 7/8] doc: ethtool: mark ETHTOOL_GRXFHINDIR as reimplemented
  2025-06-23 23:17 ` [PATCH net-next v2 7/8] doc: ethtool: mark ETHTOOL_GRXFHINDIR as reimplemented Jakub Kicinski
@ 2025-06-25  8:55   ` Donald Hunter
  0 siblings, 0 replies; 20+ messages in thread
From: Donald Hunter @ 2025-06-25  8:55 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: davem, netdev, edumazet, pabeni, andrew+netdev, horms,
	maxime.chevallier, sdf, jdamato, ecree.xilinx

Jakub Kicinski <kuba@kernel.org> writes:

> The ETHTOOL_GRXFHINDIR reimplementation has been completed around
> a year ago. We have been tweaking it so a bit hard to point
> to a single commit that completed it, but all the fields available
> in IOCTL are reported via Netlink.
>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Reviewed-by: Donald Hunter <donald.hunter@gmail.com>

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

* Re: [PATCH net-next v2 8/8] selftests: drv-net: test RSS Netlink notifications
  2025-06-23 23:17 ` [PATCH net-next v2 8/8] selftests: drv-net: test RSS Netlink notifications Jakub Kicinski
@ 2025-06-25  9:46   ` Donald Hunter
  2025-06-25 20:04     ` Jakub Kicinski
  0 siblings, 1 reply; 20+ messages in thread
From: Donald Hunter @ 2025-06-25  9:46 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: davem, netdev, edumazet, pabeni, andrew+netdev, horms,
	maxime.chevallier, sdf, jdamato, ecree.xilinx

Jakub Kicinski <kuba@kernel.org> writes:

> Test that changing the RSS config generates Netlink notifications.
>
>   # ./tools/testing/selftests/drivers/net/hw/rss_api.py
>   TAP version 13
>   1..2
>   ok 1 rss_api.test_rxfh_indir_ntf
>   ok 2 rss_api.test_rxfh_indir_ctx_ntf
>   # Totals: pass:2 fail:0 xfail:0 xpass:0 skip:0 error:0
>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  .../selftests/drivers/net/hw/rss_api.py       | 89 +++++++++++++++++++
>  1 file changed, 89 insertions(+)
>  create mode 100755 tools/testing/selftests/drivers/net/hw/rss_api.py
>
> diff --git a/tools/testing/selftests/drivers/net/hw/rss_api.py b/tools/testing/selftests/drivers/net/hw/rss_api.py
> new file mode 100755
> index 000000000000..db0f723a674b
> --- /dev/null
> +++ b/tools/testing/selftests/drivers/net/hw/rss_api.py
> @@ -0,0 +1,89 @@
> +#!/usr/bin/env python3
> +# SPDX-License-Identifier: GPL-2.0
> +
> +"""
> +API level tests for RSS (mostly Netlink vs IOCTL).
> +"""
> +
> +import glob
> +from lib.py import ksft_run, ksft_exit, ksft_eq, ksft_is, ksft_ne
> +from lib.py import KsftSkipEx, KsftFailEx
> +from lib.py import defer, ethtool
> +from lib.py import EthtoolFamily
> +from lib.py import NetDrvEnv
> +
> +
> +def _ethtool_create(cfg, act, opts):
> +    output = ethtool(f"{act} {cfg.ifname} {opts}").stdout
> +    # Output will be something like: "New RSS context is 1" or
> +    # "Added rule with ID 7", we want the integer from the end
> +    return int(output.split()[-1])

I think .split() is not required because you can access strings as
arrays.

Will this only ever need to handle single digit values?

> +def test_rxfh_indir_ntf(cfg):
> +    """
> +    Check that Netlink notifications are generated when RSS indirection
> +    table was modified.
> +    """
> +
> +    qcnt = len(glob.glob(f"/sys/class/net/{cfg.ifname}/queues/rx-*"))
> +    if qcnt < 2:
> +        raise KsftSkipEx(f"Local has only {qcnt} queues")
> +
> +    ethnl = EthtoolFamily()
> +    ethnl.ntf_subscribe("monitor")
> +
> +    ethtool(f"--disable-netlink -X {cfg.ifname} weight 0 1")
> +    reset = defer(ethtool, f"-X {cfg.ifname} default")
> +
> +    ntf = next(ethnl.poll_ntf(duration=0.2), None)
> +    if ntf is None:
> +        raise KsftFailEx("No notification received")
> +    ksft_eq(ntf["name"], "rss-ntf")
> +    ksft_eq(set(ntf["msg"]["indir"]), {1})
> +
> +    reset.exec()
> +    ntf = next(ethnl.poll_ntf(duration=0.2), None)
> +    if ntf is None:
> +        raise KsftFailEx("No notification received after reset")
> +    ksft_eq(ntf["name"], "rss-ntf")
> +    ksft_is(ntf["msg"].get("context"), None)
> +    ksft_ne(set(ntf["msg"]["indir"]), {1})
> +
> +
> +def test_rxfh_indir_ctx_ntf(cfg):
> +    """
> +    Check that Netlink notifications are generated when RSS indirection
> +    table was modified on an additional RSS context.
> +    """
> +
> +    qcnt = len(glob.glob(f"/sys/class/net/{cfg.ifname}/queues/rx-*"))
> +    if qcnt < 2:
> +        raise KsftSkipEx(f"Local has only {qcnt} queues")
> +
> +    ctx_id = _ethtool_create(cfg, "-X", "context new")
> +    defer(ethtool, f"-X {cfg.ifname} context {ctx_id} delete")
> +
> +    ethnl = EthtoolFamily()
> +    ethnl.ntf_subscribe("monitor")
> +
> +    ethtool(f"--disable-netlink -X {cfg.ifname} context {ctx_id} weight 0 1")
> +
> +    ntf = next(ethnl.poll_ntf(duration=0.2), None)
> +    if ntf is None:
> +        raise KsftFailEx("No notification received")
> +    ksft_eq(ntf["name"], "rss-ntf")
> +    ksft_eq(ntf["msg"].get("context"), ctx_id)
> +    ksft_eq(set(ntf["msg"]["indir"]), {1})
> +
> +
> +def main() -> None:
> +    """ Ksft boiler plate main """
> +
> +    with NetDrvEnv(__file__, nsim_test=False) as cfg:
> +        ksft_run(globs=globals(), case_pfx={"test_"}, args=(cfg, ))
> +    ksft_exit()
> +
> +
> +if __name__ == "__main__":
> +    main()

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

* Re: [PATCH net-next v2 8/8] selftests: drv-net: test RSS Netlink notifications
  2025-06-25  9:46   ` Donald Hunter
@ 2025-06-25 20:04     ` Jakub Kicinski
  2025-06-25 21:55       ` Donald Hunter
  0 siblings, 1 reply; 20+ messages in thread
From: Jakub Kicinski @ 2025-06-25 20:04 UTC (permalink / raw)
  To: Donald Hunter
  Cc: davem, netdev, edumazet, pabeni, andrew+netdev, horms,
	maxime.chevallier, sdf, jdamato, ecree.xilinx

On Wed, 25 Jun 2025 10:46:53 +0100 Donald Hunter wrote:
> > +def _ethtool_create(cfg, act, opts):
> > +    output = ethtool(f"{act} {cfg.ifname} {opts}").stdout
> > +    # Output will be something like: "New RSS context is 1" or
> > +    # "Added rule with ID 7", we want the integer from the end
> > +    return int(output.split()[-1])  
> 
> I think .split() is not required because you can access strings as
> arrays.
> 
> Will this only ever need to handle single digit values?

nosir, IIUC split splits on whitespace, so from:

	"Added rule with ID 7"  -> [.."ID", "7"]
	"Added rule with ID 71" -> [.."ID", "71"]

and we take the last elem, the ID. We use similar code in the rss_ctx
test, I think it works..

Unfortunately there is no plan to migrate the flow steering to netlink.
And ethtool only supports JSON output in the netlink code :S
Mountains of technical debt :)

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

* Re: [PATCH net-next v2 8/8] selftests: drv-net: test RSS Netlink notifications
  2025-06-25 20:04     ` Jakub Kicinski
@ 2025-06-25 21:55       ` Donald Hunter
  0 siblings, 0 replies; 20+ messages in thread
From: Donald Hunter @ 2025-06-25 21:55 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: davem, netdev, edumazet, pabeni, andrew+netdev, horms,
	maxime.chevallier, sdf, jdamato, ecree.xilinx

On Wed, 25 Jun 2025 at 21:04, Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Wed, 25 Jun 2025 10:46:53 +0100 Donald Hunter wrote:
> > > +def _ethtool_create(cfg, act, opts):
> > > +    output = ethtool(f"{act} {cfg.ifname} {opts}").stdout
> > > +    # Output will be something like: "New RSS context is 1" or
> > > +    # "Added rule with ID 7", we want the integer from the end
> > > +    return int(output.split()[-1])
> >
> > I think .split() is not required because you can access strings as
> > arrays.
> >
> > Will this only ever need to handle single digit values?
>
> nosir, IIUC split splits on whitespace, so from:
>
>         "Added rule with ID 7"  -> [.."ID", "7"]
>         "Added rule with ID 71" -> [.."ID", "71"]
>
> and we take the last elem, the ID. We use similar code in the rss_ctx
> test, I think it works..

Ah, my bad. Ignore my rambling and sorry for the noise.

> Unfortunately there is no plan to migrate the flow steering to netlink.
> And ethtool only supports JSON output in the netlink code :S
> Mountains of technical debt :)

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

* Re: [PATCH net-next v2 0/8] net: ethtool: rss: add notifications
  2025-06-23 23:17 [PATCH net-next v2 0/8] net: ethtool: rss: add notifications Jakub Kicinski
                   ` (8 preceding siblings ...)
  2025-06-24  6:00 ` [PATCH net-next v2 0/8] net: ethtool: rss: add notifications Maxime Chevallier
@ 2025-06-25 22:50 ` patchwork-bot+netdevbpf
  9 siblings, 0 replies; 20+ messages in thread
From: patchwork-bot+netdevbpf @ 2025-06-25 22:50 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: davem, netdev, edumazet, pabeni, andrew+netdev, horms,
	donald.hunter, maxime.chevallier, sdf, jdamato, ecree.xilinx

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Mon, 23 Jun 2025 16:17:12 -0700 you wrote:
> Next step on the path to moving RSS config to Netlink. With the
> refactoring of the driver-facing API for ETHTOOL_GRXFH/ETHTOOL_SRXFH
> out of the way we can move on to more interesting work.
> 
> Add Netlink notifications for changes in RSS configuration.
> 
> As a reminder (part) of rss-get was introduced in previous releases
> when input-xfrm (symmetric hashing) was added. rss-set isn't
> implemented, yet, but we can implement rss-ntf and hook it into
> the changes done via the IOCTL path (same as other ethtool-nl
> notifications do).
> 
> [...]

Here is the summary with links:
  - [net-next,v2,1/8] netlink: specs: add the multicast group name to spec
    https://git.kernel.org/netdev/net-next/c/826334359eac
  - [net-next,v2,2/8] net: ethtool: dynamically allocate full req size req
    https://git.kernel.org/netdev/net-next/c/ceca0769e87f
  - [net-next,v2,3/8] net: ethtool: call .parse_request for SET handlers
    https://git.kernel.org/netdev/net-next/c/963781bdfe20
  - [net-next,v2,4/8] net: ethtool: remove the data argument from ethtool_notify()
    https://git.kernel.org/netdev/net-next/c/f9dc3e52d821
  - [net-next,v2,5/8] net: ethtool: copy req_info from SET to NTF
    https://git.kernel.org/netdev/net-next/c/3073947de382
  - [net-next,v2,6/8] net: ethtool: rss: add notifications
    https://git.kernel.org/netdev/net-next/c/46837be5afc6
  - [net-next,v2,7/8] doc: ethtool: mark ETHTOOL_GRXFHINDIR as reimplemented
    https://git.kernel.org/netdev/net-next/c/47c3ed01af43
  - [net-next,v2,8/8] selftests: drv-net: test RSS Netlink notifications
    https://git.kernel.org/netdev/net-next/c/4d13c6c449af

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



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

end of thread, other threads:[~2025-06-25 22:50 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-06-23 23:17 [PATCH net-next v2 0/8] net: ethtool: rss: add notifications Jakub Kicinski
2025-06-23 23:17 ` [PATCH net-next v2 1/8] netlink: specs: add the multicast group name to spec Jakub Kicinski
2025-06-23 23:17 ` [PATCH net-next v2 2/8] net: ethtool: dynamically allocate full req size req Jakub Kicinski
2025-06-24  5:57   ` Maxime Chevallier
2025-06-23 23:17 ` [PATCH net-next v2 3/8] net: ethtool: call .parse_request for SET handlers Jakub Kicinski
2025-06-24  5:57   ` Maxime Chevallier
2025-06-23 23:17 ` [PATCH net-next v2 4/8] net: ethtool: remove the data argument from ethtool_notify() Jakub Kicinski
2025-06-24  5:56   ` Maxime Chevallier
2025-06-23 23:17 ` [PATCH net-next v2 5/8] net: ethtool: copy req_info from SET to NTF Jakub Kicinski
2025-06-24  5:55   ` Maxime Chevallier
2025-06-23 23:17 ` [PATCH net-next v2 6/8] net: ethtool: rss: add notifications Jakub Kicinski
2025-06-23 23:17 ` [PATCH net-next v2 7/8] doc: ethtool: mark ETHTOOL_GRXFHINDIR as reimplemented Jakub Kicinski
2025-06-25  8:55   ` Donald Hunter
2025-06-23 23:17 ` [PATCH net-next v2 8/8] selftests: drv-net: test RSS Netlink notifications Jakub Kicinski
2025-06-25  9:46   ` Donald Hunter
2025-06-25 20:04     ` Jakub Kicinski
2025-06-25 21:55       ` Donald Hunter
2025-06-24  6:00 ` [PATCH net-next v2 0/8] net: ethtool: rss: add notifications Maxime Chevallier
2025-06-24 14:13   ` Jakub Kicinski
2025-06-25 22:50 ` patchwork-bot+netdevbpf

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).