[PATCH net v2] net: do not let defer_count outlive an empty defer_list

[PATCH net v2] net: do not let defer_count outlive an empty defer_list

[Joseph Kain]

__skb_defer_free_flush() can leave defer_count at or above skb_defer_max
with an empty defer_list. That state is absorbing: every producer then
takes the nodefer path without queueing, so the list never refills, and
the flush keeps returning early without clearing. Deferred freeing stays
off for that (cpu, node) until reboot, and cross-CPU frees fall back to
the page allocator, which is the contention deferral exists to avoid.

Two changes from commit 844c9db7f7f5 ("net: use llist for sd->defer_list")
combine to produce it.

skb_attempt_defer_free() increments before testing the limit and does not
undo the increment when it rejects, so a rejected free leaves the counter
raised with nothing queued.

The flush clears defer_count before llist_del_all() rather than together
with it under defer_lock as it did before. A remote producer that queues
between the two has its skb drained by that same llist_del_all() but its
increment retained.

That part is not cumulative: any later flush finding a non-empty list
clears the counter again, so lost increments do not build up across
flushes. Reaching the limit takes skb_defer_max - 1 queued increments
inside a single clear-to-drain window. At a small skb_defer_max that
needs nothing special. At the default of 128 the flusher has to be
interrupted or preempted between those two lines for long enough, which
is where PREEMPT_RT matters: local_bh_disable() does not disable
preemption there, so the flusher can be scheduled out at that point.

Once the counter is stuck at or above the limit with an empty list,
nothing lowers it again, and that half needs no race at all:

  sysctl -w net.core.skb_defer_max=1    # frees increment, then reject
  sysctl -w net.core.skb_defer_max=128  # does not recover

skb_defer_max has .extra1 = SYSCTL_ZERO and no .extra2, so 1 is settable.
From v7.1 on, 0 is safe because proc_do_skb_defer_max() flips
skb_defer_disable_key and the producer returns before incrementing. On
6.18 through 7.0, which carry the llist conversion but not commit
08dc30de1a40 ("net: add skb_defer_disable_key static key"), setting 0 and
restoring the previous value strands the counter the same way -- and 0 is
the documented way to turn the feature off. Recovering needs a
skb_defer_max above the accumulated count, which an operator has no way
to determine.

Drain before clearing, so the error can only under-read, is bounded by
the producers in flight, and self-heals on the next flush. Clear the
counter on the empty-list path as well, so no count can outlive an empty
list and an already-stranded counter recovers.

llist_empty() has already loaded that cache line, so the added read is
free; the unlikely() keeps the store off the hot path, where it would
otherwise bounce the line against remote producers.

Fixes: 844c9db7f7f5 ("net: use llist for sd->defer_list")
Assisted-by: Claude:claude-opus-5
Signed-off-by: Joseph Kain <jkain@nuro.ai>
---
v2:
 - Reworded the drift description: it is not cumulative, since any later
   flush finding a non-empty list clears the counter again. Reaching the
   limit takes skb_defer_max - 1 queued increments inside a single
   clear-to-drain window, and said where PREEMPT_RT actually matters
   (Eric Dumazet).
 - Noted that 6.18..7.0 strand the counter via skb_defer_max=0, the
   documented way to disable the feature, since they carry the llist
   conversion but not 08dc30de1a40 (Eric Dumazet).
 - Dropped the paragraph describing where this was first reproduced
   (Eric Dumazet).
 - Added Assisted-by.

Built for x86_64 and arm64, with and without CONFIG_PREEMPT_RT. On
v7.3-rc6 with skb_defer_max=1 and a cross-CPU TCP load, defer_count
reached 1918118 unpatched and did not recover when the default was
restored; with this patch it peaked at 1.

v1: https://lore.kernel.org/netdev/20261007180056.1394868-1-jkain@nuro.ai/
 net/core/dev.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 18dc88990510..043a7f9166a9 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -6938,10 +6938,13 @@ static void __skb_defer_free_flush(struct skb_defer_node *sdn, int budget)
 	struct llist_node *free_list;
 	struct sk_buff *skb, *next;
 
-	if (llist_empty(&sdn->defer_list))
+	if (llist_empty(&sdn->defer_list)) {
+		if (unlikely(atomic_long_read(&sdn->defer_count)))
+			atomic_long_set(&sdn->defer_count, 0);
 		return;
-	atomic_long_set(&sdn->defer_count, 0);
+	}
 	free_list = llist_del_all(&sdn->defer_list);
