Re: [PATCH v3 0/4] mm: clarify nofail memory allocation

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

On Thu, Aug 22, 2024 at 2:37 PM Barry Song <21cnbao@gmail.com> wrote:
>
> On Thu, Aug 22, 2024 at 12:41 AM Yafang Shao <laoar.shao@gmail.com> wrote:
> >
> > On Tue, Aug 20, 2024 at 12:05 AM Linus Torvalds
> > <torvalds@linux-foundation.org> wrote:
> > >
> > > On Mon, 19 Aug 2024 at 06:02, David Hildenbrand <david@redhat.com> wrote:
> > > > >
> > > > > If we must still fail a nofail allocation, we should trigger a BUG rather
> > > > > than exposing NULL dereferences to callers who do not check the return
> > > > > value.
> > > >
> > > > I am not convinced that BUG_ON is the right tool here to save the world,
> > > > but I see how we arrived here.
> > >
> > > I think the thing to do is to just add a
> > >
> > >      WARN_ON_ONCE((flags & __GFP_NOFAIL) && bad_nofail_alloc(oder, flags));
> > >
> > > or similar, where that bad_nofail_alloc() checks that the allocation
> > > order is small and that the flags are sane for a NOFAIL allocation.
> > >
> > > Because no, BUG_ON() is *never* the answer. The answer is to make sure
> > > nobody ever sets NOFAIL in situations where the allocation can fail
> > > and there is no way forward.
> > >
> > > A BUG_ON() will quite likely just make things worse. You're better off
> > > with a WARN_ON() and letting the caller just oops.
> > >
> > > Honestly, I'm perfectly fine with just removing that stupid useless
> > > flag entirely. The flag goes back to 2003 and was introduced in
> > > 2.5.69, and was meant to be for very particular uses that otherwise
> > > just looped waiting for memory.
> > >
> > > Back in 2.5.69, there was exactly one user: the jbd journal code, that
> > > did a buffer head allocation with GFP_NOFAIL.  By 2.6.0 that had
> > > expanded by another user in XFS, and even that one had a comment
> > > saying that it needed to be narrowed down. And in fact, by the 2.6.12
> > > release, that XFS use had been removed, but the jbd journal had grown
> > > another jbd_kmalloc case for transaction data. So at the beginning of
> > > the git archives, we had exactly *one* user (with two places).
> > >
> > > *THAT* is the kind of use that the flag was meant for: small
> > > allocations required to make forward progress in writeout during
> > > memory pressure.
> > >
> > > It has then expanded and is now a problem. The cases using GFP_NOFAIL
> > > for things like vmalloc() - which is by definition not a small
> > > allocation - should be just removed as outright bugs.
> >
> > One potential approach could be to rename GFP_NOFAIL to
> > GFP_NOFAIL_FOR_SMALL_ALLOC, specifically for smaller allocations, and
> > to clear this flag for larger allocations. However, the challenge lies
> > in determining what constitutes a 'small' allocation.
>
> I'm not entirely sure if our concern is with higher order or larger size.

I believe both should be considered. Since the higher-order task might
be easier to address, starting with that seems like the more
straightforward approach.

> Higher
> order might pose a problem, but larger size(not too large) isn't
> always an issue.
> Allocating 100 * 4KiB pages is possibly easier than allocating a single
> 128KB folio.
>
> Are we trying to limit the physical size or the physical order? If the concern
> is order, vmalloc manages __GFP_NOFAIL by mapping order-0 pages. If the
> concern is higher order, this sounds reasonable.  but it seems the buddy
> system already has code to trigger a warning even for order > 1:

To avoid potential livelock, it may be wise to drop this flag for
higher-order allocations as well. Following Linus's suggestion, we
could start by removing it for "> PAGE_ALLOC_COSTLY_ORDER".

>
> struct page *rmqueue(struct zone *preferred_zone,
>                         struct zone *zone, unsigned int order,
>                         gfp_t gfp_flags, unsigned int alloc_flags,
>                         int migratetype)
> {
>         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));

This line was added by Michal in commit 0f352e5392c8 ("mm: remove
__GFP_NOFAIL is deprecated comment"), but it appears that Michal has
since reconsidered his stance. ;)



--
Regards
Yafang