Received-SPF: pass (google.com: domain of linux-kernel+bounces-132335-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Thu, 4 Apr 2024 22:44:59 -0400
From: Johannes Weiner <hannes@cmpxchg.org>
To: Yosry Ahmed <yosryahmed@google.com>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	Nhat Pham <nphamcs@gmail.com>,
	Chengming Zhou <chengming.zhou@linux.dev>, linux-mm@kvack.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v1 2/5] mm: zswap: refactor limit checking from
 zswap_store()
Message-ID: <20240405024459.GE641486@cmpxchg.org>
References: <20240405013547.1859126-1-yosryahmed@google.com>
 <20240405013547.1859126-3-yosryahmed@google.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240405013547.1859126-3-yosryahmed@google.com>

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;