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

On Sat, Aug 08, 2026 at 09:25:47PM +0800, Kairui Song wrote:
> 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?

Yeah whole thing you addressed make sense.
I will follow your guide & review and send the patch soon :)

Youngjun