Re: [PATCH v4 3/3] mm/vmscan: don't demote if there is not enough free memory in the lower memory tier

[Joshua Hahn <joshua.hahnjy@gmail.com>]

> > > Therefore, it appears that the behavior of get_swappiness() is important
> > > in this issue.
> >
> > This is quite mysterious.
> >
> > Especially because get_swappiness() is an MGLRU exclusive function, I find
> > it quite strange that the issue you mention above occurs regardless of whether
> > MGLRU is enabled or disabled. With MGLRU disabled, did you see the same hangs
> > as before? Were these hangs similarly fixed by modifying the callsite in
> > get_swappiness?
> 
> Good point.
> When MGLRU is disabled, changing only the behavior of can_demote()
> called by get_swappiness() did not solve the problem.
> 
> Instead, the problem was avoided by changing only the behavior of
> can_demote() called by can_reclaim_anon_page(), without changing the
> behavior of can_demote() called from other places.
> 
> > On a separate note, I feel a bit uncomfortable for making this the default
> > setting, regardless of whether there is swap space or not. Just as it is
> > easy to create a degenerate scenario where all memory is unreclaimable
> > and the system starts going into (wasteful) reclaim on the lower tiers,
> > it is equally easy to create a scenario where all memory is very easily
> > reclaimable (say, clean pagecache) and we OOM without making any attempt to
> > free up memory on the lower tiers.
> >
> > Reality is likely somewhere in between. And from my perspective, as long as
> > we have some amount of easily reclaimable memory, I don't think immediately
> > OOMing will be helpful for the system (and even if none of the memory is
> > easily reclaimable, we should still try doing something before killing).
> >
> > > > > The reason for this issue is that memory allocations do not directly
> > > > > trigger the oom-killer, assuming that if the target node has an underlying
> > > > > memory tier, it can always be reclaimed by demotion.
> >
> > This patch enforces that the opposite of this assumption is true; that even
> > if a target node has an underlying memory tier, it can never be reclaimed by
> > demotion.
> >
> > Certainly for systems with swap and some compression methods (z{ram, swap}),
> > this new enforcement could be harmful to the system. What do you think?
> 
> Thank you for the detailed explanation.
> 
> I understand the concern regarding the current patch, which only
> checks the free memory of the demotion target node.
> I will explore a solution.

Hello Akinobu, I hope you had a great weekend!

I noticed something that I thought was worth flagging. It seems like the
primary addition of this patch, which is to check for zone_watermark_ok
across the zones, is already a part of should_reclaim_retry():

    /*
     * Keep reclaiming pages while there is a chance this will lead
     * somewhere.  If none of the target zones can satisfy our allocation
     * request even if all reclaimable pages are considered then we are
     * screwed and have to go OOM.
     */
    for_each_zone_zonelist_nodemask(zone, z, ac->zonelist,
                ac->highest_zoneidx, ac->nodemask) {

	[...snip...]

        /*
         * Would the allocation succeed if we reclaimed all
         * reclaimable pages?
         */
        wmark = __zone_watermark_ok(zone, order, min_wmark,
                ac->highest_zoneidx, alloc_flags, available);

        if (wmark) {
            ret = true;
            break;
        }
    }

... which is called in __alloc_pages_slowpath. I wonder why we don't already
hit this. It seems to do the same thing your patch is doing?

What do you think? I hope you have a great day!
Joshua