diff mbox series

[net-next] net: dqs: introduce NETIF_F_NO_BQL device feature

Message ID 20240609131732.73156-1-kerneljasonxing@gmail.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series [net-next] net: dqs: introduce NETIF_F_NO_BQL device feature | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 5674 this patch: 5674
netdev/build_tools success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers warning 1 maintainers not CCed: virtualization@lists.linux.dev
netdev/build_clang success Errors and warnings before: 980 this patch: 980
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 5948 this patch: 5948
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 44 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 2 this patch: 2
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-06-09--21-00 (tests: 644)

Commit Message

Jason Xing June 9, 2024, 1:17 p.m. UTC 
From: Jason Xing <kernelxing@tencent.com>

Since commit 74293ea1c4db6 ("net: sysfs: Do not create sysfs for non
BQL device") limits the non-BQL driver not creating byte_queue_limits
directory, I found there is one exception, namely, virtio-net driver,
which should also be limited in netdev_uses_bql().

I decided to introduce a NO_BQL bit in device feature because
1) it can help us limit virtio-net driver for now.
2) if we found another non-BQL driver, we can take it into account.
3) we can replace all the driver meeting those two statements in
netdev_uses_bql() in future.

For now, I would like to make the first step to use this new bit for dqs
use instead of replacing/applying all the non-BQL drivers.

After this patch, 1) there is no byte_queue_limits directory in virtio-net
driver. 2) running ethtool -k eth1 shows "no-bql: on [fixed]".

Signed-off-by: Jason Xing <kernelxing@tencent.com>
---
 drivers/net/virtio_net.c        | 2 +-
 include/linux/netdev_features.h | 3 ++-
 net/core/net-sysfs.c            | 2 +-
 net/ethtool/common.c            | 1 +
 4 files changed, 5 insertions(+), 3 deletions(-)

Comments

Eric Dumazet June 9, 2024, 4:42 p.m. UTC | #1 
On Sun, Jun 9, 2024 at 3:17 PM Jason Xing <kerneljasonxing@gmail.com> wrote:
>
> From: Jason Xing <kernelxing@tencent.com>
>
> Since commit 74293ea1c4db6 ("net: sysfs: Do not create sysfs for non
> BQL device") limits the non-BQL driver not creating byte_queue_limits
> directory, I found there is one exception, namely, virtio-net driver,
> which should also be limited in netdev_uses_bql().
>
> I decided to introduce a NO_BQL bit in device feature because
> 1) it can help us limit virtio-net driver for now.
> 2) if we found another non-BQL driver, we can take it into account.
> 3) we can replace all the driver meeting those two statements in
> netdev_uses_bql() in future.
>
> For now, I would like to make the first step to use this new bit for dqs
> use instead of replacing/applying all the non-BQL drivers.
>
> After this patch, 1) there is no byte_queue_limits directory in virtio-net
> driver. 2) running ethtool -k eth1 shows "no-bql: on [fixed]".
>
> Signed-off-by: Jason Xing <kernelxing@tencent.com>

I do not think we want to consume a precious bit from dev->features
for something like that.

dev->features should be reserved for bits we used in the fast path, for instance
netif_skb_features(). It is good to have free bits for future fast path use.

(I think Vladimir was trying to make some room, this was a discussion
we had last year)