+	atomic_long_set(&sdn->defer_count, 0);
 
 	llist_for_each_entry_safe(skb, next, free_list, ll_node) {
 		prefetch(next);
-- 
2.55.0

Re: [PATCH net v2] net: do not let defer_count outlive an empty defer_list

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH net v2] net: do not let defer_count outlive an empty defer_list

[Joseph Kain]

On Thu, Oct 8, 2026 at 3:16 PM <netdev-bot+sinfo@kernel.org> wrote:
>
> Hi!
>
> This is an automated message. This series looks like a fix, but its
> commit messages seem to be missing some information:
>
>  - How the issue was discovered, e.g. hit in production, hit during
>    development, syzbot report, manual code inspection, LLM or static
>    analysis tool scan.

  Manual code inspection, LLM-assisted, during a backport -- not hit in
  production.  I hit the heavy contention on sd->defer_lock in
production and had Claude Opus

  The context: an arm64 PREEMPT_RT system hit the sd->defer_lock convoy that
  844c9db7f7f5 removes, where a descheduled lock holder queued every other
  consumer of that NIC's skbs behind it. While backporting that conversion
  to 6.1 I read through the new code and found the absorbing state this
  patch fixes.


>
> Please do not repost the series just to address the above. Instead,
> reply to this email with the missing information, so that reviewers
> can take it into account. If the series needs another revision for
> other reasons, please include the information in the commit messages
> then.
>
> The evaluation is done by an LLM so it may be wrong, if you think
> that is the case please reply and explain.

Re: [PATCH net v2] net: do not let defer_count outlive an empty defer_list

[Joseph Kain]

On Thu, Oct 8, 2026 at 3:23 PM Joseph Kain <jkain@nuro.ai> wrote:
>
> On Thu, Oct 8, 2026 at 3:16 PM <netdev-bot+sinfo@kernel.org> wrote:
> >
> > Hi!
> >
> > This is an automated message. This series looks like a fix, but its
> > commit messages seem to be missing some information:
> >
> >  - How the issue was discovered, e.g. hit in production, hit during
> >    development, syzbot report, manual code inspection, LLM or static
> >    analysis tool scan.
>
>   Manual code inspection, LLM-assisted, during a backport -- not hit in
>   production.  I hit the heavy contention on sd->defer_lock in
> production and had Claude Opus


assess backporting.  In the process it found this issue.

>
>
> >
> > Please do not repost the series just to address the above. Instead,
> > reply to this email with the missing information, so that reviewers
> > can take it into account. If the series needs another revision for
> > other reasons, please include the information in the commit messages
> > then.
> >
> > The evaluation is done by an LLM so it may be wrong, if you think
> > that is the case please reply and explain.



-- 
Thanks,
Joseph Kain

Re: [PATCH net v2] net: do not let defer_count outlive an empty defer_list

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 · Low: 0 (1)

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The patch only partly fixes a stranded defer_count.

Pre-existing issues:
- [Low] The skb_defer_max documentation and the sysctl range do not match
  how the counter works.

--

net: do not let defer_count outlive an empty defer_list

This patch changes __skb_defer_free_flush() so that it drains the
defer_list before it clears defer_count. It also clears defer_count on the
empty-list path. The goal is to stop a stale counter from turning off
deferred skb freeing for a (cpu, node) pair until reboot.

