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

[netdev-bot+sashiko@kernel.org]

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

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

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 fix depends on __skb_defer_free_flush() running on the CPU
  that owns the list.

--

net: do not let defer_count outlive an empty defer_list

The patch reorders __skb_defer_free_flush() so that defer_count is cleared
after llist_del_all() instead of before it. It also clears a non-zero
defer_count when the defer_list is found empty. The goal is to recover a
(cpu, node) whose counter is stranded at or above skb_defer_max.

> 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 reset run in the stranded state?

__skb_defer_free_flush() only runs for this_cpu's nodes. Its callers are
net_rx_action(), threaded NAPI, busy poll and CPU hotplug. The producer
side, skb_attempt_defer_free() in net/core/skbuff.c, increments and
rejects before it reaches the only kick:

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

	if (defer_count >= defer_max)
		goto nodefer;

	llist_add(&skb->ll_node, &sdn->defer_list);
    ...
	kick = (defer_count - 1) == (defer_max >> 1);
    ...
	if (unlikely(kick))
		kick_defer_list_purge(cpu);
}

Once defer_count >= defer_max, every producer goes through nodefer and
returns. None of them calls kick_defer_list_purge(), so nothing raises
NET_RX_SOFTIRQ on the owner CPU.

napi_consume_skb() sends every alien TX-completion skb here. The owner can
be an application CPU that allocates TX skbs but does no NAPI, RPS/RFS or
backlog processing itself. On such a CPU, does the commit's own reproducer
still latch?

  sysctl -w net.core.skb_defer_max=1    # every free rejected, count climbs
  sysctl -w net.core.skb_defer_max=128  # count already >= 128, no kick

In that case the new reset in __skb_defer_free_flush() never runs, and
deferral stays off for that (cpu, node).

The same thing seems to happen in two other cases. With defer_max=2, one
skb gets queued but the kick condition (defer_count - 1) == 1 never matches
before the reject. It also happens whenever skb_defer_max is lowered below
the current count. proc_do_skb_defer_max() does not reset the per-node
counters or request a flush when a positive value is restored.

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."

Is that only true when the owner CPU runs NET_RX for unrelated reasons?

Would a producer-side change also be needed to close this? Two options are
undoing the increment with atomic_long_dec() on the reject path, or kicking
or flushing the owner on reject.

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

[ ... ]

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