Re: [PATCH net v6] net: schedule a purge when the defer queue rejects a free

[Eric Dumazet]

Le lun. 19 oct. 2026 à 19:43, Joseph Kain <jkain@xxxxxxx> a écrit :
>
> __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.
> 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.
>
> skb_attempt_defer_free() increments before testing the limit and keeps
> the increment when it rejects, so a rejected free leaves the counter
> raised with nothing queued.
>
> Nothing brings it back down. The reject path returns before
> kick_defer_list_purge(), so once the counter is stuck no producer
> schedules a flush on the owner CPU. A CPU that never raises NET_RX itself
> -- an application CPU doing TX while NIC IRQs are pinned elsewhere and
> RPS is off -- then never runs the flush at all, so clearing the counter
> inside the flush cannot help it either. proc_do_skb_defer_max() neither
> resets the counters nor schedules a flush when the limit changes.
>
> It is reachable with 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.
>
> Kick the purge from the reject path instead. The reject is the one place
> that observes a full queue, so scheduling the flush there is what stops
> the counter outliving the list, and it does not depend on the owner CPU
> ever raising NET_RX on its own. kick_defer_list_purge() already coalesces
> through sd->defer_ipi_scheduled, so a burst of rejects costs one IPI.
>
> This also covers the case where skb_defer_max is lowered below the
> current defer_count. The kick condition tests equality with half the
> limit, so a reduction can move that point behind the counter and no
> later producer ever reaches it; rejecting without a kick then strands a
> non-empty list the same way. The reject-path kick recovers both.
>
> While here, test the limit with > rather than >=, so the effective limit
> is skb_defer_max rather than skb_defer_max - 1, matching what
> Documentation/admin-guide/sysctl/net.rst describes.
>
> That also makes the half-capacity kick reachable at small limits. At
> skb_defer_max = 1 nothing was ever queued, so no kick was ever sent, and
> at 2 the equality was never hit; both now kick. That is what commit
> c09b0cd2cc6c ("net: avoid strange behavior with skb_defer_max == 1") set
> out to allow.
>
> Also drain before clearing in the flush. A producer that queues between
> the clear and the drain currently has its skb drained but its increment
> kept, which over-counts and costs deferral until the next flush.
> Draining first matches the ordering __dev_xmit_skb() already uses for the
> qdisc defer_list.
>
> Found by manual code inspection, LLM-assisted, while backporting the
> llist conversion to 6.1 after hitting the sd->defer_lock contention it
> removes. The latch itself was not hit in production; it was reproduced by
> lowering net.core.skb_defer_max.
>
> Fixes: 844c9db7f7f5 ("net: use llist for sd->defer_list")
> Suggested-by: Kuniyuki Iwashima <kuniyu@xxxxxxxxxx>
> Assisted-by: Claude:claude-opus-5
> Signed-off-by: Joseph Kain <jkain@xxxxxxx>
> ---

Say not to very verbose changelog, that nobody will read.

Since commit 844c9db7f7f5 ("net: use llist for sd->defer_list"),
skb_attempt_defer_free() increments sdn->defer_count before testing
it against skb_defer_max, and keeps the increment when it rejects.
__skb_defer_free_flush() returns early on an empty list without
clearing the counter.

With skb_defer_max == 1, every free is rejected and nothing is ever
queued, so the counter grows without bound and is never reset.
Restoring a larger skb_defer_max does not recover: deferred freeing
stays disabled for that (cpu, node) until reboot.

  sysctl -w net.core.skb_defer_max=1
  sysctl -w net.core.skb_defer_max=128   # does not recover

Test the limit with '>' instead of '>=', so that the first skb after
a reset is always queued, and a counter above the limit implies a
non-empty list that a flush will clear. This also makes the effective
limit match the documentation, and makes the half-capacity kick
reachable for skb_defer_max of 1 and 2.

When rejecting, kick the owner CPU if the list is not empty, so that
lowering skb_defer_max below the current count cannot leave the list
waiting for a NET_RX on the owner CPU that may never come.
kick_defer_list_purge() coalesces through sd->defer_ipi_scheduled.

Also clear the counter after llist_del_all() rather than before,
as __dev_xmit_skb() does, so a racing producer is under-counted
rather than over-counted.