Re: [PATCH] lib/stackdepot: stackdepot: don't use __GFP_KSWAPD_RECLAIM from __stack_depot_save() if atomic context

[Michal Hocko <mhocko@suse.com>]

On Mon 22-05-23 11:47:25, Tetsuo Handa wrote:
> On 2023/05/22 11:13, Huang, Ying wrote:
> >> Any atomic allocation used by KASAN needs to drop __GFP_KSWAPD_RECLAIM bit.
> >> Where do we want to drop this bit (in the caller side, or in the callee side)?
> > 
> > Yes.  I think we should fix the KASAN.  Maybe define a new GFP_XXX
> > (instead of GFP_ATOMIC) for debug code?  The debug code may be called at
> > almost arbitrary places, and wakeup_kswap() isn't safe to be called in
> > some situations.
> 
> What do you think about removing __GFP_KSWAPD_RECLAIM from GFP_ATOMIC and GFP_NOWAIT?

Not a good idea IMO. It is really hard to achieve real locklessness in the
page allocator. If we ever need something like that it should be pretty
obviously requested by a dedicated gfp flag rather than overriding a
long term established semantic. While GFP_ATOMIC is a bit of a misnomer
it has many users who really only require non-sleeping behavior.

> Recent reports indicate that atomic allocations (GFP_ATOMIC and GFP_NOWAIT) are not safe
> enough to think "atomic". They just don't do direct reclaim, but they do take spinlocks.
> Removing __GFP_KSWAPD_RECLAIM from GFP_ATOMIC and GFP_NOWAIT simplifies locking dependency and
> reduces latency of atomic allocations (which is important when called from "atomic" context).

I would really like to see any numbers to believe this is the case
actually. Waking up kswapd should be pretty non-visible.

> I consider that memory allocations which do not do direct reclaim should be geared towards
> less locking dependency.
> 
> In general, GFP_ATOMIC or GFP_NOWAIT users will not allocate many pages.

This hugely depend on the workload. I do not think we can make any
generic statements like that.

> It is likely that somebody else tries to allocate memory using __GFP_DIRECT_RECLAIM
> right after GFP_ATOMIC or GFP_NOWAIT allocations. We unlikely need to wake kswapd
> upon GFP_ATOMIC or GFP_NOWAIT allocations.

The thing is that you do not know this is a the case. You might have a
IRQ heavy prossing making a lot of memory allocations (e.g. networking)
while the rest of the processing doesn't require any additional memory.
 
> If some GFP_ATOMIC or GFP_NOWAIT users need to allocate many pages, they can add
> __GFP_KSWAPD_RECLAIM explicitly; though allocating many pages using GFP_ATOMIC or
> GFP_NOWAIT is not recommended from the beginning...

As much as I do not really like the long term GFP_ATOMIC semantic I do
not think we should be changing it to what you are proposing for reasons
mentioned above. GFP_NOWAIT change is even more questionable. Many users
simply use GFP_NOWAIT as a way of an optimistic allocation with a more
expensinsive fallback. We do not want to allow those consumers to
consume watermark gap memory to force others to hit the direct reclaim
wall.

Really there is very likely only a handfull of users who cannot even
wake kswapd or perform other non-sleeping locking and those should
currently drop __GFP_KSWAPD_RECLAIM. Maybe we should consider an alias
for them to not bother with the low level flag. Maybe we will need
GFP_LOCKLESS or something similar.
-- 
Michal Hocko
SUSE Labs