From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from lgeamrelo13.lge.com (lgeamrelo13.lge.com [156.147.23.53]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 07BE42B9BA for ; Sun, 9 Aug 2026 04:02:34 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=156.147.23.53 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1786248158; cv=none; b=Zdtvmhg+bHsUi+XGwhj0qSZMzoqQ8Kh3yzGRVxof8sKzyKAkCYNd1mipSI4aRoP0C3LQvxb78Ercu8cgUKs+Te0xkHw+ezQ8SSPMd2zXMUXovArFjkl0Y6zGSx375Br8v3X+ruSq1yBzsEsvbrunGqgAkSuCayWUz7W/pTBOmSg= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1786248158; c=relaxed/simple; bh=exYegPhLEigpPP5wXCYubtNnCk8Mmc7tCUKnaKj/x0E=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=akwgOObP9LUdMCMNidQDh97r2cgpmeZ1q2NYxon65E4/htzpjWiZ2uKICqk/SOsphr6AuIjwwi7qEFy2Lsynw9OdiSHcL437r4w8BwT/rYrq64/Y3qdMT7XaEFyS68ReMHLW/dYyxh9VGuMGe42peysyw++Z6rxkZtfmatfW9v8= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=lge.com; spf=pass smtp.mailfrom=lge.com; arc=none smtp.client-ip=156.147.23.53 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=lge.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=lge.com Received: from unknown (HELO lgeamrelo01.lge.com) (156.147.1.125) by 156.147.23.53 with ESMTP; 9 Aug 2026 13:02:33 +0900 X-Original-SENDERIP: 156.147.1.125 X-Original-MAILFROM: youngjun.park@lge.com Received: from unknown (HELO yjaykim-PowerEdge-T330) (10.177.112.156) by 156.147.1.125 with ESMTP; 9 Aug 2026 13:02:33 +0900 X-Original-SENDERIP: 10.177.112.156 X-Original-MAILFROM: youngjun.park@lge.com Date: Sun, 9 Aug 2026 13:02:33 +0900 From: Youngjun Park To: Kairui Song Cc: Andrew Morton , Chris Li , Kairui Song , Kemeng Shi , Nhat Pham , Baoquan He , Barry Song , Jianyue Wu , her0gyugyu@gmail.com, linux-mm@kvack.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 2/4] mm, swap: give hibernation swap slots their own swap table entry type Message-ID: References: <20260806190636.446205-1-youngjun.park@lge.com> <20260806190636.446205-3-youngjun.park@lge.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: 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 > > Link: https://lore.kernel.org/linux-mm/abp7aDgYLrxF3Me8@KASONG-MC4/ > > Signed-off-by: Youngjun Park > > --- > > 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