Re: [PATCH] MM: discard __GFP_ATOMIC

[Michal Hocko <mhocko@suse.com>]

On Wed 17-11-21 15:39:30, Neil Brown wrote:
> 
> __GFP_ATOMIC serves little purpose.
> It's main effect is to set ALLOC_HARDER which adds a few little boosts to
> increase the chance of an allocation succeeding, one of which is to
> lower the water-mark at which it will succeed.
> 
> It is *always* paired with __GFP_HIGH which sets ALLOC_HIGH which also
> adjusts this watermark.  It is probable that other users of __GFP_HIGH
> should benefit from the other little bonuses that __GFP_ATOMIC gets.

While I like to see __GFP_ATOMIC going away I am not really sure about
this particular part. We have 3 ways to get to memory reserves. One of
thme is directly controlable by __GFP_HIGH and two are internal to the
allocator to handle different situations - ALLOC_OOM is to help the oom
victim to make a fwd progress and ALLOC_HARDER should be for contexts
which cannot rely on the memory reclaim to continue.

What is the point of having ALLOC_HIGH and ALLOC_HARDER if you just
add both of them for __GFP_HIGH? I think you should be instead really
get back to pre d0164adc89f6b and allow ALLOC_HARDER for requests which
have neither of the reclaim allowed. That would require tweaking
GFP_ATOMIC as well I suspect and drop __GFP_KSWAPD_RECLAIM. Or do
something else.
 
> __GFP_ATOMIC also gives a warning if used with __GFP_DIRECT_RECLAIM.
> There is little point to this.  We already get a might_sleep() warning
> if __GFP_DIRECT_RECLAIM is set.

I believe the point of the warning was to stop any abuse of an
additional memory reserves for context which can reclaim and to spare
those to interrupt handlers - which usually use GFP_ATOMIC. A lack of
any reports suggests this hasn't happened and the warning can be
dropped. Would be worth a patch on its own with this explanation.
 
> __GFP_ATOMIC allows the "watermark_boost" to be side-stepped.  It is
> probable that testing ALLOC_HARDER is a better fit here.

This has been introduced by f80b08fc44536 but I have to say that I
haven't understood why this couldn't check for __GFP_DIRECT_RECLAIM
or one ALLOC_$FOO boosters rather than __GFP_ATOMIC. Again something for
a separate patch.
 
> __GFP_ATOMIC is used by tegra-smmu.c to check if the allocation might
> sleep.  This should test __GFP_DIRECT_RECLAIM instead.

Willy has already proposed a better alternative.

Thanks!
-- 
Michal Hocko
SUSE Labs