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

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 1 · 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 does not remove the absorbing state that its commit
  message names in its first sentence (defer_count at or above…
- [Medium] skb_attempt_defer_free() (net/core/skbuff.c:7399-7400) now
  calls kick_defer_list_purge(cpu) on every rejected deferred free while…

Pre-existing issues:
- [Low] Documentation/admin-guide/sysctl/net.rst (lines 362-368)
  describes skb_defer_max as the 'Max size (in skbs) of the per-cpu…

--

net: schedule a purge when the defer queue rejects a free

This patch makes skb_attempt_defer_free() call kick_defer_list_purge()
when a deferred free is rejected and the defer_list is not empty. It also
changes the limit test from >= to >, so the effective limit becomes
skb_defer_max. Finally, __skb_defer_free_flush() now drains the list
before zeroing defer_count, so that defer_count no longer stays above
skb_defer_max while the defer_list is empty.

> diff --git a/net/core/dev.c b/net/core/dev.c
> index e76762e29360..a5912b728ddd 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -6940,8 +6940,11 @@ static void __skb_defer_free_flush(struct skb_defer_node *sdn, int budget)
>  
>  	if (llist_empty(&sdn->defer_list))
>  		return;
> -	atomic_long_set(&sdn->defer_count, 0);
>  	free_list = llist_del_all(&sdn->defer_list);
> +	/* Racy on purpose, as in __dev_xmit_skb(): a producer landing between
> +	 * the del_all and this store stays queued but uncounted.
> +	 */
> +	atomic_long_set(&sdn->defer_count, 0);

[Severity: Low]
This is a pre-existing issue, but the commit message says the new > test
matches "what Documentation/admin-guide/sysctl/net.rst describes". The
net.rst text is:

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

skb_attempt_defer_free() actually enforces the limit per (owner cpu,
producer node):

    sdn = per_cpu_ptr(net_hotdata.skb_defer_nodes, cpu) + cpu_to_node(my_cpu);

net.rst also doesn't say that 0 disables the feature through
skb_defer_disable_key in proc_do_skb_defer_max().

Should net.rst be updated to describe the per-node scope and what 0
means?

With the new ordering, a producer that calls llist_add() between
llist_del_all() and atomic_long_set() stays queued but uncounted. A list
can then go a few skbs past skb_defer_max. The old clear-then-drain order
allowed the same thing (increment before the clear, llist_add() after
the del_all). The new order only makes that window slightly larger, and
the new comment marks it as intentional.

>  
>  	llist_for_each_entry_safe(skb, next, free_list, ll_node) {
>  		prefetch(next);
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 41beaf625421..596f99811bac 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -7392,8 +7392,14 @@ nodefer:	kfree_skb_napi_cache(skb);
>  	defer_max = READ_ONCE(net_hotdata.sysctl_skb_defer_max);
>  	defer_count = atomic_long_inc_return(&sdn->defer_count);
>  
> -	if (defer_count >= defer_max)
> +	if (unlikely(defer_count > defer_max)) {
> +		/* The kept increment only comes back down when a flush zeroes
> +		 * it, and this is the one path that sees a full queue.
> +		 */
> +		if (!llist_empty(&sdn->defer_list))
> +			kick_defer_list_purge(cpu);

[Severity: High]
Can defer_count still get stuck above skb_defer_max while the
defer_list is empty?

This path only kicks when the list is not empty. The only reset of
defer_count is in __skb_defer_free_flush(), and that function returns
before the reset when the list is empty:

net/core/dev.c:__skb_defer_free_flush() {
    if (llist_empty(&sdn->defer_list))
        return;
    free_list = llist_del_all(&sdn->defer_list);
    ...
    atomic_long_set(&sdn->defer_count, 0);
    ...
}

So an increment that is rejected while the list is empty never triggers
a kick and is never cleared.

The Fixes: tag targets the 6.18 to 7.0 trees, which don't have
skb_defer_disable_key. There, net.core.skb_defer_max=0 seems to lead to
this sequence:

  - the first reject sees a non-empty list and kicks
  - the flush empties the list and sets defer_count to 0
  - every later cross-CPU free increments, rejects (count > 0), finds
    the list empty, and sends no kick

defer_count then grows without bound. Restoring 128 leaves it far above
128 with an empty list. That is the same "setting 0 and restoring the
previous value" case the commit message describes.

On mainline, proc_do_skb_defer_max() stores the new value before the
static key changes:

net/core/sysctl_net_core.c:proc_do_skb_defer_max() {
    ...
    ret = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
    nval = !net_hotdata.sysctl_skb_defer_max;

    if (nval != oval) {
        if (nval)
            static_branch_enable(&skb_defer_disable_key);
    ...
}

Producers that run between that store and static_branch_enable() taking
effect can read defer_max == 0 and raise the counter while the list is
empty. If a limit at or below the stranded count is then restored (for
example 0 followed by 1), would that latch in the same way?

If so, the claims "From v7.1 on, 0 is safe" and "The reject-path kick
recovers both" don't seem to hold. Some possible fixes:

  - undo the increment on reject
  - reset the counter, and kick, even when the list is empty
  - have proc_do_skb_defer_max() reset the counters or schedule flushes

Would one of these work?

[Severity: Medium]
Does this add a remote lock or a remote cmpxchg to every rejected free
while the list is full?

Before this patch, a reject only called kfree_skb_napi_cache(). Now every
reject with a non-empty list calls kick_defer_list_purge(). The commit
message says:

  kick_defer_list_purge() already coalesces through
  sd->defer_ipi_scheduled, so a burst of rejects costs one IPI.

When use_backlog_threads() is true, defer_ipi_scheduled is not used at
all. That is always the case on PREEMPT_RT, or with thread_backlog_napi
on the command line. The remote lock is also taken before
NAPI_STATE_SCHED is tested:

net/core/dev.c:kick_defer_list_purge() {
    if (use_backlog_threads()) {
        backlog_lock_irq_save(sd, &flags);

        if (!__test_and_set_bit(NAPI_STATE_SCHED, &sd->backlog.state))
            __napi_schedule_irqoff(&sd->backlog);

        backlog_unlock_irq_restore(sd, flags);
    } else if (!cmpxchg(&sd->defer_ipi_scheduled, 0, 1)) {
    ...
}

Every over-limit cross-CPU free (napi_consume_skb() on TX completion,
tcp_eat_recv_skb(), skb_consume_udp()) would then take the owner CPU's
sd->input_pkt_queue.lock with IRQs disabled, until that CPU's backlog
thread flushes. enqueue_to_backlog() and process_backlog() on the owner
CPU use the same lock.

In IPI mode, every reject does a cmpxchg on the remote
sd->defer_ipi_scheduled. trigger_rx_softirq() clears the flag before the
flush runs:

net/core/dev.c:trigger_rx_softirq() {
    __raise_softirq_irqoff(NET_RX_SOFTIRQ);
    smp_store_release(&sd->defer_ipi_scheduled, 0);
}

When threadirqs or ksoftirqd defers NET_RX, each reject in that gap
would send another IPI.

The notes below the cut line report about 756k kicks over about 750k
skb_attempt_defer_free() calls at skb_defer_max=1. That looks like one
kick per call rather than one per burst. Was the cost on PREEMPT_RT
measured?

>  		goto nodefer;
> +	}
>  
>  	llist_add(&skb->ll_node, &sdn->defer_list);
>

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