From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 96D3B374197 for ; Tue, 23 Jun 2026 08:40:36 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=217.140.110.172 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1782204038; cv=none; b=iFhlFyrHDxFL4nTNdTLYh5otspZ5GBuDsqIhjtvencJLohGDqenAuT96Si1zLvvyhmHkbGLJwfszaiLSeXYWVdIzE0pmd51E4uzQJxLd9Rxc0WLE51qyVvjUQanqk+VID0r0ItZQgfpcUqFVf+chOz6jpmw8EtHisDGUEou8pHQ= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1782204038; c=relaxed/simple; bh=XiQydRpXwl93K+PWXATZ3/FDrqkrMfjR/B392rLxffs=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=Lr8TlJy0drA6SKsvpW6HQdchBSv2wXJWDQJA39HjiqBriJe888q5nPN32Eevn1stHoW6CNY8B0PCQFGgbHmV5cRy1a4IrR8NiO1eopZ6p80UK12jQHMhVQThhANh5J/zTzZlthL+UijquqDEkEhOBcr1YCHKjSibhMT8JIZSFYY= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com; spf=pass smtp.mailfrom=arm.com; dkim=pass (1024-bit key) header.d=arm.com header.i=@arm.com header.b=AotjKw9x; arc=none smtp.client-ip=217.140.110.172 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=arm.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=arm.com header.i=@arm.com header.b="AotjKw9x" Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 53BB51A25; Tue, 23 Jun 2026 01:40:31 -0700 (PDT) Received: from [10.164.19.14] (unknown [10.164.19.14]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 71B8C3F632; Tue, 23 Jun 2026 01:40:28 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=arm.com; s=foss; t=1782204035; bh=XiQydRpXwl93K+PWXATZ3/FDrqkrMfjR/B392rLxffs=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=AotjKw9xPe0WVaX1GDgz+vOkiKuQ4wzcnl9b3lfkGoXVBWXxpPAxu3zLWAs3YvjLN 1X8SZUwFHc90/zc52SJ8oGEkXeGzN0dvIz2DAblavC4eCULY+YoxGOFWtgUxCzxwDP q4ggwyZiRsVCsI6Ds+xPPeFvFMBUUI5/OlVrS61Y= Message-ID: <6a85489f-8b26-4c4b-8333-38f8ae1b680a@arm.com> Date: Tue, 23 Jun 2026 14:10:25 +0530 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v4 02/12] mm/rmap: Add try_to_unmap_hugetlb_one To: "David Hildenbrand (Arm)" , akpm@linux-foundation.org, ljs@kernel.org, chrisl@kernel.org, kasong@tencent.com, hughd@google.com, liam@infradead.org Cc: riel@surriel.com, vbabka@kernel.org, harry@kernel.org, jannh@google.com, linux-mm@kvack.org, linux-kernel@vger.kernel.org, rppt@kernel.org, surenb@google.com, mhocko@suse.com, qi.zheng@linux.dev, shakeel.butt@linux.dev, baohua@kernel.org, axelrasmussen@google.com, yuanchu@google.com, weixugc@google.com, shikemeng@huaweicloud.com, nphamcs@gmail.com, bhe@redhat.com, youngjun.park@lge.com, baolin.wang@linux.alibaba.com, pfalcato@suse.de, ryan.roberts@arm.com, anshuman.khandual@arm.com References: <20260526063635.61721-1-dev.jain@arm.com> <20260526063635.61721-3-dev.jain@arm.com> Content-Language: en-US From: Dev Jain In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit On 09/06/26 7:46 pm, David Hildenbrand (Arm) wrote: > On 5/26/26 08:36, Dev Jain wrote: >> Simplify try_to_unmap_one by separating out the hugetlb parts into >> try_to_unmap_hugetlb_one. >> >> To understand the correctness of the refactoring, the following points >> are noted: >> >> 1. try_to_unmap() is called for hugetlb folios only when they are >> hwpoisoned. > > Ack > >> >> 2. A hugetlb VMA cannot be mlocked. > > Ack > >> >> 3. The pvmw API sets pvmw.pte to the base of the hugetlb folio (pvmw.pmd >> is NULL). > > Ack > >> >> 4. We won't ever process a softleaf entry that encodes a hugetlb folio; >> hugetlb folios are never swapped out, migration entries will be skipped >> (PVMW_MIGRATION not passed) and device-exclusive does not work for >> hugetlb. > > Ack, we should always find present entries. > >> >> 5. uffd-wp bit is lost when converting pvmw.pte to hwpoison softleaf >> (therefore no need to call pte_install_uffd_wp_if_needed after >> clearing pvmw.pte) > > Are you sure? And if so, can you elaborate why + document? If you see the current code, we construct pteval from the subpage, losing any bit state of the previous entry. Also it doesn't make sense anyways to keep uffd-wp for an hwpoisoned entry. > > >> >> 6. TTU_HWPOISON is always present; for it to not be present, either folio >> has to be in swapcache, or mapping_can_writeback() is true (see >> unmap_poisoned_folio), none of which is true for hugetlb folios. > > Agreed. That's one very confusing bit in the existing code. > >> >> 7. hugetlb uses separate counters from normal rss counters, therefore >> update_highwater_rss() need not be called. > > Ack. > >> >> While at it: >> >> - Change VM_BUG_* to VM_WARN_*. >> >> - Do not declare variables which are only used once >> >> - Assert that the subpage derived by the pvmw walk is exactly the head >> page. This is because try_to_unmap() does not remember the specific >> subpage which was hwpoisoned, and, since we cannot munmap/mremap >> across a hugetlb folio partially, the first pte mapping the hugetlb >> folio (in case of a contpte or contpmd mapped folio) cannot ever point >> to an intermediate page. > > Might want to add a Suggested-by:, so people know directly whom to blame :) Sure. > > I am very much in favor of decoupling both things cleanly, instead of hacking it > together and make the code all big and confusing. > >> >> Signed-off-by: Dev Jain >> --- >> mm/rmap.c | 203 ++++++++++++++++++++++++++++++++---------------------- >> 1 file changed, 121 insertions(+), 82 deletions(-) >> >> diff --git a/mm/rmap.c b/mm/rmap.c >> index 430c91c8fe2ae..06ab1158d4cd1 100644 >> --- a/mm/rmap.c >> +++ b/mm/rmap.c >> @@ -1978,6 +1978,121 @@ static inline unsigned int folio_unmap_pte_batch(struct folio *folio, >> FPB_RESPECT_WRITE | FPB_RESPECT_SOFT_DIRTY); >> } >> >> +static bool __try_to_unmap_hugetlb_one(struct folio *folio, >> + struct vm_area_struct *vma, struct page_vma_mapped_walk *pvmw, >> + struct mmu_notifier_range *range, enum ttu_flags flags) >> +{ >> + unsigned long hsz = huge_page_size(hstate_vma(vma)); > > Can be const. Ok. > >> + unsigned long address = pvmw->address; > > Can be const. But do we even need the temporary variable? It is being used 3 times in this function, so temporary variable is fine. > >> + struct mm_struct *mm = vma->vm_mm; >> + struct page *subpage; > > a) subpages do not exist. It's just a folio page. > b) subpages are irrelevant for hugetlb, just drop anything that deals with them. Ok. > >> + bool ret = true; >> + pte_t pteval; >> + > > Worth adding a comment here, why we don't need a loop. > > /* There is only a single mapping in a VMA. */ Ok. > >> + if (!page_vma_mapped_walk(pvmw)) >> + return true; >> + >> + pteval = ptep_get(pvmw->pte); > > Should we now properly use huge_ptep_get()? Sending a series to fix this soon : ) > >> + VM_WARN_ON(!pte_present(pteval)); >> + subpage = folio_page(folio, pte_pfn(pteval) - folio_pfn(folio)); > > No need for subpages. Ok. > >> + VM_WARN_ON(folio_page(folio, 0) != subpage); > > VM_WARN_ON(pte_pfn(pteval) != folio_pfn(folio)); > > > I assume both can be VM_WARN_ON_ONCE() Ok. > >> + >> + /* >> + * huge_pmd_unshare may unmap an entire PMD page. There is no way of >> + * knowing exactly which PMDs may be cached for this mm, so we must >> + * flush them all. start/end were already adjusted above to cover this >> + * range. >> + */ >> + flush_cache_range(vma, range->start, range->end); >> + >> + /* >> + * To call huge_pmd_unshare, i_mmap_rwsem must be held in write mode. >> + * Caller needs to explicitly do this outside rmap routines. >> + * >> + * We also must hold hugetlb vma_lock in write mode. Lock order dictates >> + * acquiring vma_lock BEFORE i_mmap_rwsem. We can only try lock here and >> + * fail if unsuccessful. >> + */ >> + if (!folio_test_anon(folio)) { >> + struct mmu_gather tlb; >> + >> + VM_WARN_ON(!(flags & TTU_RMAP_LOCKED)); >> + if (!hugetlb_vma_trylock_write(vma)) { >> + ret = false; >> + goto walk_done; >> + } >> + >> + tlb_gather_mmu_vma(&tlb, vma); >> + if (huge_pmd_unshare(&tlb, vma, address, pvmw->pte)) { >> + hugetlb_vma_unlock_write(vma); >> + huge_pmd_unshare_flush(&tlb, vma); >> + tlb_finish_mmu(&tlb); >> + /* >> + * The PMD table was unmapped, consequently unmapping >> + * the folio. >> + */ >> + goto walk_done; >> + } >> + hugetlb_vma_unlock_write(vma); >> + tlb_finish_mmu(&tlb); >> + } >> + pteval = huge_ptep_clear_flush(vma, address, pvmw->pte); >> + if (pte_dirty(pteval)) > > Should we now use huge_pte_dirty() for consistency? Ok. > >> + folio_mark_dirty(folio); >> + >> + VM_WARN_ON(!(flags & TTU_HWPOISON)); > > This should be checked right at the very beginning of try_to_unmap_hugetlb_one(). Ok. > >> + pteval = swp_entry_to_pte(make_hwpoison_entry(subpage)); >> + hugetlb_count_sub(folio_nr_pages(folio), mm); >> + set_huge_pte_at(mm, address, pvmw->pte, pteval, hsz); >> + hugetlb_remove_rmap(folio); >> + folio_put_refs(folio, 1); >> + >> +walk_done: >> + page_vma_mapped_walk_done(pvmw); >> + return ret; >> +} >> + >> +static bool try_to_unmap_hugetlb_one(struct folio *folio, >> + struct vm_area_struct *vma, unsigned long address, void *arg) >> +{ >> + DEFINE_FOLIO_VMA_WALK(pvmw, folio, vma, address, 0); >> + struct mmu_notifier_range range; >> + enum ttu_flags flags = (enum ttu_flags)(long)arg; >> + bool ret; >> + >> + /* >> + * The try_to_unmap() is only passed a hugetlb folio in the case >> + * where the hugetlb folio contains a poisoned page. >> + */ >> + VM_WARN_ON_FOLIO(!folio_contain_hwpoisoned_page(folio), folio); > > As discussed, I'd prefer to just have folio_test_hwpoison() here. Ok. > >> + >> + /* >> + * When racing against e.g. zap_pte_range() on another cpu, >> + * in between its ptep_get_and_clear_full() and folio_remove_rmap_*(), >> + * try_to_unmap() may return before folio_mapped() has become false, >> + * if page table locking is skipped: use TTU_SYNC to wait for that. >> + */ > > The comment should be updated to mention hugetlb primitives ... but > > ... it's a good question whether that is required *at all* ... > >> + if (flags & TTU_SYNC) >> + pvmw.flags = PVMW_SYNC; > > ... and in fact, looking at page_vma_mapped_walk(), PVMW_SYNC is irrelevant. You are right, I'll drop this. > >> + >> + /* >> + * For hugetlb, it could be much worse than THP if we need pud >> + * invalidation in the case of pmd sharing. > > I don't understand that comment. Is it really helpful? > >> + * >> + * Note that the folio can not be freed in this function as call of > > "cannot" ? > >> + * try_to_unmap() must hold a reference on the folio. >> + */ > > Do we really need this comment entirely? We don't even look at the folio after > unmapping it. Yeah I just blindly copied it, looks like historical baggage, I can't understand that comment at all. I'll drop it entirely. > >> + range.end = vma_address_end(&pvmw); >> + mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, vma->vm_mm, >> + address, range.end); >> + adjust_range_if_pmd_sharing_possible(vma, &range.start, &range.end); >> + mmu_notifier_invalidate_range_start(&range); >> + ret = __try_to_unmap_hugetlb_one(folio, vma, &pvmw, &range, >> + flags); > > Is it really worth it moving that stuff to a separate function? I derive pleasure in a code style of: foo(): lock/start __foo() unlock/end Although, I think this is entirely pointless here since foo() has barely anything apart from __foo(), and that __foo() has 5 function parameters. So I'll collapse into a single function. > > >> + mmu_notifier_invalidate_range_end(&range); >> + return ret; >> +} >> + > >