netdev - Re: [PATCH net-next v8 07/17] net: page_pool: introduce page_pool_mp_return_in

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: <20241210160651.32b5e8f8@kernel.org>
Date: Tue, 10 Dec 2024 16:06:51 -0800
From: Jakub Kicinski <kuba@...nel.org>
To: Pavel Begunkov <asml.silence@...il.com>
Cc: David Wei <dw@...idwei.uk>, io-uring@...r.kernel.org,
 netdev@...r.kernel.org, Jens Axboe <axboe@...nel.dk>, Paolo Abeni
 <pabeni@...hat.com>, "David S. Miller" <davem@...emloft.net>, Eric Dumazet
 <edumazet@...gle.com>, Jesper Dangaard Brouer <hawk@...nel.org>, David
 Ahern <dsahern@...nel.org>, Mina Almasry <almasrymina@...gle.com>,
 Stanislav Fomichev <stfomichev@...il.com>, Joe Damato <jdamato@...tly.com>,
 Pedro Tammela <pctammela@...atatu.com>
Subject: Re: [PATCH net-next v8 07/17] net: page_pool: introduce
 page_pool_mp_return_in_cache

On Tue, 10 Dec 2024 04:31:53 +0000 Pavel Begunkov wrote:
> >> +	page_pool_dma_sync_for_device(pool, netmem, -1);
> >> +	page_pool_fragment_netmem(netmem, 1);
> >> +	pool->alloc.cache[pool->alloc.count++] = netmem;  
> > 
> > and here:
> > 
> > 	return true;
> > 
> > this say mps can use return value as a stop condition in a do {} while()
> > loop, without having to duplicate the check.
> > 
> > 	do {
> > 		netmem = alloc...
> > 		... logic;
> > 	} while (page_pool_mp_alloc_refill(pp, netmem));
> > 
> > 	/* last netmem didn't fit in the cache */
> > 	return netmem;  
> 
> That last netmem is a problem. Returning it is not a bad option,
> but it doesn't feel right. Providers should rather converge to
> one way of returning buffers and batching here is needed.
> 
> I'd rather prefer this one then
> 
> while (pp_has_space()) {
> 	netmem = alloc();
> 	pp_push(netmem);
> }
> 
> Any thoughts on that?

No strong preference. If I was the one who placed the ->alloc() call
the way it is placed it's probably because it saves us a conditional,
it saves us checking if the cache is empty. 

If we want to have the page pool pick the last object out of the cache
I'd lean towards avoiding all the helpers. Make the callback:

 void alloc_netmem(struct page_pool *pool, gfp_t gfp,
                   size_t cnt, netmem_ref *arr);

you get an @arr(ay) to fill up with up to @cnt elements. Mirroring
alloc_pages_bulk_array_node() (down to its, to me at least, unusual
ordering of arguments).