Re: [PATCH v5 12/12] mm, swap: merge zeromap into swap table

[Kairui Song]

On Thu, May 21, 2026 at 06:52:04PM +0800, Andrew Morton wrote:
> On Sun, 17 May 2026 23:39:51 +0800 Kairui Song via B4 Relay <devnull+kasong.tencent.com@xxxxxxxxxx> wrote:
> 
> > From: Kairui Song <kasong@xxxxxxxxxxx>
> > 
> > By allocating one additional bit in the swap table entry's flags field
> > alongside the count, we can store the zeromap inline
> > 
> > For 64 bit systems, zeromap will store in the swap table, avoiding zeromap
> > allocation. It reduces the allocated memory. That is the happy path.
> > 
> > For certain 32-bit archs, there might not be enough bits in the swap
> > table to contain both PFN and flags. Therefore, conditionally let each
> > cluster have a zeromap field at build time, and use that instead.
> > If the swapfile cluster is not fully used, it will still save memory for
> > zeromap. The empty cluster does not allocate a zeromap. In the worst case,
> > all cluster are fully populated. We will use memory similar to the
> > previous zeromap implementation.
> > 
> > A few macros were moved to different headers for build time struct
> > definition.
> > 
> > ...
> >
> > @@ -469,13 +474,21 @@ static int swap_cluster_alloc_table(struct swap_cluster_info *ci, gfp_t gfp)
> >  		VM_WARN_ON_ONCE(ci->memcg_table);
> >  		ci->memcg_table = kzalloc_obj(*ci->memcg_table, gfp);
> >  		if (!ci->memcg_table)
> > -			ret = -ENOMEM;
> > +			goto err_free;
> >  	}
> >  #endif
> > -	if (ret)
> > -		swap_cluster_free_table(ci);
> >  
> > -	return ret;
> > +#if !SWAP_TABLE_HAS_ZEROFLAG
> > +	VM_WARN_ON_ONCE(ci->zero_bitmap);
> > +	ci->zero_bitmap = bitmap_zalloc(SWAPFILE_CLUSTER, gfp);
> > +	if (!ci->zero_bitmap)
> > +		goto err_free;
> > +#endif
> > +	return 0;
> > +
> > +err_free:
> > +	swap_cluster_free_table(ci);
> > +	return -ENOMEM;
> >  }
> 
> My m68k defconfig warned.  I'll do the below, which looks good enough. 
> Please check.
> 
> Perhaps a custom guard() handler would clean things up here.
> 
> 
> From: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> Subject: mm-swap-merge-zeromap-into-swap-table-fix-2
> Date: Thu May 21 06:39:20 PM PDT 2026
> 
> mm/swapfile.c: In function 'swap_cluster_alloc_table':
> mm/swapfile.c:488:1: warning: label 'err_free' defined but not used [-Wunused-label]
>   488 | err_free:
>       | ^~~~~~~~
> 
> Cc: Baolin Wang <baolin.wang@xxxxxxxxxxxxxxxxx>
> Cc: Baoquan He <bhe@xxxxxxxxxx>
> Cc: Barry Song <baohua@xxxxxxxxxx>
> Cc: Chengming Zhou <chengming.zhou@xxxxxxxxx>
> Cc: Chris Li <chrisl@xxxxxxxxxx>
> Cc: David Hildenbrand <david@xxxxxxxxxx>
> Cc: Hugh Dickins <hughd@xxxxxxxxxx>
> Cc: Johannes Weiner <hannes@xxxxxxxxxxx>
> Cc: Kairui Song <kasong@xxxxxxxxxxx>
> Cc: Kemeng Shi <shikemeng@xxxxxxxxxxxxxxx>
> Cc: Lorenzo Stoakes <ljs@xxxxxxxxxx>
> Cc: Muchun Song <muchun.song@xxxxxxxxx>
> Cc: Nhat Pham <nphamcs@xxxxxxxxx>
> Cc: Roman Gushchin <roman.gushchin@xxxxxxxxx>
> Cc: Shakeel Butt <shakeel.butt@xxxxxxxxx>
> Cc: Youngjun Park <youngjun.park@xxxxxxx>
> Cc: Zi Yan <ziy@xxxxxxxxxx>
> Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> ---
> 
>  mm/swapfile.c |   16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> --- a/mm/swapfile.c~mm-swap-merge-zeromap-into-swap-table-fix-2
> +++ a/mm/swapfile.c
> @@ -472,22 +472,22 @@ static int swap_cluster_alloc_table(stru
>  	if (!mem_cgroup_disabled()) {
>  		VM_WARN_ON_ONCE(ci->memcg_table);
>  		ci->memcg_table = kzalloc_obj(*ci->memcg_table, gfp);
> -		if (!ci->memcg_table)
> -			goto err_free;
> +		if (!ci->memcg_table) {
> +			swap_cluster_free_table(ci);
> +			return -ENOMEM;
> +		}
>  	}
>  #endif
>  
>  #if !SWAP_TABLE_HAS_ZEROFLAG
>  	VM_WARN_ON_ONCE(ci->zero_bitmap);
>  	ci->zero_bitmap = bitmap_zalloc(SWAPFILE_CLUSTER, gfp);
> -	if (!ci->zero_bitmap)
> -		goto err_free;
> +	if (!ci->zero_bitmap) {
> +		swap_cluster_free_table(ci);
> +		return -ENOMEM;
> +	}
>  #endif
>  	return 0;
> -
> -err_free:
> -	swap_cluster_free_table(ci);
> -	return -ENOMEM;
>  }
>  
>  /*
> _
> 

Looks good, thank you. The error path is still simple and
straight at the moment.

It will be even better if we also remove the now unused ret
variable as well (this isn't triggering warning yet since Kbuild
don't set unused-but-set-variable by default):

diff --git a/mm/swapfile.c b/mm/swapfile.c
index 9712cc862c9c..615d90867111 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -450,7 +450,6 @@ static int swap_cluster_alloc_table(struct swap_cluster_info *ci, gfp_t gfp)
 {
 	struct swap_table *table = NULL;
 	struct folio *folio;
-	int ret = 0;
 
 	/* The cluster must be empty and not on any list during allocation. */
 	VM_WARN_ON_ONCE(ci->flags || !cluster_is_empty(ci));