Re: [PATCH 3/3] mm: zswap: proactive shrinking before pool size limit is hit

[Nhat Pham <nphamcs@gmail.com>]

On Mon, May 27, 2024 at 9:34 PM Takero Funaki <flintglass@gmail.com> wrote:
>
> This patch implements proactive shrinking of zswap pool before the max
> pool size limit is reached. This also changes zswap to accept new pages
> while the shrinker is running.
>
> To prevent zswap from rejecting new pages and incurring latency when
> zswap is full, this patch queues the global shrinker by a pool usage
> threshold at the middle of 100% and accept_thr_percent, instead of the
> max pool size.  The pool size will be controlled between 90% to 95% for
> the default accept_thr_percent=90.  Since the current global shrinker
> continues to shrink until accept_thr_percent, we do not need to maintain
> the hysteresis variable tracking the pool limit overage in
> zswap_store().
>
> Before this patch, zswap rejected pages while the shrinker is running
> without incrementing zswap_pool_limit_hit counter. It could be a reason
> why zswap writethrough new pages before writeback old pages.  With this
> patch, zswap accepts new pages while shrinking, and zswap increments
> the counter when and only when zswap rejects pages by the max pool size.
>
> The name of sysfs tunable accept_thr_percent is unchanged as it is still
> the stop condition of the shrinker.
> The respective documentation is updated to describe the new behavior.

I'm a bit unsure about using this tunable. How would the user
determine the level at which the zswap pool should be kept empty?

I was actually thinking of removing this knob altogether :)

