Re: [PATCH 1/2] mm: Add memalloc_nowait_{save,restore}

[Michal Hocko <mhocko@suse.com>]

On Wed 14-08-24 16:12:27, Yafang Shao wrote:
> On Wed, Aug 14, 2024 at 3:42 PM Michal Hocko <mhocko@suse.com> wrote:
> >
> > On Mon 12-08-24 20:59:53, Yafang Shao wrote:
> > > On Mon, Aug 12, 2024 at 7:37 PM Christoph Hellwig <hch@infradead.org> wrote:
> > > >
> > > > On Mon, Aug 12, 2024 at 05:05:24PM +0800, Yafang Shao wrote:
> > > > > The PF_MEMALLOC_NORECLAIM flag was introduced in commit eab0af905bfc
> > > > > ("mm: introduce PF_MEMALLOC_NORECLAIM, PF_MEMALLOC_NOWARN"). To complement
> > > > > this, let's add two helper functions, memalloc_nowait_{save,restore}, which
> > > > > will be useful in scenarios where we want to avoid waiting for memory
> > > > > reclamation.
> > > >
> > > > No, forcing nowait on callee contets is just asking for trouble.
> > > > Unlike NOIO or NOFS this is incompatible with NOFAIL allocations
> > >
> > > I don’t see any incompatibility in __alloc_pages_slowpath(). The
> > > ~__GFP_DIRECT_RECLAIM flag only ensures that direct reclaim is not
> > > performed, but it doesn’t prevent the allocation of pages from
> > > ALLOC_MIN_RESERVE, correct?
> >
> > Right but this means that you just made any potential nested allocation
> > within the scope that is GFP_NOFAIL a busy loop essentially. Not to
> > mention it BUG_ON as non-sleeping GFP_NOFAIL allocations are
> > unsupported. I believe this is what Christoph had in mind.
> 
> If that's the case, I believe we should at least consider adding the
> following code change to the kernel:

We already do have that
                /*
                 * All existing users of the __GFP_NOFAIL are blockable, so warn
                 * of any new users that actually require GFP_NOWAIT
                 */
                if (WARN_ON_ONCE_GFP(!can_direct_reclaim, gfp_mask))
                        goto fail;

But Barry has patches to turn that into BUG because failing NOFAIL
allocations is not cool and cause unexpected failures. Have a look at
https://lore.kernel.org/all/20240731000155.109583-1-21cnbao@gmail.com/

> > I am really
> > surprised that we even have PF_MEMALLOC_NORECLAIM in the first place!
> 
> There's use cases for it.

Right but there are certain constrains that we need to worry about to
have a maintainable code. Scope allocation contrains are really a good
feature when that has a well defined semantic. E.g. NOFS, NOIO or
NOMEMALLOC (although this is more self inflicted injury exactly because
PF_MEMALLOC had a "use case"). NOWAIT scope semantic might seem a good
feature but it falls appart on nested NOFAIL allocations! So the flag is
usable _only_ if you fully control the whole scoped context. Good luck
with that long term! This is fragile, hard to review and even harder to
keep working properly. The flag would have been Nacked on that ground.
But nobody asked...
-- 
Michal Hocko
SUSE Labs