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

[sashiko-bot@kernel.org]

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