2024-03-20 02:09:02

by Yosry Ahmed

[permalink] [raw]
Subject: [PATCH 1/2] mm: zswap: increase shrinking protection for zswap swapins only

Currently, the number of protected zswap entries corresponding to an
lruvec are incremented every time we swapin a page. This happens
regardless of whether or not the page originated in zswap. Hence,
swapins from disk will lead to increasing protection on potentially
stale zswap entries. Furthermore, the increased shrinking protection can
lead to more pages skipping zswap and going to disk, eventually leading
to even more swapins from disk and starting a vicious circle.

Instead, only increase the protection when pages are loaded from zswap.
This also has a nice side effect of removing zswap_folio_swapin() and
replacing it with a static helper that is only called from zswap_load().

No problems were observed in practice, this was found through code
inspection.

Signed-off-by: Yosry Ahmed <[email protected]>
---
include/linux/zswap.h | 2 --
mm/swap_state.c | 8 ++------
mm/zswap.c | 10 +++-------
3 files changed, 5 insertions(+), 15 deletions(-)

diff --git a/include/linux/zswap.h b/include/linux/zswap.h
index 2a85b941db975..1f020b5427e3d 100644
--- a/include/linux/zswap.h
+++ b/include/linux/zswap.h
@@ -34,7 +34,6 @@ int zswap_swapon(int type, unsigned long nr_pages);
void zswap_swapoff(int type);
void zswap_memcg_offline_cleanup(struct mem_cgroup *memcg);
void zswap_lruvec_state_init(struct lruvec *lruvec);
-void zswap_folio_swapin(struct folio *folio);
bool is_zswap_enabled(void);
#else

@@ -58,7 +57,6 @@ static inline int zswap_swapon(int type, unsigned long nr_pages)
static inline void zswap_swapoff(int type) {}
static inline void zswap_memcg_offline_cleanup(struct mem_cgroup *memcg) {}
static inline void zswap_lruvec_state_init(struct lruvec *lruvec) {}
-static inline void zswap_folio_swapin(struct folio *folio) {}

static inline bool is_zswap_enabled(void)
{
diff --git a/mm/swap_state.c b/mm/swap_state.c
index bfc7e8c58a6d3..32e151054ec47 100644
--- a/mm/swap_state.c
+++ b/mm/swap_state.c
@@ -696,10 +696,8 @@ struct folio *swap_cluster_readahead(swp_entry_t entry, gfp_t gfp_mask,
/* The page was likely read above, so no need for plugging here */
folio = __read_swap_cache_async(entry, gfp_mask, mpol, ilx,
&page_allocated, false);
- if (unlikely(page_allocated)) {
- zswap_folio_swapin(folio);
+ if (unlikely(page_allocated))
swap_read_folio(folio, false, NULL);
- }
return folio;
}

@@ -872,10 +870,8 @@ static struct folio *swap_vma_readahead(swp_entry_t targ_entry, gfp_t gfp_mask,
/* The folio was likely read above, so no need for plugging here */
folio = __read_swap_cache_async(targ_entry, gfp_mask, mpol, targ_ilx,
&page_allocated, false);
- if (unlikely(page_allocated)) {
- zswap_folio_swapin(folio);
+ if (unlikely(page_allocated))
swap_read_folio(folio, false, NULL);
- }
return folio;
}

diff --git a/mm/zswap.c b/mm/zswap.c
index b31c977f53e9c..323f1dea43d22 100644
--- a/mm/zswap.c
+++ b/mm/zswap.c
@@ -773,14 +773,9 @@ void zswap_lruvec_state_init(struct lruvec *lruvec)
atomic_long_set(&lruvec->zswap_lruvec_state.nr_zswap_protected, 0);
}

-void zswap_folio_swapin(struct folio *folio)
+static void zswap_lruvec_inc_protected(struct lruvec *lruvec)
{
- struct lruvec *lruvec;
-
- if (folio) {
- lruvec = folio_lruvec(folio);
- atomic_long_inc(&lruvec->zswap_lruvec_state.nr_zswap_protected);
- }
+ atomic_long_inc(&lruvec->zswap_lruvec_state.nr_zswap_protected);
}

