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

sashiko-bot
2026-04-15T09:38:23Z

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?

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

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