[v5] zswap: memcontrol: implement zswap writeback disabling

  During our experiment with zswap, we sometimes observe swap IOs due to
occasional zswap store failures and writebacks-to-swap. These swapping
IOs prevent many users who cannot tolerate swapping from adopting zswap
to save memory and improve performance where possible.

This patch adds the option to disable this behavior entirely: do not
writeback to backing swapping device when a zswap store attempt fail,
and do not write pages in the zswap pool back to the backing swap
device (both when the pool is full, and when the new zswap shrinker is
called).

This new behavior can be opted-in/out on a per-cgroup basis via a new
cgroup file. By default, writebacks to swap device is enabled, which is
the previous behavior. Initially, writeback is enabled for the root
cgroup, and a newly created cgroup will inherit the current setting of
its parent.

Note that this is subtly different from setting memory.swap.max to 0, as
it still allows for pages to be stored in the zswap pool (which itself
consumes swap space in its current form).

This patch should be applied on top of the zswap shrinker series:

https://lore.kernel.org/lkml/20231106183159.3562879-1-nphamcs@gmail.com/

as it also disables the zswap shrinker, a major source of zswap
writebacks.

Suggested-by: Johannes Weiner <hannes@cmpxchg.org>
Signed-off-by: Nhat Pham <nphamcs@gmail.com>
---
 Documentation/admin-guide/cgroup-v2.rst | 12 ++++++++
 Documentation/admin-guide/mm/zswap.rst  |  6 ++++
 include/linux/memcontrol.h              | 12 ++++++++
 include/linux/zswap.h                   |  6 ++++
 mm/memcontrol.c                         | 38 +++++++++++++++++++++++++
 mm/page_io.c                            |  6 ++++
 mm/shmem.c                              |  3 +-
 mm/zswap.c                              | 14 +++++++++
 8 files changed, 95 insertions(+), 2 deletions(-)
  

Hi Nhat,

As we discussed, I just want to bounce some alternative ideas related
to this write back disabled feature.

Currently, the common usage case is zswap alone or zswap + SSD. We
treat zswap and
SSD as two different tiers with different swap in performance characteristics.
If we make it more generic, we can also have more than two swap tiers.
e.g. we can have zswap, SSD, network swap or HDD swap.
The disable flag is just one bit of information, it can't describe
what is the next tier.if it is not the current swap file
implementation .

One idea is that we can have a more general swap_tier_list object to
describe the order of the swap device, The system can have more than
one such list to describe different combinations of the tier
selection.

Each memcg can have a pointer point to one of such swap_tier_list
objects, replacing the disabled write back flag in this patch.
When you swap out, it will just go through each tier in the list, try
to swap it out.
 This has some implications for the zswap shrink as well. It becomes a
more generic "move swap out data to another tier". Generally need to
load into the swap cache then write to another tier.

Open question is how we deal with the swap cache index across
different tiers. The zswap.writeback_disable will not be needed if we
have a more generic swap tier framework.

Chris


