Re: [PATCH 2/4] mm, swap: give hibernation swap slots their own swap table entry type

On Fri, Aug 07, 2026 at 04:06:34AM +0800, Youngjun Park wrote:
> swap_alloc_hibernation_slot() stores a fake shadow in the slot it hands
> out.  An anon slot swapped out with no workingset shadow looks exactly the
> same, so nothing in mm can tell the two apart.
> 
> Give hibernation slots their own type.  Bit 4 and every bit above it are
> set, the same shape as SWP_TB_BAD.  Bits 0 to 3 are taken by the shadow,
> PFN, pointer and bad marks, so bit 4 is the first free one.  Neither type
> holds data, so the value alone says what it is.
> 
> The entry has no swap count.  Hibernation only allocates and frees a slot,
> so a count would never change.  swap_free_hibernation_slot() frees the slot
> directly, there is no count to put first.
> 
> The next patch needs these slots to stop looking like shadows.
> 
> Suggested-by: Kairui Song <[email protected]>
> Link: https://lore.kernel.org/linux-mm/abp7aDgYLrxF3Me8@KASONG-MC4/
> Signed-off-by: Youngjun Park <[email protected]>
> ---
>  mm/swap_table.h | 12 ++++++++++++
>  mm/swapfile.c   | 13 +++++++------
>  2 files changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/mm/swap_table.h b/mm/swap_table.h
> index e6613e62f8d0..c1c516bcc17e 100644
> --- a/mm/swap_table.h
> +++ b/mm/swap_table.h
> @@ -30,6 +30,7 @@ struct swap_memcg_table {
>   * PFN:      |SWAP_COUNT|Z|------ PFN -------|10| - Cached slot
>   * Pointer:  |----------- Pointer ----------|100| - (Unused)
>   * Bad:      |------------- 1 -------------|1000| - Bad slot
> + * Hibern:   |------------ 1 -------------|10000| - Hibernation slot

Nice!

Just one idea, would it be nicer if we have:
* Hibern:   |   0   |------- 1 -------------|10000| - Hibernation slot

Or:
* Hibern:   |0..001|------- 1 -------------|10000| - Hibernation slot

That way if we accidentally used __swp_tb_get_count, it return a actual
meaningful value instead of MAX. Either 0 - the slot is not used as
a countable ordinary slot, or 1 - the slot has one user: hibernation.

Maybe 0 is better at least for the intermediate commit, see below.

>  
> +static inline bool swp_tb_is_hibernation(unsigned long swp_tb)
> +{
> +	return swp_tb == SWP_TB_HIB;
> +}
> +
>  static inline bool swp_tb_is_countable(unsigned long swp_tb)
>  {
>  	return (swp_tb_is_shadow(swp_tb) || swp_tb_is_folio(swp_tb) ||
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index f5dfc7e59191..a337387f7431 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -928,7 +928,7 @@ static bool __swap_cluster_alloc_entries(struct swap_info_struct *si,
>  	 * upon folio unmap.
>  	 *
>  	 * Else, it's a exclusive order 0 allocation for hibernation.
> -	 * The slot starts with count == 1 and never increases.
> +	 * The slot carries no swap count and is freed by offset.
>  	 */
>  	if (likely(folio)) {
>  		order = folio_order(folio);
> @@ -940,8 +940,8 @@ static bool __swap_cluster_alloc_entries(struct swap_info_struct *si,
>  		order = 0;
>  		nr_pages = 1;
>  		swap_cluster_assert_empty(ci, ci_off, 1, false);
> -		/* Fake shadow placeholder with no flag, hibernation does not use the zeromap */
> -		__swap_table_set(ci, ci_off, __swp_tb_mk_count(shadow_to_swp_tb(NULL, 0), 1));
> +		/* Exclusively owned by hibernation, must never enter the swap cache */
> +		__swap_table_set(ci, ci_off, SWP_TB_HIB);
>  	} else {
>  		/* Allocation without folio is only possible with hibernation */
>  		WARN_ON_ONCE(1);
> @@ -1929,9 +1929,11 @@ void __swap_cluster_free_entries(struct swap_info_struct *si,
>  		old_tb = __swap_table_get(ci, ci_off);
>  		/*
>  		 * Freeing is done after release of the last swap count
> -		 * ref, or after swap cache is dropped
> +		 * ref, or after swap cache is dropped. A hibernation slot
> +		 * has no count and is freed directly by its owner.
>  		 */
> -		VM_WARN_ON(!swp_tb_is_shadow(old_tb) || __swp_tb_get_count(old_tb) > 1);
> +		VM_WARN_ON(!swp_tb_is_hibernation(old_tb) &&
> +			   (!swp_tb_is_shadow(old_tb) || __swp_tb_get_count(old_tb) > 1));
>  
>  		/* Resetting the slot to NULL also clears the inline flags. */
>  		__swap_table_set(ci, ci_off, null_to_swp_tb());
> @@ -2201,7 +2203,6 @@ void swap_free_hibernation_slot(swp_entry_t entry)
>  	pgoff_t offset = swp_offset(entry);
>  
>  	ci = swap_cluster_lock(si, offset);
> -	__swap_cluster_put_entry(ci, offset % SWAPFILE_CLUSTER);
>  	/*
>  	 * A slot with a folio in the swap cache is freed when the folio
>  	 * leaves the cache, the same rule swap_put_entries_cluster() follows.

This idea is right, but is the patch in the right order? If readahead
tried to add a folio to a hibernate slot by accident, seems nothing
blocks that in the current patch, and that PFN slot will have a (MAX)
count value, and considered countable? If the that folio is somehow
reclaimed, we got a corrupted shadow (hib type is gone)?

If we have the count part of a hibernation slot be 0,
__swap_cache_add_check will fail natively, seems there will be no
such risk. A few existing helpers can also help catch potential
wrong freeing of hibernation slot. (underflow check).

The layout can be changed again afterwards.

How do you think?