Re: [PATCH v4 3/3] mm: warn about illegal __GFP_NOFAIL usage in a more appropriate location and manner

[Yafang Shao <laoar.shao@gmail.com>]

On Mon, Sep 2, 2024 at 12:00 PM Barry Song <21cnbao@gmail.com> wrote:
>
> On Mon, Sep 2, 2024 at 3:23 PM Yafang Shao <laoar.shao@gmail.com> wrote:
> >
> > On Sat, Aug 31, 2024 at 4:29 AM Barry Song <21cnbao@gmail.com> wrote:
> > >
> > > From: Barry Song <v-songbaohua@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 <vbabka@suse.cz>
> > > Signed-off-by: Barry Song <v-songbaohua@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