Re: [PATCH v5 2/2] mm: zswap: fix global shrinker error handling logic

[Chengming Zhou <chengming.zhou@linux.dev>]

On 2024/7/31 08:49, Takero Funaki wrote:
> This patch fixes the zswap global shrinker, which did not shrink the
> zpool as expected.
> 
> The issue addressed is that shrink_worker() did not distinguish between
> unexpected errors and expected errors, such as failed writeback from an
> empty memcg. The shrinker would stop shrinking after iterating through
> the memcg tree 16 times, even if there was only one empty memcg.
> 
> With this patch, the shrinker no longer considers encountering an empty
> memcg, encountering a memcg with writeback disabled, or reaching the end
> of a memcg tree walk as a failure, as long as there are memcgs that are
> candidates for writeback. Systems with one or more empty memcgs will now
> observe significantly higher zswap writeback activity after the zswap
> pool limit is hit.
> 
> To avoid an infinite loop when there are no writeback candidates, this
> patch tracks writeback attempts during memcg tree walks and limits
> reties if no writeback candidates are found.
> 
> To handle the empty memcg case, the helper function shrink_memcg() is
> modified to check if the memcg is empty and then return -ENOENT.
> 
> Fixes: a65b0e7607cc ("zswap: make shrinking memcg-aware")
> Signed-off-by: Takero Funaki <flintglass@gmail.com>

Looks good to me:

Reviewed-by: Chengming Zhou <chengming.zhou@linux.dev>

Thanks.

> ---
>   mm/zswap.c | 40 +++++++++++++++++++++++++++++++++-------
>   1 file changed, 33 insertions(+), 7 deletions(-)
> 
> diff --git a/mm/zswap.c b/mm/zswap.c
> index 3c16a1192252..d46caa42ed4f 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -1287,10 +1287,10 @@ static struct shrinker *zswap_alloc_shrinker(void)
>   
>   static int shrink_memcg(struct mem_cgroup *memcg)
>   {
> -	int nid, shrunk = 0;
> +	int nid, shrunk = 0, scanned = 0;
>   
>   	if (!mem_cgroup_zswap_writeback_enabled(memcg))
> -		return -EINVAL;
> +		return -ENOENT;
>   
>   	/*
>   	 * Skip zombies because their LRUs are reparented and we would be
> @@ -1304,21 +1304,34 @@ static int shrink_memcg(struct mem_cgroup *memcg)
>   
>   		shrunk += list_lru_walk_one(&zswap_list_lru, nid, memcg,
>   					    &shrink_memcg_cb, NULL, &nr_to_walk);
> +		scanned += 1 - nr_to_walk;
>   	}
> +
> +	if (!scanned)
> +		return -ENOENT;
> +
>   	return shrunk ? 0 : -EAGAIN;
>   }
>   
>   static void shrink_worker(struct work_struct *w)
>   {
>   	struct mem_cgroup *memcg;
> -	int ret, failures = 0;
> +	int ret, failures = 0, attempts = 0;
>   	unsigned long thr;
>   
>   	/* Reclaim down to the accept threshold */
>   	thr = zswap_accept_thr_pages();
>   
>   	/*
> -	 * Global reclaim will select cgroup in a round-robin fashion.
> +	 * Global reclaim will select cgroup in a round-robin fashion from all
> +	 * online memcgs, but memcgs that have no pages in zswap and
> +	 * writeback-disabled memcgs (memory.zswap.writeback=0) are not
> +	 * candidates for shrinking.
> +	 *
> +	 * Shrinking will be aborted if we encounter the following
> +	 * MAX_RECLAIM_RETRIES times:
> +	 * - No writeback-candidate memcgs found in a memcg tree walk.
> +	 * - Shrinking a writeback-candidate memcg failed.
>   	 *
>   	 * We save iteration cursor memcg into zswap_next_shrink,
>   	 * which can be modified by the offline memcg cleaner
> @@ -1356,9 +1369,14 @@ static void shrink_worker(struct work_struct *w)
>   		spin_unlock(&zswap_shrink_lock);
>   
>   		if (!memcg) {
> -			if (++failures == MAX_RECLAIM_RETRIES)
> +			/*
> +			 * Continue shrinking without incrementing failures if
> +			 * we found candidate memcgs in the last tree walk.
> +			 */
> +			if (!attempts && ++failures == MAX_RECLAIM_RETRIES)
>   				break;
>   
> +			attempts = 0;
>   			goto resched;
>   		}
>   
> @@ -1366,8 +1384,16 @@ static void shrink_worker(struct work_struct *w)
>   		/* drop the extra reference */
>   		mem_cgroup_put(memcg);
>   
> -		if (ret == -EINVAL)
> -			break;
> +		/*
> +		 * There are no writeback-candidate pages in the memcg.
> +		 * This is not an issue as long as we can find another memcg
> +		 * with pages in zswap. Skip this without incrementing attempts
> +		 * and failures.
> +		 */
> +		if (ret == -ENOENT)
> +			continue;
> +		++attempts;
> +
>   		if (ret && ++failures == MAX_RECLAIM_RETRIES)
>   			break;
>   resched: