All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net-next v2] net: sysfs: Do not create sysfs for non BQL device
@ 2024-02-16  9:41 Breno Leitao
  2024-02-16 17:29 ` Stephen Hemminger
  0 siblings, 1 reply; 7+ messages in thread
From: Breno Leitao @ 2024-02-16  9:41 UTC (permalink / raw)
  To: kuba, davem, pabeni, edumazet; +Cc: netdev, linux-kernel, horms, Johannes Berg

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>
---
 net/core/net-sysfs.c | 35 ++++++++++++++++++++++++-----------
 1 file changed, 24 insertions(+), 11 deletions(-)

diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
index 946caefdd959..af238026ac3c 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);
 	}
 
-- 
2.39.3


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

* Re: [PATCH net-next v2] net: sysfs: Do not create sysfs for non BQL device
  2024-02-16  9:41 [PATCH net-next v2] net: sysfs: Do not create sysfs for non BQL device Breno Leitao
@ 2024-02-16 17:29 ` Stephen Hemminger
  2024-02-16 18:41   ` Florian Fainelli
  2024-02-19 20:16   ` Jakub Kicinski
  0 siblings, 2 replies; 7+ messages in thread
From: Stephen Hemminger @ 2024-02-16 17:29 UTC (permalink / raw)
  To: Breno Leitao
  Cc: kuba, davem, pabeni, edumazet, netdev, linux-kernel, horms,
	Johannes Berg

On Fri, 16 Feb 2024 01:41:52 -0800
Breno Leitao <leitao@debian.org> wrote:

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

Various compilers will warn about missing parens in that expression.
It is valid but mixing & and || can be bug trap.

	if ((dev->features & NETIF_F_LLTX) || (dev->priv_flags & IFF_NO_QUEUE))
		return false;

Not all drivers will be using bql, it requires driver to have that code.
So really it means driver could be using BQL.
Not sure if there is a way to find out if driver has the required BQL bits.

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

* Re: [PATCH net-next v2] net: sysfs: Do not create sysfs for non BQL device
  2024-02-16 17:29 ` Stephen Hemminger
@ 2024-02-16 18:41   ` Florian Fainelli
  2024-02-16 18:45     ` Eric Dumazet
  2024-02-19 20:16   ` Jakub Kicinski
  1 sibling, 1 reply; 7+ messages in thread
From: Florian Fainelli @ 2024-02-16 18:41 UTC (permalink / raw)
  To: Stephen Hemminger, Breno Leitao
  Cc: kuba, davem, pabeni, edumazet, netdev, linux-kernel, horms,
	Johannes Berg

On 2/16/24 09:29, Stephen Hemminger wrote:
> On Fri, 16 Feb 2024 01:41:52 -0800
> Breno Leitao <leitao@debian.org> wrote:
> 
>> +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);
>> +}
> 
> Various compilers will warn about missing parens in that expression.
> It is valid but mixing & and || can be bug trap.
> 
> 	if ((dev->features & NETIF_F_LLTX) || (dev->priv_flags & IFF_NO_QUEUE))
> 		return false;
> 
> Not all drivers will be using bql, it requires driver to have that code.
> So really it means driver could be using BQL.
> Not sure if there is a way to find out if driver has the required BQL bits.

There is not a feature flag to be keying off if that is what you are 
after, you would need to audit the drivers and see whether they make 
calls to netdev_tx_sent_queue(), netdev_tx_reset_queue(), 
netdev_tx_completed_queue().

I suppose you might be able to programmatically extract that information 
by looking at whether a given driver object file has a reference to 
dql_{reset,avail,completed} or do that at the source level, whichever is 
easier.
-- 
Florian


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

* Re: [PATCH net-next v2] net: sysfs: Do not create sysfs for non BQL device
  2024-02-16 18:41   ` Florian Fainelli
@ 2024-02-16 18:45     ` Eric Dumazet
  2024-02-19  9:46       ` Breno Leitao
  2024-02-19 10:46       ` Simon Horman
  0 siblings, 2 replies; 7+ messages in thread
