On Mon, May 27, 2024 at 9:34 PM Takero Funaki <flintglass@xxxxxxxxx> 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@xxxxxxxxx>
> ---
> 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
>
