Re: [PATCH v2 net] net: skbuff: don't copy alloc_cpu in skb header copies

[Jason Xing]

On Tue, Oct 13, 2026 at 11:59 PM Carolina Jubran <cjubran@xxxxxxxxxx> wrote:
>
> Commit 68822bdf76f1 ("net: generalize skb freeing deferral to
> per-cpu lists") added alloc_cpu to struct sk_buff and placed it in the
> headers group copied by __copy_skb_header().
>
> This is wrong for newly allocated skbs. For example, skb_segment()
> allocates a new skb and records the current CPU in alloc_cpu, but
> __copy_skb_header() overwrites it with the value from the original skb.
>
> If the original skb was allocated on another CPU, the segment may later
> be treated as remotely allocated and unnecessarily deferred there when
> it is freed.
>
> Move alloc_cpu out of the headers group so it is preserved across
> header copies. Initialize it explicitly in skb_clone(), which allocates

nit: s/Initialize/Copying

> the skb object directly instead of going through __alloc_skb().
>
> This was discovered while investigating a throughput regression with
> software IPsec. The load used 512 IPsec tunnels carrying traffic over
> VXLAN, with 512 parallel iperf3 flows and an MTU of 4500.
>
> Before the fix:
>
>   16.09%  queued_spin_lock_slowpath
>              |
>              --15.90%--_raw_spin_lock_irqsave
>                        |
>                        --9.37%--__slab_free
>                                  kfree
>                                  skb_release_data
>                                  napi_consume_skb
>                                  mlx5e_poll_tx_cq
>
> After the fix:
>
>    6.84%  queued_spin_lock_slowpath
>              |
>              --6.57%--_raw_spin_lock_irqsave
>                       |
>                       --5.00%--__slab_free
>                                 kfree
>                                 skb_release_data
>                                 napi_consume_skb
>                                 mlx5e_poll_tx_cq
>
> Fixes: 68822bdf76f1 ("net: generalize skb freeing deferral to per-cpu lists")
> Signed-off-by: Carolina Jubran <cjubran@xxxxxxxxxx>
> Reviewed-by: Dragos Tatulea <dtatulea@xxxxxxxxxx>
> Reviewed-by: Björn Töpel <bjorn@xxxxxxxxxx>

Thanks. Overall it looks good to me:
Reviewed-by: Jason Xing <kerneljasonxing@xxxxxxxxx>

> ---
>  include/linux/skbuff.h | 2 +-
>  net/core/skbuff.c      | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> ---
> V2 log:
> - Move alloc_cpu after the headers group and fix the indentation. (Eric Dumazet)
> ---
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index a3ff380d43f01..b3272d379409e 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -1036,7 +1036,6 @@ struct sk_buff {
>         __u16                   tc_index;       /* traffic control index */
>  #endif
>
> -       u16                     alloc_cpu;
>

nit: no blank line here after you remove it

>         union {
>                 __wsum          csum;
> @@ -1088,6 +1087,7 @@ struct sk_buff {
>
>         ); /* end headers group */
>
> +       u16                     alloc_cpu;
>         /* These elements must be at the end, see alloc_skb() for details.  */
>         sk_buff_data_t          tail;
>         sk_buff_data_t          end;
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 41beaf625421a..3d82eb87f4582 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -1593,7 +1593,6 @@ static void __copy_skb_header(struct sk_buff *new, const struct sk_buff *old)
>  #ifdef CONFIG_NET_RX_BUSY_POLL
>         CHECK_SKB_FIELD(napi_id);
>  #endif
> -       CHECK_SKB_FIELD(alloc_cpu);
>  #ifdef CONFIG_XPS
>         CHECK_SKB_FIELD(sender_cpu);
>  #endif
> @@ -1618,6 +1617,7 @@ static struct sk_buff *__skb_clone(struct sk_buff *n, struct sk_buff *skb)
>         C(len);
>         C(data_len);
>         C(mac_len);
> +       C(alloc_cpu);

One minor question: does it affect the following two cases:
1) skb_morph()
2) else branch in skb_clone()

Looking at the above logic, I think as to #2 point it seems the impact
doesn't matter...

And I'm a bit confused reading the allocation process because there
are two possible split allocations for the skb structure itself and
the head/data area. They might run on different cpus, which means it's
hard for 'alloc_cpu" to choose appropriate one to free under this
condition.

Thanks,
Jason

>         n->hdr_len = skb->nohdr ? skb_headroom(skb) : skb->hdr_len;
>         n->cloned = 1;
>         n->nohdr = 0;
> --
> 2.38.1
>