Re: [PATCH v6 6/9] mm: multigenerational lru: aging

[Yu Zhao <yuzhao@google.com>]

On Wed, Jan 12, 2022 at 11:28:57AM +0100, Michal Hocko wrote:
> On Tue 11-01-22 16:16:57, Yu Zhao wrote:
> > On Mon, Jan 10, 2022 at 04:01:13PM +0100, Michal Hocko wrote:
> > > On Thu 06-01-22 17:12:18, Michal Hocko wrote:
> > > > On Tue 04-01-22 13:22:25, Yu Zhao wrote:
> > > > > +static struct lru_gen_mm_walk *alloc_mm_walk(void)
> > > > > +{
> > > > > +	if (!current->reclaim_state || !current->reclaim_state->mm_walk)
> > > > > +		return kvzalloc(sizeof(struct lru_gen_mm_walk), GFP_KERNEL);
> > > 
> > > One thing I have overlooked completely.
> > 
> > I appreciate your attention to details but GFP_KERNEL is legit in the
> > reclaim path. It's been used many years in our production, e.g.,
> >   page reclaim
> >     swap_writepage()
> >       frontswap_store()
> >         zswap_frontswap_store()
> >           zswap_entry_cache_alloc(GFP_KERNEL)
> > 
> > (And I always test my changes with lockdep, kasan, DEBUG_VM, etc., no
> >  warnings ever seen from using GFP_KERNEL in the reclaim path.)
> 
> OK, I can see it now. __need_reclaim will check for PF_MEMALLOC and skip
> the fs_reclaim tracking.
> 
> I still maintain I am not really happy about (nor in the zswap example)
> allocations from the direct reclaim context. I would really recommend
> using a pre-allocated pool of objects.

Not trying to argue anything -- there are many other places in the
reclaim path that must allocate memory to make progress, e.g.,

  add_to_swap_cache()
    xas_nomem()

  __swap_writepage()
    bio_alloc()

The only way to not allocate memory is drop clean pages. Writing dirty
pages (not swap) might require allocations as well. (But we only write
dirty pages in kswapd, not in the direct reclaim path.)

> If there are strong reasons for not doing so then at lease change that
> to kzalloc.

Consider it done.