> diff --git a/net/core/dev.c b/net/core/dev.c
> index e76762e29360e..e847e243a692e 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -6938,10 +6938,13 @@ static void __skb_defer_free_flush(struct skb_defer_node *sdn, int budget)
>  	struct llist_node *free_list;
>  	struct sk_buff *skb, *next;
>  
> -	if (llist_empty(&sdn->defer_list))
> +	if (llist_empty(&sdn->defer_list)) {
> +		if (unlikely(atomic_long_read(&sdn->defer_count)))
> +			atomic_long_set(&sdn->defer_count, 0);
>  		return;

[Severity: High]
Does this recovery path ever run on a CPU whose counter is stranded?

The counter is only cleared when the owner CPU calls
__skb_defer_free_flush() itself. skb_defer_free_flush() only walks the
this_cpu_ptr() sdns. It is called from net_rx_action(),
napi_threaded_poll_loop() and busy-poll.

The producer side in skb_attempt_defer_free() is unchanged:

net/core/skbuff.c:skb_attempt_defer_free() {
    ...
	defer_count = atomic_long_inc_return(&sdn->defer_count);

	if (defer_count >= defer_max)
		goto nodefer;
    ...
	/* Send an IPI every time queue reaches half capacity. */
	kick = (defer_count - 1) == (defer_max >> 1);
    ...
	if (unlikely(kick))
		kick_defer_list_purge(cpu);
}

The reject path keeps the increment and never reaches
kick_defer_list_purge(). Once the counter is at or above defer_max, no
producer schedules a flush on the owner CPU.

Some CPUs only get NET_RX softirq through these kicks. One example is an
application CPU that sends TCP or UDP while NIC IRQs are pinned to
housekeeping CPUs and RPS/RFS is off. napi_consume_skb() frees its TX
skbs in a remote TX-completion NAPI. That path calls
skb_attempt_defer_free() because skb->alloc_cpu != smp_processor_id().

On such a CPU, wouldn't the reproducer from the commit message still
strand the counter with this patch applied?

  sysctl -w net.core.skb_defer_max=1
  sysctl -w net.core.skb_defer_max=128

The list is empty and the counter is large. Nothing calls
__skb_defer_free_flush() on that CPU, so the new clear never runs. When
the limit changes, proc_do_skb_defer_max() neither resets the counters
nor schedules a flush.

The same applies if the limit is lowered below the current count before
the half-capacity kick fires, for example from 128 to 32 with the counter
at 40. The queued skbs would then stay on a kick-only CPU's list
indefinitely.

The commit message says:

  "Clear the counter on the empty-list path as well, so no count can
  outlive an empty list and an already-stranded counter recovers."

Does that hold when the owner CPU never runs net_rx_action() on its own?
The cross-CPU TCP RX test would not show this case, because there the
alloc_cpu is the NAPI CPU.

Could the producer side change as well? One option is not to leave the
increment in place on reject, either by undoing it or by checking before
incrementing. Another is to make sure the owner CPU gets one kick when
the limit is reached.

> -	atomic_long_set(&sdn->defer_count, 0);
> +	}
>  	free_list = llist_del_all(&sdn->defer_list);
> +	atomic_long_set(&sdn->defer_count, 0);

[Severity: Low]
This is a pre-existing issue, but the skb_defer_max documentation and the
sysctl range don't match how the counter works.

Documentation/admin-guide/sysctl/net.rst says:

  Max size (in skbs) of the per-cpu list of skbs being freed
  by the cpu which allocated them.

skb_attempt_defer_free() increments before it checks
defer_count >= defer_max, so at most defer_max - 1 skbs are queued. The
list is also per (cpu, node), not per cpu. The doc also doesn't say that 0
turns deferral off, which proc_do_skb_defer_max() handles through
skb_defer_disable_key.

The ctl_table entry has .extra1 = SYSCTL_ZERO and no .extra2, so 1 and 2
are accepted. With those values, this half-capacity kick never fires:

	kick = (defer_count - 1) == (defer_max >> 1);

With 1, nothing is queued and the counter keeps growing. With 2, one skb
is queued and waits until the owner CPU runs NET_RX on its own.

One part of this is new with this patch. A producer that lands between
llist_del_all() and atomic_long_set() now leaves its skb queued but not
counted. The list can therefore briefly grow past defer_max - 1. The
commit message covers this ("bounded by the producers in flight").

Could a short comment here say that this is intentional? The similar
qdisc path in __dev_xmit_skb() has one:

	/* There is a small race because we clear defer_count not atomically
	 * with the prior llist_del_all(). This means defer_list could grow
	 * over qdisc_max_burst.
	 */

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261008220256.1844072-1-jkain%40nuro.ai

Re: [PATCH net v2] net: do not let defer_count outlive an empty defer_list

[Joseph Kain]

