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

[Carolina Jubran <cjubran@nvidia.com>]

On 16/10/2026 11:11, Jason Xing wrote:
> On Tue, Oct 13, 2026 at 11:59 PM Carolina Jubran <cjubran@nvidia.com> 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@nvidia.com>
>> Reviewed-by: Dragos Tatulea <dtatulea@nvidia.com>
>> Reviewed-by: Björn Töpel <bjorn@kernel.org>
> Thanks. Overall it looks good to me:
> Reviewed-by: Jason Xing <kerneljasonxing@gmail.com>
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
>>