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.
>
> --
> Regards
> Yafang
Thanks
Barry
