Date:   Fri, 3 Feb 2023 10:11:39 -0500
From:   Johannes Weiner <hannes@cmpxchg.org>
To:     Yosry Ahmed <yosryahmed@google.com>
Cc:     Dave Chinner <david@fromorbit.com>,
        Alexander Viro <viro@zeniv.linux.org.uk>,
        "Darrick J. Wong" <djwong@kernel.org>,
        Christoph Lameter <cl@linux.com>,
        David Rientjes <rientjes@google.com>,
        Joonsoo Kim <iamjoonsoo.kim@lge.com>,
        Vlastimil Babka <vbabka@suse.cz>,
        Roman Gushchin <roman.gushchin@linux.dev>,
        Hyeonggon Yoo <42.hyeyoo@gmail.com>,
        "Matthew Wilcox (Oracle)" <willy@infradead.org>,
        Miaohe Lin <linmiaohe@huawei.com>,
        David Hildenbrand <david@redhat.com>,
        Peter Xu <peterx@redhat.com>, NeilBrown <neilb@suse.de>,
        Shakeel Butt <shakeelb@google.com>,
        Michal Hocko <mhocko@kernel.org>,
        linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-xfs@vger.kernel.org, linux-mm@kvack.org
Subject: Re: [RFC PATCH v1 0/2] Ignore non-LRU-based reclaim in memcg reclaim
Message-ID: <Y90kK5jnxBbE9tV4@cmpxchg.org>
References: <20230202233229.3895713-1-yosryahmed@google.com>
 <20230203000057.GS360264@dread.disaster.area>
 <CAJD7tkazLFO8sc1Ly7+2_SGTxDq2XuPnvxxTnpQyXQELmq+m4A@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAJD7tkazLFO8sc1Ly7+2_SGTxDq2XuPnvxxTnpQyXQELmq+m4A@mail.gmail.com>
Precedence: bulk

On Thu, Feb 02, 2023 at 04:17:18PM -0800, Yosry Ahmed wrote:
> On Thu, Feb 2, 2023 at 4:01 PM Dave Chinner <david@fromorbit.com> wrote:
> > > Patch 1 is just refactoring updating reclaim_state into a helper
> > > function, and renames reclaimed_slab to just reclaimed, with a comment
> > > describing its true purpose.
> > >
> > > Patch 2 ignores pages reclaimed outside of LRU reclaim in memcg reclaim.
> > >
> > > The original draft was a little bit different. It also kept track of
> > > uncharged objcg pages, and reported them only in memcg reclaim and only
> > > if the uncharged memcg is in the subtree of the memcg under reclaim.
> > > This was an attempt to make reporting of memcg reclaim even more
> > > accurate, but was dropped due to questionable complexity vs benefit
> > > tradeoff. It can be revived if there is interest.
> > >
> > > Yosry Ahmed (2):
> > >   mm: vmscan: refactor updating reclaimed pages in reclaim_state
> > >   mm: vmscan: ignore non-LRU-based reclaim in memcg reclaim
> > >
> > >  fs/inode.c           |  3 +--
> >
> > Inodes and inode mapping pages are directly charged to the memcg
> > that allocated them and the shrinker is correctly marked as
> > SHRINKER_MEMCG_AWARE. Freeing the pages attached to the inode will
> > account them correctly to the related memcg, regardless of which
> > memcg is triggering the reclaim.  Hence I'm not sure that skipping
> > the accounting of the reclaimed memory is even correct in this case;
> 
> Please note that we are not skipping any accounting here. The pages
> are still uncharged from the memcgs they are charged to (the allocator
> memcgs as you pointed out). We just do not report them in the return
> value of try_to_free_mem_cgroup_pages(), to avoid over-reporting.

I was wondering the same thing as Dave, reading through this. But
you're right, we'll catch the accounting during uncharge. Can you
please add a comment on the !cgroup_reclaim() explaining this?

There is one wrinkle with this, though. We have the following
(simplified) sequence during charging:

	nr_reclaimed = try_to_free_mem_cgroup_pages(mem_over_limit, nr_pages,
						    gfp_mask, reclaim_options);

	if (mem_cgroup_margin(mem_over_limit) >= nr_pages)
		goto retry;

	/*
	 * Even though the limit is exceeded at this point, reclaim
	 * may have been able to free some pages.  Retry the charge
	 * before killing the task.
	 *
	 * Only for regular pages, though: huge pages are rather
	 * unlikely to succeed so close to the limit, and we fall back
	 * to regular pages anyway in case of failure.
	 */
	if (nr_reclaimed && nr_pages <= (1 << PAGE_ALLOC_COSTLY_ORDER))
		goto retry;

So in the unlikely scenario where the first call doesn't make the
necessary headroom, and the shrinkers are the only thing that made
forward progress, we would OOM prematurely.

Not that an OOM would seem that far away in that scenario, anyway. But I
remember long discussions with DavidR on probabilistic OOM regressions ;)

> > I think the code should still be accounting for all pages that
> > belong to the memcg being scanned that are reclaimed, not ignoring
> > them altogether...
> 
> 100% agree. Ideally I would want to:
> - For pruned inodes: report all freed pages for global reclaim, and
> only report pages charged to the memcg under reclaim for memcg
> reclaim.

This only happens on highmem systems at this point, as elsewhere
populated inodes aren't on the shrinker LRUs anymore. We'd probably be
ok with a comment noting the inaccuracy in the proactive reclaim stats
for the time being, until somebody actually cares about that combination.

> - For slab: report all freed pages for global reclaim, and only report
> uncharged objcg pages from the memcg under reclaim for memcg reclaim.
> 
> The only problem is that I thought people would think this is too much
> complexity and not worth it. If people agree this should be the
> approach to follow, I can prepare patches for this. I originally
> implemented this for slab pages, but held off on sending it.

I'd be curious to see the code!