mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [PATCH net] netpoll: bound the deferred transmit queue
@ 2026-09-14  4:12 Zack Gomez
  2026-09-14  8:47 ` Breno Leitao
  0 siblings, 1 reply; 5+ messages in thread
From: Zack Gomez @ 2026-09-14  4:12 UTC (permalink / raw)
  To: davem, edumazet, kuba, pabeni
  Cc: horms, leitao, netdev, linux-kernel, Zack Gomez

__netpoll_send_skb() parks an skb on npinfo->txq whenever the device
cannot take it at once, and once the queue is non-empty every later skb
goes straight there to keep ordering. queue_process() drains it from a
workqueue and, unlike the direct path, never polls the device for
completions: when the ring is stopped it backs off HZ/10. Nothing limits
the queue length.

A producer that outruns that drain therefore grows the queue until the
host is out of memory. Observed with netconsole forwarding a GPU driver
that logged one line at ~1e5/s after a firmware hang. The NIC was
moving ~17k packets/s: completions for each burst surfaced tens of ms
later, outside the one-tick window, so queue_process() slept HZ/10 per
ring while ~1e5 lines/s kept arriving. The queue grew at ~170 MB/s,
unreclaimable slab reached 51 GiB in five minutes and the OOM killer
ran from kswapd with 341 MiB of anonymous memory on the whole box. What
the queue held was the flood itself; the OOM report never left the
host.

Reproduced on the same host (netconsole over a 10G ConnectX-4 Lx) under
the same slow-completion condition: 200k lines to /dev/kmsg in 0.12 s
left 188k skbs and 173 MiB of unreclaimable slab parked, draining at
~8-10k packets/s. With prompt completions the same burst drains at line
rate; any stall on the link reproduces the growth.

Until the 2006 netpoll rework [1] the deferred path drained through
dev_queue_xmit(), with the stack's own backpressure, and was capped at
16 skbs (MAX_QUEUE_DEPTH). That series moved it to a direct
hard_start_xmit() with the HZ/10 back-off and made the queue
per-device, dropping the cap on the way. Neither change was discussed
on the list.

Cap it at 1024 skbs and drop new skbs beyond that. netconsole already
counts NET_XMIT_DROP in its per-target xmit_drop_count, so the loss is
visible in configfs. Nothing is logged on the drop path because that
would recurse into the console being drained.

[1] https://lore.kernel.org/netdev/20061026225645.482978803@osdl.org/

Fixes: b6cd27ed3388 ("netpoll per device txq")
Signed-off-by: Zack Gomez <zack.gomez@gmail.com>
---
Tested on 7.2.5 with this patch applied: the 200k-line burst that parked
188k skbs / 173 MiB on the unpatched kernel parks at most ~2k skbs and
+4 MiB, with 182k drops counted in the target's transmit_errors; with
prompt completions the same burst drains at line rate with a peak
backlog of ~180 skbs and no drops. Built with LLVM=1 W=1, checkpatch
--strict clean.

Two choices I would take direction on: tail drop keeps the oldest
messages and loses the newest, which for a console are usually the
ones wanted, so dropping from the head is a few more lines; and 1024
is arbitrary, about 1 MiB of skbs.

queue_process()'s HZ/10 back-off is why slow completions turn into a
~10k packet/s trickle; a shorter retry is a separate change I have
not measured yet.

 net/core/netpoll.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/net/core/netpoll.c b/net/core/netpoll.c
index fe1e0cda5d6..8fd640955e4 100644
--- a/net/core/netpoll.c
+++ b/net/core/netpoll.c
@@ -38,6 +38,14 @@
 
 #define USEC_PER_POLL	50
 
+/*
+ * Cap on skbs parked in npinfo->txq while the device is busy. The queue
+ * exists to ride out a transient stall; a producer that outruns the
+ * device for longer than that must lose packets, not grow it without
+ * bound.
+ */
+#define NETPOLL_TXQ_MAX	1024
+
 /*
  * carrier_timeout is netconsole-specific and only kept here to preserve the
  * netpoll.carrier_timeout module-parameter ABI. Its value is exposed to
@@ -314,6 +322,10 @@ static netdev_tx_t __netpoll_send_skb(struct netpoll *np, struct sk_buff *skb)
 	}
 
 	if (!dev_xmit_complete(status)) {
+		if (skb_queue_len(&npinfo->txq) >= NETPOLL_TXQ_MAX) {
+			dev_kfree_skb_irq(skb);
+			goto out;
+		}
 		skb_queue_tail(&npinfo->txq, skb);
 		schedule_delayed_work(&npinfo->tx_work,0);
 	}

base-commit: e6b6078ea1731b05b3b552497b3bce4bf8b014ae
-- 
2.55.0


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH net] netpoll: bound the deferred transmit queue
  2026-09-14  4:12 [PATCH net] netpoll: bound the deferred transmit queue Zack Gomez
@ 2026-09-14  8:47 ` Breno Leitao
  2026-09-15 16:40   ` Zack Gomez
  2026-09-15 16:45   ` Zack Gomez
  0 siblings, 2 replies; 5+ messages in thread
