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

[Barry Song <21cnbao@gmail.com>]

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.

>
> --
> Regards
> Yafang

Thanks
Barry