From: Eric Dumazet @ 2024-02-16 18:45 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: Stephen Hemminger, Breno Leitao, kuba, davem, pabeni, netdev,
	linux-kernel, horms, Johannes Berg

On Fri, Feb 16, 2024 at 7:41 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
>
> On 2/16/24 09:29, Stephen Hemminger wrote:
> > On Fri, 16 Feb 2024 01:41:52 -0800
> > Breno Leitao <leitao@debian.org> wrote:
> >
> >> +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);
> >> +}
> >
> > Various compilers will warn about missing parens in that expression.
> > It is valid but mixing & and || can be bug trap.
> >
> >       if ((dev->features & NETIF_F_LLTX) || (dev->priv_flags & IFF_NO_QUEUE))
> >               return false;
> >
> > Not all drivers will be using bql, it requires driver to have that code.
> > So really it means driver could be using BQL.
> > Not sure if there is a way to find out if driver has the required BQL bits.
>
> There is not a feature flag to be keying off if that is what you are
> after, you would need to audit the drivers and see whether they make
> calls to netdev_tx_sent_queue(), netdev_tx_reset_queue(),
> netdev_tx_completed_queue().
>
> I suppose you might be able to programmatically extract that information
> by looking at whether a given driver object file has a reference to
> dql_{reset,avail,completed} or do that at the source level, whichever is
> easier.

Note that the suggested patch does not change current functionality.

Traditionally, we had sysfs entries fpr BQL for all netdev, regardless of them
using BQL or not.

The patch seems to be a good first step.

If anyone wants to refine it further, this is great, but I suspect
very few users will benefit from
having less sysfs entries for real/physical devices....

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

* Re: [PATCH net-next v2] net: sysfs: Do not create sysfs for non BQL device
  2024-02-16 18:45     ` Eric Dumazet
@ 2024-02-19  9:46       ` Breno Leitao
  2024-02-19 10:46       ` Simon Horman
  1 sibling, 0 replies; 7+ messages in thread
From: Breno Leitao @ 2024-02-19  9:46 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: Florian Fainelli, Stephen Hemminger, kuba, davem, pabeni, netdev,
	linux-kernel, horms, Johannes Berg

On Fri, Feb 16, 2024 at 07:45:37PM +0100, Eric Dumazet wrote:
> On Fri, Feb 16, 2024 at 7:41 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
> >
> > On 2/16/24 09:29, Stephen Hemminger wrote:
> > > On Fri, 16 Feb 2024 01:41:52 -0800
> > > Breno Leitao <leitao@debian.org> wrote:
> > >
> > >> +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);
> > >> +}
> > >
> > > Various compilers will warn about missing parens in that expression.
> > > It is valid but mixing & and || can be bug trap.
> > >
> > >       if ((dev->features & NETIF_F_LLTX) || (dev->priv_flags & IFF_NO_QUEUE))
> > >               return false;
> > >
> > > Not all drivers will be using bql, it requires driver to have that code.
> > > So really it means driver could be using BQL.
> > > Not sure if there is a way to find out if driver has the required BQL bits.
> >
> > There is not a feature flag to be keying off if that is what you are
> > after, you would need to audit the drivers and see whether they make
> > calls to netdev_tx_sent_queue(), netdev_tx_reset_queue(),
> > netdev_tx_completed_queue().
> >
> > I suppose you might be able to programmatically extract that information
> > by looking at whether a given driver object file has a reference to
> > dql_{reset,avail,completed} or do that at the source level, whichever is
> > easier.
> 
> Note that the suggested patch does not change current functionality.
> 
> Traditionally, we had sysfs entries fpr BQL for all netdev, regardless of them
> using BQL or not.
> 
> The patch seems to be a good first step.

Thanks Eric. I agree it solves the problem without creating a new
feature flag, that could also be done, but maybe less important than
this first step.

Hoping this is OK, I am planning to send a v2 adding the extra
parenthesis as reported above. 

Thanks

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

* Re: [PATCH net-next v2] net: sysfs: Do not create sysfs for non BQL device
  2024-02-16 18:45     ` Eric Dumazet
  2024-02-19  9:46       ` Breno Leitao
@ 2024-02-19 10:46       ` Simon Horman
  1 sibling, 0 replies; 7+ messages in thread