I do not see the reason to report to ethtool the 'nobql bit' :
If a driver opts-out, then the bql sysfs files will not be there, user
space can see the absence of the files.
Jason Xing June 9, 2024, 11:55 p.m. UTC | #2 
On Mon, Jun 10, 2024 at 12:42 AM Eric Dumazet <edumazet@google.com> wrote:
>
> On Sun, Jun 9, 2024 at 3:17 PM Jason Xing <kerneljasonxing@gmail.com> wrote:
> >
> > From: Jason Xing <kernelxing@tencent.com>
> >
> > Since commit 74293ea1c4db6 ("net: sysfs: Do not create sysfs for non
> > BQL device") limits the non-BQL driver not creating byte_queue_limits
> > directory, I found there is one exception, namely, virtio-net driver,
> > which should also be limited in netdev_uses_bql().
> >
> > I decided to introduce a NO_BQL bit in device feature because
> > 1) it can help us limit virtio-net driver for now.
> > 2) if we found another non-BQL driver, we can take it into account.
> > 3) we can replace all the driver meeting those two statements in
> > netdev_uses_bql() in future.
> >
> > For now, I would like to make the first step to use this new bit for dqs
> > use instead of replacing/applying all the non-BQL drivers.
> >
> > After this patch, 1) there is no byte_queue_limits directory in virtio-net
> > driver. 2) running ethtool -k eth1 shows "no-bql: on [fixed]".
> >
> > Signed-off-by: Jason Xing <kernelxing@tencent.com>
>
> I do not think we want to consume a precious bit from dev->features
> for something like that.
>
> dev->features should be reserved for bits we used in the fast path, for instance
> netif_skb_features(). It is good to have free bits for future fast path use.
>
> (I think Vladimir was trying to make some room, this was a discussion
> we had last year)

Thanks for your reminder. When I was trying to introduce one new bit,
I noticed an overflow warning when compiling.

>
> I do not see the reason to report to ethtool the 'nobql bit' :
> If a driver opts-out, then the bql sysfs files will not be there, user
> space can see the absence of the files.

The reason is that I just followed the comment to force myself to
report to ethtool. Now I see.

It seems not that easy to consider all the non-BQL drivers. Let me
think more about it.

Thanks,
Jason
Jiri Pirko June 10, 2024, 6:33 a.m. UTC | #3 
Sun, Jun 09, 2024 at 03:17:32PM CEST, kerneljasonxing@gmail.com wrote:
>From: Jason Xing <kernelxing@tencent.com>
>
>Since commit 74293ea1c4db6 ("net: sysfs: Do not create sysfs for non
>BQL device") limits the non-BQL driver not creating byte_queue_limits
>directory, I found there is one exception, namely, virtio-net driver,
>which should also be limited in netdev_uses_bql().
>
>I decided to introduce a NO_BQL bit in device feature because
>1) it can help us limit virtio-net driver for now.
>2) if we found another non-BQL driver, we can take it into account.
>3) we can replace all the driver meeting those two statements in
>netdev_uses_bql() in future.
>
>For now, I would like to make the first step to use this new bit for dqs
>use instead of replacing/applying all the non-BQL drivers.
>
>After this patch, 1) there is no byte_queue_limits directory in virtio-net
>driver. 2) running ethtool -k eth1 shows "no-bql: on [fixed]".

Wait, you introduce this flag only for the sake of virtio_net driver,
don't you. Since there is currently an attempt to implement bql in
virtio_net, wouldn't it make this flag obsolete? Can't you wait?


