Re: [PATCH v4 02/12] mm/rmap: Add try_to_unmap_hugetlb_one

[Lance Yang]

On 2026/6/24 13:50, Dev Jain wrote:
> On 22/06/26 1:47 pm, David Hildenbrand (Arm) wrote:
> > On 6/22/26 10:14, Dev Jain wrote:
> > > On 22/06/26 1:43 pm, Dev Jain wrote:
> > > > On 18/06/26 3:31 pm, Lance Yang wrote:
> > > > > Yeah, looks like this was already there before the split. Should this
> > > > > be fixed separately?
> > > >
> > > > Same bug is there in try_to_migrate_one(), check_pte(), remove_migration_pte()
> > > > and prot_none_hugetlb_entry() :)
> > >
> > > Lemme send a series for them. Main thing is identifying the fixes tag really.
> >
> > Thanks!
>
> I am confused w.r.t backport. So currently we just have to do this:
>
> -               /*
> -                * Handle PFN swap PTEs, such as device-exclusive ones, that
> -                * actually map pages.
> -                */
> -               pteval = ptep_get(pvmw.pte);
> +               address = pvmw.address;
> +               if (folio_test_hugetlb(folio)) {
> +                       pteval = huge_ptep_get(mm, address, pvmw.pte);
> +               } else {
> +                       /*
> +                        * Handle PFN swap PTEs, such as device-exclusive ones,
> +                        * that actually map pages.
> +                        */
> +                       pteval = ptep_get(pvmw.pte);
> +               }
>
>
> At commit c7ab0d2fdc84, try_to_unmap_one() was converted to use the pvmw API. The
> code was:
>
> 	while (page_vma_mapped_walk(&pvmw)) {
> 		subpage = page - page_to_pfn(page) + pte_pfn(*pvmw.pte);
>
> Doing a plain dereference. Before this commit, the code was:
>
>
> pte_t *__page_check_address(struct page *page, struct mm_struct *mm,
> 			  unsigned long address, spinlock_t **ptlp, int sync)
> {
> 	pmd_t *pmd;
> 	pte_t *pte;
> 	spinlock_t *ptl;
>
> 	if (unlikely(PageHuge(page))) {
> 		/* when pud is not present, pte will be NULL */
> 		pte = huge_pte_offset(mm, address);
> 		if (!pte)
> 			return NULL;
>
> 		ptl = huge_pte_lockptr(page_hstate(page), mm, pte);
> 		goto check;
> 	}
>
> 	pmd = mm_find_pmd(mm, address);
> 	if (!pmd)
> 		return NULL;
>
> 	pte = pte_offset_map(pmd, address);
> 	/* Make a quick check before getting the lock */
> 	if (!sync && !pte_present(*pte)) {
> 		pte_unmap(pte);
> 		return NULL;
> 	}
>
> 	ptl = pte_lockptr(mm, pmd);
> check:
> 	spin_lock(ptl);
> 	if (pte_present(*pte) && page_to_pfn(page) == pte_pfn(*pte)) {
> 		*ptlp = ptl;
> 		return pte;
> 	}
> 	pte_unmap_unlock(pte, ptl);
> 	return NULL;
> }
>
>
> This does pte_pfn(*pte), a plain dereference. I am not sure how back I need
> to go for the backport and do I need to post multiple patches for different
> stable versions.

Yeah ... looks old old old, not just c7ab0d2fdc84.

Stable backport looks kinda messy though. Mainline can probably stay
as one clean series, but stable might needs separate review per tree.
Going to be a pain ...
> And, it is crazy that the bug has stayed for this long and testing didn't find this.
> Perhaps no one bothered running hugetlb-read-hwpoison on PPC or s390. And for some
> reason those tests are in a category of "destructive" in run_vmtests.sh, requiring
> a command line option - and even that option was fixed just now in 3432cbb291aa!

Sigh ...