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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] net: skb_attempt_defer_free() still latches defer_count on skb_defer_max=0
- [Medium] net: repeated IPIs and lock contention in skb_attempt_defer_free()
--

commit 6c549c105bac5ff899022b2e84c0969ec08de899
Author: Joseph Kain <jkain@nuro.ai>

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

This commit schedules a defer list purge from the reject path in
skb_attempt_defer_free() when defer_count exceeds defer_max, changes the
limit check from >= to >, and moves the defer_count reset after
llist_del_all() in __skb_defer_free_flush().

> diff --git a/net/core/dev.c b/net/core/dev.c
> index e76762e29360e..a5912b728ddd7 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);
>  
>  	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 41beaf625421a..596f99811bacd 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);
>  		goto nodefer;
> +	}

[Severity: Medium]
Can setting net.core.skb_defer_max to 0 and later restoring a positive
limit still latch sdn->defer_count in the absorbing state?

In net/core/sysctl_net_core.c:proc_do_skb_defer_max(),
proc_dointvec_minmax() writes 0 to net_hotdata.sysctl_skb_defer_max
before static_branch_enable(&skb_defer_disable_key) patches the jump
label across CPUs:

    oval = !net_hotdata.sysctl_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);
        else
            static_branch_disable(&skb_defer_disable_key);
    }

Additionally, preemptible callers in tcp_eat_recv_skb(),
skb_consume_udp(), or mptcp_eat_recv_skb() may already have passed the
skb_defer_disable_key check in skb_attempt_defer_free(), and on 6.18
through 7.0 stable kernels targeted by Fixes: 844c9db7f7f5,
skb_defer_disable_key is not present at all.

When defer_max is 0 and sdn->defer_list is empty (either initially or
after a flush drains existing entries), each producer in
skb_attempt_defer_free() increments sdn->defer_count to 1, 2, ..., N,
sees defer_count > 0, skips kick_defer_list_purge(cpu) because
llist_empty(&sdn->defer_list) is true, and jumps to nodefer without
adding to sdn->defer_list.

Because __skb_defer_free_flush() returns early when
llist_empty(&sdn->defer_list) is true without zeroing sdn->defer_count,
restoring net.core.skb_defer_max to a value <= N leaves sdn->defer_count
at or above defer_max with an empty sdn->defer_list, permanently
disabling deferred freeing for that (cpu, node) pair.

Also, if sdn->defer_list has fewer than (defer_max >> 1) entries queued
when net.core.skb_defer_max is set to 0, enabling skb_defer_disable_key
causes subsequent producers to return before the reject path while
proc_do_skb_defer_max() neither kicks nor flushes pending nodes, which
can strand those queued SKBs if the owner CPU does not raise
NET_RX_SOFTIRQ on its own.

[Severity: Medium]
Does calling kick_defer_list_purge(cpu) on every rejected SKB cause
repeated IPIs and lock contention when the target CPU has BH disabled or
uses backlog threads?

The commit message notes that kick_defer_list_purge() coalesces through
sd->defer_ipi_scheduled so a burst of rejects costs one IPI. However, in
net/core/dev.c:trigger_rx_softirq(), sd->defer_ipi_scheduled is cleared
in hardirq IPI context immediately after raising NET_RX_SOFTIRQ, before
net_rx_action() and skb_defer_free_flush() run:

    static void trigger_rx_softirq(void *data)
    {
        struct softnet_data *sd = data;

        __raise_softirq_irqoff(NET_RX_SOFTIRQ);
        smp_store_release(&sd->defer_ipi_scheduled, 0);
    }

If the target alloc_cpu is running with BH disabled (for example under
bh_lock_sock(), local_bh_disable(), or napi_poll()), or if softirq
processing is deferred to ksoftirqd, hardirqs remain enabled on that
CPU. Each CSD IPI interrupts the target CPU in hardirq context and clears
sd->defer_ipi_scheduled to 0 while sdn->defer_list remains non-empty,
allowing subsequent rejected frees in skb_attempt_defer_free() to send
repeated IPIs until skb_defer_free_flush() actually drains the list.

Furthermore, when use_backlog_threads() is true (such as on
CONFIG_PREEMPT_RT), kick_defer_list_purge() unconditionally acquires
sd->input_pkt_queue.lock via backlog_lock_irq_save() on every rejected
free even when NAPI_STATE_SCHED is already set on sd->backlog.state:

    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);
    }

>  
>  	llist_add(&skb->ll_node, &sdn->defer_list);
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019174400.1377093-1-jkain@nuro.ai?part=1