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

Jason Xing
2026-04-16T02:46:38Z

On Wed, Apr 15, 2026 at 5:38 PM <sashiko-bot@kernel.org> 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 <kernelxing@tencent.com>
>
> 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.

Thanks,
Jason

>
> > + data = dc[--dc_count];
> > + size = SKB_SMALL_HEAD_CACHE_SIZE;
> > + } else {
> > + data = kmalloc_reserve(&size, gfp_mask,
> > + node, skb);
> > + if (unlikely(!data)) {
> > + *err = -ENOBUFS;
> > + break;
> > + }
> > }
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260415082654.21026-1-kerneljasonxing@gmail.com?part=4