void zswap_memcg_offline_cleanup(struct mem_cgroup *memcg)
@@ -1644,6 +1639,7 @@ bool zswap_load(struct folio *folio)
zswap_entry_free(entry);

folio_mark_dirty(folio);
+ zswap_lruvec_inc_protected(folio_lruvec(folio));

return true;
}
--
2.44.0.291.gc1ea87d7ee-goog



2024-03-20 02:10:03

by Yosry Ahmed

[permalink] [raw]
Subject: [PATCH 2/2] mm: zswap: remove nr_zswap_stored atomic

zswap_nr_stored is used to maintain the number of stored pages in zswap
that are not same-filled pages. It is used in zswap_shrinker_count() to
scale the number of freeable compressed pages by the compression ratio.
That is, to reduce the amount of writeback from zswap with higher
compression ratios as the ROI from IO diminishes.

However, the need for this counter is questionable due to two reasons:
- It is redundant. The value can be inferred from (zswap_stored_pages -
zswap_same_filled_pages).
- When memcgs are enabled, we use memcg_page_state(memcg,
MEMCG_ZSWAPPED), which includes same-filled pages anyway (i.e.
equivalent to zswap_stored_pages).

Use zswap_stored_pages instead in zswap_shrinker_count() to keep things
consistent whether memcgs are enabled or not, and add a comment about
the number of freeable pages possibly being scaled down more than it
should if we have lots of same-filled pages (i.e. inflated compression
ratio).

Remove nr_zswap_stored and one atomic operation in the store and free
paths.