>
>Signed-off-by: Jason Xing <kernelxing@tencent.com>
>---
> drivers/net/virtio_net.c        | 2 +-
> include/linux/netdev_features.h | 3 ++-
> net/core/net-sysfs.c            | 2 +-
> net/ethtool/common.c            | 1 +
> 4 files changed, 5 insertions(+), 3 deletions(-)
>
>diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>index 61a57d134544..619908fed14b 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_NO_BQL;
> 
> 	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..9bc603bb4227 100644
>--- a/include/linux/netdev_features.h
>+++ b/include/linux/netdev_features.h
>@@ -14,7 +14,6 @@ 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,
> 	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 +90,7 @@ enum {
> 	NETIF_F_HW_HSR_FWD_BIT,		/* Offload HSR forwarding */
> 	NETIF_F_HW_HSR_DUP_BIT,		/* Offload HSR duplication */
> 
>+	NETIF_F_NO_BQL_BIT,		/* non-BQL driver */
> 	/*
> 	 * Add your fresh new feature above and remember to update
> 	 * netdev_features_strings[] in net/ethtool/common.c and maybe
>@@ -168,6 +168,7 @@ enum {
> #define NETIF_F_HW_HSR_TAG_RM	__NETIF_F(HW_HSR_TAG_RM)
> #define NETIF_F_HW_HSR_FWD	__NETIF_F(HW_HSR_FWD)
> #define NETIF_F_HW_HSR_DUP	__NETIF_F(HW_HSR_DUP)
>+#define NETIF_F_NO_BQL		__NETIF_F(NO_BQL)
> 
> /* Finds the next feature with the highest number of the range of start-1 till 0.
>  */
>diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
>index 4c27a360c294..ff397a76f1fe 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_NO_BQL) ||
> 	    dev->priv_flags & IFF_NO_QUEUE)
> 		return false;
> 
>diff --git a/net/ethtool/common.c b/net/ethtool/common.c
>index 6b2a360dcdf0..efa7ac4158ce 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_NO_BQL_BIT] =		 "no-bql",
> };
> 
> const char
>-- 
>2.37.3
>
>
Jason Xing June 10, 2024, 11:07 a.m. UTC | #4 
On Mon, Jun 10, 2024 at 2:33 PM Jiri Pirko <jiri@resnulli.us> wrote:
>
> Sun, Jun 09, 2024 at 03:17:32PM CEST, kerneljasonxing@gmail.com wrote:
> >From: Jason Xing <kernelxing@tencent.com>
> >
> >Since commit 74293ea1c4db6 ("net: sysfs: Do not create sysfs for non
> >BQL device") limits the non-BQL driver not creating byte_queue_limits
> >directory, I found there is one exception, namely, virtio-net driver,
> >which should also be limited in netdev_uses_bql().
> >
> >I decided to introduce a NO_BQL bit in device feature because
> >1) it can help us limit virtio-net driver for now.
> >2) if we found another non-BQL driver, we can take it into account.
> >3) we can replace all the driver meeting those two statements in
> >netdev_uses_bql() in future.
> >
> >For now, I would like to make the first step to use this new bit for dqs
> >use instead of replacing/applying all the non-BQL drivers.
> >
> >After this patch, 1) there is no byte_queue_limits directory in virtio-net
> >driver. 2) running ethtool -k eth1 shows "no-bql: on [fixed]".
>
> Wait, you introduce this flag only for the sake of virtio_net driver,
> don't you. Since there is currently an attempt to implement bql in
> virtio_net, wouldn't it make this flag obsolete? Can't you wait?

I admit the way of introducing a new flag is not a good way to go, but
I cannot figure out a better way. I'm still thinking. Virtio_net
driver, I think, is not the only non-BQL we miss in dqs.

You can keep trying sending BQL patches for the virtio_net driver but
they don't conflict with the current patch. Can you guarantee you can
smoothly introduce BQL for virtio_net in a few days? I have no
confidence in your recent action about blindly deleting old features
and introducing a new one without considering the real world, I mean,
the world outside of the community. They are thousands and hundreds of
servers running outside. You have to prove deleting an old one doesn't
harm the users instead of asking users to prove the old feature is
useful. It's very weird. The key reason is about whether it breaks
userspace compatibility and how you handle the compatibility. For
example, I don't think kernel developers can delete some old proc
files even though they are buggy/inaccurate. I'm just saying what I'm
worried about. Let the maintainers decide finally.
+ Jason Wang, + Michael S. Tsirkin

Let's get back to this patch, I think actually it is a bug we need to
fix since netdev_uses_bql() decided to limit non-BQL drivers. I'm
still thinking, as I said...
Jakub Kicinski June 11, 2024, 1:45 a.m. UTC | #5 
On Mon, 10 Jun 2024 07:55:55 +0800 Jason Xing wrote:
> > (I think Vladimir was trying to make some room, this was a discussion
> > we had last year)  

