Re: [PATCH] MM: discard __GFP_ATOMIC

From: NeilBrown
Date: Mon Nov 22 2021 - 23:15:34 EST

On Tue, 23 Nov 2021, Michal Hocko wrote:
> On Sat 20-11-21 21:51:20, Neil Brown wrote:
> > On Sat, 20 Nov 2021, Matthew Wilcox wrote:
> > > On Fri, Nov 19, 2021 at 10:14:38AM +1100, NeilBrown wrote:
> > > > On Thu, 18 Nov 2021, Matthew Wilcox wrote:
> > > > > Surely this should be gfpflags_allow_blocking() instead of poking about
> > > > > in the innards of gfp flags?
> > > >
> > > > Possibly. Didn't know about gfpflags_allow_blocking(). From a quick
> > > > grep in the kernel, a whole lot of other people don't know about it
> > > > either, though clearly some do.
> > > >
> > > > Maybe we should reaname "__GFP_DIRECT_RECLAIM" to
> > > > "__GFP_ALLOW_BLOCKING", because that is what most users seems to care
> > > > about.
> > >
> > > I tend towards the school of thought that the __GFP flags should make
> > > sense to the implementation and users should use either GFP_ or functions.
> > > When we see users adding or subtracting __GFP flags, that's a problem.
> >
> > Except __GFP_NOWARN of course, or __GFP_ZERO, or __GFP_NOFAIL.
> > What about __GFP_HIGHMEM? __GFP_DMA? __GFP_HIGH?
> >
> > They all seem to be quite meaningful to the caller - explicitly
> > specifying properties of the memory or properties of the service.
> > (But maybe you would prefer __GFP_HIGH be spelled "__GFP_LOW_WATERMARK"
> > so it would make more sense to the implementation).
> >
> > __GFP_DIRECTRECLAIM seems to me to be more the exception than the rule -
> > specifying internal implementation details.
> I do not think it is viable to fix up gfp flags to be consistent :/

You may be right :-) Of course if we don't try - you'll definitely be right.

> Both __GFP_DIRECT_RECLAIM and __GFP_KSWAPD_RECLAIM are way too lowlevel
> but historically we've had requests to inhibit kswapd for a particular
> requests because that has led to problems - fun reading caf491916b1c1.

Unfortunately that commit doesn't provide any reasoning, just an
The best reasoning I could find was in caf491916b1c1 which was the initial
revert. There the primary reasoning was "there is a bug that we don't
have time for a proper fix before the next release, so let's just use
this quick fix".
... and maybe "the quick fix" was "the right fix", but I cannot tell from
the commit logs :-(

> __GFP_ALLOW_BLOCKING would make a lot of sense but I am not sure it
> would be a good match to __GFP_KSWAPD_RECLAIM.

So? __GFP_ALLOW_BLOCKING makes it clear what is, or is not, acceptable
to the caller. How much reclaim, or other activity, alloc_page()
engages in is largely irrelevant to the caller as lock as it doesn't
block if asked not to (and doesn't enter an FS if asked not to, etc).

> > Actually ... I take it back about __GFP_NOWARN. That probably shouldn't
> > exist at all. Warnings should be based on how stressed the mm system is,
> > not on whether the caller wants thinks failure is manageable.
> Unless we change the way when allocation warnings are triggered then we
> really need this. There are many opportunistic allocations with a
> fallback behavior which do not want to swamp kernel logs with failures
> that are of no use. Think of a THP allocation that really want to be
> just very quick and falls back to normal base pages otherwise. Deducing
> context which is just fine to not report failures is quite tricky and it
> can get wrong easily. Callers should know whether warning can be of any
> use in many cases.

"Unless" being the key work.
It makes sense to warn when a __GFP_HIGH or __GFP_MEMALLOC allocation
fails, because they are clearly important.
It makes sense to warning if direct reclaim and retrying were enabled,
as then alloc_page() has tried really hard, but failed anyway. Thought
maybe if COSTLY_ORDER is exceeded, then the warning is unlikely to be
But does it ever make sense to warn if either of

If we always suppressed warning when those flags were present, then many
(most?) uses for __GFP_NOWARN can be discarded.

I can see that some of the __GFP flags are designed to each perform a
single well-defined function and internally to mm/ that makes sense.
But exposing those flags to all users appears to be a recipe for
trouble. Hiding them all behind "__" doesn't stop people from using and
misusing them. Others are externally meaningful. Making them visually
similar to the ones we want to hide isn't helping anyone.

When Willy wrote:
> When we see users adding or subtracting __GFP flags, that's a problem.
the "problem" is not so much in the fact that they *do* but in the fact
that they *can*.

I would be greatly in favour of GFP flags which made sense to callers,
and a mapping to ALLOC_ flags in mm/ which makes sense to allocators.

I doubt anything outside mm/ cares about whether KSWAPD is woken or not.
IT probably should be for small-order allocations, and not so much for
large-order allocations. but mm/khugepaged.c might make other decisions.