Signed-off-by: Yosry Ahmed <[email protected]>
---
mm/zswap.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/mm/zswap.c b/mm/zswap.c
index 323f1dea43d22..ffcfce05a4408 100644
--- a/mm/zswap.c
+++ b/mm/zswap.c
@@ -181,8 +181,6 @@ struct zswap_pool {

/* Global LRU lists shared by all zswap pools. */
static struct list_lru zswap_list_lru;
-/* counter of pages stored in all zswap pools. */
-static atomic_t zswap_nr_stored = ATOMIC_INIT(0);

/* The lock protects zswap_next_shrink updates. */
static DEFINE_SPINLOCK(zswap_shrink_lock);
@@ -880,7 +878,6 @@ static void zswap_entry_free(struct zswap_entry *entry)
else {
zswap_lru_del(&zswap_list_lru, entry);
zpool_free(zswap_find_zpool(entry), entry->handle);
- atomic_dec(&zswap_nr_stored);
zswap_pool_put(entry->pool);
}
if (entry->objcg) {
@@ -1305,7 +1302,7 @@ static unsigned long zswap_shrinker_count(struct shrinker *shrinker,
#else
/* use pool stats instead of memcg stats */
nr_backing = zswap_total_pages();
- nr_stored = atomic_read(&zswap_nr_stored);
+ nr_stored = atomic_read(&zswap_stored_pages);
#endif

if (!nr_stored)
@@ -1325,6 +1322,11 @@ static unsigned long zswap_shrinker_count(struct shrinker *shrinker,
* This ensures that the better zswap compresses memory, the fewer
* pages we will evict to swap (as it will otherwise incur IO for
* relatively small memory saving).
+ *
+ * The memory saving factor calculated here takes same-filled pages into
+ * account, but those are not freeable since they almost occupy no
+ * space. Hence, we may scale nr_freeable down a little bit more than we
+ * should if we have a lot of same-filled pages.
*/
return mult_frac(nr_freeable, nr_backing, nr_stored);
}
@@ -1570,7 +1572,6 @@ bool zswap_store(struct folio *folio)
if (entry->length) {
INIT_LIST_HEAD(&entry->lru);
zswap_lru_add(&zswap_list_lru, entry);
- atomic_inc(&zswap_nr_stored);
}
spin_unlock(&tree->lock);

--
2.44.0.291.gc1ea87d7ee-goog


2024-03-20 09:51:26

by Johannes Weiner

[permalink] [raw]
Subject: Re: [PATCH 1/2] mm: zswap: increase shrinking protection for zswap swapins only

On Wed, Mar 20, 2024 at 02:08:22AM +0000, Yosry Ahmed wrote:
> Currently, the number of protected zswap entries corresponding to an
> lruvec are incremented every time we swapin a page.

Correct. This is the primary signal that the shrinker is being too
aggressive in moving entries to disk and should slow down...?

> This happens regardless of whether or not the page originated in
> zswap. Hence, swapins from disk will lead to increasing protection
> on potentially stale zswap entries. Furthermore, the increased
> shrinking protection can lead to more pages skipping zswap and going
> to disk, eventually leading to even more swapins from disk and
> starting a vicious circle.

How does shrinker protection affect zswap stores?

On the contrary, I would expect this patch to create a runaway
shrinker. The more aggressively it moves entries out to disk, the
lower the rate of zswap loads, the more aggressively it moves more
entries out to disk.

> Instead, only increase the protection when pages are loaded from zswap.
> This also has a nice side effect of removing zswap_folio_swapin() and
> replacing it with a static helper that is only called from zswap_load().
>
> No problems were observed in practice, this was found through code
> inspection.

This is missing test results :)

2024-03-20 14:48:33

by Nhat Pham

[permalink] [raw]
Subject: Re: [PATCH 1/2] mm: zswap: increase shrinking protection for zswap swapins only

On Wed, Mar 20, 2024 at 2:51 AM Johannes Weiner <[email protected]> wrote:
>
> On Wed, Mar 20, 2024 at 02:08:22AM +0000, Yosry Ahmed wrote:
> > Currently, the number of protected zswap entries corresponding to an
> > lruvec are incremented every time we swapin a page.
>
> Correct. This is the primary signal that the shrinker is being too
> aggressive in moving entries to disk and should slow down...?

Yup. Currently, there are two scenarios in which we increase zswap
protection area:

1. zswap lru movement - for instance, if a page for some reasons is
rotated in the LRU. When this happens, we increment the protection
size, so that the page at the tail end of the protected area does not
lose its protection because of (potentially) spurious LRU churnings.
2. swapin - when this happens, it is a signal that the shrinker is
being too... enthusiastic in its reclaiming action. We should be more
conservative, hence the increase in protection.

I think there's some confusion around this, because we use the
same-ish mechanism for two different events. Maybe I should have put
yet another fat comment at the callsites of zswap_folio_swapin() too
:)

>
> > This happens regardless of whether or not the page originated in
> > zswap. Hence, swapins from disk will lead to increasing protection
> > on potentially stale zswap entries. Furthermore, the increased

Hmmm my original thinking was that, had we protected the swapped-in
page back when it was still in zswap, we would have prevented this IO.
And since the pages in zswap are (at least conceptually) "warmer" than
the swapped in page, it is appropriate to increase the zswap
protection.

In fact, I was toying with the idea to max out the protection on
swap-in to temporarily cease all zswap shrinking, but that is perhaps
overkill :)

> > shrinking protection can lead to more pages skipping zswap and going

I think this can only happen when the protection is so strong that the
zswap pool is full, right?

In practice I have never seen this happen though. Did you observe it?
We have a fairly aggressive lru-size-based protection decaying as
well, so that should not happen...

Also technically the protection only applies to the dynamic shrinker -
the capacity-driven shrinking is not affected (although it's not very
effective - perhaps we can rework this?)

