> -----Original Message-----
> From: Yunsheng Lin <linyunsh...@huawei.com>
> Sent: Wednesday, April 16, 2025 3:38 AM
> To: Haiyang Zhang <haiya...@microsoft.com>; linux-hyp...@vger.kernel.org;
> a...@linux-foundation.org; cor...@lwn.net; linux...@kvack.org; linux-
> d...@vger.kernel.org
> Cc: Dexuan Cui <de...@microsoft.com>; KY Srinivasan <k...@microsoft.com>;
> Paul Rosswurm <paul...@microsoft.com>; o...@aepfle.de;
> vkuzn...@redhat.com; da...@davemloft.net; wei....@kernel.org; Long Li
> <lon...@microsoft.com>; linux-ker...@vger.kernel.org
> 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 <haiya...@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.
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))
