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