linux-kernel - [PATCH v3 1/3] mm/memcontrol: respect zswap.writeback setting from parent cg too

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-next>] [day] [month] [year] [list]

Message-ID: <20240823162506.12117-1-me@yhndnzj.com>
Date: Fri, 23 Aug 2024 16:27:06 +0000
From: Mike Yuan <me@...dnzj.com>
To: linux-kernel@...r.kernel.org
Cc: Mike Yuan <me@...dnzj.com>, linux-mm@...ck.org, cgroups@...r.kernel.org, Nhat Pham <nphamcs@...il.com>, Yosry Ahmed <yosryahmed@...gle.com>, Johannes Weiner <hannes@...xchg.org>, Andrew Morton <akpm@...ux-foundation.org>, Muchun Song <muchun.song@...ux.dev>, Shakeel Butt <shakeel.butt@...ux.dev>, Roman Gushchin <roman.gushchin@...ux.dev>, Michal Hocko <mhocko@...nel.org>, Michal Koutný <mkoutny@...e.com>, stable@...r.kernel.org
Subject: [PATCH v3 1/3] mm/memcontrol: respect zswap.writeback setting from parent cg too

Currently, the behavior of zswap.writeback wrt.
the cgroup hierarchy seems a bit odd. Unlike zswap.max,
it doesn't honor the value from parent cgroups. This
surfaced when people tried to globally disable zswap writeback,
i.e. reserve physical swap space only for hibernation [1] -
disabling zswap.writeback only for the root cgroup results
in subcgroups with zswap.writeback=1 still performing writeback.

The inconsistency became more noticeable after I introduced
the MemoryZSwapWriteback= systemd unit setting [2] for
controlling the knob. The patch assumed that the kernel would
enforce the value of parent cgroups. It could probably be
workarounded from systemd's side, by going up the slice unit
tree and inheriting the value. Yet I think it's more sensible
to make it behave consistently with zswap.max and friends.

[1] https://wiki.archlinux.org/title/Power_management/Suspend_and_hibernate#Disable_zswap_writeback_to_use_the_swap_space_only_for_hibernation
[2] https://github.com/systemd/systemd/pull/31734

Changes in v3:
- Additionally drop inheritance of zswap.writeback setting
  on cgroup creation, which is no longer needed
Link to v2: https://lore.kernel.org/linux-kernel/20240816144344.18135-1-me@yhndnzj.com/

Changes in v2:
- Actually base on latest tree (is_zswap_enabled() -> zswap_is_enabled())
- Update Documentation/admin-guide/cgroup-v2.rst to reflect the change
Link to v1: https://lore.kernel.org/linux-kernel/20240814171800.23558-1-me@yhndnzj.com/

Cc: Nhat Pham <nphamcs@...il.com>
Cc: Yosry Ahmed <yosryahmed@...gle.com>
Cc: Johannes Weiner <hannes@...xchg.org>
Cc: Andrew Morton <akpm@...ux-foundation.org>
Cc: Michal Koutný <mkoutny@...e.com>

Fixes: 501a06fe8e4c ("zswap: memcontrol: implement zswap writeback disabling")
Cc: <stable@...r.kernel.org>

Signed-off-by: Mike Yuan <me@...dnzj.com>
Reviewed-by: Nhat Pham <nphamcs@...il.com>
Acked-by: Yosry Ahmed <yosryahmed@...gle.com>
---
 Documentation/admin-guide/cgroup-v2.rst |  7 ++++---
 mm/memcontrol.c                         | 12 +++++++++---
 2 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst
index 86311c2907cd..95c18bc17083 100644
--- a/Documentation/admin-guide/cgroup-v2.rst
+++ b/Documentation/admin-guide/cgroup-v2.rst
@@ -1717,9 +1717,10 @@ The following nested keys are defined.
 	entries fault back in or are written out to disk.
 
   memory.zswap.writeback
-	A read-write single value file. The default value is "1". The
-	initial value of the root cgroup is 1, and when a new cgroup is
-	created, it inherits the current value of its parent.
+	A read-write single value file. The default value is "1".
+	Note that this setting is hierarchical, i.e. the writeback would be
+	implicitly disabled for child cgroups if the upper hierarchy
+	does so.
 
 	When this is set to 0, all swapping attempts to swapping devices
 	are disabled. This included both zswap writebacks, and swapping due
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index f29157288b7d..d563fb515766 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -3613,8 +3613,7 @@ mem_cgroup_css_alloc(struct cgroup_subsys_state *parent_css)
 	memcg1_soft_limit_reset(memcg);
 #ifdef CONFIG_ZSWAP
 	memcg->zswap_max = PAGE_COUNTER_MAX;
-	WRITE_ONCE(memcg->zswap_writeback,
-		!parent || READ_ONCE(parent->zswap_writeback));
+	WRITE_ONCE(memcg->zswap_writeback, true);
 #endif
 	page_counter_set_high(&memcg->swap, PAGE_COUNTER_MAX);
 	if (parent) {
@@ -5320,7 +5319,14 @@ void obj_cgroup_uncharge_zswap(struct obj_cgroup *objcg, size_t size)
 bool mem_cgroup_zswap_writeback_enabled(struct mem_cgroup *memcg)
 {
 	/* if zswap is disabled, do not block pages going to the swapping device */
-	return !zswap_is_enabled() || !memcg || READ_ONCE(memcg->zswap_writeback);
+	if (!zswap_is_enabled())
+		return true;
+
+	for (; memcg; memcg = parent_mem_cgroup(memcg))
+		if (!READ_ONCE(memcg->zswap_writeback))
+			return false;
+
+	return true;
 }
 
 static u64 zswap_current_read(struct cgroup_subsys_state *css,

base-commit: 47ac09b91befbb6a235ab620c32af719f8208399
-- 
2.46.0