s/Vladimir/Olek/ ?

> Thanks for your reminder. When I was trying to introduce one new bit,
> I noticed an overflow warning when compiling.
> 
> > I do not see the reason to report to ethtool the 'nobql bit' :
> > If a driver opts-out, then the bql sysfs files will not be there, user
> > space can see the absence of the files.  
> 
> The reason is that I just followed the comment to force myself to
> report to ethtool. Now I see.
> 
> It seems not that easy to consider all the non-BQL drivers. Let me
> think more about it.

All Eric was saying, AFAIU, is that you can for example add a bit 
in somewhere towards the end of struct nedevice, no need to pack
this info into feature bits.

BTW the Fixes tag is a bit of an exaggeration here. The heuristic in
netdev_uses_bql() is best effort, its fine to miss some devices.
Jason Xing June 11, 2024, 2:56 a.m. UTC | #6 
On Tue, Jun 11, 2024 at 9:45 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Mon, 10 Jun 2024 07:55:55 +0800 Jason Xing wrote:
> > > (I think Vladimir was trying to make some room, this was a discussion
> > > we had last year)
>
> s/Vladimir/Olek/ ?
>
> > Thanks for your reminder. When I was trying to introduce one new bit,
> > I noticed an overflow warning when compiling.
> >
> > > I do not see the reason to report to ethtool the 'nobql bit' :
> > > If a driver opts-out, then the bql sysfs files will not be there, user
> > > space can see the absence of the files.
> >
> > The reason is that I just followed the comment to force myself to
> > report to ethtool. Now I see.
> >
> > It seems not that easy to consider all the non-BQL drivers. Let me
> > think more about it.
>
> All Eric was saying, AFAIU, is that you can for example add a bit
> in somewhere towards the end of struct nedevice, no need to pack
> this info into feature bits.

Oh, thanks for pointing this out.

I would like to add a new bit field in the enum netdev_priv_flags
because it is better that it's grouped into an existing enum for
future compatibility.

>
> BTW the Fixes tag is a bit of an exaggeration here. The heuristic in
> netdev_uses_bql() is best effort, its fine to miss some devices.

I see.

Thanks,
Jason
diff mbox series

Patch

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 61a57d134544..619908fed14b 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_NO_BQL;
 
 	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..9bc603bb4227 100644
--- a/include/linux/netdev_features.h
+++ b/include/linux/netdev_features.h
@@ -14,7 +14,6 @@  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,
 	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 +90,7 @@  enum {
 	NETIF_F_HW_HSR_FWD_BIT,		/* Offload HSR forwarding */
 	NETIF_F_HW_HSR_DUP_BIT,		/* Offload HSR duplication */
 
+	NETIF_F_NO_BQL_BIT,		/* non-BQL driver */
 	/*
 	 * Add your fresh new feature above and remember to update
 	 * netdev_features_strings[] in net/ethtool/common.c and maybe
@@ -168,6 +168,7 @@  enum {
 #define NETIF_F_HW_HSR_TAG_RM	__NETIF_F(HW_HSR_TAG_RM)
 #define NETIF_F_HW_HSR_FWD	__NETIF_F(HW_HSR_FWD)
 #define NETIF_F_HW_HSR_DUP	__NETIF_F(HW_HSR_DUP)
+#define NETIF_F_NO_BQL		__NETIF_F(NO_BQL)
 
 /* Finds the next feature with the highest number of the range of start-1 till 0.
  */
diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
index 4c27a360c294..ff397a76f1fe 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_NO_BQL) ||
 	    dev->priv_flags & IFF_NO_QUEUE)
 		return false;
 
diff --git a/net/ethtool/common.c b/net/ethtool/common.c
index 6b2a360dcdf0..efa7ac4158ce 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_NO_BQL_BIT] =		 "no-bql",
 };
 
 const char