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

[Jason Xing]

On Sun, Oct 18, 2026 at 11:58 PM Carolina Jubran <cjubran@xxxxxxxxxx> wrote:
>
>
> On 16/10/2026 11:11, Jason Xing wrote:
> > 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>
> Thanks for the review!
> >> ---
> >>   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
>
>
> Yeah, I will drop one line. Thanks
>
> >
> >>          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...
>
> For skb_morph() this patch does not change the existing behavior.
> skb_morph() reuses an existing destination skb and turns it into a clone of
> src by calling __skb_clone(dst, src).
> Before this patch, alloc_cpu was part of the headers group, so
> __copy_skb_header() inside __skb_clone() copied src->alloc_cpu to
> dst->alloc_cpu implicitly.
> After moving alloc_cpu out of the headers group, the explicit
> C(alloc_cpu);
> in __skb_clone() preserves exactly the same behavior.

Makes sense.

>
>
> >
> > 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.
>
> Good point. The current documentation describes alloc_cpu as the CPU
> that allocated the skb, while skb_attempt_defer_free() refers to the
> CPU that allocated the skb/pages.
> So it seems that alloc_cpu is best effort for deferred freeing?

I suppose so.

Thanks,
Jason