On Mon, Oct 12, 2026 at 4:28 PM <netdev-bot+sashiko@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
>
> Critical: 0 · High: 1 · Medium: 0 · Low: 0 (1)
>
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
>
> - [High] The patch only partly fixes a stranded defer_count.
>
> Pre-existing issues:
> - [Low] The skb_defer_max documentation and the sysctl range do not match
>   how the counter works.
>
> --
>
> net: do not let defer_count outlive an empty defer_list
>
> This patch changes __skb_defer_free_flush() so that it drains the
> defer_list before it clears defer_count. It also clears defer_count on the
> empty-list path. The goal is to stop a stale counter from turning off
> deferred skb freeing for a (cpu, node) pair until reboot.
>
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index e76762e29360e..e847e243a692e 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -6938,10 +6938,13 @@ static void __skb_defer_free_flush(struct skb_defer_node *sdn, int budget)
> >       struct llist_node *free_list;
> >       struct sk_buff *skb, *next;
> >
> > -     if (llist_empty(&sdn->defer_list))
> > +     if (llist_empty(&sdn->defer_list)) {
> > +             if (unlikely(atomic_long_read(&sdn->defer_count)))
> > +                     atomic_long_set(&sdn->defer_count, 0);
> >               return;
>
> [Severity: High]
> Does this recovery path ever run on a CPU whose counter is stranded?
>
> The counter is only cleared when the owner CPU calls
> __skb_defer_free_flush() itself. skb_defer_free_flush() only walks the
> this_cpu_ptr() sdns. It is called from net_rx_action(),
> napi_threaded_poll_loop() and busy-poll.
>
> The producer side in skb_attempt_defer_free() is unchanged:
>
> net/core/skbuff.c:skb_attempt_defer_free() {
>     ...
>         defer_count = atomic_long_inc_return(&sdn->defer_count);
>
>         if (defer_count >= defer_max)
>                 goto nodefer;
>     ...
>         /* Send an IPI every time queue reaches half capacity. */
>         kick = (defer_count - 1) == (defer_max >> 1);
>     ...
>         if (unlikely(kick))
>                 kick_defer_list_purge(cpu);
> }
>
> The reject path keeps the increment and never reaches
> kick_defer_list_purge(). Once the counter is at or above defer_max, no
> producer schedules a flush on the owner CPU.
>
> Some CPUs only get NET_RX softirq through these kicks. One example is an
> application CPU that sends TCP or UDP while NIC IRQs are pinned to
> housekeeping CPUs and RPS/RFS is off. napi_consume_skb() frees its TX
> skbs in a remote TX-completion NAPI. That path calls
> skb_attempt_defer_free() because skb->alloc_cpu != smp_processor_id().
>
> On such a CPU, wouldn't the reproducer from the commit message still
> strand the counter with this patch applied?
>
>   sysctl -w net.core.skb_defer_max=1
>   sysctl -w net.core.skb_defer_max=128
>
> The list is empty and the counter is large. Nothing calls
> __skb_defer_free_flush() on that CPU, so the new clear never runs. When
> the limit changes, proc_do_skb_defer_max() neither resets the counters
> nor schedules a flush.
>
> The same applies if the limit is lowered below the current count before
> the half-capacity kick fires, for example from 128 to 32 with the counter
> at 40. The queued skbs would then stay on a kick-only CPU's list
> indefinitely.
>
> The commit message says:
>
>   "Clear the counter on the empty-list path as well, so no count can
>   outlive an empty list and an already-stranded counter recovers."
>
> Does that hold when the owner CPU never runs net_rx_action() on its own?
> The cross-CPU TCP RX test would not show this case, because there the
> alloc_cpu is the NAPI CPU.
>
> Could the producer side change as well? One option is not to leave the
> increment in place on reject, either by undoing it or by checking before
> incrementing. Another is to make sure the owner CPU gets one kick when
> the limit is reached.
>
> > -     atomic_long_set(&sdn->defer_count, 0);
> > +     }
> >       free_list = llist_del_all(&sdn->defer_list);
> > +     atomic_long_set(&sdn->defer_count, 0);
>
> [Severity: Low]
> This is a pre-existing issue, but the skb_defer_max documentation and the
> sysctl range don't match how the counter works.
>
> Documentation/admin-guide/sysctl/net.rst says:
>
>   Max size (in skbs) of the per-cpu list of skbs being freed
>   by the cpu which allocated them.
>
> skb_attempt_defer_free() increments before it checks
> defer_count >= defer_max, so at most defer_max - 1 skbs are queued. The
> list is also per (cpu, node), not per cpu. The doc also doesn't say that 0
> turns deferral off, which proc_do_skb_defer_max() handles through
> skb_defer_disable_key.
>
> The ctl_table entry has .extra1 = SYSCTL_ZERO and no .extra2, so 1 and 2
> are accepted. With those values, this half-capacity kick never fires:
>
>         kick = (defer_count - 1) == (defer_max >> 1);
>
> With 1, nothing is queued and the counter keeps growing. With 2, one skb
> is queued and waits until the owner CPU runs NET_RX on its own.
>
> One part of this is new with this patch. A producer that lands between
> llist_del_all() and atomic_long_set() now leaves its skb queued but not
> counted. The list can therefore briefly grow past defer_max - 1. The
> commit message covers this ("bounded by the producers in flight").
>
> Could a short comment here say that this is intentional? The similar
> qdisc path in __dev_xmit_skb() has one:
>
>         /* There is a small race because we clear defer_count not atomically
>          * with the prior llist_del_all(). This means defer_list could grow
>          * over qdisc_max_burst.
>          */
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261008220256.1844072-1-jkain%40nuro.ai

Thank you, this is good feedback.  I will address this tomorrow in a v3 by
undoing the increment on the reject path instead and removing the
empty-list clear from v2 as it will become redundant.

--
Joseph Kain