| Message ID | 20210115003123.1254314-3-weiwan@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On Thu, Jan 14, 2021 at 4:33 PM Wei Wang <weiwan@google.com> wrote: > > This patch allows running each napi poll loop inside its own > kernel thread. > The threaded mode could be enabled through napi_set_threaded() > api, and does not require a device up/down. The kthread gets > created on demand when napi_set_threaded() is called, and gets > shut down eventually in napi_disable(). > > Once that threaded mode is enabled and the kthread is > started, napi_schedule() will wake-up such thread instead > of scheduling the softirq. > > The threaded poll loop behaves quite likely the net_rx_action, > but it does not have to manipulate local irqs and uses > an explicit scheduling point based on netdev_budget. > > 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: Jakub Kicinski <kuba@kernel.org> > Signed-off-by: Jakub Kicinski <kuba@kernel.org> > Signed-off-by: Wei Wang <weiwan@google.com> > --- > include/linux/netdevice.h | 12 ++-- > net/core/dev.c | 113 ++++++++++++++++++++++++++++++++++++++ > 2 files changed, 118 insertions(+), 7 deletions(-) > > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h > index 5b949076ed23..c24ed232c746 100644 > --- a/include/linux/netdevice.h > +++ b/include/linux/netdevice.h > @@ -347,6 +347,7 @@ struct napi_struct { > struct list_head dev_list; > struct hlist_node napi_hash_node; > unsigned int napi_id; > + struct task_struct *thread; > }; > > enum { > @@ -358,6 +359,7 @@ enum { > NAPI_STATE_NO_BUSY_POLL, /* Do not add in napi_hash, no busy polling */ > NAPI_STATE_IN_BUSY_POLL, /* sk_busy_loop() owns this NAPI */ > NAPI_STATE_PREFER_BUSY_POLL, /* prefer busy-polling over softirq processing*/ > + NAPI_STATE_THREADED, /* The poll is performed inside its own thread*/ > }; > > enum { > @@ -369,6 +371,7 @@ enum { > NAPIF_STATE_NO_BUSY_POLL = BIT(NAPI_STATE_NO_BUSY_POLL), > NAPIF_STATE_IN_BUSY_POLL = BIT(NAPI_STATE_IN_BUSY_POLL), > NAPIF_STATE_PREFER_BUSY_POLL = BIT(NAPI_STATE_PREFER_BUSY_POLL), > + NAPIF_STATE_THREADED = BIT(NAPI_STATE_THREADED), > }; > > enum gro_result { > @@ -510,13 +513,7 @@ void napi_disable(struct napi_struct *n); > * Resume NAPI from being scheduled on this context. > * Must be paired with napi_disable. > */ > -static inline void napi_enable(struct napi_struct *n) > -{ > - BUG_ON(!test_bit(NAPI_STATE_SCHED, &n->state)); > - smp_mb__before_atomic(); > - clear_bit(NAPI_STATE_SCHED, &n->state); > - clear_bit(NAPI_STATE_NPSVC, &n->state); > -} > +void napi_enable(struct napi_struct *n); If you are reducing the function to just a prototype it might make sense to move the doxygen comments to the definition of the function rather than leaving them here. That way when you change the functionality it is easier to update the comments as it is all in the same place. > > /** > * napi_synchronize - wait until NAPI is not running > @@ -2140,6 +2137,7 @@ struct net_device { > struct lock_class_key *qdisc_tx_busylock; > struct lock_class_key *qdisc_running_key; > bool proto_down; > + bool threaded; > unsigned wol_enabled:1; I would prefer to see this done like the wol_enabled as a bit field or at least converted to a u8 instead of a bool. Last I knew we weren't really supposed to use bool in structures since some architectures will make it a 32b value. > struct list_head net_notifier_list; > diff --git a/net/core/dev.c b/net/core/dev.c > index 83b59e4c0f37..edcfec1361e9 100644 > --- a/net/core/dev.c > +++ b/net/core/dev.c > @@ -91,6 +91,7 @@ > #include <linux/etherdevice.h> > #include <linux/ethtool.h> > #include <linux/skbuff.h> > +#include <linux/kthread.h> > #include <linux/bpf.h> > #include <linux/bpf_trace.h> > #include <net/net_namespace.h> > @@ -1493,6 +1494,36 @@ void netdev_notify_peers(struct net_device *dev) > } > EXPORT_SYMBOL(netdev_notify_peers); > > +static int napi_threaded_poll(void *data); > + > +static int napi_kthread_create(struct napi_struct *n) > +{ > + int err = 0; > + > + /* Create and wake up the kthread once to put it in > + * TASK_INTERRUPTIBLE mode to avoid the blocked task > + * warning and work with loadavg. > + */ > + n->thread = kthread_run(napi_threaded_poll, n, "napi/%s-%d", > + n->dev->name, n->napi_id); > + if (IS_ERR(n->thread)) { > + err = PTR_ERR(n->thread); > + pr_err("kthread_run failed with err %d\n", err); > + n->thread = NULL; > + } > + > + return err; > +} > + > +static void napi_kthread_stop(struct napi_struct *n) > +{ > + if (!n->thread) > + return; I would add a blank line after this if statement. > + kthread_stop(n->thread); > + clear_bit(NAPI_STATE_THREADED, &n->state); > + n->thread = NULL; > +} > + > static int __dev_open(struct net_device *dev, struct netlink_ext_ack *extack) > { > const struct net_device_ops *ops = dev->netdev_ops; > @@ -4252,6 +4283,11 @@ int gro_normal_batch __read_mostly = 8; > static inline void ____napi_schedule(struct softnet_data *sd, > struct napi_struct *napi) > { > + if (test_bit(NAPI_STATE_THREADED, &napi->state)) { > + wake_up_process(napi->thread); > + return; > + } > + > list_add_tail(&napi->poll_list, &sd->poll_list); > __raise_softirq_irqoff(NET_RX_SOFTIRQ); > } > @@ -6697,6 +6733,27 @@ static void init_gro_hash(struct napi_struct *napi) > napi->gro_bitmask = 0; > } > > +static int napi_set_threaded(struct napi_struct *n, bool threaded) > +{ > + int err = 0; > + > + if (threaded == !!test_bit(NAPI_STATE_THREADED, &n->state)) > + return 0; A blank line between these two statements might be nice. > + if (threaded) { > + if (!n->thread) { > + err = napi_kthread_create(n); > + if (err) > + goto out; > + } > + set_bit(NAPI_STATE_THREADED, &n->state); > + } else { > + clear_bit(NAPI_STATE_THREADED, &n->state); > + } You could probably flatten this bit by doing: if (!threaded) { clear_bit(NAPI_STATE_THREADED, &n->state); return 0; } Then you could just pull the rest of this out of the if statement and flatten it a bit. > + > +out: > + return err; > +} > + > void netif_napi_add(struct net_device *dev, struct napi_struct *napi, > int (*poll)(struct napi_struct *, int), int weight) > { > @@ -6738,12 +6795,23 @@ void napi_disable(struct napi_struct *n) > msleep(1); > > hrtimer_cancel(&n->timer); > + napi_kthread_stop(n); > > clear_bit(NAPI_STATE_PREFER_BUSY_POLL, &n->state); > clear_bit(NAPI_STATE_DISABLE, &n->state); > } > EXPORT_SYMBOL(napi_disable); > > +void napi_enable(struct napi_struct *n) > +{ > + BUG_ON(!test_bit(NAPI_STATE_SCHED, &n->state)); > + smp_mb__before_atomic(); > + clear_bit(NAPI_STATE_SCHED, &n->state); > + clear_bit(NAPI_STATE_NPSVC, &n->state); > + WARN_ON(napi_set_threaded(n, n->dev->threaded)); I am not sure what the point is in having a return value if you are just using it to trigger a WARN_ON. It might make more sense to actually set the WARN_ON inside of napi_set_threaded instead of having it here as you could then identify the error much more easily. Or for that matter you might be able to use something like pr_warn which would allow you a more detailed message about the specific netdev that experienced the failure. > +} > +EXPORT_SYMBOL(napi_enable); > + > static void flush_gro_hash(struct napi_struct *napi) > { > int i; > @@ -6866,6 +6934,51 @@ static int napi_poll(struct napi_struct *n, struct list_head *repoll) > return work; > } > > +static int napi_thread_wait(struct napi_struct *napi) > +{ > + set_current_state(TASK_INTERRUPTIBLE); > + > + while (!kthread_should_stop() && !napi_disable_pending(napi)) { > + if (test_bit(NAPI_STATE_SCHED, &napi->state)) { > + WARN_ON(!list_empty(&napi->poll_list)); > + __set_current_state(TASK_RUNNING); > + return 0; > + } > + > + schedule(); > + set_current_state(TASK_INTERRUPTIBLE); > + } > + __set_current_state(TASK_RUNNING); > + return -1; > +} > + > +static int napi_threaded_poll(void *data) > +{ > + struct napi_struct *napi = data; > + void *have; > + > + while (!napi_thread_wait(napi)) { > + for (;;) { > + bool repoll = false; > + > + local_bh_disable(); > + > + have = netpoll_poll_lock(napi); > + __napi_poll(napi, &repoll); > + netpoll_poll_unlock(have); > + So it looks like v3 of this patch set was making use of the budget but for v4 that went away and you had this. I was wondering why? One thing that seems odd to me is that we are limiting the device to only clearing one NAPI_POLL_WEIGHT of packets between flushing the freed skbs and reenabling the bottom halves. I'm wondering if it might make more sense to have a tunable like netdev_budget that could be used for this to allow for more time between flushes, bh enables, and cond_resched checks. > + __kfree_skb_flush(); > + local_bh_enable(); > + > + if (!repoll) > + break; > + > + cond_resched(); > + } > + } > + return 0; > +} > + > static __latent_entropy void net_rx_action(struct softirq_action *h) > { > struct softnet_data *sd = this_cpu_ptr(&softnet_data); > -- > 2.30.0.284.gd98b1dd5eaa7-goog >
On Thu, Jan 14, 2021 at 7:14 PM Alexander Duyck <alexander.duyck@gmail.com> wrote: > > On Thu, Jan 14, 2021 at 4:33 PM Wei Wang <weiwan@google.com> wrote: > > <snip> > > +void napi_enable(struct napi_struct *n) > > +{ > > + BUG_ON(!test_bit(NAPI_STATE_SCHED, &n->state)); > > + smp_mb__before_atomic(); > > + clear_bit(NAPI_STATE_SCHED, &n->state); > > + clear_bit(NAPI_STATE_NPSVC, &n->state); > > + WARN_ON(napi_set_threaded(n, n->dev->threaded)); > > I am not sure what the point is in having a return value if you are > just using it to trigger a WARN_ON. It might make more sense to > actually set the WARN_ON inside of napi_set_threaded instead of having > it here as you could then identify the error much more easily. Or for > that matter you might be able to use something like pr_warn which > would allow you a more detailed message about the specific netdev that > experienced the failure. One additional change I would make here. The call to napi_set_threaded should be moved to before the smp_mb__before_atomic(). That way we can guarantee that the threaded flag and task_struct pointer are visible to all consumers before they can set NAPI_STATE_SCHED. Otherwise I think we run the risk of a race where a napi request could fire before we have finished configuring it.
Resending the replies in plain text mode to avoid delivery failure. On Fri, Jan 15, 2021 at 1:38 PM Wei Wang <weiwan@google.com> wrote: > > > > On Thu, Jan 14, 2021 at 7:14 PM Alexander Duyck <alexander.duyck@gmail.com> wrote: >> >> On Thu, Jan 14, 2021 at 4:33 PM Wei Wang <weiwan@google.com> wrote: >> > >> > This patch allows running each napi poll loop inside its own >> > kernel thread. >> > The threaded mode could be enabled through napi_set_threaded() >> > api, and does not require a device up/down. The kthread gets >> > created on demand when napi_set_threaded() is called, and gets >> > shut down eventually in napi_disable(). >> > >> > Once that threaded mode is enabled and the kthread is >> > started, napi_schedule() will wake-up such thread instead >> > of scheduling the softirq. >> > >> > The threaded poll loop behaves quite likely the net_rx_action, >> > but it does not have to manipulate local irqs and uses >> > an explicit scheduling point based on netdev_budget. >> > >> > 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: Jakub Kicinski <kuba@kernel.org> >> > Signed-off-by: Jakub Kicinski <kuba@kernel.org> >> > Signed-off-by: Wei Wang <weiwan@google.com> >> > --- >> > include/linux/netdevice.h | 12 ++-- >> > net/core/dev.c | 113 ++++++++++++++++++++++++++++++++++++++ >> > 2 files changed, 118 insertions(+), 7 deletions(-) >> > >> > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h >> > index 5b949076ed23..c24ed232c746 100644 >> > --- a/include/linux/netdevice.h >> > +++ b/include/linux/netdevice.h >> > @@ -347,6 +347,7 @@ struct napi_struct { >> > struct list_head dev_list; >> > struct hlist_node napi_hash_node; >> > unsigned int napi_id; >> > + struct task_struct *thread; >> > }; >> > >> > enum { >> > @@ -358,6 +359,7 @@ enum { >> > NAPI_STATE_NO_BUSY_POLL, /* Do not add in napi_hash, no busy polling */ >> > NAPI_STATE_IN_BUSY_POLL, /* sk_busy_loop() owns this NAPI */ >> > NAPI_STATE_PREFER_BUSY_POLL, /* prefer busy-polling over softirq processing*/ >> > + NAPI_STATE_THREADED, /* The poll is performed inside its own thread*/ >> > }; >> > >> > enum { >> > @@ -369,6 +371,7 @@ enum { >> > NAPIF_STATE_NO_BUSY_POLL = BIT(NAPI_STATE_NO_BUSY_POLL), >> > NAPIF_STATE_IN_BUSY_POLL = BIT(NAPI_STATE_IN_BUSY_POLL), >> > NAPIF_STATE_PREFER_BUSY_POLL = BIT(NAPI_STATE_PREFER_BUSY_POLL), >> > + NAPIF_STATE_THREADED = BIT(NAPI_STATE_THREADED), >> > }; >> > >> > enum gro_result { >> > @@ -510,13 +513,7 @@ void napi_disable(struct napi_struct *n); >> > * Resume NAPI from being scheduled on this context. >> > * Must be paired with napi_disable. >> > */ >> > -static inline void napi_enable(struct napi_struct *n) >> > -{ >> > - BUG_ON(!test_bit(NAPI_STATE_SCHED, &n->state)); >> > - smp_mb__before_atomic(); >> > - clear_bit(NAPI_STATE_SCHED, &n->state); >> > - clear_bit(NAPI_STATE_NPSVC, &n->state); >> > -} >> > +void napi_enable(struct napi_struct *n); >> >> If you are reducing the function to just a prototype it might make >> sense to move the doxygen comments to the definition of the function >> rather than leaving them here. That way when you change the >> functionality it is easier to update the comments as it is all in the >> same place. >> >> > >> > /** >> > * napi_synchronize - wait until NAPI is not running >> > @@ -2140,6 +2137,7 @@ struct net_device { >> > struct lock_class_key *qdisc_tx_busylock; >> > struct lock_class_key *qdisc_running_key; >> > bool proto_down; >> > + bool threaded; >> > unsigned wol_enabled:1; >> >> I would prefer to see this done like the wol_enabled as a bit field >> or at least converted to a u8 instead of a bool. Last I knew we >> weren't really supposed to use bool in structures since some >> architectures will make it a 32b value. >> >> >> > struct list_head net_notifier_list; >> > diff --git a/net/core/dev.c b/net/core/dev.c >> > index 83b59e4c0f37..edcfec1361e9 100644 >> > --- a/net/core/dev.c >> > +++ b/net/core/dev.c >> > @@ -91,6 +91,7 @@ >> > #include <linux/etherdevice.h> >> > #include <linux/ethtool.h> >> > #include <linux/skbuff.h> >> > +#include <linux/kthread.h> >> > #include <linux/bpf.h> >> > #include <linux/bpf_trace.h> >> > #include <net/net_namespace.h> >> > @@ -1493,6 +1494,36 @@ void netdev_notify_peers(struct net_device *dev) >> > } >> > EXPORT_SYMBOL(netdev_notify_peers); >> > >> > +static int napi_threaded_poll(void *data); >> > + >> > +static int napi_kthread_create(struct napi_struct *n) >> > +{ >> > + int err = 0; >> > + >> > + /* Create and wake up the kthread once to put it in >> > + * TASK_INTERRUPTIBLE mode to avoid the blocked task >> > + * warning and work with loadavg. >> > + */ >> > + n->thread = kthread_run(napi_threaded_poll, n, "napi/%s-%d", >> > + n->dev->name, n->napi_id); >> > + if (IS_ERR(n->thread)) { >> > + err = PTR_ERR(n->thread); >> > + pr_err("kthread_run failed with err %d\n", err); >> > + n->thread = NULL; >> > + } >> > + >> > + return err; >> > +} >> > + >> > +static void napi_kthread_stop(struct napi_struct *n) >> > +{ >> > + if (!n->thread) >> > + return; >> >> I would add a blank line after this if statement. >> >> > + kthread_stop(n->thread); >> > + clear_bit(NAPI_STATE_THREADED, &n->state); >> > + n->thread = NULL; >> > +} >> > + >> > static int __dev_open(struct net_device *dev, struct netlink_ext_ack *extack) >> > { >> > const struct net_device_ops *ops = dev->netdev_ops; >> > @@ -4252,6 +4283,11 @@ int gro_normal_batch __read_mostly = 8; >> > static inline void ____napi_schedule(struct softnet_data *sd, >> > struct napi_struct *napi) >> > { >> > + if (test_bit(NAPI_STATE_THREADED, &napi->state)) { >> > + wake_up_process(napi->thread); >> > + return; >> > + } >> > + >> > list_add_tail(&napi->poll_list, &sd->poll_list); >> > __raise_softirq_irqoff(NET_RX_SOFTIRQ); >> > } >> > @@ -6697,6 +6733,27 @@ static void init_gro_hash(struct napi_struct *napi) >> > napi->gro_bitmask = 0; >> > } >> > >> > +static int napi_set_threaded(struct napi_struct *n, bool threaded) >> > +{ >> > + int err = 0; >> > + >> > + if (threaded == !!test_bit(NAPI_STATE_THREADED, &n->state)) >> > + return 0; >> >> A blank line between these two statements might be nice. >> >> > + if (threaded) { >> > + if (!n->thread) { >> > + err = napi_kthread_create(n); >> > + if (err) >> > + goto out; >> > + } >> > + set_bit(NAPI_STATE_THREADED, &n->state); >> > + } else { >> > + clear_bit(NAPI_STATE_THREADED, &n->state); >> > + } >> >> You could probably flatten this bit by doing: >> if (!threaded) { >> clear_bit(NAPI_STATE_THREADED, &n->state); >> return 0; >> } >> >> Then you could just pull the rest of this out of the if statement and >> flatten it a bit. > > > > Ack for all comments before this. Will update in next version. > >> >> >> > + >> > +out: >> > + return err; >> > +} >> > + >> > void netif_napi_add(struct net_device *dev, struct napi_struct *napi, >> > int (*poll)(struct napi_struct *, int), int weight) >> > { >> > @@ -6738,12 +6795,23 @@ void napi_disable(struct napi_struct *n) >> > msleep(1); >> > >> > hrtimer_cancel(&n->timer); >> > + napi_kthread_stop(n); >> > >> > clear_bit(NAPI_STATE_PREFER_BUSY_POLL, &n->state); >> > clear_bit(NAPI_STATE_DISABLE, &n->state); >> > } >> > EXPORT_SYMBOL(napi_disable); >> > >> > +void napi_enable(struct napi_struct *n) >> > +{ >> > + BUG_ON(!test_bit(NAPI_STATE_SCHED, &n->state)); >> > + smp_mb__before_atomic(); >> > + clear_bit(NAPI_STATE_SCHED, &n->state); >> > + clear_bit(NAPI_STATE_NPSVC, &n->state); >> > + WARN_ON(napi_set_threaded(n, n->dev->threaded)); >> >> I am not sure what the point is in having a return value if you are >> just using it to trigger a WARN_ON. It might make more sense to >> actually set the WARN_ON inside of napi_set_threaded instead of having >> it here as you could then identify the error much more easily. Or for >> that matter you might be able to use something like pr_warn which >> would allow you a more detailed message about the specific netdev that >> experienced the failure. > > > This function is called from modify_napi_threaded() from sysfs path. The return value is used from that path. > >> >> >> > +} >> > +EXPORT_SYMBOL(napi_enable); >> > + >> > static void flush_gro_hash(struct napi_struct *napi) >> > { >> > int i; >> > @@ -6866,6 +6934,51 @@ static int napi_poll(struct napi_struct *n, struct list_head *repoll) >> > return work; >> > } >> > >> > +static int napi_thread_wait(struct napi_struct *napi) >> > +{ >> > + set_current_state(TASK_INTERRUPTIBLE); >> > + >> > + while (!kthread_should_stop() && !napi_disable_pending(napi)) { >> > + if (test_bit(NAPI_STATE_SCHED, &napi->state)) { >> > + WARN_ON(!list_empty(&napi->poll_list)); >> > + __set_current_state(TASK_RUNNING); >> > + return 0; >> > + } >> > + >> > + schedule(); >> > + set_current_state(TASK_INTERRUPTIBLE); >> > + } >> > + __set_current_state(TASK_RUNNING); >> > + return -1; >> > +} >> > + >> > +static int napi_threaded_poll(void *data) >> > +{ >> > + struct napi_struct *napi = data; >> > + void *have; >> > + >> > + while (!napi_thread_wait(napi)) { >> > + for (;;) { >> > + bool repoll = false; >> > + >> > + local_bh_disable(); >> > + >> > + have = netpoll_poll_lock(napi); >> > + __napi_poll(napi, &repoll); >> > + netpoll_poll_unlock(have); >> > + >> >> So it looks like v3 of this patch set was making use of the budget but >> for v4 that went away and you had this. I was wondering why? One thing >> that seems odd to me is that we are limiting the device to only > > > In v3, the bugdet existed in the previous proposed patch from Paolo. It also got modified to this same implementation in patch 4 in v3. > >> >> clearing one NAPI_POLL_WEIGHT of packets between flushing the freed >> skbs and reenabling the bottom halves. >> >> >> I'm wondering if it might make more sense to have a tunable like >> netdev_budget that could be used for this to allow for more time >> between flushes, bh enables, and cond_resched checks. > > > I think this depends on how long we want to keep the CPU before yielding. And I think there is a slight difference between this function vs napi_schedule() called from net_rx_action(). > net_rx_action() has a 'budget' parameter. But in that function, there are potentially multiple napi instances queued. And for each napi, it only gets polled for max of NAPI_POLL_WEIGHT as well. The 'bugdet' is actually controlling how many napi instances to poll at most. > In napi_threaded_poll(), it is only responsible for polling 1 napi instance. So the behavior here is somewhat consistent: the napi instance gets polled once before reenabling bottem half, and yeild ng the CPU. > >> >> >> > + __kfree_skb_flush(); >> > + local_bh_enable(); >> > + >> > + if (!repoll) >> > + break; >> > + >> > + cond_resched(); >> > + } >> > + } >> > + return 0; >> > +} >> > + >> > static __latent_entropy void net_rx_action(struct softirq_action *h) >> > { >> > struct softnet_data *sd = this_cpu_ptr(&softnet_data); >> > -- >> > 2.30.0.284.gd98b1dd5eaa7-goog >> >
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index 5b949076ed23..c24ed232c746 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -347,6 +347,7 @@ struct napi_struct { struct list_head dev_list; struct hlist_node napi_hash_node; unsigned int napi_id; + struct task_struct *thread; }; enum { @@ -358,6 +359,7 @@ enum { NAPI_STATE_NO_BUSY_POLL, /* Do not add in napi_hash, no busy polling */ NAPI_STATE_IN_BUSY_POLL, /* sk_busy_loop() owns this NAPI */ NAPI_STATE_PREFER_BUSY_POLL, /* prefer busy-polling over softirq processing*/ + NAPI_STATE_THREADED, /* The poll is performed inside its own thread*/ }; enum { @@ -369,6 +371,7 @@ enum { NAPIF_STATE_NO_BUSY_POLL = BIT(NAPI_STATE_NO_BUSY_POLL), NAPIF_STATE_IN_BUSY_POLL = BIT(NAPI_STATE_IN_BUSY_POLL), NAPIF_STATE_PREFER_BUSY_POLL = BIT(NAPI_STATE_PREFER_BUSY_POLL), + NAPIF_STATE_THREADED = BIT(NAPI_STATE_THREADED), }; enum gro_result { @@ -510,13 +513,7 @@ void napi_disable(struct napi_struct *n); * Resume NAPI from being scheduled on this context. * Must be paired with napi_disable. */ -static inline void napi_enable(struct napi_struct *n) -{ - BUG_ON(!test_bit(NAPI_STATE_SCHED, &n->state)); - smp_mb__before_atomic(); - clear_bit(NAPI_STATE_SCHED, &n->state); - clear_bit(NAPI_STATE_NPSVC, &n->state); -} +void napi_enable(struct napi_struct *n); /** * napi_synchronize - wait until NAPI is not running @@ -2140,6 +2137,7 @@ struct net_device { struct lock_class_key *qdisc_tx_busylock; struct lock_class_key *qdisc_running_key; bool proto_down; + bool threaded; unsigned wol_enabled:1; struct list_head net_notifier_list; diff --git a/net/core/dev.c b/net/core/dev.c index 83b59e4c0f37..edcfec1361e9 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -91,6 +91,7 @@ #include <linux/etherdevice.h> #include <linux/ethtool.h> #include <linux/skbuff.h> +#include <linux/kthread.h> #include <linux/bpf.h> #include <linux/bpf_trace.h> #include <net/net_namespace.h> @@ -1493,6 +1494,36 @@ void netdev_notify_peers(struct net_device *dev) } EXPORT_SYMBOL(netdev_notify_peers); +static int napi_threaded_poll(void *data); + +static int napi_kthread_create(struct napi_struct *n) +{ + int err = 0; + + /* Create and wake up the kthread once to put it in + * TASK_INTERRUPTIBLE mode to avoid the blocked task + * warning and work with loadavg. + */ + n->thread = kthread_run(napi_threaded_poll, n, "napi/%s-%d", + n->dev->name, n->napi_id); + if (IS_ERR(n->thread)) { + err = PTR_ERR(n->thread); + pr_err("kthread_run failed with err %d\n", err); + n->thread = NULL; + } + + return err; +} + +static void napi_kthread_stop(struct napi_struct *n) +{ + if (!n->thread) + return; + kthread_stop(n->thread); + clear_bit(NAPI_STATE_THREADED, &n->state); + n->thread = NULL; +} + static int __dev_open(struct net_device *dev, struct netlink_ext_ack *extack) { const struct net_device_ops *ops = dev->netdev_ops; @@ -4252,6 +4283,11 @@ int gro_normal_batch __read_mostly = 8; static inline void ____napi_schedule(struct softnet_data *sd, struct napi_struct *napi) { + if (test_bit(NAPI_STATE_THREADED, &napi->state)) { + wake_up_process(napi->thread); + return; + } + list_add_tail(&napi->poll_list, &sd->poll_list); __raise_softirq_irqoff(NET_RX_SOFTIRQ); } @@ -6697,6 +6733,27 @@ static void init_gro_hash(struct napi_struct *napi) napi->gro_bitmask = 0; } +static int napi_set_threaded(struct napi_struct *n, bool threaded) +{ + int err = 0; + + if (threaded == !!test_bit(NAPI_STATE_THREADED, &n->state)) + return 0; + if (threaded) { + if (!n->thread) { + err = napi_kthread_create(n); + if (err) + goto out; + } + set_bit(NAPI_STATE_THREADED, &n->state); + } else { + clear_bit(NAPI_STATE_THREADED, &n->state); + } + +out: + return err; +} + void netif_napi_add(struct net_device *dev, struct napi_struct *napi, int (*poll)(struct napi_struct *, int), int weight) { @@ -6738,12 +6795,23 @@ void napi_disable(struct napi_struct *n) msleep(1); hrtimer_cancel(&n->timer); + napi_kthread_stop(n); clear_bit(NAPI_STATE_PREFER_BUSY_POLL, &n->state); clear_bit(NAPI_STATE_DISABLE, &n->state); } EXPORT_SYMBOL(napi_disable); +void napi_enable(struct napi_struct *n) +{ + BUG_ON(!test_bit(NAPI_STATE_SCHED, &n->state)); + smp_mb__before_atomic(); + clear_bit(NAPI_STATE_SCHED, &n->state); + clear_bit(NAPI_STATE_NPSVC, &n->state); + WARN_ON(napi_set_threaded(n, n->dev->threaded)); +} +EXPORT_SYMBOL(napi_enable); + static void flush_gro_hash(struct napi_struct *napi) { int i; @@ -6866,6 +6934,51 @@ static int napi_poll(struct napi_struct *n, struct list_head *repoll) return work; } +static int napi_thread_wait(struct napi_struct *napi) +{ + set_current_state(TASK_INTERRUPTIBLE); + + while (!kthread_should_stop() && !napi_disable_pending(napi)) { + if (test_bit(NAPI_STATE_SCHED, &napi->state)) { + WARN_ON(!list_empty(&napi->poll_list)); + __set_current_state(TASK_RUNNING); + return 0; + } + + schedule(); + set_current_state(TASK_INTERRUPTIBLE); + } + __set_current_state(TASK_RUNNING); + return -1; +} + +static int napi_threaded_poll(void *data) +{ + struct napi_struct *napi = data; + void *have; + + while (!napi_thread_wait(napi)) { + for (;;) { + bool repoll = false; + + local_bh_disable(); + + have = netpoll_poll_lock(napi); + __napi_poll(napi, &repoll); + netpoll_poll_unlock(have); + + __kfree_skb_flush(); + local_bh_enable(); + + if (!repoll) + break; + + cond_resched(); + } + } + return 0; +} + static __latent_entropy void net_rx_action(struct softirq_action *h) { struct softnet_data *sd = this_cpu_ptr(&softnet_data);