| Message ID | 20240613023549.15213-1-kerneljasonxing@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v3] net: dqs: introduce IFF_NO_BQL private flag for non-BQL drivers | expand |
Thu, Jun 13, 2024 at 04:35:49AM 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(). Let me give it a >try first. > >I decided to introduce a NO_BQL bit 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 in one go. > >As Jakub said, "netdev_uses_bql() is best effort", I think, we can add >new non-BQL drivers as soon as we find one. > >After this patch, there is no byte_queue_limits directory in virtio-net >driver. Please note following patch is currently trying to push bql support for virtio_net: https://lore.kernel.org/netdev/20240612170851.1004604-1-jiri@resnulli.us/ When that is merged, this patch is not needed. Could we wait?
On Thu, Jun 13, 2024 at 1:38 PM Jiri Pirko <jiri@resnulli.us> wrote: > > Thu, Jun 13, 2024 at 04:35:49AM 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(). Let me give it a > >try first. > > > >I decided to introduce a NO_BQL bit 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 in one go. > > > >As Jakub said, "netdev_uses_bql() is best effort", I think, we can add > >new non-BQL drivers as soon as we find one. > > > >After this patch, there is no byte_queue_limits directory in virtio-net > >driver. > > Please note following patch is currently trying to push bql support for > virtio_net: > https://lore.kernel.org/netdev/20240612170851.1004604-1-jiri@resnulli.us/ I saw this one this morning and I'm reviewing/testing it. > > When that is merged, this patch is not needed. Could we wait? Please note this patch is not only written for virtio_net driver. Virtio_net driver is one of possible cases. After your patch gets merged (I think it will take some time), you could simply remove that one line in virtio_net.c. Thanks.
Thu, Jun 13, 2024 at 08:08:36AM CEST, kerneljasonxing@gmail.com wrote: >On Thu, Jun 13, 2024 at 1:38 PM Jiri Pirko <jiri@resnulli.us> wrote: >> >> Thu, Jun 13, 2024 at 04:35:49AM 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(). Let me give it a >> >try first. >> > >> >I decided to introduce a NO_BQL bit 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 in one go. >> > >> >As Jakub said, "netdev_uses_bql() is best effort", I think, we can add >> >new non-BQL drivers as soon as we find one. >> > >> >After this patch, there is no byte_queue_limits directory in virtio-net >> >driver. >> >> Please note following patch is currently trying to push bql support for >> virtio_net: >> https://lore.kernel.org/netdev/20240612170851.1004604-1-jiri@resnulli.us/ > >I saw this one this morning and I'm reviewing/testing it. > >> >> When that is merged, this patch is not needed. Could we wait? > >Please note this patch is not only written for virtio_net driver. >Virtio_net driver is one of possible cases. Yeah, but without virtio_net, there will be no users. What's the point of having that in code? I mean, in general, no-user kernel code gets removed. > >After your patch gets merged (I think it will take some time), you >could simply remove that one line in virtio_net.c. > >Thanks.
On Thu, Jun 13, 2024 at 3:19 PM Jiri Pirko <jiri@resnulli.us> wrote: > > Thu, Jun 13, 2024 at 08:08:36AM CEST, kerneljasonxing@gmail.com wrote: > >On Thu, Jun 13, 2024 at 1:38 PM Jiri Pirko <jiri@resnulli.us> wrote: > >> > >> Thu, Jun 13, 2024 at 04:35:49AM 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(). Let me give it a > >> >try first. > >> > > >> >I decided to introduce a NO_BQL bit 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 in one go. > >> > > >> >As Jakub said, "netdev_uses_bql() is best effort", I think, we can add > >> >new non-BQL drivers as soon as we find one. > >> > > >> >After this patch, there is no byte_queue_limits directory in virtio-net > >> >driver. > >> > >> Please note following patch is currently trying to push bql support for > >> virtio_net: > >> https://lore.kernel.org/netdev/20240612170851.1004604-1-jiri@resnulli.us/ > > > >I saw this one this morning and I'm reviewing/testing it. > > > >> > >> When that is merged, this patch is not needed. Could we wait? > > > >Please note this patch is not only written for virtio_net driver. > >Virtio_net driver is one of possible cases. > > Yeah, but without virtio_net, there will be no users. What's the point > of having that in code? I mean, in general, no-user kernel code gets > removed. Are you sure netdev_uses_bql() can limit all the non-bql drivers with those two checks? I haven't investigated this part. > > > > > >After your patch gets merged (I think it will take some time), you > >could simply remove that one line in virtio_net.c. > > > >Thanks.
On Thu, Jun 13, 2024 at 3:24 PM Jason Xing <kerneljasonxing@gmail.com> wrote: > > On Thu, Jun 13, 2024 at 3:19 PM Jiri Pirko <jiri@resnulli.us> wrote: > > > > Thu, Jun 13, 2024 at 08:08:36AM CEST, kerneljasonxing@gmail.com wrote: > > >On Thu, Jun 13, 2024 at 1:38 PM Jiri Pirko <jiri@resnulli.us> wrote: > > >> > > >> Thu, Jun 13, 2024 at 04:35:49AM 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(). Let me give it a > > >> >try first. > > >> > > > >> >I decided to introduce a NO_BQL bit 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 in one go. > > >> > > > >> >As Jakub said, "netdev_uses_bql() is best effort", I think, we can add > > >> >new non-BQL drivers as soon as we find one. > > >> > > > >> >After this patch, there is no byte_queue_limits directory in virtio-net > > >> >driver. > > >> > > >> Please note following patch is currently trying to push bql support for > > >> virtio_net: > > >> https://lore.kernel.org/netdev/20240612170851.1004604-1-jiri@resnulli.us/ > > > > > >I saw this one this morning and I'm reviewing/testing it. > > > > > >> > > >> When that is merged, this patch is not needed. Could we wait? > > > > > >Please note this patch is not only written for virtio_net driver. > > >Virtio_net driver is one of possible cases. > > > > Yeah, but without virtio_net, there will be no users. What's the point > > of having that in code? I mean, in general, no-user kernel code gets > > removed. > > Are you sure netdev_uses_bql() can limit all the non-bql drivers with > those two checks? I haven't investigated this part. I just googled it very quickly and saw the ENA driver which turns off BQL by default due to the problem of head-of line blocking[1]. IIUC, ENA is not limited by the checks in netdev_uses_bql() but it should. I run this command: grep -ir -E "NO_QUEUE|NETIF_F_LLTX" drivers/net/ethernet/amazon/ena Am I right? [1]: https://github.com/amzn/amzn-drivers/issues/262 > > > > > > > > > > >After your patch gets merged (I think it will take some time), you > > >could simply remove that one line in virtio_net.c. > > > > > >Thanks.
Thu, Jun 13, 2024 at 09:24:27AM CEST, kerneljasonxing@gmail.com wrote: >On Thu, Jun 13, 2024 at 3:19 PM Jiri Pirko <jiri@resnulli.us> wrote: >> >> Thu, Jun 13, 2024 at 08:08:36AM CEST, kerneljasonxing@gmail.com wrote: >> >On Thu, Jun 13, 2024 at 1:38 PM Jiri Pirko <jiri@resnulli.us> wrote: >> >> >> >> Thu, Jun 13, 2024 at 04:35:49AM 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(). Let me give it a >> >> >try first. >> >> > >> >> >I decided to introduce a NO_BQL bit 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 in one go. >> >> > >> >> >As Jakub said, "netdev_uses_bql() is best effort", I think, we can add >> >> >new non-BQL drivers as soon as we find one. >> >> > >> >> >After this patch, there is no byte_queue_limits directory in virtio-net >> >> >driver. >> >> >> >> Please note following patch is currently trying to push bql support for >> >> virtio_net: >> >> https://lore.kernel.org/netdev/20240612170851.1004604-1-jiri@resnulli.us/ >> > >> >I saw this one this morning and I'm reviewing/testing it. >> > >> >> >> >> When that is merged, this patch is not needed. Could we wait? >> > >> >Please note this patch is not only written for virtio_net driver. >> >Virtio_net driver is one of possible cases. >> >> Yeah, but without virtio_net, there will be no users. What's the point >> of having that in code? I mean, in general, no-user kernel code gets >> removed. > >Are you sure netdev_uses_bql() can limit all the non-bql drivers with >those two checks? I haven't investigated this part. Nope. What I say is, if there are other users, let's find them and let them use what you are introducing here. Otherwise don't add unused code. > >> >> >> > >> >After your patch gets merged (I think it will take some time), you >> >could simply remove that one line in virtio_net.c. >> > >> >Thanks.
On Thu, Jun 13, 2024 at 3:51 PM Jiri Pirko <jiri@resnulli.us> wrote: > > Thu, Jun 13, 2024 at 09:24:27AM CEST, kerneljasonxing@gmail.com wrote: > >On Thu, Jun 13, 2024 at 3:19 PM Jiri Pirko <jiri@resnulli.us> wrote: > >> > >> Thu, Jun 13, 2024 at 08:08:36AM CEST, kerneljasonxing@gmail.com wrote: > >> >On Thu, Jun 13, 2024 at 1:38 PM Jiri Pirko <jiri@resnulli.us> wrote: > >> >> > >> >> Thu, Jun 13, 2024 at 04:35:49AM 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(). Let me give it a > >> >> >try first. > >> >> > > >> >> >I decided to introduce a NO_BQL bit 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 in one go. > >> >> > > >> >> >As Jakub said, "netdev_uses_bql() is best effort", I think, we can add > >> >> >new non-BQL drivers as soon as we find one. > >> >> > > >> >> >After this patch, there is no byte_queue_limits directory in virtio-net > >> >> >driver. > >> >> > >> >> Please note following patch is currently trying to push bql support for > >> >> virtio_net: > >> >> https://lore.kernel.org/netdev/20240612170851.1004604-1-jiri@resnulli.us/ > >> > > >> >I saw this one this morning and I'm reviewing/testing it. > >> > > >> >> > >> >> When that is merged, this patch is not needed. Could we wait? > >> > > >> >Please note this patch is not only written for virtio_net driver. > >> >Virtio_net driver is one of possible cases. > >> > >> Yeah, but without virtio_net, there will be no users. What's the point > >> of having that in code? I mean, in general, no-user kernel code gets > >> removed. > > > >Are you sure netdev_uses_bql() can limit all the non-bql drivers with > >those two checks? I haven't investigated this part. > > Nope. What I say is, if there are other users, let's find them and let > them use what you are introducing here. Otherwise don't add unused code. Please take a look at what I just wrote. I found one. > > > > >> > >> > >> > > >> >After your patch gets merged (I think it will take some time), you > >> >could simply remove that one line in virtio_net.c. > >> > > >> >Thanks.
On Thu, Jun 13, 2024 at 09:51:00AM +0200, Jiri Pirko wrote: > Thu, Jun 13, 2024 at 09:24:27AM CEST, kerneljasonxing@gmail.com wrote: > >On Thu, Jun 13, 2024 at 3:19 PM Jiri Pirko <jiri@resnulli.us> wrote: > >> > >> Thu, Jun 13, 2024 at 08:08:36AM CEST, kerneljasonxing@gmail.com wrote: > >> >On Thu, Jun 13, 2024 at 1:38 PM Jiri Pirko <jiri@resnulli.us> wrote: > >> >> > >> >> Thu, Jun 13, 2024 at 04:35:49AM 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(). Let me give it a > >> >> >try first. > >> >> > > >> >> >I decided to introduce a NO_BQL bit 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 in one go. > >> >> > > >> >> >As Jakub said, "netdev_uses_bql() is best effort", I think, we can add > >> >> >new non-BQL drivers as soon as we find one. > >> >> > > >> >> >After this patch, there is no byte_queue_limits directory in virtio-net > >> >> >driver. > >> >> > >> >> Please note following patch is currently trying to push bql support for > >> >> virtio_net: > >> >> https://lore.kernel.org/netdev/20240612170851.1004604-1-jiri@resnulli.us/ > >> > > >> >I saw this one this morning and I'm reviewing/testing it. > >> > > >> >> > >> >> When that is merged, this patch is not needed. Could we wait? > >> > > >> >Please note this patch is not only written for virtio_net driver. > >> >Virtio_net driver is one of possible cases. > >> > >> Yeah, but without virtio_net, there will be no users. What's the point > >> of having that in code? I mean, in general, no-user kernel code gets > >> removed. > > > >Are you sure netdev_uses_bql() can limit all the non-bql drivers with > >those two checks? I haven't investigated this part. > > Nope. What I say is, if there are other users, let's find them and let > them use what you are introducing here. Otherwise don't add unused code. Additionally, it looks like virtio is going to become a "sometimes BQL sometimes no-BQL" driver, so what's the plan - to set/clear the flag accordingly then? What kind of locking will be needed? > > > >> > >> > >> > > >> >After your patch gets merged (I think it will take some time), you > >> >could simply remove that one line in virtio_net.c. > >> > > >> >Thanks.
On Thu, Jun 13, 2024 at 3:56 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Thu, Jun 13, 2024 at 09:51:00AM +0200, Jiri Pirko wrote: > > Thu, Jun 13, 2024 at 09:24:27AM CEST, kerneljasonxing@gmail.com wrote: > > >On Thu, Jun 13, 2024 at 3:19 PM Jiri Pirko <jiri@resnulli.us> wrote: > > >> > > >> Thu, Jun 13, 2024 at 08:08:36AM CEST, kerneljasonxing@gmail.com wrote: > > >> >On Thu, Jun 13, 2024 at 1:38 PM Jiri Pirko <jiri@resnulli.us> wrote: > > >> >> > > >> >> Thu, Jun 13, 2024 at 04:35:49AM 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(). Let me give it a > > >> >> >try first. > > >> >> > > > >> >> >I decided to introduce a NO_BQL bit 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 in one go. > > >> >> > > > >> >> >As Jakub said, "netdev_uses_bql() is best effort", I think, we can add > > >> >> >new non-BQL drivers as soon as we find one. > > >> >> > > > >> >> >After this patch, there is no byte_queue_limits directory in virtio-net > > >> >> >driver. > > >> >> > > >> >> Please note following patch is currently trying to push bql support for > > >> >> virtio_net: > > >> >> https://lore.kernel.org/netdev/20240612170851.1004604-1-jiri@resnulli.us/ > > >> > > > >> >I saw this one this morning and I'm reviewing/testing it. > > >> > > > >> >> > > >> >> When that is merged, this patch is not needed. Could we wait? > > >> > > > >> >Please note this patch is not only written for virtio_net driver. > > >> >Virtio_net driver is one of possible cases. > > >> > > >> Yeah, but without virtio_net, there will be no users. What's the point > > >> of having that in code? I mean, in general, no-user kernel code gets > > >> removed. > > > > > >Are you sure netdev_uses_bql() can limit all the non-bql drivers with > > >those two checks? I haven't investigated this part. > > > > Nope. What I say is, if there are other users, let's find them and let > > them use what you are introducing here. Otherwise don't add unused code. > > > Additionally, it looks like virtio is going to become a > "sometimes BQL sometimes no-BQL" driver, so what's the plan - > to set/clear the flag accordingly then? What kind of locking > will be needed? Could we consider the default mode is BQL, so we can remove that new IFF_NO_BQL flag? If it's hard to take care of these two situations, I think we could follow this suggestion from Jakub: "netdev_uses_bql() is best effort". What do you think? > > > > > > >> > > >> > > >> > > > >> >After your patch gets merged (I think it will take some time), you > > >> >could simply remove that one line in virtio_net.c. > > >> > > > >> >Thanks. >
On Thu, Jun 13, 2024 at 5:26 PM Jason Xing <kerneljasonxing@gmail.com> wrote: > > On Thu, Jun 13, 2024 at 3:56 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > On Thu, Jun 13, 2024 at 09:51:00AM +0200, Jiri Pirko wrote: > > > Thu, Jun 13, 2024 at 09:24:27AM CEST, kerneljasonxing@gmail.com wrote: > > > >On Thu, Jun 13, 2024 at 3:19 PM Jiri Pirko <jiri@resnulli.us> wrote: > > > >> > > > >> Thu, Jun 13, 2024 at 08:08:36AM CEST, kerneljasonxing@gmail.com wrote: > > > >> >On Thu, Jun 13, 2024 at 1:38 PM Jiri Pirko <jiri@resnulli.us> wrote: > > > >> >> > > > >> >> Thu, Jun 13, 2024 at 04:35:49AM 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(). Let me give it a > > > >> >> >try first. > > > >> >> > > > > >> >> >I decided to introduce a NO_BQL bit 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 in one go. > > > >> >> > > > > >> >> >As Jakub said, "netdev_uses_bql() is best effort", I think, we can add > > > >> >> >new non-BQL drivers as soon as we find one. > > > >> >> > > > > >> >> >After this patch, there is no byte_queue_limits directory in virtio-net > > > >> >> >driver. > > > >> >> > > > >> >> Please note following patch is currently trying to push bql support for > > > >> >> virtio_net: > > > >> >> https://lore.kernel.org/netdev/20240612170851.1004604-1-jiri@resnulli.us/ > > > >> > > > > >> >I saw this one this morning and I'm reviewing/testing it. > > > >> > > > > >> >> > > > >> >> When that is merged, this patch is not needed. Could we wait? > > > >> > > > > >> >Please note this patch is not only written for virtio_net driver. > > > >> >Virtio_net driver is one of possible cases. > > > >> > > > >> Yeah, but without virtio_net, there will be no users. What's the point > > > >> of having that in code? I mean, in general, no-user kernel code gets > > > >> removed. > > > > > > > >Are you sure netdev_uses_bql() can limit all the non-bql drivers with > > > >those two checks? I haven't investigated this part. > > > > > > Nope. What I say is, if there are other users, let's find them and let > > > them use what you are introducing here. Otherwise don't add unused code. > > > > > > Additionally, it looks like virtio is going to become a > > "sometimes BQL sometimes no-BQL" driver, so what's the plan - > > to set/clear the flag accordingly then? What kind of locking > > will be needed? > > Could we consider the default mode is BQL, so we can remove that new > IFF_NO_BQL flag? If it's hard to take care of these two situations, I > think we could follow this suggestion from Jakub: "netdev_uses_bql() > is best effort". What do you think? ENA driver faces the same 'problem' because it also has BQL and no-BQL. Honestly, I don't want to spend too much time on this patch because it's not worth it. Allow me to set the IFF_NO_BQL flag for the default mode to try our 'best effort' for virtio_net and ENA driver? Thanks, Jason
Thu, Jun 13, 2024 at 11:26:05AM CEST, kerneljasonxing@gmail.com wrote: >On Thu, Jun 13, 2024 at 3:56 PM Michael S. Tsirkin <mst@redhat.com> wrote: >> >> On Thu, Jun 13, 2024 at 09:51:00AM +0200, Jiri Pirko wrote: >> > Thu, Jun 13, 2024 at 09:24:27AM CEST, kerneljasonxing@gmail.com wrote: >> > >On Thu, Jun 13, 2024 at 3:19 PM Jiri Pirko <jiri@resnulli.us> wrote: >> > >> >> > >> Thu, Jun 13, 2024 at 08:08:36AM CEST, kerneljasonxing@gmail.com wrote: >> > >> >On Thu, Jun 13, 2024 at 1:38 PM Jiri Pirko <jiri@resnulli.us> wrote: >> > >> >> >> > >> >> Thu, Jun 13, 2024 at 04:35:49AM 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(). Let me give it a >> > >> >> >try first. >> > >> >> > >> > >> >> >I decided to introduce a NO_BQL bit 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 in one go. >> > >> >> > >> > >> >> >As Jakub said, "netdev_uses_bql() is best effort", I think, we can add >> > >> >> >new non-BQL drivers as soon as we find one. >> > >> >> > >> > >> >> >After this patch, there is no byte_queue_limits directory in virtio-net >> > >> >> >driver. >> > >> >> >> > >> >> Please note following patch is currently trying to push bql support for >> > >> >> virtio_net: >> > >> >> https://lore.kernel.org/netdev/20240612170851.1004604-1-jiri@resnulli.us/ >> > >> > >> > >> >I saw this one this morning and I'm reviewing/testing it. >> > >> > >> > >> >> >> > >> >> When that is merged, this patch is not needed. Could we wait? >> > >> > >> > >> >Please note this patch is not only written for virtio_net driver. >> > >> >Virtio_net driver is one of possible cases. >> > >> >> > >> Yeah, but without virtio_net, there will be no users. What's the point >> > >> of having that in code? I mean, in general, no-user kernel code gets >> > >> removed. >> > > >> > >Are you sure netdev_uses_bql() can limit all the non-bql drivers with >> > >those two checks? I haven't investigated this part. >> > >> > Nope. What I say is, if there are other users, let's find them and let >> > them use what you are introducing here. Otherwise don't add unused code. >> >> >> Additionally, it looks like virtio is going to become a >> "sometimes BQL sometimes no-BQL" driver, so what's the plan - >> to set/clear the flag accordingly then? What kind of locking >> will be needed? > >Could we consider the default mode is BQL, so we can remove that new >IFF_NO_BQL flag? If it's hard to take care of these two situations, I >think we could follow this suggestion from Jakub: "netdev_uses_bql() >is best effort". What do you think? Make sense. Also, note that virtio_net bql utilization is going to be not only dynamically configured, but also per-queue. It would be hard to expose that over one device flag :) > >> >> > > >> > >> >> > >> >> > >> > >> > >> >After your patch gets merged (I think it will take some time), you >> > >> >could simply remove that one line in virtio_net.c. >> > >> > >> > >> >Thanks. >>
On Thu, Jun 13, 2024 at 9:17 PM Jiri Pirko <jiri@resnulli.us> wrote: > > Thu, Jun 13, 2024 at 11:26:05AM CEST, kerneljasonxing@gmail.com wrote: > >On Thu, Jun 13, 2024 at 3:56 PM Michael S. Tsirkin <mst@redhat.com> wrote: > >> > >> On Thu, Jun 13, 2024 at 09:51:00AM +0200, Jiri Pirko wrote: > >> > Thu, Jun 13, 2024 at 09:24:27AM CEST, kerneljasonxing@gmail.com wrote: > >> > >On Thu, Jun 13, 2024 at 3:19 PM Jiri Pirko <jiri@resnulli.us> wrote: > >> > >> > >> > >> Thu, Jun 13, 2024 at 08:08:36AM CEST, kerneljasonxing@gmail.com wrote: > >> > >> >On Thu, Jun 13, 2024 at 1:38 PM Jiri Pirko <jiri@resnulli.us> wrote: > >> > >> >> > >> > >> >> Thu, Jun 13, 2024 at 04:35:49AM 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(). Let me give it a > >> > >> >> >try first. > >> > >> >> > > >> > >> >> >I decided to introduce a NO_BQL bit 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 in one go. > >> > >> >> > > >> > >> >> >As Jakub said, "netdev_uses_bql() is best effort", I think, we can add > >> > >> >> >new non-BQL drivers as soon as we find one. > >> > >> >> > > >> > >> >> >After this patch, there is no byte_queue_limits directory in virtio-net > >> > >> >> >driver. > >> > >> >> > >> > >> >> Please note following patch is currently trying to push bql support for > >> > >> >> virtio_net: > >> > >> >> https://lore.kernel.org/netdev/20240612170851.1004604-1-jiri@resnulli.us/ > >> > >> > > >> > >> >I saw this one this morning and I'm reviewing/testing it. > >> > >> > > >> > >> >> > >> > >> >> When that is merged, this patch is not needed. Could we wait? > >> > >> > > >> > >> >Please note this patch is not only written for virtio_net driver. > >> > >> >Virtio_net driver is one of possible cases. > >> > >> > >> > >> Yeah, but without virtio_net, there will be no users. What's the point > >> > >> of having that in code? I mean, in general, no-user kernel code gets > >> > >> removed. > >> > > > >> > >Are you sure netdev_uses_bql() can limit all the non-bql drivers with > >> > >those two checks? I haven't investigated this part. > >> > > >> > Nope. What I say is, if there are other users, let's find them and let > >> > them use what you are introducing here. Otherwise don't add unused code. > >> > >> > >> Additionally, it looks like virtio is going to become a > >> "sometimes BQL sometimes no-BQL" driver, so what's the plan - > >> to set/clear the flag accordingly then? What kind of locking > >> will be needed? > > > >Could we consider the default mode is BQL, so we can remove that new > >IFF_NO_BQL flag? If it's hard to take care of these two situations, I > >think we could follow this suggestion from Jakub: "netdev_uses_bql() > >is best effort". What do you think? > > Make sense. > > Also, note that virtio_net bql utilization is going to be not only > dynamically configured, but also per-queue. It would be hard to expose > that over one device flag :) At that time, I would let virtio_net driver go, that is to say, I wouldn't take it into consideration in netdev_uses_bql() since it's too complicated. BTW, hope to see your per-queue configured feature patchset soon :) > > > > > >> > >> > > > >> > >> > >> > >> > >> > >> > > >> > >> >After your patch gets merged (I think it will take some time), you > >> > >> >could simply remove that one line in virtio_net.c. > >> > >> > > >> > >> >Thanks. > >>
Thu, Jun 13, 2024 at 04:11:12PM CEST, kerneljasonxing@gmail.com wrote: >On Thu, Jun 13, 2024 at 9:17 PM Jiri Pirko <jiri@resnulli.us> wrote: >> >> Thu, Jun 13, 2024 at 11:26:05AM CEST, kerneljasonxing@gmail.com wrote: >> >On Thu, Jun 13, 2024 at 3:56 PM Michael S. Tsirkin <mst@redhat.com> wrote: >> >> >> >> On Thu, Jun 13, 2024 at 09:51:00AM +0200, Jiri Pirko wrote: >> >> > Thu, Jun 13, 2024 at 09:24:27AM CEST, kerneljasonxing@gmail.com wrote: >> >> > >On Thu, Jun 13, 2024 at 3:19 PM Jiri Pirko <jiri@resnulli.us> wrote: >> >> > >> >> >> > >> Thu, Jun 13, 2024 at 08:08:36AM CEST, kerneljasonxing@gmail.com wrote: >> >> > >> >On Thu, Jun 13, 2024 at 1:38 PM Jiri Pirko <jiri@resnulli.us> wrote: >> >> > >> >> >> >> > >> >> Thu, Jun 13, 2024 at 04:35:49AM 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(). Let me give it a >> >> > >> >> >try first. >> >> > >> >> > >> >> > >> >> >I decided to introduce a NO_BQL bit 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 in one go. >> >> > >> >> > >> >> > >> >> >As Jakub said, "netdev_uses_bql() is best effort", I think, we can add >> >> > >> >> >new non-BQL drivers as soon as we find one. >> >> > >> >> > >> >> > >> >> >After this patch, there is no byte_queue_limits directory in virtio-net >> >> > >> >> >driver. >> >> > >> >> >> >> > >> >> Please note following patch is currently trying to push bql support for >> >> > >> >> virtio_net: >> >> > >> >> https://lore.kernel.org/netdev/20240612170851.1004604-1-jiri@resnulli.us/ >> >> > >> > >> >> > >> >I saw this one this morning and I'm reviewing/testing it. >> >> > >> > >> >> > >> >> >> >> > >> >> When that is merged, this patch is not needed. Could we wait? >> >> > >> > >> >> > >> >Please note this patch is not only written for virtio_net driver. >> >> > >> >Virtio_net driver is one of possible cases. >> >> > >> >> >> > >> Yeah, but without virtio_net, there will be no users. What's the point >> >> > >> of having that in code? I mean, in general, no-user kernel code gets >> >> > >> removed. >> >> > > >> >> > >Are you sure netdev_uses_bql() can limit all the non-bql drivers with >> >> > >those two checks? I haven't investigated this part. >> >> > >> >> > Nope. What I say is, if there are other users, let's find them and let >> >> > them use what you are introducing here. Otherwise don't add unused code. >> >> >> >> >> >> Additionally, it looks like virtio is going to become a >> >> "sometimes BQL sometimes no-BQL" driver, so what's the plan - >> >> to set/clear the flag accordingly then? What kind of locking >> >> will be needed? >> > >> >Could we consider the default mode is BQL, so we can remove that new >> >IFF_NO_BQL flag? If it's hard to take care of these two situations, I >> >think we could follow this suggestion from Jakub: "netdev_uses_bql() >> >is best effort". What do you think? >> >> Make sense. >> >> Also, note that virtio_net bql utilization is going to be not only >> dynamically configured, but also per-queue. It would be hard to expose >> that over one device flag :) > >At that time, I would let virtio_net driver go, that is to say, I >wouldn't take it into consideration in netdev_uses_bql() since it's >too complicated. > >BTW, hope to see your per-queue configured feature patchset soon :) It's done already. See virtnet_set_per_queue_coalesce() if ec->tx_max_coalesced_frames is 0, napi_weight is set to 0 and napi orphan mode is used. > >> >> >> > >> >> >> >> > > >> >> > >> >> >> > >> >> >> > >> > >> >> > >> >After your patch gets merged (I think it will take some time), you >> >> > >> >could simply remove that one line in virtio_net.c. >> >> > >> > >> >> > >> >Thanks. >> >>
On Thu, Jun 13, 2024 at 10:34 PM Jiri Pirko <jiri@resnulli.us> wrote: > > Thu, Jun 13, 2024 at 04:11:12PM CEST, kerneljasonxing@gmail.com wrote: > >On Thu, Jun 13, 2024 at 9:17 PM Jiri Pirko <jiri@resnulli.us> wrote: > >> > >> Thu, Jun 13, 2024 at 11:26:05AM CEST, kerneljasonxing@gmail.com wrote: > >> >On Thu, Jun 13, 2024 at 3:56 PM Michael S. Tsirkin <mst@redhat.com> wrote: > >> >> > >> >> On Thu, Jun 13, 2024 at 09:51:00AM +0200, Jiri Pirko wrote: > >> >> > Thu, Jun 13, 2024 at 09:24:27AM CEST, kerneljasonxing@gmail.com wrote: > >> >> > >On Thu, Jun 13, 2024 at 3:19 PM Jiri Pirko <jiri@resnulli.us> wrote: > >> >> > >> > >> >> > >> Thu, Jun 13, 2024 at 08:08:36AM CEST, kerneljasonxing@gmail.com wrote: > >> >> > >> >On Thu, Jun 13, 2024 at 1:38 PM Jiri Pirko <jiri@resnulli.us> wrote: > >> >> > >> >> > >> >> > >> >> Thu, Jun 13, 2024 at 04:35:49AM 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(). Let me give it a > >> >> > >> >> >try first. > >> >> > >> >> > > >> >> > >> >> >I decided to introduce a NO_BQL bit 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 in one go. > >> >> > >> >> > > >> >> > >> >> >As Jakub said, "netdev_uses_bql() is best effort", I think, we can add > >> >> > >> >> >new non-BQL drivers as soon as we find one. > >> >> > >> >> > > >> >> > >> >> >After this patch, there is no byte_queue_limits directory in virtio-net > >> >> > >> >> >driver. > >> >> > >> >> > >> >> > >> >> Please note following patch is currently trying to push bql support for > >> >> > >> >> virtio_net: > >> >> > >> >> https://lore.kernel.org/netdev/20240612170851.1004604-1-jiri@resnulli.us/ > >> >> > >> > > >> >> > >> >I saw this one this morning and I'm reviewing/testing it. > >> >> > >> > > >> >> > >> >> > >> >> > >> >> When that is merged, this patch is not needed. Could we wait? > >> >> > >> > > >> >> > >> >Please note this patch is not only written for virtio_net driver. > >> >> > >> >Virtio_net driver is one of possible cases. > >> >> > >> > >> >> > >> Yeah, but without virtio_net, there will be no users. What's the point > >> >> > >> of having that in code? I mean, in general, no-user kernel code gets > >> >> > >> removed. > >> >> > > > >> >> > >Are you sure netdev_uses_bql() can limit all the non-bql drivers with > >> >> > >those two checks? I haven't investigated this part. > >> >> > > >> >> > Nope. What I say is, if there are other users, let's find them and let > >> >> > them use what you are introducing here. Otherwise don't add unused code. > >> >> > >> >> > >> >> Additionally, it looks like virtio is going to become a > >> >> "sometimes BQL sometimes no-BQL" driver, so what's the plan - > >> >> to set/clear the flag accordingly then? What kind of locking > >> >> will be needed? > >> > > >> >Could we consider the default mode is BQL, so we can remove that new > >> >IFF_NO_BQL flag? If it's hard to take care of these two situations, I > >> >think we could follow this suggestion from Jakub: "netdev_uses_bql() > >> >is best effort". What do you think? > >> > >> Make sense. > >> > >> Also, note that virtio_net bql utilization is going to be not only > >> dynamically configured, but also per-queue. It would be hard to expose > >> that over one device flag :) > > > >At that time, I would let virtio_net driver go, that is to say, I > >wouldn't take it into consideration in netdev_uses_bql() since it's > >too complicated. > > > >BTW, hope to see your per-queue configured feature patchset soon :) > > It's done already. See virtnet_set_per_queue_coalesce() > if ec->tx_max_coalesced_frames is 0, napi_weight is set to 0 and napi > orphan mode is used. Oh right, I missed that... Thanks for reminding me.
On Thu, Jun 13, 2024 at 10:35 AM 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(). Let me give it a > try first. > > I decided to introduce a NO_BQL bit 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 in one go. > > As Jakub said, "netdev_uses_bql() is best effort", I think, we can add > new non-BQL drivers as soon as we find one. > > After this patch, there is no byte_queue_limits directory in virtio-net > driver. > > Signed-off-by: Jason Xing <kernelxing@tencent.com> Hello Jakub, I wonder why the status of this patch was changed to 'Changes Requested'? Is there anything else I should adjust? Thanks, Jason
On Thu, 13 Jun 2024 22:55:16 +0800 Jason Xing wrote: > I wonder why the status of this patch was changed to 'Changes > Requested'? Is there anything else I should adjust? Sorry to flip the question on you, but do you think the patch should be merged as is? Given Jiri is adding BQL support to virtio?
On Thu, Jun 13, 2024 at 5:02 PM Jakub Kicinski <kuba@kernel.org> wrote: > > On Thu, 13 Jun 2024 22:55:16 +0800 Jason Xing wrote: > > I wonder why the status of this patch was changed to 'Changes > > Requested'? Is there anything else I should adjust? > > Sorry to flip the question on you, but do you think the patch should > be merged as is? Given Jiri is adding BQL support to virtio? Also what is the rationale for all this discussion ? Don't we have many sys files that are never used anyway ?
On Thu, Jun 13, 2024 at 11:02 PM Jakub Kicinski <kuba@kernel.org> wrote: > > On Thu, 13 Jun 2024 22:55:16 +0800 Jason Xing wrote: > > I wonder why the status of this patch was changed to 'Changes > > Requested'? Is there anything else I should adjust? > > Sorry to flip the question on you, but do you think the patch should > be merged as is? Given Jiri is adding BQL support to virtio? I think we should, but you are the maintainer :) One of the previous emails I wrote: ENA driver also uses no-BQL mode by default. I believe there are other drivers like this, so the purpose of this patch aims at those missing drivers.
On Thu, Jun 13, 2024 at 11:26 PM Eric Dumazet <edumazet@google.com> wrote: > > On Thu, Jun 13, 2024 at 5:02 PM Jakub Kicinski <kuba@kernel.org> wrote: > > > > On Thu, 13 Jun 2024 22:55:16 +0800 Jason Xing wrote: > > > I wonder why the status of this patch was changed to 'Changes > > > Requested'? Is there anything else I should adjust? > > > > Sorry to flip the question on you, but do you think the patch should > > be merged as is? Given Jiri is adding BQL support to virtio? > > Also what is the rationale for all this discussion ? > > Don't we have many sys files that are never used anyway ? At the very beginning, I thought the current patch is very simple and easy to get merged because I just found other non-BQL drivers passing the checks in netdev_uses_bql(). Also see the commit: commit 74293ea1c4db62cb969e741fbfd479a34d935024 Author: Breno Leitao <leitao@debian.org> Date: Fri Feb 16 01:41:52 2024 -0800 net: sysfs: Do not create sysfs for non BQL device 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> Link: https://lore.kernel.org/r/20240216094154.3263843-1-leitao@debian.org Signed-off-by: Jakub Kicinski <kuba@kernel.org> I followed this patch and introduced a flag only. Actually, it's against my expectations. It involved too many discussions. As I said again: at the very beginning, I thought it's very easy to get merged... :(
On Thu, Jun 13, 2024 at 5:37 PM Jason Xing <kerneljasonxing@gmail.com> wrote: > > On Thu, Jun 13, 2024 at 11:26 PM Eric Dumazet <edumazet@google.com> wrote: > > > > On Thu, Jun 13, 2024 at 5:02 PM Jakub Kicinski <kuba@kernel.org> wrote: > > > > > > On Thu, 13 Jun 2024 22:55:16 +0800 Jason Xing wrote: > > > > I wonder why the status of this patch was changed to 'Changes > > > > Requested'? Is there anything else I should adjust? > > > > > > Sorry to flip the question on you, but do you think the patch should > > > be merged as is? Given Jiri is adding BQL support to virtio? > > > > Also what is the rationale for all this discussion ? > > > > Don't we have many sys files that are never used anyway ? > > At the very beginning, I thought the current patch is very simple and > easy to get merged because I just found other non-BQL drivers passing > the checks in netdev_uses_bql(). Also see the commit: > Suggested-by: Eric Dumazet <edumazet@google.com> > Signed-off-by: Breno Leitao <leitao@debian.org> > Link: https://lore.kernel.org/r/20240216094154.3263843-1-leitao@debian.org > Signed-off-by: Jakub Kicinski <kuba@kernel.org> > > I followed this patch and introduced a flag only. > > Actually, it's against my expectations. It involved too many > discussions. As I said again: at the very beginning, I thought it's > very easy to get merged... :( I think you missed the point of the original suggestion leading to Breno patch. At Google, we create gazillion of netns per minute, with few virtual drivers in them (like loopback, ipvlan) This was pretty easy to avoid /sys/class/net/lo/queues/tx-0/byte_queue_limits/* creation. cpu savings for little investment. But when it comes to physical devices, I really do not see the benefit of being picky about some sysfs files. So let me repeat my question : Why do you need this ?
On Thu, Jun 13, 2024 at 11:49 PM Eric Dumazet <edumazet@google.com> wrote: > > On Thu, Jun 13, 2024 at 5:37 PM Jason Xing <kerneljasonxing@gmail.com> wrote: > > > > On Thu, Jun 13, 2024 at 11:26 PM Eric Dumazet <edumazet@google.com> wrote: > > > > > > On Thu, Jun 13, 2024 at 5:02 PM Jakub Kicinski <kuba@kernel.org> wrote: > > > > > > > > On Thu, 13 Jun 2024 22:55:16 +0800 Jason Xing wrote: > > > > > I wonder why the status of this patch was changed to 'Changes > > > > > Requested'? Is there anything else I should adjust? > > > > > > > > Sorry to flip the question on you, but do you think the patch should > > > > be merged as is? Given Jiri is adding BQL support to virtio? > > > > > > Also what is the rationale for all this discussion ? > > > > > > Don't we have many sys files that are never used anyway ? > > > > At the very beginning, I thought the current patch is very simple and > > easy to get merged because I just found other non-BQL drivers passing > > the checks in netdev_uses_bql(). Also see the commit: > > > Suggested-by: Eric Dumazet <edumazet@google.com> > > Signed-off-by: Breno Leitao <leitao@debian.org> > > Link: https://lore.kernel.org/r/20240216094154.3263843-1-leitao@debian.org > > Signed-off-by: Jakub Kicinski <kuba@kernel.org> > > > > I followed this patch and introduced a flag only. > > > > Actually, it's against my expectations. It involved too many > > discussions. As I said again: at the very beginning, I thought it's > > very easy to get merged... :( > > I think you missed the point of the original suggestion leading to Breno patch. > > At Google, we create gazillion of netns per minute, with few virtual > drivers in them (like loopback, ipvlan) > > This was pretty easy to avoid /sys/class/net/lo/queues/tx-0/byte_queue_limits/* > creation. cpu savings for little investment. > > But when it comes to physical devices, I really do not see the benefit > of being picky about some sysfs files. > > So let me repeat my question : Why do you need this ? Sometimes people can see the sysfs files, sometimes not. They may get confused. For non-BQL drivers, those sysfs files are totally needless (not working) with one single flag. Well, Eric, I don't expect this patch to keep involving more discussions. If you or Jakub or other maintainers decide to reject this patch, I'm fine. I can take it. We spend too much time on this trivial patch. Many patches like this are trivial, not worth spending you too much precious time on this. As I said, I thought it's very simple and easy... Thanks for your explanation. Thanks, Jason
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index 61a57d134544..728f4b9844cc 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -5631,7 +5631,7 @@ static int virtnet_probe(struct virtio_device *vdev) /* Set up network device as normal. */ dev->priv_flags |= IFF_UNICAST_FLT | IFF_LIVE_ADDR_CHANGE | - IFF_TX_SKB_NO_LINEAR; + IFF_TX_SKB_NO_LINEAR | IFF_NO_BQL; dev->netdev_ops = &virtnet_netdev; dev->stat_ops = &virtnet_stat_ops; dev->features = NETIF_F_HIGHDMA; diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index f148a01dd1d1..d371c2b425ca 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -1649,6 +1649,8 @@ struct net_device_ops { * @IFF_SEE_ALL_HWTSTAMP_REQUESTS: device wants to see calls to * ndo_hwtstamp_set() for all timestamp requests regardless of source, * even if those aren't HWTSTAMP_SOURCE_NETDEV. + * @IFF_NO_BQL: driver doesn't support BQL, don't create "byte_queue_limits" + * directories in sysfs. */ enum netdev_priv_flags { IFF_802_1Q_VLAN = 1<<0, @@ -1685,6 +1687,7 @@ enum netdev_priv_flags { IFF_TX_SKB_NO_LINEAR = BIT_ULL(31), IFF_CHANGE_PROTO_DOWN = BIT_ULL(32), IFF_SEE_ALL_HWTSTAMP_REQUESTS = BIT_ULL(33), + IFF_NO_BQL = BIT_ULL(34), }; #define IFF_802_1Q_VLAN IFF_802_1Q_VLAN diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c index 4c27a360c294..7d99fbbad6af 100644 --- a/net/core/net-sysfs.c +++ b/net/core/net-sysfs.c @@ -1765,7 +1765,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 || - dev->priv_flags & IFF_NO_QUEUE) + dev->priv_flags & (IFF_NO_QUEUE | IFF_NO_BQL)) return false; return IS_ENABLED(CONFIG_BQL);