On Wed, Nov 15, 2023 at 9:24 AM Nhat Pham <nphamcs@gmail.com> wrote:
>
> During our experiment with zswap, we sometimes observe swap IOs due to
> occasional zswap store failures and writebacks-to-swap. These swapping
> IOs prevent many users who cannot tolerate swapping from adopting zswap
> to save memory and improve performance where possible.
>
> This patch adds the option to disable this behavior entirely: do not
> writeback to backing swapping device when a zswap store attempt fail,
> and do not write pages in the zswap pool back to the backing swap
> device (both when the pool is full, and when the new zswap shrinker is
> called).
>
> This new behavior can be opted-in/out on a per-cgroup basis via a new
> cgroup file. By default, writebacks to swap device is enabled, which is
> the previous behavior. Initially, writeback is enabled for the root
> cgroup, and a newly created cgroup will inherit the current setting of
> its parent.
>
> Note that this is subtly different from setting memory.swap.max to 0, as
> it still allows for pages to be stored in the zswap pool (which itself
> consumes swap space in its current form).
>
> This patch should be applied on top of the zswap shrinker series:
>
> https://lore.kernel.org/lkml/20231106183159.3562879-1-nphamcs@gmail.com/
>
> as it also disables the zswap shrinker, a major source of zswap
> writebacks.
>
> Suggested-by: Johannes Weiner <hannes@cmpxchg.org>
> Signed-off-by: Nhat Pham <nphamcs@gmail.com>
> ---
>  Documentation/admin-guide/cgroup-v2.rst | 12 ++++++++
>  Documentation/admin-guide/mm/zswap.rst  |  6 ++++
>  include/linux/memcontrol.h              | 12 ++++++++
>  include/linux/zswap.h                   |  6 ++++
>  mm/memcontrol.c                         | 38 +++++++++++++++++++++++++
>  mm/page_io.c                            |  6 ++++
>  mm/shmem.c                              |  3 +-
>  mm/zswap.c                              | 14 +++++++++
>  8 files changed, 95 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst
> index 3f85254f3cef..2b4ac43efdc8 100644
> --- a/Documentation/admin-guide/cgroup-v2.rst
> +++ b/Documentation/admin-guide/cgroup-v2.rst
> @@ -1679,6 +1679,18 @@ PAGE_SIZE multiple when read back.
>         limit, it will refuse to take any more stores before existing
>         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.
> +
> +       When this is set to 0, all swapping attempts to swapping devices
> +       are disabled. This included both zswap writebacks, and swapping due
> +       to zswap store failure.
> +
> +       Note that this is subtly different from setting memory.swap.max to
> +       0, as it still allows for pages to be written to the zswap pool.
> +
>    memory.pressure
>         A read-only nested-keyed file.
>
> diff --git a/Documentation/admin-guide/mm/zswap.rst b/Documentation/admin-guide/mm/zswap.rst
> index 522ae22ccb84..b987e58edb70 100644
> --- a/Documentation/admin-guide/mm/zswap.rst
> +++ b/Documentation/admin-guide/mm/zswap.rst
> @@ -153,6 +153,12 @@ attribute, e. g.::
>
>  Setting this parameter to 100 will disable the hysteresis.
>
> +Some users cannot tolerate the swapping that comes with zswap store failures
> +and zswap writebacks. Swapping can be disabled entirely (without disabling
> +zswap itself) on a cgroup-basis as follows:
> +
> +       echo 0 > /sys/fs/cgroup/<cgroup-name>/memory.zswap.writeback
> +
>  When there is a sizable amount of cold memory residing in the zswap pool, it
>  can be advantageous to proactively write these cold pages to swap and reclaim
>  the memory for other use cases. By default, the zswap shrinker is disabled.
> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> index 83590fd0d6d1..3901ff4dae63 100644
> --- a/include/linux/memcontrol.h
> +++ b/include/linux/memcontrol.h
> @@ -219,6 +219,12 @@ struct mem_cgroup {
>
>  #if defined(CONFIG_MEMCG_KMEM) && defined(CONFIG_ZSWAP)
>         unsigned long zswap_max;
> +
> +       /*
> +        * Prevent pages from this memcg from being written back from zswap to
> +        * swap, and from being swapped out on zswap store failures.
> +        */
> +       bool zswap_writeback;
>  #endif
>
>         unsigned long soft_limit;
> @@ -1931,6 +1937,7 @@ static inline void count_objcg_event(struct obj_cgroup *objcg,
>  bool obj_cgroup_may_zswap(struct obj_cgroup *objcg);
>  void obj_cgroup_charge_zswap(struct obj_cgroup *objcg, size_t size);
>  void obj_cgroup_uncharge_zswap(struct obj_cgroup *objcg, size_t size);
> +bool mem_cgroup_zswap_writeback_enabled(struct mem_cgroup *memcg);
>  #else
>  static inline bool obj_cgroup_may_zswap(struct obj_cgroup *objcg)
>  {
> @@ -1944,6 +1951,11 @@ static inline void obj_cgroup_uncharge_zswap(struct obj_cgroup *objcg,
>                                              size_t size)
>  {
>  }
> +static inline bool mem_cgroup_zswap_writeback_enabled(struct mem_cgroup *memcg)
> +{
> +       /* if zswap is disabled, do not block pages going to the swapping device */
> +       return true;
> +}
>  #endif
>
>  #endif /* _LINUX_MEMCONTROL_H */
> diff --git a/include/linux/zswap.h b/include/linux/zswap.h
> index cbd373ba88d2..b4997e27a74b 100644
> --- a/include/linux/zswap.h
> +++ b/include/linux/zswap.h
> @@ -35,6 +35,7 @@ void zswap_swapoff(int type);
>  void zswap_memcg_offline_cleanup(struct mem_cgroup *memcg);
>  void zswap_lruvec_state_init(struct lruvec *lruvec);
>  void zswap_lruvec_swapin(struct page *page);
> +bool is_zswap_enabled(void);
>  #else
>
>  struct zswap_lruvec_state {};
> @@ -55,6 +56,11 @@ static inline void zswap_swapoff(int type) {}
>  static inline void zswap_memcg_offline_cleanup(struct mem_cgroup *memcg) {}
>  static inline void zswap_lruvec_init(struct lruvec *lruvec) {}
>  static inline void zswap_lruvec_swapin(struct page *page) {}
> +
> +static inline bool is_zswap_enabled(void)
> +{
> +       return false;
> +}
>  #endif
>
>  #endif /* _LINUX_ZSWAP_H */
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 786c7edf5836..5ad71ce31c74 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -5522,6 +5522,8 @@ mem_cgroup_css_alloc(struct cgroup_subsys_state *parent_css)
>         WRITE_ONCE(memcg->soft_limit, PAGE_COUNTER_MAX);
>  #if defined(CONFIG_MEMCG_KMEM) && defined(CONFIG_ZSWAP)
>         memcg->zswap_max = PAGE_COUNTER_MAX;
> +       WRITE_ONCE(memcg->zswap_writeback,
> +               !parent || READ_ONCE(parent->zswap_writeback));
>  #endif
>         page_counter_set_high(&memcg->swap, PAGE_COUNTER_MAX);
>         if (parent) {
> @@ -8146,6 +8148,12 @@ void obj_cgroup_uncharge_zswap(struct obj_cgroup *objcg, size_t size)
>         rcu_read_unlock();
>  }
>
> +bool mem_cgroup_zswap_writeback_enabled(struct mem_cgroup *memcg)
> +{
> +       /* if zswap is disabled, do not block pages going to the swapping device */
> +       return !is_zswap_enabled() || !memcg || READ_ONCE(memcg->zswap_writeback);
> +}
> +
>  static u64 zswap_current_read(struct cgroup_subsys_state *css,
>                               struct cftype *cft)
>  {
> @@ -8176,6 +8184,31 @@ static ssize_t zswap_max_write(struct kernfs_open_file *of,
>         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",
> @@ -8188,6 +8221,11 @@ 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 */
> diff --git a/mm/page_io.c b/mm/page_io.c
> index cb559ae324c6..5e606f1aa2f6 100644
> --- a/mm/page_io.c
> +++ b/mm/page_io.c
> @@ -201,6 +201,12 @@ int swap_writepage(struct page *page, struct writeback_control *wbc)
>                 folio_end_writeback(folio);
>                 return 0;
>         }
> +
> +       if (!mem_cgroup_zswap_writeback_enabled(folio_memcg(folio))) {
> +               folio_mark_dirty(folio);
> +               return AOP_WRITEPAGE_ACTIVATE;
> +       }
> +
>         __swap_writepage(&folio->page, wbc);
>         return 0;
>  }
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 0d1ce70bce38..ccbaaa5f1c16 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -1514,8 +1514,7 @@ static int shmem_writepage(struct page *page, struct writeback_control *wbc)
>
>                 mutex_unlock(&shmem_swaplist_mutex);
>                 BUG_ON(folio_mapped(folio));
> -               swap_writepage(&folio->page, wbc);
> -               return 0;
> +               return swap_writepage(&folio->page, wbc);
>         }
>
>         mutex_unlock(&shmem_swaplist_mutex);
> diff --git a/mm/zswap.c b/mm/zswap.c
> index 943090dfe793..caa467e40009 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -152,6 +152,11 @@ module_param_named(exclusive_loads, zswap_exclusive_loads_enabled, bool, 0644);
>  static bool zswap_shrinker_enabled;
>  module_param_named(shrinker_enabled, zswap_shrinker_enabled, bool, 0644);
>
> +bool is_zswap_enabled(void)
> +{
> +       return zswap_enabled;
> +}
> +
>  /*********************************
>  * data structures
>  **********************************/
> @@ -589,6 +594,9 @@ static unsigned long zswap_shrinker_scan(struct shrinker *shrinker,
>         struct zswap_pool *pool = shrinker->private_data;
>         bool encountered_page_in_swapcache = false;
>
> +       if (!mem_cgroup_zswap_writeback_enabled(sc->memcg))
> +               return SHRINK_STOP;
> +
>         nr_protected =
>                 atomic_long_read(&lruvec->zswap_lruvec_state.nr_zswap_protected);
>         lru_size = list_lru_shrink_count(&pool->list_lru, sc);
> @@ -619,6 +627,9 @@ static unsigned long zswap_shrinker_count(struct shrinker *shrinker,
>         struct lruvec *lruvec = mem_cgroup_lruvec(memcg, NODE_DATA(sc->nid));
>         unsigned long nr_backing, nr_stored, nr_freeable, nr_protected;
>
> +       if (!mem_cgroup_zswap_writeback_enabled(memcg))
> +               return 0;
> +
>  #ifdef CONFIG_MEMCG_KMEM
>         cgroup_rstat_flush(memcg->css.cgroup);
>         nr_backing = memcg_page_state(memcg, MEMCG_ZSWAP_B) >> PAGE_SHIFT;
> @@ -934,6 +945,9 @@ static int shrink_memcg(struct mem_cgroup *memcg)
>         struct zswap_pool *pool;
>         int nid, shrunk = 0;
>
> +       if (!mem_cgroup_zswap_writeback_enabled(memcg))
> +               return -EINVAL;
> +
>         /*
>          * Skip zombies because their LRUs are reparented and we would be
>          * reclaiming from the parent instead of the dead memcg.
> --
> 2.34.1
  

On Thu, Nov 16, 2023 at 12:53 PM Chris Li <chrisl@kernel.org> wrote:
>
> Hi Nhat,
>
> As we discussed, I just want to bounce some alternative ideas related
> to this write back disabled feature.
>
> Currently, the common usage case is zswap alone or zswap + SSD. We
> treat zswap and
> SSD as two different tiers with different swap in performance characteristics.
> If we make it more generic, we can also have more than two swap tiers.
> e.g. we can have zswap, SSD, network swap or HDD swap.
> The disable flag is just one bit of information, it can't describe
> what is the next tier.if it is not the current swap file
> implementation .
>
> One idea is that we can have a more general swap_tier_list object to
> describe the order of the swap device, The system can have more than
> one such list to describe different combinations of the tier
> selection.
>
> Each memcg can have a pointer point to one of such swap_tier_list
> objects, replacing the disabled write back flag in this patch.
> When you swap out, it will just go through each tier in the list, try
> to swap it out.
>  This has some implications for the zswap shrink as well. It becomes a
> more generic "move swap out data to another tier". Generally need to
> load into the swap cache then write to another tier.
>
> Open question is how we deal with the swap cache index across
> different tiers. The zswap.writeback_disable will not be needed if we
> have a more generic swap tier framework.

I agree that this should be the long-term goal, and I suggested that
we make the interface more future-proof by making it more generic to
accept the types or tiers of swap allowed by the memcg:

https://lore.kernel.org/lkml/CAJD7tkY8iPBo99+1gdsSRMNDu4jkVKz8rb=W+xk9=GE0y=kSuw@mail.gmail.com/

Since we only have swap and zswap now, the implementation can be
similar to this code and basically just disable writeback if zswap is
the only allowed swapping mechanism. So we don't necessarily need to
have a full swap tiering implementation, but I agree with Chris that
at least having a future-proof interface to work with generic swap
tiering is preferrable.



>
> Chris
>
>
> On Wed, Nov 15, 2023 at 9:24 AM Nhat Pham <nphamcs@gmail.com> wrote:
> >
> > During our experiment with zswap, we sometimes observe swap IOs due to
> > occasional zswap store failures and writebacks-to-swap. These swapping
> > IOs prevent many users who cannot tolerate swapping from adopting zswap
> > to save memory and improve performance where possible.
> >
> > This patch adds the option to disable this behavior entirely: do not
> > writeback to backing swapping device when a zswap store attempt fail,
> > and do not write pages in the zswap pool back to the backing swap
> > device (both when the pool is full, and when the new zswap shrinker is
> > called).
> >
> > This new behavior can be opted-in/out on a per-cgroup basis via a new
> > cgroup file. By default, writebacks to swap device is enabled, which is
> > the previous behavior. Initially, writeback is enabled for the root
> > cgroup, and a newly created cgroup will inherit the current setting of
> > its parent.
> >
> > Note that this is subtly different from setting memory.swap.max to 0, as
> > it still allows for pages to be stored in the zswap pool (which itself
> > consumes swap space in its current form).
> >
> > This patch should be applied on top of the zswap shrinker series:
> >
> > https://lore.kernel.org/lkml/20231106183159.3562879-1-nphamcs@gmail.com/
> >
> > as it also disables the zswap shrinker, a major source of zswap
> > writebacks.
> >
> > Suggested-by: Johannes Weiner <hannes@cmpxchg.org>
> > Signed-off-by: Nhat Pham <nphamcs@gmail.com>
> > ---
> >  Documentation/admin-guide/cgroup-v2.rst | 12 ++++++++
> >  Documentation/admin-guide/mm/zswap.rst  |  6 ++++
> >  include/linux/memcontrol.h              | 12 ++++++++
> >  include/linux/zswap.h                   |  6 ++++
> >  mm/memcontrol.c                         | 38 +++++++++++++++++++++++++
> >  mm/page_io.c                            |  6 ++++
> >  mm/shmem.c                              |  3 +-
> >  mm/zswap.c                              | 14 +++++++++
> >  8 files changed, 95 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst
> > index 3f85254f3cef..2b4ac43efdc8 100644
> > --- a/Documentation/admin-guide/cgroup-v2.rst
> > +++ b/Documentation/admin-guide/cgroup-v2.rst
> > @@ -1679,6 +1679,18 @@ PAGE_SIZE multiple when read back.
> >         limit, it will refuse to take any more stores before existing
> >         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.
> > +
> > +       When this is set to 0, all swapping attempts to swapping devices
> > +       are disabled. This included both zswap writebacks, and swapping due
> > +       to zswap store failure.
> > +
> > +       Note that this is subtly different from setting memory.swap.max to
> > +       0, as it still allows for pages to be written to the zswap pool.
> > +
> >    memory.pressure
> >         A read-only nested-keyed file.
> >
> > diff --git a/Documentation/admin-guide/mm/zswap.rst b/Documentation/admin-guide/mm/zswap.rst
> > index 522ae22ccb84..b987e58edb70 100644
> > --- a/Documentation/admin-guide/mm/zswap.rst
> > +++ b/Documentation/admin-guide/mm/zswap.rst
> > @@ -153,6 +153,12 @@ attribute, e. g.::
> >
> >  Setting this parameter to 100 will disable the hysteresis.
> >
> > +Some users cannot tolerate the swapping that comes with zswap store failures
> > +and zswap writebacks. Swapping can be disabled entirely (without disabling
> > +zswap itself) on a cgroup-basis as follows:
> > +
> > +       echo 0 > /sys/fs/cgroup/<cgroup-name>/memory.zswap.writeback
> > +
> >  When there is a sizable amount of cold memory residing in the zswap pool, it
> >  can be advantageous to proactively write these cold pages to swap and reclaim
> >  the memory for other use cases. By default, the zswap shrinker is disabled.
> > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> > index 83590fd0d6d1..3901ff4dae63 100644
> > --- a/include/linux/memcontrol.h
> > +++ b/include/linux/memcontrol.h
> > @@ -219,6 +219,12 @@ struct mem_cgroup {
> >
> >  #if defined(CONFIG_MEMCG_KMEM) && defined(CONFIG_ZSWAP)
> >         unsigned long zswap_max;
> > +
> > +       /*
> > +        * Prevent pages from this memcg from being written back from zswap to
> > +        * swap, and from being swapped out on zswap store failures.
> > +        */
> > +       bool zswap_writeback;
> >  #endif
> >
> >         unsigned long soft_limit;
> > @@ -1931,6 +1937,7 @@ static inline void count_objcg_event(struct obj_cgroup *objcg,
> >  bool obj_cgroup_may_zswap(struct obj_cgroup *objcg);
> >  void obj_cgroup_charge_zswap(struct obj_cgroup *objcg, size_t size);
> >  void obj_cgroup_uncharge_zswap(struct obj_cgroup *objcg, size_t size);
> > +bool mem_cgroup_zswap_writeback_enabled(struct mem_cgroup *memcg);
> >  #else
> >  static inline bool obj_cgroup_may_zswap(struct obj_cgroup *objcg)
> >  {
> > @@ -1944,6 +1951,11 @@ static inline void obj_cgroup_uncharge_zswap(struct obj_cgroup *objcg,
> >                                              size_t size)
> >  {
> >  }
> > +static inline bool mem_cgroup_zswap_writeback_enabled(struct mem_cgroup *memcg)
> > +{
> > +       /* if zswap is disabled, do not block pages going to the swapping device */
> > +       return true;
> > +}
> >  #endif
> >
> >  #endif /* _LINUX_MEMCONTROL_H */
> > diff --git a/include/linux/zswap.h b/include/linux/zswap.h
> > index cbd373ba88d2..b4997e27a74b 100644
> > --- a/include/linux/zswap.h
> > +++ b/include/linux/zswap.h
> > @@ -35,6 +35,7 @@ void zswap_swapoff(int type);
> >  void zswap_memcg_offline_cleanup(struct mem_cgroup *memcg);
> >  void zswap_lruvec_state_init(struct lruvec *lruvec);
> >  void zswap_lruvec_swapin(struct page *page);
> > +bool is_zswap_enabled(void);
> >  #else
> >
> >  struct zswap_lruvec_state {};
> > @@ -55,6 +56,11 @@ static inline void zswap_swapoff(int type) {}
> >  static inline void zswap_memcg_offline_cleanup(struct mem_cgroup *memcg) {}
> >  static inline void zswap_lruvec_init(struct lruvec *lruvec) {}
> >  static inline void zswap_lruvec_swapin(struct page *page) {}
> > +
> > +static inline bool is_zswap_enabled(void)
> > +{
> > +       return false;
> > +}
> >  #endif
> >
> >  #endif /* _LINUX_ZSWAP_H */
> > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > index 786c7edf5836..5ad71ce31c74 100644
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> > @@ -5522,6 +5522,8 @@ mem_cgroup_css_alloc(struct cgroup_subsys_state *parent_css)
> >         WRITE_ONCE(memcg->soft_limit, PAGE_COUNTER_MAX);
> >  #if defined(CONFIG_MEMCG_KMEM) && defined(CONFIG_ZSWAP)
> >         memcg->zswap_max = PAGE_COUNTER_MAX;
> > +       WRITE_ONCE(memcg->zswap_writeback,
> > +               !parent || READ_ONCE(parent->zswap_writeback));
> >  #endif
> >         page_counter_set_high(&memcg->swap, PAGE_COUNTER_MAX);
> >         if (parent) {
> > @@ -8146,6 +8148,12 @@ void obj_cgroup_uncharge_zswap(struct obj_cgroup *objcg, size_t size)
> >         rcu_read_unlock();
> >  }
> >
> > +bool mem_cgroup_zswap_writeback_enabled(struct mem_cgroup *memcg)
> > +{
> > +       /* if zswap is disabled, do not block pages going to the swapping device */
> > +       return !is_zswap_enabled() || !memcg || READ_ONCE(memcg->zswap_writeback);
> > +}
> > +
> >  static u64 zswap_current_read(struct cgroup_subsys_state *css,
> >                               struct cftype *cft)
> >  {
> > @@ -8176,6 +8184,31 @@ static ssize_t zswap_max_write(struct kernfs_open_file *of,
> >         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",
> > @@ -8188,6 +8221,11 @@ 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 */
> > diff --git a/mm/page_io.c b/mm/page_io.c
> > index cb559ae324c6..5e606f1aa2f6 100644
> > --- a/mm/page_io.c
> > +++ b/mm/page_io.c
> > @@ -201,6 +201,12 @@ int swap_writepage(struct page *page, struct writeback_control *wbc)
> >                 folio_end_writeback(folio);
> >                 return 0;
> >         }
> > +
> > +       if (!mem_cgroup_zswap_writeback_enabled(folio_memcg(folio))) {
> > +               folio_mark_dirty(folio);
> > +               return AOP_WRITEPAGE_ACTIVATE;
> > +       }
> > +
> >         __swap_writepage(&folio->page, wbc);
> >         return 0;
> >  }
> > diff --git a/mm/shmem.c b/mm/shmem.c
> > index 0d1ce70bce38..ccbaaa5f1c16 100644
> > --- a/mm/shmem.c
> > +++ b/mm/shmem.c
> > @@ -1514,8 +1514,7 @@ static int shmem_writepage(struct page *page, struct writeback_control *wbc)
> >
> >                 mutex_unlock(&shmem_swaplist_mutex);
> >                 BUG_ON(folio_mapped(folio));
> > -               swap_writepage(&folio->page, wbc);
> > -               return 0;
> > +               return swap_writepage(&folio->page, wbc);
> >         }
> >
> >         mutex_unlock(&shmem_swaplist_mutex);
> > diff --git a/mm/zswap.c b/mm/zswap.c
> > index 943090dfe793..caa467e40009 100644
> > --- a/mm/zswap.c
> > +++ b/mm/zswap.c
> > @@ -152,6 +152,11 @@ module_param_named(exclusive_loads, zswap_exclusive_loads_enabled, bool, 0644);
> >  static bool zswap_shrinker_enabled;
> >  module_param_named(shrinker_enabled, zswap_shrinker_enabled, bool, 0644);
> >
> > +bool is_zswap_enabled(void)
> > +{
> > +       return zswap_enabled;
> > +}
> > +
> >  /*********************************
> >  * data structures
> >  **********************************/
> > @@ -589,6 +594,9 @@ static unsigned long zswap_shrinker_scan(struct shrinker *shrinker,
> >         struct zswap_pool *pool = shrinker->private_data;
> >         bool encountered_page_in_swapcache = false;
> >
> > +       if (!mem_cgroup_zswap_writeback_enabled(sc->memcg))
> > +               return SHRINK_STOP;
> > +
> >         nr_protected =
> >                 atomic_long_read(&lruvec->zswap_lruvec_state.nr_zswap_protected);
> >         lru_size = list_lru_shrink_count(&pool->list_lru, sc);
> > @@ -619,6 +627,9 @@ static unsigned long zswap_shrinker_count(struct shrinker *shrinker,
> >         struct lruvec *lruvec = mem_cgroup_lruvec(memcg, NODE_DATA(sc->nid));
> >         unsigned long nr_backing, nr_stored, nr_freeable, nr_protected;
> >
> > +       if (!mem_cgroup_zswap_writeback_enabled(memcg))
> > +               return 0;
> > +
> >  #ifdef CONFIG_MEMCG_KMEM
> >         cgroup_rstat_flush(memcg->css.cgroup);
> >         nr_backing = memcg_page_state(memcg, MEMCG_ZSWAP_B) >> PAGE_SHIFT;
> > @@ -934,6 +945,9 @@ static int shrink_memcg(struct mem_cgroup *memcg)
> >         struct zswap_pool *pool;
> >         int nid, shrunk = 0;
> >
> > +       if (!mem_cgroup_zswap_writeback_enabled(memcg))
> > +               return -EINVAL;
> > +
> >         /*
> >          * Skip zombies because their LRUs are reparented and we would be
> >          * reclaiming from the parent instead of the dead memcg.
> > --
> > 2.34.1
  

Hi Yosry,

I think we are in agreement here. I am already Acked on the previous
version of this patch.
Consider this version as well because the change suggestion is from me.
I am fine with merging the zswap.writeback.

It is also good to get some discussion on the more general ABI as
well. Just like you said, we can also explore the zswap + swapfile
using the more general API. Because obsoleting user-visible ABI is
much harder.

Chris

On Thu, Nov 16, 2023 at 1:01 PM Yosry Ahmed <yosryahmed@google.com> wrote:
>
> On Thu, Nov 16, 2023 at 12:53 PM Chris Li <chrisl@kernel.org> wrote:
> >
> > Hi Nhat,
> >
> > As we discussed, I just want to bounce some alternative ideas related
> > to this write back disabled feature.
> >
> > Currently, the common usage case is zswap alone or zswap + SSD. We
> > treat zswap and
> > SSD as two different tiers with different swap in performance characteristics.
> > If we make it more generic, we can also have more than two swap tiers.
> > e.g. we can have zswap, SSD, network swap or HDD swap.
> > The disable flag is just one bit of information, it can't describe
> > what is the next tier.if it is not the current swap file
> > implementation .
> >
> > One idea is that we can have a more general swap_tier_list object to
> > describe the order of the swap device, The system can have more than
> > one such list to describe different combinations of the tier
> > selection.
> >
> > Each memcg can have a pointer point to one of such swap_tier_list
> > objects, replacing the disabled write back flag in this patch.
> > When you swap out, it will just go through each tier in the list, try
> > to swap it out.
> >  This has some implications for the zswap shrink as well. It becomes a
> > more generic "move swap out data to another tier". Generally need to
> > load into the swap cache then write to another tier.
> >
> > Open question is how we deal with the swap cache index across
> > different tiers. The zswap.writeback_disable will not be needed if we
> > have a more generic swap tier framework.
>
> I agree that this should be the long-term goal, and I suggested that
> we make the interface more future-proof by making it more generic to
> accept the types or tiers of swap allowed by the memcg:
>
> https://lore.kernel.org/lkml/CAJD7tkY8iPBo99+1gdsSRMNDu4jkVKz8rb=W+xk9=GE0y=kSuw@mail.gmail.com/
>
> Since we only have swap and zswap now, the implementation can be
> similar to this code and basically just disable writeback if zswap is
> the only allowed swapping mechanism. So we don't necessarily need to
> have a full swap tiering implementation, but I agree with Chris that
> at least having a future-proof interface to work with generic swap
> tiering is preferrable.
>
>
>
> >
> > Chris
> >
> >
> > On Wed, Nov 15, 2023 at 9:24 AM Nhat Pham <nphamcs@gmail.com> wrote:
> > >
> > > During our experiment with zswap, we sometimes observe swap IOs due to
> > > occasional zswap store failures and writebacks-to-swap. These swapping
> > > IOs prevent many users who cannot tolerate swapping from adopting zswap
> > > to save memory and improve performance where possible.
> > >
> > > This patch adds the option to disable this behavior entirely: do not
> > > writeback to backing swapping device when a zswap store attempt fail,
> > > and do not write pages in the zswap pool back to the backing swap
> > > device (both when the pool is full, and when the new zswap shrinker is
> > > called).
> > >
> > > This new behavior can be opted-in/out on a per-cgroup basis via a new
> > > cgroup file. By default, writebacks to swap device is enabled, which is
> > > the previous behavior. Initially, writeback is enabled for the root
> > > cgroup, and a newly created cgroup will inherit the current setting of
> > > its parent.
> > >
> > > Note that this is subtly different from setting memory.swap.max to 0, as
> > > it still allows for pages to be stored in the zswap pool (which itself
> > > consumes swap space in its current form).
> > >
> > > This patch should be applied on top of the zswap shrinker series:
> > >
> > > https://lore.kernel.org/lkml/20231106183159.3562879-1-nphamcs@gmail.com/
> > >
> > > as it also disables the zswap shrinker, a major source of zswap
> > > writebacks.
> > >
> > > Suggested-by: Johannes Weiner <hannes@cmpxchg.org>
> > > Signed-off-by: Nhat Pham <nphamcs@gmail.com>
> > > ---
> > >  Documentation/admin-guide/cgroup-v2.rst | 12 ++++++++
> > >  Documentation/admin-guide/mm/zswap.rst  |  6 ++++
> > >  include/linux/memcontrol.h              | 12 ++++++++
> > >  include/linux/zswap.h                   |  6 ++++
> > >  mm/memcontrol.c                         | 38 +++++++++++++++++++++++++
> > >  mm/page_io.c                            |  6 ++++
> > >  mm/shmem.c                              |  3 +-
> > >  mm/zswap.c                              | 14 +++++++++
> > >  8 files changed, 95 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst
> > > index 3f85254f3cef..2b4ac43efdc8 100644
> > > --- a/Documentation/admin-guide/cgroup-v2.rst
> > > +++ b/Documentation/admin-guide/cgroup-v2.rst
> > > @@ -1679,6 +1679,18 @@ PAGE_SIZE multiple when read back.
> > >         limit, it will refuse to take any more stores before existing
> > >         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.
> > > +
> > > +       When this is set to 0, all swapping attempts to swapping devices
> > > +       are disabled. This included both zswap writebacks, and swapping due
> > > +       to zswap store failure.
> > > +
> > > +       Note that this is subtly different from setting memory.swap.max to
> > > +       0, as it still allows for pages to be written to the zswap pool.
> > > +
> > >    memory.pressure
> > >         A read-only nested-keyed file.
> > >
> > > diff --git a/Documentation/admin-guide/mm/zswap.rst b/Documentation/admin-guide/mm/zswap.rst
> > > index 522ae22ccb84..b987e58edb70 100644
> > > --- a/Documentation/admin-guide/mm/zswap.rst
> > > +++ b/Documentation/admin-guide/mm/zswap.rst
> > > @@ -153,6 +153,12 @@ attribute, e. g.::
> > >
> > >  Setting this parameter to 100 will disable the hysteresis.
> > >
> > > +Some users cannot tolerate the swapping that comes with zswap store failures
> > > +and zswap writebacks. Swapping can be disabled entirely (without disabling
> > > +zswap itself) on a cgroup-basis as follows:
> > > +
> > > +       echo 0 > /sys/fs/cgroup/<cgroup-name>/memory.zswap.writeback
> > > +
> > >  When there is a sizable amount of cold memory residing in the zswap pool, it
> > >  can be advantageous to proactively write these cold pages to swap and reclaim
> > >  the memory for other use cases. By default, the zswap shrinker is disabled.
> > > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> > > index 83590fd0d6d1..3901ff4dae63 100644
> > > --- a/include/linux/memcontrol.h
> > > +++ b/include/linux/memcontrol.h
> > > @@ -219,6 +219,12 @@ struct mem_cgroup {
> > >
> > >  #if defined(CONFIG_MEMCG_KMEM) && defined(CONFIG_ZSWAP)
> > >         unsigned long zswap_max;
> > > +
> > > +       /*
> > > +        * Prevent pages from this memcg from being written back from zswap to
> > > +        * swap, and from being swapped out on zswap store failures.
> > > +        */
> > > +       bool zswap_writeback;
> > >  #endif
> > >
> > >         unsigned long soft_limit;
> > > @@ -1931,6 +1937,7 @@ static inline void count_objcg_event(struct obj_cgroup *objcg,
> > >  bool obj_cgroup_may_zswap(struct obj_cgroup *objcg);
> > >  void obj_cgroup_charge_zswap(struct obj_cgroup *objcg, size_t size);
> > >  void obj_cgroup_uncharge_zswap(struct obj_cgroup *objcg, size_t size);
> > > +bool mem_cgroup_zswap_writeback_enabled(struct mem_cgroup *memcg);
> > >  #else
> > >  static inline bool obj_cgroup_may_zswap(struct obj_cgroup *objcg)
> > >  {
> > > @@ -1944,6 +1951,11 @@ static inline void obj_cgroup_uncharge_zswap(struct obj_cgroup *objcg,
> > >                                              size_t size)
> > >  {
> > >  }
> > > +static inline bool mem_cgroup_zswap_writeback_enabled(struct mem_cgroup *memcg)
> > > +{
> > > +       /* if zswap is disabled, do not block pages going to the swapping device */
> > > +       return true;
> > > +}
> > >  #endif
> > >
> > >  #endif /* _LINUX_MEMCONTROL_H */
> > > diff --git a/include/linux/zswap.h b/include/linux/zswap.h
> > > index cbd373ba88d2..b4997e27a74b 100644
> > > --- a/include/linux/zswap.h
> > > +++ b/include/linux/zswap.h
> > > @@ -35,6 +35,7 @@ void zswap_swapoff(int type);
> > >  void zswap_memcg_offline_cleanup(struct mem_cgroup *memcg);
> > >  void zswap_lruvec_state_init(struct lruvec *lruvec);
> > >  void zswap_lruvec_swapin(struct page *page);
> > > +bool is_zswap_enabled(void);
> > >  #else
> > >
> > >  struct zswap_lruvec_state {};
> > > @@ -55,6 +56,11 @@ static inline void zswap_swapoff(int type) {}
> > >  static inline void zswap_memcg_offline_cleanup(struct mem_cgroup *memcg) {}
> > >  static inline void zswap_lruvec_init(struct lruvec *lruvec) {}
> > >  static inline void zswap_lruvec_swapin(struct page *page) {}
> > > +
> > > +static inline bool is_zswap_enabled(void)
> > > +{
> > > +       return false;
> > > +}
> > >  #endif
> > >
> > >  #endif /* _LINUX_ZSWAP_H */
> > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > > index 786c7edf5836..5ad71ce31c74 100644
> > > --- a/mm/memcontrol.c
> > > +++ b/mm/memcontrol.c
> > > @@ -5522,6 +5522,8 @@ mem_cgroup_css_alloc(struct cgroup_subsys_state *parent_css)
> > >         WRITE_ONCE(memcg->soft_limit, PAGE_COUNTER_MAX);
> > >  #if defined(CONFIG_MEMCG_KMEM) && defined(CONFIG_ZSWAP)
> > >         memcg->zswap_max = PAGE_COUNTER_MAX;
> > > +       WRITE_ONCE(memcg->zswap_writeback,
> > > +               !parent || READ_ONCE(parent->zswap_writeback));
> > >  #endif
> > >         page_counter_set_high(&memcg->swap, PAGE_COUNTER_MAX);
> > >         if (parent) {
> > > @@ -8146,6 +8148,12 @@ void obj_cgroup_uncharge_zswap(struct obj_cgroup *objcg, size_t size)
> > >         rcu_read_unlock();
> > >  }
> > >
> > > +bool mem_cgroup_zswap_writeback_enabled(struct mem_cgroup *memcg)
> > > +{
> > > +       /* if zswap is disabled, do not block pages going to the swapping device */
> > > +       return !is_zswap_enabled() || !memcg || READ_ONCE(memcg->zswap_writeback);
> > > +}
> > > +
> > >  static u64 zswap_current_read(struct cgroup_subsys_state *css,
> > >                               struct cftype *cft)
> > >  {
> > > @@ -8176,6 +8184,31 @@ static ssize_t zswap_max_write(struct kernfs_open_file *of,
> > >         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",
> > > @@ -8188,6 +8221,11 @@ 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 */
> > > diff --git a/mm/page_io.c b/mm/page_io.c
> > > index cb559ae324c6..5e606f1aa2f6 100644
> > > --- a/mm/page_io.c
> > > +++ b/mm/page_io.c
> > > @@ -201,6 +201,12 @@ int swap_writepage(struct page *page, struct writeback_control *wbc)
> > >                 folio_end_writeback(folio);
> > >                 return 0;
> > >         }
> > > +
> > > +       if (!mem_cgroup_zswap_writeback_enabled(folio_memcg(folio))) {
> > > +               folio_mark_dirty(folio);
> > > +               return AOP_WRITEPAGE_ACTIVATE;
> > > +       }
> > > +
> > >         __swap_writepage(&folio->page, wbc);
> > >         return 0;
> > >  }
> > > diff --git a/mm/shmem.c b/mm/shmem.c
> > > index 0d1ce70bce38..ccbaaa5f1c16 100644
> > > --- a/mm/shmem.c
> > > +++ b/mm/shmem.c
> > > @@ -1514,8 +1514,7 @@ static int shmem_writepage(struct page *page, struct writeback_control *wbc)
> > >
> > >                 mutex_unlock(&shmem_swaplist_mutex);
> > >                 BUG_ON(folio_mapped(folio));
> > > -               swap_writepage(&folio->page, wbc);
> > > -               return 0;
> > > +               return swap_writepage(&folio->page, wbc);
> > >         }
> > >
> > >         mutex_unlock(&shmem_swaplist_mutex);
> > > diff --git a/mm/zswap.c b/mm/zswap.c
> > > index 943090dfe793..caa467e40009 100644
> > > --- a/mm/zswap.c
> > > +++ b/mm/zswap.c
> > > @@ -152,6 +152,11 @@ module_param_named(exclusive_loads, zswap_exclusive_loads_enabled, bool, 0644);
> > >  static bool zswap_shrinker_enabled;
> > >  module_param_named(shrinker_enabled, zswap_shrinker_enabled, bool, 0644);
> > >
> > > +bool is_zswap_enabled(void)
> > > +{
> > > +       return zswap_enabled;
> > > +}
> > > +
> > >  /*********************************
> > >  * data structures
> > >  **********************************/
> > > @@ -589,6 +594,9 @@ static unsigned long zswap_shrinker_scan(struct shrinker *shrinker,
> > >         struct zswap_pool *pool = shrinker->private_data;
> > >         bool encountered_page_in_swapcache = false;
> > >
> > > +       if (!mem_cgroup_zswap_writeback_enabled(sc->memcg))
> > > +               return SHRINK_STOP;
> > > +
> > >         nr_protected =
> > >                 atomic_long_read(&lruvec->zswap_lruvec_state.nr_zswap_protected);
> > >         lru_size = list_lru_shrink_count(&pool->list_lru, sc);
> > > @@ -619,6 +627,9 @@ static unsigned long zswap_shrinker_count(struct shrinker *shrinker,
> > >         struct lruvec *lruvec = mem_cgroup_lruvec(memcg, NODE_DATA(sc->nid));
> > >         unsigned long nr_backing, nr_stored, nr_freeable, nr_protected;
> > >
> > > +       if (!mem_cgroup_zswap_writeback_enabled(memcg))
> > > +               return 0;
> > > +
> > >  #ifdef CONFIG_MEMCG_KMEM
> > >         cgroup_rstat_flush(memcg->css.cgroup);
> > >         nr_backing = memcg_page_state(memcg, MEMCG_ZSWAP_B) >> PAGE_SHIFT;
> > > @@ -934,6 +945,9 @@ static int shrink_memcg(struct mem_cgroup *memcg)
> > >         struct zswap_pool *pool;
> > >         int nid, shrunk = 0;
> > >
> > > +       if (!mem_cgroup_zswap_writeback_enabled(memcg))
> > > +               return -EINVAL;
> > > +
> > >         /*
> > >          * Skip zombies because their LRUs are reparented and we would be
> > >          * reclaiming from the parent instead of the dead memcg.
> > > --
> > > 2.34.1
>
  

On Thu, Nov 16, 2023 at 7:39 PM Chris Li <chrisl@kernel.org> wrote:
>
> Hi Yosry,
>
> I think we are in agreement here. I am already Acked on the previous
> version of this patch.
> Consider this version as well because the change suggestion is from me.
> I am fine with merging the zswap.writeback.
>
> It is also good to get some discussion on the more general ABI as
> well. Just like you said, we can also explore the zswap + swapfile
> using the more general API. Because obsoleting user-visible ABI is
> much harder.
>
> Chris

Hmm how about this - in the future, we support the following
options:

1. zswap.writeback == 1: no limitation to zswap writeback.
All backing swap devices (sorted by priorities?) are fair game.

2. zswap.writeback == 0: disable all forms of zswap writeback.

3. zswap.writeback == <tiers description>:  attempt to write to each
tier, one at a time.

The first two are basically what we have for this patch.
The last one will be added in a future patch.

This is from the userspace perspective. Internally, we can modify
memcg->writeback to be a pointer or a struct instead of this bool.
(as you suggested).

This way, the API remains intact and backward compatible
(and FWIW, I think there are still a lot of values in having simple
options for the users who have simple memory hierarchies).

>
> On Thu, Nov 16, 2023 at 1:01 PM Yosry Ahmed <yosryahmed@google.com> wrote:
> >
> > On Thu, Nov 16, 2023 at 12:53 PM Chris Li <chrisl@kernel.org> wrote:
> > >
> > > Hi Nhat,
> > >
> > > As we discussed, I just want to bounce some alternative ideas related
> > > to this write back disabled feature.
> > >
> > > Currently, the common usage case is zswap alone or zswap + SSD. We
> > > treat zswap and
> > > SSD as two different tiers with different swap in performance characteristics.
> > > If we make it more generic, we can also have more than two swap tiers.
> > > e.g. we can have zswap, SSD, network swap or HDD swap.
> > > The disable flag is just one bit of information, it can't describe
> > > what is the next tier.if it is not the current swap file
> > > implementation .
> > >
> > > One idea is that we can have a more general swap_tier_list object to
> > > describe the order of the swap device, The system can have more than
> > > one such list to describe different combinations of the tier
> > > selection.
> > >
> > > Each memcg can have a pointer point to one of such swap_tier_list
> > > objects, replacing the disabled write back flag in this patch.
> > > When you swap out, it will just go through each tier in the list, try
> > > to swap it out.
> > >  This has some implications for the zswap shrink as well. It becomes a
> > > more generic "move swap out data to another tier". Generally need to
> > > load into the swap cache then write to another tier.
> > >
> > > Open question is how we deal with the swap cache index across
> > > different tiers. The zswap.writeback_disable will not be needed if we
> > > have a more generic swap tier framework.
> >
> > I agree that this should be the long-term goal, and I suggested that
> > we make the interface more future-proof by making it more generic to
> > accept the types or tiers of swap allowed by the memcg:
> >
> > https://lore.kernel.org/lkml/CAJD7tkY8iPBo99+1gdsSRMNDu4jkVKz8rb=W+xk9=GE0y=kSuw@mail.gmail.com/
> >
> > Since we only have swap and zswap now, the implementation can be
> > similar to this code and basically just disable writeback if zswap is
> > the only allowed swapping mechanism. So we don't necessarily need to
> > have a full swap tiering implementation, but I agree with Chris that
> > at least having a future-proof interface to work with generic swap
> > tiering is preferrable.
> >
> >
> >
> > >
> > > Chris
> > >
> > >
> > > On Wed, Nov 15, 2023 at 9:24 AM Nhat Pham <nphamcs@gmail.com> wrote:
> > > >
> > > > During our experiment with zswap, we sometimes observe swap IOs due to
> > > > occasional zswap store failures and writebacks-to-swap. These swapping
> > > > IOs prevent many users who cannot tolerate swapping from adopting zswap
> > > > to save memory and improve performance where possible.
> > > >
> > > > This patch adds the option to disable this behavior entirely: do not
> > > > writeback to backing swapping device when a zswap store attempt fail,
> > > > and do not write pages in the zswap pool back to the backing swap
> > > > device (both when the pool is full, and when the new zswap shrinker is
> > > > called).
> > > >
> > > > This new behavior can be opted-in/out on a per-cgroup basis via a new
> > > > cgroup file. By default, writebacks to swap device is enabled, which is
> > > > the previous behavior. Initially, writeback is enabled for the root
> > > > cgroup, and a newly created cgroup will inherit the current setting of
> > > > its parent.
> > > >
> > > > Note that this is subtly different from setting memory.swap.max to 0, as
> > > > it still allows for pages to be stored in the zswap pool (which itself
> > > > consumes swap space in its current form).
> > > >
> > > > This patch should be applied on top of the zswap shrinker series:
> > > >
> > > > https://lore.kernel.org/lkml/20231106183159.3562879-1-nphamcs@gmail.com/
> > > >
> > > > as it also disables the zswap shrinker, a major source of zswap
> > > > writebacks.
> > > >
> > > > Suggested-by: Johannes Weiner <hannes@cmpxchg.org>
> > > > Signed-off-by: Nhat Pham <nphamcs@gmail.com>
> > > > ---
> > > >  Documentation/admin-guide/cgroup-v2.rst | 12 ++++++++
> > > >  Documentation/admin-guide/mm/zswap.rst  |  6 ++++
> > > >  include/linux/memcontrol.h              | 12 ++++++++
> > > >  include/linux/zswap.h                   |  6 ++++
> > > >  mm/memcontrol.c                         | 38 +++++++++++++++++++++++++
> > > >  mm/page_io.c                            |  6 ++++
> > > >  mm/shmem.c                              |  3 +-
> > > >  mm/zswap.c                              | 14 +++++++++
> > > >  8 files changed, 95 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst
> > > > index 3f85254f3cef..2b4ac43efdc8 100644
> > > > --- a/Documentation/admin-guide/cgroup-v2.rst
> > > > +++ b/Documentation/admin-guide/cgroup-v2.rst
> > > > @@ -1679,6 +1679,18 @@ PAGE_SIZE multiple when read back.
> > > >         limit, it will refuse to take any more stores before existing
> > > >         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.
> > > > +
> > > > +       When this is set to 0, all swapping attempts to swapping devices
> > > > +       are disabled. This included both zswap writebacks, and swapping due
> > > > +       to zswap store failure.
> > > > +
> > > > +       Note that this is subtly different from setting memory.swap.max to
> > > > +       0, as it still allows for pages to be written to the zswap pool.
> > > > +
> > > >    memory.pressure
> > > >         A read-only nested-keyed file.
> > > >
> > > > diff --git a/Documentation/admin-guide/mm/zswap.rst b/Documentation/admin-guide/mm/zswap.rst
> > > > index 522ae22ccb84..b987e58edb70 100644
> > > > --- a/Documentation/admin-guide/mm/zswap.rst
> > > > +++ b/Documentation/admin-guide/mm/zswap.rst
> > > > @@ -153,6 +153,12 @@ attribute, e. g.::
> > > >
> > > >  Setting this parameter to 100 will disable the hysteresis.
> > > >
> > > > +Some users cannot tolerate the swapping that comes with zswap store failures
> > > > +and zswap writebacks. Swapping can be disabled entirely (without disabling
> > > > +zswap itself) on a cgroup-basis as follows:
> > > > +
> > > > +       echo 0 > /sys/fs/cgroup/<cgroup-name>/memory.zswap.writeback
> > > > +
> > > >  When there is a sizable amount of cold memory residing in the zswap pool, it
> > > >  can be advantageous to proactively write these cold pages to swap and reclaim
> > > >  the memory for other use cases. By default, the zswap shrinker is disabled.
> > > > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> > > > index 83590fd0d6d1..3901ff4dae63 100644
> > > > --- a/include/linux/memcontrol.h
> > > > +++ b/include/linux/memcontrol.h
> > > > @@ -219,6 +219,12 @@ struct mem_cgroup {
> > > >
> > > >  #if defined(CONFIG_MEMCG_KMEM) && defined(CONFIG_ZSWAP)
> > > >         unsigned long zswap_max;
> > > > +
> > > > +       /*
> > > > +        * Prevent pages from this memcg from being written back from zswap to
> > > > +        * swap, and from being swapped out on zswap store failures.
> > > > +        */
> > > > +       bool zswap_writeback;
> > > >  #endif
> > > >
> > > >         unsigned long soft_limit;
> > > > @@ -1931,6 +1937,7 @@ static inline void count_objcg_event(struct obj_cgroup *objcg,
> > > >  bool obj_cgroup_may_zswap(struct obj_cgroup *objcg);
> > > >  void obj_cgroup_charge_zswap(struct obj_cgroup *objcg, size_t size);
> > > >  void obj_cgroup_uncharge_zswap(struct obj_cgroup *objcg, size_t size);
> > > > +bool mem_cgroup_zswap_writeback_enabled(struct mem_cgroup *memcg);
> > > >  #else
> > > >  static inline bool obj_cgroup_may_zswap(struct obj_cgroup *objcg)
> > > >  {
> > > > @@ -1944,6 +1951,11 @@ static inline void obj_cgroup_uncharge_zswap(struct obj_cgroup *objcg,
> > > >                                              size_t size)
> > > >  {
> > > >  }
> > > > +static inline bool mem_cgroup_zswap_writeback_enabled(struct mem_cgroup *memcg)
> > > > +{
> > > > +       /* if zswap is disabled, do not block pages going to the swapping device */
> > > > +       return true;
> > > > +}
> > > >  #endif
> > > >
> > > >  #endif /* _LINUX_MEMCONTROL_H */
> > > > diff --git a/include/linux/zswap.h b/include/linux/zswap.h
> > > > index cbd373ba88d2..b4997e27a74b 100644
> > > > --- a/include/linux/zswap.h
> > > > +++ b/include/linux/zswap.h
> > > > @@ -35,6 +35,7 @@ void zswap_swapoff(int type);
> > > >  void zswap_memcg_offline_cleanup(struct mem_cgroup *memcg);
> > > >  void zswap_lruvec_state_init(struct lruvec *lruvec);
> > > >  void zswap_lruvec_swapin(struct page *page);
> > > > +bool is_zswap_enabled(void);
> > > >  #else
> > > >
> > > >  struct zswap_lruvec_state {};
> > > > @@ -55,6 +56,11 @@ static inline void zswap_swapoff(int type) {}
> > > >  static inline void zswap_memcg_offline_cleanup(struct mem_cgroup *memcg) {}
> > > >  static inline void zswap_lruvec_init(struct lruvec *lruvec) {}
> > > >  static inline void zswap_lruvec_swapin(struct page *page) {}
> > > > +
> > > > +static inline bool is_zswap_enabled(void)
> > > > +{
> > > > +       return false;
> > > > +}
> > > >  #endif
> > > >
> > > >  #endif /* _LINUX_ZSWAP_H */
> > > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > > > index 786c7edf5836..5ad71ce31c74 100644
> > > > --- a/mm/memcontrol.c
> > > > +++ b/mm/memcontrol.c
> > > > @@ -5522,6 +5522,8 @@ mem_cgroup_css_alloc(struct cgroup_subsys_state *parent_css)
> > > >         WRITE_ONCE(memcg->soft_limit, PAGE_COUNTER_MAX);
> > > >  #if defined(CONFIG_MEMCG_KMEM) && defined(CONFIG_ZSWAP)
> > > >         memcg->zswap_max = PAGE_COUNTER_MAX;
> > > > +       WRITE_ONCE(memcg->zswap_writeback,
> > > > +               !parent || READ_ONCE(parent->zswap_writeback));
> > > >  #endif
> > > >         page_counter_set_high(&memcg->swap, PAGE_COUNTER_MAX);
> > > >         if (parent) {
> > > > @@ -8146,6 +8148,12 @@ void obj_cgroup_uncharge_zswap(struct obj_cgroup *objcg, size_t size)
> > > >         rcu_read_unlock();
> > > >  }
> > > >
> > > > +bool mem_cgroup_zswap_writeback_enabled(struct mem_cgroup *memcg)
> > > > +{
> > > > +       /* if zswap is disabled, do not block pages going to the swapping device */
> > > > +       return !is_zswap_enabled() || !memcg || READ_ONCE(memcg->zswap_writeback);
> > > > +}
> > > > +
> > > >  static u64 zswap_current_read(struct cgroup_subsys_state *css,
> > > >                               struct cftype *cft)
> > > >  {
> > > > @@ -8176,6 +8184,31 @@ static ssize_t zswap_max_write(struct kernfs_open_file *of,
> > > >         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",
> > > > @@ -8188,6 +8221,11 @@ 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 */
> > > > diff --git a/mm/page_io.c b/mm/page_io.c
> > > > index cb559ae324c6..5e606f1aa2f6 100644
> > > > --- a/mm/page_io.c
> > > > +++ b/mm/page_io.c
> > > > @@ -201,6 +201,12 @@ int swap_writepage(struct page *page, struct writeback_control *wbc)
> > > >                 folio_end_writeback(folio);
> > > >                 return 0;
> > > >         }
> > > > +
> > > > +       if (!mem_cgroup_zswap_writeback_enabled(folio_memcg(folio))) {
> > > > +               folio_mark_dirty(folio);
> > > > +               return AOP_WRITEPAGE_ACTIVATE;
> > > > +       }
> > > > +
> > > >         __swap_writepage(&folio->page, wbc);
> > > >         return 0;
> > > >  }
> > > > diff --git a/mm/shmem.c b/mm/shmem.c
> > > > index 0d1ce70bce38..ccbaaa5f1c16 100644
> > > > --- a/mm/shmem.c
> > > > +++ b/mm/shmem.c
> > > > @@ -1514,8 +1514,7 @@ static int shmem_writepage(struct page *page, struct writeback_control *wbc)
> > > >
> > > >                 mutex_unlock(&shmem_swaplist_mutex);
> > > >                 BUG_ON(folio_mapped(folio));
> > > > -               swap_writepage(&folio->page, wbc);
> > > > -               return 0;
> > > > +               return swap_writepage(&folio->page, wbc);
> > > >         }
> > > >
> > > >         mutex_unlock(&shmem_swaplist_mutex);
> > > > diff --git a/mm/zswap.c b/mm/zswap.c
> > > > index 943090dfe793..caa467e40009 100644
> > > > --- a/mm/zswap.c
> > > > +++ b/mm/zswap.c
> > > > @@ -152,6 +152,11 @@ module_param_named(exclusive_loads, zswap_exclusive_loads_enabled, bool, 0644);
> > > >  static bool zswap_shrinker_enabled;
> > > >  module_param_named(shrinker_enabled, zswap_shrinker_enabled, bool, 0644);
> > > >
> > > > +bool is_zswap_enabled(void)
> > > > +{
> > > > +       return zswap_enabled;
> > > > +}
> > > > +
> > > >  /*********************************
> > > >  * data structures
> > > >  **********************************/
> > > > @@ -589,6 +594,9 @@ static unsigned long zswap_shrinker_scan(struct shrinker *shrinker,
> > > >         struct zswap_pool *pool = shrinker->private_data;
> > > >         bool encountered_page_in_swapcache = false;
> > > >
> > > > +       if (!mem_cgroup_zswap_writeback_enabled(sc->memcg))
> > > > +               return SHRINK_STOP;
> > > > +
> > > >         nr_protected =
> > > >                 atomic_long_read(&lruvec->zswap_lruvec_state.nr_zswap_protected);
> > > >         lru_size = list_lru_shrink_count(&pool->list_lru, sc);
> > > > @@ -619,6 +627,9 @@ static unsigned long zswap_shrinker_count(struct shrinker *shrinker,
> > > >         struct lruvec *lruvec = mem_cgroup_lruvec(memcg, NODE_DATA(sc->nid));
> > > >         unsigned long nr_backing, nr_stored, nr_freeable, nr_protected;
> > > >
> > > > +       if (!mem_cgroup_zswap_writeback_enabled(memcg))
> > > > +               return 0;
> > > > +
> > > >  #ifdef CONFIG_MEMCG_KMEM
> > > >         cgroup_rstat_flush(memcg->css.cgroup);
> > > >         nr_backing = memcg_page_state(memcg, MEMCG_ZSWAP_B) >> PAGE_SHIFT;
> > > > @@ -934,6 +945,9 @@ static int shrink_memcg(struct mem_cgroup *memcg)
> > > >         struct zswap_pool *pool;
> > > >         int nid, shrunk = 0;
> > > >
> > > > +       if (!mem_cgroup_zswap_writeback_enabled(memcg))
> > > > +               return -EINVAL;
> > > > +
> > > >         /*
> > > >          * Skip zombies because their LRUs are reparented and we would be
> > > >          * reclaiming from the parent instead of the dead memcg.
> > > > --
> > > > 2.34.1
> >
  

On Sat, Nov 18, 2023 at 11:23 AM Nhat Pham <nphamcs@gmail.com> wrote:
>
> Hmm how about this - in the future, we support the following
> options:
>
> 1. zswap.writeback == 1: no limitation to zswap writeback.
> All backing swap devices (sorted by priorities?) are fair game.
>
> 2. zswap.writeback == 0: disable all forms of zswap writeback.
>
> 3. zswap.writeback == <tiers description>:  attempt to write to each
> tier, one at a time.

We can merge the zswap.writeback as it is for now to unblock you.

For the future. I think we should remove zswap.writeback completely.

Instead we have:

swap.tiers == <swap_tier_list_name>
swap.tiers == "all" all available swap tiers. "zswap + swap file".
This is the default.
swap.tiers == "zswap" zswap only, no other swap file. Internally set
zswap.writeback = 0
swap.tiers == "foo" foo is a list of swap devices it can use. You can
define your town custom swap tier list in
swap.tiers == "none" or "disabled" Not allowed to swap.

"all", "zswap", "none" are reserved keywords.
"foo", "bar" etc are custom lists of swap tiers. User define custom
tier list in sys/kernel/mm/swap/tiers:
ssd:zswap,/dev/nvme01p4
hdd:/dev/sda4,/dev/sdb4

That would define two custom tiers. "ssd" can use zswap then /dev/nvme01p4.
The exact name of the "swap.tiers" and tiers name are open to suggestions.

>
> The first two are basically what we have for this patch.
> The last one will be added in a future patch.
>
> This is from the userspace perspective. Internally, we can modify
> memcg->writeback to be a pointer or a struct instead of this bool.
> (as you suggested).

Internally I would suggest memcg->swaptiers, the write back name is
somewhat confusing. As your patch indicated. It has two situation:
1. shrinking from zpool to real swapfile. The write back is appropriate here.
 2. zswap store failed (compression ratio too low, out of memory etc).
The write back is confusing here. It is more like writing through or
skip.

>
> This way, the API remains intact and backward compatible
> (and FWIW, I think there are still a lot of values in having simple
> options for the users who have simple memory hierarchies).

swap.tiers can be simple. For example, you can modify your patch to
"swap.tires == zswap" to
set zswap.writeback bool to 0 for now. Most of your patch is still re-usable.

I think we should discuss if we want to keep zswap.writeback in the
future because that would be some code undeletable and functionally
overlap with swap.tiers

Chris
  

On Sun, Nov 19, 2023 at 1:39 AM Chris Li <chrisl@kernel.org> wrote:
>
> On Sat, Nov 18, 2023 at 11:23 AM Nhat Pham <nphamcs@gmail.com> wrote:
> >
> > Hmm how about this - in the future, we support the following
> > options:
> >
> > 1. zswap.writeback == 1: no limitation to zswap writeback.
> > All backing swap devices (sorted by priorities?) are fair game.
> >
> > 2. zswap.writeback == 0: disable all forms of zswap writeback.
> >
> > 3. zswap.writeback == <tiers description>:  attempt to write to each
> > tier, one at a time.
>
> We can merge the zswap.writeback as it is for now to unblock you.
>
> For the future. I think we should remove zswap.writeback completely.

I'm a bit weary about API changes, especially changes that affect
backward compatibility. Breaking existing userspace programs simply
with a kernel upgrade does not sound very nice to me.

(although I've heard that the eventual plan is to deprecate cgroupv1
- not sure how that is gonna proceed).

Hence my attempt at creating something that can both serve the
current use case, while still remaining (fairly) extensible for future
ideas.

>
> Instead we have:
>
> swap.tiers == <swap_tier_list_name>
> swap.tiers == "all" all available swap tiers. "zswap + swap file".
> This is the default.
> swap.tiers == "zswap" zswap only, no other swap file. Internally set
> zswap.writeback = 0
> swap.tiers == "foo" foo is a list of swap devices it can use. You can
> define your town custom swap tier list in
> swap.tiers == "none" or "disabled" Not allowed to swap.

swap.tiers == "none" or "disabled" means disallowing zswap as
well, correct?

>
> "all", "zswap", "none" are reserved keywords.
> "foo", "bar" etc are custom lists of swap tiers. User define custom
> tier list in sys/kernel/mm/swap/tiers:
> ssd:zswap,/dev/nvme01p4
> hdd:/dev/sda4,/dev/sdb4

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).

I'll let other people weigh in about this of course.
Johannes, how do you feel about this proposed API?

>
> That would define two custom tiers. "ssd" can use zswap then /dev/nvme01p4.
> The exact name of the "swap.tiers" and tiers name are open to suggestions.
>
> >
> > The first two are basically what we have for this patch.
> > The last one will be added in a future patch.
> >
> > This is from the userspace perspective. Internally, we can modify
> > memcg->writeback to be a pointer or a struct instead of this bool.
> > (as you suggested).
>
> Internally I would suggest memcg->swaptiers, the write back name is
> somewhat confusing. As your patch indicated. It has two situation:
> 1. shrinking from zpool to real swapfile. The write back is appropriate here.
>  2. zswap store failed (compression ratio too low, out of memory etc).
> The write back is confusing here. It is more like writing through or
> skip.
>
> >
> > This way, the API remains intact and backward compatible
> > (and FWIW, I think there are still a lot of values in having simple
> > options for the users who have simple memory hierarchies).
>
> swap.tiers can be simple. For example, you can modify your patch to
> "swap.tires == zswap" to
> set zswap.writeback bool to 0 for now. Most of your patch is still re-usable.
>

I'm less concerned about internals - that is always up to changes.
I'm a bit more concerned with the API we're exposing to the users.

> I think we should discuss if we want to keep zswap.writeback in the
> future because that would be some code undeletable and functionally
> overlap with swap.tiers

This is a fair point.

>
> Chris
  

On Sun, Nov 19, 2023 at 11:08 AM Nhat Pham <nphamcs@gmail.com> wrote:
>
> On Sun, Nov 19, 2023 at 1:39 AM Chris Li <chrisl@kernel.org> wrote:
> >
> > On Sat, Nov 18, 2023 at 11:23 AM Nhat Pham <nphamcs@gmail.com> wrote:
> > >
> > > Hmm how about this - in the future, we support the following
> > > options:
> > >
> > > 1. zswap.writeback == 1: no limitation to zswap writeback.
> > > All backing swap devices (sorted by priorities?) are fair game.
> > >
> > > 2. zswap.writeback == 0: disable all forms of zswap writeback.
> > >
> > > 3. zswap.writeback == <tiers description>:  attempt to write to each
> > > tier, one at a time.
> >
> > We can merge the zswap.writeback as it is for now to unblock you.
> >
> > For the future. I think we should remove zswap.writeback completely.
>
> I'm a bit weary about API changes, especially changes that affect
> backward compatibility. Breaking existing userspace programs simply
> with a kernel upgrade does not sound very nice to me.
>
> (although I've heard that the eventual plan is to deprecate cgroupv1
> - not sure how that is gonna proceed).
>
> Hence my attempt at creating something that can both serve the
> current use case, while still remaining (fairly) extensible for future
> ideas.

With that reasoning, the "swap.tiers" would serve better than "zswap.writeback",
do you think so?

>
> >
> > Instead we have:
> >
> > swap.tiers == <swap_tier_list_name>
> > swap.tiers == "all" all available swap tiers. "zswap + swap file".
> > This is the default.
> > swap.tiers == "zswap" zswap only, no other swap file. Internally set
> > zswap.writeback = 0
> > swap.tiers == "foo" foo is a list of swap devices it can use. You can
> > define your town custom swap tier list in
> > swap.tiers == "none" or "disabled" Not allowed to swap.
>
> swap.tiers == "none" or "disabled" means disallowing zswap as
> well, correct?

Correct, no swap at all.

>
> >
> > "all", "zswap", "none" are reserved keywords.
> > "foo", "bar" etc are custom lists of swap tiers. User define custom
> > tier list in sys/kernel/mm/swap/tiers:
> > ssd:zswap,/dev/nvme01p4
> > hdd:/dev/sda4,/dev/sdb4
>
> 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'll let other people weigh in about this of course.
> Johannes, how do you feel about this proposed API?
>
> >
> > That would define two custom tiers. "ssd" can use zswap then /dev/nvme01p4.
> > The exact name of the "swap.tiers" and tiers name are open to suggestions.
> >
> > >
> > > The first two are basically what we have for this patch.
> > > The last one will be added in a future patch.
> > >
> > > This is from the userspace perspective. Internally, we can modify
> > > memcg->writeback to be a pointer or a struct instead of this bool.
> > > (as you suggested).
> >
> > Internally I would suggest memcg->swaptiers, the write back name is
> > somewhat confusing. As your patch indicated. It has two situation:
> > 1. shrinking from zpool to real swapfile. The write back is appropriate here.
> >  2. zswap store failed (compression ratio too low, out of memory etc).
> > The write back is confusing here. It is more like writing through or
> > skip.
> >
> > >
> > > This way, the API remains intact and backward compatible
> > > (and FWIW, I think there are still a lot of values in having simple
> > > options for the users who have simple memory hierarchies).
> >
> > swap.tiers can be simple. For example, you can modify your patch to
> > "swap.tires == zswap" to
> > set zswap.writeback bool to 0 for now. Most of your patch is still re-usable.
> >
>
> I'm less concerned about internals - that is always up to changes.
> I'm a bit more concerned with the API we're exposing to the users.

Me too. I think we are in agreement here. That is why I think
"swap.tiers" is more general.

>
> > I think we should discuss if we want to keep zswap.writeback in the
> > future because that would be some code undeletable and functionally
> > overlap with swap.tiers
>
> This is a fair point.

If you think we have the risk of not being able to obsolete
"zswap.writeback", then it would be much better not to introduce
"zswap.writeback" in the first place. Just have a minimal
implementation of "swap.tiers" instead. I believe from the code
complexity point of view, the complexity should be very similar.

Chris
  

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 */
  

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

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!
  

On Tue, Nov 21, 2023 at 10:13 AM Nhat Pham <nphamcs@gmail.com> wrote:
>
> Hi Chris!
>
> Thanks for the patch. Would you mind if I spend some time staring
> at the suggestion again and testing it some more?

Of course, by all means. That is just the minimal version to be
functional compatible with your zswap.writeback.

I might consider a follow up patch to add "no_zswap" and "none" to
convert the SSD only swapfile, which can't be expressed by
zswap.writeback.
That should cover all 4 combinations of zswap and swap files without
creating a custom swap tiers list.

"all": zswap + swapfile
"zswap": zswap only
"no_zswap": swapfile only.
"none": no swap.

All keyword names are open to suggestions.

>
> 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.

Great!

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

My pleasure to discuss the swap with you. We should do the online
"swap meet" and invite other developers who are interested in the swap
area as well.

Chris
  

On Tue, Nov 21, 2023 at 11:09 AM Chris Li <chrisl@kernel.org> wrote:
>
> On Tue, Nov 21, 2023 at 10:13 AM Nhat Pham <nphamcs@gmail.com> wrote:
> >
> > Hi Chris!
> >
> > Thanks for the patch. Would you mind if I spend some time staring
> > at the suggestion again and testing it some more?
>
> Of course, by all means. That is just the minimal version to be
> functional compatible with your zswap.writeback.
>
> I might consider a follow up patch to add "no_zswap" and "none" to
> convert the SSD only swapfile, which can't be expressed by
> zswap.writeback.
> That should cover all 4 combinations of zswap and swap files without
> creating a custom swap tiers list.
>
> "all": zswap + swapfile
> "zswap": zswap only
> "no_zswap": swapfile only.
> "none": no swap.
>
> All keyword names are open to suggestions.

SGTM! There might be some functionality duplication between
memory.swap.tiers = no_zswap and memory.zswap.max = 0, but
otherwise this seems reasonable to me.

no_zswap sounds a bit awkward, but I can't come up with a better
name.

>
> >
> > 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.
>
> Great!
>
> >
> > Anyway, have a nice Thanksgiving break everyone! Thanks for
> > taking the time to review my patch and discuss the API with me!
>
> My pleasure to discuss the swap with you. We should do the online
> "swap meet" and invite other developers who are interested in the swap
> area as well.

I look forward to this meeting! I'd love to discuss more about (z)swap
development (and more generally, multi-tier memory management).

Generic page promoter/demoter that takes into account workload
(cgroup), access recency (LRU + generations)/frequency, and tier
characteristics (latency, bandwidth, etc.) will be awesome to explore!

>
> Chris
  

On Tue, Nov 21, 2023 at 5:19 PM Nhat Pham <nphamcs@gmail.com> wrote:
>
> On Tue, Nov 21, 2023 at 11:09 AM Chris Li <chrisl@kernel.org> wrote:
> >
> > On Tue, Nov 21, 2023 at 10:13 AM Nhat Pham <nphamcs@gmail.com> wrote:
> > >
> > > Hi Chris!
> > >
> > > Thanks for the patch. Would you mind if I spend some time staring
> > > at the suggestion again and testing it some more?
> >
> > Of course, by all means. That is just the minimal version to be
> > functional compatible with your zswap.writeback.
> >
> > I might consider a follow up patch to add "no_zswap" and "none" to
> > convert the SSD only swapfile, which can't be expressed by
> > zswap.writeback.
> > That should cover all 4 combinations of zswap and swap files without
> > creating a custom swap tiers list.
> >
> > "all": zswap + swapfile
> > "zswap": zswap only
> > "no_zswap": swapfile only.
> > "none": no swap.
> >
> > All keyword names are open to suggestions.
>
> SGTM! There might be some functionality duplication between
> memory.swap.tiers = no_zswap and memory.zswap.max = 0, but
> otherwise this seems reasonable to me.

Yes, there is some function duplication. However, there is some small
difference that no_zswap will not enter zswap code at all. Vs
memory.zswap.max will take a short trip into zswap code to find out
Oops, not zswap for you.

>
> no_zswap sounds a bit awkward, but I can't come up with a better
> name.

Again, I am open to better suggestions.
I have also considered "!zswap", "!" has special meaning in bash, so
it will require quoting in bash.
How about "-zswap"? This does not require special quoting in bash.

>
> >
> > >
> > > 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.
> >
> > Great!
> >
> > >
> > > Anyway, have a nice Thanksgiving break everyone! Thanks for
> > > taking the time to review my patch and discuss the API with me!
> >
> > My pleasure to discuss the swap with you. We should do the online
> > "swap meet" and invite other developers who are interested in the swap
> > area as well.
>
> I look forward to this meeting! I'd love to discuss more about (z)swap
> development (and more generally, multi-tier memory management).

Let me arrange one then. I am thinking maybe every second week of the
month. That can avoid thanksgiving, christmas and new year.

Let me throw in some more ideas: writing compressed zswap data to SSD
without swap cache.

> Generic page promoter/demoter that takes into account workload
> (cgroup), access recency (LRU + generations)/frequency, and tier
> characteristics (latency, bandwidth, etc.) will be awesome to explore!

Sounds great. Looking forward to it.

Chris
  

On Tue, Nov 21, 2023 at 5:19 PM Nhat Pham <nphamcs@gmail.com> wrote:

> > "all": zswap + swapfile
> > "zswap": zswap only
> > "no_zswap": swapfile only.
> > "none": no swap.
> >
> > All keyword names are open to suggestions.
>
> SGTM! There might be some functionality duplication between
> memory.swap.tiers = no_zswap and memory.zswap.max = 0, but
> otherwise this seems reasonable to me.
>
> no_zswap sounds a bit awkward, but I can't come up with a better
> name.

I sleep on it a bit. I  should apply my own suggestion of using the
positive words rather than negative one to myself.
I actually define it as a non RAM base swap device. How about "disk"?
It will include SSD and HDD disk.

The current 4 combination will be:

"all": zswap + disk swap file
"zswap": zswap only
"disk": disk only (including SSD and HDD)
"none": no swap for you.

Chris
  

On Wed, Nov 22, 2023 at 7:01 AM Chris Li <chrisl@kernel.org> wrote:
>
> On Tue, Nov 21, 2023 at 5:19 PM Nhat Pham <nphamcs@gmail.com> wrote:
>
> > > "all": zswap + swapfile
> > > "zswap": zswap only
> > > "no_zswap": swapfile only.
> > > "none": no swap.
> > >
> > > All keyword names are open to suggestions.
> >
> > SGTM! There might be some functionality duplication between
> > memory.swap.tiers = no_zswap and memory.zswap.max = 0, but
> > otherwise this seems reasonable to me.
> >
> > no_zswap sounds a bit awkward, but I can't come up with a better
> > name.
>
> I sleep on it a bit. I  should apply my own suggestion of using the
> positive words rather than negative one to myself.
> I actually define it as a non RAM base swap device. How about "disk"?
> It will include SSD and HDD disk.
>
> The current 4 combination will be:
>
> "all": zswap + disk swap file
> "zswap": zswap only
> "disk": disk only (including SSD and HDD)
> "none": no swap for you.
>
> Chris

Hi Chris,

I chatted with Johannes a bit more about this design. While we still
think it's potentially useful for the future, it lacks a concrete use
case at the moment. We don't even have the infrastructure for multiple
swap tiers at the moment, so adding this interface now is just making
it more confusing for the users. I think zswap.writeback is a much
more specific interface, with concrete and immediate usability (it
stems from internal chatters and requests - so the demand is already
there).

I think we should just land the change we currently have (rebased on
top of mm-unstable to resolve merge conflicts etc.). I don't think
zswap.writeback will get in the way of any swap.tiers functionality,
correct? There might be some functionality duplication, but that's not
too bad IHMO. Then we can work on swap.tiers design and implementation
as we add the support for multiple swap tiers.