| Message ID | 20210120033455.4034611-1-weiwan@google.com |
|---|---|
| Headers | show |
| Series | implement kthread based napi poll | expand |
On Wed, Jan 20, 2021 at 9:29 AM Florian Fainelli <f.fainelli@gmail.com> wrote: > > > > On 1/19/2021 7:34 PM, Wei Wang wrote: > > This patch adds a new sysfs attribute to the network device class. > > Said attribute provides a per-device control to enable/disable the > > threaded mode for all the napi instances of the given network device. > > User sets it to 1 or 0 to enable or disable threaded mode per device. > > However, when user reads from this sysfs entry, it could return: > > 1: means all napi instances belonging to this device have threaded > > mode enabled. > > 0: means all napi instances belonging to this device have threaded > > mode disabled. > > -1: means the system fails to enable threaded mode for certain napi > > instances when user requests to enable threaded mode. This happens > > when the kthread fails to be created for certain napi instances. > > > > Co-developed-by: Paolo Abeni <pabeni@redhat.com> > > Signed-off-by: Paolo Abeni <pabeni@redhat.com> > > Co-developed-by: Hannes Frederic Sowa <hannes@stressinduktion.org> > > Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org> > > Co-developed-by: Felix Fietkau <nbd@nbd.name> > > Signed-off-by: Felix Fietkau <nbd@nbd.name> > > Signed-off-by: Wei Wang <weiwan@google.com> > > Can you document the new threaded sysfs attribute under > Documentation/ABI/testing/sysfs-class-net? OK. Will do. > -- > Florian
On Wed, Jan 20, 2021 at 10:07 AM Wei Wang <weiwan@google.com> wrote: > > On Wed, Jan 20, 2021 at 8:13 AM Alexander Duyck > <alexander.duyck@gmail.com> wrote: > > > > On Tue, Jan 19, 2021 at 7:35 PM Wei Wang <weiwan@google.com> wrote: > > > > > > This patch adds a new sysfs attribute to the network device class. > > > Said attribute provides a per-device control to enable/disable the > > > threaded mode for all the napi instances of the given network device. > > > User sets it to 1 or 0 to enable or disable threaded mode per device. > > > However, when user reads from this sysfs entry, it could return: > > > 1: means all napi instances belonging to this device have threaded > > > mode enabled. > > > 0: means all napi instances belonging to this device have threaded > > > mode disabled. > > > -1: means the system fails to enable threaded mode for certain napi > > > instances when user requests to enable threaded mode. This happens > > > when the kthread fails to be created for certain napi instances. > > > > > > Co-developed-by: Paolo Abeni <pabeni@redhat.com> > > > Signed-off-by: Paolo Abeni <pabeni@redhat.com> > > > Co-developed-by: Hannes Frederic Sowa <hannes@stressinduktion.org> > > > Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org> > > > Co-developed-by: Felix Fietkau <nbd@nbd.name> > > > Signed-off-by: Felix Fietkau <nbd@nbd.name> > > > Signed-off-by: Wei Wang <weiwan@google.com> > > > --- > > > include/linux/netdevice.h | 2 ++ > > > net/core/dev.c | 28 ++++++++++++++++ > > > net/core/net-sysfs.c | 68 +++++++++++++++++++++++++++++++++++++++ > > > 3 files changed, 98 insertions(+) > > > > > > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h > > > index 8cb8d43ea5fa..26c3e8cf4c01 100644 > > > --- a/include/linux/netdevice.h > > > +++ b/include/linux/netdevice.h > > > @@ -497,6 +497,8 @@ static inline bool napi_complete(struct napi_struct *n) > > > return napi_complete_done(n, 0); > > > } > > > > > > +int dev_set_threaded(struct net_device *dev, bool threaded); > > > + > > > /** > > > * napi_disable - prevent NAPI from scheduling > > > * @n: NAPI context > > > diff --git a/net/core/dev.c b/net/core/dev.c > > > index 7ffa91475856..e71c2fd91595 100644 > > > --- a/net/core/dev.c > > > +++ b/net/core/dev.c > > > @@ -6767,6 +6767,34 @@ static int napi_set_threaded(struct napi_struct *n, bool threaded) > > > return 0; > > > } > > > > > > +static void dev_disable_threaded_all(struct net_device *dev) > > > +{ > > > + struct napi_struct *napi; > > > + > > > + list_for_each_entry(napi, &dev->napi_list, dev_list) > > > + napi_set_threaded(napi, false); > > > +} > > > + > > > +int dev_set_threaded(struct net_device *dev, bool threaded) > > > +{ > > > + struct napi_struct *napi; > > > + int ret; > > > + > > > + dev->threaded = threaded; > > > + list_for_each_entry(napi, &dev->napi_list, dev_list) { > > > + ret = napi_set_threaded(napi, threaded); > > > + if (ret) { > > > + /* Error occurred on one of the napi, > > > + * reset threaded mode on all napi. > > > + */ > > > + dev_disable_threaded_all(dev); > > > + break; > > > + } > > > + } > > > + > > > + return ret; > > > +} > > > + > > > > So I have a question about this function. Is there any reason why > > napi_set_threaded couldn't be broken into two pieces and handled in > > two passes with the first allocating the kthread and the second > > setting the threaded bit assuming the allocations all succeeded? The > > setting or clearing of the bit shouldn't need any return value since > > it is void and the allocation of the kthread is the piece that can > > fail. So it seems like it would make sense to see if you can allocate > > all of the kthreads first before you go through and attempt to enable > > threaded NAPI. > > > > Then you should only need to make a change to netif_napi_add that will > > allocate the kthread if adding a new instance on a device that is > > running in threaded mode and if a thread allocation fails you could > > clear dev->threaded so that when napi_enable is called we don't bother > > enabling any threaded setups since some of the threads are > > non-functional. > > > If we create kthreads during netif_napi_add() when dev->threaded is > set, that means the user has to bring down the device, set > dev->threaded and then bring up the device in order to use threaded > mode. I think this brings extra steps, while we could have made it > easier. I believe being able to live switch between on and off without > device up/down is a good and useful thing. I think you missed my point. I wasn't saying you needed to change this code to call netif_napi_add. What I was suggesting is breaking this into two passes. The first pass would allocate the kthreads and call the function you would likely use in the netif_napi_add call, and the second would set the threaded bit. That way you don't have queues in limbo during the process where you enable them and then disable them. Right now if the device doesn't have any napi instances enabled and you make this call all it will be doing is setting dev->threaded. So the thought is to try to reproduce this two step approach at a driver level. Then when napi queues are added you would have to add the kthread to the new napi threads. You are currently doing that in the napi_enable call. I would argue that it should happen in the netif_napi_add so you can add them long before the napi is enabled. Unfortunately that still isn't perfect as it seems some drivers go immediately from netif_napi_add to napi_enable, however many will add all of their NAPI instances before they enable them so in that case you could have similar functionality to this enable call where first pass does the allocation, and then if it all succeeded you then enable the feature on all the queues when you call napi_enable and only modify the bit there instead of trying to do the allocation. > The other way to do this might be that we always create the kthread > during netif_napi_add() regardless of the dev->threaded value. And > when user requests to enable threaded mode, we only enable it, after > checking every napi has its kthread created. It doesn't make sense to always allocate the thread. I would only do that if the netdev has dev->threaded set. Otherwise you can take care of allocating it here when you toggle the value and leave it allocated until the napi instance is deleted. > But this also has its drawbacks. First, it means there will be several > idle kthreads hanging around even if the user never enables threaded > mode. Also, there is still the possibility that the kthread creation As I said, I would only do the allocation if dev->threaded is set which means the device wants the threaded NAPI. Also if any allocation fails we should clear the bit on all existing napi instances and clear dev->threaded. As far as freeing the kthreads that would probably need to wait until we delete the NAPI instance. So we would need to add a check somewhere to make sure we aren't overwriting existing kthreads in the allocation. > fails. But since netif_napi_add() does not have a return value, no one > will handle it. Do we just never enable threaded mode in this case? Or > do we try to recreate the thread when the user tries to enable > threaded mode through the sysfs interface? Right now we could handle this the same way we do for napi_enable which is to not enable the feature if the thread isn't there. The only difference is I would take it one step further and probably require that dev->threaded be cleared on an allocation failure. Then before we set the threaded bit we have to test for it being set and the kthread is allocated before we allow setting the bit. Otherwise we clear the threaded bit.
On Wed, Jan 20, 2021 at 7:36 PM Alexander Duyck <alexander.duyck@gmail.com> wrote: > > On Wed, Jan 20, 2021 at 10:07 AM Wei Wang <weiwan@google.com> wrote: > > > > On Wed, Jan 20, 2021 at 8:13 AM Alexander Duyck > > <alexander.duyck@gmail.com> wrote: > > > > > > On Tue, Jan 19, 2021 at 7:35 PM Wei Wang <weiwan@google.com> wrote: > > > > > > > > This patch adds a new sysfs attribute to the network device class. > > > > Said attribute provides a per-device control to enable/disable the > > > > threaded mode for all the napi instances of the given network device. > > > > User sets it to 1 or 0 to enable or disable threaded mode per device. > > > > However, when user reads from this sysfs entry, it could return: > > > > 1: means all napi instances belonging to this device have threaded > > > > mode enabled. > > > > 0: means all napi instances belonging to this device have threaded > > > > mode disabled. > > > > -1: means the system fails to enable threaded mode for certain napi > > > > instances when user requests to enable threaded mode. This happens > > > > when the kthread fails to be created for certain napi instances. > > > > > > > > Co-developed-by: Paolo Abeni <pabeni@redhat.com> > > > > Signed-off-by: Paolo Abeni <pabeni@redhat.com> > > > > Co-developed-by: Hannes Frederic Sowa <hannes@stressinduktion.org> > > > > Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org> > > > > Co-developed-by: Felix Fietkau <nbd@nbd.name> > > > > Signed-off-by: Felix Fietkau <nbd@nbd.name> > > > > Signed-off-by: Wei Wang <weiwan@google.com> > > > > --- > > > > include/linux/netdevice.h | 2 ++ > > > > net/core/dev.c | 28 ++++++++++++++++ > > > > net/core/net-sysfs.c | 68 +++++++++++++++++++++++++++++++++++++++ > > > > 3 files changed, 98 insertions(+) > > > > > > > > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h > > > > index 8cb8d43ea5fa..26c3e8cf4c01 100644 > > > > --- a/include/linux/netdevice.h > > > > +++ b/include/linux/netdevice.h > > > > @@ -497,6 +497,8 @@ static inline bool napi_complete(struct napi_struct *n) > > > > return napi_complete_done(n, 0); > > > > } > > > > > > > > +int dev_set_threaded(struct net_device *dev, bool threaded); > > > > + > > > > /** > > > > * napi_disable - prevent NAPI from scheduling > > > > * @n: NAPI context > > > > diff --git a/net/core/dev.c b/net/core/dev.c > > > > index 7ffa91475856..e71c2fd91595 100644 > > > > --- a/net/core/dev.c > > > > +++ b/net/core/dev.c > > > > @@ -6767,6 +6767,34 @@ static int napi_set_threaded(struct napi_struct *n, bool threaded) > > > > return 0; > > > > } > > > > > > > > +static void dev_disable_threaded_all(struct net_device *dev) > > > > +{ > > > > + struct napi_struct *napi; > > > > + > > > > + list_for_each_entry(napi, &dev->napi_list, dev_list) > > > > + napi_set_threaded(napi, false); > > > > +} > > > > + > > > > +int dev_set_threaded(struct net_device *dev, bool threaded) > > > > +{ > > > > + struct napi_struct *napi; > > > > + int ret; > > > > + > > > > + dev->threaded = threaded; > > > > + list_for_each_entry(napi, &dev->napi_list, dev_list) { > > > > + ret = napi_set_threaded(napi, threaded); > > > > + if (ret) { > > > > + /* Error occurred on one of the napi, > > > > + * reset threaded mode on all napi. > > > > + */ > > > > + dev_disable_threaded_all(dev); > > > > + break; > > > > + } > > > > + } > > > > + > > > > + return ret; > > > > +} > > > > + > > > > > > So I have a question about this function. Is there any reason why > > > napi_set_threaded couldn't be broken into two pieces and handled in > > > two passes with the first allocating the kthread and the second > > > setting the threaded bit assuming the allocations all succeeded? The > > > setting or clearing of the bit shouldn't need any return value since > > > it is void and the allocation of the kthread is the piece that can > > > fail. So it seems like it would make sense to see if you can allocate > > > all of the kthreads first before you go through and attempt to enable > > > threaded NAPI. > > > > > > Then you should only need to make a change to netif_napi_add that will > > > allocate the kthread if adding a new instance on a device that is > > > running in threaded mode and if a thread allocation fails you could > > > clear dev->threaded so that when napi_enable is called we don't bother > > > enabling any threaded setups since some of the threads are > > > non-functional. > > > > > If we create kthreads during netif_napi_add() when dev->threaded is > > set, that means the user has to bring down the device, set > > dev->threaded and then bring up the device in order to use threaded > > mode. I think this brings extra steps, while we could have made it > > easier. I believe being able to live switch between on and off without > > device up/down is a good and useful thing. > > I think you missed my point. I wasn't saying you needed to change > this code to call netif_napi_add. What I was suggesting is breaking > this into two passes. The first pass would allocate the kthreads and > call the function you would likely use in the netif_napi_add call, and > the second would set the threaded bit. That way you don't have queues > in limbo during the process where you enable them and then disable > them. > > Right now if the device doesn't have any napi instances enabled and > you make this call all it will be doing is setting dev->threaded. So > the thought is to try to reproduce this two step approach at a driver > level. > > Then when napi queues are added you would have to add the kthread to > the new napi threads. You are currently doing that in the napi_enable > call. I would argue that it should happen in the netif_napi_add so you > can add them long before the napi is enabled. Unfortunately that still > isn't perfect as it seems some drivers go immediately from > netif_napi_add to napi_enable, however many will add all of their NAPI > instances before they enable them so in that case you could have > similar functionality to this enable call where first pass does the > allocation, and then if it all succeeded you then enable the feature > on all the queues when you call napi_enable and only modify the bit > there instead of trying to do the allocation. > > > The other way to do this might be that we always create the kthread > > during netif_napi_add() regardless of the dev->threaded value. And > > when user requests to enable threaded mode, we only enable it, after > > checking every napi has its kthread created. > > It doesn't make sense to always allocate the thread. I would only do > that if the netdev has dev->threaded set. Otherwise you can take care > of allocating it here when you toggle the value and leave it allocated > until the napi instance is deleted. > > > But this also has its drawbacks. First, it means there will be several > > idle kthreads hanging around even if the user never enables threaded > > mode. Also, there is still the possibility that the kthread creation > > As I said, I would only do the allocation if dev->threaded is set > which means the device wants the threaded NAPI. > > Also if any allocation fails we should clear the bit on all existing > napi instances and clear dev->threaded. As far as freeing the kthreads > that would probably need to wait until we delete the NAPI instance. So > we would need to add a check somewhere to make sure we aren't > overwriting existing kthreads in the allocation. > > > fails. But since netif_napi_add() does not have a return value, no one > > will handle it. Do we just never enable threaded mode in this case? Or > > do we try to recreate the thread when the user tries to enable > > threaded mode through the sysfs interface? > > Right now we could handle this the same way we do for napi_enable > which is to not enable the feature if the thread isn't there. The only > difference is I would take it one step further and probably require > that dev->threaded be cleared on an allocation failure. Then before we > set the threaded bit we have to test for it being set and the kthread > is allocated before we allow setting the bit. Otherwise we clear the > threaded bit. Thanks for the clarification. I think I got it. The only inconsistency would be the case where you've mentioned that if the driver calls napi_enable() immediately after netif_napi_add() without adding all the napi instances in advance. I would ignore that case and still use dev->threaded to indicate whether threaded mode is enabled or not. This way, we could get rid of the ternary value on dev->threaded.
The idea of moving the napi poll process out of softirq context to a kernel thread based context is not new. Paolo Abeni and Hannes Frederic Sowa have proposed patches to move napi poll to kthread back in 2016. And Felix Fietkau has also proposed patches of similar ideas to use workqueue to process napi poll just a few weeks ago. The main reason we'd like to push forward with this idea is that the scheduler has poor visibility into cpu cycles spent in softirq context, and is not able to make optimal scheduling decisions of the user threads. For example, we see in one of the application benchmark where network load is high, the CPUs handling network softirqs has ~80% cpu util. And user threads are still scheduled on those CPUs, despite other more idle cpus available in the system. And we see very high tail latencies. In this case, we have to explicitly pin away user threads from the CPUs handling network softirqs to ensure good performance. With napi poll moved to kthread, scheduler is in charge of scheduling both the kthreads handling network load, and the user threads, and is able to make better decisions. In the previous benchmark, if we do this and we pin the kthreads processing napi poll to specific CPUs, scheduler is able to schedule user threads away from these CPUs automatically. And the reason we prefer 1 kthread per napi, instead of 1 workqueue entity per host, is that kthread is more configurable than workqueue, and we could leverage existing tuning tools for threads, like taskset, chrt, etc to tune scheduling class and cpu set, etc. Another reason is if we eventually want to provide busy poll feature using kernel threads for napi poll, kthread seems to be more suitable than workqueue. Furthermore, for large platforms with 2 NICs attached to 2 sockets, kthread is more flexible to be pinned to different sets of CPUs. In this patch series, I revived Paolo and Hannes's patch in 2016 and made modifications. Then there are changes proposed by Felix, Jakub, Paolo and myself on top of those, with suggestions from Eric Dumazet. In terms of performance, I ran tcp_rr tests with 1000 flows with various request/response sizes, with RFS/RPS disabled, and compared performance between softirq vs kthread vs workqueue (patchset proposed by Felix Fietkau). Host has 56 hyper threads and 100Gbps nic, 8 rx queues and only 1 numa node. All threads are unpinned. req/resp QPS 50%tile 90%tile 99%tile 99.9%tile softirq 1B/1B 2.75M 337us 376us 1.04ms 3.69ms kthread 1B/1B 2.67M 371us 408us 455us 550us workq 1B/1B 2.56M 384us 435us 673us 822us softirq 5KB/5KB 1.46M 678us 750us 969us 2.78ms kthread 5KB/5KB 1.44M 695us 789us 891us 1.06ms workq 5KB/5KB 1.34M 720us 905us 1.06ms 1.57ms softirq 1MB/1MB 11.0K 79ms 166ms 306ms 630ms kthread 1MB/1MB 11.0K 75ms 177ms 303ms 596ms workq 1MB/1MB 11.0K 79ms 180ms 303ms 587ms When running workqueue implementation, I found the number of threads used is usually twice as much as kthread implementation. This probably introduces higher scheduling cost, which results in higher tail latencies in most cases. I also ran an application benchmark, which performs fixed qps remote SSD read/write operations, with various sizes. Again, both with RFS/RPS disabled. The result is as follows: op_size QPS 50%tile 95%tile 99%tile 99.9%tile softirq 4K 572.6K 385us 1.5ms 3.16ms 6.41ms kthread 4K 572.6K 390us 803us 2.21ms 6.83ms workq 4k 572.6K 384us 763us 3.12ms 6.87ms softirq 64K 157.9K 736us 1.17ms 3.40ms 13.75ms kthread 64K 157.9K 745us 1.23ms 2.76ms 9.87ms workq 64K 157.9K 746us 1.23ms 2.76ms 9.96ms softirq 1M 10.98K 2.03ms 3.10ms 3.7ms 11.56ms kthread 1M 10.98K 2.13ms 3.21ms 4.02ms 13.3ms workq 1M 10.98K 2.13ms 3.20ms 3.99ms 14.12ms In this set of tests, the latency is predominant by the SSD operation. Also, the user threads are much busier compared to tcp_rr tests. We have to pin the kthreads/workqueue threads to limit to a few CPUs, to not disturb user threads, and provide some isolation. Changes since v6: Added memory barrier in napi_set_threaded(). Changed /sys/class/net/<dev>/thread to a ternary value. Change dev->threaded to a bit instead of bool. Changes since v5: Removed ASSERT_RTNL() from napi_set_threaded() and removed rtnl_lock() operation from napi_enable(). Changes since v4: Recorded the threaded setting in dev and restore it in napi_enable(). Changes since v3: Merged and rearranged patches in a logical order for easier review. Changed sysfs control to be per device. Changes since v2: Corrected typo in patch 1, and updated the cover letter with more detailed and updated test results. Changes since v1: Replaced kthread_create() with kthread_run() in patch 5 as suggested by Felix Fietkau. Changes since RFC: Renamed the kthreads to be napi/<dev>-<napi_id> in patch 5 as suggested by Hannes Frederic Sowa. Felix Fietkau (1): net: extract napi poll functionality to __napi_poll() Wei Wang (2): net: implement threaded-able napi poll loop support net: add sysfs attribute to control napi threaded mode include/linux/netdevice.h | 21 ++-- net/core/dev.c | 200 ++++++++++++++++++++++++++++++++++++-- net/core/net-sysfs.c | 68 +++++++++++++ 3 files changed, 265 insertions(+), 24 deletions(-)