2024年5月29日(水) 1:01 Nhat Pham <nphamcs@xxxxxxxxx>:
>
> 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 :)
>
If we see a large pool_limit_hit, that indicates we should lower the
accept threshold to make more space proactively, to store new pages
from active processes rather than rejecting.
If not, we can set a higher accept threshold for swapin, s.t.
low-activity background processes.
That depends on one's workload, and can be tuned by the admin, I think.
> >
> > 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))
>
Thanks for the review and info. I will remove the tests.
>
> >
> > /* 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
> >
--
<flintglass@xxxxxxxxx>
