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@xxxxxxxxxx> 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