diff mbox series

[net-next,v3] net: sysfs: Do not create sysfs for non BQL device

Message ID 20240219104238.3782658-1-leitao@debian.org
State New
Headers
Series [net-next,v3] net: sysfs: Do not create sysfs for non BQL device |

Commit Message

Breno Leitao Feb. 19, 2024, 10:42 a.m. UTC 
  Creation of sysfs entries is expensive, mainly for workloads that
constantly creates netdev and netns often.

Do not create BQL sysfs entries for devices that don't need,
basically those that do not have a real queue, i.e, devices that has
NETIF_F_LLTX and IFF_NO_QUEUE, such as `lo` interface.

This will remove the /sys/class/net/eth0/queues/tx-X/byte_queue_limits/
directory for these devices.

In the example below, eth0 has the `byte_queue_limits` directory but not
`lo`.

	# ls /sys/class/net/lo/queues/tx-0/
	traffic_class  tx_maxrate  tx_timeout  xps_cpus  xps_rxqs

	# ls /sys/class/net/eth0/queues/tx-0/byte_queue_limits/
	hold_time  inflight  limit  limit_max  limit_min

This also removes the #ifdefs, since we can also use netdev_uses_bql() to
check if the config is enabled. (as suggested by Jakub).

Suggested-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: Breno Leitao <leitao@debian.org>
---
Changelog

v1:
 * https://lore.kernel.org/all/20240215112729.1778958-1-leitao@debian.org/
v2:
 * Removed the ifdefs by adding them inside the netdev_uses_bql(), as
   suggested by Jakub Kicinski.
v3:
 * Added an extra parenthesis between or (||) clauses as suggested by
   Stephen Hemminger

---
 net/core/net-sysfs.c | 35 ++++++++++++++++++++++++-----------
 1 file changed, 24 insertions(+), 11 deletions(-)

Comments

Simon Horman Feb. 20, 2024, 8:40 a.m. UTC | #1 
On Mon, Feb 19, 2024 at 02:42:36AM -0800, Breno Leitao wrote:
> Creation of sysfs entries is expensive, mainly for workloads that
> constantly creates netdev and netns often.
> 
> Do not create BQL sysfs entries for devices that don't need,
> basically those that do not have a real queue, i.e, devices that has
> NETIF_F_LLTX and IFF_NO_QUEUE, such as `lo` interface.
> 
> This will remove the /sys/class/net/eth0/queues/tx-X/byte_queue_limits/
> directory for these devices.
> 
> In the example below, eth0 has the `byte_queue_limits` directory but not
> `lo`.
> 
> 	# ls /sys/class/net/lo/queues/tx-0/
> 	traffic_class  tx_maxrate  tx_timeout  xps_cpus  xps_rxqs
> 
> 	# ls /sys/class/net/eth0/queues/tx-0/byte_queue_limits/
> 	hold_time  inflight  limit  limit_max  limit_min
> 
> This also removes the #ifdefs, since we can also use netdev_uses_bql() to
> check if the config is enabled. (as suggested by Jakub).
> 
> Suggested-by: Eric Dumazet <edumazet@google.com>
> Signed-off-by: Breno Leitao <leitao@debian.org>

Thanks Breno,

I like that this removes sysfs entries for devices for which they do not
act on.  Although I understand it is not a complete solution - still not
all devices that have these entries use BQL - I think a key value there is
to alleviate potential confusion for users.  And as such, this is a step in
the right direction.

I also like that some #ifdefs disappear.

Reviewed-by: Simon Horman <horms@kernel.org>

..
Jakub Kicinski Feb. 20, 2024, 3:40 p.m. UTC | #2 
On Mon, 19 Feb 2024 02:42:36 -0800 Breno Leitao wrote:
> Creation of sysfs entries is expensive, mainly for workloads that
> constantly creates netdev and netns often.
> 
> Do not create BQL sysfs entries for devices that don't need,
> basically those that do not have a real queue, i.e, devices that has
> NETIF_F_LLTX and IFF_NO_QUEUE, such as `lo` interface.

FTR, I applied v2, thanks!
diff mbox series

Patch

diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
index 946caefdd959..e2a01b7b7992 100644
--- a/net/core/net-sysfs.c
+++ b/net/core/net-sysfs.c
@@ -1459,6 +1459,9 @@  static const struct attribute_group dql_group = {
 	.name  = "byte_queue_limits",
 	.attrs  = dql_attrs,
 };
+#else
+/* Fake declaration, all the code using it should be dead */
+extern const struct attribute_group dql_group;
 #endif /* CONFIG_BQL */
 
 #ifdef CONFIG_XPS
@@ -1696,6 +1699,15 @@  static const struct kobj_type netdev_queue_ktype = {
 	.get_ownership = netdev_queue_get_ownership,
 };
 
+static bool netdev_uses_bql(const struct net_device *dev)
+{
+	if ((dev->features & NETIF_F_LLTX) ||
+	    (dev->priv_flags & IFF_NO_QUEUE))
+		return false;
+
+	return IS_ENABLED(CONFIG_BQL);
+}
+
 static int netdev_queue_add_kobject(struct net_device *dev, int index)
 {
 	struct netdev_queue *queue = dev->_tx + index;
@@ -1713,11 +1725,11 @@  static int netdev_queue_add_kobject(struct net_device *dev, int index)
 	if (error)
 		goto err;
 
-#ifdef CONFIG_BQL
-	error = sysfs_create_group(kobj, &dql_group);
-	if (error)
-		goto err;
-#endif
+	if (netdev_uses_bql(dev)) {
+		error = sysfs_create_group(kobj, &dql_group);
+		if (error)
+			goto err;
+	}
 
 	kobject_uevent(kobj, KOBJ_ADD);
 	return 0;
@@ -1738,9 +1750,9 @@  static int tx_queue_change_owner(struct net_device *ndev, int index,
 	if (error)
 		return error;
 
-#ifdef CONFIG_BQL
-	error = sysfs_group_change_owner(kobj, &dql_group, kuid, kgid);
-#endif
+	if (netdev_uses_bql(ndev))
+		error = sysfs_group_change_owner(kobj, &dql_group, kuid, kgid);
+
 	return error;
 }
 #endif /* CONFIG_SYSFS */
@@ -1772,9 +1784,10 @@  netdev_queue_update_kobjects(struct net_device *dev, int old_num, int new_num)
 
 		if (!refcount_read(&dev_net(dev)->ns.count))
 			queue->kobj.uevent_suppress = 1;
-#ifdef CONFIG_BQL
-		sysfs_remove_group(&queue->kobj, &dql_group);
-#endif
+
+		if (netdev_uses_bql(dev))
+			sysfs_remove_group(&queue->kobj, &dql_group);
+
 		kobject_put(&queue->kobj);
 	}