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

[Carolina Jubran]

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.


> 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?


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