> > to disk, eventually leading to even more swapins from disk and
> > starting a vicious circle.
>
> How does shrinker protection affect zswap stores?
>
> On the contrary, I would expect this patch to create a runaway
> shrinker. The more aggressively it moves entries out to disk, the
> lower the rate of zswap loads, the more aggressively it moves more
> entries out to disk.
>
> > Instead, only increase the protection when pages are loaded from zswap.
> > This also has a nice side effect of removing zswap_folio_swapin() and
> > replacing it with a static helper that is only called from zswap_load().
> >
> > No problems were observed in practice, this was found through code
> > inspection.
>
> This is missing test results :)

Yeah it'd be nice to see benchmark numbers :)

I mean all this protection is heuristics anyway, so maybe I'm just
being overly conservative. But only numbers can show this.

2024-03-20 19:28:41

by Yosry Ahmed

[permalink] [raw]
Subject: Re: [PATCH 1/2] mm: zswap: increase shrinking protection for zswap swapins only

On Wed, Mar 20, 2024 at 05:50:53AM -0400, Johannes Weiner wrote:
> On Wed, Mar 20, 2024 at 02:08:22AM +0000, Yosry Ahmed wrote:
> > Currently, the number of protected zswap entries corresponding to an
> > lruvec are incremented every time we swapin a page.
>
> Correct. This is the primary signal that the shrinker is being too
> aggressive in moving entries to disk and should slow down...?

Right, I think that's where I was confused. See below :)

>
> > This happens regardless of whether or not the page originated in
> > zswap. Hence, swapins from disk will lead to increasing protection
> > on potentially stale zswap entries. Furthermore, the increased
> > shrinking protection can lead to more pages skipping zswap and going
> > to disk, eventually leading to even more swapins from disk and
> > starting a vicious circle.
>
> How does shrinker protection affect zswap stores?
>
> On the contrary, I would expect this patch to create a runaway
> shrinker. The more aggressively it moves entries out to disk, the
> lower the rate of zswap loads, the more aggressively it moves more
> entries out to disk.

I think I found the source of my confusion. As you described, the
intention of the protection is to detect that we are doing writeback
more aggressively than we should. This is indicated by swapins
(especially those from disk).

However, this assumes that pages swapped in from disk had gone there by
shrinking/writeback. What I was focused on was pages that end up on disk
because of the zswap limit. In this case, a disk swapin is actually a
sign that we swapped hot pages to disk instead of putting them in zswap,
which probably indicates that we should shrink zswap more aggressively
to make room for these pages.

I guess the general assumption is that with the shrinker at play, pages
skipping zswap due to the limit becomes the unlikely scenario -- which
makes sense. However, pages that end up on disk because they skipped
zswap should have a higher likelihood of being swapped in, because they
are hotter by definition.

Ideally, we would be able to tell during swapin if this page was sent
to disk due to writeback or skipping zswap and increase or decrease
protection accordingly -- but this isn't the case.

So perhaps the answer is to decrease the protection when pages skip
zswap instead? Or is this not necessary because we kick off a background
shrinker anyway? IIUC the latter will stop once we reach the acceptance
threshold, but it might be a good idea to increase shrinking beyond
that under memory pressure.

Also, in an ideal world I guess it would be better to distinguish
swapins from disk vs. zswap? Swapins from disk should lead to a bigger
increase in protection IIUC (assuming they happened due to writeback).

Sorry if my analysis is still off, I am still familiarizing myself with
the shrinker heuristics :)

>
> > Instead, only increase the protection when pages are loaded from zswap.
> > This also has a nice side effect of removing zswap_folio_swapin() and
> > replacing it with a static helper that is only called from zswap_load().
> >
> > No problems were observed in practice, this was found through code
> > inspection.
>
> This is missing test results :)

I intentionally wanted to send out the patch first to get some feedback,
knowing that I probably missed something. In retrospect, this should
have been an RFC patch. Patch 2 should be irrelevant tho, I only
bundled them because they touch the same area.

2024-03-20 19:33:06

by Yosry Ahmed

[permalink] [raw]
Subject: Re: [PATCH 1/2] mm: zswap: increase shrinking protection for zswap swapins only

