Re: [PATCH] mm/zblock: use vmalloc for page allocations

[Uladzislau Rezki <urezki@gmail.com>]

On Mon, May 05, 2025 at 10:29:49AM -0400, Johannes Weiner wrote:
> On Fri, May 02, 2025 at 10:01:56AM +0200, Vitaly Wool wrote:
> >  static struct zblock_block *alloc_block(struct zblock_pool *pool,
> >  					int block_type, gfp_t gfp,
> > -					unsigned long *handle)
> > +					unsigned long *handle,
> > +					unsigned int nid)
> >  {
> > +	struct block_list *block_list = &pool->block_lists[block_type];
> > +	unsigned int num_pages = block_desc[block_type].num_pages;
> >  	struct zblock_block *block;
> > -	struct block_list *block_list;
> > +	struct page *page = NULL;
> >  
> > -	block = (void *)__get_free_pages(gfp, block_desc[block_type].order);
> > -	if (!block)
> > -		return NULL;
> > +	if (!vmalloc_small_blocks && zblock_get_order(num_pages) >= 0) {
> > +		page = try_alloc_pages(nid, zblock_get_order(num_pages));
> 
> This is broken in several ways.
> 
> The function is meant for NMI contexts - the "try" refers to
> trylocking the freelists, in case whatever got interrupted was inside
> the allocator already. This will fall back to vmalloc unpredictably.
> 
> It also doesn't take a gfp parameter, which ignores the zswap ones,
> and substitutes a set that doesn't make any sense in this context:
> __GFP_NOMEMALLOC is counter productive inside reclaim; __GFP_ACCOUNT
> wreaks complete havoc on how compressed memory is charged to cgroups
> (double charging the wrong groups for shared blocks).
> 
+ "&& zblock_get_order(num_pages) >= 0" is always true?

A fallback makes sense to use when order > 0, IMO. Or just stick
fully to vmalloc. Another option is kvmalloc()/kvfree().

--
Uladzislau Rezki