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

[Nhat Pham]

On Sun, Nov 19, 2023 at 6:41 PM Chris Li <chrisl@xxxxxxxxxx> wrote:
>
> 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@xxxxxxxxx> 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 */

Hi Chris!

Thanks for the patch. Would you mind if I spend some time staring
at the suggestion again and testing it some more?

If everything is good, I'll squash this patch with the original version,
(keeping you as a co-developer of the final patch of course), and
update the documentation before re-sending everything as v6.

Anyway, have a nice Thanksgiving break everyone! Thanks for
taking the time to review my patch and discuss the API with me!