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 <pab...@redhat.com>
> Signed-off-by: Paolo Abeni <pab...@redhat.com>
> Co-developed-by: Hannes Frederic Sowa <han...@stressinduktion.org>
> Signed-off-by: Hannes Frederic Sowa <han...@stressinduktion.org>
> Co-developed-by: Jakub Kicinski <k...@kernel.org>
> Signed-off-by: Jakub Kicinski <k...@kernel.org>
> Signed-off-by: Wei Wang <wei...@google.com>
> Reviewed-by: Eric Dumazet <eduma...@google.com>

> @@ -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);
>  }

>  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);

Reply via email to