Re: [PATCH RFC net-next v4 04/14] xsk: cache data buffers to avoid frequently calling kmalloc_reserve

On Thu, Apr 16, 2026 at 10:45 AM Jason Xing <[email protected]> wrote:
>
> On Wed, Apr 15, 2026 at 5:38 PM <[email protected]> wrote:
> >
> > Sashiko AI review found 2 potential issue(s):
> > - [High] Cached data buffers are consumed without validating current allocation flags, potentially violating memory placement constraints.
> > - [Medium] Meaningful code and commit message mismatch regarding cache refill logic.
> > --
> >
> > commit 5a442e9c44f0a64a147908843084cffe5e2fed9f
> > Author: Jason Xing <[email protected]>
> >
> > xsk: cache data buffers to avoid frequently calling kmalloc_reserve
> >
> > This commit introduces a per-socket data buffer cache for small packets
> > to reduce kmalloc_reserve() overhead. It adds bulk allocation from
> > skb_small_head_cache and persists unused buffers across batch cycles.
> >
> > >     Inside the Phase-1 per-descriptor loop, when a small packet needs a
> > >     data buffer and the cache is empty, a single kmem_cache_alloc_bulk()
> > >     refills it with generic_xmit_batch objects. Subsequent small packets
> > >     pop directly from the cache. Large packets bypass the cache entirely
> > >     and fall back to kmalloc_reserve(). Unused buffers remain in the
> > >     cache for the next batch.
> >
> > Is this description accurate? Looking at the implementation below, the bulk
> > allocation happens outside the Phase-1 loop. If the cache runs empty during
> > the loop, it appears to fall back to kmalloc_reserve() for each subsequent
> > packet rather than refilling the cache via kmem_cache_alloc_bulk(). Could
> > the commit message be updated to match the actual execution flow?
>
> Will update it.
>
> >
> > > diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> > > index c7e42c2966cdd..3b3969adfd23f 100644
> > > --- a/net/core/skbuff.c
> > > +++ b/net/core/skbuff.c
> > [ ... ]
> > > @@ -709,10 +718,17 @@ int xsk_alloc_batch_skb(struct xdp_sock *xs, u32 nb_pkts, u32 nb_descs, int *err
> > >
> > >                       skb = skbs[skb_count - 1 - i];
> > >                       skbuff_clear(skb);
> > > -                     data = kmalloc_reserve(&size, gfp_mask, node, skb);
> > > -                     if (unlikely(!data)) {
> > > -                             *err = -ENOBUFS;
> > > -                             break;
> > > +                     if (dc_count &&
> > > +                         SKB_HEAD_ALIGN(size) <= SKB_SMALL_HEAD_CACHE_SIZE) {
> >
> > If the socket's sk_allocation (which populates gfp_mask) was modified to
> > require restricted memory placement (e.g. __GFP_DMA), could this code use a
> > standard memory buffer left over from a previous batch?
> >
> > While the bulk allocation check before the loop correctly avoids allocating
> > from skb_small_head_cache if KMALLOC_NOT_NORMAL_BITS is set, the
> > consumption path here only checks dc_count. Does this need to validate
> > gfp_mask against KMALLOC_NOT_NORMAL_BITS before consuming from the cache,
> > to ensure memory placement constraints are not bypassed?
>
> I can add the check but it seems meaningless to me. In the whole
> implementation of xsk, there is no place where sk_allocation can be
> set to a different value other than GPF_KERNEL.

After a few rounds of investigation, I finally gave up the idea of
using KMALLOC_NOT_NORMAL_BITS. The reason has been explained.

Thanks,
Jason