Re: [PATCH net] net: page_pool: use in_softirq() instead

[Jesper Dangaard Brouer]

On 03/02/2023 14.05, DENG Qingfang wrote:
> Hi Jesper,
>
> On Fri, Feb 3, 2023 at 7:15 PM Jesper Dangaard Brouer
> <jbrouer@xxxxxxxxxx> wrote:
> > How can I enable threaded NAPI on my system?
>
> dev_set_threaded(napi_dev, true);
>
> You can also enable it at runtime by writing 1 to
> /sys/class/net/<devname>/threaded, but it only works if the driver
> does _not_ use a dummy netdev for NAPI poll.

Thanks for providing this setup info.

I quickly tested driver i40e with a XDP_DROP workload, and switch 
between the threaded and normal NAPI, and no performance difference.
(p.s. driver i40e doesn't use page_pool)

> > I think other cases (above) are likely safe, but I worry a little about
> > this case, as the page_pool_recycle_in_cache() rely on RX-NAPI protection.
> > Meaning it is only the CPU that handles RX-NAPI for this RX-queue that
> > is allowed to access this lockless array.
>
> The major changes to the threaded NAPI is that instead of scheduling a
> NET_RX softirq, it wakes up a kthread which then does the polling,
> allowing it to be scheduled to another CPU. The driver's poll function
> is called with BH disabled so it's still considered BH context.

As long as drivers NAPI poll function doesn't migrate between CPUs while
it is running this should be fine. (This guarantee is needed as XDP + TC
have per_cpu bpf_redirect_info).

Looking at the code napi_threaded_poll() in net/core/dev.c I can see
this is guarantee is provided by the local_bh_disable() +
local_bh_enable around the call to __napi_poll().

> > We do have the 'allow_direct' boolean, and if every driver/user uses
> > this correctly, then this should be safe.  Changing this makes it
> > possible for drivers to use page_pool API incorrectly and this leads to
> > hard-to-debug errors.
>
> "incorrectly", like, calling it outside RX-NAPI?

Yes.

--Jesper