From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ua0-f198.google.com (mail-ua0-f198.google.com [209.85.217.198]) by kanga.kvack.org (Postfix) with ESMTP id 5802B6B0003 for ; Tue, 29 May 2018 22:57:29 -0400 (EDT) Received: by mail-ua0-f198.google.com with SMTP id u23-v6so11004271ual.4 for ; Tue, 29 May 2018 19:57:29 -0700 (PDT) Received: from mail-sor-f65.google.com (mail-sor-f65.google.com. [209.85.220.65]) by mx.google.com with SMTPS id j62-v6sor3288488vkd.287.2018.05.29.19.57.27 for (Google Transport Security); Tue, 29 May 2018 19:57:27 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: References: <20180524095752.17770-1-liwang@redhat.com> From: Li Wang Date: Wed, 30 May 2018 10:57:26 +0800 Message-ID: Subject: Re: [PATCH RFC] zswap: reject to compress/store page if zswap_max_pool_percent is 0 Content-Type: multipart/alternative; boundary="000000000000d1a445056d63823e" Sender: owner-linux-mm@kvack.org List-ID: To: Dan Streetman Cc: Linux-MM , linux-kernel , Seth Jennings , Huang Ying , Yu Zhao --000000000000d1a445056d63823e Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hi Dan, On Wed, May 30, 2018 at 5:14 AM, Dan Streetman wrote: > On Thu, May 24, 2018 at 5:57 AM, Li Wang wrote: > > The '/sys/../zswap/stored_pages:' keep raising in zswap test with > > "zswap.max_pool_percent=3D0" parameter. But theoretically, it should > > not compress or store pages any more since there is no space for > > compressed pool. > > > > Reproduce steps: > > > > 1. Boot kernel with "zswap.enabled=3D1 zswap.max_pool_percent=3D17" > > 2. Set the max_pool_percent to 0 > > # echo 0 > /sys/module/zswap/parameters/max_pool_percent > > Confirm this parameter works fine > > # cat /sys/kernel/debug/zswap/pool_total_size > > 0 > > 3. Do memory stress test to see if some pages have been compressed > > # stress --vm 1 --vm-bytes $mem_available"M" --timeout 60s > > Watching the 'stored_pages' numbers increasing or not > > > > The root cause is: > > > > When the zswap_max_pool_percent is set to 0 via kernel parameter, the > zswap_is_full() > > will always return true to shrink the pool size by zswap_shrink(). If > the pool size > > has been shrinked a little success, zswap will do compress/store page= s > again. Then we > > get fails on that as above. > > special casing 0% doesn't make a lot of sense to me, and I'm not > entirely sure what exactly you are trying to fix here. > =E2=80=8BSorry for that confusing, I am a pretty new to zswap. To specify 0 to max_pool_percent is purpose to verify if zswap stopping work when there is no space in compressed pool.=E2=80=8B Another consideration from me is: [Method A] --- a/mm/zswap.c +++ b/mm/zswap.c @@ -1021,7 +1021,7 @@ static int zswap_frontswap_store(unsigned type, pgoff_t offset, /* reclaim space if needed */ if (zswap_is_full()) { zswap_pool_limit_hit++; - if (zswap_shrink()) { + if (!zswap_max_pool_percent || zswap_shrink()) { zswap_reject_reclaim_fail++; ret =3D -ENOMEM; goto reject; This make sure the compressed pool is enough to do zswap_shrink(). > > however, zswap does currently do a zswap_is_full() check, and then if > it's able to reclaim a page happily proceeds to store another page, > without re-checking zswap_is_full(). If you're trying to fix that, > then I would ack a patch that adds a second zswap_is_full() check > after zswap_shrink() to make sure it's now under the max_pool_percent > (or somehow otherwise fixes that behavior). > > =E2=80=8BOk, it sounds like can also fix the issue. The changes maybe like: [Method B] --- a/mm/zswap.c +++ b/mm/zswap.c @@ -1026,6 +1026,15 @@ static int zswap_frontswap_store(unsigned type, pgoff_t offset, ret =3D -ENOMEM; goto reject; } + + /* A second zswap_is_full() check after + * zswap_shrink() to make sure it's now + * under the max_pool_percent + */ + if (zswap_is_full()) { + ret =3D -ENOMEM; + goto reject; + } } So, which one do you think is better, A or B? --=20 Regards, Li Wang --000000000000d1a445056d63823e Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi = Dan,

