On Mon, Sep 2, 2024 at 12:00 PM Barry Song <21cn...@gmail.com> wrote:
>
> On Mon, Sep 2, 2024 at 3:23 PM Yafang Shao <laoar.s...@gmail.com> wrote:
> >
> > On Sat, Aug 31, 2024 at 4:29 AM Barry Song <21cn...@gmail.com> wrote:
> > >
> > > From: Barry Song <v-songbao...@oppo.com>
> > >
> > > Three points for this change:
> > >
> > > 1. We should consolidate all warnings in one place. Currently, the
> > > order > 1 warning is in the hotpath, while others are in less
> > > likely scenarios. Moving all warnings to the slowpath will reduce
> > > the overhead for order > 1 and increase the visibility of other
> > > warnings.
> > >
> > > 2. We currently have two warnings for order: one for order > 1 in
> > > the hotpath and another for order > costly_order in the laziest
> > > path. I suggest standardizing on order > 1 since it’s been in
> > > use for a long time.
> > >
> > > 3. We don't need to check for __GFP_NOWARN in this case. __GFP_NOWARN
> > > is meant to suppress allocation failure reports, but here we're
> > > dealing with bug detection, not allocation failures. So replace
> > > WARN_ON_ONCE_GFP by WARN_ON_ONCE.
> > >
> > > Suggested-by: Vlastimil Babka <vba...@suse.cz>
> > > Signed-off-by: Barry Song <v-songbao...@oppo.com>
> > > ---
> > > mm/page_alloc.c | 50 ++++++++++++++++++++++++-------------------------
> > > 1 file changed, 25 insertions(+), 25 deletions(-)
> > >
> > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > > index c81ee5662cc7..e790b4227322 100644
> > > --- a/mm/page_alloc.c
> > > +++ b/mm/page_alloc.c
> > > @@ -3033,12 +3033,6 @@ struct page *rmqueue(struct zone *preferred_zone,
> > > {
> > > struct page *page;
> > >
> > > - /*
> > > - * We most definitely don't want callers attempting to
> > > - * allocate greater than order-1 page units with __GFP_NOFAIL.
> > > - */
> > > - WARN_ON_ONCE((gfp_flags & __GFP_NOFAIL) && (order > 1));
> > > -
> > > if (likely(pcp_allowed_order(order))) {
> > > page = rmqueue_pcplist(preferred_zone, zone, order,
> > > migratetype, alloc_flags);
> > > @@ -4175,6 +4169,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int
> > > order,
> > > {
> > > bool can_direct_reclaim = gfp_mask & __GFP_DIRECT_RECLAIM;
> > > bool can_compact = gfp_compaction_allowed(gfp_mask);
> > > + bool nofail = gfp_mask & __GFP_NOFAIL;
> > > const bool costly_order = order > PAGE_ALLOC_COSTLY_ORDER;
> > > struct page *page = NULL;
> > > unsigned int alloc_flags;
> > > @@ -4187,6 +4182,25 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned
> > > int order,
> > > unsigned int zonelist_iter_cookie;
> > > int reserve_flags;
> > >
> > > + if (unlikely(nofail)) {
> > > + /*
> > > + * We most definitely don't want callers attempting to
> > > + * allocate greater than order-1 page units with
> > > __GFP_NOFAIL.
> > > + */
> > > + WARN_ON_ONCE(order > 1);
> > > + /*
> > > + * Also we don't support __GFP_NOFAIL without
> > > __GFP_DIRECT_RECLAIM,
> > > + * otherwise, we may result in lockup.
> > > + */
> > > + WARN_ON_ONCE(!can_direct_reclaim);
> > > + /*
> > > + * PF_MEMALLOC request from this context is rather bizarre
> > > + * because we cannot reclaim anything and only can loop
> > > waiting
> > > + * for somebody to do a work for us.
> > > + */
> > > + WARN_ON_ONCE(current->flags & PF_MEMALLOC);
> >
> > I believe we should add below warning as well:
> >
> > WARN_ON_ONCE(gfp_mask & __GFP_NOMEMALLOC);
> > WARN_ON_ONCE(gfp_mask & __GFP_NORETRY);
> > WARN_ON_ONCE(gfp_mask & __GFP_RETRY_MAYFAIL);
> > ...
> >
> > I'm not sure if that is enough.
> > __GFP_NOFAIL is a really horrible thing.
>
> Thanks! I'd prefer to keep this patchset focused on the existing
> warnings and bugs. Any new warnings about size limits or checks
> for new flags can be addressed separately.
OK
Thanks for your work.
--
Regards
Yafang
