Re: [PATCH v5] zswap: memcontrol: implement zswap writeback disabling

[Chris Li <chrisl@kernel.org>]

Hi Nhat,

On Sun, Nov 19, 2023 at 01:50:17PM -0800, Chris Li wrote:
> On Sun, Nov 19, 2023 at 11:08 AM Nhat Pham <nphamcs@gmail.com> wrote:
> > I don't have any major argument against this. It just seems a bit
> > heavyweight for what we need at the moment (only disabling
> > swap-to-disk usage).
> 
> The first milestone we just implement the reserved keywords without
> the custom swap tier list.
> That should be very similar to "zswap.writeback". Instead of writing 0
> to "zswap.writeback".
> You write "zswap" to "swap.tiers". Writing "none" will disable all
> swap. Writing "all" will allow all swap devices.
> I consider this conceptually cleaner than the "zswap.writeback" == 0
> will also disable other swap types behavior. "disabled zswap writeback
> == disable all swap" feels less natural.

I implement a minimal version of the "swap.tiers" to replace the "zswap.writeback".
It only implements the ABI level. Under the hook it is using the writeback bool.

This patch builds on top of your V5 patch.

implement memory.swap.tiers on top of memory.zswap.writeback.

"memory.swap.tiers" supports two key words for now:
all: all swap swap tiers are considered. (previously zswap.writback == 1)
zswap: only zswap tier are considered. (previously zswap.writeback == 0)

Index: linux/mm/memcontrol.c
===================================================================
--- linux.orig/mm/memcontrol.c
+++ linux/mm/memcontrol.c
@@ -7992,6 +7992,32 @@ static int swap_events_show(struct seq_f
 	return 0;
 }
 
+static int swap_tiers_show(struct seq_file *m, void *v)
+{
+	struct mem_cgroup *memcg = mem_cgroup_from_seq(m);
+
+	seq_printf(m, "%s\n", READ_ONCE(memcg->zswap_writeback) ? "all" : "zswap");
+	return 0;
+}
+
+static ssize_t swap_tiers_write(struct kernfs_open_file *of,
+				char *buf, size_t nbytes, loff_t off)
+{
+	struct mem_cgroup *memcg = mem_cgroup_from_css(of_css(of));
+	int zswap_writeback;
+
+	buf = strstrip(buf);
+	if (!strcmp(buf, "all"))
+		zswap_writeback = 1;
+	else if (!strcmp(buf, "zswap"))
+		zswap_writeback = 0;
+	else
+		return -EINVAL;
+
+	WRITE_ONCE(memcg->zswap_writeback, zswap_writeback);
+	return nbytes;
+}
+
 static struct cftype swap_files[] = {
 	{
 		.name = "swap.current",
@@ -8021,6 +8047,12 @@ static struct cftype swap_files[] = {
 		.file_offset = offsetof(struct mem_cgroup, swap_events_file),
 		.seq_show = swap_events_show,
 	},
+	{
+		.name = "swap.tiers",
+		.seq_show = swap_tiers_show,
+		.write = swap_tiers_write,
+	},
+
 	{ }	/* terminate */
 };
 
@@ -8183,31 +8215,6 @@ static ssize_t zswap_max_write(struct ke
 	return nbytes;
 }
 
-static int zswap_writeback_show(struct seq_file *m, void *v)
-{
-	struct mem_cgroup *memcg = mem_cgroup_from_seq(m);
-
-	seq_printf(m, "%d\n", READ_ONCE(memcg->zswap_writeback));
-	return 0;
-}
-
-static ssize_t zswap_writeback_write(struct kernfs_open_file *of,
-				char *buf, size_t nbytes, loff_t off)
-{
-	struct mem_cgroup *memcg = mem_cgroup_from_css(of_css(of));
-	int zswap_writeback;
-	ssize_t parse_ret = kstrtoint(strstrip(buf), 0, &zswap_writeback);
-
-	if (parse_ret)
-		return parse_ret;
-
-	if (zswap_writeback != 0 && zswap_writeback != 1)
-		return -EINVAL;
-
-	WRITE_ONCE(memcg->zswap_writeback, zswap_writeback);
-	return nbytes;
-}
-
 static struct cftype zswap_files[] = {
 	{
 		.name = "zswap.current",
@@ -8220,11 +8227,6 @@ static struct cftype zswap_files[] = {
 		.seq_show = zswap_max_show,
 		.write = zswap_max_write,
 	},
-	{
-		.name = "zswap.writeback",
-		.seq_show = zswap_writeback_show,
-		.write = zswap_writeback_write,
-	},
 	{ }	/* terminate */
 };
 #endif /* CONFIG_MEMCG_KMEM && CONFIG_ZSWAP */