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 <[email protected]>
Signed-off-by: Breno Leitao <[email protected]>
---
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
On Fri, 16 Feb 2024 01:41:52 -0800
Breno Leitao <[email protected]> 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.
On 2/16/24 09:29, Stephen Hemminger wrote:
> On Fri, 16 Feb 2024 01:41:52 -0800
> Breno Leitao <[email protected]> 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
On Fri, Feb 16, 2024 at 7:41 PM Florian Fainelli <[email protected]> wrote:
>
> On 2/16/24 09:29, Stephen Hemminger wrote:
> > On Fri, 16 Feb 2024 01:41:52 -0800
> > Breno Leitao <[email protected]> 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....
On Fri, Feb 16, 2024 at 07:45:37PM +0100, Eric Dumazet wrote:
> On Fri, Feb 16, 2024 at 7:41 PM Florian Fainelli <[email protected]> wrote:
> >
> > On 2/16/24 09:29, Stephen Hemminger wrote:
> > > On Fri, 16 Feb 2024 01:41:52 -0800
> > > Breno Leitao <[email protected]> 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
On Fri, Feb 16, 2024 at 07:45:37PM +0100, Eric Dumazet wrote:
> On Fri, Feb 16, 2024 at 7:41 PM Florian Fainelli <[email protected]> wrote:
> >
> > On 2/16/24 09:29, Stephen Hemminger wrote:
> > > On Fri, 16 Feb 2024 01:41:52 -0800
> > > Breno Leitao <[email protected]> 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.
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.