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

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

[Jakub Kicinski]

page pool and XDP should not be accessed from IRQ context
which may happen if drivers try to clean up XDP TX with
NAPI budget of 0.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: corbet@lwn.net
CC: linux-doc@vger.kernel.org
---
 Documentation/networking/napi.rst | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/Documentation/networking/napi.rst b/Documentation/networking/napi.rst
index a7a047742e93..7bf7b95c4f7a 100644
--- a/Documentation/networking/napi.rst
+++ b/Documentation/networking/napi.rst
@@ -65,15 +65,16 @@ argument - drivers can process completions for any number of Tx
 packets but should only process up to ``budget`` number of
 Rx packets. Rx processing is usually much more expensive.
 
-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.
 
 .. 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)
-- 
2.41.0

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

[Wei Fang]

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: 2023年7月21日 0:13
> To: davem@davemloft.net
> Cc: netdev@vger.kernel.org; edumazet@google.com; pabeni@redhat.com;
> Jakub Kicinski <kuba@kernel.org>; corbet@lwn.net; linux-doc@vger.kernel.org
> Subject: [PATCH net] docs: net: clarify the NAPI rules around XDP Tx
> 
> page pool and XDP should not be accessed from IRQ context which may
> happen if drivers try to clean up XDP TX with NAPI budget of 0.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> CC: corbet@lwn.net
> CC: linux-doc@vger.kernel.org
> ---
>  Documentation/networking/napi.rst | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/networking/napi.rst
> b/Documentation/networking/napi.rst
> index a7a047742e93..7bf7b95c4f7a 100644
> --- a/Documentation/networking/napi.rst
> +++ b/Documentation/networking/napi.rst
> @@ -65,15 +65,16 @@ argument - drivers can process completions for any
> number of Tx  packets but should only process up to ``budget`` number of
> Rx packets. Rx processing is usually much more expensive.
> 
> -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.
> 
Can I ask a stupid question why tx processing cannot call any XDP (or page pool)
APIs if the "budget" is 0?

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

[Jakub Kicinski]

On Fri, 21 Jul 2023 02:35:41 +0000 Wei Fang 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.
> >   
> Can I ask a stupid question why tx processing cannot call any XDP (or page pool)
> APIs if the "budget" is 0?

Because in that case we may be in an interrupt context, and page pool
assumes it's either in process or softirq context. See commit
afbed3f74830 ("net/mlx5e: do as little as possible in napi poll when
budget is 0") for an example stack trace.

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

[Wei Fang]

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: 2023年7月21日 11:07
> To: Wei Fang <wei.fang@nxp.com>
> Cc: netdev@vger.kernel.org; edumazet@google.com; pabeni@redhat.com;
> corbet@lwn.net; linux-doc@vger.kernel.org; davem@davemloft.net
> Subject: Re: [PATCH net] docs: net: clarify the NAPI rules around XDP Tx
> 
> On Fri, 21 Jul 2023 02:35:41 +0000 Wei Fang 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.
> > >
> > Can I ask a stupid question why tx processing cannot call any XDP (or
> > page pool) APIs if the "budget" is 0?
> 
> Because in that case we may be in an interrupt context, and page pool
> assumes it's either in process or softirq context. See commit
> afbed3f74830 ("net/mlx5e: do as little as possible in napi poll when budget is
> 0") for an example stack trace.
I got it, thank you!

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

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Thu, 20 Jul 2023 09:13:23 -0700 you wrote:
> page pool and XDP should not be accessed from IRQ context
> which may happen if drivers try to clean up XDP TX with
> NAPI budget of 0.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> CC: corbet@lwn.net
> CC: linux-doc@vger.kernel.org
> 
> [...]

Here is the summary with links:
  - [net] docs: net: clarify the NAPI rules around XDP Tx
    https://git.kernel.org/netdev/net/c/32ad45b76990

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

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

[Alexander H Duyck]

On Thu, 2023-07-20 at 09:13 -0700, Jakub Kicinski wrote:
> page pool and XDP should not be accessed from IRQ context
> which may happen if drivers try to clean up XDP TX with
> NAPI budget of 0.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> CC: corbet@lwn.net
> CC: linux-doc@vger.kernel.org
> ---
>  Documentation/networking/napi.rst | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/networking/napi.rst b/Documentation/networking/napi.rst
> index a7a047742e93..7bf7b95c4f7a 100644
> --- a/Documentation/networking/napi.rst
> +++ b/Documentation/networking/napi.rst
> @@ -65,15 +65,16 @@ argument - drivers can process completions for any number of Tx
>  packets but should only process up to ``budget`` number of
>  Rx packets. Rx processing is usually much more expensive.
>  
> -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.

>  .. 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.

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

[Jakub Kicinski]

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.

> >  .. 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.

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

[Alexander Duyck]

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.

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

[Jakub Kicinski]

On Tue, 25 Jul 2023 13:10:18 -0700 Alexander Duyck wrote:
> On Tue, Jul 25, 2023 at 11:55 AM Jakub Kicinski <kuba@kernel.org> wrote:
> > > 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

We do? Can you point me to where netpoll disables BH?

> 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.
>
> > > 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.

I'm not disagreeing with you. I just don't have time to take a deeper
look and add the IRQ checks myself and I'm 90% sure the current code
can't work with netpoll. So I thought I'd at least document that :(

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

[Alexander Duyck]

On Tue, Jul 25, 2023 at 1:41 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Tue, 25 Jul 2023 13:10:18 -0700 Alexander Duyck wrote:
> > On Tue, Jul 25, 2023 at 11:55 AM Jakub Kicinski <kuba@kernel.org> wrote:
> > > > 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
>
> We do? Can you point me to where netpoll disables BH?

I misread the code. Basically it looks like netconsole is explicitly
disabling interrupts via spin_lock_irqsave in write_msg is what is
going on.

> > 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.
> >
> > > > 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.
>
> I'm not disagreeing with you. I just don't have time to take a deeper
> look and add the IRQ checks myself and I'm 90% sure the current code
> can't work with netpoll. So I thought I'd at least document that :(

So looking at it more I realized the way we are getting around the
issue is that the skbuffs are ALWAYS freed in softirq context.
Basically we hand them off to dev_consume_skb_any, which will hand
them off to dev_kfree_skb_irq_reason, and it is queueing them up to be
processed in the net_tx_action handler.

As far as the page pool pages themselves I wonder if we couldn't just
look at modifying __page_pool_put_page() so that it had something
similar to dev_consume_skb_any_reason() so if we are in a hardirq or
IRQs are disabled we just force the page to be freed.

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

[Jakub Kicinski]

On Tue, 25 Jul 2023 17:02:42 -0700 Alexander Duyck wrote:
> So looking at it more I realized the way we are getting around the
> issue is that the skbuffs are ALWAYS freed in softirq context.
> Basically we hand them off to dev_consume_skb_any, which will hand
> them off to dev_kfree_skb_irq_reason, and it is queueing them up to be
> processed in the net_tx_action handler.

SG.

> As far as the page pool pages themselves I wonder if we couldn't just
> look at modifying __page_pool_put_page() so that it had something
> similar to dev_consume_skb_any_reason() so if we are in a hardirq or
> IRQs are disabled we just force the page to be freed.

Yup (same for the bulk API). I think that Olek was trying to implement
this somehow nicely, not sure how far he got.