Miscellaneous cleanups for limit checking and same-filled handling in
the store path. This series was broken out of the "zswap: store
zero-filled pages more efficiently" series [1]. It contains the cleanups
and drops the main functional changes.
[1]https://lore.kernel.org/lkml/[email protected]/
Yosry Ahmed (5):
mm: zswap: always shrink in zswap_store() if zswap_pool_reached_full
mm: zswap: refactor limit checking from zswap_store()
mm: zswap: move more same-filled pages checks outside of zswap_store()
mm: zswap: remove same_filled module params
mm: zswap: do not check the global limit for same-filled pages
mm/zswap.c | 93 +++++++++++++++++++++---------------------------------
1 file changed, 36 insertions(+), 57 deletions(-)
--
2.44.0.478.gd926399ef9-goog
The cleanup code in zswap_store() is not pretty, particularly the
'shrink' label at the bottom that ends up jumping between cleanup
labels.
Instead of having a dedicated label to shrink the pool, just use
zswap_pool_reached_full directly to figure out if the pool needs
shrinking. zswap_pool_reached_full should be true if and only if the
pool needs shrinking.
The only caveat is that the value of zswap_pool_reached_full may be
changed by concurrent zswap_store() calls between checking the limit and
testing zswap_pool_reached_full in the cleanup code. This is fine
because:
- If zswap_pool_reached_full was true during limit checking then became
false during the cleanup code, then someone else already took care of
shrinking the pool and there is no need to queue the worker. That
would be a good change.
- If zswap_pool_reached_full was false during limit checking then became
true during the cleanup code, then someone else hit the limit
meanwhile. In this case, both threads will try to queue the worker,
but it never gets queued more than once anyway. Also, calling
queue_work() multiple times when the limit is hit could already happen
today, so this isn't a significant change in any way.
Signed-off-by: Yosry Ahmed <[email protected]>
Reviewed-by: Nhat Pham <[email protected]>
Reviewed-by: Chengming Zhou <[email protected]>
Acked-by: Johannes Weiner <[email protected]>
---
mm/zswap.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/mm/zswap.c b/mm/zswap.c
index c4979c76d58e3..1cf3ab4b22e64 100644
--- a/mm/zswap.c
+++ b/mm/zswap.c
@@ -1429,12 +1429,12 @@ bool zswap_store(struct folio *folio)
if (cur_pages >= max_pages) {
zswap_pool_limit_hit++;
zswap_pool_reached_full = true;
- goto shrink;
+ goto reject;
}
if (zswap_pool_reached_full) {
if (cur_pages > zswap_accept_thr_pages())
- goto shrink;
+ goto reject;
else
zswap_pool_reached_full = false;
}
@@ -1540,6 +1540,8 @@ bool zswap_store(struct folio *folio)
zswap_entry_cache_free(entry);
reject:
obj_cgroup_put(objcg);
+ if (zswap_pool_reached_full)
+ queue_work(shrink_wq, &zswap_shrink_work);
check_old:
/*
* If the zswap store fails or zswap is disabled, we must invalidate the
@@ -1550,10 +1552,6 @@ bool zswap_store(struct folio *folio)
if (entry)
zswap_entry_free(entry);
return false;
-
-shrink:
- queue_work(shrink_wq, &zswap_shrink_work);
- goto reject;
}
bool zswap_load(struct folio *folio)
--
2.44.0.478.gd926399ef9-goog
Refactor limit and acceptance threshold checking outside of
zswap_store(). This code will be moved around in a following patch, so
it would be cleaner to move a function call around.
Signed-off-by: Yosry Ahmed <[email protected]>
---
mm/zswap.c | 32 ++++++++++++++++----------------
1 file changed, 16 insertions(+), 16 deletions(-)
diff --git a/mm/zswap.c b/mm/zswap.c
index 1cf3ab4b22e64..fba8f3c3596ab 100644
--- a/mm/zswap.c
+++ b/mm/zswap.c
@@ -1391,6 +1391,21 @@ static void zswap_fill_page(void *ptr, unsigned long value)
memset_l(page, value, PAGE_SIZE / sizeof(unsigned long));
}
+static bool zswap_check_full(void)
+{
+ unsigned long cur_pages = zswap_total_pages();
+ unsigned long thr = zswap_accept_thr_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 <= thr) {
+ zswap_pool_reached_full = false;
+ }
+ return zswap_pool_reached_full;
+}
+
bool zswap_store(struct folio *folio)
{
swp_entry_t swp = folio->swap;
@@ -1399,7 +1414,6 @@ bool zswap_store(struct folio *folio)
struct zswap_entry *entry, *old;
struct obj_cgroup *objcg = NULL;
struct mem_cgroup *memcg = NULL;
- unsigned long max_pages, cur_pages;
VM_WARN_ON_ONCE(!folio_test_locked(folio));
VM_WARN_ON_ONCE(!folio_test_swapcache(folio));
@@ -1422,22 +1436,8 @@ bool zswap_store(struct folio *folio)
mem_cgroup_put(memcg);
}
- /* Check global limits */
- cur_pages = zswap_total_pages();
- max_pages = zswap_max_pages();
-
- if (cur_pages >= max_pages) {
- zswap_pool_limit_hit++;
- zswap_pool_reached_full = true;
+ if (zswap_check_full())
goto reject;
- }
-
- if (zswap_pool_reached_full) {
- if (cur_pages > zswap_accept_thr_pages())
- goto reject;
- else
- zswap_pool_reached_full = false;
- }
/* allocate entry */
entry = zswap_entry_cache_alloc(GFP_KERNEL, folio_nid(folio));
--
2.44.0.478.gd926399ef9-goog
These knobs offer more fine-grained control to userspace than needed and
directly expose/influence kernel implementation; remove them.
For disabling same_filled handling, there is no logical reason to refuse
storing same-filled pages more efficiently and opt for compression.
Scanning pages for patterns may be an argument, but the page contents
will be read into the CPU cache anyway during compression. Also,
removing the same_filled handling code does not move the needle
significantly in terms of performance anyway [1].
For disabling non_same_filled handling, it was added when the compressed
pages in zswap were not being properly charged to memcgs, as workloads
could escape the accounting with compression [2]. This is no longer the
case after commit f4840ccfca25 ("zswap: memcg accounting"), and using
zswap without compression does not make much sense.
[1]https://lore.kernel.org/lkml/CAJD7tkaySFP2hBQw4pnZHJJwe3bMdjJ1t9VC2VJd=khn1_TXvA@mail.gmail.com/
[2]https://lore.kernel.org/lkml/[email protected]/
Signed-off-by: Yosry Ahmed <[email protected]>
---
mm/zswap.c | 19 -------------------
1 file changed, 19 deletions(-)
diff --git a/mm/zswap.c b/mm/zswap.c
index b92fa37bee277..a85c9235d19d3 100644
--- a/mm/zswap.c
+++ b/mm/zswap.c
@@ -123,19 +123,6 @@ static unsigned int zswap_accept_thr_percent = 90; /* of max pool size */
module_param_named(accept_threshold_percent, zswap_accept_thr_percent,
uint, 0644);
-/*
- * Enable/disable handling same-value filled pages (enabled by default).
- * If disabled every page is considered non-same-value filled.
- */
-static bool zswap_same_filled_pages_enabled = true;
-module_param_named(same_filled_pages_enabled, zswap_same_filled_pages_enabled,
- bool, 0644);
-
-/* Enable/disable handling non-same-value filled pages (enabled by default) */
-static bool zswap_non_same_filled_pages_enabled = true;
-module_param_named(non_same_filled_pages_enabled, zswap_non_same_filled_pages_enabled,
- bool, 0644);
-
/* Number of zpools in zswap_pool (empirically determined for scalability) */
#define ZSWAP_NR_ZPOOLS 32
@@ -1368,9 +1355,6 @@ static bool zswap_is_folio_same_filled(struct folio *folio, unsigned long *value
unsigned int pos, last_pos = PAGE_SIZE / sizeof(*page) - 1;
bool ret = false;
- if (!zswap_same_filled_pages_enabled)
- return false;
-
page = kmap_local_folio(folio, 0);
val = page[0];
@@ -1460,9 +1444,6 @@ bool zswap_store(struct folio *folio)
goto store_entry;
}
- if (!zswap_non_same_filled_pages_enabled)
- goto freepage;
-
/* if entry is successfully added, it keeps the reference */
entry->pool = zswap_pool_current_get();
if (!entry->pool)
--
2.44.0.478.gd926399ef9-goog
When storing same-filled pages, there is no point of checking the global
zswap limit as storing them does not contribute toward the limit Move
the limit checking after same-filled pages are handled.
This avoids having same-filled pages skip zswap and go to disk swap if
the limit is hit. It also avoids queueing the shrink worker, which may
end up being unnecessary if the zswap usage goes down on its own before
another store is attempted.
Ignoring the memcg limits as well for same-filled pages is more
controversial. Those limits are more a matter of per-workload policy.
Some workloads disable zswap completely by setting memory.zswap.max = 0,
and those workloads could start observing some zswap activity even after
disabling zswap. Although harmless, this could cause confusion to
userspace. Remain conservative and keep respecting those limits.
Signed-off-by: Yosry Ahmed <[email protected]>
---
mm/zswap.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/mm/zswap.c b/mm/zswap.c
index a85c9235d19d3..8763a1e938441 100644
--- a/mm/zswap.c
+++ b/mm/zswap.c
@@ -1404,6 +1404,7 @@ bool zswap_store(struct folio *folio)
struct zswap_entry *entry, *old;
struct obj_cgroup *objcg = NULL;
struct mem_cgroup *memcg = NULL;
+ bool same_filled = false;
unsigned long value;
VM_WARN_ON_ONCE(!folio_test_locked(folio));
@@ -1427,7 +1428,8 @@ bool zswap_store(struct folio *folio)
mem_cgroup_put(memcg);
}
- if (zswap_check_full())
+ same_filled = zswap_is_folio_same_filled(folio, &value);
+ if (!same_filled && zswap_check_full())
goto reject;
/* allocate entry */
@@ -1437,7 +1439,7 @@ bool zswap_store(struct folio *folio)
goto reject;
}
- if (zswap_is_folio_same_filled(folio, &value)) {
+ if (same_filled) {
entry->length = 0;
entry->value = value;
atomic_inc(&zswap_same_filled_pages);
--
2.44.0.478.gd926399ef9-goog
Currently, zswap_store() checks zswap_same_filled_pages_enabled, kmaps
the folio, then calls zswap_is_page_same_filled() to check the folio
contents. Move this logic into zswap_is_page_same_filled() as well (and
rename it to use 'folio' while we are at it).
This makes zswap_store() cleaner, and makes following changes to that
logic contained within the helper. While we are at it, rename the
insert_entry label to store_entry to match xa_store().
No functional change intended.
Signed-off-by: Yosry Ahmed <[email protected]>
Reviewed-by: Nhat Pham <[email protected]>
Reviewed-by: Chengming Zhou <[email protected]>
---
mm/zswap.c | 39 +++++++++++++++++++--------------------
1 file changed, 19 insertions(+), 20 deletions(-)
diff --git a/mm/zswap.c b/mm/zswap.c
index fba8f3c3596ab..b92fa37bee277 100644
--- a/mm/zswap.c
+++ b/mm/zswap.c
@@ -1361,26 +1361,32 @@ static void shrink_worker(struct work_struct *w)
} while (zswap_total_pages() > thr);
}
-static int zswap_is_page_same_filled(void *ptr, unsigned long *value)
+static bool zswap_is_folio_same_filled(struct folio *folio, unsigned long *value)
{
unsigned long *page;
unsigned long val;
unsigned int pos, last_pos = PAGE_SIZE / sizeof(*page) - 1;
+ bool ret = false;
- page = (unsigned long *)ptr;
+ if (!zswap_same_filled_pages_enabled)
+ return false;
+
+ page = kmap_local_folio(folio, 0);
val = page[0];
if (val != page[last_pos])
- return 0;
+ goto out;
for (pos = 1; pos < last_pos; pos++) {
if (val != page[pos])
- return 0;
+ goto out;
}
*value = val;
-
- return 1;
+ ret = true;
+out:
+ kunmap_local(page);
+ return ret;
}
static void zswap_fill_page(void *ptr, unsigned long value)
@@ -1414,6 +1420,7 @@ bool zswap_store(struct folio *folio)
struct zswap_entry *entry, *old;
struct obj_cgroup *objcg = NULL;
struct mem_cgroup *memcg = NULL;
+ unsigned long value;
VM_WARN_ON_ONCE(!folio_test_locked(folio));
VM_WARN_ON_ONCE(!folio_test_swapcache(folio));
@@ -1446,19 +1453,11 @@ bool zswap_store(struct folio *folio)
goto reject;
}
- if (zswap_same_filled_pages_enabled) {
- unsigned long value;
- u8 *src;
-
- src = kmap_local_folio(folio, 0);
- if (zswap_is_page_same_filled(src, &value)) {
- kunmap_local(src);
- entry->length = 0;
- entry->value = value;
- atomic_inc(&zswap_same_filled_pages);
- goto insert_entry;
- }
- kunmap_local(src);
+ if (zswap_is_folio_same_filled(folio, &value)) {
+ entry->length = 0;
+ entry->value = value;
+ atomic_inc(&zswap_same_filled_pages);
+ goto store_entry;
}
if (!zswap_non_same_filled_pages_enabled)
@@ -1481,7 +1480,7 @@ bool zswap_store(struct folio *folio)
if (!zswap_compress(folio, entry))
goto put_pool;
-insert_entry:
+store_entry:
entry->swpentry = swp;
entry->objcg = objcg;
--
2.44.0.478.gd926399ef9-goog
On Fri, Apr 05, 2024 at 01:35:45AM +0000, Yosry Ahmed wrote:
> Currently, zswap_store() checks zswap_same_filled_pages_enabled, kmaps
> the folio, then calls zswap_is_page_same_filled() to check the folio
> contents. Move this logic into zswap_is_page_same_filled() as well (and
> rename it to use 'folio' while we are at it).
>
> This makes zswap_store() cleaner, and makes following changes to that
> logic contained within the helper. While we are at it, rename the
> insert_entry label to store_entry to match xa_store().
>
> No functional change intended.
>
> Signed-off-by: Yosry Ahmed <[email protected]>
> Reviewed-by: Nhat Pham <[email protected]>
> Reviewed-by: Chengming Zhou <[email protected]>
Nice one.
Acked-by: Johannes Weiner <[email protected]>
On Fri, Apr 05, 2024 at 01:35:46AM +0000, Yosry Ahmed wrote:
> These knobs offer more fine-grained control to userspace than needed and
> directly expose/influence kernel implementation; remove them.
>
> For disabling same_filled handling, there is no logical reason to refuse
> storing same-filled pages more efficiently and opt for compression.
> Scanning pages for patterns may be an argument, but the page contents
> will be read into the CPU cache anyway during compression. Also,
> removing the same_filled handling code does not move the needle
> significantly in terms of performance anyway [1].
>
> For disabling non_same_filled handling, it was added when the compressed
> pages in zswap were not being properly charged to memcgs, as workloads
> could escape the accounting with compression [2]. This is no longer the
> case after commit f4840ccfca25 ("zswap: memcg accounting"), and using
> zswap without compression does not make much sense.
>
> [1]https://lore.kernel.org/lkml/CAJD7tkaySFP2hBQw4pnZHJJwe3bMdjJ1t9VC2VJd=khn1_TXvA@mail.gmail.com/
> [2]https://lore.kernel.org/lkml/[email protected]/
>
> Signed-off-by: Yosry Ahmed <[email protected]>
Acked-by: Johannes Weiner <[email protected]>
On Fri, Apr 05, 2024 at 01:35:44AM +0000, Yosry Ahmed wrote:
> Refactor limit and acceptance threshold checking outside of
> zswap_store(). This code will be moved around in a following patch, so
> it would be cleaner to move a function call around.
>
> Signed-off-by: Yosry Ahmed <[email protected]>
> ---
> mm/zswap.c | 32 ++++++++++++++++----------------
> 1 file changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/mm/zswap.c b/mm/zswap.c
> index 1cf3ab4b22e64..fba8f3c3596ab 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -1391,6 +1391,21 @@ static void zswap_fill_page(void *ptr, unsigned long value)
> memset_l(page, value, PAGE_SIZE / sizeof(unsigned long));
> }
>
> +static bool zswap_check_full(void)
> +{
> + unsigned long cur_pages = zswap_total_pages();
> + unsigned long thr = zswap_accept_thr_pages();
I know this looks neater, but it adds an extra division to the very
common path where the limit hasn't been reached yet. It should really
stay inside the branch.
Another option could be to precalculate the max and the accept
threshold in absolute pages whenever their respective module param
changes. That would eliminate both divisions from the hot path.
> + 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 <= thr) {
> + zswap_pool_reached_full = false;
> + }
> + return zswap_pool_reached_full;
> +}
> +
> bool zswap_store(struct folio *folio)
> {
> swp_entry_t swp = folio->swap;
On Fri, Apr 05, 2024 at 01:35:47AM +0000, Yosry Ahmed wrote:
> When storing same-filled pages, there is no point of checking the global
> zswap limit as storing them does not contribute toward the limit Move
> the limit checking after same-filled pages are handled.
>
> This avoids having same-filled pages skip zswap and go to disk swap if
> the limit is hit. It also avoids queueing the shrink worker, which may
> end up being unnecessary if the zswap usage goes down on its own before
> another store is attempted.
>
> Ignoring the memcg limits as well for same-filled pages is more
> controversial. Those limits are more a matter of per-workload policy.
> Some workloads disable zswap completely by setting memory.zswap.max = 0,
> and those workloads could start observing some zswap activity even after
> disabling zswap. Although harmless, this could cause confusion to
> userspace. Remain conservative and keep respecting those limits.
>
> Signed-off-by: Yosry Ahmed <[email protected]>
I'm not sure this buys us enough in practice to justify special-casing
those entries even further. Especially with the quirk of checking
cgroup limits but not the global ones; that would definitely need a
code comment similar to what you have in the changelog; and once you
add that, the real estate this special treatment takes up really
doesn't seem reasonable anymore.
In most cases we'd expect a mix of pages to hit swap. Waking up the
shrinker on a zero-filled entry is not strictly necessary of course,
but the zswap limit has been reached and the system is swapping - a
wakeup seems imminent anyway.
On Thu, Apr 4, 2024 at 7:45 PM Johannes Weiner <[email protected]> wrote:
>
> On Fri, Apr 05, 2024 at 01:35:44AM +0000, Yosry Ahmed wrote:
> > Refactor limit and acceptance threshold checking outside of
> > zswap_store(). This code will be moved around in a following patch, so
> > it would be cleaner to move a function call around.
> >
> > Signed-off-by: Yosry Ahmed <[email protected]>
> > ---
> > mm/zswap.c | 32 ++++++++++++++++----------------
> > 1 file changed, 16 insertions(+), 16 deletions(-)
> >
> > diff --git a/mm/zswap.c b/mm/zswap.c
> > index 1cf3ab4b22e64..fba8f3c3596ab 100644
> > --- a/mm/zswap.c
> > +++ b/mm/zswap.c
> > @@ -1391,6 +1391,21 @@ static void zswap_fill_page(void *ptr, unsigned long value)
> > memset_l(page, value, PAGE_SIZE / sizeof(unsigned long));
> > }
> >
> > +static bool zswap_check_full(void)
> > +{
> > + unsigned long cur_pages = zswap_total_pages();
> > + unsigned long thr = zswap_accept_thr_pages();
>
> I know this looks neater, but it adds an extra division to the very
> common path where the limit hasn't been reached yet. It should really
> stay inside the branch.
I assumed the compiler is smart enough to do the division only when
necessary, but I didn't check tbh.
>
> Another option could be to precalculate the max and the accept
> threshold in absolute pages whenever their respective module param
> changes. That would eliminate both divisions from the hot path.
Yeah, that's better and cleaner. I will do that in the next version.
Thanks!
>
> > + 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 <= thr) {
> > + zswap_pool_reached_full = false;
> > + }
> > + return zswap_pool_reached_full;
> > +}
> > +
> > bool zswap_store(struct folio *folio)
> > {
> > swp_entry_t swp = folio->swap;
On Thu, Apr 4, 2024 at 7:54 PM Johannes Weiner <[email protected]> wrote:
>
> On Fri, Apr 05, 2024 at 01:35:47AM +0000, Yosry Ahmed wrote:
> > When storing same-filled pages, there is no point of checking the global
> > zswap limit as storing them does not contribute toward the limit Move
> > the limit checking after same-filled pages are handled.
> >
> > This avoids having same-filled pages skip zswap and go to disk swap if
> > the limit is hit. It also avoids queueing the shrink worker, which may
> > end up being unnecessary if the zswap usage goes down on its own before
> > another store is attempted.
> >
> > Ignoring the memcg limits as well for same-filled pages is more
> > controversial. Those limits are more a matter of per-workload policy.
> > Some workloads disable zswap completely by setting memory.zswap.max = 0,
> > and those workloads could start observing some zswap activity even after
> > disabling zswap. Although harmless, this could cause confusion to
> > userspace. Remain conservative and keep respecting those limits.
> >
> > Signed-off-by: Yosry Ahmed <[email protected]>
>
> I'm not sure this buys us enough in practice to justify special-casing
> those entries even further. Especially with the quirk of checking
> cgroup limits but not the global ones; that would definitely need a
> code comment similar to what you have in the changelog; and once you
> add that, the real estate this special treatment takes up really
> doesn't seem reasonable anymore.
I was on the fence about this, and I thought it would be obvious
without a comment. But you are right that not skipping the limit check
for the cgroup limits would look weird.
>
> In most cases we'd expect a mix of pages to hit swap. Waking up the
> shrinker on a zero-filled entry is not strictly necessary of course,
> but the zswap limit has been reached and the system is swapping - a
> wakeup seems imminent anyway.
Theoretically, it is possible that we never have to wake up the
shrinker if zswap usage falls on its own before the next swapout,
especially with the shrinker in place. I thought it's a nice
optimization without much code, but the need for a large comment
replicating the commit log makes it less appealing tbh. I will just
drop this patch.