From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pg1-f180.google.com (mail-pg1-f180.google.com [209.85.215.180]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 848DE3A873E for ; Thu, 25 Jun 2026 14:41:26 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.215.180 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1782398488; cv=none; b=cnu2509Ldt7kkg19XdsR+hL8uCn6VE3qrwb8+31mID6FW8Zi4uhkdJiasxKSHMz/7bT1hdQ4TA/28M+AAgl8s8gUltLR3QaB0Ue5ReL8t7NaRyRIuGmAZZuIujH8r+MlJPv381Y9DziFpimzSe3lp9/O8vuPkcZosce9gPpXAP0= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1782398488; c=relaxed/simple; bh=L+9b8P0084d9TSaEwH1ICGm+FAL2YznmMX+bOBpEcpE=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=P2bSYvoIw4/XQwwczxyDk0nCUJy9Oo/c06s9jeSsf7aCCMWu9Iumir1RDx1bAcfI2ldWzdMokmyguWVbpCn90aV48cdvnNO0ATAw23ZijEXCGRYZ+UPai1fwrTTlmDNdsOAzPEBu9HtI9PMePeVCFiR3lh9pl1VFwmbk7KQ/aaI= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=DseBssvC; arc=none smtp.client-ip=209.85.215.180 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="DseBssvC" Received: by mail-pg1-f180.google.com with SMTP id 41be03b00d2f7-c890110acdaso936095a12.3 for ; Thu, 25 Jun 2026 07:41:26 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20251104; t=1782398486; x=1783003286; darn=vger.kernel.org; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date:from:to :cc:subject:date:message-id:reply-to; bh=slSRypEduuNZ9JgILeAUNdKSkqsw9CkFddos67AsSDk=; b=DseBssvCZcuQVImJBwGDWDWTDE4M2SKhk0NwF7CWD6uAJ/mkB7yODRUU1qkQj0h52T OLOVdBJAJXmRsmCXp39srcg8dpTcQcF5BOyHoqz668hFyak5LSODRzRw+kU71lgJaLq6 xJUNs7StmCZQFYzQ6t3H3QBtbftgScoekTLFs/Wb8p4r9pRsX9ktJu1meu+5fWlZOYN3 UJ1PanP3H/20yCERJXc7Ww216NDDVs5mE0UxAupClEY66x71m6mNBjOVQg/AXBKbJmDg jmE+76DcefilV6ck8mXMt2rYQAkbkQAiw4oyfKfGvHhmGC6rtBvJ9C6c/WOSJ7nHcWWt t5aQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1782398486; x=1783003286; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date:x-gm-gg :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=slSRypEduuNZ9JgILeAUNdKSkqsw9CkFddos67AsSDk=; b=SmgxJgVYVusl4enDiwctSJBw1ALiwlTrgGPccYPMzQbomdF7khwiGey88NhQt6dKVp /wJo/36oUPZOIbBrnwhvIh+lC7BJSue9LqTabzVt7yfK65XMsK8kvwIgwAyb3tmkQp+H gEjklLMNnbEiMv8Ix3H7nM8yjCm1R9lGPfWJWCrDjRitx2BdWfCYK/nvdEEqR7O4ylQ+ X7yzO21meIVnulrGTDVxolWghwYw1YBWAIQCN4WpLFw4rKHEOnEDDXXbsksf+CRGzoto vTWL9BO0FdHtp+E/bC/IMYBxDxYWQuJ15D/1kOrsJqrv8McsuLyK6dTeLGUKn5vVedbF 5f3g== X-Forwarded-Encrypted: i=1; AFNElJ9SU2wmBXKVXeKWRKpjNjFCnad90btZ1ODgIWWzm4eySU2yW2tiFb4sFTDO7lCWboEbuI9oYG/Iru4JwOc=@vger.kernel.org X-Gm-Message-State: AOJu0Yx5gnsOUtHibxzBN1lGKFg9ow0K7TcMeUaVXdCeNkPvVF9bsbIx OpkGcUxBe2qCEsSWRreJegwCOSw1EMpwBNDmltNSI0kbNjlOH2x3vI2J X-Gm-Gg: AfdE7clazjSeKUH+Wz/8TGaVOJ23yEZg6X00SIbDCsR7xvrMvGxiPBpvBwTKZ3NvbkA lNpnF6z+CN8ldfdur8A0G6vhEmAbmknku6rT/91iCtWypY8Wc/o8HVXA4SawdhjIAiXEpl+6k3f zXf7WMf7luQbBkjAP8sJU1gRJ7P5T5oCELC6GOtek/VLr/NMATTntHZbqx90tvV4hKtKHoYP4nv xxD2E+LjjB9nYNLIfuXrmL3KtLoAENHoXvakFOM29UX6diuqSYDwwcvOo6mdGyvLu0r1wV027Ro Fgw12G2ydheuAmrnrnKH8i3b4VvLqwEKz/BOZaeXbg+En2ZRwVD6BXq0lWmC3sXItKlOPATJEbe Z6Cikh2ZKIbQBUu+QmenPPqjmgD+BgRbT9zILex9BgGQl+bdABfoHc7l69eNP1ipBu78uZGFFYi fkrAPOGBCdF7uwBXHwJdZmvJQqbrdhnPR1jz3B/ra7QWNF+LZJ X-Received: by 2002:a05:6a20:728a:b0:3b9:5efb:cb2a with SMTP id adf61e73a8af0-3bd4ac85f3cmr3409358637.3.1782398485438; Thu, 25 Jun 2026 07:41:25 -0700 (PDT) Received: from KASONG-MC4 ([115.171.41.167]) by smtp.gmail.com with ESMTPSA id 41be03b00d2f7-c92b96ec739sm1834186a12.2.2026.06.25.07.41.14 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 25 Jun 2026 07:41:24 -0700 (PDT) Date: Thu, 25 Jun 2026 22:40:50 +0800 From: Kairui Song To: Barry Song , "David Hildenbrand (Arm)" Cc: akpm@linux-foundation.org, linux-mm@kvack.org, baoquan.he@linux.dev, chrisl@kernel.org, jp.kobryn@linux.dev, kasong@tencent.com, liam@infradead.org, linux-kernel@vger.kernel.org, ljs@kernel.org, mhocko@suse.com, nphamcs@gmail.com, rppt@kernel.org, shakeel.butt@linux.dev, shikemeng@huaweicloud.com, surenb@google.com, usama.arif@linux.dev, vbabka@kernel.org, youngjun.park@lge.com Subject: Re: [PATCH v2 4/4] mm: try to free swapcache for non-LRU folios Message-ID: References: <20260623231635.43086-1-baohua@kernel.org> <20260623231635.43086-5-baohua@kernel.org> 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=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: On Thu, Jun 25, 2026 at 05:14:45AM +0800, Barry Song wrote: > On Wed, Jun 24, 2026 at 11:20 PM David Hildenbrand (Arm) > wrote: > > > > On 6/24/26 01:16, Barry Song (Xiaomi) wrote: > > > Originally, we unconditionally called lru_add_drain() for write > > > swap-in page faults. This might drop the reference held by the per-CPU > > > LRU cache if the folio happened to reside there. However, there was no > > > guarantee that the folio was actually cached on the current CPU. > > > > > > Now that lru_add_drain() has been removed, we have lost one > > > opportunity to drop a reference held by the LRU cache. We could > > > instead incorporate that possibility into the condition evaluated by > > > should_try_to_free_swap(). > > > > > > Suggested-by: Kairui Song > > > Signed-off-by: Barry Song (Xiaomi) > > > --- > > > mm/memory.c | 5 ++++- > > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > > > diff --git a/mm/memory.c b/mm/memory.c > > > index 2983a6baf474..14577c67c61a 100644 > > > --- a/mm/memory.c > > > +++ b/mm/memory.c > > > @@ -5087,8 +5087,11 @@ vm_fault_t do_swap_page(struct vm_fault *vmf) > > > * Remove the swap entry and conditionally try to free up the swapcache. > > > * Do it after mapping, so raced page faults will likely see the folio > > > * in swap cache and wait on the folio lock. > > > + * Assume non-LRU folios may be queued in the LRU cache, which contributes > > > + * an additional reference to the folio. > > > */ > > > - if (should_try_to_free_swap(si, folio, vma, nr_pages, vmf->flags)) > > > + if (should_try_to_free_swap(si, folio, vma, nr_pages + > > > + !folio_test_lru(folio), vmf->flags)) > > > folio_free_swap(folio); > > > > > > folio_unlock(folio); > > > > Hm, in wp_can_reuse_anon_folio() we'll try dropping the swapcache ourselves. > > > > So I wonder if we still need that handling ("If we want to map a page that's in > > the swapcache writable, we ...") at all? > > > > > > Ahh, I see the problem now: > > > > commit 4b34f1d82c6549837b2061096dea249e881a4495 > > Author: Kairui Song > > Date: Sat Dec 20 03:43:35 2025 +0800 > > > > mm, swap: free the swap cache after folio is mapped > > > > Currently, we remove the folio from the swap cache and free the swap cache > > before mapping the PTE. To reduce repeated faults due to parallel swapins > > of the same PTE, change it to remove the folio from the swap cache after > > it is mapped. So new faults from the swap PTE will be much more likely to > > see the folio in the swap cache and wait on it. > > > > This does not eliminate all swapin races: an ongoing swapin fault may > > still see an empty swap cache. That's harmless, as the PTE is changed > > before the swap cache is cleared, so it will just return and not trigger > > any repeated faults. This does help to reduce the chance. > > > > > > That changed that behavior such that we *must* now always fallback to do_wp_page(). > > > > What a mess (I didn't ack) That's awkward, my bad, terribly sorry about this :( I sincerely apologize for the oversight and the trouble this has caused. I haven't seen any performance regression in any workload recently though, or any correctness issue, perhaps the round trip of do_wp_page wasn't that bad. It should still catch the reuse folios, just more costly than doing things in-place. I think we should restore the original check first. We might also want to avoid dropping the swap cache if the folio will not be reused, which was discussed here: https://lore.kernel.org/linux-mm/CAMgjq7BDfvNXdWH0cqarsujjUn3i3tDDhDkmSg01TR4h-tDorQ@mail.gmail.com/ Maybe extracting some common part into a helper can help make this cleaner. > > So it seems we need a patch before patch 4 to fix the missed > swapcache freeing on write faults, because the write bit has already > been cleared by the time the folio is mapped and reused? > > diff --git a/mm/memory.c b/mm/memory.c > index 14577c67c61a..e8fc5a86f6ea 100644 > --- a/mm/memory.c > +++ b/mm/memory.c > @@ -4752,6 +4752,7 @@ static void check_swap_exclusive(struct folio > *folio, swp_entry_t entry, > */ > vm_fault_t do_swap_page(struct vm_fault *vmf) > { > + enum fault_flag fault_flags = vmf->flags; > struct vm_area_struct *vma = vmf->vma; > struct folio *swapcache = NULL, *folio; > struct page *page; > @@ -5091,7 +5092,7 @@ vm_fault_t do_swap_page(struct vm_fault *vmf) > * an additional reference to the folio. > */ > if (should_try_to_free_swap(si, folio, vma, nr_pages + > - !folio_test_lru(folio), vmf->flags)) > + !folio_test_lru(folio), fault_flags)) > folio_free_swap(folio); > > folio_unlock(folio); > > > Best Regards > Barry Hi Barry, The problem is more than that, the `exclusive || folio_ref_count(folio) == 1` in do_swap_page is also ineffective now. But that earlier commit 4b34f1d82c65 is kind of meaningless after recent refactor of swapin. It was trying to avoid redundant folio allocation and thrashing from concurrent faulting process of the same PTE, but recent swap table rework already gated that with unified folio allocation. We are freeing the swap entry first here, that is effective enough so that commit is no longer needed. So perhaps we can first just revert it, how do you think?: >From 8009328e0782fc22365ef32474bae749beda637b Mon Sep 17 00:00:00 2001 From: Kairui Song Date: Thu, 25 Jun 2026 11:44:26 +0800 Subject: [PATCH] mm/swap: free swap cache before mapping to fix folio reuse This is effectively a revert of commit 4b34f1d82c65 ("mm, swap: free the swap cache after folio is mapped"). That commit was trying to reduce wasted fault and folio allocation from parallel swapins of the same entry: keeping the folio in the swap cache until the PTE is installed makes a racing fault more likely to find it and wait on the folio lock, rather than allocate and charge a new folio only to discover the race and throw it away, which leads to thrashing. That benefit is marginal now. Folio allocation and swapin are both gated on the swap entry count in the swap cache layer since commit 02d733a7ec1d ("mm, swap: unify large folio allocation"). Once the winning fault drops the entry count, a racing fault that misses the cache will fail the swap entry count check too and back off at the page table lock before allocating anything. Note there is still a tiny window with or without either patch, but it's already minimized now and should be fixed in other ways later if needed. Meanwhile the earlier commit has a real problem, as David pointed out [1]. The swap cache reference is still held during the write fault folio reference check, so a sole-owned but non-exclusive write fault always falls back to do_wp_page() instead of being reused in place. The write-fault path of should_try_to_free_swap() is broken too as the FAULT_FLAG_WRITE flag is missing. There is no correctness problem though, only the reuse and cleanup optimizations were lost. So revert it. This makes the sole-owner check effective again and recovers the reuse fast path. Also slightly adjust a sanity check to be more rigorous. Fixes: 4b34f1d82c65 ("mm, swap: free the swap cache after folio is mapped") Reported-by: David Hildenbrand (Arm) Closes: https://lore.kernel.org/linux-mm/e56c4d73-ed4a-48bb-8d0a-97b1200d4a35@kernel.org/ [1] Signed-off-by: Kairui Song --- mm/memory.c | 33 +++++++++++++++++++-------------- 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/mm/memory.c b/mm/memory.c index ff338c2abe92..f31d6b8e8c0b 100644 --- a/mm/memory.c +++ b/mm/memory.c @@ -4512,7 +4512,6 @@ static vm_fault_t remove_device_exclusive_entry(struct vm_fault *vmf) static inline bool should_try_to_free_swap(struct swap_info_struct *si, struct folio *folio, struct vm_area_struct *vma, - unsigned int extra_refs, unsigned int fault_flags) { if (!folio_test_swapcache(folio)) @@ -4535,7 +4534,7 @@ static inline bool should_try_to_free_swap(struct swap_info_struct *si, * reference only in case it's likely that we'll be the exclusive user. */ return (fault_flags & FAULT_FLAG_WRITE) && !folio_test_ksm(folio) && - folio_ref_count(folio) == (extra_refs + folio_nr_pages(folio)); + folio_ref_count(folio) == (1 + folio_nr_pages(folio)); } static vm_fault_t pte_marker_clear(struct vm_fault *vmf) @@ -5033,6 +5032,24 @@ vm_fault_t do_swap_page(struct vm_fault *vmf) */ arch_swap_restore(folio_swap(entry, folio), folio); + /* + * Remove the swap entry and conditionally try to free up the swapcache. + * We're already holding a reference on the page but haven't mapped it + * yet. + * + * The swap count has to be freed to 0 first so folio_free_swap + * can free exclusive clean cache. Concurrent fault is very unlikely + * to trigger redundant folio alloc, nor will it cause redundant IO, + * as the swap entry count gates it. + */ + VM_WARN_ON_ONCE(nr_pages != 1 && nr_pages != folio_nr_pages(folio)); + if (folio != swapcache) + folio_put_swap(swapcache, NULL); + else + folio_put_swap(folio, nr_pages < folio_nr_pages(folio) ? page : NULL); + if (should_try_to_free_swap(si, folio, vma, vmf->flags)) + folio_free_swap(folio); + add_mm_counter(vma->vm_mm, MM_ANONPAGES, nr_pages); add_mm_counter(vma->vm_mm, MM_SWAPENTS, -nr_pages); pte = mk_pte(page, vma->vm_page_prot); @@ -5067,7 +5084,6 @@ vm_fault_t do_swap_page(struct vm_fault *vmf) if (unlikely(folio != swapcache)) { folio_add_new_anon_rmap(folio, vma, address, RMAP_EXCLUSIVE); folio_add_lru_vma(folio, vma); - folio_put_swap(swapcache, NULL); } else if (!folio_test_anon(folio)) { /* * We currently only expect !anon folios that are fully @@ -5076,12 +5092,9 @@ vm_fault_t do_swap_page(struct vm_fault *vmf) VM_WARN_ON_ONCE_FOLIO(folio_nr_pages(folio) != nr_pages, folio); VM_WARN_ON_ONCE_FOLIO(folio_mapped(folio), folio); folio_add_new_anon_rmap(folio, vma, address, rmap_flags); - folio_put_swap(folio, NULL); } else { - VM_WARN_ON_ONCE(nr_pages != 1 && nr_pages != folio_nr_pages(folio)); folio_add_anon_rmap_ptes(folio, page, nr_pages, vma, address, rmap_flags); - folio_put_swap(folio, nr_pages == 1 ? page : NULL); } VM_BUG_ON(!folio_test_anon(folio) || @@ -5090,14 +5103,6 @@ vm_fault_t do_swap_page(struct vm_fault *vmf) arch_do_swap_page_nr(vma->vm_mm, vma, address, pte, pte, nr_pages); - /* - * Remove the swap entry and conditionally try to free up the swapcache. - * Do it after mapping, so raced page faults will likely see the folio - * in swap cache and wait on the folio lock. - */ - if (should_try_to_free_swap(si, folio, vma, nr_pages, vmf->flags)) - folio_free_swap(folio); - folio_unlock(folio); if (unlikely(folio != swapcache)) { /* -- 2.54.0