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

[Haiyang Zhang]

> -----Original Message-----
> From: Yunsheng Lin <linyunsheng@xxxxxxxxxx>
> Sent: Wednesday, April 16, 2025 3:38 AM
> To: Haiyang Zhang <haiyangz@xxxxxxxxxxxxx>; linux-hyperv@xxxxxxxxxxxxxxx;
> akpm@xxxxxxxxxxxxxxxxxxxx; corbet@xxxxxxx; linux-mm@xxxxxxxxx; linux-
> doc@xxxxxxxxxxxxxxx
> Cc: Dexuan Cui <decui@xxxxxxxxxxxxx>; KY Srinivasan <kys@xxxxxxxxxxxxx>;
> Paul Rosswurm <paulros@xxxxxxxxxxxxx>; olaf@xxxxxxxxx;
> vkuznets@xxxxxxxxxx; davem@xxxxxxxxxxxxx; wei.liu@xxxxxxxxxx; Long Li
> <longli@xxxxxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx
> Subject: [EXTERNAL] Re: [PATCH 1/2] mm: page_frag: Check fragsz at the
> beginning of __page_frag_alloc_align()
> 
> 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@xxxxxxxxxxxxx>
> > ---
> >  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.

Thanks, I will use VM_BUG_ON() instead of a checking here. Also, will
keep the checking below unchanged.

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