linux-kernel - Re: [PATCH] mm/swap, workingset: make anon shadow nodes memcg aware

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <az5fhwmdwxpsgsxb7j3ruzbjevmxqrmx5xswwzuazh6zq5ytrs@bmgbu44mz3po>
Date: Tue, 20 Aug 2024 17:21:48 -0700
From: Shakeel Butt <shakeel.butt@...ux.dev>
To: Kairui Song <kasong@...cent.com>
Cc: linux-mm@...ck.org, Andrew Morton <akpm@...ux-foundation.org>, 
	Matthew Wilcox <willy@...radead.org>, Johannes Weiner <hannes@...xchg.org>, 
	Roman Gushchin <roman.gushchin@...ux.dev>, Nhat Pham <nphamcs@...il.com>, Michal Hocko <mhocko@...e.com>, 
	Chengming Zhou <zhouchengming@...edance.com>, Muchun Song <muchun.song@...ux.dev>, 
	Chris Li <chrisl@...nel.org>, Yosry Ahmed <yosryahmed@...gle.com>, 
	"Huang, Ying" <ying.huang@...el.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mm/swap, workingset: make anon shadow nodes memcg aware

On Tue, Aug 20, 2024 at 05:23:59PM GMT, Kairui Song wrote:
> From: Kairui Song <kasong@...cent.com>
> 
> Currently, the workingset (shadow) nodes of the swap cache are not
> accounted to their corresponding memory cgroup, instead, they are
> all accounted to the root cgroup. This leads to inaccurate accounting
> and ineffective reclaiming. One cgroup could swap out a large amount
> of memory, take up a large amount of memory with shadow nodes without
> being accounted.
> 
> This issue is similar to commit 7b785645e8f1 ("mm: fix page cache
> convergence regression"), where page cache shadow nodes were incorrectly
> accounted. That was due to the accidental dropping of the accounting
> flag during the XArray conversion in commit a28334862993
> ("page cache: Finish XArray conversion").
> 
> However, this fix has a different cause. Swap cache shadow nodes were
> never accounted even before the XArray conversion, since they did not
> exist until commit 3852f6768ede ("mm/swapcache: support to handle the
> shadow entries"), which was years after the XArray conversion.
> 
> It's worth noting that one anon shadow Xarray node may contain
> different entries from different cgroup, and it gets accounted at reclaim
> time, so it's arguable which cgroup it should be accounted to (as
> Shakeal Butt pointed out [1]). File pages may suffer similar issue
> but less common. Things like proactive memory reclaim could make thing
> more complex.
> 
> So this commit still can't provide a 100% accurate accounting of anon
> shadows, but it covers the cases when one memory cgroup uses significant
> amount of swap, and in most cases memory pressure in one cgroup only
> suppose to reclaim this cgroup and children. Besides, this fix is clean and
> easy enough.
> 
> Link: https://lore.kernel.org/all/7gzevefivueqtebzvikzbucnrnpurmh3scmfuiuo2tnrs37xso@haj7gzepjur2/ [1]
> Signed-off-by: Kairui Song <kasong@...cent.com>
> 

Is this a real issue? Have you seen systems in the production with
large amount of memory occupied by anon shadow entries? This is still
limited to the amount of swap a cgroup is allowed to use.

The reason I am asking is that this solution is worse than the perceived
problem at least to me. With this patch, the kernel will be charging
unrelated cgroups for the memory of swap xarray nodes during global
reclaim and proactive reclaim.

You can reduce this weirdness by using set_active_memcg() in
add_to_swap_cache() using the given folio's memcg but still you have the
case of multiple unrelated folios and shadow entries of different
cgroups within the same node. For filesystem case, the userspace can
control which files are shared between different cgroups and has more
control on it. That is not the case for swap space.

thanks,
Shakeel