From: Breno Leitao @ 2026-09-14  8:47 UTC (permalink / raw)
  To: Zack Gomez; +Cc: davem, edumazet, kuba, pabeni, horms, netdev, linux-kernel

On Mon, Sep 14, 2026 at 12:12:21AM -0400, Zack Gomez wrote:
> A producer that outruns that drain therefore grows the queue until the
> host is out of memory.
...
> Observed with netconsole forwarding a GPU driver
> that logged one line at ~1e5/s after a firmware hang.

I know what you mean, I'm seeing the same issue on my side.

> Cap it at 1024 skbs and drop new skbs beyond that.

I don't think netpoll should be the one doing rate limiting here.

I think the rate limiting should live on the netconsole side, not
netpoll.

I have a patchset that does exactly that, which I wrote after
hitting a similar issue in production. Would you mind having a test?

https://lore.kernel.org/all/20260910-netcons_ratelimit-v2-0-ebf0dd91e26e@debian.org/

> @@ -314,6 +322,10 @@ static netdev_tx_t __netpoll_send_skb(struct netpoll *np, struct sk_buff *skb)
>  	}
>  
>  	if (!dev_xmit_complete(status)) {
> +		if (skb_queue_len(&npinfo->txq) >= NETPOLL_TXQ_MAX) {
> +			dev_kfree_skb_irq(skb);
> +			goto out;

If we do end up also rate limiting at the netpoll side, I'd like to
see a WARN_ON_ONCE() here, since it shouldn't be the main ratelimit
path, but rather something to flag that something is off.

--breno

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH net] netpoll: bound the deferred transmit queue
  2026-09-14  8:47 ` Breno Leitao
@ 2026-09-15 16:40   ` Zack Gomez
  2026-09-15 16:45   ` Zack Gomez
  1 sibling, 0 replies; 5+ messages in thread
From: Zack Gomez @ 2026-09-15 16:40 UTC (permalink / raw)
  To: Breno Leitao; +Cc: davem, edumazet, kuba, pabeni, horms, netdev, linux-kernel

On Mon, Sep 14, 2026 at 01:47:39AM -0700, Breno Leitao wrote:
> I have a patchset that does exactly that, which I wrote after
> hitting a similar issue in production. Would you mind having a test?

Tested your v2 on 7.2.5 with stock netpoll (no cap), same host, NIC
and reproducer as the commit message. Two writers offer 200k lines/s
to /dev/kmsg. The slow-completion condition is the one from the
original event: the sink only ever receives, so its MAC ages out of
the switch's table, the switch floods each frame to every port and
pauses this one. "paused" below is that state; "learned" is after a
ping from the sink put the MAC back in the table. parked is the peak
skb count in npinfo->txq, slab the peak growth in unreclaimable slab.

bucket link sent parked slab backlog
20k/s paused 8.3k/s 152k +116 MiB growing
10k/s paused 8.6k/s 58k +27 MiB growing
5k/s paused* 5.2k/s 1.1k 0 flat
20k/s learned 20.1k/s 1.1k 0 flat
unlimited learned 200k/s 3.0k 0 flat

* at 5k/s the flood is light enough that the switch never pauses
the port.

Seems to work as advertised: with the link healthy the target sends
exactly the configured rate and nothing is parked.

> I think the rate limiting should live on the netconsole side, not
> netpoll.

I'd still argue the netpoll change is required: there should not be
a path to OOMing the box. The rate limit is opt-in, and even once
set it OOMs the same way if the limit is above what the link is
draining at that moment, which above is 8k/s under the pause and
200k/s without it. The two rows at 20k/s are the same configuration
on a good day and a bad day.

I read the two as solving different problems. Yours is consumer
side: it limits how much damage one machine can do to a listener.
The netpoll cap is producer side: it keeps the sender from damaging
itself. Both seem worth having.

> If we do end up also rate limiting at the netpoll side, I'd like to
> see a WARN_ON_ONCE() here, since it shouldn't be the main ratelimit
> path, but rather something to flag that something is off.

No strong opinion on that; I'll defer to the maintainers. The drop
is counted either way: netconsole sees NET_XMIT_DROP in
xmit_drop_count and exposes it as transmit_errors.

Zack

On Mon, Sep 14, 2026 at 4:47 AM Breno Leitao <leitao@debian.org> wrote:
>
> On Mon, Sep 14, 2026 at 12:12:21AM -0400, Zack Gomez wrote:
> > A producer that outruns that drain therefore grows the queue until the
> > host is out of memory.
> ...
> > Observed with netconsole forwarding a GPU driver
> > that logged one line at ~1e5/s after a firmware hang.
>
> I know what you mean, I'm seeing the same issue on my side.
>
> > Cap it at 1024 skbs and drop new skbs beyond that.
>
> I don't think netpoll should be the one doing rate limiting here.
>
> I think the rate limiting should live on the netconsole side, not
> netpoll.
>
> I have a patchset that does exactly that, which I wrote after
> hitting a similar issue in production. Would you mind having a test?
>
> https://lore.kernel.org/all/20260910-netcons_ratelimit-v2-0-ebf0dd91e26e@debian.org/
>
> > @@ -314,6 +322,10 @@ static netdev_tx_t __netpoll_send_skb(struct netpoll *np, struct sk_buff *skb)
> >       }
> >
> >       if (!dev_xmit_complete(status)) {
> > +             if (skb_queue_len(&npinfo->txq) >= NETPOLL_TXQ_MAX) {
> > +                     dev_kfree_skb_irq(skb);
> > +                     goto out;
>
> If we do end up also rate limiting at the netpoll side, I'd like to
> see a WARN_ON_ONCE() here, since it shouldn't be the main ratelimit
> path, but rather something to flag that something is off.
>
> --breno

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH net] netpoll: bound the deferred transmit queue
  2026-09-14  8:47 ` Breno Leitao
  2026-09-15 16:40   ` Zack Gomez
@ 2026-09-15 16:45   ` Zack Gomez
  2026-09-16 10:01     ` Breno Leitao
  1 sibling, 1 reply; 5+ messages in thread
From: Zack Gomez @ 2026-09-15 16:45 UTC (permalink / raw)
  To: Breno Leitao; +Cc: davem, edumazet, kuba, pabeni, horms, netdev, linux-kernel

On Mon, Sep 14, 2026 at 01:47:39AM -0700, Breno Leitao wrote:
> I have a patchset that does exactly that, which I wrote after
> hitting a similar issue in production. Would you mind having a test?

Tested your v2 on 7.2.5 with stock netpoll (no cap), same host, NIC
and reproducer as the commit message. Two writers offer 200k lines/s
to /dev/kmsg. The slow-completion condition is the one from the
original event: the sink only ever receives, so its MAC ages out of
the switch's table, the switch floods each frame to every port and
pauses this one. "paused" below is that state; "learned" is after a
ping from the sink put the MAC back in the table. parked is the peak
skb count in npinfo->txq, slab the peak growth in unreclaimable slab.

bucket link sent parked slab backlog
20k/s paused 8.3k/s 152k +116 MiB growing
10k/s paused 8.6k/s 58k +27 MiB growing
5k/s paused* 5.2k/s 1.1k 0 flat
20k/s learned 20.1k/s 1.1k 0 flat
unlimited learned 200k/s 3.0k 0 flat

* at 5k/s the flood is light enough that the switch never pauses
the port.

Seems to work as advertised: with the link healthy the target sends
exactly the configured rate and nothing is parked.

> I think the rate limiting should live on the netconsole side, not
> netpoll.

I'd still argue the netpoll change is required: there should not be
a path to OOMing the box. The rate limit is opt-in, and even once
set it OOMs the same way if the limit is above what the link is
draining at that moment, which above is 8k/s under the pause and
200k/s without it. The two rows at 20k/s are the same configuration
on a good day and a bad day.

I read the two as solving different problems. Yours is consumer
side: it limits how much damage one machine can do to a listener.
The netpoll cap is producer side: it keeps the sender from damaging
itself. Both seem worth having.

> If we do end up also rate limiting at the netpoll side, I'd like to
> see a WARN_ON_ONCE() here, since it shouldn't be the main ratelimit
> path, but rather something to flag that something is off.

No strong opinion on that; I'll defer to the maintainers. The drop
is counted either way: netconsole sees NET_XMIT_DROP in
xmit_drop_count and exposes it as transmit_errors.

Zack



On Mon, Sep 14, 2026 at 4:47 AM Breno Leitao <leitao@debian.org> wrote:
>
> On Mon, Sep 14, 2026 at 12:12:21AM -0400, Zack Gomez wrote:
> > A producer that outruns that drain therefore grows the queue until the
> > host is out of memory.
> ...
> > Observed with netconsole forwarding a GPU driver
> > that logged one line at ~1e5/s after a firmware hang.
>
> I know what you mean, I'm seeing the same issue on my side.
>
> > Cap it at 1024 skbs and drop new skbs beyond that.
>
> I don't think netpoll should be the one doing rate limiting here.
>
> I think the rate limiting should live on the netconsole side, not
> netpoll.
>
> I have a patchset that does exactly that, which I wrote after
> hitting a similar issue in production. Would you mind having a test?
>
> https://lore.kernel.org/all/20260910-netcons_ratelimit-v2-0-ebf0dd91e26e@debian.org/
>
> > @@ -314,6 +322,10 @@ static netdev_tx_t __netpoll_send_skb(struct netpoll *np, struct sk_buff *skb)
> >       }
> >
> >       if (!dev_xmit_complete(status)) {
> > +             if (skb_queue_len(&npinfo->txq) >= NETPOLL_TXQ_MAX) {
> > +                     dev_kfree_skb_irq(skb);
> > +                     goto out;
>
> If we do end up also rate limiting at the netpoll side, I'd like to
> see a WARN_ON_ONCE() here, since it shouldn't be the main ratelimit
> path, but rather something to flag that something is off.
>
> --breno

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH net] netpoll: bound the deferred transmit queue
  2026-09-15 16:45   ` Zack Gomez
@ 2026-09-16 10:01     ` Breno Leitao
  0 siblings, 0 replies; 5+ messages in thread
From: Breno Leitao @ 2026-09-16 10:01 UTC (permalink / raw)
  To: Zack Gomez; +Cc: davem, edumazet, kuba, pabeni, horms, netdev, linux-kernel


On Tue, Sep 15, 2026 at 12:45:00PM -0400, Zack Gomez wrote:
> On Mon, Sep 14, 2026 at 01:47:39AM -0700, Breno Leitao wrote:
>
> Seems to work as advertised: with the link healthy the target sends
> exactly the configured rate and nothing is parked. 

Good to hear it.

> > I think the rate limiting should live on the netconsole side, not
> > netpoll.
> 
> I'd still argue the netpoll change is required: there should not be
> a path to OOMing the box. The rate limit is opt-in, and even once
> set it OOMs the same way if the limit is above what the link is
> draining at that moment, which above is 8k/s under the pause and
> 200k/s without it. The two rows at 20k/s are the same configuration
> on a good day and a bad day.
> 
> I read the two as solving different problems. Yours is consumer
> side: it limits how much damage one machine can do to a listener.
> The netpoll cap is producer side: it keeps the sender from damaging
> itself. Both seem worth having.

Yes, I agree.

> > If we do end up also rate limiting at the netpoll side, I'd like to
> > see a WARN_ON_ONCE() here, since it shouldn't be the main ratelimit
> > path, but rather something to flag that something is off.
> 
> No strong opinion on that; I'll defer to the maintainers. The drop
> is counted either way: netconsole sees NET_XMIT_DROP in
> xmit_drop_count and exposes it as transmit_errors.

I am not worried about netconsole here, it copes with the drop fine.

My concern is some other netpoll users that do not check for return
value. Is this change going to affect them?

For instance, team doesn't even check the return value.

	static inline void team_netpoll_send_skb(struct team_port *port,
						struct sk_buff *skb)
	{
		netpoll_send_skb(port->np, skb);
	}

One way or another, I am in favor of adding it, but, with more
extra information about this drop.

If a WARN is too heavy and it seems it might be, would a drop with
a reason be better than just dev_kfree_skb_irq() ?

      dev_kfree_skb_irq_reason(skb, SKB_DROP_REASON_FULL_RING); 


Thanks,
--breno

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2026-09-16 10:01 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-09-14  4:12 [PATCH net] netpoll: bound the deferred transmit queue Zack Gomez
2026-09-14  8:47 ` Breno Leitao
2026-09-15 16:40   ` Zack Gomez
2026-09-15 16:45   ` Zack Gomez
2026-09-16 10:01     ` Breno Leitao

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®