Re: [PATCH net-next v4 2/3] net: implement threaded-able napi poll loop support
From: Jakub Kicinski <kuba@kernel.org>
Date: 2020-12-12 22:51:21
On Tue, 8 Dec 2020 16:54:43 -0800 Wei Wang 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 <redacted> Signed-off-by: Hannes Frederic Sowa <redacted> Co-developed-by: Jakub Kicinski <kuba@kernel.org> Signed-off-by: Jakub Kicinski <kuba@kernel.org> Signed-off-by: Wei Wang <redacted> Reviewed-by: Eric Dumazet <edumazet@google.com>
quoted hunk ↗ jump to hunk
@@ -4234,6 +4265,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);
FTR your implementation depends on the fact that this is the only place that can wake the worker and not set kthread_should_stop(). Which I trust you is the case :) maybe I already mentioned this..
+ return; + } + list_add_tail(&napi->poll_list, &sd->poll_list); __raise_softirq_irqoff(NET_RX_SOFTIRQ); }
quoted hunk ↗ jump to hunk
void netif_napi_add(struct net_device *dev, struct napi_struct *napi, int (*poll)(struct napi_struct *, int), int weight) {@@ -6731,6 +6790,7 @@ void napi_disable(struct napi_struct *n) msleep(1); hrtimer_cancel(&n->timer); + napi_kthread_stop(n);
I'm surprised that we stop the thread on napi_disable() but there is no start/create in napi_enable(). NAPIs can (and do get) disabled and enabled again. But that'd make your code crash with many popular drivers if you tried to change rings with threaded napi enabled so I feel like I must be missing something..
clear_bit(NAPI_STATE_PREFER_BUSY_POLL, &n->state); clear_bit(NAPI_STATE_DISABLE, &n->state);