Received-SPF: pass (google.com: domain of linux-kernel+bounces-194157-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
References: <20240528043404.39327-2-flintglass@gmail.com> <20240528043404.39327-5-flintglass@gmail.com>
 <CAKEwX=NcdpgGTp8Kj_kiTkvAK=k8kzayhMo+oeaXGZFA5RuoEw@mail.gmail.com>
In-Reply-To: <CAKEwX=NcdpgGTp8Kj_kiTkvAK=k8kzayhMo+oeaXGZFA5RuoEw@mail.gmail.com>
From: Takero Funaki <flintglass@gmail.com>
Date: Wed, 29 May 2024 22:27:53 +0900
Message-ID: <CAPpoddeKguLK3paGKAXQ+jO30jC05hvdvZiro2yfVGgrZWnOeA@mail.gmail.com>
Subject: Re: [PATCH 3/3] mm: zswap: proactive shrinking before pool size limit
 is hit
To: Nhat Pham <nphamcs@gmail.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>, Yosry Ahmed <yosryahmed@google.com>, 
	Chengming Zhou <chengming.zhou@linux.dev>, Jonathan Corbet <corbet@lwn.net>, 
	Andrew Morton <akpm@linux-foundation.org>, 
	Domenico Cerasuolo <cerasuolodomenico@gmail.com>, linux-mm@kvack.org, linux-doc@vger.kernel.org, 
	linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

2024=E5=B9=B45=E6=9C=8829=E6=97=A5(=E6=B0=B4) 1:01 Nhat Pham <nphamcs@gmail=
com>:
>
> On Mon, May 27, 2024 at 9:34=E2=80=AFPM Takero Funaki <flintglass@gmail.c=
om> 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=3D90.  Since the current global shrinker
> > continues to shrink until accept_thr_percent, we do not need to maintai=
n
> > 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 stil=
l
> > 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@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/adm=
in-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-f=
illed
> >  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 hysteresi=
s to
> > -refuse taking pages into zswap pool until it has sufficient space if t=
he limit
> > -has been hit. To set the threshold at which zswap would start acceptin=
g 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 z=
swap 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 th=
e 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 ma=
x 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 fa=
ilures
> >  and zswap writebacks. Swapping can be disabled entirely (without disab=
ling
> > 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, &zs=
wap_zpool_type, 0644);
> >  static unsigned int zswap_max_pool_percent =3D 20;
> >  module_param_named(max_pool_percent, zswap_max_pool_percent, uint, 064=
4);
> >
> > -/* The threshold for accepting new pages after the max_pool_percent wa=
s hit */
> > +/*
> > + * The percentage of pool size that the global shrinker keeps in memor=
y.
> > + * It does not protect old pages from the dynamic shrinker.
> > + */
> >  static unsigned int zswap_accept_thr_percent =3D 90; /* of max pool si=
ze */
> >  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_perce=
nt)/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 =3D zswap_total_pages();
> > -       unsigned long max_pages =3D zswap_max_pages();
> > -
> > -       if (cur_pages >=3D max_pages) {
> > -               zswap_pool_limit_hit++;
> > -               zswap_pool_reached_full =3D true;
> > -       } else if (zswap_pool_reached_full &&
> > -                  cur_pages <=3D zswap_accept_thr_pages()) {
> > -                       zswap_pool_reached_full =3D false;
> > -       }
> > -       return zswap_pool_reached_full;
> > -}
> > -
> >  /*********************************
> >  * param callbacks
> >  **********************************/
> > @@ -1475,6 +1469,8 @@ bool zswap_store(struct folio *folio)
> >         struct obj_cgroup *objcg =3D NULL;
> >         struct mem_cgroup *memcg =3D NULL;
> >         unsigned long value;
> > +       unsigned long cur_pages;
> > +       bool need_global_shrink =3D 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 =3D zswap_total_pages();
> > +
> > +       if (cur_pages >=3D zswap_max_pages()) {
> > +               zswap_pool_limit_hit++;
> > +               need_global_shrink =3D true;
> >                 goto reject;
> > +       }
> > +
> > +       /* schedule shrink for incoming pages */
> > +       if (cur_pages >=3D 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 =3D zswap_entry_cache_alloc(GFP_KERNEL, folio_nid(folio))=
;
> > @@ -1541,6 +1547,9 @@ bool zswap_store(struct folio *folio)
> >
> >                 WARN_ONCE(err !=3D -ENOMEM, "unexpected xarray error: %=
d\n", err);
> >                 zswap_reject_alloc_fail++;
> > +
> > +               /* reduce entry in array */
> > +               need_global_shrink =3D 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
> >


--=20

<flintglass@gmail.com>