[PATCH net-next v2] net: sysfs: Do not create sysfs for non BQL device

[PATCH net-next v2] net: sysfs: Do not create sysfs for non BQL device

[Breno Leitao]

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

Re: [PATCH net-next v2] net: sysfs: Do not create sysfs for non BQL device

[Stephen Hemminger]

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.

Re: [PATCH net-next v2] net: sysfs: Do not create sysfs for non BQL device

[Florian Fainelli]

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

Re: [PATCH net-next v2] net: sysfs: Do not create sysfs for non BQL device

[Eric Dumazet]

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....

Re: [PATCH net-next v2] net: sysfs: Do not create sysfs for non BQL device

[Breno Leitao]

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

Re: [PATCH net-next v2] net: sysfs: Do not create sysfs for non BQL device

[Jason Xing]

Hello Breno, Eric

On Mon, Feb 19, 2024 at 5:47 PM Breno Leitao <leitao@debian.org> wrote:
>
> 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.

When I'm reading and testing the dqs codes in my VM, I realize that
the virtio_net driver should have been excluded from BQL drivers,
which means VM using virtio_net driver should not have the
/sys/class/net/eth1/queues/tx-1/byte_queue_limits directory because It
has neither NETIF_F_LLTX nor IFF_NO_QUEUE.

I'm trying to cook a patch to fix it without introducing a new feature
like NETIF_F_LOOPBACK, but I failed to have a good patch.

I have two options in my mind:
1) introduce a feature flag only for virtion_net [1]
2) introduce a BQL flag, and then apply it to all the drivers which
has either NETIF_F_LLTX or IFF_NO_QUEUE bit, including virtio_net
driver

Do you have any ideas or suggestions?

Thanks in advance!

[1]
untested patch for now:
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 61a57d134544..e39417d99ea8 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -5634,7 +5634,7 @@ static int virtnet_probe(struct virtio_device *vdev)
                           IFF_TX_SKB_NO_LINEAR;
        dev->netdev_ops = &virtnet_netdev;
        dev->stat_ops = &virtnet_stat_ops;
-       dev->features = NETIF_F_HIGHDMA;
+       dev->features = NETIF_F_HIGHDMA | NETIF_F_VIRTNET;

        dev->ethtool_ops = &virtnet_ethtool_ops;
        SET_NETDEV_DEV(dev, &vdev->dev);
diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
index 7c2d77d75a88..4ade9cdf079e 100644
--- a/include/linux/netdev_features.h
+++ b/include/linux/netdev_features.h
@@ -14,7 +14,7 @@ typedef u64 netdev_features_t;
 enum {
        NETIF_F_SG_BIT,                 /* Scatter/gather IO. */
        NETIF_F_IP_CSUM_BIT,            /* Can checksum TCP/UDP over IPv4. */
-       __UNUSED_NETIF_F_1,
+       //__UNUSED_NETIF_F_1,
        NETIF_F_HW_CSUM_BIT,            /* Can checksum all the packets. */
        NETIF_F_IPV6_CSUM_BIT,          /* Can checksum TCP/UDP over IPV6 */
        NETIF_F_HIGHDMA_BIT,            /* Can DMA to high memory. */
@@ -91,6 +91,7 @@ enum {
        NETIF_F_HW_HSR_FWD_BIT,         /* Offload HSR forwarding */
        NETIF_F_HW_HSR_DUP_BIT,         /* Offload HSR duplication */

+       NETIF_F_VIRTNET_BIT,            /* Enable virtnet */
        /*
         * Add your fresh new feature above and remember to update
         * netdev_features_strings[] in net/ethtool/common.c and maybe
@@ -122,6 +123,7 @@ enum {
 #define NETIF_F_IPV6_CSUM      __NETIF_F(IPV6_CSUM)
 #define NETIF_F_LLTX           __NETIF_F(LLTX)
 #define NETIF_F_LOOPBACK       __NETIF_F(LOOPBACK)
+#define NETIF_F_VIRTNET                __NETIF_F(VIRTNET)
 #define NETIF_F_LRO            __NETIF_F(LRO)
 #define NETIF_F_NETNS_LOCAL    __NETIF_F(NETNS_LOCAL)
 #define NETIF_F_NOCACHE_COPY   __NETIF_F(NOCACHE_COPY)
diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
index 4c27a360c294..d52d95ea6fb6 100644
--- a/net/core/net-sysfs.c
+++ b/net/core/net-sysfs.c
@@ -1764,7 +1764,7 @@ static const struct kobj_type netdev_queue_ktype = {

 static bool netdev_uses_bql(const struct net_device *dev)
 {
-       if (dev->features & NETIF_F_LLTX ||
+       if (dev->features & (NETIF_F_LLTX | NETIF_F_VIRTNET) ||
            dev->priv_flags & IFF_NO_QUEUE)
                return false;

diff --git a/net/ethtool/common.c b/net/ethtool/common.c
index 6b2a360dcdf0..efb39a185e4b 100644
--- a/net/ethtool/common.c
+++ b/net/ethtool/common.c
@@ -74,6 +74,7 @@ const char
netdev_features_strings[NETDEV_FEATURE_COUNT][ETH_GSTRING_LEN] = {
        [NETIF_F_HW_HSR_TAG_RM_BIT] =    "hsr-tag-rm-offload",
        [NETIF_F_HW_HSR_FWD_BIT] =       "hsr-fwd-offload",
        [NETIF_F_HW_HSR_DUP_BIT] =       "hsr-dup-offload",
+       [NETIF_F_VIRTNET_BIT] =         "virtnet",
 };

 const char

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

Re: [PATCH net-next v2] net: sysfs: Do not create sysfs for non BQL device

[Jason Xing]

On Sun, Jun 9, 2024 at 12:47 PM Jason Xing <kerneljasonxing@gmail.com> wrote:
>
> Hello Breno, Eric
>
> On Mon, Feb 19, 2024 at 5:47 PM Breno Leitao <leitao@debian.org> wrote:
> >
> > 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.
>
> When I'm reading and testing the dqs codes in my VM, I realize that
> the virtio_net driver should have been excluded from BQL drivers,
> which means VM using virtio_net driver should not have the
> /sys/class/net/eth1/queues/tx-1/byte_queue_limits directory because It
> has neither NETIF_F_LLTX nor IFF_NO_QUEUE.
>
> I'm trying to cook a patch to fix it without introducing a new feature
> like NETIF_F_LOOPBACK, but I failed to have a good patch.
>
> I have two options in my mind:
> 1) introduce a feature flag only for virtion_net [1]
> 2) introduce a BQL flag, and then apply it to all the drivers which
> has either NETIF_F_LLTX or IFF_NO_QUEUE bit, including virtio_net
> driver

I decided to use this method. Please review :
https://lore.kernel.org/all/20240609131732.73156-1-kerneljasonxing@gmail.com/

Thanks.

>
> Do you have any ideas or suggestions?
>
> Thanks in advance!
>
> [1]
> untested patch for now:
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 61a57d134544..e39417d99ea8 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -5634,7 +5634,7 @@ static int virtnet_probe(struct virtio_device *vdev)
>                            IFF_TX_SKB_NO_LINEAR;
>         dev->netdev_ops = &virtnet_netdev;
>         dev->stat_ops = &virtnet_stat_ops;
> -       dev->features = NETIF_F_HIGHDMA;
> +       dev->features = NETIF_F_HIGHDMA | NETIF_F_VIRTNET;
>
>         dev->ethtool_ops = &virtnet_ethtool_ops;
>         SET_NETDEV_DEV(dev, &vdev->dev);
> diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
> index 7c2d77d75a88..4ade9cdf079e 100644
> --- a/include/linux/netdev_features.h
> +++ b/include/linux/netdev_features.h
> @@ -14,7 +14,7 @@ typedef u64 netdev_features_t;
>  enum {
>         NETIF_F_SG_BIT,                 /* Scatter/gather IO. */
>         NETIF_F_IP_CSUM_BIT,            /* Can checksum TCP/UDP over IPv4. */
> -       __UNUSED_NETIF_F_1,
> +       //__UNUSED_NETIF_F_1,
>         NETIF_F_HW_CSUM_BIT,            /* Can checksum all the packets. */
>         NETIF_F_IPV6_CSUM_BIT,          /* Can checksum TCP/UDP over IPV6 */
>         NETIF_F_HIGHDMA_BIT,            /* Can DMA to high memory. */
> @@ -91,6 +91,7 @@ enum {
>         NETIF_F_HW_HSR_FWD_BIT,         /* Offload HSR forwarding */
>         NETIF_F_HW_HSR_DUP_BIT,         /* Offload HSR duplication */
>
> +       NETIF_F_VIRTNET_BIT,            /* Enable virtnet */
>         /*
>          * Add your fresh new feature above and remember to update
>          * netdev_features_strings[] in net/ethtool/common.c and maybe
> @@ -122,6 +123,7 @@ enum {
>  #define NETIF_F_IPV6_CSUM      __NETIF_F(IPV6_CSUM)
>  #define NETIF_F_LLTX           __NETIF_F(LLTX)
>  #define NETIF_F_LOOPBACK       __NETIF_F(LOOPBACK)
> +#define NETIF_F_VIRTNET                __NETIF_F(VIRTNET)
>  #define NETIF_F_LRO            __NETIF_F(LRO)
>  #define NETIF_F_NETNS_LOCAL    __NETIF_F(NETNS_LOCAL)
>  #define NETIF_F_NOCACHE_COPY   __NETIF_F(NOCACHE_COPY)
> diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
> index 4c27a360c294..d52d95ea6fb6 100644
> --- a/net/core/net-sysfs.c
> +++ b/net/core/net-sysfs.c
> @@ -1764,7 +1764,7 @@ static const struct kobj_type netdev_queue_ktype = {
>
>  static bool netdev_uses_bql(const struct net_device *dev)
>  {
> -       if (dev->features & NETIF_F_LLTX ||
> +       if (dev->features & (NETIF_F_LLTX | NETIF_F_VIRTNET) ||
>             dev->priv_flags & IFF_NO_QUEUE)
>                 return false;
>
> diff --git a/net/ethtool/common.c b/net/ethtool/common.c
> index 6b2a360dcdf0..efb39a185e4b 100644
> --- a/net/ethtool/common.c
> +++ b/net/ethtool/common.c
> @@ -74,6 +74,7 @@ const char
> netdev_features_strings[NETDEV_FEATURE_COUNT][ETH_GSTRING_LEN] = {
>         [NETIF_F_HW_HSR_TAG_RM_BIT] =    "hsr-tag-rm-offload",
>         [NETIF_F_HW_HSR_FWD_BIT] =       "hsr-fwd-offload",
>         [NETIF_F_HW_HSR_DUP_BIT] =       "hsr-dup-offload",
> +       [NETIF_F_VIRTNET_BIT] =         "virtnet",
>  };
>
>  const char
>
> >
> > Hoping this is OK, I am planning to send a v2 adding the extra
> > parenthesis as reported above.
> >
> > Thanks
> >

Re: [PATCH net-next v2] net: sysfs: Do not create sysfs for non BQL device

[Simon Horman]

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.

Re: [PATCH net-next v2] net: sysfs: Do not create sysfs for non BQL device

[Jakub Kicinski]

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.