diff mbox series

[v1,2/5] mm: zswap: refactor limit checking from zswap_store()

Message ID 20240405013547.1859126-3-yosryahmed@google.com (mailing list archive)
State New
Headers show
Series zswap same-filled and limit checking cleanups | expand

Commit Message

Yosry Ahmed April 5, 2024, 1:35 a.m. UTC
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 <yosryahmed@google.com>
---
 mm/zswap.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

Comments

Johannes Weiner April 5, 2024, 2:44 a.m. UTC | #1
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 <yosryahmed@google.com>
> ---
>  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;
Yosry Ahmed April 5, 2024, 4:15 a.m. UTC | #2
On Thu, Apr 4, 2024 at 7:45 PM Johannes Weiner <hannes@cmpxchg.org> 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 <yosryahmed@google.com>
> > ---
> >  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;
diff mbox series

Patch

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