linux-kernel - Re: [PATCH net-next v2 5/8] net: skbuff: introduce napi_skb_cache_get

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5cde5d32-c077-4323-8be6-1a051e6fbc46@intel.com>
Date: Mon, 13 Jan 2025 14:47:50 +0100
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Paolo Abeni <pabeni@...hat.com>
CC: Andrew Lunn <andrew+netdev@...n.ch>, "David S. Miller"
	<davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski
	<kuba@...nel.org>, Lorenzo Bianconi <lorenzo@...nel.org>, Daniel Xu
	<dxu@...uu.xyz>, Alexei Starovoitov <ast@...nel.org>, Daniel Borkmann
	<daniel@...earbox.net>, Andrii Nakryiko <andrii@...nel.org>, John Fastabend
	<john.fastabend@...il.com>, Toke Høiland-Jørgensen
	<toke@...nel.org>, Jesper Dangaard Brouer <hawk@...nel.org>, Martin KaFai Lau
	<martin.lau@...ux.dev>, <netdev@...r.kernel.org>, <bpf@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net-next v2 5/8] net: skbuff: introduce
 napi_skb_cache_get_bulk()

From: Paolo Abeni <pabeni@...hat.com>
Date: Thu, 9 Jan 2025 14:16:22 +0100

> On 1/7/25 4:29 PM, Alexander Lobakin wrote:
>> Add a function to get an array of skbs from the NAPI percpu cache.
>> It's supposed to be a drop-in replacement for
>> kmem_cache_alloc_bulk(skbuff_head_cache, GFP_ATOMIC) and
>> xdp_alloc_skb_bulk(GFP_ATOMIC). The difference (apart from the
>> requirement to call it only from the BH) is that it tries to use
>> as many NAPI cache entries for skbs as possible, and allocate new
>> ones only if needed.

[...]

>> +u32 napi_skb_cache_get_bulk(void **skbs, u32 n)
>> +{
>> +	struct napi_alloc_cache *nc = this_cpu_ptr(&napi_alloc_cache);
>> +	u32 bulk, total = n;
>> +
>> +	local_lock_nested_bh(&napi_alloc_cache.bh_lock);
>> +
>> +	if (nc->skb_count >= n)
>> +		goto get;
> 
> I (mis?)understood from the commit message this condition should be
> likely, too?!?

It depends, I didn't want to make this unlikely() as will happen
sometimes anyway, while the two unlikely() below can happen only on when
the system is low on memory.

> 
>> +	/* No enough cached skbs. Try refilling the cache first */
>> +	bulk = min(NAPI_SKB_CACHE_SIZE - nc->skb_count, NAPI_SKB_CACHE_BULK);
>> +	nc->skb_count += kmem_cache_alloc_bulk(net_hotdata.skbuff_cache,
>> +					       GFP_ATOMIC | __GFP_NOWARN, bulk,
>> +					       &nc->skb_cache[nc->skb_count]);
>> +	if (likely(nc->skb_count >= n))
>> +		goto get;
>> +
>> +	/* Still not enough. Bulk-allocate the missing part directly, zeroed */
>> +	n -= kmem_cache_alloc_bulk(net_hotdata.skbuff_cache,
>> +				   GFP_ATOMIC | __GFP_ZERO | __GFP_NOWARN,
>> +				   n - nc->skb_count, &skbs[nc->skb_count]);
> 
> You should probably cap 'n' to NAPI_SKB_CACHE_SIZE. Also what about
> latency spikes when n == 48 (should be the maximum possible with such
> limit) here?

The current users never allocate more than 8 skbs in one bulk. Anyway,
the current approach wants to be a drop-in for
kmem_cache_alloc_bulk(skbuff_cache), which doesn't cap anything.
Not that this last branch allocates to @skbs directly, not to the percpu
NAPI cache.

> 
> /P

Thanks,
Olek