zswap: reject to compress/store page if zswap_max_pool_percent is 0

The '/sys/../zswap/stored_pages:' keep raising in zswap test with "zswap.max_pool_percent=0" 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=1 zswap.max_pool_percent=17"
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 pages again. Then we get fails on that as above.

Signed-off-by Li Wang <[email protected]>

Cc Seth Jennings <[email protected]>

Cc Dan Streetman <[email protected]>

Cc Huang Ying <[email protected]>

Cc Yu Zhao <[email protected]>

---
 mm/zswap.c | 5 +++++
 1 file changed, 5 insertions(+)

mm/zswap.c +5 -1

index 61a5c41..2b537bb 100644
@@ -1007,6 +1007,11 @@ static int zswap_frontswap_store(unsigned type, pgoff_t offset,
        u8 *src, *dst;
        struct zswap_header zhdr = { .swpentry = swp_entry(type, offset) };
 
+       if (!zswap_max_pool_percent) {
+               ret = -ENOMEM;
+               goto reject;
+       }
+
        /* THP isn't supported */
        if (PageTransHuge(page)) {
                ret = -EINVAL;
--

2.9.5

Quoted 17 lines · Li Wang

The '/sys/../zswap/stored_pages:' keep raising in zswap test with "zswap.max_pool_percent=0" 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=1 zswap.max_pool_percent=17"
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

Quoted 3 lines

The root cause is:

When the zswap_max_pool_percent is set to 0 via kernel parameter, the

zswap_is_full()

Quoted 1 line

will always return true to shrink the pool size by zswap_shrink(). If

the pool size

Quoted 1 line

has been shrinked a little success, zswap will do compress/store pages

again. Then we

Quoted 1 line

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.

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).

Hi Dan,

Quoted 29 lines · Dan Streetman

Quoted 19 lines · Li Wang

The '/sys/../zswap/stored_pages:' keep raising in zswap test with "zswap.max_pool_percent=0" 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=1 zswap.max_pool_percent=17"
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:

Quoted 7 lines

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 pages
again. Then we
get fails on that as above.

Quoted 2 lines

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.

Sorry 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.

Another consideration from me is:

[Method A]

mm/zswap.c +1 -1

@@ -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 = -ENOMEM;

                        goto reject;

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

Quoted 9 lines

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).

Ok, it sounds like can also fix the issue. The changes maybe like:

[Method B]

mm/zswap.c +9 -0

@@ -1026,6 +1026,15 @@ static int zswap_frontswap_store(unsigned type,
 pgoff_t offset,
                        ret = -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 = -ENOMEM;
+                       goto reject;
+               }
        }
 

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

--
Regards,
Li Wang