From: Simon Horman @ 2024-02-19 10:46 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: Florian Fainelli, Stephen Hemminger, Breno Leitao, kuba, davem,
	pabeni, netdev, linux-kernel, Johannes Berg

On Fri, Feb 16, 2024 at 07:45:37PM +0100, Eric Dumazet wrote:
> On Fri, Feb 16, 2024 at 7:41 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
> >
> > On 2/16/24 09:29, Stephen Hemminger wrote:
> > > On Fri, 16 Feb 2024 01:41:52 -0800
> > > Breno Leitao <leitao@debian.org> wrote:
> > >
> > >> +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);
> > >> +}
> > >
> > > Various compilers will warn about missing parens in that expression.
> > > It is valid but mixing & and || can be bug trap.
> > >
> > >       if ((dev->features & NETIF_F_LLTX) || (dev->priv_flags & IFF_NO_QUEUE))
> > >               return false;
> > >
> > > Not all drivers will be using bql, it requires driver to have that code.
> > > So really it means driver could be using BQL.
> > > Not sure if there is a way to find out if driver has the required BQL bits.
> >
> > There is not a feature flag to be keying off if that is what you are
> > after, you would need to audit the drivers and see whether they make
> > calls to netdev_tx_sent_queue(), netdev_tx_reset_queue(),
> > netdev_tx_completed_queue().
> >
> > I suppose you might be able to programmatically extract that information
> > by looking at whether a given driver object file has a reference to
> > dql_{reset,avail,completed} or do that at the source level, whichever is
> > easier.
> 
> Note that the suggested patch does not change current functionality.
> 
> Traditionally, we had sysfs entries fpr BQL for all netdev, regardless of them
> using BQL or not.
> 
> The patch seems to be a good first step.
> 
> If anyone wants to refine it further, this is great, but I suspect
> very few users will benefit from
> having less sysfs entries for real/physical devices....
> 

From my point of view the main advantage in not having these entries
would be that it is really a bit confusing for them to be there
that don't use BQL. But I agree, that is (also) likely to benefit
few users.

In any case, I agree this is a good first step.


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

* Re: [PATCH net-next v2] net: sysfs: Do not create sysfs for non BQL device
  2024-02-16 17:29 ` Stephen Hemminger
  2024-02-16 18:41   ` Florian Fainelli
@ 2024-02-19 20:16   ` Jakub Kicinski
  1 sibling, 0 replies; 7+ messages in thread
From: Jakub Kicinski @ 2024-02-19 20:16 UTC (permalink / raw)
  To: Stephen Hemminger
  Cc: Breno Leitao, davem, pabeni, edumazet, netdev, linux-kernel,
	horms, Johannes Berg

On Fri, 16 Feb 2024 09:29:05 -0800 Stephen Hemminger wrote:
> Various compilers will warn about missing parens in that expression.
> It is valid but mixing & and || can be bug trap.

$ git grep ' & [A-Z0-9_]* ||' | wc -l
855

Let's not sprinkle parenthesis in correct code because some old
compiler somewhere may doubt our ability to remember the precedence 
of 14 operators.

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

end of thread, other threads:[~2024-02-19 20:16 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-02-16  9:41 [PATCH net-next v2] net: sysfs: Do not create sysfs for non BQL device Breno Leitao
2024-02-16 17:29 ` Stephen Hemminger
2024-02-16 18:41   ` Florian Fainelli
2024-02-16 18:45     ` Eric Dumazet
2024-02-19  9:46       ` Breno Leitao
2024-02-19 10:46       ` Simon Horman
2024-02-19 20:16   ` 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.