Re: [PATCH RFC net-next v4 12/14] xsk: separate read-mostly and write-heavy fields in xsk_buff_pool

On Wed, Apr 15, 2026 at 5:20 PM <[email protected]> wrote:
>
> Sashiko AI review found 1 potential issue(s):
> - [High] The patch groups NAPI RX write-heavy fields and NAPI TX write-heavy fields into the exact same cacheline, introducing severe false sharing between RX and TX CPUs.
> --
>
> commit cf39ff4311089098b2e57c37ba6e85e7a36ac4a8
> Author: Jason Xing <[email protected]>
>
> xsk: separate read-mostly and write-heavy fields in xsk_buff_pool
>
> This commit separates read-mostly pool geometry fields from write-heavy
> fields in struct xsk_buff_pool by adding cacheline alignment. This is
> intended to eliminate false sharing between the sendmsg CPU and the NAPI
> TX-completion CPU.
>
> > diff --git a/include/net/xsk_buff_pool.h b/include/net/xsk_buff_pool.h
> > index 0b1abdb99c9ea..0376bd4cc95b0 100644
> > --- a/include/net/xsk_buff_pool.h
> > +++ b/include/net/xsk_buff_pool.h
>
> [ ... ]
>
> > @@ -73,23 +73,27 @@ struct xsk_buff_pool {
>
> [ ... ]
>
> >       void *addrs;
> > +
> > +     /* Write-heavy fields */
> >       /* Mutual exclusion of the completion ring in the SKB mode.
> >        * Protect: NAPI TX thread and sendmsg error paths in the SKB
> >        * destructor callback.
> >        */
> > -     spinlock_t cq_prod_lock;
> > +     spinlock_t cq_prod_lock ____cacheline_aligned_in_smp;
> > +     u8 cached_need_wakeup;
> > +     u32 free_heads_cnt;
> >       struct xdp_buff_xsk *free_heads[];
> >  };
>
> Could placing these four fields together in the same aligned cacheline
> introduce new false sharing between the NAPI RX and TX CPUs?
>
> The fields cq_prod_lock and cached_need_wakeup are updated by the TX
> completion path.
>
> However, free_heads_cnt and the free_heads flexible array are heavily
> mutated by the RX path during buffer allocation and release.
>
> Because cq_prod_lock (spinlock_t) and cached_need_wakeup (u8) only consume
> a fraction of the 64-byte cacheline, free_heads_cnt and the first several
> elements of the free_heads array will pack into this exact same cacheline.
>
> If a deployment pins RX and TX interrupts to different CPU cores, it looks
> like this might cause continuous cacheline bouncing between the RX CPU and
> the TX CPU for every packet processed.

It's not introduced by this patch. Previously those counters were put
in the same cacheline.

But the suggestion is good, which inspires me to separate the tx and
rx counters at the expense of adding more bits of the whole structure.

Now the problem is gone:
spinlock_t cq_prod_lock ____cacheline_aligned_in_smp;
u8 cached_need_wakeup;
u32 free_heads_cnt ____cacheline_aligned_in_smp;
struct xdp_buff_xsk *free_heads[];

Thanks,
Jason

>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=12