On Wed, Mar 20, 2024 at 07:48:13AM -0700, Nhat Pham wrote:
> On Wed, Mar 20, 2024 at 2:51 AM Johannes Weiner <[email protected]> wrote:
> >
> > On Wed, Mar 20, 2024 at 02:08:22AM +0000, Yosry Ahmed wrote:
> > > Currently, the number of protected zswap entries corresponding to an
> > > lruvec are incremented every time we swapin a page.
> >
> > Correct. This is the primary signal that the shrinker is being too
> > aggressive in moving entries to disk and should slow down...?
>
> Yup. Currently, there are two scenarios in which we increase zswap
> protection area:
>
> 1. zswap lru movement - for instance, if a page for some reasons is
> rotated in the LRU. When this happens, we increment the protection
> size, so that the page at the tail end of the protected area does not
> lose its protection because of (potentially) spurious LRU churnings.

I don't think we update the protected area during rotations anymore,
only when a new entry is added to the LRU (with potential decay).

> 2. swapin - when this happens, it is a signal that the shrinker is
> being too... enthusiastic in its reclaiming action. We should be more
> conservative, hence the increase in protection.
>
> I think there's some confusion around this, because we use the
> same-ish mechanism for two different events. Maybe I should have put
> yet another fat comment at the callsites of zswap_folio_swapin() too
> :)

I think it makes sense. The confusion was mostly around the
interpretation of finding a page on disk. I was focused on pages that
skip zswap, but the main intention was for pages that were written back,
as I explained in my response to Johannes.

>
> >
> > > This happens regardless of whether or not the page originated in
> > > zswap. Hence, swapins from disk will lead to increasing protection
> > > on potentially stale zswap entries. Furthermore, the increased
>
> Hmmm my original thinking was that, had we protected the swapped-in
> page back when it was still in zswap, we would have prevented this IO.
> And since the pages in zswap are (at least conceptually) "warmer" than
> the swapped in page, it is appropriate to increase the zswap
> protection.
>
> In fact, I was toying with the idea to max out the protection on
> swap-in to temporarily cease all zswap shrinking, but that is perhaps
> overkill :)

This would be problematic for pages that skip zswap IIUC, we'd want more
shrinking in this case.

>
> > > shrinking protection can lead to more pages skipping zswap and going
>
> I think this can only happen when the protection is so strong that the
> zswap pool is full, right?

Yeah that's what I had in mind.

>
> In practice I have never seen this happen though. Did you observe it?
> We have a fairly aggressive lru-size-based protection decaying as
> well, so that should not happen...

No this was all code inspection as I mentioned :)

>
> Also technically the protection only applies to the dynamic shrinker -
> the capacity-driven shrinking is not affected (although it's not very
> effective - perhaps we can rework this?)

That logic is flawed anyway imo due to the acceptance threshold. We
should really get rid of that as we discussed before.

2024-03-21 21:10:41

by Yosry Ahmed

[permalink] [raw]
Subject: Re: [PATCH 2/2] mm: zswap: remove nr_zswap_stored atomic

On Tue, Mar 19, 2024 at 7:08 PM Yosry Ahmed <[email protected]> wrote:
>
> zswap_nr_stored is used to maintain the number of stored pages in zswap
> that are not same-filled pages. It is used in zswap_shrinker_count() to
> scale the number of freeable compressed pages by the compression ratio.
> That is, to reduce the amount of writeback from zswap with higher
> compression ratios as the ROI from IO diminishes.
>
> However, the need for this counter is questionable due to two reasons:
> - It is redundant. The value can be inferred from (zswap_stored_pages -
> zswap_same_filled_pages).
> - When memcgs are enabled, we use memcg_page_state(memcg,
> MEMCG_ZSWAPPED), which includes same-filled pages anyway (i.e.
> equivalent to zswap_stored_pages).
>
> Use zswap_stored_pages instead in zswap_shrinker_count() to keep things
> consistent whether memcgs are enabled or not, and add a comment about
> the number of freeable pages possibly being scaled down more than it
> should if we have lots of same-filled pages (i.e. inflated compression
> ratio).
>
> Remove nr_zswap_stored and one atomic operation in the store and free
> paths.
>
> Signed-off-by: Yosry Ahmed <[email protected]>

