Currently, zswap_store() check 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.
Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
---
mm/zswap.c | 45 ++++++++++++++++++++++++---------------------
1 file changed, 24 insertions(+), 21 deletions(-)
diff --git a/mm/zswap.c b/mm/zswap.c
index 6b890c8590ef7..498a6c5839bef 100644
--- a/mm/zswap.c
+++ b/mm/zswap.c
@@ -1385,26 +1385,36 @@ 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;
- 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;
+ if (val != page[last_pos]) {
+ ret = false;
+ goto out;
+ }
for (pos = 1; pos < last_pos; pos++) {
- if (val != page[pos])
- return 0;
+ if (val != page[pos]) {
+ ret = false;
+ goto out;
+ }
}
*value = val;
-
- return 1;
+ ret = true;
+out:
+ kunmap_local(page);
+ return ret;
}
static void zswap_fill_page(void *ptr, unsigned long value)
@@ -1438,6 +1448,7 @@ bool zswap_store(struct folio *folio)
struct obj_cgroup *objcg = NULL;
struct mem_cgroup *memcg = NULL;
struct zswap_entry *entry;
+ unsigned long value;
VM_WARN_ON_ONCE(!folio_test_locked(folio));
VM_WARN_ON_ONCE(!folio_test_swapcache(folio));
@@ -1470,19 +1481,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 insert_entry;
}
if (!zswap_non_same_filled_pages_enabled)
--
2.44.0.396.g6e790dbe36-googOn 2024/3/26 07:50, Yosry Ahmed wrote:
> Currently, zswap_store() check 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.
>
> Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
LGTM with one comment below:
Reviewed-by: Chengming Zhou <chengming.zhou@linux.dev>
> ---
> mm/zswap.c | 45 ++++++++++++++++++++++++---------------------
> 1 file changed, 24 insertions(+), 21 deletions(-)
>
> diff --git a/mm/zswap.c b/mm/zswap.c
> index 6b890c8590ef7..498a6c5839bef 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -1385,26 +1385,36 @@ 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;
>
> - 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;
> + if (val != page[last_pos]) {
> + ret = false;
> + goto out;
> + }
>
> for (pos = 1; pos < last_pos; pos++) {
> - if (val != page[pos])
> - return 0;
> + if (val != page[pos]) {
> + ret = false;
nit: ret can be initialized to false, so
> + goto out;
> + }
> }
>
> *value = val;
> -
> - return 1;
> + ret = true;
only need to set to true here.
Thanks.
> +out:
> + kunmap_local(page);
> + return ret;
> }
>
> static void zswap_fill_page(void *ptr, unsigned long value)
> @@ -1438,6 +1448,7 @@ bool zswap_store(struct folio *folio)
> struct obj_cgroup *objcg = NULL;
> struct mem_cgroup *memcg = NULL;
> struct zswap_entry *entry;
> + unsigned long value;
>
> VM_WARN_ON_ONCE(!folio_test_locked(folio));
> VM_WARN_ON_ONCE(!folio_test_swapcache(folio));
> @@ -1470,19 +1481,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 insert_entry;
> }
>
> if (!zswap_non_same_filled_pages_enabled)
On Tue, Mar 26, 2024 at 7:39 PM Chengming Zhou <chengming.zhou@linux.dev> wrote:
>
> On 2024/3/26 07:50, Yosry Ahmed wrote:
> > Currently, zswap_store() check 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.
> >
> > Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
>
> LGTM with one comment below:
>
> Reviewed-by: Chengming Zhou <chengming.zhou@linux.dev>
>
> > ---
> > mm/zswap.c | 45 ++++++++++++++++++++++++---------------------
> > 1 file changed, 24 insertions(+), 21 deletions(-)
> >
> > diff --git a/mm/zswap.c b/mm/zswap.c
> > index 6b890c8590ef7..498a6c5839bef 100644
> > --- a/mm/zswap.c
> > +++ b/mm/zswap.c
> > @@ -1385,26 +1385,36 @@ 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;
> >
> > - 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;
> > + if (val != page[last_pos]) {
> > + ret = false;
> > + goto out;
> > + }
> >
> > for (pos = 1; pos < last_pos; pos++) {
> > - if (val != page[pos])
> > - return 0;
> > + if (val != page[pos]) {
> > + ret = false;
>
> nit: ret can be initialized to false, so
>
> > + goto out;
> > + }
> > }
> >
> > *value = val;
> > -
> > - return 1;
> > + ret = true;
>
> only need to set to true here.
I didn't bother improving the code here because patch 6 will replace
it anyway, but I will do that in the next version anyway, might as
well.
Thanks!
On Mon, Mar 25, 2024 at 4:50 PM Yosry Ahmed <yosryahmed@google.com> wrote: > > Currently, zswap_store() check 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. > > Signed-off-by: Yosry Ahmed <yosryahmed@google.com> Reviewed-by: Nhat Pham <nphamcs@gmail.com>
© 2016 - 2026 Red Hat, Inc.