On = Wed, May 30, 2018 at 5:14 AM, Dan Streetman <ddstreet@ieee.org> wrote:
On Thu, M= ay 24, 2018 at 5:57 AM, Li Wang <li= wang@redhat.com> wrote:
> The '/sys/../zswap/stored_pages:' keep raising in zswap test w= ith
> "zswap.max_pool_percent=3D0" parameter. But theoretically, i= t should
> not compress or store pages any more since there is no space for
> compressed pool.
>
> Reproduce steps:
>
>=C2=A0 =C2=A01. Boot kernel with "zswap.enabled=3D1 zswap.max_pool= _percent=3D17"
>=C2=A0 =C2=A02. Set the max_pool_percent to 0
>=C2=A0 =C2=A0 =C2=A0 =C2=A0# echo 0 > /sys/module/zswap/parameters/<= wbr>max_pool_percent
>=C2=A0 =C2=A0 =C2=A0 Confirm this parameter works fine
>=C2=A0 =C2=A0 =C2=A0 =C2=A0# cat /sys/kernel/debug/zswap/pool_tota= l_size
>=C2=A0 =C2=A0 =C2=A0 =C2=A00
>=C2=A0 =C2=A03. Do memory stress test to see if some pages have been co= mpressed
>=C2=A0 =C2=A0 =C2=A0 =C2=A0# stress --vm 1 --vm-bytes $mem_available&qu= ot;M" --timeout 60s
>=C2=A0 =C2=A0 =C2=A0 Watching the 'stored_pages' numbers increa= sing or not
>
> The root cause is:
>
>=C2=A0 =C2=A0When the zswap_max_pool_percent is set to 0 via kernel par= ameter, the zswap_is_full()
>=C2=A0 =C2=A0will always return true to shrink the pool size by zswap_s= hrink(). If the pool size
>=C2=A0 =C2=A0has been shrinked a little success, zswap will do compress= /store pages again. Then we
>=C2=A0 =C2=A0get fails on that as above.

special casing 0% doesn't make a lot of sense to me, and I'm not entirely sure what exactly you are trying to fix here.

=E2= =80=8BSorry for that confusing, I am a pretty new to zswap.

To specify 0 to max_pool_percent is purp= ose to verify if zswap stopping work when there is no space in compressed p= ool.=E2=80=8B
<= br>
Another con= sideration from me is:

[M= ethod A]

--- a/mm/zswap.c=
+++ b/mm/zswap.c
@@ -1021,7 +1021,7 @@ static int zswap_frontswap_st= ore(unsigned type, pgoff_t offset,
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0 /* reclaim space if needed */
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0 if (zswap_is_full()) {
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 zswap_pool_limit_hit++;=
-=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0 if (zswap_shrink()) {
+=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (!zswap_max_pool_per= cent || zswap_shrink()) {
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0 zswap_reject_reclaim_fail++;
=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ret =3D -ENOMEM;
=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 goto reject;

This make sure the compressed pool= is enough to do zswap_shrink().

=C2=A0

however, zswap does currently do a zswap_is_full() check, and then if
it's able to reclaim a page happily proceeds to store another page,
without re-checking zswap_is_full().=C2=A0 If you're trying to fix that= ,
then I would ack a patch that adds a second zswap_is_full() check
after zswap_shrink() to make sure it's now under the max_pool_percent (or somehow otherwise fixes that behavior).


= =E2=80=8BOk, it sounds like can also fix the issue. The changes maybe like:=

=
[Method B]

--- a/mm/zswap.c
+++ b/mm/zswap= .c
@@ -1026,6 +1026,15 @@ static int zswap_frontswap_store(unsigned type= , pgoff_t offset,
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0 ret =3D -ENOMEM;
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0 goto reject;
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 }
+
+=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 /*= A second zswap_is_full() check after
+=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * zswap_shrink() = to make sure it's now
+=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * under the max_pool_percent<= br>+=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0 */
+=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (zswap_is_full()) {
+=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ret =3D -ENOMEM;
+= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 goto reject;
+= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0 }
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 }


So, which one do you think is = better, A or B?

--
Regards,
Li Wa= ng
--000000000000d1a445056d63823e--