>
> Signed-off-by: Takero Funaki <flintglass@gmail.com>
> ---
>  Documentation/admin-guide/mm/zswap.rst | 17 +++++----
>  mm/zswap.c                             | 49 +++++++++++++++-----------
>  2 files changed, 37 insertions(+), 29 deletions(-)
>
> diff --git a/Documentation/admin-guide/mm/zswap.rst b/Documentation/admin-guide/mm/zswap.rst
> index 3598dcd7dbe7..a1d8f167a27a 100644
> --- a/Documentation/admin-guide/mm/zswap.rst
> +++ b/Documentation/admin-guide/mm/zswap.rst
> @@ -111,18 +111,17 @@ checked if it is a same-value filled page before compressing it. If true, the
>  compressed length of the page is set to zero and the pattern or same-filled
>  value is stored.
>
> -To prevent zswap from shrinking pool when zswap is full and there's a high
> -pressure on swap (this will result in flipping pages in and out zswap pool
> -without any real benefit but with a performance drop for the system), a
> -special parameter has been introduced to implement a sort of hysteresis to
> -refuse taking pages into zswap pool until it has sufficient space if the limit
> -has been hit. To set the threshold at which zswap would start accepting pages
> -again after it became full, use the sysfs ``accept_threshold_percent``
> -attribute, e. g.::
> +To prevent zswap from rejecting new pages and incurring latency when zswap is
> +full, zswap initiates a worker called global shrinker that proactively evicts
> +some pages from the pool to swap devices while the pool is reaching the limit.
> +The global shrinker continues to evict pages until there is sufficient space to
> +accept new pages. To control how many pages should remain in the pool, use the
> +sysfs ``accept_threshold_percent`` attribute as a percentage of the max pool
> +size, e. g.::
>
>         echo 80 > /sys/module/zswap/parameters/accept_threshold_percent
>
> -Setting this parameter to 100 will disable the hysteresis.
> +Setting this parameter to 100 will disable the proactive shrinking.
>
>  Some users cannot tolerate the swapping that comes with zswap store failures
>  and zswap writebacks. Swapping can be disabled entirely (without disabling
> diff --git a/mm/zswap.c b/mm/zswap.c
> index 08a6f5a6bf62..0186224be8fc 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -71,8 +71,6 @@ static u64 zswap_reject_kmemcache_fail;
>
>  /* Shrinker work queue */
>  static struct workqueue_struct *shrink_wq;
> -/* Pool limit was hit, we need to calm down */
> -static bool zswap_pool_reached_full;
>
>  /*********************************
>  * tunables
> @@ -118,7 +116,10 @@ module_param_cb(zpool, &zswap_zpool_param_ops, &zswap_zpool_type, 0644);
>  static unsigned int zswap_max_pool_percent = 20;
>  module_param_named(max_pool_percent, zswap_max_pool_percent, uint, 0644);
>
> -/* The threshold for accepting new pages after the max_pool_percent was hit */
> +/*
> + * The percentage of pool size that the global shrinker keeps in memory.
> + * It does not protect old pages from the dynamic shrinker.
> + */
>  static unsigned int zswap_accept_thr_percent = 90; /* of max pool size */
>  module_param_named(accept_threshold_percent, zswap_accept_thr_percent,
>                    uint, 0644);
> @@ -487,6 +488,14 @@ static unsigned long zswap_accept_thr_pages(void)
>         return zswap_max_pages() * zswap_accept_thr_percent / 100;
>  }
>
> +/*
> + * Returns threshold to start proactive global shrinking.
> + */
> +static inline unsigned long zswap_shrink_start_pages(void)
> +{
> +       return zswap_max_pages() * (100 - (100 - zswap_accept_thr_percent)/2) / 100;
> +}
> +
>  unsigned long zswap_total_pages(void)
>  {
>         struct zswap_pool *pool;
> @@ -504,21 +513,6 @@ unsigned long zswap_total_pages(void)
>         return total;
>  }
>
> -static bool zswap_check_limits(void)
> -{
> -       unsigned long cur_pages = zswap_total_pages();
> -       unsigned long max_pages = zswap_max_pages();
> -
> -       if (cur_pages >= max_pages) {
> -               zswap_pool_limit_hit++;
> -               zswap_pool_reached_full = true;
> -       } else if (zswap_pool_reached_full &&
> -                  cur_pages <= zswap_accept_thr_pages()) {
> -                       zswap_pool_reached_full = false;
> -       }
> -       return zswap_pool_reached_full;
> -}
> -
>  /*********************************
>  * param callbacks
>  **********************************/
> @@ -1475,6 +1469,8 @@ bool zswap_store(struct folio *folio)
>         struct obj_cgroup *objcg = NULL;
>         struct mem_cgroup *memcg = NULL;
>         unsigned long value;
> +       unsigned long cur_pages;
> +       bool need_global_shrink = false;
>
>         VM_WARN_ON_ONCE(!folio_test_locked(folio));
>         VM_WARN_ON_ONCE(!folio_test_swapcache(folio));
> @@ -1497,8 +1493,18 @@ bool zswap_store(struct folio *folio)
>                 mem_cgroup_put(memcg);
>         }
>
> -       if (zswap_check_limits())
> +       cur_pages = zswap_total_pages();
> +
> +       if (cur_pages >= zswap_max_pages()) {
> +               zswap_pool_limit_hit++;
> +               need_global_shrink = true;
>                 goto reject;
> +       }
> +
> +       /* schedule shrink for incoming pages */
> +       if (cur_pages >= zswap_shrink_start_pages()
> +                       && !work_pending(&zswap_shrink_work))
> +               queue_work(shrink_wq, &zswap_shrink_work);

I think work_pending() check here is redundant. If you look at the
documentation, queue_work only succeeds if zswap_shrink_work is not
already on the shrink_wq workqueue.

More specifically, if you check the code, queue_work calls
queue_work_on, which has this check:

if (!test_and_set_bit(WORK_STRUCT_PENDING_BIT, work_data_bits(work)) &&
   !clear_pending_if_disabled(work)) {

This is the same bit-check as work_pending, which is defined as:

#define work_pending(work) \
test_bit(WORK_STRUCT_PENDING_BIT, work_data_bits(work))


>
>         /* allocate entry */
>         entry = zswap_entry_cache_alloc(GFP_KERNEL, folio_nid(folio));
> @@ -1541,6 +1547,9 @@ bool zswap_store(struct folio *folio)
>
>                 WARN_ONCE(err != -ENOMEM, "unexpected xarray error: %d\n", err);
>                 zswap_reject_alloc_fail++;
> +
> +               /* reduce entry in array */
> +               need_global_shrink = true;
>                 goto store_failed;
>         }
>
> @@ -1590,7 +1599,7 @@ bool zswap_store(struct folio *folio)
>         zswap_entry_cache_free(entry);
>  reject:
>         obj_cgroup_put(objcg);
> -       if (zswap_pool_reached_full)
> +       if (need_global_shrink && !work_pending(&zswap_shrink_work))
>                 queue_work(shrink_wq, &zswap_shrink_work);
>  check_old:
>         /*
> --
> 2.43.0
>