Re: [PATCH 1/2] mm: page_frag: Check fragsz at the beginning of __page_frag_alloc_align()

[Yunsheng Lin <linyunsheng@huawei.com>]

On 2025/4/4 5:21, Haiyang Zhang wrote:
> Frag allocator is not designed for fragsz > PAGE_SIZE. So, check and return
> the error at the beginning of __page_frag_alloc_align(), instead of
> succeed for a few times, then fail due to not refilling the cache.
> 
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> ---
>  mm/page_frag_cache.c | 22 +++++++++-------------
>  1 file changed, 9 insertions(+), 13 deletions(-)
> 
> diff --git a/mm/page_frag_cache.c b/mm/page_frag_cache.c
> index d2423f30577e..d6bf022087e7 100644
> --- a/mm/page_frag_cache.c
> +++ b/mm/page_frag_cache.c
> @@ -98,6 +98,15 @@ void *__page_frag_alloc_align(struct page_frag_cache *nc,
>  	unsigned int size, offset;
>  	struct page *page;
>  
> +	if (unlikely(fragsz > PAGE_SIZE)) {
> +		/*
> +		 * The caller is trying to allocate a fragment
> +		 * with fragsz > PAGE_SIZE which is not supported
> +		 * by design. So we simply return NULL here.
> +		 */
> +		return NULL;
> +	}

The checking is done at below to avoid doing the checking for the
likely case of cache being enough as the frag API is mostly used
to allocate small memory.

And it seems my recent refactoring to frag API have made two
frag API misuse more obvious if I recalled it correctly. If more
explicit about that for all the codepath is really helpful, perhaps
VM_BUG_ON() is an option to make it more explicit while avoiding
the checking as much as possible.

> +
>  	if (unlikely(!encoded_page)) {
>  refill:
>  		page = __page_frag_cache_refill(nc, gfp_mask);
> @@ -119,19 +128,6 @@ void *__page_frag_alloc_align(struct page_frag_cache *nc,
>  	size = PAGE_SIZE << encoded_page_decode_order(encoded_page);
>  	offset = __ALIGN_KERNEL_MASK(nc->offset, ~align_mask);
>  	if (unlikely(offset + fragsz > size)) {
> -		if (unlikely(fragsz > PAGE_SIZE)) {
> -			/*
> -			 * The caller is trying to allocate a fragment
> -			 * with fragsz > PAGE_SIZE but the cache isn't big
> -			 * enough to satisfy the request, this may
> -			 * happen in low memory conditions.
> -			 * We don't release the cache page because
> -			 * it could make memory pressure worse
> -			 * so we simply return NULL here.
> -			 */
> -			return NULL;
> -		}
> -
>  		page = encoded_page_decode_page(encoded_page);
>  
>  		if (!page_ref_sub_and_test(page, nc->pagecnt_bias))