linux-kernel - Re: [PATCH v1 2/5] mm: zswap: refactor limit checking from zswap

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Thu, 4 Apr 2024 22:44:59 -0400
From: Johannes Weiner <hannes@...xchg.org>
To: Yosry Ahmed <yosryahmed@...gle.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
	Nhat Pham <nphamcs@...il.com>,
	Chengming Zhou <chengming.zhou@...ux.dev>, linux-mm@...ck.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 2/5] mm: zswap: refactor limit checking from
 zswap_store()

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@...gle.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;