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

[Jakub Kicinski <kuba@kernel.org>]

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).