Re: [PATCH net] docs: net: clarify the NAPI rules around XDP Tx

[Alexander Duyck <alexander.duyck@gmail.com>]

On Tue, Jul 25, 2023 at 11:55 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Tue, 25 Jul 2023 10:30:24 -0700 Alexander H Duyck wrote:
> > > -In other words, it is recommended to ignore the budget argument when
> > > -performing TX buffer reclamation to ensure that the reclamation is not
> > > -arbitrarily bounded; however, it is required to honor the budget argument
> > > -for RX processing.
> > > +In other words for Rx processing the ``budget`` argument limits how many
> > > +packets driver can process in a single poll. Rx specific APIs like page
> > > +pool or XDP cannot be used at all when ``budget`` is 0.
> > > +skb Tx processing should happen regardless of the ``budget``, but if
> > > +the argument is 0 driver cannot call any XDP (or page pool) APIs.
> >
> > This isn't accurate, and I would say it is somewhat dangerous advice.
> > The Tx still needs to be processed regardless of if it is processing
> > page_pool pages or XDP pages. I agree the Rx should not be processed,
> > but the Tx must be processed using mechanisms that do NOT make use of
> > NAPI optimizations when budget is 0.
> >
> > So specifically, xdp_return_frame is safe in non-NAPI Tx cleanup. The
> > xdp_return_frame_rx_napi is not.
> >
> > Likewise there is napi_consume_skb which will use either a NAPI or non-
> > NAPI version of things depending on if budget is 0 or not.
> >
> > For the page_pool calls there is the "allow_direct" argument that is
> > meant to decide between recycling in directly into the page_pool cache
> > or not. It should only be used in the Rx handler itself when budget is
> > non-zero.
> >
> > I realise this was written up in response to a patch on the Mellanox
> > driver. Based on the patch in question it looks like they were calling
> > page_pool_recycle_direct outside of NAPI context. There is an explicit
> > warning above that function about NOT calling it outside of NAPI
> > context.
>
> Unless I'm missing something budget=0 can be called from hard IRQ
> context. And page pool takes _bh() locks. So unless we "teach it"
> not to recycle _anything_ in hard IRQ context, it is not safe to call.

That is the thing. We have to be able to free the pages regardless of
context. Otherwise we make a huge mess of things. Also there isn't
much way to differentiate between page_pool and non-page_pool pages
because an skb can be composed of page pool pages just as easy as an
XDP frame can be. All you would just have to enable routing or
bridging for Rx frames to end up with page pool pages in the Tx path.

As far as netpoll itself we are safe because it has BH disabled and so
as a result page_pool doesn't use the _bh locks. There is code in
place to account for that in the producer locking code, and if it were
an issue we would have likely blown up long before now. The fact is
that page_pool has proliferated into skbs, so you are still freeing
page_pool pages indirectly anyway.

That said, there are calls that are not supposed to be used outside of
NAPI context, such as page_pool_recycle_direct(). Those have mostly
been called out in the page_pool.h header itself, so if someone
decides to shoot themselves in the foot with one of those, that is on
them. What we need to watch out for are people abusing the "direct"
calls and such or just passing "true" for allow_direct in the
page_pool calls without taking proper steps to guarantee the context.

> > >  .. warning::
> > >
> > > -   The ``budget`` argument may be 0 if core tries to only process Tx completions
> > > -   and no Rx packets.
> > > +   The ``budget`` argument may be 0 if core tries to only process
> > > +   skb Tx completions and no Rx or XDP packets.
> > >
> > >  The poll method returns the amount of work done. If the driver still
> > >  has outstanding work to do (e.g. ``budget`` was exhausted)
> >
> > We cannot make this distinction if both XDP and skb are processed in
> > the same Tx queue. Otherwise you will cause the Tx to stall and break
> > netpoll. If the ring is XDP only then yes, it can be skipped like what
> > they did in the Mellanox driver, but if it is mixed then the XDP side
> > of things needs to use the "safe" versions of the calls.
>
> IDK, a rare delay in sending of a netpoll message is not a major
> concern.

The whole point of netpoll is to get data out after something like a
crash. Otherwise we could have just been using regular NAPI. If the Tx
ring is hung it might not be a delay but rather a complete stall that
prevents data on the Tx queue from being transmitted on since the
system will likely not be recovering. Worse yet is if it is a scenario
where the Tx queue can recover it might trigger the Tx watchdog since
I could see scenarios where the ring fills, but interrupts were
dropped because of the netpoll.