Any thoughts on this patch? Should I resend it separately?

> ---
> mm/zswap.c | 11 ++++++-----
> 1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/mm/zswap.c b/mm/zswap.c
> index 323f1dea43d22..ffcfce05a4408 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -181,8 +181,6 @@ struct zswap_pool {
>
> /* Global LRU lists shared by all zswap pools. */
> static struct list_lru zswap_list_lru;
> -/* counter of pages stored in all zswap pools. */
> -static atomic_t zswap_nr_stored = ATOMIC_INIT(0);
>
> /* The lock protects zswap_next_shrink updates. */
> static DEFINE_SPINLOCK(zswap_shrink_lock);
> @@ -880,7 +878,6 @@ static void zswap_entry_free(struct zswap_entry *entry)
> else {
> zswap_lru_del(&zswap_list_lru, entry);
> zpool_free(zswap_find_zpool(entry), entry->handle);
> - atomic_dec(&zswap_nr_stored);
> zswap_pool_put(entry->pool);
> }
> if (entry->objcg) {
> @@ -1305,7 +1302,7 @@ static unsigned long zswap_shrinker_count(struct shrinker *shrinker,
> #else
> /* use pool stats instead of memcg stats */
> nr_backing = zswap_total_pages();
> - nr_stored = atomic_read(&zswap_nr_stored);
> + nr_stored = atomic_read(&zswap_stored_pages);
> #endif
>
> if (!nr_stored)
> @@ -1325,6 +1322,11 @@ static unsigned long zswap_shrinker_count(struct shrinker *shrinker,
> * This ensures that the better zswap compresses memory, the fewer
> * pages we will evict to swap (as it will otherwise incur IO for
> * relatively small memory saving).
> + *
> + * The memory saving factor calculated here takes same-filled pages into
> + * account, but those are not freeable since they almost occupy no
> + * space. Hence, we may scale nr_freeable down a little bit more than we
> + * should if we have a lot of same-filled pages.
> */
> return mult_frac(nr_freeable, nr_backing, nr_stored);
> }
> @@ -1570,7 +1572,6 @@ bool zswap_store(struct folio *folio)
> if (entry->length) {
> INIT_LIST_HEAD(&entry->lru);
> zswap_lru_add(&zswap_list_lru, entry);
> - atomic_inc(&zswap_nr_stored);
> }
> spin_unlock(&tree->lock);
>
> --
> 2.44.0.291.gc1ea87d7ee-goog
>

2024-03-21 23:50:29

by Nhat Pham

[permalink] [raw]
Subject: Re: [PATCH 2/2] mm: zswap: remove nr_zswap_stored atomic

On Thu, Mar 21, 2024 at 2:09 PM Yosry Ahmed <[email protected]> wrote:
>
> On Tue, Mar 19, 2024 at 7:08 PM Yosry Ahmed <[email protected]> wrote:
> >
> > zswap_nr_stored is used to maintain the number of stored pages in zswap
> > that are not same-filled pages. It is used in zswap_shrinker_count() to
> > scale the number of freeable compressed pages by the compression ratio.
> > That is, to reduce the amount of writeback from zswap with higher
> > compression ratios as the ROI from IO diminishes.
> >
> > However, the need for this counter is questionable due to two reasons:
> > - It is redundant. The value can be inferred from (zswap_stored_pages -
> > zswap_same_filled_pages).

Ah, I forgot about this. For context, nr_stored was originally a
zswap_pool-specific stat, but I think Chengming has pulled it out and
converted it into a global pool stat in an earlier patch - yet,
globally, we already have zswap_stored_pages that is (mostly) the same
counter.

Might as well use existing counters (zswap_stored_pages) then, rather
than a newly introduced counter. Probably will shave off a couple
cycles here and there for the atomic increment/decrement :)

> > - When memcgs are enabled, we use memcg_page_state(memcg,
> > MEMCG_ZSWAPPED), which includes same-filled pages anyway (i.e.
> > equivalent to zswap_stored_pages).

This is fine I suppose. I was aware of this weird inaccuracy. However,
for the CONFIG_MEMCG case, it was kinda silly to introduce the counter
for per-cgroup same filled zswap pages, just for this one purpose, so
I decided to accept the inaccuracy.

> >
> > Use zswap_stored_pages instead in zswap_shrinker_count() to keep things
> > consistent whether memcgs are enabled or not, and add a comment about
> > the number of freeable pages possibly being scaled down more than it
> > should if we have lots of same-filled pages (i.e. inflated compression
> > ratio).
> >
> > Remove nr_zswap_stored and one atomic operation in the store and free
> > paths.
> >
> > Signed-off-by: Yosry Ahmed <[email protected]>
>
> Any thoughts on this patch? Should I resend it separately?

Might be worth resending it separately, but up to you and Andrew!

Reviewed-by: Nhat Pham <[email protected]>

2024-03-21 23:58:13

by Yosry Ahmed

[permalink] [raw]
Subject: Re: [PATCH 2/2] mm: zswap: remove nr_zswap_stored atomic

On Thu, Mar 21, 2024 at 4:50 PM Nhat Pham <[email protected]> wrote:
>
> On Thu, Mar 21, 2024 at 2:09 PM Yosry Ahmed <[email protected]> wrote:
> >
> > On Tue, Mar 19, 2024 at 7:08 PM Yosry Ahmed <[email protected]> wrote:
> > >
> > > zswap_nr_stored is used to maintain the number of stored pages in zswap
> > > that are not same-filled pages. It is used in zswap_shrinker_count() to
> > > scale the number of freeable compressed pages by the compression ratio.
> > > That is, to reduce the amount of writeback from zswap with higher
> > > compression ratios as the ROI from IO diminishes.
> > >
> > > However, the need for this counter is questionable due to two reasons:
> > > - It is redundant. The value can be inferred from (zswap_stored_pages -
> > > zswap_same_filled_pages).
>
> Ah, I forgot about this. For context, nr_stored was originally a
> zswap_pool-specific stat, but I think Chengming has pulled it out and
> converted it into a global pool stat in an earlier patch - yet,
> globally, we already have zswap_stored_pages that is (mostly) the same
> counter.

Thanks for the context.

>
> Might as well use existing counters (zswap_stored_pages) then, rather
> than a newly introduced counter. Probably will shave off a couple
> cycles here and there for the atomic increment/decrement :)
>
> > > - When memcgs are enabled, we use memcg_page_state(memcg,
> > > MEMCG_ZSWAPPED), which includes same-filled pages anyway (i.e.
> > > equivalent to zswap_stored_pages).
>
> This is fine I suppose. I was aware of this weird inaccuracy. However,
> for the CONFIG_MEMCG case, it was kinda silly to introduce the counter
> for per-cgroup same filled zswap pages, just for this one purpose, so
> I decided to accept the inaccuracy.
>
> > >
> > > Use zswap_stored_pages instead in zswap_shrinker_count() to keep things
> > > consistent whether memcgs are enabled or not, and add a comment about
> > > the number of freeable pages possibly being scaled down more than it
> > > should if we have lots of same-filled pages (i.e. inflated compression
> > > ratio).
> > >
> > > Remove nr_zswap_stored and one atomic operation in the store and free
> > > paths.
> > >
> > > Signed-off-by: Yosry Ahmed <[email protected]>
> >
> > Any thoughts on this patch? Should I resend it separately?
>
> Might be worth resending it separately, but up to you and Andrew!

I will resend to add some context and include your R-b, thanks.

>
> Reviewed-by: Nhat Pham <[email protected]>