Received-SPF: pass (google.com: domain of linux-kernel+bounces-23670-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Thu, 11 Jan 2024 08:41:25 -0500
From: Johannes Weiner <hannes@cmpxchg.org>
To: Zhiguo Jiang <justinjiang@vivo.com>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	David Hildenbrand <david@redhat.com>,
	Matthew Wilcox <willy@infradead.org>, Chris Li <chrisl@kernel.org>,
	Michal Hocko <mhocko@suse.com>,
	Kefeng Wang <wangkefeng.wang@huawei.com>,
	Dan Schatzberg <schatzberg.dan@gmail.com>,
	Yosry Ahmed <yosryahmed@google.com>, Yue Zhao <findns94@gmail.com>,
	Hugh Dickins <hughd@google.com>, linux-kernel@vger.kernel.org,
	linux-mm@kvack.org, opensource.kernel@vivo.com
Subject: Re: [PATCH v1 1/2] mm:vmscan: fix workingset eviction memcg issue
Message-ID: <20240111134125.GA390292@cmpxchg.org>
References: <20240111122451.682-1-justinjiang@vivo.com>
 <20240111122451.682-2-justinjiang@vivo.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240111122451.682-2-justinjiang@vivo.com>

On Thu, Jan 11, 2024 at 08:24:50PM +0800, Zhiguo Jiang wrote:
> The parameter of target_memcg is NULL in workingset_eviction(), and
> the lruvec obtained by mem_cgroup_lruvec(target_memcg, pgdat) is always
> root_mem_cgroup->lruvec, rather than the lruvec of mem_cgroup where
> folio is actually located.

WTF? No!

	/*
	 * The memory cgroup that hit its limit and as a result is the
	 * primary target of this reclaim invocation.
	 */
	struct mem_cgroup *target_mem_cgroup;

The cgroup that is stored in the eviction cookie is the one whose
limit triggered the reclaim cycle. This is often several levels above
the cgroups that own the pages. Subsequent refaults need to be
evaluated at the eviction level:

	/*
	 * The activation decision for this folio is made at the level
	 * where the eviction occurred, as that is where the LRU order
	 * during folio reclaim is being determined.
	 *
	 * However, the cgroup that will own the folio is the one that
	 * is actually experiencing the refault event.
	 */

> Fix target_memcg to the memcg obtained by folio_memcg(folio).
> 
> Signed-off-by: Zhiguo Jiang <justinjiang@vivo.com>

Nacked-by: Johannes Weiner <hannes@cmpxchg.org>

Please take more time to read into the code you're proposing to
change. You made it sound like a trivial simplification, but this
totally screws up aging and pressure detection in containers.